Bug 22469

Summary: Rename DOMWindowTimer into DOMTimer and move it into separate file
Product: WebKit Reporter: Dmitry Titov <dimich>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Severity: Normal CC: ap, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Description Flags
proposed patch
darin: review+
patch with changes after review
More changes. eric: review+

Description Dmitry Titov 2008-11-24 14:56:52 PST
This is a preparation step for adding timers to Workers. The plan is to eventually move the setTimeout and friends from JSDOMWindowBase to ScriptExecutionContext, make timers thread-aware and at the end to have timers in Workers same way as they are on window object.

The root bug is https://bugs.webkit.org/show_bug.cgi?id=22328
Per suggestion from ap@, I'm doing it in small steps. Here is the first step - to move DOMWindowTimer into its own file and rename it DOMTimer.

Subsequently, I'll make DOMTimer to be thread-safe and be able to work on a Worker thread.
Comment 1 Dmitry Titov 2008-11-24 15:16:55 PST
Created attachment 25454 [details]
proposed patch
Comment 2 Sam Weinig 2008-11-24 15:29:42 PST
Moving the code seems fine, but I think it should stay in bindings/js as the code is still very JS specific.  If we want to abstract DOMTimers and the setTimeout/setInterval methods enough that they can be used with other bindings (ObjC/COM), then we should move the class then.
Comment 3 Darin Adler 2008-11-24 15:29:43 PST
Comment on attachment 25454 [details]
proposed patch

> +				<FileConfiguration
> +					Name="Release_PGO|Win32"
> +					>
> +					<Tool
> +						Name="VCCLCompilerTool"
> +						WholeProgramOptimization="true"
> +					/>
> +				</FileConfiguration>

I don't think we need this in the vcproj, do we?

> +static int lastUsedTimeoutId = 0;

This can go inside the DOMTimer constructor instead of outside the function. It would be nice to scope it a little more tightly.

> +#include "ActiveDOMObject.h"

Why are you including this header in DOMTimer.h?

> +class DOMTimer : public TimerBase {
> +public:
> +    // Creates a new timer with the next id and nesting level.
> +    DOMTimer(JSDOMWindowBase* object, ScheduledAction* action);

Normally we'd omit the names "object" and "action" here, because the types make it clear what the arguments are without names.

> +    // Creates a timer from PausedTimeout, takes timeoutId and nestingLevel as they were persisted.
> +    DOMTimer(int timeoutId, int nestingLevel, JSDOMWindowBase* object, ScheduledAction* action);

Same here.

Comment 4 Dmitry Titov 2008-11-24 16:49:52 PST
Created attachment 25460 [details]
patch with changes after review

Made changes recommended by Darin and moved new files to bindings/js per Sam's suggestion.
Comment 5 Dmitry Titov 2008-11-25 10:51:29 PST
Created attachment 25487 [details]
More changes.

removed tabs from ChangeLog, seems good.
Comment 6 Eric Seidel (no email) 2008-11-25 15:34:07 PST
Comment on attachment 25487 [details]
More changes.

Still looks good.
Comment 7 Alexey Proskuryakov 2008-11-26 01:10:37 PST
Committed revision 38780.