[V8] Remove ScriptController::windowShell()
Created attachment 170370 [details] Patch
Comment on attachment 170370 [details] Patch Attachment 170370 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14545216 New failing tests: fast/frames/frame-limit.html
Comment on attachment 170370 [details] Patch Attachment 170370 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14553208 New failing tests: fast/frames/frame-limit.html
Comment on attachment 170370 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170370&action=review This looks great! I'm not sure what's going on with the test failure, but you might want to investigate that before landing. > Source/WebCore/bindings/v8/DOMWrapperWorld.cpp:70 > + for (WorldMap::iterator i = isolatedWorlds.begin(), e = isolatedWorlds.end(); i != e; ++i) style nit: we usually use "it" rather than "i" for iterators. Also, there's no reason to store "e" in a variable. You can just call end() each time in the loop condition. > Source/WebCore/bindings/v8/ScriptController.cpp:492 > + v8::Handle<v8::Context> v8Context = m_windowShell->context(); > v8Context->AllowCodeGenerationFromStrings(true); I would combine these lines. > Source/WebCore/bindings/v8/ScriptController.h:176 > // Dummy method to avoid a bunch of ifdef's in WebCore. Is this comment still accurate?
Created attachment 170898 [details] Patch
Adam, the comments you've made have been addressed. The failing test was timing out because ScriptController::updateDocument() was causing unnecessary calls to v8::Context::New (2X the number needed), which it apparently a very slow process. To fix this I added the below clause: void ScriptController::updateDocument() { if (!m_frame->loader()->frameHasLoaded()) return; I really have no particular idea if this is the correct solution here or if there is a more appropriate check, but this solution at least passes all the tests in debug.
Comment on attachment 170898 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170898&action=review > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3421 > + if (frame->script()->initializeMainWorld()) { There's no "IfNeeded" here? > Source/WebCore/bindings/v8/ScriptController.cpp:328 > +static DOMWrapperWorld* existingWindowShellWorkaroundWorld() Sometimes we use the name "depreciated" in functions we don't want more people to call, but this is fine too. > Source/WebCore/bindings/v8/ScriptController.cpp:720 > + if (!m_frame->loader()->frameHasLoaded()) > + return; What is this?
> Adam, the comments you've made have been addressed. The failing test was timing out because ScriptController::updateDocument() was causing unnecessary calls to v8::Context::New (2X the number needed), which it apparently a very slow process. Uh... Yes! We don't want to call v8::Context::New any extra times. It's not that it is slow. We don't wan to to be making extra context objects! > To fix this I added the below clause: > > void ScriptController::updateDocument() > { > if (!m_frame->loader()->frameHasLoaded()) > return; > > I really have no particular idea if this is the correct solution here or if there is a more appropriate check, but this solution at least passes all the tests in debug. This is very unlikely to be the right thing. I don't really understand the issue. Why were we creating more contexts than we should have? The way this worked before is that we'd always try to initialize things, and that would bail out early if the context was already initialized. It seems odd that we'd end up calling v8::Context::New multiple times.
Created attachment 171254 [details] Patch
(In reply to comment #7) > (From update of attachment 170898 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=170898&action=review > > > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3421 > > + if (frame->script()->initializeMainWorld()) { > > There's no "IfNeeded" here? > > > Source/WebCore/bindings/v8/ScriptController.cpp:328 This is a new method. It returns true if the world wasn't initialized and initialization was successful. The semantics are slightly different that the method on V8DOMWindowShell, so I wasn't sure what to call it. It is exactly equivalent to: windowShell.context().IsEmpty() && windowShell. initializeIfNeeded()
> This is very unlikely to be the right thing. I don't really understand the issue. Why were we creating more contexts than we should have? The way this worked before is that we'd always try to initialize things, and that would bail out early if the context was already initialized. It seems odd that we'd end up calling v8::Context::New multiple times. The reason this has changes is that I have moved V8DOMWindowShell initialization into ScriptController. In fact almost all functions in V8DOMWindowShell are now only called by ScriptController function. The previous implementation managed to not init the window shell during FrameLoader::init(), which is when a frame is initially attached but instead on frame access, which is what we want. An empty frame that is never used (as in the failing test), does not need to init the context. I have implemented that same workaround in this time around in ScriptController::updateDocument, albeit with slightly different logic, since I do in ScriptController rather than in the window shell.
Created attachment 173219 [details] Patch
Comment on attachment 173219 [details] Patch rebased
Comment on attachment 173219 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173219&action=review > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3410 > - if (frame->script()->windowShell()->context().IsEmpty() && frame->script()->windowShell()->initializeIfNeeded()) { > - // initializeIfNeeded may have created a wrapper for the object, retry from the start. > + if (frame->script()->initializeMainWorld()) { > + // initializeMainWorld may have created a wrapper for the object, retry from the start. You need to run-bindings-tests --reset-results to avoid turning the bot red. > Source/WebCore/bindings/v8/ScriptController.cpp:348 > - return iter == m_isolatedWorlds.end() ? 0 : iter->value; > + return iter == m_isolatedWorlds.end() ? 0 : iter->value->isContextInitialized() ? iter->value : 0; I would probably have broken this down into multiple statements. Nested ternary operators can be slightly hard to read. > Source/WebCore/bindings/v8/ScriptController.cpp:369 > + if (!shell->isContextInitialized()) { > + if (shell->initializeIfNeeded()) { These lines can be combined. Do we really need to call isContextInitialized? I would think that initializeIfNeeded would check isContextInitialized because it says "if needed". > Source/WebCore/bindings/v8/ScriptController.cpp:727 > + if ((!m_windowShell->isContextInitialized() || !m_windowShell->isGlobalInitialized()) && m_frame->loader()->stateMachine()->creatingInitialEmptyDocument()) This line still seems strange, but I think we should worry about it later.
Created attachment 173333 [details] Patch for landing
To save you the timezone round-trip, I applied these trivial comments for you.
Comment on attachment 173333 [details] Patch for landing Clearing flags on attachment: 173333 Committed r134089: <http://trac.webkit.org/changeset/134089>
All reviewed patches have been landed. Closing bug.