Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1416053012:
Switch V8HiddenValue from hidden values to privates (Closed)
|
Created:
5 years, 1 month ago by jochen (gone - plz use gerrit) Modified:
5 years, 1 month ago CC:
blink-reviews, blink-reviews-bindings_chromium.org, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSwitch V8HiddenValue from hidden values to privates
Privates are like ES6 symbols, but are not accesible by JavaScript.
Hidden values on the other hand will be deleted form the API soon.
BUG=none
R=haraken@chromium.org
Committed: https://crrev.com/b1802c0fc28635a50e7f55e40c71e69281cc85f3
Cr-Commit-Position: refs/heads/master@{#360786}
Patch Set 1 #
Total comments: 2
Patch Set 2 : updates #Patch Set 3 : updates #Patch Set 4 : updates #Patch Set 5 : updates #Patch Set 6 : updates #Patch Set 7 : updates #Messages
Total messages: 22 (3 generated)
LGTM https://codereview.chromium.org/1416053012/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/V8HiddenValue.cpp (right): https://codereview.chromium.org/1416053012/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/V8HiddenValue.cpp:35: return object->SetPrivate(isolate->GetCurrentContext(), v8::Private::ForApi(isolate, key), value).FromJust(); Can you use v8Call()? https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://codereview.chromium.org/1416053012/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/V8HiddenValue.cpp:40: return object->DeletePrivate(isolate->GetCurrentContext(), v8::Private::ForApi(isolate, key)).FromJust(); Ditto.
done
On 2015/11/06 16:41:47, jochen (slow - traveling) wrote: > done LGTM
FYI, keep in mind that this CL may affect performance of some blink_perf benchmarks. I've made a couple of changes to V8HiddenValues before but had to revert them because they caused regressions.
jochen@chromium.org changed reviewers: + vogelheim@chromium.org
... and finally, the switch of the API
LGTM
The CQ bit was checked by jochen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1416053012/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1416053012/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/b1802c0fc28635a50e7f55e40c71e69281cc85f3 Cr-Commit-Position: refs/heads/master@{#360786}
Message was sent while issue was closed.
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
Message was sent while issue was closed.
Causing https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Oil... ?
Message was sent while issue was closed.
Also behind the meltdown on Oilpan DBG bots, it appears: http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac%20Oilpan%20...
Message was sent while issue was closed.
On 2015/11/20 at 14:23:25, sigbjornf wrote: > Also behind the meltdown on Oilpan DBG bots, it appears: > > http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac%20Oilpan%20... hum, running script in the prologue is a problem, that mustn't happen :-/
Message was sent while issue was closed.
Another problematic callstack, https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Win_Oilpa...
Message was sent while issue was closed.
A dozen (flaky) crashes here due to same, https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Oil... Until kinks have been worked out, too soon to make this switch?
Message was sent while issue was closed.
On 2015/11/21 08:10:28, sof wrote: > A dozen (flaky) crashes here due to same, > > > https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Oil... > > Until kinks have been worked out, too soon to make this switch? Yeah... We also trigger an Oilpan's GC in the gc epilogue callback. jochen@: I don't want to block your work by an issue that happens in ENABLE(OILPAN) only, but would it be possible to wait for landing this CL until we fix the oilpan-side issue?
Message was sent while issue was closed.
On 2015/11/21 08:30:39, haraken wrote: > On 2015/11/21 08:10:28, sof wrote: > > A dozen (flaky) crashes here due to same, > > > > > > > https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Oil... > > > > Until kinks have been worked out, too soon to make this switch? > > Yeah... We also trigger an Oilpan's GC in the gc epilogue callback. > > jochen@: I don't want to block your work by an issue that happens in > ENABLE(OILPAN) only, but would it be possible to wait for landing this CL until > we fix the oilpan-side issue? We really need to get Oilpan Canaries rolling, so instabilities in Oilpan land are problematic in that regard.
Message was sent while issue was closed.
On 2015/11/21 08:34:26, sof wrote: > On 2015/11/21 08:30:39, haraken wrote: > > On 2015/11/21 08:10:28, sof wrote: > > > A dozen (flaky) crashes here due to same, > > > > > > > > > > > > https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Oil... > > > > > > Until kinks have been worked out, too soon to make this switch? > > > > Yeah... We also trigger an Oilpan's GC in the gc epilogue callback. > > > > jochen@: I don't want to block your work by an issue that happens in > > ENABLE(OILPAN) only, but would it be possible to wait for landing this CL > until > > we fix the oilpan-side issue? > > We really need to get Oilpan Canaries rolling, so instabilities in Oilpan land > are problematic in that regard. Yeah... Sorry but let me revert the CL to push the Oilpan Canaries forward :/
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/1466733006/ by haraken@chromium.org. The reason for reverting is: This CL broke tests in oilpan builds. https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Oil... . |
