Remove ScriptController::updateDocument

Remove ScriptController::initializeMainWorld

ScriptController::updateDocument is unnecessarily complicated.

Assuming that ScriptController::updateDocument is never called after
the context is detached (I added a dcheck to verify the fact in WindowProxy::updateDocument()),
we can simplify ScriptController::updateDocument.

It also enables us to remove ScriptController::initializeMainWorld.

BUG=677253
Review-Url: https://codereview.chromium.org/2608163009
Cr-Commit-Position: refs/heads/master@{#441896}
Committed: https://chromium.googlesource.com/chromium/src/+/40458d4dd913d5fd6f4f1bb2da083a8a7136a9af

[haraken, 2017-01-05 11:18:25]

haraken@chromium.org changed reviewers:
+ yukishiino@chromium.org

[haraken, 2017-01-05 11:18:25]

PTAL

[Yuki, 2017-01-05 12:10:06]

https://codereview.chromium.org/2608163009/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp (right):

https://codereview.chromium.org/2608163009/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:308: }
Do we really need the below code?  If so, in what scenarios?

At this point,
a) we already initialized the Window once, and
b) the Window was detached for some reason.

As we talked offline before, I think the document must NOT get updated while the
Window is detached.

https://codereview.chromium.org/2608163009/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:309:
windowProxy(DOMWrapperWorld::mainWorld());
I know that |windowProxy(world)| initializes a WindowProxy for the given world
as needed, however, it's hard to understand from its name.  I'd like to have a
comment like below.

// |windowProxy(world)| guarantees that a WindowProxy for the given
// |world| is initialized.

It's a good idea to write a similar comment for ScriptController::windowProxy in
ScriptController.h.

https://codereview.chromium.org/2608163009/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:310: if
(!m_windowProxyManager->mainWorldProxy()->isContextInitialized()) {
You made WindowProxy::initialize{,IfNeeded} always succeed and return void.  Why
do we need this line?  The WindowProxy must be always initialized at this point.

https://codereview.chromium.org/2608163009/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2608163009/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1755: // Forcibly
instantiate WindowProxy.
This is a good comment!

Why don't we have similar comments in ScriptController.{h,cpp}?

[haraken, 2017-01-05 12:32:16]

Description was changed from

==========
Remove ScriptController::initializeMainWorld

There is no functional change in this CL.
In the next CL, I'll clean up the complicated condition in
ScriptController::updateDocument().

BUG=677253
==========

to

==========
Remove ScriptController::initializeMainWorld

ScriptController::updateDocument is unnecessarily complicated.

Assuming that ScriptController::updateDocument is never called after
the context is detached (I added a dcheck to verify the fact in
WindowProxy::updateDocument()),
we can simplify ScriptController::updateDocument.

It also enables us to remove ScriptController::initializeMainWorld.

BUG=677253
==========

[haraken, 2017-01-05 12:35:07]

On 2017/01/05 12:10:06, Yuki wrote:
>
https://codereview.chromium.org/2608163009/diff/1/third_party/WebKit/Source/b...
> File third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp (right):
> 
>
https://codereview.chromium.org/2608163009/diff/1/third_party/WebKit/Source/b...
> third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:308: }
> Do we really need the below code?  If so, in what scenarios?
> 
> At this point,
> a) we already initialized the Window once, and
> b) the Window was detached for some reason.
> 
> As we talked offline before, I think the document must NOT get updated while
the
> Window is detached.
> 
>
https://codereview.chromium.org/2608163009/diff/1/third_party/WebKit/Source/b...
> third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:309:
> windowProxy(DOMWrapperWorld::mainWorld());
> I know that |windowProxy(world)| initializes a WindowProxy for the given world
> as needed, however, it's hard to understand from its name.  I'd like to have a
> comment like below.
> 
> // |windowProxy(world)| guarantees that a WindowProxy for the given
> // |world| is initialized.
> 
> It's a good idea to write a similar comment for ScriptController::windowProxy
in
> ScriptController.h.
> 
>
https://codereview.chromium.org/2608163009/diff/1/third_party/WebKit/Source/b...
> third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:310: if
> (!m_windowProxyManager->mainWorldProxy()->isContextInitialized()) {
> You made WindowProxy::initialize{,IfNeeded} always succeed and return void. 
Why
> do we need this line?  The WindowProxy must be always initialized at this
point.

That's exactly what I was planning to change in follow-up CLs (to make each CL
obvious).

