|
|
Descriptionv8binding: Initializes WindowProxy iff it's uninitialized.
Separates WindowProxy::Lifecycle::ContextDetached into
two: GlobalObjectIsDetached and FrameIsDetached,
because they have difference about the context
(re)initialization. The detailed explanation is written
as a comment at WindowProxy::Lifecycle in WindowProxy.h.
In short,
* GlobalObjectIsDetached is expected to navigate to a new
page, thus re-initialization may happen.
* FrameIsDetached must not navigate to a new page, thus
re-initialization must not happen.
We'd like to (re)initialize the context if and only if
ContextIsUninitialized or GlobalObjectIsDetached.
BUG=
Review-Url: https://codereview.chromium.org/2769803003
Cr-Commit-Position: refs/heads/master@{#461086}
Committed: https://chromium.googlesource.com/chromium/src/+/8c9d8f928bcb9796ea67ad39c58baf5e760f1729
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #Patch Set 4 : . #
Total comments: 24
Patch Set 5 : Addressed review comments. #Patch Set 6 : Synced. #
Total comments: 13
Patch Set 7 : Simply reordered Lifecycle's items. #Patch Set 8 : Changed DCHECK conditions in namedItem{Added,Removed}. #Patch Set 9 : Addressed review comments. #Patch Set 10 : Synced. #
Total comments: 4
Patch Set 11 : Addressed review comments. #
Messages
Total messages: 46 (35 generated)
The CQ bit was checked by yukishiino@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 yukishiino@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by yukishiino@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 checked by yukishiino@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...
Description was changed from ========== v8binding: Initializes WindowProxy iff it's uninitialized. BUG= ========== to ========== v8binding: Initializes WindowProxy iff it's uninitialized. Separates WindowProxy::Lifecycle::ContextDetached into two: ContextDetachedFromFrame and GlobalObjectDetached, because they have difference about the context (re)initialization. The detailed explanation is written as a comment at WindowProxy::Lifecycle in WindowProxy.h. In short, * ContextDetachedFromFrame must not navigate to a new page, thus re-initialization must not happen. * GlobalObjectDetached is expected to navigate to a new page, thus re-initialization may happen. We'd like to (re)initialize the context if and only if ContextUninitialized or GlobalObjectDetached. BUG= ==========
yukishiino@chromium.org changed reviewers: + dcheng@chromium.org, haraken@chromium.org
Could you guys review this CL? By the way, this is part of effort to land a WIP CL: https://crrev.com/2693893007/ I'm still aiming to land this CL (again).
https://codereview.chromium.org/2769803003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp (right): https://codereview.chromium.org/2769803003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp:393: m_lifecycle == Lifecycle::ContextDetachedFromFrame); I might want to change this to CHECK. Are you pretty sure that we don't hit this path with m_lifecycle being Lifecycle::GlobalObjectDetached? https://codereview.chromium.org/2769803003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp:408: m_lifecycle == Lifecycle::ContextDetachedFromFrame); Ditto. https://codereview.chromium.org/2769803003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp:426: if (m_lifecycle == Lifecycle::GlobalObjectDetached) Why can't this be: DCHECK(m_lifecycle == Lifecycle::ContextInitialized || m_lifecycle == Lifecycle::ContextDetachedFromFrame); ? https://codereview.chromium.org/2769803003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/WindowProxy.h (right): https://codereview.chromium.org/2769803003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:178: // way for author script to access to the context of |ContextUninitialized| access the context https://codereview.chromium.org/2769803003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:180: // to the context, hence there must exist no reference to the context. accessed the context https://codereview.chromium.org/2769803003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:183: // The context is initialized and yet attached to a frame. yet => still https://codereview.chromium.org/2769803003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:191: // a detached window. Blink does not support more things than the spec, // Blink supports less things than the spec. https://codereview.chromium.org/2769803003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:193: // - Possible next states: n/a Isn't it possible to transit from ContextDetachedFromFrame to GlobalObjectDetached? n/a => GlobalObjectDetached or destructor ? https://codereview.chromium.org/2769803003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:199: // author script may have reference to the context. references https://codereview.chromium.org/2769803003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:201: // |ContextDetachedFromFrame|. Blink does not support this state well samely. // The spec does not support full web features in this state. Blink supports less things than the spec. https://codereview.chromium.org/2769803003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:202: // - Possible next states: ContextUninitialized ContextUninitialized or destructor ? https://codereview.chromium.org/2769803003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:204: // |ContextUninitialized| for a new window/context/document of a new page. This comment looks a bit confusing. It's possible that the WindowProxy gets destructed without becoming ContextUninitialized, right?
The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run
https://codereview.chromium.org/2769803003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp (right): https://codereview.chromium.org/2769803003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp:393: m_lifecycle == Lifecycle::ContextDetachedFromFrame); On 2017/03/28 11:12:42, haraken wrote: > > I might want to change this to CHECK. Hmm, IIUC, the guideline suggests DCHECK for detecting programming errors, and we're using DCHECK for all other places about m_lifecycle. > Are you pretty sure that we don't hit this path with m_lifecycle being > Lifecycle::GlobalObjectDetached? Yes. If the state is GlobalObjectDetached, then windowProxy(world)->namedItemAdded(...); triggers the re-initialization of WindowProxy. After that, the state must be ContextInitialized. https://codereview.chromium.org/2769803003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp:408: m_lifecycle == Lifecycle::ContextDetachedFromFrame); On 2017/03/28 11:12:42, haraken wrote: > > Ditto. Ditto. https://codereview.chromium.org/2769803003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp:426: if (m_lifecycle == Lifecycle::GlobalObjectDetached) On 2017/03/28 11:12:42, haraken wrote: > > Why can't this be: > > DCHECK(m_lifecycle == Lifecycle::ContextInitialized || m_lifecycle == > Lifecycle::ContextDetachedFromFrame); > > ? It seems that I confused you. In case of GlobalObjectDetached, the WindowProxy is expected to be re-initialized as same as ContextUninitialized. I've updated this part of code as well as the comment at WindowProxy::Lifecycle in WindowProxy.h. In short, the state after a navigation remains being GlobalObjectDetached until it gets re-initialized on demand, mostly the same as ContextUninitialized. So it's possible to be GlobalObjectDetached here. https://codereview.chromium.org/2769803003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/WindowProxy.h (right): https://codereview.chromium.org/2769803003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:178: // way for author script to access to the context of |ContextUninitialized| On 2017/03/28 11:12:42, haraken wrote: > > access the context Done. https://codereview.chromium.org/2769803003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:180: // to the context, hence there must exist no reference to the context. On 2017/03/28 11:12:42, haraken wrote: > > accessed the context Done. https://codereview.chromium.org/2769803003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:183: // The context is initialized and yet attached to a frame. On 2017/03/28 11:12:42, haraken wrote: > > yet => still Done. https://codereview.chromium.org/2769803003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:191: // a detached window. Blink does not support more things than the spec, On 2017/03/28 11:12:42, haraken wrote: > > // Blink supports less things than the spec. Done. https://codereview.chromium.org/2769803003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:193: // - Possible next states: n/a On 2017/03/28 11:12:42, haraken wrote: > > Isn't it possible to transit from ContextDetachedFromFrame to > GlobalObjectDetached? IIUC, no way. GlobalObjectDetached means that it's navigating, but it's not possible to navigate without attached to a frame. > n/a => GlobalObjectDetached or destructor ? Hmm, I personally don't think that "destructor" is a state. Lifecycle doesn't have such value. "n/a", "nothing" or "no state"? https://codereview.chromium.org/2769803003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:199: // author script may have reference to the context. On 2017/03/28 11:12:42, haraken wrote: > > references Done. https://codereview.chromium.org/2769803003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:201: // |ContextDetachedFromFrame|. Blink does not support this state well samely. On 2017/03/28 11:12:42, haraken wrote: > > // The spec does not support full web features in this state. Blink supports > less things than the spec. Done. https://codereview.chromium.org/2769803003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:202: // - Possible next states: ContextUninitialized On 2017/03/28 11:12:42, haraken wrote: > > ContextUninitialized or destructor ? Ditto. https://codereview.chromium.org/2769803003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:204: // |ContextUninitialized| for a new window/context/document of a new page. On 2017/03/28 11:12:42, haraken wrote: > > This comment looks a bit confusing. It's possible that the WindowProxy gets > destructed without becoming ContextUninitialized, right? Oops, I was wrong on this point. GlobalObjectDetached transitions directly to ContextInitialized without being ContextUninitialized. This is *Possible* next states, so the transition is not mandatory. This is true for all other states and transitions. They are optional. Anyway, I rewrote this part.
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by yukishiino@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.
https://codereview.chromium.org/2769803003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp (left): https://codereview.chromium.org/2769803003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp:396: if (m_lifecycle == Lifecycle::ContextDetached) Is this just reverting us to the original behavior here? https://codereview.chromium.org/2769803003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/WindowProxy.h (right): https://codereview.chromium.org/2769803003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:187: // The context is initialized, once attached to a frame and now detached. Note Hmm... maybe be more explicit that it's the frame that's detached, not just the context itself? "The context was initialized, but its frame has been detached from the DOM." https://codereview.chromium.org/2769803003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:199: // author script may have references to the context. May be worth noting that this state is also used when swapping frames. https://codereview.chromium.org/2769803003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:204: // context gets accessed, the WindowProxy is lazily re-initialized for a new It might be more accurate to say that the context will always be reinitialized in this state, as installNewDocument() (almost) unconditionally calls ScriptController::updateDocument(). https://codereview.chromium.org/2769803003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:212: ContextDetachedFromFrame, Nit: I think it would be more accurate to call this "ContextForDetachedFrame", since the context isn't detached from the frame--it's the frame that's detached from the DOM. Another nit: from an ordering perspective, it might make slightly more sense to order this after GlobalObjectDetached, since this is a final state.
LGTM https://codereview.chromium.org/2769803003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/WindowProxy.h (right): https://codereview.chromium.org/2769803003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:212: ContextDetachedFromFrame, On 2017/03/28 21:25:42, dcheng wrote: > Nit: I think it would be more accurate to call this "ContextForDetachedFrame", > since the context isn't detached from the frame--it's the frame that's detached > from the DOM. > > Another nit: from an ordering perspective, it might make slightly more sense to > order this after GlobalObjectDetached, since this is a final state. ContextIsNotInitialized, ContextIsInitialized, GlobalObjectIsDetached, ContextIsDetached ?
The CQ bit was checked by yukishiino@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 CQ bit was checked by yukishiino@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...
Description was changed from ========== v8binding: Initializes WindowProxy iff it's uninitialized. Separates WindowProxy::Lifecycle::ContextDetached into two: ContextDetachedFromFrame and GlobalObjectDetached, because they have difference about the context (re)initialization. The detailed explanation is written as a comment at WindowProxy::Lifecycle in WindowProxy.h. In short, * ContextDetachedFromFrame must not navigate to a new page, thus re-initialization must not happen. * GlobalObjectDetached is expected to navigate to a new page, thus re-initialization may happen. We'd like to (re)initialize the context if and only if ContextUninitialized or GlobalObjectDetached. BUG= ========== to ========== v8binding: Initializes WindowProxy iff it's uninitialized. Separates WindowProxy::Lifecycle::ContextDetached into two: GlobalObjectIsDetached and FrameIsDetached, because they have difference about the context (re)initialization. The detailed explanation is written as a comment at WindowProxy::Lifecycle in WindowProxy.h. In short, * GlobalObjectIsDetached is expected to navigate to a new page, thus re-initialization may happen. * FrameIsDetached must not navigate to a new page, thus re-initialization must not happen. We'd like to (re)initialize the context if and only if ContextIsUninitialized or GlobalObjectIsDetached. BUG= ==========
Could you guys take another look? https://codereview.chromium.org/2769803003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp (left): https://codereview.chromium.org/2769803003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp:396: if (m_lifecycle == Lifecycle::ContextDetached) On 2017/03/28 21:25:42, dcheng wrote: > Is this just reverting us to the original behavior here? Good catch. I considered this point again and again, and now I think that: a) Blink's current implementation reaches here only when ContextIsInitialized. b) However, we should support FrameIsDetached case, too, as the spec is saying so. Updated the code and comments. I'll leave the if-clause in this CL, but will remove it in a follow-up CL. https://codereview.chromium.org/2769803003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/WindowProxy.h (right): https://codereview.chromium.org/2769803003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:187: // The context is initialized, once attached to a frame and now detached. Note On 2017/03/28 21:25:42, dcheng wrote: > Hmm... maybe be more explicit that it's the frame that's detached, not just the > context itself? > > "The context was initialized, but its frame has been detached from the DOM." Done. https://codereview.chromium.org/2769803003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:199: // author script may have references to the context. On 2017/03/28 21:25:42, dcheng wrote: > May be worth noting that this state is also used when swapping frames. Good to know. When swapping frames, currently we don't change m_lifecycle, so I don't write it in this section of the comment at this moment. We might want to introduce another state as SwappingGlobalObject for example in future, supposing that it's good to distinguish swapping frames from disposeContext(). https://codereview.chromium.org/2769803003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:204: // context gets accessed, the WindowProxy is lazily re-initialized for a new On 2017/03/28 21:25:42, dcheng wrote: > It might be more accurate to say that the context will always be reinitialized > in this state, as installNewDocument() (almost) unconditionally calls > ScriptController::updateDocument(). Done. https://codereview.chromium.org/2769803003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:212: ContextDetachedFromFrame, On 2017/03/29 08:54:53, haraken wrote: > On 2017/03/28 21:25:42, dcheng wrote: > > Nit: I think it would be more accurate to call this "ContextForDetachedFrame", > > since the context isn't detached from the frame--it's the frame that's > detached > > from the DOM. > > > > Another nit: from an ordering perspective, it might make slightly more sense > to > > order this after GlobalObjectDetached, since this is a final state. > > ContextIsNotInitialized, ContextIsInitialized, GlobalObjectIsDetached, > ContextIsDetached ? Good points. Updated to: ContextIsNotInitialized ContextIsInitialized GlobalObjectIsDetached FrameIsDetached WDYT?
On 2017/03/30 14:48:38, Yuki wrote: > Could you guys take another look? > > https://codereview.chromium.org/2769803003/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp (left): > > https://codereview.chromium.org/2769803003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp:396: if > (m_lifecycle == Lifecycle::ContextDetached) > On 2017/03/28 21:25:42, dcheng wrote: > > Is this just reverting us to the original behavior here? > > Good catch. I considered this point again and again, and now I think that: > a) Blink's current implementation reaches here only when ContextIsInitialized. > b) However, we should support FrameIsDetached case, too, as the spec is saying > so. > > Updated the code and comments. > > I'll leave the if-clause in this CL, but will remove it in a follow-up CL. > > https://codereview.chromium.org/2769803003/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/bindings/core/v8/WindowProxy.h (right): > > https://codereview.chromium.org/2769803003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:187: // The context is > initialized, once attached to a frame and now detached. Note > On 2017/03/28 21:25:42, dcheng wrote: > > Hmm... maybe be more explicit that it's the frame that's detached, not just > the > > context itself? > > > > "The context was initialized, but its frame has been detached from the DOM." > > Done. > > https://codereview.chromium.org/2769803003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:199: // author script > may have references to the context. > On 2017/03/28 21:25:42, dcheng wrote: > > May be worth noting that this state is also used when swapping frames. > > Good to know. > > When swapping frames, currently we don't change m_lifecycle, so I don't write it > in this section of the comment at this moment. > > We might want to introduce another state as SwappingGlobalObject for example in > future, supposing that it's good to distinguish swapping frames from > disposeContext(). > > https://codereview.chromium.org/2769803003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:204: // context gets > accessed, the WindowProxy is lazily re-initialized for a new > On 2017/03/28 21:25:42, dcheng wrote: > > It might be more accurate to say that the context will always be reinitialized > > in this state, as installNewDocument() (almost) unconditionally calls > > ScriptController::updateDocument(). > > Done. > > https://codereview.chromium.org/2769803003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:212: > ContextDetachedFromFrame, > On 2017/03/29 08:54:53, haraken wrote: > > On 2017/03/28 21:25:42, dcheng wrote: > > > Nit: I think it would be more accurate to call this > "ContextForDetachedFrame", > > > since the context isn't detached from the frame--it's the frame that's > > detached > > > from the DOM. > > > > > > Another nit: from an ordering perspective, it might make slightly more sense > > to > > > order this after GlobalObjectDetached, since this is a final state. > > > > ContextIsNotInitialized, ContextIsInitialized, GlobalObjectIsDetached, > > ContextIsDetached ? > > Good points. Updated to: > > ContextIsNotInitialized > ContextIsInitialized > GlobalObjectIsDetached > FrameIsDetached > > WDYT? LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with nits https://codereview.chromium.org/2769803003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/WindowProxy.h (right): https://codereview.chromium.org/2769803003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:199: // author script may have references to the context. On 2017/03/30 14:48:37, Yuki wrote: > On 2017/03/28 21:25:42, dcheng wrote: > > May be worth noting that this state is also used when swapping frames. > > Good to know. > > When swapping frames, currently we don't change m_lifecycle, so I don't write it > in this section of the comment at this moment. > > We might want to introduce another state as SwappingGlobalObject for example in > future, supposing that it's good to distinguish swapping frames from > disposeContext(). We actually do =) See https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebFrame.c... https://codereview.chromium.org/2769803003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right): https://codereview.chromium.org/2769803003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:144: m_lifecycle == Lifecycle::GlobalObjectIsDetached) { I wonder how often we actually take advantage of the GlobalObjectIsDetached path. In theory, I think that LocalDOMWindow::installNewDocument() will always reinitialize, so maybe we can just DCHECK(m_lifecycle != Lifecycle::GlobalObjectIsDetached). Maybe something to investigate in a followup. https://codereview.chromium.org/2769803003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/WindowProxy.h (right): https://codereview.chromium.org/2769803003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:189: // the page is now navigated away. The global object (inner global) is Maybe something like this would be slightly clearer: The context is initialized and its frame is still attached to the DOM, but the inner global's Document is no longer the active Document of the frame (i.e. it is being navigated away).
https://codereview.chromium.org/2769803003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/WindowProxy.h (right): https://codereview.chromium.org/2769803003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:199: // author script may have references to the context. On 2017/03/30 21:03:01, dcheng wrote: > On 2017/03/30 14:48:37, Yuki wrote: > > On 2017/03/28 21:25:42, dcheng wrote: > > > May be worth noting that this state is also used when swapping frames. > > > > Good to know. > > > > When swapping frames, currently we don't change m_lifecycle, so I don't write > it > > in this section of the comment at this moment. > > > > We might want to introduce another state as SwappingGlobalObject for example > in > > future, supposing that it's good to distinguish swapping frames from > > disposeContext(). > > We actually do =) > See > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebFrame.c... Oops, thank you for pointing it out. I've added a line: "This state is also used when swapping frames. See also |WebFrame::swap|." https://codereview.chromium.org/2769803003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right): https://codereview.chromium.org/2769803003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:144: m_lifecycle == Lifecycle::GlobalObjectIsDetached) { On 2017/03/30 21:03:01, dcheng wrote: > I wonder how often we actually take advantage of the GlobalObjectIsDetached > path. In theory, I think that LocalDOMWindow::installNewDocument() will always > reinitialize, so maybe we can just DCHECK(m_lifecycle != > Lifecycle::GlobalObjectIsDetached). > > Maybe something to investigate in a followup. Yes, I'll address this point separately. As I quickly checked, when swapping frames, the following happens. WebFrame::swap() { // This made m_lifecycle = GlobalObjectIsDetached. windowProxyManager->clearForNavigation(); windowProxyManager->releaseGlobals(); windowProxyManager->setGlobals(); } WindowProxy::setGlobal(global) { m_globalProxy.set(global); initializeIfNeeded(); // [1] Reinitialization HERE! } It seems we're entering to initializeIfNeeded with m_lifecycle = GlobalObjectIsDetached. If you guys agree, I will make it: WindowProxy::setGlobal(global) { m_globalProxy.set(global); DCHECK_EQ(m_lifecycle, GloablObjectIsDetached); initialize(); } then, we may be able to remove GlobalObjectIsDetached case from initializeIfNeeded(). https://codereview.chromium.org/2769803003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/WindowProxy.h (right): https://codereview.chromium.org/2769803003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:189: // the page is now navigated away. The global object (inner global) is On 2017/03/30 21:03:01, dcheng wrote: > Maybe something like this would be slightly clearer: > > The context is initialized and its frame is still attached to the DOM, > but the inner global's Document is no longer the active Document of the > frame (i.e. it is being navigated away). Done.
The CQ bit was checked by yukishiino@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2769803003/#ps200001 (title: "Addressed review comments.")
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": 200001, "attempt_start_ts": 1490945551282980, "parent_rev": "42692e1c75507ae5a4f9f55c357c5fda2b0a89ab", "commit_rev": "8c9d8f928bcb9796ea67ad39c58baf5e760f1729"}
Message was sent while issue was closed.
Description was changed from ========== v8binding: Initializes WindowProxy iff it's uninitialized. Separates WindowProxy::Lifecycle::ContextDetached into two: GlobalObjectIsDetached and FrameIsDetached, because they have difference about the context (re)initialization. The detailed explanation is written as a comment at WindowProxy::Lifecycle in WindowProxy.h. In short, * GlobalObjectIsDetached is expected to navigate to a new page, thus re-initialization may happen. * FrameIsDetached must not navigate to a new page, thus re-initialization must not happen. We'd like to (re)initialize the context if and only if ContextIsUninitialized or GlobalObjectIsDetached. BUG= ========== to ========== v8binding: Initializes WindowProxy iff it's uninitialized. Separates WindowProxy::Lifecycle::ContextDetached into two: GlobalObjectIsDetached and FrameIsDetached, because they have difference about the context (re)initialization. The detailed explanation is written as a comment at WindowProxy::Lifecycle in WindowProxy.h. In short, * GlobalObjectIsDetached is expected to navigate to a new page, thus re-initialization may happen. * FrameIsDetached must not navigate to a new page, thus re-initialization must not happen. We'd like to (re)initialize the context if and only if ContextIsUninitialized or GlobalObjectIsDetached. BUG= Review-Url: https://codereview.chromium.org/2769803003 Cr-Commit-Position: refs/heads/master@{#461086} Committed: https://chromium.googlesource.com/chromium/src/+/8c9d8f928bcb9796ea67ad39c58b... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/8c9d8f928bcb9796ea67ad39c58b... |