Summary: | [V8] V8GCForContextDispose should indicate whether a context is disposed for the main frame or not | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ulan Degenbaev <ulan> | ||||||||||||||
Component: | WebCore JavaScript | Assignee: | Ulan Degenbaev <ulan> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | abarth, dglazkov, erikcorry, haraken, japhet, mstarzinger, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Ulan Degenbaev
2012-03-15 02:17:34 PDT
Created attachment 132269 [details] patch This depends on this V8 CL https://chromiumcodereview.appspot.com/9701093/ and should be landed after V8 with the CL rolls into Chromium. Change looks good to me. We are rolling the CL that this patch depends on to Chromium: https://chromiumcodereview.appspot.com/9701093/ Created attachment 133020 [details]
Rebase and add a check for m_frame->page()
The corresponding V8 CL is in Chromium and is sticking: http://src.chromium.org/viewvc/chrome?view=rev&revision=127940 So this patch can now be committed. Adam, Kentaro, Nate: could someone of you review and commit the patch? Michael from the V8 team reviewed the part where we call V8::IdleNotification(). Comment on attachment 133020 [details] Rebase and add a check for m_frame->page() View in context: https://bugs.webkit.org/attachment.cgi?id=133020&action=review LGTM with a style nit. > Source/WebCore/bindings/v8/V8GCForContextDispose.cpp:72 > + const int kLongIdlePauseInMs = 1000; > + const int kShortIdlePauseInMs = 10; We don't typically prefix constants with k. So just longIdlePauseInMS and shortIdlePauseInMs. Created attachment 133053 [details]
Rename constants.
Comment on attachment 133053 [details] Rename constants. Attachment 133053 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12035555 Created attachment 133061 [details]
fix compile error
Created attachment 133066 [details]
Rename constants.
Comment on attachment 133066 [details] Rename constants. View in context: https://bugs.webkit.org/attachment.cgi?id=133066&action=review > Source/WebCore/bindings/v8/V8GCForContextDispose.cpp:40 > + : m_pseudoIdleTimer(this, &V8GCForContextDispose::pseudoIdleTimerFired), > + m_contextDisposedForMainFrame(false) The comma goes on line 40, just below the : > Source/WebCore/bindings/v8/V8GCForContextDispose.cpp:46 > + m_contextDisposedForMainFrame = m_contextDisposedForMainFrame || isMainFrame; Sorry to nit-pick you names, but consider a name like m_didDisposeContextForMainFrame. This is an Objective-C style name, which we often use in WebKit because of the strong Mac influence on the project. Created attachment 133070 [details]
rename m_contextDisposedForMainFrame and fix comma placement
Thanks for reviewing!
Comment on attachment 133070 [details]
rename m_contextDisposedForMainFrame and fix comma placement
Thanks.
Comment on attachment 133070 [details] rename m_contextDisposedForMainFrame and fix comma placement Clearing flags on attachment: 133070 Committed r111586: <http://trac.webkit.org/changeset/111586> All reviewed patches have been landed. Closing bug. |