Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(3)

Issue 2735973006: Bindings: Separate WorldIdConstants into WorldTypes and WorldId (Closed)

Created:
8 months, 2 weeks ago by nhiroki
Modified:
8 months, 1 week ago
Reviewers:
haraken, peria, Yuki
CC:
chromium-reviews, blink-reviews, kinuko+watch, dominicc+watchlist_chromium.org, blink-reviews-bindings_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Bindings: Separate WorldIdConstants to WorldTypes and WorldId This is a clean-up CL and doesn't change behavior. For improving extensibility, this CL separates WorldIdConstants to 2 parts: WorldTypes and WorldId. This encapsulates identifier allocation logic in DOMWrapperWorld[1] and makes it easier to expand the identifier space for Worklets[2]. [1] WorldIds for IsolatedWorlds still need to be given from out of DOMWrapperWorld because of its unique convention to allocate the identifier. [2] https://codereview.chromium.org/2735823006/ BUG=697622, 697629 Review-Url: https://codereview.chromium.org/2735973006 Cr-Commit-Position: refs/heads/master@{#455700} Committed: https://chromium.googlesource.com/chromium/src/+/611dfce737a7df5d5a1e88e8528d405ebed37b8d

Patch Set 1 #

Total comments: 10

Patch Set 2 : address review comments #

Total comments: 10

Patch Set 3 : address review comments #

Patch Set 4 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+102 lines, -41 lines) Patch
M third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.h View 1 2 3 4 chunks +44 lines, -20 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp View 1 2 3 6 chunks +42 lines, -11 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp View 1 2 1 chunk +3 lines, -1 line 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.cpp View 1 2 1 chunk +3 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp View 1 1 chunk +2 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/xml/DocumentXMLTreeViewer.cpp View 1 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/modules/fetch/DataConsumerHandleTestUtil.cpp View 1 2 1 chunk +4 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/web/WebLocalFrameImpl.cpp View 1 3 chunks +3 lines, -3 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 43 (31 generated)
nhiroki
Hi, can you take a look at this? Thanks!
8 months, 2 weeks ago (2017-03-08 09:30:06 UTC) #16
haraken
https://codereview.chromium.org/2735973006/diff/100001/third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right): https://codereview.chromium.org/2735973006/diff/100001/third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp#newcode58 third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:58: NOTREACHED(); Why is it not reached?
8 months, 2 weeks ago (2017-03-08 09:39:09 UTC) #18
peria
LGTM % nits https://codereview.chromium.org/2735973006/diff/100001/third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right): https://codereview.chromium.org/2735973006/diff/100001/third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp#newcode60 third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:60: // Currently, WorldId for a worker/worklet ...
8 months, 2 weeks ago (2017-03-08 09:48:19 UTC) #19
nhiroki
Thank you! Updated. https://codereview.chromium.org/2735973006/diff/100001/third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right): https://codereview.chromium.org/2735973006/diff/100001/third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp#newcode58 third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:58: NOTREACHED(); On 2017/03/08 09:39:09, haraken wrote: ...
8 months, 2 weeks ago (2017-03-08 10:24:53 UTC) #25
haraken
https://codereview.chromium.org/2735973006/diff/100001/third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right): https://codereview.chromium.org/2735973006/diff/100001/third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp#newcode58 third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:58: NOTREACHED(); On 2017/03/08 10:24:52, nhiroki (slow) wrote: > On ...
8 months, 2 weeks ago (2017-03-08 10:27:47 UTC) #26
nhiroki
https://codereview.chromium.org/2735973006/diff/100001/third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right): https://codereview.chromium.org/2735973006/diff/100001/third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp#newcode58 third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:58: NOTREACHED(); On 2017/03/08 10:27:47, haraken wrote: > On 2017/03/08 ...
8 months, 2 weeks ago (2017-03-08 10:44:23 UTC) #27
haraken
LGTM
8 months, 2 weeks ago (2017-03-08 10:48:56 UTC) #28
Yuki
Looks good in general. https://codereview.chromium.org/2735973006/diff/110001/third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right): https://codereview.chromium.org/2735973006/diff/110001/third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp#newcode323 third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:323: int DOMWrapperWorld::getWorldIdForType(WorldType worldType) { IIUC, ...
8 months, 2 weeks ago (2017-03-08 14:07:51 UTC) #31
nhiroki
Thank you! Updated. https://codereview.chromium.org/2735973006/diff/110001/third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right): https://codereview.chromium.org/2735973006/diff/110001/third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp#newcode323 third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:323: int DOMWrapperWorld::getWorldIdForType(WorldType worldType) { On 2017/03/08 ...
8 months, 1 week ago (2017-03-09 03:40:29 UTC) #34
Yuki
LGTM.
8 months, 1 week ago (2017-03-09 07:32:46 UTC) #37
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/2735973006/150001
8 months, 1 week ago (2017-03-09 07:46:41 UTC) #40
commit-bot: I haz the power
8 months, 1 week ago (2017-03-09 08:45:45 UTC) #43
Message was sent while issue was closed.
Committed patchset #4 (id:150001) as
https://chromium.googlesource.com/chromium/src/+/611dfce737a7df5d5a1e88e8528d...

Powered by Google App Engine
This is Rietveld efc10ee0f