|
|
DescriptionMove 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
Patch Set 1 #
Total comments: 4
Patch Set 2 : GlobalProxy -> GlobalObject #
Messages
Total messages: 33 (24 generated)
The CQ bit was checked by dcheng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dcheng@chromium.org changed reviewers: + haraken@chromium.org, yukishiino@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by dcheng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by dcheng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by dcheng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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.
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.
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.
The CQ bit was checked by dcheng@chromium.org to run a CQ dry run
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.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM.
LGTM
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 ==========
The CQ bit was unchecked by dcheng@chromium.org
The CQ bit was checked by dcheng@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 20001, "attempt_start_ts": 1489047056835240, "parent_rev": "3f16ca95660bbe3f7294db7e3049bdfb39644928", "commit_rev": "a1236121a61abad307064fa8a23d6556b03ec3e2"}
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/a1236121a61abad307064fa8a23d... |