Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1709003002:
Geometry: Reimplement DOMPoint using V8 extras. (Closed)
|
Created:
4 years, 10 months ago by zino Modified:
3 years, 3 months ago CC:
blink-reviews, blink-reviews-bindings_chromium.org, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionGeometry: Reimplement DOMPoint using V8 extras.
This implements the DOMPoint[1] class, using experimental V8 extras (so that it
will only show up with experimental web platform features enabled).
This CL also replaces a existing layout test with a testharness-based test.
[1] Spec: https://drafts.fxtf.org/geometry/#DOMPoint
BUG=388780
Patch Set 1 #Patch Set 2 : #
Total comments: 18
Patch Set 3 : #
Total comments: 6
Patch Set 4 : #
Total comments: 12
Messages
Total messages: 61 (11 generated)
Description was changed from ========== Geometry: Reimplement DOMPoint using V8 extras. This implements the DOMPoint class, using experimental V8 extras (so that it will only show up with experimental web platform features enabled). BUG=388780 ========== to ========== Geometry: Reimplement DOMPoint using V8 extras. This implements the DOMPoint class, using experimental V8 extras (so that it will only show up with experimental web platform features enabled). TODO: add comment about removing constructor. TODO: add todo comment to IDLs. TODO: modify webexposed BUG=388780 ==========
Description was changed from ========== Geometry: Reimplement DOMPoint using V8 extras. This implements the DOMPoint class, using experimental V8 extras (so that it will only show up with experimental web platform features enabled). TODO: add comment about removing constructor. TODO: add todo comment to IDLs. TODO: modify webexposed BUG=388780 ========== to ========== Geometry: Reimplement DOMPoint using V8 extras. This implements the DOMPoint class, using experimental V8 extras (so that it will only show up with experimental web platform features enabled). BUG=388780 ==========
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Geometry: Reimplement DOMPoint using V8 extras. This implements the DOMPoint class, using experimental V8 extras (so that it will only show up with experimental web platform features enabled). BUG=388780 ========== to ========== Geometry: Reimplement DOMPoint using V8 extras. This implements the DOMPoint[1] class, using experimental V8 extras (so that it will only show up with experimental web platform features enabled). [1] Spec: https://drafts.fxtf.org/geometry/#DOMPoint BUG=388780 ==========
jinho.bang@samsung.com changed reviewers: + domenic@chromium.org, haraken@chromium.org, philipj@opera.com
PTAL Thank you. https://codereview.chromium.org/1709003002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VREyeParameters.idl (right): https://codereview.chromium.org/1709003002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VREyeParameters.idl:14: [CallWith=ScriptState] readonly attribute any eyeTranslation; I used 'any' keyword but I'm not sure if this is a right way.
Overall looks good! Lots of little things, but in general this continues to look like a perfect match for V8 extras. https://codereview.chromium.org/1709003002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-point-expected.txt (left): https://codereview.chromium.org/1709003002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-point-expected.txt:19: # DOMPoint({ x : 2 }) Why remove these tests? https://codereview.chromium.org/1709003002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt (right): https://codereview.chromium.org/1709003002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt:102: interface DOMPoint : DOMPointReadOnly Yay, this makes us more spec-compliant! (The spec says [Exposed=(Window,Worker)]) https://codereview.chromium.org/1709003002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt (left): https://codereview.chromium.org/1709003002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt:830: getter w These should still be present, after you go to the private symbol + getter approach. https://codereview.chromium.org/1709003002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/GeometryInterfaces.cpp (right): https://codereview.chromium.org/1709003002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/GeometryInterfaces.cpp:23: return ScriptValue(scriptState, V8ScriptRunner::callExtra(scriptState, "createDOMPoint", args)); Probably callExtraOrCrash? https://codereview.chromium.org/1709003002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/geometry/DOMPointReadOnly.js (right): https://codereview.chromium.org/1709003002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/geometry/DOMPointReadOnly.js:12: defineProperty(this, 'x', { While I personally prefer this design, the spec mandates the use of getters and setters (that's what Web IDL's "attributes" are). So you'll instead need to store these values in private symbols, and add things like `get x() { return this[xSymbol]; }` to the class definition. https://codereview.chromium.org/1709003002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/geometry/DOMPointReadOnly.js:46: binding.createDOMPointReadOnly = DOMPointReadOnly.fromPoint; An option here would be to instead do `binding.createDOMPointReadOnly = (x, y, z, w) => new DOMPointReadOnly(x, y, z, w)`. That would save you from doing the V8ObjectBuilder stuff and avoid a level of indirection. Might even give a slight perf boost. https://codereview.chromium.org/1709003002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VREyeParameters.idl (right): https://codereview.chromium.org/1709003002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VREyeParameters.idl:14: [CallWith=ScriptState] readonly attribute any eyeTranslation; On 2016/02/18 at 16:25:33, zino wrote: > I used 'any' keyword but I'm not sure if this is a right way. That has been what we have been doing so far, but as more V8 extras start appearing we'll probably want to change the bindings generator in some way. haraken@ might have more thoughts. Maybe for now it'd be worth adding a comment like `// spec says DOMPoint, but we use any since it is implemented with V8 extras` https://codereview.chromium.org/1709003002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VRPositionState.cpp (right): https://codereview.chromium.org/1709003002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRPositionState.cpp:41: m_orientation = vecToDomPoint(scriptState, m_state.orientation, m_state.flags & WebVRSensorStateOrientation); This re-creates the ScriptValue every time. I think that is an observable behavior change from JavaScript code, right? It would mean `vrPositionState.orientation !== vrPositionState.orientation`? Which would be bad. Why not create them in the setState method, like before?
Are we already shipping things on stable that depend on V8 extras? Trying to figure out whether this would commit us to V8 extras any more than we already are.
On 2016/02/19 at 04:12:17, philipj wrote: > Are we already shipping things on stable that depend on V8 extras? Trying to figure out whether this would commit us to V8 extras any more than we already are. I am not sure it has made it to stable yet, but ReadableStream has been in master for a while now. Both ReadableStream and this CL are hiding themselves behind experimental web platform features though.
On 2016/02/19 22:13:29, domenic wrote: > On 2016/02/19 at 04:12:17, philipj wrote: > > Are we already shipping things on stable that depend on V8 extras? Trying to > figure out whether this would commit us to V8 extras any more than we already > are. > > I am not sure it has made it to stable yet, but ReadableStream has been in > master for a while now. Both ReadableStream and this CL are hiding themselves > behind experimental web platform features though. Oh, good, this CL doesn't change the status quo then.
Patchset #4 (id:80001) has been deleted
Patchset #3 (id:60001) has been deleted
Thank you for review! :) PTAL. https://codereview.chromium.org/1709003002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-point-expected.txt (left): https://codereview.chromium.org/1709003002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-point-expected.txt:19: # DOMPoint({ x : 2 }) On 2016/02/18 19:04:40, domenic wrote: > Why remove these tests? After your suggestion[1], it might be changed. The spec removed overloaded constructor and then introduced static fromPoint method. [1] https://lists.w3.org/Archives/Public/public-fx/2015JanMar/0129.html [2] https://drafts.fxtf.org/geometry/#dompoint https://codereview.chromium.org/1709003002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt (right): https://codereview.chromium.org/1709003002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt:102: interface DOMPoint : DOMPointReadOnly On 2016/02/18 19:04:40, domenic wrote: > Yay, this makes us more spec-compliant! (The spec says > [Exposed=(Window,Worker)]) Done. https://codereview.chromium.org/1709003002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt (left): https://codereview.chromium.org/1709003002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt:830: getter w On 2016/02/18 19:04:40, domenic wrote: > These should still be present, after you go to the private symbol + getter > approach. Done. https://codereview.chromium.org/1709003002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/GeometryInterfaces.cpp (right): https://codereview.chromium.org/1709003002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/GeometryInterfaces.cpp:23: return ScriptValue(scriptState, V8ScriptRunner::callExtra(scriptState, "createDOMPoint", args)); On 2016/02/18 19:04:41, domenic wrote: > Probably callExtraOrCrash? Done. https://codereview.chromium.org/1709003002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/geometry/DOMPointReadOnly.js (right): https://codereview.chromium.org/1709003002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/geometry/DOMPointReadOnly.js:12: defineProperty(this, 'x', { On 2016/02/18 19:04:41, domenic wrote: > While I personally prefer this design, the spec mandates the use of getters and > setters (that's what Web IDL's "attributes" are). So you'll instead need to > store these values in private symbols, and add things like `get x() { return > this[xSymbol]; }` to the class definition. Done. https://codereview.chromium.org/1709003002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/geometry/DOMPointReadOnly.js:46: binding.createDOMPointReadOnly = DOMPointReadOnly.fromPoint; On 2016/02/18 19:04:41, domenic wrote: > An option here would be to instead do `binding.createDOMPointReadOnly = (x, y, > z, w) => new DOMPointReadOnly(x, y, z, w)`. That would save you from doing the > V8ObjectBuilder stuff and avoid a level of indirection. Might even give a slight > perf boost. Done. https://codereview.chromium.org/1709003002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VREyeParameters.idl (right): https://codereview.chromium.org/1709003002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VREyeParameters.idl:14: [CallWith=ScriptState] readonly attribute any eyeTranslation; On 2016/02/18 19:04:41, domenic wrote: > On 2016/02/18 at 16:25:33, zino wrote: > > I used 'any' keyword but I'm not sure if this is a right way. > > That has been what we have been doing so far, but as more V8 extras start > appearing we'll probably want to change the bindings generator in some way. > haraken@ might have more thoughts. Maybe for now it'd be worth adding a comment > like `// spec says DOMPoint, but we use any since it is implemented with V8 > extras` Done. https://codereview.chromium.org/1709003002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VRPositionState.cpp (right): https://codereview.chromium.org/1709003002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRPositionState.cpp:41: m_orientation = vecToDomPoint(scriptState, m_state.orientation, m_state.flags & WebVRSensorStateOrientation); On 2016/02/18 19:04:41, domenic wrote: > This re-creates the ScriptValue every time. I think that is an observable > behavior change from JavaScript code, right? It would mean > `vrPositionState.orientation !== vrPositionState.orientation`? Which would be > bad. Why not create them in the setState method, like before? Done. https://codereview.chromium.org/1709003002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/geometry/DOMPoint.js (right): https://codereview.chromium.org/1709003002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/geometry/DOMPoint.js:44: class DOMPoint extends DOMPointReadOnly { BTW, I didn't found a way to share private symbols in each other files. So, I wrote DOMPoint and DOMPointReadOnly classes in the same file. DOMPoint* classes are simple and small but In case of DOMMatrix* classes, I think it is better to split them into individual file. Can we do like this? (But it looks hacky..) v8.xSymbol = v8.createPrivateSymbol('[[x]]'); Do you have better ideas to split these classes into individual file?
https://codereview.chromium.org/1709003002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-point-expected.txt (left): https://codereview.chromium.org/1709003002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/geometry-interfaces-dom-point-expected.txt:19: # DOMPoint({ x : 2 }) On 2016/02/22 at 09:51:01, zino wrote: > On 2016/02/18 19:04:40, domenic wrote: > > Why remove these tests? > > After your suggestion[1], it might be changed. > The spec removed overloaded constructor and then introduced static fromPoint method. > > [1] https://lists.w3.org/Archives/Public/public-fx/2015JanMar/0129.html > [2] https://drafts.fxtf.org/geometry/#dompoint Ah, I see! That is good then. https://codereview.chromium.org/1709003002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/geometry/DOMPoint.js (right): https://codereview.chromium.org/1709003002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/geometry/DOMPoint.js:24: return this[xSymbol]; I realized something else that we need to do for spec-compliance :(. We need to add if (!(xSymbol in this)) { throw new TypeError('Illegal invocation'); } to all the getters and setters. Since something has a ySymbol/zSymbol/wSymbol iff it has a xSymbol, you could extract this into a standalone function: function ensureDOMPoint(obj) { if (!(xSymbol in obj)) { throw new TypeError('Illegal invocation'); } } and then just add `ensureDOMPoint(this)` at the top of each getter/setter. https://codereview.chromium.org/1709003002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/geometry/DOMPoint.js:44: class DOMPoint extends DOMPointReadOnly { On 2016/02/22 at 09:51:01, zino wrote: > BTW, I didn't found a way to share private symbols in each other files. > So, I wrote DOMPoint and DOMPointReadOnly classes in the same file. > DOMPoint* classes are simple and small but > In case of DOMMatrix* classes, I think it is better to split them into individual file. > > Can we do like this? (But it looks hacky..) > v8.xSymbol = v8.createPrivateSymbol('[[x]]'); > > Do you have better ideas to split these classes into individual file? That's pretty reasonable, except you should use `binding` instead of `v8` to store your symbol. `binding` is specifically meant as a place to store things that are shared between V8 extras and C++, so it is a good place for two V8 extras to work together. If you do that you might want to namespace them a bit more since `binding` is a flat namespace. E.g. `const _x = binding.domPointX = v8.createPrivateSymbol('[[x]]')` and then in your other file you could do `const { domPointX: _x, domPointY: _y, ... } = binding`. (You can use `xSymbol` instead of `_x` too; I just thought that looked a little nicer.) https://codereview.chromium.org/1709003002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/geometry/DOMPoint.js:89: if (obj instanceof DOMPoint) { A better test (that can't be fooled by author code doing `someObj[Symbol.hasInstance] = () => true`) is `if (xSymbol in obj)`.
Thank you for review and questions. https://codereview.chromium.org/1709003002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/geometry/DOMPoint.js (right): https://codereview.chromium.org/1709003002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/geometry/DOMPoint.js:24: return this[xSymbol]; Thank you for comment first! But I'm so sorry. I didn't understand your input exactly. Why do we need to ensure `xSymbol in this`? In case of updateDOMPoint function, I think it is necessary because ScriptValue can't be DOMPoint in C++ layer. On the other hand, in case of getter and setter, it's not exposed to binding layer. So, I think that xSymbol(and y, z, w) always exist in DOMPoint internally and JS author can't remove it from object. Could you share me the test cases to throw the exception? https://codereview.chromium.org/1709003002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/geometry/DOMPoint.js:54: this[xSymbol] = x; Is it correct to set NaN if x isn't Number. e.g. this[xSymbol] = isNaN(x) ? NaN : x;
On 2016/02/23 at 16:13:25, jinho.bang wrote: > Thank you for review and questions. > > https://codereview.chromium.org/1709003002/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/geometry/DOMPoint.js (right): > > https://codereview.chromium.org/1709003002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/geometry/DOMPoint.js:24: return this[xSymbol]; > Thank you for comment first! > > But I'm so sorry. I didn't understand your input exactly. > Why do we need to ensure `xSymbol in this`? > > In case of updateDOMPoint function, I think it is necessary because ScriptValue can't be DOMPoint in C++ layer. > On the other hand, in case of getter and setter, it's not exposed to binding layer. > So, I think that xSymbol(and y, z, w) always exist in DOMPoint internally and JS author can't remove it from object. > > Could you share me the test cases to throw the exception? Sure. Here is the test case: const getter = Object.getOwnPropertyDescriptor(DOMPointReadOnly.prototype, "x").get; getter.call(new DOMPoint()); // should work OK and return 0 getter.call({}); // should throw a TypeError, but with your current implementation will return undefined. > > https://codereview.chromium.org/1709003002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/geometry/DOMPoint.js:54: this[xSymbol] = x; > Is it correct to set NaN if x isn't Number. > > e.g. > this[xSymbol] = isNaN(x) ? NaN : x; Good question. For `unrestricted double` the best way is `this[xSymbol] = +x`.
Description was changed from ========== Geometry: Reimplement DOMPoint using V8 extras. This implements the DOMPoint[1] class, using experimental V8 extras (so that it will only show up with experimental web platform features enabled). [1] Spec: https://drafts.fxtf.org/geometry/#DOMPoint BUG=388780 ========== to ========== Geometry: Reimplement DOMPoint using V8 extras. This implements the DOMPoint[1] class, using experimental V8 extras (so that it will only show up with experimental web platform features enabled). This CL also replaces a existing layout test with a testharness-based test. [1] Spec: https://drafts.fxtf.org/geometry/#DOMPoint BUG=388780 ==========
Thank you for your review and inputs. I addressed your comments and added test. Please take a look.
On 2016/02/24 at 14:53:42, jinho.bang wrote: > Thank you for your review and inputs. > > I addressed your comments and added test. > > Please take a look. This LGTM; very nice. haraken@ should look at the bindings and give committer sign-off. In particular the createDOMPoint(ReadOnly) methods are a bit different from what we did for ReadableStream: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... (we explicitly pass in ScriptState instead of using the current one). But maybe that is OK for your use cases. https://codereview.chromium.org/1709003002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRPositionState.cpp (right): https://codereview.chromium.org/1709003002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRPositionState.cpp:37: vecToDomPoint(m_orientation, state.orientation, state.flags & WebVRSensorStateOrientation); Is using references in this way OK according to Blink style? I am not sure myself; maybe haraken@ or philipj@ can comment.
Unfortunately this CL has a bunch of problems from the binding perspective (although they are fixable)... I'm just curious but what's the advantage of moving DOMPoint to V8Extras? Currently DOMPoint.js looks too small to justify the complexity we need to introduce to write it in V8Extras. Do we have a plan to add more functionalities to DOMPoint.js in the future? https://codereview.chromium.org/1709003002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/GeometryInterfaces.cpp (right): https://codereview.chromium.org/1709003002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/GeometryInterfaces.cpp:13: v8::Isolate* isolate = v8::Isolate::GetCurrent(); v8::Isolate::GetCurrent() is deprecated. You should pass in the ScriptState from touchPositionAdjustedToBestClickableNode (you can get the ScriptState by adding [CallWith=ScriptState] to the IDL file) down to this method. https://codereview.chromium.org/1709003002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/GeometryInterfaces.cpp:14: v8::HandleScope scope(isolate); This shouldn't be needed. If GeometryInterfaces::createDOMPoint is called in a state where it doesn't have a HandleScope, it's a bug. https://codereview.chromium.org/1709003002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/GeometryInterfaces.cpp:26: void GeometryInterfaces::updateDOMPoint(ScriptValue& scriptValue, double x, double y, double z, double w) scriptValue => eyeTranslation ? https://codereview.chromium.org/1709003002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/GeometryInterfaces.cpp:28: v8::Isolate* isolate = v8::Isolate::GetCurrent(); Remove this. https://codereview.chromium.org/1709003002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/GeometryInterfaces.cpp:29: v8::HandleScope scope(isolate); Remove this. https://codereview.chromium.org/1709003002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/GeometryInterfaces.cpp:38: ScriptState* scriptState = ScriptState::current(isolate); With this, you'll end up with using a wrong ScriptState because it's not guaranteed that you're in a correct context when you call GeometryInterfaces::updateDOMPoint. You must use scriptValue.scriptState() instead. https://codereview.chromium.org/1709003002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/GeometryInterfaces.h (right): https://codereview.chromium.org/1709003002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/GeometryInterfaces.h:8: #include "bindings/core/v8/ScriptValue.h" Forward declaration would be enough. https://codereview.chromium.org/1709003002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/testing/Internals.idl (right): https://codereview.chromium.org/1709003002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/Internals.idl:136: [RaisesException] any touchPositionAdjustedToBestClickableNode(long x, long y, long width, long height, Document document); Add [CallWith=ScriptState] and pass it down to methods that use V8 extras. https://codereview.chromium.org/1709003002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/Internals.idl:142: [RaisesException] any touchPositionAdjustedToBestContextMenuNode(long x, long y, long width, long height, Document document); Ditto. https://codereview.chromium.org/1709003002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VREyeParameters.idl (right): https://codereview.chromium.org/1709003002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VREyeParameters.idl:18: readonly attribute any eyeTranslation; If this change is not an urgent one, can we fix this bug before landing this change? It's very important that IDL files are aligned with the spec. https://codereview.chromium.org/1709003002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRPositionState.cpp (right): https://codereview.chromium.org/1709003002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRPositionState.cpp:17: value = ScriptValue::createNull(ScriptState::current(v8::Isolate::GetCurrent())); Please don't use ScriptState::current without understanding what you're doing :) You need to pass the ScriptState from V8 bindings (by adding [CallWith=ScriptState]) down to this method.
jinho.bang@samsung.com changed reviewers: + pdr@chromium.org
On 2016/02/25 11:21:11, haraken wrote: > Unfortunately this CL has a bunch of problems from the binding perspective > (although they are fixable)... > > I'm just curious but what's the advantage of moving DOMPoint to V8Extras? > Currently DOMPoint.js looks too small to justify the complexity we need to > introduce to write it in V8Extras. Do we have a plan to add more functionalities > to DOMPoint.js in the future? > Thank you for review first. The DOMPoint is one of geometry interfaces such as DOMMatrix, DOMRect and so on. I implemented it with C++ but there was performance issues due to overhead from binding layer. You can see the discussion on blink-dev. - https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/V_bJNtOg0oM When I attended BlinkOn5, I got to know V8 extras in lightening talk time. I thought it is a good solution. Because the interfaces is too small, it is better to implement in V8extras, isn't it? (Then we can avoid crossing binding layer for too small calculation in DOMPoint or DOMMatrix.)
Added pdr@ as reviewer. What do you think?
Maybe I'm just behind :) How does it improve performance? For example, when you call updateDOMPoint, you anyway need to go through the "binding" in GeometryInterfaces::updateDOMPoint. How does the overhead different from the normal V8 binding for C++?
On 2016/02/25 11:51:31, haraken wrote: > Maybe I'm just behind :) > > How does it improve performance? For example, when you call updateDOMPoint, you > anyway need to go through the "binding" in GeometryInterfaces::updateDOMPoint. > How does the overhead different from the normal V8 binding for C++? Yep, there can be no differences in that case. (e.g. returning some object from C++) But you can imagine the following situation. var matrix = new DOMMatrix(); // If the DOMMatrix is implemented in V8 Extras. // The following operations will not cross the binding layer to transform the matrix. matrix.translate(…) matrix.rotate(…) matrix.skew(...) … matrix.someOperation(…) // Apply the transformed matrix to canvas context. canvas_context.setTransform(matrix);
On 2016/02/25 13:57:43, zino wrote: > On 2016/02/25 11:51:31, haraken wrote: > > Maybe I'm just behind :) > > > > How does it improve performance? For example, when you call updateDOMPoint, > you > > anyway need to go through the "binding" in GeometryInterfaces::updateDOMPoint. > > How does the overhead different from the normal V8 binding for C++? > > Yep, there can be no differences in that case. (e.g. returning some object from > C++) > But you can imagine the following situation. > > var matrix = new DOMMatrix(); // If the DOMMatrix is implemented in V8 Extras. > > // The following operations will not cross the binding layer to transform the > matrix. > matrix.translate(…) > matrix.rotate(…) > matrix.skew(...) > … > matrix.someOperation(…) > > // Apply the transformed matrix to canvas context. > canvas_context.setTransform(matrix); I'm not quite sure how the operation could work without going through the binding layer. Does V8 extra have a mechanism to install DOM attribute/method on a DOM object (i.e., how does V8 extra install DOMMatrix.translate)? I guess that currently only V8DOMConfiguration has that mechanism. It's for normal C++ V8 bindings. So the only way matrix.translate() works would be: 1) matrix.translate() calls V8DOMMatrix::translateCallback in normal C++ V8 bindings. 2) V8DOMMatrix::translateCallback calls DOMMatrix::translate in C++. 3) DOMMatrix::translate calls V8 extra that has an actual implementation of the translation. (Sorry if I'm misunderstanding V8 extras.)
On 2016/02/25 14:19:28, haraken wrote:
> I'm not quite sure how the operation could work without going through the
> binding layer. Does V8 extra have a mechanism to install DOM attribute/method
on
> a DOM object (i.e., how does V8 extra install DOMMatrix.translate)?
>
> I guess that currently only V8DOMConfiguration has that mechanism. It's for
> normal C++ V8 bindings. So the only way matrix.translate() works would be:
>
> 1) matrix.translate() calls V8DOMMatrix::translateCallback in normal C++ V8
> bindings.
> 2) V8DOMMatrix::translateCallback calls DOMMatrix::translate in C++.
> 3) DOMMatrix::translate calls V8 extra that has an actual implementation of
the
> translation.
>
> (Sorry if I'm misunderstanding V8 extras.)
I'm not sure but as far as I know the v8 extra feature will work like built-in
JS library. (such as Array, Promise and so on)
So, standalone operation will not call any method or callback in C++ binding
side.
I modified my previous example more details.
// Let's presume that DOMMatrix class is implemented in V8 Extras.
var matrix = new DOMMatrix() // Only run in JS side. (not call any callback in
binding side)
// The following operations will transform the matrix itself but it only run in
JS side.
// NOTE: the following operations don't belong to operation set of canvas 2d.
matrix.translateSelf(...) // Only run in JS side. (not call any callback in
binding side)
matrix.rotateSelf(...) // Only run in JS side. (not call any callback in
binding side)
matrix.skewSelf(...) // Only run in JS side. (not call any callback in binding
side)
// Until now, the matrix has been being transformed value but any operation
haven't crossed binding layer yet.
// To calculate the matrix, we don't need calling C++ function.
// We can just set the result of the matrix which is already calculated.
// The following operation will transform CTM of canvas 2d context and it run in
C++ binding side.
document.querySelector('canvas').getContext('2d').currentTransform = matrix;
I'm not expert and I think that you and Domenic will know V8 more than me.
If my understanding is something wrong, please correct me.
Thank you!!
On 2016/02/25 14:19:28, haraken wrote:
> I'm not quite sure how the operation could work without going through the
> binding layer. Does V8 extra have a mechanism to install DOM attribute/method
on
> a DOM object (i.e., how does V8 extra install DOMMatrix.translate)?
>
> I guess that currently only V8DOMConfiguration has that mechanism. It's for
> normal C++ V8 bindings. So the only way matrix.translate() works would be:
>
> 1) matrix.translate() calls V8DOMMatrix::translateCallback in normal C++ V8
> bindings.
> 2) V8DOMMatrix::translateCallback calls DOMMatrix::translate in C++.
> 3) DOMMatrix::translate calls V8 extra that has an actual implementation of
the
> translation.
>
> (Sorry if I'm misunderstanding V8 extras.)
I'm not sure but as far as I know the v8 extra feature will work like built-in
JS library. (such as Array, Promise and so on)
So, standalone operation will not call any method or callback in C++ binding
side.
I modified my previous example more details.
// Let's presume that DOMMatrix class is implemented in V8 Extras.
var matrix = new DOMMatrix() // Only run in JS side.
// The following operations will transform the matrix itself
// but it only run in JS side.
// NOTE: the following operations don't belong to operation set of canvas 2d.
matrix.translateSelf(...) // Only run in JS side.
matrix.rotateSelf(...) // Only run in JS side.
matrix.skewSelf(...) // Only run in JS side.
// Until now, the matrix has been being transformed value but any operation
// haven't crossed binding layer yet.
// To calculate the matrix, we don't need calling C++ function.
// We can just set the result of the matrix which is already calculated.
// The following operation will transform CTM of canvas 2d context
// and it run in C++ binding side.
document.querySelector('canvas').getContext('2d').currentTransform = matrix;
I'm not expert and I think that you and Domenic will know V8 more than me.
If my understanding is something wrong, please correct me.
Thank you!!
@haraken, some backstory... a naive implementation of the DOMMatrix API would be somewhat inefficient for common operations such as rotateSelf. On the thread I argued that these objects will end up being used for more than just interfacing with the DOM since our interfaces lead developers that way. For example, a 2d sprite game engine might use DOMMatrix both for the DOM interfaces, and for their internal math / physics calc / etc. I think we should ensure DOMMatrix.rotateSelf is at least as fast as the equivalent code in pure javascript. Is that feasible using this approach? @jinho.bang, how does this look on http://jsperf.com/dommatrix2/3 (or a similar testcase)?
On 2016/02/26 06:37:20, pdr wrote: > @haraken, some backstory... a naive implementation of the DOMMatrix API would be > somewhat inefficient for common operations such as rotateSelf. On the thread I > argued that these objects will end up being used for more than just interfacing > with the DOM since our interfaces lead developers that way. For example, a 2d > sprite game engine might use DOMMatrix both for the DOM interfaces, and for > their internal math / physics calc / etc. I think we should ensure > DOMMatrix.rotateSelf is at least as fast as the equivalent code in pure > javascript. Is that feasible using this approach? Yes, I understand your context :) My question is how it's feasible. domenic: Would you explain how the V8 extra works for matrix.translateSelf? How can the matrix.translateSelf called in a user script call the translateSelf function implemented in the V8 extra without going through any C++ bindings? > > @jinho.bang, how does this look on http://jsperf.com/dommatrix2/3 (or a similar > testcase)?
On 2016/02/26 at 09:07:58, haraken wrote:
> domenic: Would you explain how the V8 extra works for matrix.translateSelf?
How can the matrix.translateSelf called in a user script call the translateSelf
function implemented in the V8 extra without going through any C++ bindings?
Hmm, I am not sure how else to explain it than to just say that it does. V8
extras execute JavaScript code, like class declarations, directly in the context
of the page. It is similar to how in the following user script:
class MyDOMMatrix {
translateSelf() {
// do stuff to `this`
}
}
const d = new MyDOMMatrix();
d.translateSelf();
no C++ bindings are encountered. No C++ bindings even exist to go through!
The C++ code in this patch is to allow other C++ bindings to create and
manipulate ScriptValues representing DOMPoints (or, in a future CL,
DOMMatrixes). But methods and properties, like the x/y/z/w getter/setter in this
CL, or translateSelf in a future CL, will be implemented in JavaScript without
any C++ bindings.
On 2016/02/26 17:01:47, domenic wrote:
> On 2016/02/26 at 09:07:58, haraken wrote:
>
> > domenic: Would you explain how the V8 extra works for matrix.translateSelf?
> How can the matrix.translateSelf called in a user script call the
translateSelf
> function implemented in the V8 extra without going through any C++ bindings?
>
> Hmm, I am not sure how else to explain it than to just say that it does. V8
> extras execute JavaScript code, like class declarations, directly in the
context
> of the page. It is similar to how in the following user script:
>
> class MyDOMMatrix {
> translateSelf() {
> // do stuff to `this`
> }
> }
>
> const d = new MyDOMMatrix();
> d.translateSelf();
>
>
> no C++ bindings are encountered. No C++ bindings even exist to go through!
Does it mean that V8 extra ignores IDL files? For example, if DOMMatrix.idl has
[NoInterfaceObject], [Exposed=...], [Replaceable], [NotEnumerable] etc, they are
just ignored and V8 extra is responsible for implementing the behaviors?
Also is there any existing example where V8 extra is used for bypassing the C++
binding layer? I'm a big fan of the idea (mainly for performance reasons) but
I'm asking this because there would be a couple of things we need to be careful
about when bypassing the C++ binding layer.
On 2016/02/27 at 15:20:25, haraken wrote: > Does it mean that V8 extra ignores IDL files? For example, if DOMMatrix.idl has [NoInterfaceObject], [Exposed=...], [Replaceable], [NotEnumerable] etc, they are just ignored and V8 extra is responsible for implementing the behaviors? Right, that is why this CL deletes DOMPointInit.idl, DOMPointReadOnly.idl, and DOMPoint.idl. > Also is there any existing example where V8 extra is used for bypassing the C++ binding layer? Yeah, we're using it in https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit.... This is a bit confusing because there are currently IDL versions in the nearby ReadableByteStream.idl and ReadableByteStreamReader.idl, but we are planning to get rid of those after the rest of the fetch code starts to understand the ReadableStream.js version. (Certainly before moving ReadableStream.js out of the experimental extras category, into the shipping extras category.) > I'm a big fan of the idea (mainly for performance reasons) but I'm asking this because there would be a couple of things we need to be careful about when bypassing the C++ binding layer. I am glad you are a fan :). I do strongly agree that caution is needed. In https://bit.ly/v8-extras I go into some of the precautions that are necessary and emphasize that V8 extras are not a good fit for cases like Node or similar that are very intertwined with the binding system and C++ class hierarchy. They are a good fit for ReadableStream and DOMPoint/DOMMatrix/DOMQuad though because they are so standalone. You still need to be a bit careful, e.g. the ensureDOMPoint code in this CL is something that the bindings would normally take care of for you. But in general I think this CL is a good demonstration of how V8 extras can be used to get "bare metal" performance without too much additional complexity from bypassing the bindings layer. The trickiest part, for both this CL and for the ReadableStream work, is integrating with C++: the GeometryInterfaces.{h,cpp} in this CL; the ReadableStreamOperations.{h,cpp} files in master; and the issue where we (for now) have to use `any` instead of `DOMPoint`/etc in other .idl files that reference these. I think now that we have a couple of examples of V8 extras in use we should start considering ways to make those integrations more automatic. E.g. maybe we could work on generating GeometryInterfaces.{h,cpp} and ReadableStreamOperations.{h,cpp} from some kind of "v8 extras IDL".
On 2016/02/27 17:06:39, domenic wrote: > On 2016/02/27 at 15:20:25, haraken wrote: > > > Does it mean that V8 extra ignores IDL files? For example, if DOMMatrix.idl > has [NoInterfaceObject], [Exposed=...], [Replaceable], [NotEnumerable] etc, they > are just ignored and V8 extra is responsible for implementing the behaviors? > > Right, that is why this CL deletes DOMPointInit.idl, DOMPointReadOnly.idl, and > DOMPoint.idl. > > > Also is there any existing example where V8 extra is used for bypassing the > C++ binding layer? > > Yeah, we're using it in > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit.... > This is a bit confusing because there are currently IDL versions in the nearby > ReadableByteStream.idl and ReadableByteStreamReader.idl, but we are planning to > get rid of those after the rest of the fetch code starts to understand the > ReadableStream.js version. (Certainly before moving ReadableStream.js out of the > experimental extras category, into the shipping extras category.) > > > I'm a big fan of the idea (mainly for performance reasons) but I'm asking this > because there would be a couple of things we need to be careful about when > bypassing the C++ binding layer. > > I am glad you are a fan :). I do strongly agree that caution is needed. In > https://bit.ly/v8-extras I go into some of the precautions that are necessary > and emphasize that V8 extras are not a good fit for cases like Node or similar > that are very intertwined with the binding system and C++ class hierarchy. They > are a good fit for ReadableStream and DOMPoint/DOMMatrix/DOMQuad though because > they are so standalone. You still need to be a bit careful, e.g. the > ensureDOMPoint code in this CL is something that the bindings would normally > take care of for you. But in general I think this CL is a good demonstration of > how V8 extras can be used to get "bare metal" performance without too much > additional complexity from bypassing the bindings layer. > > The trickiest part, for both this CL and for the ReadableStream work, is > integrating with C++: the GeometryInterfaces.{h,cpp} in this CL; the > ReadableStreamOperations.{h,cpp} files in master; and the issue where we (for > now) have to use `any` instead of `DOMPoint`/etc in other .idl files that > reference these. I think now that we have a couple of examples of V8 extras in > use we should start considering ways to make those integrations more automatic. > E.g. maybe we could work on generating GeometryInterfaces.{h,cpp} and > ReadableStreamOperations.{h,cpp} from some kind of "v8 extras IDL". Thanks for the clarification! I'm now convinced that implementing DOMPoint/DOMMatrix in V8 extra is a way to go. A couple of thoughts: a) I'm not really happy about getting rid of IDL files because all web-exposed interfaces should be described as IDL files. Ideally I might want to have IDL files annotated with [ImplementedInV8Extra], which generate necessary bindings between V8 extra and JS (e.g., install the DOM interface on a global object, install attributes/methods on the DOM interface etc). Currently you're manually writing all of the bindings. That is fine in short term, but I'd like to keep the IDL files with the DOM interface annotated with [ImplementedInV8Extra]. You can just tell the IDL compiler to ignore DOM interfaces annotated with [ImplementedInV8Extra]. b) Let's consider fixing the 'any' or 'DOMPoint' issue. Can we fix a) before landing this CL? b) could be fixed later.
On 2016/02/29 01:25:10, haraken wrote: > a) I'm not really happy about getting rid of IDL files because all web-exposed > interfaces should be described as IDL files. Ideally I might want to have IDL > files annotated with [ImplementedInV8Extra], which generate necessary bindings > between V8 extra and JS (e.g., install the DOM interface on a global object, > install attributes/methods on the DOM interface etc). Currently you're manually > writing all of the bindings. That is fine in short term, but I'd like to keep > the IDL files with the DOM interface annotated with [ImplementedInV8Extra]. You > can just tell the IDL compiler to ignore DOM interfaces annotated with > [ImplementedInV8Extra]. Yeah, it would be great if the IDL files could be counted on to accurately reflect the actual behavior, so making the bindings generator output the necessary JS sounds like the perfect goal. There are so many little things that are easy to overlook. I'll defer to haraken@ as to when this should happen, though.
On 2016/02/29 04:46:17, philipj_UTC7 wrote: > On 2016/02/29 01:25:10, haraken wrote: > > > a) I'm not really happy about getting rid of IDL files because all web-exposed > > interfaces should be described as IDL files. Ideally I might want to have IDL > > files annotated with [ImplementedInV8Extra], which generate necessary bindings > > between V8 extra and JS (e.g., install the DOM interface on a global object, > > install attributes/methods on the DOM interface etc). Currently you're > manually > > writing all of the bindings. That is fine in short term, but I'd like to keep > > the IDL files with the DOM interface annotated with [ImplementedInV8Extra]. > You > > can just tell the IDL compiler to ignore DOM interfaces annotated with > > [ImplementedInV8Extra]. > > Yeah, it would be great if the IDL files could be counted on to accurately > reflect the actual behavior, so making the bindings generator output the > necessary JS sounds like the perfect goal. There are so many little things that > are easy to overlook. I'll defer to haraken@ as to when this should happen, > though. It will be a lot of work, so I won't ask domenic@ to implement the V8extra-binding-generator right now. But I'd prefer introducing [ImplementedInV8Extra] so that we can keep IDL files for V8 extras on trunk.
jochen@chromium.org changed reviewers: + jochen@chromium.org
I don't think this is a good use of V8 Extras. Note that those classes will be exposed to all contexts - that includes the proxy autoconf scripts, or scripts running as part of PDF documents. With should make C++ bindings fast enough so we don't need to resort to hacks like this.
you could of course argue that a point and a matrix are pretty generic concepts and can be part of all the contexts - but then the DOM prefix should be dropped IMO We also need faster bindings.
On 2016/02/29 08:27:12, jochen wrote: > you could of course argue that a point and a matrix are pretty generic concepts > and can be part of all the contexts - but then the DOM prefix should be dropped > IMO > > We also need faster bindings. Per jochen's comment on https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/g4..., it looks like that V8 extra is not a mechanism to bypass C++ bindings for performance reasons. So, not LGTM.
On 2016/02/29 at 09:14:56, haraken wrote: > On 2016/02/29 08:27:12, jochen wrote: > > you could of course argue that a point and a matrix are pretty generic concepts > > and can be part of all the contexts - but then the DOM prefix should be dropped > > IMO > > > > We also need faster bindings. > > Per jochen's comment on https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/g4..., it looks like that V8 extra is not a mechanism to bypass C++ bindings for performance reasons. So, not LGTM. as I also mentioned in the post, we can go down that road, but then we also need to be prepared to deal with some of the JS subtleties. e.g. I'd expect getOwnProperty / hasOwnProperties instead of _x in this or this[_x] calls. What about @@toString?
On 2016/02/29 at 08:27:12, jochen wrote: > you could of course argue that a point and a matrix are pretty generic concepts and can be part of all the contexts - but then the DOM prefix should be dropped IMO > > We also need faster bindings. Jochen, I strongly disagree with your assessment that this is not a good fit for V8 extras. These are indeed generic concepts, and the DOM prefix is suboptimal but exists for reasons which the CSSWG decided were important. (Presumably web-compat from not squatting on a common name, and compatibility with already-shipping implementations like Firefox.) They are pure JS libraries, decoupled from the rest of the DOM, that make sense in all contexts. Specification politics and the question of whose specs needed them first have landed them in the CSSWG instead of anywhere else, but basic point/rect/quad/matrix types are clearly standard library fodder. I also don't understand how faster bindings could be the solution here. Would it even be possible to have bindings that allowed inlining between JS and C++? These are pure JS data structures and avoiding the binding layer makes a lot of sense for them. V8 extras allows us to cut through a lot of abstraction. Most of the time that abstraction is important and necessary, but V8 extras gives us the option of treating it as a tradeoff instead of a hard requirement. This is important because in the past the overhead of that abstraction has prevented features---like the Geometry interfaces---from ever shipping. On 2016/02/29 at 09:58:23, jochen wrote: > > as I also mentioned in the post, we can go down that road, but then we also need to be prepared to deal with some of the JS subtleties. > > e.g. I'd expect getOwnProperty / hasOwnProperties instead of _x in this or this[_x] calls. What about @@toString? The getOwnProperty and in behavior is more correct per spec; inheritance is allowed. @@toStringTag is indeed missing; good catch. On 2016/02/29 at 01:25:10, haraken wrote: > a) I'm not really happy about getting rid of IDL files because all web-exposed interfaces should be described as IDL files. Ideally I might want to have IDL files annotated with [ImplementedInV8Extra], which generate necessary bindings between V8 extra and JS (e.g., install the DOM interface on a global object, install attributes/methods on the DOM interface etc). Currently you're manually writing all of the bindings. That is fine in short term, but I'd like to keep the IDL files with the DOM interface annotated with [ImplementedInV8Extra]. You can just tell the IDL compiler to ignore DOM interfaces annotated with [ImplementedInV8Extra]. Haraken, can you clarify how you expect this to work? The idea of IDL files that do nothing seems strange to me. Would it be different if, like Streams, we asked the CSSWG to remove the IDL from the spec and specify these as JS objects? It is not doing very much, especially given all the `unrestricted double` types. Maybe that way we could get them to use data properties instead of getters. Later you and philipj mention a bindings generator in JS. I have actually worked on something similar in the past for the jsdom project. (Examples: https://gist.github.com/domenic/30aaa634eeac03606508 and https://gist.github.com/domenic/b5c395e036781173e123) It is a lot of work and introduces a lot of the same overhead and abstraction layers that we are trying to avoid. Maybe they can be inlined and JITted away to nothing, but it still seems like a big project to retool the IDL binding generator to output JS, for unclear purposes. When I upthread talked about a "v8 extras IDL", I meant something more like this: [V8ExtrasBinding] interface GeometryInterfaces { DOMPoint createDOMPoint(optional unrestricted double x = 0, optional unrestricted double y = 0, optional unrestricted double z = 0, optional unrestricted double w = 1); void updateDOMPoint(DOMPoint p, optional unrestricted double x = 0, optional unrestricted double y = 0, optional unrestricted double z = 0, optional unrestricted double w = 1); } which would generate the contents of this CL's GeometryInterfaces.{h,cpp}.
One more thing... On 2016/02/29 at 01:25:10, haraken wrote: > a) I'm not really happy about getting rid of IDL files because all web-exposed interfaces should be described as IDL files. This is already not the case for Promise, Map, Set, and others (including all the streams classes, e.g. ReadableStream/ReadableStreamReader/etc.). It's not clear to me what criterion you are employing here.
As I said in the mail Kentaro referenced, I don't think that we can't go down this route, but I'd first like to see a detailed analysis of the behavior we'll end up exposing by bindings defined via v8 extras. I'd expect to end up with a doc that explains when to use v8 extras and when not, and the bindings should be generated and not hand-written. btw, we can (and do) inline C++ bindings calls. A common problem there is that some accessor is defined on a prototype, and a given callsites is hit repeatedly with different concrete object which is something that the V8 type system doesn't deal well with. This is, however, also a problem for pure JS methods.
On 2016/02/29 at 13:01:00, jochen wrote:
> As I said in the mail Kentaro referenced, I don't think that we can't go down
this route, but I'd first like to see a detailed analysis of the behavior we'll
end up exposing by bindings defined via v8 extras. I'd expect to end up with a
doc that explains when to use v8 extras and when not, and the bindings should be
generated and not hand-written.
Can you explain what you mean by "bindings" here? Is it the
GeometryInterfaces.{h,cpp} files in this CL, or are you referring instead to
creating a parallel set of Python scripts that output JS instead of C++?
On 2016/02/29 12:40:55, domenic wrote: > On 2016/02/29 at 01:25:10, haraken wrote: > > a) I'm not really happy about getting rid of IDL files because all web-exposed > interfaces should be described as IDL files. Ideally I might want to have IDL > files annotated with [ImplementedInV8Extra], which generate necessary bindings > between V8 extra and JS (e.g., install the DOM interface on a global object, > install attributes/methods on the DOM interface etc). Currently you're manually > writing all of the bindings. That is fine in short term, but I'd like to keep > the IDL files with the DOM interface annotated with [ImplementedInV8Extra]. You > can just tell the IDL compiler to ignore DOM interfaces annotated with > [ImplementedInV8Extra]. > > Haraken, can you clarify how you expect this to work? The idea of IDL files that > do nothing seems strange to me. > > Would it be different if, like Streams, we asked the CSSWG to remove the IDL > from the spec and specify these as JS objects? It is not doing very much, > especially given all the `unrestricted double` types. Maybe that way we could > get them to use data properties instead of getters. > > Later you and philipj mention a bindings generator in JS. I have actually worked > on something similar in the past for the jsdom project. (Examples: > https://gist.github.com/domenic/30aaa634eeac03606508 and > https://gist.github.com/domenic/b5c395e036781173e123) It is a lot of work and > introduces a lot of the same overhead and abstraction layers that we are trying > to avoid. Maybe they can be inlined and JITted away to nothing, but it still > seems like a big project to retool the IDL binding generator to output JS, for > unclear purposes. For me, I wouldn't have a problem nuking the IDL files if this wasn't even defined using WebIDL in the spec. My concern with spec'ing them with IDL but not using a bindings generator is similar to the problem of [Custom], where it's quite easy for little problems to slip by. Just as a random example, for a very long time the custom code for addEventListener would be a no-op with zero arguments, but addEventListener.length would still be 2, because the length attribute was set by the generated code. If there's *no* generated code such a mismatch cannot happen, but failing to throw the right kinds of exceptions in the right places is more likely.
On 2016/02/29 at 17:56:26, philipj wrote: > For me, I wouldn't have a problem nuking the IDL files if this wasn't even defined using WebIDL in the spec. My concern with spec'ing them with IDL but not using a bindings generator is similar to the problem of [Custom], where it's quite easy for little problems to slip by. Just as a random example, for a very long time the custom code for addEventListener would be a no-op with zero arguments, but addEventListener.length would still be 2, because the length attribute was set by the generated code. Hmm, OK. It seems a little weird that we have to ask a standards group to change the format they use to define their thing before we are allowed to use a certain implementation strategy. In other words, I am unsure how to phrase the bug report to the CSSWG: "please remove this part of the spec for reasons related to Blink's engineering process". It will probably also not be well-received by other vendors which don't have V8 extras-like infrastructure. > If there's *no* generated code such a mismatch cannot happen, but failing to throw the right kinds of exceptions in the right places is more likely. I agree that it's quite possible to make mistakes when hand-coding web-exposed behavior. As I've said, it's a balancing act. In this case I think the benefits outweigh the costs; the surface area is small enough that we should be able to catch such things, IMO.
On 2016/02/29 18:04:26, domenic wrote: > On 2016/02/29 at 17:56:26, philipj wrote: > > > For me, I wouldn't have a problem nuking the IDL files if this wasn't even > defined using WebIDL in the spec. My concern with spec'ing them with IDL but not > using a bindings generator is similar to the problem of [Custom], where it's > quite easy for little problems to slip by. Just as a random example, for a very > long time the custom code for addEventListener would be a no-op with zero > arguments, but addEventListener.length would still be 2, because the length > attribute was set by the generated code. > > Hmm, OK. It seems a little weird that we have to ask a standards group to change > the format they use to define their thing before we are allowed to use a certain > implementation strategy. In other words, I am unsure how to phrase the bug > report to the CSSWG: "please remove this part of the spec for reasons related to > Blink's engineering process". It will probably also not be well-received by > other vendors which don't have V8 extras-like infrastructure. I presume that in redefining it, it could also be made faster by avoiding certain checks that are really only needed because WebIDL says so. But you're right, it would look like an odd request if not accompanied by any observable changes. > > If there's *no* generated code such a mismatch cannot happen, but failing to > throw the right kinds of exceptions in the right places is more likely. > > I agree that it's quite possible to make mistakes when hand-coding web-exposed > behavior. As I've said, it's a balancing act. In this case I think the benefits > outweigh the costs; the surface area is small enough that we should be able to > catch such things, IMO. Sure, if the amount of V8 extras code is low enough that it can be reviewed very thoroughly for these kinds of problems it sounds OK. I suppose for the time being there isn't much risk for an avalanche of V8 extras code.
esprehn@chromium.org changed reviewers: + esprehn@chromium.org
I'm not very comfortable having these implemented without the idl files. One big advantage of the blink binding system over ex. gin is that it's all idl files.
On 2016/02/29 at 20:06:16, esprehn wrote: > I'm not very comfortable having these implemented without the idl files. Do you have a proposed alternative? > One big advantage of the blink binding system over ex. gin is that it's all idl files. The idea is to bypass the binding system.
On 2016/02/29 at 20:09:01, domenic wrote: > On 2016/02/29 at 20:06:16, esprehn wrote: > > I'm not very comfortable having these implemented without the idl files. > > Do you have a proposed alternative? Code generate JS binding wrappers? ex. if DOMPointInit is supposed to be doubles, we should be able to code generate something that enforces that from the idl. > > > One big advantage of the blink binding system over ex. gin is that it's all idl files. > > The idea is to bypass the binding system. That seems like a non-goal to me. Specs which are written in terms of idl should have matching idl files in blink we can audit, verify, and generate code from. Skipping the bindings system also just doesn't make sense, other browsers are much faster in the bindings without having to "skip the bindings system" which doesn't benefit the rest of the platform like the DOM. You shouldn't need to read the JS code to figure out if the idl spec has been followed faithfully. ex. "do we parse this property as a double?"
On 2016/02/29 21:50:28, esprehn wrote: > On 2016/02/29 at 20:09:01, domenic wrote: > > On 2016/02/29 at 20:06:16, esprehn wrote: > > > I'm not very comfortable having these implemented without the idl files. > > > > Do you have a proposed alternative? > > Code generate JS binding wrappers? ex. if DOMPointInit is supposed to be > doubles, we should be able to code generate something that enforces that from > the idl. > > > > > > One big advantage of the blink binding system over ex. gin is that it's all > idl files. > > > > The idea is to bypass the binding system. > > That seems like a non-goal to me. Specs which are written in terms of idl should > have matching idl files in blink we can audit, verify, and generate code from. > Skipping the bindings system also just doesn't make sense, other browsers are > much faster in the bindings without having to "skip the bindings system" which > doesn't benefit the rest of the platform like the DOM. > > You shouldn't need to read the JS code to figure out if the idl spec has been > followed faithfully. ex. "do we parse this property as a double?" I share these concerns, and I do love having IDL files that can be counted on to reflect actual behavior, but what are the practical options for getting the geometry interfaces shipped? ClientRect and WebKitCSSMatrix have already been shipping using C++ for a very long time, so the equivalent functionality is already part of the platform. Why do other browsers have faster bindings, is there any low-hanging fruit in Blink's bindings generator? For PODs like these I imagine it would be very hard for C++ bindings to compete with a JS-only solution, but benchmarks of the various approaches would be very useful. Jinho?
On 2016/03/01 03:57:50, philipj_UTC7 wrote: > On 2016/02/29 21:50:28, esprehn wrote: > > On 2016/02/29 at 20:09:01, domenic wrote: > > > On 2016/02/29 at 20:06:16, esprehn wrote: > > > > I'm not very comfortable having these implemented without the idl files. > > > > > > Do you have a proposed alternative? > > > > Code generate JS binding wrappers? ex. if DOMPointInit is supposed to be > > doubles, we should be able to code generate something that enforces that from > > the idl. > > > > > > > > > One big advantage of the blink binding system over ex. gin is that it's > all > > idl files. > > > > > > The idea is to bypass the binding system. > > > > That seems like a non-goal to me. Specs which are written in terms of idl > should > > have matching idl files in blink we can audit, verify, and generate code from. > > Skipping the bindings system also just doesn't make sense, other browsers are > > much faster in the bindings without having to "skip the bindings system" which > > doesn't benefit the rest of the platform like the DOM. > > > > You shouldn't need to read the JS code to figure out if the idl spec has been > > followed faithfully. ex. "do we parse this property as a double?" > > I share these concerns, and I do love having IDL files that can be counted on to > reflect actual behavior, but what are the practical options for getting the > geometry interfaces shipped? ClientRect and WebKitCSSMatrix have already been > shipping using C++ for a very long time, so the equivalent functionality is > already part of the platform. > > Why do other browsers have faster bindings, is there any low-hanging fruit in > Blink's bindings generator? For PODs like these I imagine it would be very hard > for C++ bindings to compete with a JS-only solution, but benchmarks of the > various approaches would be very useful. Jinho? Yeah, benchmarks are helpful to understand what's slow. The binding team will look into it.
I don't think we should bypass the bindings system. We can talk about teaching the bindings system to generate JS / V8 extras based bindings. But it has proven to be key for the security and maintainability of the bindings system to be auto-generated from IDL files.
On 2016/03/01 at 14:42:00, jochen wrote: > I don't think we should bypass the bindings system. > > We can talk about teaching the bindings system to generate JS / V8 extras based bindings. But it has proven to be key for the security and maintainability of the bindings system to be auto-generated from IDL files. It sounds like that is the conclusion then. Unfortunately I don't have time to work on a bindings generator that outputs JS, especially one written in Python. Is that something the bindings team could consider for upcoming quarters? The first step, I suppose, would be to create some feasible code that looks like generated output (perhaps similar to https://gist.github.com/domenic/30aaa634eeac03606508#file-3-dompointreadonly-... but using V8 extras instead of Node's module system, and maybe with DOMMatrix instead of DOMPoint so we get the more performance-sensitive operations). Then it could be benchmarked vs. this CL to see how much overhead the generated JS indirections add, to see if such a project is even worthwhile, or if the indirections kill the idea outright and we'll simply never be able to implement a fast DOMMatrix given the political requirements. (Although, it sounds like maybe we could get it implemented in the fashion of this CL if we convinced the CSSWG to remove the IDL from the spec?) It's a real shame to block Jinho's CL on that larger work item, which I assume will take at least several months. As is, he has been trying to implement this feature in one form or another since June 2014 but got blocked on performance concerns [1]. It would have been ideal if these concerns could have been brought up in his intent to implement back in November [2], when we first came up with the idea of using V8 extras to solve the 40x slowdown. [1]: https://groups.google.com/a/chromium.org/d/msg/blink-dev/V_bJNtOg0oM/3ziq9DWr... [2]: https://groups.google.com/a/chromium.org/d/msg/blink-dev/V_bJNtOg0oM/BsMzrWqa...
On 2016/03/02 22:33:21, domenic wrote: > On 2016/03/01 at 14:42:00, jochen wrote: > > I don't think we should bypass the bindings system. > > > > We can talk about teaching the bindings system to generate JS / V8 extras > based bindings. But it has proven to be key for the security and maintainability > of the bindings system to be auto-generated from IDL files. > > It sounds like that is the conclusion then. Unfortunately I don't have time to > work on a bindings generator that outputs JS, especially one written in Python. > Is that something the bindings team could consider for upcoming quarters? I'll chat with the binding team. I'm basically a fan of the idea because the JS-JS binding will provide a super-fast binding system for web features. But as jochen pointed out, there are a bunch of things we need to be careful about. > > The first step, I suppose, would be to create some feasible code that looks like > generated output (perhaps similar to > https://gist.github.com/domenic/30aaa634eeac03606508#file-3-dompointreadonly-... > but using V8 extras instead of Node's module system, and maybe with DOMMatrix > instead of DOMPoint so we get the more performance-sensitive operations). Then > it could be benchmarked vs. this CL to see how much overhead the generated JS > indirections add, to see if such a project is even worthwhile, or if the > indirections kill the idea outright and we'll simply never be able to implement > a fast DOMMatrix given the political requirements. (Although, it sounds like > maybe we could get it implemented in the fashion of this CL if we convinced the > CSSWG to remove the IDL from the spec?) > > It's a real shame to block Jinho's CL on that larger work item, which I assume > will take at least several months. As is, he has been trying to implement this > feature in one form or another since June 2014 but got blocked on performance > concerns [1]. It would have been ideal if these concerns could have been brought > up in his intent to implement back in November [2], when we first came up with > the idea of using V8 extras to solve the 40x slowdown. > > [1]: > https://groups.google.com/a/chromium.org/d/msg/blink-dev/V_bJNtOg0oM/3ziq9DWr... > [2]: > https://groups.google.com/a/chromium.org/d/msg/blink-dev/V_bJNtOg0oM/BsMzrWqa...
On 2016/03/02 at 22:33:21, domenic wrote: > On 2016/03/01 at 14:42:00, jochen wrote: > > I don't think we should bypass the bindings system. > > > > We can talk about teaching the bindings system to generate JS / V8 extras based bindings. But it has proven to be key for the security and maintainability of the bindings system to be auto-generated from IDL files. > > It sounds like that is the conclusion then. Unfortunately I don't have time to work on a bindings generator that outputs JS, especially one written in Python. Is that something the bindings team could consider for upcoming quarters? > > The first step, I suppose, would be to create some feasible code that looks like generated output (perhaps similar to https://gist.github.com/domenic/30aaa634eeac03606508#file-3-dompointreadonly-... but using V8 extras instead of Node's module system, and maybe with DOMMatrix instead of DOMPoint so we get the more performance-sensitive operations). Then it could be benchmarked vs. this CL to see how much overhead the generated JS indirections add, to see if such a project is even worthwhile, or if the indirections kill the idea outright and we'll simply never be able to implement a fast DOMMatrix given the political requirements. (Although, it sounds like maybe we could get it implemented in the fashion of this CL if we convinced the CSSWG to remove the IDL from the spec?) > > It's a real shame to block Jinho's CL on that larger work item, which I assume will take at least several months. As is, he has been trying to implement this feature in one form or another since June 2014 but got blocked on performance concerns [1]. It would have been ideal if these concerns could have been brought up in his intent to implement back in November [2], when we first came up with the idea of using V8 extras to solve the 40x slowdown. > > [1]: https://groups.google.com/a/chromium.org/d/msg/blink-dev/V_bJNtOg0oM/3ziq9DWr... > [2]: https://groups.google.com/a/chromium.org/d/msg/blink-dev/V_bJNtOg0oM/BsMzrWqa... Sorry for not seeing the mention of v8 extras on the intent to implement. Since I'm no expert on CSS I usually only skim those. Please feel free to explicitly add bindings folks in the future of nobody reacts
I agree with domenic@ that it would be sad to block this for many months more. How about if it's implemented using helper functions that are roughly what you would expect from generated bindings, which would be reviewed by the bindings team? Then converting DOMPoint to generated bindings would be the first goal for the JS-JS bindings generator.
On 2016/03/03 07:09:53, philipj_UTC7 wrote: > I agree with domenic@ that it would be sad to block this for many months more. > How about if it's implemented using helper functions that are roughly what you > would expect from generated bindings, which would be reviewed by the bindings > team? Then converting DOMPoint to generated bindings would be the first goal for > the JS-JS bindings generator. This sounds like a good idea, but keep in mind that that will be a kind of experimental project. I'm excited about the idea of the JS-JS binding but I'm not 100% sure if it's really feasible (e.g., how to deal with JS subtleties, how to hide binding stuff from user JS, @@toStringTag etc). Implementing the DOMPoint/DOMMatrix would definitely be a good first step for the JS-JS binding, but there is a risk that we have to give up the idea if the DOMPoint/DOMMatrix discloses problems we're not yet aware of.
Either way (i.e., whether we implement the JS-JS bindings or not), it would be helpful to have a benchmark that shows the performance problem of DOMMatrix. Then the binding team can look into what is slow and how to fix the problem :) |
