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

Issue 2815453005: [Bindings] Use SetNativeDataProperty to install features on instances (Closed)

Created:
2 years, 5 months ago by peria
Modified:
2 years, 4 months ago
Reviewers:
haraken, bashi, jbroman, Yuki
CC:
blink-reviews, blink-reviews-bindings_chromium.org, chromium-reviews
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Use SetNativeDataProperty to install features dynamically on instances This CL replace DefineOwnProperty with SetNativeDataProperty in InstallAttributeInternal<v8::Object>() not to create function templates or instances on feature installations. This change enables us to use the function for all data properties and it improves performance of preparing origin-trial features and dynamic installs of runtime enabled features. BUG=617892 Review-Url: https://codereview.chromium.org/2815453005 Cr-Commit-Position: refs/heads/master@{#467260} Committed: https://chromium.googlesource.com/chromium/src/+/040755ece35f949c4de74d85f5a651e84a0beafe

Patch Set 1 #

Patch Set 2 : Add DCHECK #

Total comments: 4

Patch Set 3 : . #

Patch Set 4 : Remove different DCHECKs #

Total comments: 3

Patch Set 5 : Use SetNativeDataProperty #

Unified diffs Side-by-side diffs Delta from patch set Stats (+19 lines, -27 lines) Patch
M third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp View 1 2 3 4 1 chunk +19 lines, -27 lines 0 comments Download

Messages

Total messages: 37 (25 generated)
peria
PTL
2 years, 5 months ago (2017-04-13 07:48:34 UTC) #11
haraken
LGTM but let's wait for an approval from shiino-san. https://codereview.chromium.org/2815453005/diff/20001/third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (left): https://codereview.chromium.org/2815453005/diff/20001/third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp#oldcode105 third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:105: ...
2 years, 5 months ago (2017-04-13 08:06:09 UTC) #13
bashi
https://codereview.chromium.org/2815453005/diff/20001/third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right): https://codereview.chromium.org/2815453005/diff/20001/third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp#newcode109 third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:109: DCHECK(instance Why do we need DCHECK()? ToChecked() should crash ...
2 years, 5 months ago (2017-04-13 08:08:36 UTC) #14
peria
https://codereview.chromium.org/2815453005/diff/20001/third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (left): https://codereview.chromium.org/2815453005/diff/20001/third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp#oldcode105 third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:105: DCHECK_EQ(&V8ConstructorAttributeGetter, attribute.getter); On 2017/04/13 08:06:09, haraken wrote: > > ...
2 years, 5 months ago (2017-04-13 09:51:21 UTC) #15
jbroman
https://codereview.chromium.org/2815453005/diff/60001/third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right): https://codereview.chromium.org/2815453005/diff/60001/third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp#newcode117 third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:117: ->SetAccessor(context, name, getter, setter, data, v8::DEFAULT, This is only ...
2 years, 5 months ago (2017-04-13 14:54:45 UTC) #21
haraken
https://codereview.chromium.org/2815453005/diff/60001/third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right): https://codereview.chromium.org/2815453005/diff/60001/third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp#newcode117 third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:117: ->SetAccessor(context, name, getter, setter, data, v8::DEFAULT, On 2017/04/13 14:54:45, ...
2 years, 5 months ago (2017-04-13 15:40:19 UTC) #22
jbroman
https://codereview.chromium.org/2815453005/diff/60001/third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right): https://codereview.chromium.org/2815453005/diff/60001/third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp#newcode117 third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:117: ->SetAccessor(context, name, getter, setter, data, v8::DEFAULT, On 2017/04/13 at ...
2 years, 5 months ago (2017-04-13 17:11:24 UTC) #23
Yuki
The CL itself LGTM. Talked with peria-san offline about another issue. Let's fix it first.
2 years, 5 months ago (2017-04-14 07:39:20 UTC) #24
peria
I prepared v8::Object::SetNativeDataProperty()[1], and update this CL to use it. Please take another look. [1] ...
2 years, 5 months ago (2017-04-20 04:01:31 UTC) #26
Yuki
LGTM.
2 years, 5 months ago (2017-04-20 09:12:38 UTC) #31
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/2815453005/80001
2 years, 4 months ago (2017-04-26 05:35:56 UTC) #34
commit-bot: I haz the power
2 years, 4 months ago (2017-04-26 07:21:05 UTC) #37
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/040755ece35f949c4de74d85f5a6...

Powered by Google App Engine
This is Rietveld 408576698