Summary: | Rename DOMWindowTimer into DOMTimer and move it into separate file | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dmitry Titov <dimich> | ||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ap, sam | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Mac | ||||||||||
OS: | OS X 10.5 | ||||||||||
Attachments: |
|
Description
Dmitry Titov
2008-11-24 14:56:52 PST
Created attachment 25454 [details]
proposed patch
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 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 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.
Created attachment 25487 [details]
More changes.
removed tabs from ChangeLog, seems good.
Comment on attachment 25487 [details]
More changes.
Still looks good.
Committed revision 38780. |