Move asserts for tracking global object detachment to WindowProxy.

Move asserts for tracking global object detachment to WindowProxy.

RemoteWindowProxy won't have a ScriptState in the future, so move the
DCHECKs so it's possible to continue asserting invariants that should
always hold true in WindowProxy.

BUG=527190
Review-Url: https://codereview.chromium.org/2744483002
Cr-Commit-Position: refs/heads/master@{#455715}
Committed: https://chromium.googlesource.com/chromium/src/+/a1236121a61abad307064fa8a23d6556b03ec3e2

[dcheng, 2017-03-09 00:40:38]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 00:41:17]

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

[dcheng, 2017-03-09 00:42:15]

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

[commit-bot: I haz the power, 2017-03-09 01:20:25]

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

[commit-bot: I haz the power, 2017-03-09 01:20:26]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dcheng, 2017-03-09 01:24:03]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 01:24:49]

Dry run: 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-03-09 02:15:27]

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

[commit-bot: I haz the power, 2017-03-09 02:15:28]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dcheng, 2017-03-09 02:16:32]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 02:17:03]

Dry run: 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-03-09 04:01:26]

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

[commit-bot: I haz the power, 2017-03-09 04:01:26]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dcheng, 2017-03-09 04:01:48]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 04:02:08]

Dry run: 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-03-09 05:04:00]

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

[commit-bot: I haz the power, 2017-03-09 05:04:01]

Dry run: This issue passed the CQ dry run.

[Yuki, 2017-03-09 07:15:17]

The change itself LGTM, but I'm not comfortable with the name.

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

https://codereview.chromium.org/2744483002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp:97:
didDetachGlobalProxy();
nit: Is this name appropriate?

The old implementation was tracking that a global object gets detached from a
global proxy.  didDetachGlobalProxy sounds like we're detaching a global proxy
rather than global object.  It looks weird to me.

[dcheng, 2017-03-09 07:19:57]

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

https://codereview.chromium.org/2744483002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp:97:
didDetachGlobalProxy();
On 2017/03/09 07:15:17, Yuki(slow_til_mar08) wrote:
> nit: Is this name appropriate?
> 
> The old implementation was tracking that a global object gets detached from a
> global proxy.  didDetachGlobalProxy sounds like we're detaching a global proxy
> rather than global object.  It looks weird to me.

Hmm... so what we want to track here is that the fields on the global proxy were
cleared. If you have other suggestions for what to call this, I'm open to ideas.

[Yuki, 2017-03-09 07:26:31]

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

https://codereview.chromium.org/2744483002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp:97:
didDetachGlobalProxy();
On 2017/03/09 07:19:56, dcheng wrote:
> On 2017/03/09 07:15:17, Yuki(slow_til_mar08) wrote:
> > nit: Is this name appropriate?
> > 
> > The old implementation was tracking that a global object gets detached from
a
> > global proxy.  didDetachGlobalProxy sounds like we're detaching a global
proxy
> > rather than global object.  It looks weird to me.
> 
> Hmm... so what we want to track here is that the fields on the global proxy
were
> cleared. If you have other suggestions for what to call this, I'm open to
ideas.

A global object of V8 must be tightly bound to a DOMWindow instance of Blink, so
it should be equivalent to track the detachment of a global object.

But if we want to emphasize the internal fields, we can call it
didDetachDOMWindow(Instance), didDetachGlobalAndDOMWindow or something like
that.

Anyway, I'm thinking we're detaching something from a global proxy, not a global
proxy from something, because we keep the global proxy as m_globalProxy.

[dcheng, 2017-03-09 08:00:50]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[dcheng, 2017-03-09 08:00:52]

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

https://codereview.chromium.org/2744483002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp:97:
didDetachGlobalProxy();
On 2017/03/09 07:26:31, Yuki(slow_til_mar08) wrote:
> On 2017/03/09 07:19:56, dcheng wrote:
> > On 2017/03/09 07:15:17, Yuki(slow_til_mar08) wrote:
> > > nit: Is this name appropriate?
> > > 
> > > The old implementation was tracking that a global object gets detached
from
> a
> > > global proxy.  didDetachGlobalProxy sounds like we're detaching a global
> proxy
> > > rather than global object.  It looks weird to me.
> > 
> > Hmm... so what we want to track here is that the fields on the global proxy
> were
> > cleared. If you have other suggestions for what to call this, I'm open to
> ideas.
> 
> A global object of V8 must be tightly bound to a DOMWindow instance of Blink,
so
> it should be equivalent to track the detachment of a global object.

Remote contexts have an inner global object, so that receiver lookup when using
v8::Signature works. However, remote contexts don't (currently) have an
equivalent concept of global object detachment.

> 
> But if we want to emphasize the internal fields, we can call it
> didDetachDOMWindow(Instance), didDetachGlobalAndDOMWindow or something like
> that.
> 
> Anyway, I'm thinking we're detaching something from a global proxy, not a
global
> proxy from something, because we keep the global proxy as m_globalProxy.

OK, makes sense. Renamed it to didDetachGlobalObject(). I'll figure out a way to
make the semantics work for a remote context.

[commit-bot: I haz the power, 2017-03-09 08:01:05]

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

[Yuki, 2017-03-09 08:08:38]

LGTM.

[haraken, 2017-03-09 08:08:50]

LGTM

[dcheng, 2017-03-09 08:10:50]

Description was changed from

==========
Move asserts for tracking global proxy initialization to WindowProxy.

RemoteWindowProxy won't have a ScriptState in the future, so move the
DCHECKs so it's possible to continue asserting invariants that should
always hold true in WindowProxy.

BUG=527190
==========

to

==========
Move asserts for tracking global object detachment to WindowProxy.

RemoteWindowProxy won't have a ScriptState in the future, so move the
DCHECKs so it's possible to continue asserting invariants that should
always hold true in WindowProxy.

BUG=527190
==========

[dcheng, 2017-03-09 08:10:56]

The CQ bit was unchecked by dcheng@chromium.org

[dcheng, 2017-03-09 08:10:56]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2017-03-09 08:11: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-03-09 11:21:51]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1489047056835240,
"parent_rev": "3f16ca95660bbe3f7294db7e3049bdfb39644928", "commit_rev":
"a1236121a61abad307064fa8a23d6556b03ec3e2"}

[commit-bot: I haz the power, 2017-03-09 11:22:28]

Description was changed from

==========
Move asserts for tracking global object detachment to WindowProxy.

RemoteWindowProxy won't have a ScriptState in the future, so move the
DCHECKs so it's possible to continue asserting invariants that should
always hold true in WindowProxy.

BUG=527190
==========

to

==========
Move asserts for tracking global object detachment to WindowProxy.

RemoteWindowProxy won't have a ScriptState in the future, so move the
DCHECKs so it's possible to continue asserting invariants that should
always hold true in WindowProxy.

BUG=527190

Review-Url: https://codereview.chromium.org/2744483002
Cr-Commit-Position: refs/heads/master@{#455715}
Committed:
https://chromium.googlesource.com/chromium/src/+/a1236121a61abad307064fa8a23d...
==========

[commit-bot: I haz the power, 2017-03-09 11:22:30]

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