Bug 22469 - Rename DOMWindowTimer into DOMTimer and move it into separate file
Summary: Rename DOMWindowTimer into DOMTimer and move it into separate file
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-11-24 14:56 PST by Dmitry Titov
Modified: 2008-11-26 01:10 PST (History)
2 users (show)

See Also:


Attachments
proposed patch (18.68 KB, patch)
2008-11-24 15:16 PST, Dmitry Titov
darin: review+
Details | Formatted Diff | Diff
patch with changes after review (18.21 KB, patch)
2008-11-24 16:49 PST, Dmitry Titov
no flags Details | Formatted Diff | Diff
More changes. (18.23 KB, patch)
2008-11-25 10:51 PST, Dmitry Titov
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.

r=me
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.