Applied all planned changes to PS3. PTAL.

[haraken, 2017-01-05 13:05:24]

(Updated the CL as discussed offline.)

[Yuki, 2017-01-05 13:21:26]

LGTM.

https://codereview.chromium.org/2608163009/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp (left):

https://codereview.chromium.org/2608163009/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:307: // context
initialization.
I think this comment was very good.

https://codereview.chromium.org/2608163009/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/2608163009/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:466:
DCHECK(!m_scriptState || isContextInitialized());
I think it's worth commenting that we can defer updating the document until when
the window proxy gets initialized because the WindowProxy initialization
includes updateDocument().  So we can return immediately if !m_scriptState (i.e.
!isGlobalInitialized()).

[dcheng, 2017-01-05 16:20:31]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2017-01-05 16:20:32]

https://codereview.chromium.org/2608163009/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp (left):

https://codereview.chromium.org/2608163009/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:308: if
(!m_windowProxyManager->mainWorldProxy()->isGlobalInitialized())
Note this is the only caller to isGlobalInitialized(). However, the last time I
tried to remove this, kouhei@ (who added this in
https://chromium.googlesource.com/chromium/src/+/246e25c5bd72fac9dce3b9b1254e...)
mentioned that this was a useful improvement for pages that never needed to use
JS: https://codereview.chromium.org/2392843002/#msg15

[haraken, 2017-01-05 23:46:19]

https://codereview.chromium.org/2608163009/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp (left):

https://codereview.chromium.org/2608163009/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:307: // context
initialization.
On 2017/01/05 13:21:26, Yuki wrote:
> I think this comment was very good.

Done.

https://codereview.chromium.org/2608163009/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/2608163009/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:466:
DCHECK(!m_scriptState || isContextInitialized());
On 2017/01/05 13:21:26, Yuki wrote:
> I think it's worth commenting that we can defer updating the document until
when
> the window proxy gets initialized because the WindowProxy initialization
> includes updateDocument().  So we can return immediately if !m_scriptState
(i.e.
> !isGlobalInitialized()).

Done.

[haraken, 2017-01-05 23:47:18]

On 2017/01/05 16:20:32, dcheng wrote:
>
https://codereview.chromium.org/2608163009/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp (left):
> 
>
https://codereview.chromium.org/2608163009/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:308: if
> (!m_windowProxyManager->mainWorldProxy()->isGlobalInitialized())
> Note this is the only caller to isGlobalInitialized(). However, the last time
I
> tried to remove this, kouhei@ (who added this in
>
https://chromium.googlesource.com/chromium/src/+/246e25c5bd72fac9dce3b9b1254e...)
> mentioned that this was a useful improvement for pages that never needed to
use
> JS: https://codereview.chromium.org/2392843002/#msg15

You're right. After this CL, the isGlobalInitialized() check is now covered by
if(!isContextInitialized) in WindowProxy::updateDocument(). Added a comment
about it.

[haraken, 2017-01-05 23:47:24]

The CQ bit was checked by haraken@chromium.org

[haraken, 2017-01-05 23:47:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2608163009/#ps80001
(title: "temp")

[commit-bot: I haz the power, 2017-01-05 23:47:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-05 23:51:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-05 23:51:10]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[haraken, 2017-01-05 23:57:23]

The CQ bit was checked by haraken@chromium.org

[haraken, 2017-01-05 23:57:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2608163009/#ps100001
(title: "temp")

[commit-bot: I haz the power, 2017-01-05 23:58:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 00:48:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-06 00:48:36]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[haraken, 2017-01-06 00:55:18]

I noticed that some browser tests are calling updateDocument for a detached
window proxy. We're hitting the dcheck:

https://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_lin...

At the moment, let me simply drop the dcheck and land the CL (to keep the
existing behavior). I'll try to add the dcheck in a follow-up CL.

[haraken, 2017-01-06 00:55:26]

The CQ bit was checked by haraken@chromium.org

[haraken, 2017-01-06 00:55:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2608163009/#ps120001
(title: "temp")

[commit-bot: I haz the power, 2017-01-06 00:56:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 02:46:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-06 02:46:09]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[haraken, 2017-01-06 02:46:37]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2017-01-06 02:47:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 03:25:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-06 03:25:50]

Failed to apply patch for
third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:
While running git apply --index -p1;
  error: patch failed:
third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:303
  error: third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp: patch
does not apply

Patch:       third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp
Index: third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp
diff --git a/third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp
b/third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp
index
b08a2e2228273a00556a9a45d694fd97b88f2d6c..e68e3188858e49beab901b55581707bfba387efa
100644
--- a/third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp
+++ b/third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp
@@ -209,12 +209,6 @@ void ScriptController::executeScriptInMainWorld(
                        InspectorUpdateCountersEvent::data());
 }
 
-bool ScriptController::initializeMainWorld() {
-  if (m_windowProxyManager->mainWorldProxy()->isContextInitialized())
-    return false;
-  return windowProxy(DOMWrapperWorld::mainWorld())->isContextInitialized();
-}
-
 WindowProxy* ScriptController::windowProxy(DOMWrapperWorld& world) {
   WindowProxy* windowProxy = m_windowProxyManager->windowProxy(world);
   if (!windowProxy->isContextInitialized()) {
@@ -303,13 +297,7 @@ void ScriptController::clearWindowProxy() {
 }
 
 void ScriptController::updateDocument() {
-  // For an uninitialized main window windowProxy, do not incur the cost of
-  // context initialization.
-  if (!m_windowProxyManager->mainWorldProxy()->isGlobalInitialized())
-    return;
-
-  if (!initializeMainWorld())
-    windowProxy(DOMWrapperWorld::mainWorld())->updateDocument();
+  m_windowProxyManager->mainWorldProxy()->updateDocument();
 }
 
 void ScriptController::namedItemAdded(HTMLDocument* doc,

[haraken, 2017-01-06 03:32:24]

The CQ bit was checked by haraken@chromium.org

[haraken, 2017-01-06 03:32:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2608163009/#ps140001
(title: "temp")

[commit-bot: I haz the power, 2017-01-06 03:32:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 05:01:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-06 05:01:28]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[haraken, 2017-01-06 05:02:05]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2017-01-06 05:02:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 06:34:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-06 06:34:12]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[haraken, 2017-01-06 06:36:20]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2017-01-06 06:36:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 07:09:48]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1483684580818790,
"parent_rev": "3898922f0e5f70f0cbd8a7edbc6d7001b0d60ec5", "commit_rev":
"40458d4dd913d5fd6f4f1bb2da083a8a7136a9af"}

[commit-bot: I haz the power, 2017-01-06 07:10:18]

Description was changed from

==========
Remove ScriptController::initializeMainWorld

ScriptController::updateDocument is unnecessarily complicated.

Assuming that ScriptController::updateDocument is never called after
the context is detached (I added a dcheck to verify the fact in
WindowProxy::updateDocument()),
we can simplify ScriptController::updateDocument.

It also enables us to remove ScriptController::initializeMainWorld.

BUG=677253
==========

to

==========
Remove ScriptController::initializeMainWorld

ScriptController::updateDocument is unnecessarily complicated.

Assuming that ScriptController::updateDocument is never called after
the context is detached (I added a dcheck to verify the fact in
WindowProxy::updateDocument()),
we can simplify ScriptController::updateDocument.

It also enables us to remove ScriptController::initializeMainWorld.

BUG=677253

Review-Url: https://codereview.chromium.org/2608163009
Cr-Commit-Position: refs/heads/master@{#441896}
Committed:
https://chromium.googlesource.com/chromium/src/+/40458d4dd913d5fd6f4f1bb2da08...
==========

[commit-bot: I haz the power, 2017-01-06 07:10:19]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/40458d4dd913d5fd6f4f1bb2da08...