[webkit-reviews] review granted: [Bug 45362] Reduce minimum DOMTimer interval : [Attachment 67856] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Matthew Delaney
<mdelaney at apple.com>'s request for review:
Bug 45362: Reduce minimum DOMTimer interval
https://bugs.webkit.org/show_bug.cgi?id=45362

Attachment 67856: Patch
https://bugs.webkit.org/attachment.cgi?id=67856&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 67677)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,21 @@
> +2010-09-16  Matthew Delaney	<mdelaney at apple.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Reduce minimum DOMTimer interval
> +	   https://bugs.webkit.org/show_bug.cgi?id=45362

You need a description here of what the problem is, and what you changed to fix
it.

> +	   No new tests. (OOPS!)

Remove this, and justify why you couldn't add any LayoutTests.

> +	   * WebCore.exp.in:
> +	   * WebCore.xcodeproj/project.pbxproj:

The project changes are bogus.

> Index: WebCore/WebCore.xcodeproj/project.pbxproj
> ===================================================================
> --- WebCore/WebCore.xcodeproj/project.pbxproj (revision 67626)
> +++ WebCore/WebCore.xcodeproj/project.pbxproj (working copy)
> @@ -2384,8 +2384,8 @@
>		893C47B81238A099002B3D86 /* JSFileCallback.h in Headers */ =
{isa = PBXBuildFile; fileRef = 893C47B61238A099002B3D86 /* JSFileCallback.h */;
};
>		893C47BB1238A0A9002B3D86 /* JSFileWriterCallback.cpp in Sources
*/ = {isa = PBXBuildFile; fileRef = 893C47B91238A0A9002B3D86 /*
JSFileWriterCallback.cpp */; };
>		893C47BC1238A0A9002B3D86 /* JSFileWriterCallback.h in Headers
*/ = {isa = PBXBuildFile; fileRef = 893C47BA1238A0A9002B3D86 /*
JSFileWriterCallback.h */; };
> -		893C47DF123EF4A9002B3D86 /* JSDirectoryEntryCustom.cpp in
Sources */ = {isa = PBXBuildFile; fileRef = 893C47DE123EF4A9002B3D86 /*
JSDirectoryEntryCustom.cpp */; };
>		893C47CC123EEBA2002B3D86 /* JSEntryCustom.cpp in Sources */ =
{isa = PBXBuildFile; fileRef = 893C47CA123EEBA2002B3D86 /* JSEntryCustom.cpp
*/; };
> +		893C47DF123EF4A9002B3D86 /* JSDirectoryEntryCustom.cpp in
Sources */ = {isa = PBXBuildFile; fileRef = 893C47DE123EF4A9002B3D86 /*
JSDirectoryEntryCustom.cpp */; };
>		89878552122CA064003AABDA /* DirectoryEntry.cpp in Sources */ =
{isa = PBXBuildFile; fileRef = 89878539122CA064003AABDA /* DirectoryEntry.cpp
*/; };
>		89878553122CA064003AABDA /* DirectoryEntry.h in Headers */ =
{isa = PBXBuildFile; fileRef = 8987853A122CA064003AABDA /* DirectoryEntry.h */;
};
>		89878554122CA064003AABDA /* DirectoryReader.cpp in Sources */ =
{isa = PBXBuildFile; fileRef = 8987853B122CA064003AABDA /* DirectoryReader.cpp
*/; };
> @@ -8290,8 +8290,8 @@
>		893C47B61238A099002B3D86 /* JSFileCallback.h */ = {isa =
PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path =
JSFileCallback.h; sourceTree = "<group>"; };
>		893C47B91238A0A9002B3D86 /* JSFileWriterCallback.cpp */ = {isa
= PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp;
path = JSFileWriterCallback.cpp; sourceTree = "<group>"; };
>		893C47BA1238A0A9002B3D86 /* JSFileWriterCallback.h */ = {isa =
PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path =
JSFileWriterCallback.h; sourceTree = "<group>"; };
> -		893C47DE123EF4A9002B3D86 /* JSDirectoryEntryCustom.cpp */ =
{isa = PBXFileReference; fileEncoding = 4; lastKnownFileType =
sourcecode.cpp.cpp; path = JSDirectoryEntryCustom.cpp; sourceTree = "<group>";
};
>		893C47CA123EEBA2002B3D86 /* JSEntryCustom.cpp */ = {isa =
PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp;
path = JSEntryCustom.cpp; sourceTree = "<group>"; };
> +		893C47DE123EF4A9002B3D86 /* JSDirectoryEntryCustom.cpp */ =
{isa = PBXFileReference; fileEncoding = 4; lastKnownFileType =
sourcecode.cpp.cpp; path = JSDirectoryEntryCustom.cpp; sourceTree = "<group>";
};
>		89878539122CA064003AABDA /* DirectoryEntry.cpp */ = {isa =
PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp;
name = DirectoryEntry.cpp; path = fileapi/DirectoryEntry.cpp; sourceTree =
"<group>"; };
>		8987853A122CA064003AABDA /* DirectoryEntry.h */ = {isa =
PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name =
DirectoryEntry.h; path = fileapi/DirectoryEntry.h; sourceTree = "<group>"; };
>		8987853B122CA064003AABDA /* DirectoryReader.cpp */ = {isa =
PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp;
name = DirectoryReader.cpp; path = fileapi/DirectoryReader.cpp; sourceTree =
"<group>"; };

These changes look unrelated.

> Index: WebCore/page/Settings.h
> ===================================================================
> --- WebCore/page/Settings.h	(revision 67626)
> +++ WebCore/page/Settings.h	(working copy)
> @@ -200,6 +200,8 @@ namespace WebCore {
>	   void setDOMPasteAllowed(bool);
>	   bool isDOMPasteAllowed() const { return m_isDOMPasteAllowed; }
>	   
> +	   static void setDOMTimerMinInterval(double);

I think setMinDOMTimerInterval() would be a better name. Also, it doesn't have
to be static. You should add a comment saying what the units are for the
paramter (seconds, milliseconds?).

> Index: WebKit2/ChangeLog
> ===================================================================

> +	   Reduce minimum DOMTimer interval
> +	   https://bugs.webkit.org/show_bug.cgi?id=45362
> +
> +	   * WebProcess/WebProcess.cpp: Keeping webkit2 in sync with webkit to
set the dom timer's min interval down to 4ms.

You don't need the "keeping in sync" comment. Please capitalize your sentences
correctly: DOM, WebKit2.

> Index: WebKit2/WebProcess/WebProcess.cpp
> ===================================================================
> --- WebKit2/WebProcess/WebProcess.cpp (revision 67626)
> +++ WebKit2/WebProcess/WebProcess.cpp (working copy)
> @@ -98,6 +98,7 @@ WebProcess::WebProcess()
>      // Initialize our platform strategies.
>      WebPlatformStrategies::initialize();
>  #endif // USE(PLATFORM_STRATEGIES)
> +    Settings::setDOMTimerMinInterval(0.004);

I wonder if the 0.004 should be declared somewhere central?

> Index: WebKit/win/ChangeLog
> ===================================================================
> --- WebKit/win/ChangeLog	(revision 67677)
> +++ WebKit/win/ChangeLog	(working copy)
> @@ -1,3 +1,13 @@
> +2010-09-16  Matthew Delaney	<mdelaney at apple.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Reduce minimum DOMTimer interval
> +	   https://bugs.webkit.org/show_bug.cgi?id=45362
> +
> +	   * WebView.cpp: 
> +	   (WebView::initWithFrame): Having init with frame set the dom timer
min interval down to 4ms.

Please improve the description of what you added.