Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2784473002:
[Bindings] Move some private symbols from V8HiddenValue to V8PrivateProperty (Closed)
DescriptionMove event, state, port1, and port2 from V8HiddenValue to V8PrivateProperty
BUG=611864
Review-Url: https://codereview.chromium.org/2784473002
Cr-Commit-Position: refs/heads/master@{#461463}
Committed: https://chromium.googlesource.com/chromium/src/+/0582ff42568b3f83c5da9e6ccf5fce1fc02e252d
Patch Set 1 : . #
Total comments: 14
Patch Set 2 : Work for comments #
Total comments: 8
Patch Set 3 : . #
Total comments: 4
Patch Set 4 : work for nits #Patch Set 5 : Rebase #Patch Set 6 : rebase #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 41 (26 generated)
Description was changed from ========== Move port1/2 Move state Move event from V8HiddenValue to V8PrivateProperty BUG= ========== to ========== Move event, state, port1, and port2 from V8HiddenValue to V8PrivateProperty BUG=611864 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
peria@chromium.org changed reviewers: + haraken@chromium.org, yukishiino@chromium.org
PTL
https://codereview.chromium.org/2784473002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp (right): https://codereview.chromium.org/2784473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp:159: eventSymbol.set(context, context->Global(), v8::Undefined(isolate())); Looks like we can write as follows at a glance. savedEvent = eventSymbol.getOrUndefined(); eventSymbol.set(jsEvent); // ... eventSymbol.set(savedEvent); https://codereview.chromium.org/2784473002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h (right): https://codereview.chromium.org/2784473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:40: X(Message, Port1) \ X(MessageChannel, Port1) https://codereview.chromium.org/2784473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:50: X(Script, State) \ X(PopStateEvent, State) https://codereview.chromium.org/2784473002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/custom/V8MessageChannelCustom.cpp (right): https://codereview.chromium.org/2784473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/custom/V8MessageChannelCustom.cpp:56: scriptState->context(), wrapper, For v8::Private APIs, you should simply use the current context. i.e. isolate->GetCurrentContext(). https://codereview.chromium.org/2784473002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp (right): https://codereview.chromium.org/2784473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:139: v8::Local<v8::Value> jsEvent = V8PrivateProperty::getGlobalEvent(isolate).get( Seems like you can use getOrUndefined() without the following if-clause. https://codereview.chromium.org/2784473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:140: context, context->Global()); Not directly related to your CL, but we can use info.Holder() instead of context->Global(), and then we don't need to extract |context| using toV8Context(). This can be get(isolate->GetCurrentContext(), info.Holder()); https://codereview.chromium.org/2784473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:166: V8PrivateProperty::getGlobalEvent(isolate).set(context, context->Global(), Ditto. Not directly related to your CL, this can be set(isolate->GetCurrentContext(), info.Holder(), value);
Patchset #2 (id:60001) has been deleted
https://codereview.chromium.org/2784473002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp (right): https://codereview.chromium.org/2784473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp:159: eventSymbol.set(context, context->Global(), v8::Undefined(isolate())); On 2017/03/28 14:10:50, Yuki wrote: > Looks like we can write as follows at a glance. > > savedEvent = eventSymbol.getOrUndefined(); > eventSymbol.set(jsEvent); > // ... > eventSymbol.set(savedEvent); Agreed. Done. https://codereview.chromium.org/2784473002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h (right): https://codereview.chromium.org/2784473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:40: X(Message, Port1) \ On 2017/03/28 14:10:50, Yuki wrote: > X(MessageChannel, Port1) Done. https://codereview.chromium.org/2784473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:50: X(Script, State) \ On 2017/03/28 14:10:50, Yuki wrote: > X(PopStateEvent, State) Done. https://codereview.chromium.org/2784473002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/custom/V8MessageChannelCustom.cpp (right): https://codereview.chromium.org/2784473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/custom/V8MessageChannelCustom.cpp:56: scriptState->context(), wrapper, On 2017/03/28 14:10:50, Yuki wrote: > For v8::Private APIs, you should simply use the current context. i.e. > isolate->GetCurrentContext(). Done. https://codereview.chromium.org/2784473002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp (right): https://codereview.chromium.org/2784473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:139: v8::Local<v8::Value> jsEvent = V8PrivateProperty::getGlobalEvent(isolate).get( On 2017/03/28 14:10:50, Yuki wrote: > Seems like you can use getOrUndefined() without the following if-clause. Done. https://codereview.chromium.org/2784473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:140: context, context->Global()); On 2017/03/28 14:10:50, Yuki wrote: > Not directly related to your CL, but we can use info.Holder() instead of > context->Global(), and then we don't need to extract |context| using > toV8Context(). > > This can be > get(isolate->GetCurrentContext(), info.Holder()); Done. https://codereview.chromium.org/2784473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:166: V8PrivateProperty::getGlobalEvent(isolate).set(context, context->Global(), On 2017/03/28 14:10:50, Yuki wrote: > Ditto. > > Not directly related to your CL, this can be > set(isolate->GetCurrentContext(), info.Holder(), value); Done.
The CQ bit was checked by peria@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_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
LGTM. https://codereview.chromium.org/2784473002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h (right): https://codereview.chromium.org/2784473002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:50: X(PopStateEvent, State) \ nit: sort in alphabetical order? https://codereview.chromium.org/2784473002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/custom/V8PopStateEventCustom.cpp (right): https://codereview.chromium.org/2784473002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/custom/V8PopStateEventCustom.cpp:99: stateSymbol.get(context, v8History))); s/get/getOrUndefined/ because you've already checked hasValue().
https://codereview.chromium.org/2784473002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp (right): https://codereview.chromium.org/2784473002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp:139: // expose it. Do you know how the js event is exposed on the global object? Hidden values / private properties won't be exposed to user JS. https://codereview.chromium.org/2784473002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp (right): https://codereview.chromium.org/2784473002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:156: info.Holder(), value); It looks like that the comment in line 158 (before this CL) is saying that we should set info.Holder()->CreationContext(). Is the new code correct?
https://codereview.chromium.org/2784473002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp (right): https://codereview.chromium.org/2784473002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp:139: // expose it. On 2017/03/29 06:35:39, haraken wrote: > > Do you know how the js event is exposed on the global object? Hidden values / > private properties won't be exposed to user JS. V8Window::eventAttributeGetterCustom and V8Window::eventAttributeSetterCustom in V8WindowCustom.cpp gets / sets the value from / to the v8::Private appropriately. https://codereview.chromium.org/2784473002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp (right): https://codereview.chromium.org/2784473002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:156: info.Holder(), value); On 2017/03/29 06:35:39, haraken wrote: > > It looks like that the comment in line 158 (before this CL) is saying that we > should set info.Holder()->CreationContext(). Is the new code correct? Yes, the new code is more correct than before. We're not creating any new object, so we don't need the DOM object's creation context (= relevant realm for the context object). |value| is the object to be set, but it's given from author script as an argument. We're not creating it. A possible exception object is an exception, we could create and throw an exception, but it should be created in the current context. (So, the old implementation is wrong on this point.) Plus, v8:Private:{Get,Set} must not invoke any author script.
https://codereview.chromium.org/2784473002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h (right): https://codereview.chromium.org/2784473002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:50: X(PopStateEvent, State) \ On 2017/03/29 06:22:53, Yuki wrote: > nit: sort in alphabetical order? Done. https://codereview.chromium.org/2784473002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/custom/V8PopStateEventCustom.cpp (right): https://codereview.chromium.org/2784473002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/custom/V8PopStateEventCustom.cpp:99: stateSymbol.get(context, v8History))); On 2017/03/29 06:22:53, Yuki wrote: > s/get/getOrUndefined/ > because you've already checked hasValue(). Done.
The CQ bit was checked by peria@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...
LGTM https://codereview.chromium.org/2784473002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp (right): https://codereview.chromium.org/2784473002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:132: ASSERT(frame); We can remove this code. https://codereview.chromium.org/2784473002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:153: ASSERT(frame); Ditto.
https://codereview.chromium.org/2784473002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp (right): https://codereview.chromium.org/2784473002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:132: ASSERT(frame); On 2017/03/29 07:37:24, haraken wrote: > > We can remove this code. Done. https://codereview.chromium.org/2784473002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:153: ASSERT(frame); On 2017/03/29 07:37:24, haraken wrote: > > Ditto. Done.
The CQ bit was checked by peria@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yukishiino@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2784473002/#ps120001 (title: "work for nits")
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
Try jobs failed on following builders: linux_chromium_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 peria@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_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 peria@chromium.org to run a CQ dry run
Rebased. PTAL just in case.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM.
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 peria@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2784473002/#ps160001 (title: "rebase")
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": 160001, "attempt_start_ts": 1491239775786700,
"parent_rev": "92933ff654ebb9bb8be0de9eaad60668eb395443", "commit_rev":
"0582ff42568b3f83c5da9e6ccf5fce1fc02e252d"}
Message was sent while issue was closed.
Description was changed from ========== Move event, state, port1, and port2 from V8HiddenValue to V8PrivateProperty BUG=611864 ========== to ========== Move event, state, port1, and port2 from V8HiddenValue to V8PrivateProperty BUG=611864 Review-Url: https://codereview.chromium.org/2784473002 Cr-Commit-Position: refs/heads/master@{#461463} Committed: https://chromium.googlesource.com/chromium/src/+/0582ff42568b3f83c5da9e6ccf5f... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:160001) as https://chromium.googlesource.com/chromium/src/+/0582ff42568b3f83c5da9e6ccf5f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
