Remove an isContextInitialized() check from ScriptController::windowProxy()

Remove an isContextInitialized() check from ScriptController::windowProxy()

WindowProxy::initializeIfNeeded() is no-op when isContextInitialized() returns true.
So we can remove the check from ScriptController::windowProxy() and unconditionally
call WindowProxy::initializeIfNeeded().

This CL will cause a subtle change: Before this CL, dispatchDidClearWindowObjectInMainWorld
was not called if WindowProxy gets initialized via WindowProxy::setGlobal. After this CL,
dispatchDidClearWindowObjectInMainWorld will be called always when WindowProxy gets initialized.
I don't think the difference matters (and I guess the new behavior will be more correct).

BUG=677253
Review-Url: https://codereview.chromium.org/2613893002
Cr-Commit-Position: refs/heads/master@{#441908}
Committed: https://chromium.googlesource.com/chromium/src/+/559b67fbc41fba740506cd5d6f3d20d20f65039b

[haraken, 2017-01-05 09:48:07]

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

[haraken, 2017-01-05 09:48:08]

PTAL

[Yuki, 2017-01-05 10:19:50]

To me, it doesn't read correct to call dispatchDidClearWindowObjectInMainWorld
when the WindowProxy was not initialized (i.e. the WindowProxy had no window
instance) because there should be no window object to be cleared.

However, as far as I read the actual implementation of
dispatchDidClearWindowObjectInMainWorld, this function actually does a kind of
initialization for a new window and/or document instead of clearing the old
window.  The name looks strange.

Should we rename this function to dispatchDidInitialize(or
SetUp?)WindowAndDocumentInMainWorld (in a separate CL)?  Then, this CL looks
correct, hence LGTM.

[haraken, 2017-01-05 10:22:43]

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

[haraken, 2017-01-05 10:22:44]

On 2017/01/05 10:19:50, Yuki wrote:
> To me, it doesn't read correct to call dispatchDidClearWindowObjectInMainWorld
> when the WindowProxy was not initialized (i.e. the WindowProxy had no window
> instance) because there should be no window object to be cleared.
> 
> However, as far as I read the actual implementation of
> dispatchDidClearWindowObjectInMainWorld, this function actually does a kind of
> initialization for a new window and/or document instead of clearing the old
> window.  The name looks strange.
> 
> Should we rename this function to dispatchDidInitialize(or
> SetUp?)WindowAndDocumentInMainWorld (in a separate CL)?  Then, this CL looks
> correct, hence LGTM.

Yeah, agreed. I thought dcheng@ was planning to rename & clean up those methods
(a couple of years ago IIUC).

[haraken, 2017-01-05 10:22:47]

The CQ bit was checked by haraken@chromium.org

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

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 13:41:36]

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

[commit-bot: I haz the power, 2017-01-05 13:41:37]

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

[dcheng, 2017-01-06 02:41:44]

On 2017/01/05 10:22:44, haraken wrote:
> On 2017/01/05 10:19:50, Yuki wrote:
> > To me, it doesn't read correct to call
dispatchDidClearWindowObjectInMainWorld
> > when the WindowProxy was not initialized (i.e. the WindowProxy had no window
> > instance) because there should be no window object to be cleared.
> > 
> > However, as far as I read the actual implementation of
> > dispatchDidClearWindowObjectInMainWorld, this function actually does a kind
of
> > initialization for a new window and/or document instead of clearing the old
> > window.  The name looks strange.
> > 
> > Should we rename this function to dispatchDidInitialize(or
> > SetUp?)WindowAndDocumentInMainWorld (in a separate CL)?  Then, this CL looks
> > correct, hence LGTM.
> 
> Yeah, agreed. I thought dcheng@ was planning to rename & clean up those
methods
> (a couple of years ago IIUC).

Yes, there's a long-term desire to clean this up--we have a lot of one-off hooks
for various extension things...
It hasn't caused (enough) problems yet, but one day, I'm hoping to clean this up
=)

I agree that the behavior change for the callback should be more correct as
well, so LGTM.

[haraken, 2017-01-06 07:53:44]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2017-01-06 07:53:57]

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 09:20:45]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1483689224427160,
"parent_rev": "e5b0bedb6f6b053faaf5b2df7f06be8408a1ed67", "commit_rev":
"559b67fbc41fba740506cd5d6f3d20d20f65039b"}

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

Description was changed from

==========
Remove an isContextInitialized() check from ScriptController::windowProxy()

WindowProxy::initializeIfNeeded() is no-op when isContextInitialized() returns
true.
So we can remove the check from ScriptController::windowProxy() and
unconditionally
call WindowProxy::initializeIfNeeded().

This CL will cause a subtle change: Before this CL,
dispatchDidClearWindowObjectInMainWorld
was not called if WindowProxy gets initialized via WindowProxy::setGlobal. After
this CL,
dispatchDidClearWindowObjectInMainWorld will be called always when WindowProxy
gets initialized.
I don't think the difference matters (and I guess the new behavior will be more
correct).

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

to

==========
Remove an isContextInitialized() check from ScriptController::windowProxy()

WindowProxy::initializeIfNeeded() is no-op when isContextInitialized() returns
true.
So we can remove the check from ScriptController::windowProxy() and
unconditionally
call WindowProxy::initializeIfNeeded().

This CL will cause a subtle change: Before this CL,
dispatchDidClearWindowObjectInMainWorld
was not called if WindowProxy gets initialized via WindowProxy::setGlobal. After
this CL,
dispatchDidClearWindowObjectInMainWorld will be called always when WindowProxy
gets initialized.
I don't think the difference matters (and I guess the new behavior will be more
correct).

BUG=677253

Review-Url: https://codereview.chromium.org/2613893002
Cr-Commit-Position: refs/heads/master@{#441908}
Committed:
https://chromium.googlesource.com/chromium/src/+/559b67fbc41fba740506cd5d6f3d...
==========

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

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/559b67fbc41fba740506cd5d6f3d...