Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1837823003:
[Binding] Add [OverrideBuiltins] label onto HTMLDocument interface (Closed)
|
Created:
4 years, 8 months ago by peria Modified:
4 years, 7 months ago CC:
chromium-reviews, blink-reviews-html_chromium.org, sof, eae+blinkwatch, blink-reviews-dom_chromium.org, dglazkov+blink, blink-reviews-bindings_chromium.org, blink-reviews, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd [OverrideBuiltins] label onto HTMLDocument interface.
This CL also removes special handling of named properties
using V8 script controller, which is no longer needed.
Currently, we have no general routines to work for [Unforgeable]
on [OverrideBuiltins] interface, and "location" in "HTMLDocument"
is the only one which meets the situation.
So now, we handle it as an edge case of named property.
Spec: https://html.spec.whatwg.org/multipage/dom.html#dom-document-namedItem-which
Updated a test expectation; Chrome has been FAILing it, while IE, FF, and Safari PASS it.
BUG=611632
Committed: https://crrev.com/09649cf329fd0644f50624b80f6a2f9c564dcb2d
Cr-Commit-Position: refs/heads/master@{#395570}
Patch Set 1 : #
Total comments: 10
Patch Set 2 : #
Total comments: 8
Patch Set 3 : #Messages
Total messages: 49 (36 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Test named property BUG= ========== to ========== Implement named property getter in HTMLDocument. This CL also removes registering V8 wrappers of named items while a browser process parses a HTML file. BUG= ==========
Patchset #1 (id:60001) has been deleted
Patchset #6 (id:180001) has been deleted
Patchset #5 (id:160001) has been deleted
Patchset #5 (id:200001) has been deleted
Description was changed from ========== Implement named property getter in HTMLDocument. This CL also removes registering V8 wrappers of named items while a browser process parses a HTML file. BUG= ========== to ========== Implement named property getter in HTMLDocument. This CL also removes registering V8 wrappers of named items while a browser process parses a HTML file. Current issues: - document['location'] returns <iframe name="location">, but it should return a Location object, because 'location' is a [Unforgeable] attribute in Document. - document[1] and document['1'] do not return <img name="1">. BUG= ==========
Description was changed from ========== Implement named property getter in HTMLDocument. This CL also removes registering V8 wrappers of named items while a browser process parses a HTML file. Current issues: - document['location'] returns <iframe name="location">, but it should return a Location object, because 'location' is a [Unforgeable] attribute in Document. - document[1] and document['1'] do not return <img name="1">. BUG= ========== to ========== Implement named property getter in HTMLDocument. This CL also removes registering V8 wrappers of named items while a browser process parses a HTML file. Current issues: - document['location'] returns <iframe name="location"> - it should return a Location object - because 'location' is a [Unforgeable] attribute in Document - 'location' is a readonly attribute, but it is possible to update it in practical behavior. - document[1] and document['1'] do not return <img name="1">. BUG= ==========
Description was changed from ========== Implement named property getter in HTMLDocument. This CL also removes registering V8 wrappers of named items while a browser process parses a HTML file. Current issues: - document['location'] returns <iframe name="location"> - it should return a Location object - because 'location' is a [Unforgeable] attribute in Document - 'location' is a readonly attribute, but it is possible to update it in practical behavior. - document[1] and document['1'] do not return <img name="1">. BUG= ========== to ========== Implement named property getter in HTMLDocument. This CL also removes registering V8 wrappers of named items while a browser process parses a HTML file. Current issues: - img.foo returns <img id="foo"> - it should return undefined - because <img> can be referred with id iff it has a non empty name. BUG= ==========
Description was changed from ========== Implement named property getter in HTMLDocument. This CL also removes registering V8 wrappers of named items while a browser process parses a HTML file. Current issues: - img.foo returns <img id="foo"> - it should return undefined - because <img> can be referred with id iff it has a non empty name. BUG= ========== to ========== Implement named property getter in HTMLDocument. This CL also removes registering V8 wrappers of named items while a browser process parses a HTML file. Spec: https://html.spec.whatwg.org/multipage/dom.html#dom-document-namedItem-which Current issues: - img.foo returns <img id="foo"> - it should return undefined - because <img> can be referred with id iff it has a non empty name. BUG= ==========
Patchset #7 (id:260001) has been deleted
Patchset #6 (id:240001) has been deleted
Patchset #6 (id:280001) has been deleted
Description was changed from ========== Implement named property getter in HTMLDocument. This CL also removes registering V8 wrappers of named items while a browser process parses a HTML file. Spec: https://html.spec.whatwg.org/multipage/dom.html#dom-document-namedItem-which Current issues: - img.foo returns <img id="foo"> - it should return undefined - because <img> can be referred with id iff it has a non empty name. BUG= ========== to ========== Implement named property getter in HTMLDocument. This CL also removes registering V8 wrappers of named items while a browser process parses a HTML file. Updated a test expectation; Chrome had been FAILing it, while IE, FF, and Safari are PASSing it. Spec: https://html.spec.whatwg.org/multipage/dom.html#dom-document-namedItem-which Current issues: - document.location returns <iframe name="location"> BUG= ==========
Patchset #7 (id:320001) has been deleted
Description was changed from ========== Implement named property getter in HTMLDocument. This CL also removes registering V8 wrappers of named items while a browser process parses a HTML file. Updated a test expectation; Chrome had been FAILing it, while IE, FF, and Safari are PASSing it. Spec: https://html.spec.whatwg.org/multipage/dom.html#dom-document-namedItem-which Current issues: - document.location returns <iframe name="location"> BUG= ========== to ========== Implement named property getter in HTMLDocument. This CL also removes registering V8 wrappers of named items while a browser process parses a HTML file. Updated a test expectation; Chrome had been FAILing it, while IE, FF, and Safari are PASSing it. Spec: https://html.spec.whatwg.org/multipage/dom.html#dom-document-namedItem-which Current issues: - document.location returns <iframe name="location"> BUG=611632 ==========
Patchset #8 (id:360001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Description was changed from ========== Implement named property getter in HTMLDocument. This CL also removes registering V8 wrappers of named items while a browser process parses a HTML file. Updated a test expectation; Chrome had been FAILing it, while IE, FF, and Safari are PASSing it. Spec: https://html.spec.whatwg.org/multipage/dom.html#dom-document-namedItem-which Current issues: - document.location returns <iframe name="location"> BUG=611632 ========== to ========== Implement named property getter in HTMLDocument. This CL also removes registering V8 wrappers of named items while a browser process parses a HTML file. Updated a test expectation; Chrome had been FAILing it, while IE, FF, and Safari are PASSing it. Spec: https://html.spec.whatwg.org/multipage/dom.html#dom-document-namedItem-which BUG=611632 ==========
Patchset #1 (id:140001) has been deleted
Patchset #1 (id:220001) has been deleted
Patchset #1 (id:300001) has been deleted
Patchset #1 (id:340001) has been deleted
Description was changed from ========== Implement named property getter in HTMLDocument. This CL also removes registering V8 wrappers of named items while a browser process parses a HTML file. Updated a test expectation; Chrome had been FAILing it, while IE, FF, and Safari are PASSing it. Spec: https://html.spec.whatwg.org/multipage/dom.html#dom-document-namedItem-which BUG=611632 ========== to ========== Add [OverrideBuiltins] label onto HTMLDocument interface. This CL also removes special handling of named properties using V8 script controller, which is no longer needed. Currently, we have no general routines to work for [Unforgeable] on [OverrideBuiltins] interface, and "location" in "HTMLDocument" is the only one which meets the situation. So now, we handle it as an edge case of named property. Spec: https://html.spec.whatwg.org/multipage/dom.html#dom-document-namedItem-which Updated a test expectation; Chrome had been FAILing it, while IE, FF, and Safari are PASSing it. BUG=611632 ==========
Patchset #1 (id:380001) has been deleted
Patchset #1 (id:400001) has been deleted
Description was changed from ========== Add [OverrideBuiltins] label onto HTMLDocument interface. This CL also removes special handling of named properties using V8 script controller, which is no longer needed. Currently, we have no general routines to work for [Unforgeable] on [OverrideBuiltins] interface, and "location" in "HTMLDocument" is the only one which meets the situation. So now, we handle it as an edge case of named property. Spec: https://html.spec.whatwg.org/multipage/dom.html#dom-document-namedItem-which Updated a test expectation; Chrome had been FAILing it, while IE, FF, and Safari are PASSing it. BUG=611632 ========== to ========== Add [OverrideBuiltins] label onto HTMLDocument interface. This CL also removes special handling of named properties using V8 script controller, which is no longer needed. Currently, we have no general routines to work for [Unforgeable] on [OverrideBuiltins] interface, and "location" in "HTMLDocument" is the only one which meets the situation. So now, we handle it as an edge case of named property. Spec: https://html.spec.whatwg.org/multipage/dom.html#dom-document-namedItem-which Updated a test expectation; Chrome has been FAILing it, while IE, FF, and Safari PASS it. BUG=611632 ==========
peria@chromium.org changed reviewers: + haraken@chromium.org, yukishiino@chromium.org
PTL. This CL follows the discussion in crbug.com/611632
LGTM. Thanks for working on this. https://codereview.chromium.org/1837823003/diff/420001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLDocumentCustom.cpp (right): https://codereview.chromium.org/1837823003/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLDocumentCustom.cpp:9: #include "bindings/core/v8/V8Location.h" Seems V8HTMLCollection.h, V8Location.h and V8Node.h are not used. https://codereview.chromium.org/1837823003/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLDocumentCustom.cpp:11: #include "core/HTMLNames.h" Not sure, but it's possible that you're not using HTMLNames.h, LocalFrame.h, etc., too. Please check them out. https://codereview.chromium.org/1837823003/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLDocumentCustom.cpp:23: v8::Local<v8::Value> getNamedProperty(HTMLDocument* htmlDocument, const AtomicString& key, v8::Local<v8::Object> creationContext, v8::Isolate* isolate) Our convention is that v8::Isolate* (or v8::Local<v8::Context>) should be the first argument. Please follow the convention. (Some of functions don't follow this rule, but it's just a historical reason, and we're lazy to rewrite all of them.) I'd recommend (isolate, htmlDocument, key, creationContext). https://codereview.chromium.org/1837823003/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLDocumentCustom.cpp:38: return toV8(frame->domWindow(), creationContext, isolate); Better to #include "ToV8.h". https://codereview.chromium.org/1837823003/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLDocumentCustom.cpp:68: // [OverrideBuiltin] interface. This is a workaround for the case. URL to the spec is helpful.
https://codereview.chromium.org/1837823003/diff/420001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLDocumentCustom.cpp (right): https://codereview.chromium.org/1837823003/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLDocumentCustom.cpp:9: #include "bindings/core/v8/V8Location.h" On 2016/05/23 08:32:43, Yuki wrote: > Seems V8HTMLCollection.h, V8Location.h and V8Node.h are not used. Done. https://codereview.chromium.org/1837823003/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLDocumentCustom.cpp:11: #include "core/HTMLNames.h" On 2016/05/23 08:32:43, Yuki wrote: > Not sure, but it's possible that you're not using HTMLNames.h, LocalFrame.h, > etc., too. Please check them out. Done. https://codereview.chromium.org/1837823003/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLDocumentCustom.cpp:23: v8::Local<v8::Value> getNamedProperty(HTMLDocument* htmlDocument, const AtomicString& key, v8::Local<v8::Object> creationContext, v8::Isolate* isolate) On 2016/05/23 08:32:43, Yuki wrote: > Our convention is that v8::Isolate* (or v8::Local<v8::Context>) should be the > first argument. Please follow the convention. (Some of functions don't follow > this rule, but it's just a historical reason, and we're lazy to rewrite all of > them.) > > I'd recommend (isolate, htmlDocument, key, creationContext). Done. https://codereview.chromium.org/1837823003/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLDocumentCustom.cpp:38: return toV8(frame->domWindow(), creationContext, isolate); On 2016/05/23 08:32:43, Yuki wrote: > Better to #include "ToV8.h". Done. https://codereview.chromium.org/1837823003/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLDocumentCustom.cpp:68: // [OverrideBuiltin] interface. This is a workaround for the case. On 2016/05/23 08:32:43, Yuki wrote: > URL to the spec is helpful. Done.
Mostly looks good. https://codereview.chromium.org/1837823003/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (left): https://codereview.chromium.org/1837823003/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:515: if (prototype->IsObject()) { Would you help me understand why you can remove this code? (Actually I don't fully understand what this old code was doing.) https://codereview.chromium.org/1837823003/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLDocumentCustom.cpp (right): https://codereview.chromium.org/1837823003/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLDocumentCustom.cpp:60: AtomicString atomicName(toCoreAtomicString(name.As<v8::String>())); I guess you need to check 'if(name->IsString())' before converting the Name to a String. See https://codereview.chromium.org/1967453002/. https://codereview.chromium.org/1837823003/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLDocumentCustom.cpp:60: AtomicString atomicName(toCoreAtomicString(name.As<v8::String>())); I want to defer the AtomicString's instantiation as much as possible (because it's heavy). Can we use V8StringResource in namedPropertyGetterCustom, indexedPropertyGetterCustom and propertyGetterCustom? https://codereview.chromium.org/1837823003/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLDocumentCustom.cpp:62: // [OverrideBuiltin] interface. This is a workaround for the case. Slightly better: [Unforgeable] attributes in an [OverrideBuiltin] interface must be looked up before any named properties (http://heycam.github.io/webidl/#dfn-named-property-visibility). So we return immediately before looking up named propertiees. TODO(peria): Support the behavior in the IDL compiler.
https://codereview.chromium.org/1837823003/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLDocumentCustom.cpp (right): https://codereview.chromium.org/1837823003/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLDocumentCustom.cpp:60: AtomicString atomicName(toCoreAtomicString(name.As<v8::String>())); On 2016/05/23 21:50:17, haraken wrote: > > I guess you need to check 'if(name->IsString())' before converting the Name to a > String. > > See https://codereview.chromium.org/1967453002/. I've already landed the following CL. https://codereview.chromium.org/1993593003/ You no longer need to check it. It's redundant here.
https://codereview.chromium.org/1837823003/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (left): https://codereview.chromium.org/1837823003/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:515: if (prototype->IsObject()) { On 2016/05/23 21:50:16, haraken wrote: > > Would you help me understand why you can remove this code? (Actually I don't > fully understand what this old code was doing.) This routine was to emulate named properties using own properties, and they behaved like real named properties because HTMLDocument was not [OverrideBuiltins]. If we keep this routine while we add [OverrideBuiltins] on HTMLDocument as spec, they will hide built-in methods, e.g. "location". JFYI, these static functions are moved to V8HTMLDocumentCustom.cpp in this CL. https://codereview.chromium.org/1837823003/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLDocumentCustom.cpp (right): https://codereview.chromium.org/1837823003/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLDocumentCustom.cpp:60: AtomicString atomicName(toCoreAtomicString(name.As<v8::String>())); Postponed the instantiation of AtomicString. In callee functions, we have no early-returns before using AtomicString, so they can receive AtomicString. https://codereview.chromium.org/1837823003/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLDocumentCustom.cpp:62: // [OverrideBuiltin] interface. This is a workaround for the case. On 2016/05/23 21:50:16, haraken wrote: > > Slightly better: > > [Unforgeable] attributes in an [OverrideBuiltin] interface must be looked up > before any named properties > (http://heycam.github.io/webidl/#dfn-named-property-visibility). So we return > immediately before looking up named propertiees. > TODO(peria): Support the behavior in the IDL compiler. Done.
LGTM The interceptor is very performance-sensitive, so let's keep watching the perf bot after landing this CL.
Thank you for such a quick response. I'll check the perf.
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 Link to the patchset: https://codereview.chromium.org/1837823003/#ps460001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1837823003/460001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1837823003/460001
Message was sent while issue was closed.
Description was changed from ========== Add [OverrideBuiltins] label onto HTMLDocument interface. This CL also removes special handling of named properties using V8 script controller, which is no longer needed. Currently, we have no general routines to work for [Unforgeable] on [OverrideBuiltins] interface, and "location" in "HTMLDocument" is the only one which meets the situation. So now, we handle it as an edge case of named property. Spec: https://html.spec.whatwg.org/multipage/dom.html#dom-document-namedItem-which Updated a test expectation; Chrome has been FAILing it, while IE, FF, and Safari PASS it. BUG=611632 ========== to ========== Add [OverrideBuiltins] label onto HTMLDocument interface. This CL also removes special handling of named properties using V8 script controller, which is no longer needed. Currently, we have no general routines to work for [Unforgeable] on [OverrideBuiltins] interface, and "location" in "HTMLDocument" is the only one which meets the situation. So now, we handle it as an edge case of named property. Spec: https://html.spec.whatwg.org/multipage/dom.html#dom-document-namedItem-which Updated a test expectation; Chrome has been FAILing it, while IE, FF, and Safari PASS it. BUG=611632 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:460001)
Message was sent while issue was closed.
Description was changed from ========== Add [OverrideBuiltins] label onto HTMLDocument interface. This CL also removes special handling of named properties using V8 script controller, which is no longer needed. Currently, we have no general routines to work for [Unforgeable] on [OverrideBuiltins] interface, and "location" in "HTMLDocument" is the only one which meets the situation. So now, we handle it as an edge case of named property. Spec: https://html.spec.whatwg.org/multipage/dom.html#dom-document-namedItem-which Updated a test expectation; Chrome has been FAILing it, while IE, FF, and Safari PASS it. BUG=611632 ========== to ========== Add [OverrideBuiltins] label onto HTMLDocument interface. This CL also removes special handling of named properties using V8 script controller, which is no longer needed. Currently, we have no general routines to work for [Unforgeable] on [OverrideBuiltins] interface, and "location" in "HTMLDocument" is the only one which meets the situation. So now, we handle it as an edge case of named property. Spec: https://html.spec.whatwg.org/multipage/dom.html#dom-document-namedItem-which Updated a test expectation; Chrome has been FAILing it, while IE, FF, and Safari PASS it. BUG=611632 Committed: https://crrev.com/09649cf329fd0644f50624b80f6a2f9c564dcb2d Cr-Commit-Position: refs/heads/master@{#395570} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/09649cf329fd0644f50624b80f6a2f9c564dcb2d Cr-Commit-Position: refs/heads/master@{#395570}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:460001) has been created in https://codereview.chromium.org/2011553003/ by peria@chromium.org. The reason for reverting is: performance regression BUG=614559.
Message was sent while issue was closed.
On 2016/05/25 07:29:21, peria wrote: > A revert of this CL (patchset #3 id:460001) has been created in > https://codereview.chromium.org/2011553003/ by mailto:peria@chromium.org. > > The reason for reverting is: performance regression > BUG=614559. This CL regressed micro benchmarks which we're tackling for 2x-3x. For example, 10^6 createElement() take 301ms-->950ms, and 10^6 createElement() + setAttribute() take 1388ms-->2841ms on my machine. These regressions are unacceptablly large. |
