Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(267)

Issue 2769803003: v8binding: Initializes WindowProxy iff it's uninitialized. (Closed)

Created:
3 years, 9 months ago by Yuki
Modified:
3 years, 8 months ago
Reviewers:
haraken, dcheng
CC:
chromium-reviews, blink-reviews, blink-reviews-bindings_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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/+/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. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+98 lines, -57 lines) Patch
M third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp View 1 2 3 4 5 6 7 8 7 chunks +30 lines, -33 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp View 1 2 3 4 5 6 7 8 3 chunks +7 lines, -7 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/WindowProxy.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +55 lines, -10 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp View 1 2 3 4 5 6 7 8 5 chunks +6 lines, -7 lines 0 comments Download

Messages

Total messages: 46 (35 generated)
Yuki
Could you guys review this CL? By the way, this is part of effort to ...
3 years, 8 months ago (2017-03-28 10:06:38 UTC) #15
haraken
https://codereview.chromium.org/2769803003/diff/60001/third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp File third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp (right): https://codereview.chromium.org/2769803003/diff/60001/third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp#newcode393 third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp:393: m_lifecycle == Lifecycle::ContextDetachedFromFrame); I might want to change this ...
3 years, 8 months ago (2017-03-28 11:12:42 UTC) #16
Yuki
https://codereview.chromium.org/2769803003/diff/60001/third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp File third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp (right): https://codereview.chromium.org/2769803003/diff/60001/third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp#newcode393 third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp:393: m_lifecycle == Lifecycle::ContextDetachedFromFrame); On 2017/03/28 11:12:42, haraken wrote: > ...
3 years, 8 months ago (2017-03-28 12:22:17 UTC) #18
dcheng
https://codereview.chromium.org/2769803003/diff/100001/third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp File third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp (left): https://codereview.chromium.org/2769803003/diff/100001/third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp#oldcode396 third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp:396: if (m_lifecycle == Lifecycle::ContextDetached) Is this just reverting us ...
3 years, 8 months ago (2017-03-28 21:25:43 UTC) #26
haraken
LGTM https://codereview.chromium.org/2769803003/diff/100001/third_party/WebKit/Source/bindings/core/v8/WindowProxy.h File third_party/WebKit/Source/bindings/core/v8/WindowProxy.h (right): https://codereview.chromium.org/2769803003/diff/100001/third_party/WebKit/Source/bindings/core/v8/WindowProxy.h#newcode212 third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:212: ContextDetachedFromFrame, On 2017/03/28 21:25:42, dcheng wrote: > Nit: ...
3 years, 8 months ago (2017-03-29 08:54:53 UTC) #27
Yuki
Could you guys take another look? https://codereview.chromium.org/2769803003/diff/100001/third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp File third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp (left): https://codereview.chromium.org/2769803003/diff/100001/third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp#oldcode396 third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp:396: if (m_lifecycle == ...
3 years, 8 months ago (2017-03-30 14:48:38 UTC) #35
haraken
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/Source/bindings/core/v8/LocalWindowProxy.cpp ...
3 years, 8 months ago (2017-03-30 14:58:28 UTC) #36
dcheng
LGTM with nits https://codereview.chromium.org/2769803003/diff/100001/third_party/WebKit/Source/bindings/core/v8/WindowProxy.h File third_party/WebKit/Source/bindings/core/v8/WindowProxy.h (right): https://codereview.chromium.org/2769803003/diff/100001/third_party/WebKit/Source/bindings/core/v8/WindowProxy.h#newcode199 third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:199: // author script may have references ...
3 years, 8 months ago (2017-03-30 21:03:01 UTC) #39
Yuki
https://codereview.chromium.org/2769803003/diff/100001/third_party/WebKit/Source/bindings/core/v8/WindowProxy.h File third_party/WebKit/Source/bindings/core/v8/WindowProxy.h (right): https://codereview.chromium.org/2769803003/diff/100001/third_party/WebKit/Source/bindings/core/v8/WindowProxy.h#newcode199 third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:199: // author script may have references to the context. ...
3 years, 8 months ago (2017-03-31 07:32:09 UTC) #40
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2769803003/200001
3 years, 8 months ago (2017-03-31 07:32:40 UTC) #43
commit-bot: I haz the power
3 years, 8 months ago (2017-03-31 09:25:38 UTC) #46
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/8c9d8f928bcb9796ea67ad39c58b...

Powered by Google App Engine
This is Rietveld 408576698