Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
263583002:
Reland: Do not initialize v8::Context on |ScriptController::updateDocument|. (Closed)
|
Created:
6 years, 7 months ago by kouhei (in TOK) Modified:
6 years, 6 months ago CC:
blink-reviews, Nils Barth (inactive), arv+blink, jsbell+bindings_chromium.org, sof, kouhei+bindings_chromium.org, abarth-chromium, marja+watch_chromium.org, adamk+blink_chromium.org, Nate Chapin, Inactive Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionReland: Do not initialize |v8::Context| on |ScriptController::updateDocument|.
> The reason for reverting is: broke interactive_ui_tests:
>
> Referrer
> SearchReusesInstantTab
> TypedSearchURLDoesntReuseInstantTab.
I fixed these tests in https://src.chromium.org/viewvc/chrome?view=rev&revision=273996 .
Original Description:
Initializing |v8::Context| is a heavy operation, and should be avoided if necessary. This is meant to be delayed until first time |toV8Context| is called, but |ScriptController::updateDocument| was forcing creation.
This patch fixes the if branch so that it would not create a |v8::Context| if it does not exist.
BUG=368548, 368555
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173044
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175240
Patch Set 1 #
Total comments: 2
Patch Set 2 : fix_crash #
Total comments: 4
Patch Set 3 : simplify #
Messages
Total messages: 20 (0 generated)
haraken: Would you take a look? This patch does seem to fix the svg case ( crbug.com/368555 )
+dcarney https://codereview.chromium.org/263583002/diff/1/Source/bindings/v8/ScriptCon... File Source/bindings/v8/ScriptController.cpp (right): https://codereview.chromium.org/263583002/diff/1/Source/bindings/v8/ScriptCon... Source/bindings/v8/ScriptController.cpp:476: // For an uninitialized main window shell, do not incur the cost of context initialization during FrameLoader::init(). the comment is no longer correct
LGTM but Dan should take another look. > BUG=368458 This bug is not related.
On 2014/04/30 04:46:09, haraken wrote: > LGTM but Dan should take another look. > > > BUG=368458 > > This bug is not related. Fixed. https://codereview.chromium.org/263583002/diff/1/Source/bindings/v8/ScriptCon... File Source/bindings/v8/ScriptController.cpp (right): https://codereview.chromium.org/263583002/diff/1/Source/bindings/v8/ScriptCon... Source/bindings/v8/ScriptController.cpp:476: // For an uninitialized main window shell, do not incur the cost of context initialization during FrameLoader::init(). On 2014/04/30 04:37:18, jochen (OOO Wed-Thu) wrote: > the comment is no longer correct Done.
lgtm, but you should check if this can be simplified https://codereview.chromium.org/263583002/diff/20001/Source/bindings/v8/Scrip... File Source/bindings/v8/ScriptController.cpp (right): https://codereview.chromium.org/263583002/diff/20001/Source/bindings/v8/Scrip... Source/bindings/v8/ScriptController.cpp:477: if (!m_windowShell->isContextInitialized() || !m_windowShell->isGlobalInitialized() || m_frame->loader().stateMachine()->creatingInitialEmptyDocument()) is it possible that the third condition is enough here?
https://codereview.chromium.org/263583002/diff/20001/Source/bindings/v8/Scrip... File Source/bindings/v8/ScriptController.cpp (right): https://codereview.chromium.org/263583002/diff/20001/Source/bindings/v8/Scrip... Source/bindings/v8/ScriptController.cpp:477: if (!m_windowShell->isContextInitialized() || !m_windowShell->isGlobalInitialized() || m_frame->loader().stateMachine()->creatingInitialEmptyDocument()) On 2014/04/30 07:01:13, dcarney wrote: > is it possible that the third condition is enough here? No. In this patch, we want to avoid creating windowshell when loading html without Javascript. It is not necessary an InitialEmptyDocument(). For simplifying, we may be able to drop the third condition instead. !m_windowShell->isContextInitialized() should always hold when m_frame->loader().stateMachine()->creatingInitialEmptyDocument().
https://codereview.chromium.org/263583002/diff/20001/Source/bindings/v8/Scrip... File Source/bindings/v8/ScriptController.cpp (right): https://codereview.chromium.org/263583002/diff/20001/Source/bindings/v8/Scrip... Source/bindings/v8/ScriptController.cpp:477: if (!m_windowShell->isContextInitialized() || !m_windowShell->isGlobalInitialized() || m_frame->loader().stateMachine()->creatingInitialEmptyDocument()) On 2014/04/30 07:04:53, kouhei wrote: > On 2014/04/30 07:01:13, dcarney wrote: > > is it possible that the third condition is enough here? > No. In this patch, we want to avoid creating windowshell when loading html > without Javascript. It is not necessary an InitialEmptyDocument(). > > For simplifying, we may be able to drop the third condition instead. > !m_windowShell->isContextInitialized() should always hold when > m_frame->loader().stateMachine()->creatingInitialEmptyDocument(). Ok, I see what you mean. I remember needing it when I put it there, but I'm no longer sure why. I'm not sure anymore why the second one is there. It doesn't look right. Maybe the first condition is sufficient if the other two are removed?
https://codereview.chromium.org/263583002/diff/20001/Source/bindings/v8/Scrip... File Source/bindings/v8/ScriptController.cpp (right): https://codereview.chromium.org/263583002/diff/20001/Source/bindings/v8/Scrip... Source/bindings/v8/ScriptController.cpp:477: if (!m_windowShell->isContextInitialized() || !m_windowShell->isGlobalInitialized() || m_frame->loader().stateMachine()->creatingInitialEmptyDocument()) On 2014/04/30 07:11:49, dcarney wrote: > On 2014/04/30 07:04:53, kouhei wrote: > > On 2014/04/30 07:01:13, dcarney wrote: > > > is it possible that the third condition is enough here? > > No. In this patch, we want to avoid creating windowshell when loading html > > without Javascript. It is not necessary an InitialEmptyDocument(). > > > > For simplifying, we may be able to drop the third condition instead. > > !m_windowShell->isContextInitialized() should always hold when > > m_frame->loader().stateMachine()->creatingInitialEmptyDocument(). > > Ok, I see what you mean. I remember needing it when I put it there, but I'm no > longer sure why. I'm not sure anymore why the second one is there. It doesn't > look right. Maybe the first condition is sufficient if the other two are > removed? I think this should be 'if (!m_windowShell->isGlobalInitialized())". It is possible that global w/ document property is still there while context is lost.
On 2014/04/30 07:25:43, kouhei wrote: > https://codereview.chromium.org/263583002/diff/20001/Source/bindings/v8/Scrip... > File Source/bindings/v8/ScriptController.cpp (right): > > https://codereview.chromium.org/263583002/diff/20001/Source/bindings/v8/Scrip... > Source/bindings/v8/ScriptController.cpp:477: if > (!m_windowShell->isContextInitialized() || !m_windowShell->isGlobalInitialized() > || m_frame->loader().stateMachine()->creatingInitialEmptyDocument()) > On 2014/04/30 07:11:49, dcarney wrote: > > On 2014/04/30 07:04:53, kouhei wrote: > > > On 2014/04/30 07:01:13, dcarney wrote: > > > > is it possible that the third condition is enough here? > > > No. In this patch, we want to avoid creating windowshell when loading html > > > without Javascript. It is not necessary an InitialEmptyDocument(). > > > > > > For simplifying, we may be able to drop the third condition instead. > > > !m_windowShell->isContextInitialized() should always hold when > > > m_frame->loader().stateMachine()->creatingInitialEmptyDocument(). > > > > Ok, I see what you mean. I remember needing it when I put it there, but I'm > no > > longer sure why. I'm not sure anymore why the second one is there. It > doesn't > > look right. Maybe the first condition is sufficient if the other two are > > removed? > I think this should be 'if (!m_windowShell->isGlobalInitialized())". It is > possible that global w/ document property is still there while context is lost. Trybots seem happy. Will land.
The CQ bit was checked by kouhei@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kouhei@chromium.org/263583002/40001
Message was sent while issue was closed.
Change committed as 173044
Message was sent while issue was closed.
On 2014/05/01 00:39:38, I haz the power (commit-bot) wrote: > Change committed as 173044 It looks like this breaks some interactive_ui_tests: Referrer SearchReusesInstantTab TypedSearchURLDoesntReuseInstantTab http://build.chromium.org/p/chromium.webkit/builders/Linux%20Tests/builds/36390 I tried reverting this change locally, and the tests pass again.
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/268623003/ by kouhei@chromium.org. The reason for reverting is: broke interactive_ui_tests: Referrer SearchReusesInstantTab TypedSearchURLDoesntReuseInstantTab.
On 2014/05/01 04:22:01, kouhei wrote: > A revert of this CL has been created in > https://codereview.chromium.org/268623003/ by mailto:kouhei@chromium.org. > > The reason for reverting is: broke interactive_ui_tests: > > Referrer > SearchReusesInstantTab > TypedSearchURLDoesntReuseInstantTab. I think it is the interactive_ui_tests that is wrong. Proposed CL https://codereview.chromium.org/302433013
On 2014/05/28 05:10:10, kouhei wrote: > On 2014/05/01 04:22:01, kouhei wrote: > > A revert of this CL has been created in > > https://codereview.chromium.org/268623003/ by mailto:kouhei@chromium.org. > > > > The reason for reverting is: broke interactive_ui_tests: > > > > Referrer > > SearchReusesInstantTab > > TypedSearchURLDoesntReuseInstantTab. > > I think it is the interactive_ui_tests that is wrong. Proposed CL > https://codereview.chromium.org/302433013 CL landed. Relanding this CL.
The CQ bit was checked by kouhei@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kouhei@chromium.org/263583002/40001
Message was sent while issue was closed.
Change committed as 175240 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
