Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2161003002:
CustomElements: upgrade element patch (Closed)
|
Created:
4 years, 5 months ago by davaajav Modified:
4 years, 4 months ago CC:
blink-reviews, blink-reviews-bindings_chromium.org, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@upgrade-element Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCustomElements: upgrade element patch, setting custom element
state at the right time
HTMLConstructor should change status to "custom" only when the
definitions construction stack is empty, for example, when the
constructor is invoked with new.
If return value from the constructor is different from the
custom element, upgrade should throw "InvalidStateError"
DOMException.
BUG=594918
Committed: https://crrev.com/d41cf519e902ba24e51317004dc36e14af64fc50
Cr-Commit-Position: refs/heads/master@{#409445}
Patch Set 1 #Patch Set 2 : deleted comment #Patch Set 3 : set state to custom after upgrade #Patch Set 4 : workflow commit #Patch Set 5 : test for step 8 #Patch Set 6 : delete failure expectation #Patch Set 7 : workflow commit #
Total comments: 12
Patch Set 8 : patch update #
Total comments: 5
Patch Set 9 : workflow commit #Patch Set 10 : workflow commit #Patch Set 11 : RegistryTest update, removed setting element`s state to custom in the test #Messages
Total messages: 37 (15 generated)
Description was changed from ========== CustomElements: upgrade element C++ patch HTMLConstructor should change status to "custom" only when the definitions construction stack is empty, for example, when the constructor is invoked with new. If that is the case an element`s state is modified in the following order: uncustomized -> undefined -> custom. BUG=594918 ========== to ========== CustomElements: upgrade element C++ patch HTMLConstructor should change status to "custom" only when the definitions construction stack is empty, for example, when the constructor is invoked with new. If return value from the constructor is different from the custom element, upgrade should throw "InvalidStateError" DOMException. BUG=594918 ==========
davaajav@google.com changed reviewers: + dominicc@chromium.org, kojii@chromium.org
PTL
dominicc@chromium.org changed reviewers: + domenic@chromium.org
Good description. Could you change the first line though? Don't mention "C++", probably 95%+ of patches are C++ so that's not so important. Mention custom elements and setting the custom state at the right time. Some comments inline. +domenic, you might enjoy reviewing this! You know the subtlety with which constructor behavior depends on new vs upgrade; davaajav's tests are interesting illustrations of this. https://codereview.chromium.org/2161003002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/custom-elements/spec/upgrade-element-expected.txt (left): https://codereview.chromium.org/2161003002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/upgrade-element-expected.txt:1: This is a testharness.js-based test. Yay. https://codereview.chromium.org/2161003002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/custom-elements/spec/upgrade-element.html (right): https://codereview.chromium.org/2161003002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/upgrade-element.html:35: // Unlike calling new, upgrading element with createElement does not set element's state Great comment. https://codereview.chromium.org/2161003002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/upgrade-element.html:39: assert_equals(changedCallbackArgs.length, 1, 'attributeChangedCallback should invoke only once'); invoke only once -> only be invoked once https://codereview.chromium.org/2161003002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/upgrade-element.html:40: assert_array_equals(invocations[0], ['constructor', a], 'consturctor should execute first'); Spelling: constructor https://codereview.chromium.org/2161003002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/upgrade-element.html:46: // Case when constructor is invoked with new Maybe mention "Step 6 case when ..." If someone comes in and writes a new test or reorders these, we don't want them to move this one away from the one above it. https://codereview.chromium.org/2161003002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/upgrade-element.html:68: assert_array_equals(invocations[0], ['attributeChanged', a], 'attributeChangedCallback non-conformantly executes before the constructor is finished'); I wouldn't say "non-conformantly" here. It's the *constructor* that is non-conformant, but invoking the callback *is* conformant. I would just say 'the attributeChangedCallback for "a" should execute before the constructor is finished'. https://codereview.chromium.org/2161003002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/upgrade-element.html:70: assert_array_equals(invocations[2], ['attributeChanged', a], 'invoked by setAttribute outside of the constructor'); Use 'should' in these descriptions to describe what should be happening. https://codereview.chromium.org/2161003002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/upgrade-element.html:73: }, 'The constructor non-conformatly uses API decorated with the [CEReactions] when consturctor is invoked with new'); This, and the test above, have great names. It is good to just vary the name at the end 'with new'/'with upgrade' like you have here; it makes it clear how they're very similar but just different in this one respect. https://codereview.chromium.org/2161003002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp (right): https://codereview.chromium.org/2161003002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:247: ExecutionContext* executionContext = m_scriptState->getExecutionContext(); Could you move this line closer to where it is used? https://codereview.chromium.org/2161003002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:254: v8::Local<v8::Value> data = tryCatch.Exception(); I don't think "data" is a very descriptive name. Maybe just call tryCatch.Exception() inline? https://codereview.chromium.org/2161003002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp (right): https://codereview.chromium.org/2161003002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp:65: element->setCustomElementState(CustomElementState::Custom); I would change the setCustomElementState assertions to allow uncustomized->custom transitions. If you want to do it in a follow-up patch, that is OK, but in that case put a TODO on the line above so we remember to do that. https://codereview.chromium.org/2161003002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp (right): https://codereview.chromium.org/2161003002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp:127: // CHECK(element->getCustomElementState() == CustomElementState::Custom); Delete this line.
Description was changed from ========== CustomElements: upgrade element C++ patch HTMLConstructor should change status to "custom" only when the definitions construction stack is empty, for example, when the constructor is invoked with new. If return value from the constructor is different from the custom element, upgrade should throw "InvalidStateError" DOMException. BUG=594918 ========== to ========== CustomElements: upgrade element patch, setting custom element state at the right time HTMLConstructor should change status to "custom" only when the definitions construction stack is empty, for example, when the constructor is invoked with new. If return value from the constructor is different from the custom element, upgrade should throw "InvalidStateError" DOMException. BUG=594918 ==========
PTL
LGTM with a couple of nits inline. https://codereview.chromium.org/2161003002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/custom-elements/spec/upgrade-element.html (right): https://codereview.chromium.org/2161003002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/upgrade-element.html:94: console.log(error_log.join(',')); We can remove this console.log now. https://codereview.chromium.org/2161003002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp (right): https://codereview.chromium.org/2161003002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/custom/V8HTMLElementCustom.cpp:65: element->setCustomElementState(CustomElementState::Custom); Could we put a TODO here to just write this as one call to setCustomElementState instead of two?
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/2161003002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp (right): https://codereview.chromium.org/2161003002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:255: executionContext->reportException(event, NotSharableCrossOrigin); Do we really need to report the exception synchronously? This is the only place we're doing that (outside error event handlers).
On 2016/07/22 at 09:31:53, haraken wrote: > https://codereview.chromium.org/2161003002/diff/140001/third_party/WebKit/Sou... > File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp (right): > > https://codereview.chromium.org/2161003002/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:255: executionContext->reportException(event, NotSharableCrossOrigin); > > Do we really need to report the exception synchronously? This is the only place we're doing that (outside error event handlers). I think the spec says to do this synchronously. Domenic?
On 2016/07/22 at 09:33:16, dominicc wrote: > On 2016/07/22 at 09:31:53, haraken wrote: > > Do we really need to report the exception synchronously? This is the only place we're doing that (outside error event handlers). > > I think the spec says to do this synchronously. Domenic? The spec does indeed say to do this synchronously. This is modeled after event dispatch, which also reports the exception synchronously... http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4342
Weren't we using setVerbose to"Report an exception"? What's different here?
On 2016/07/26 at 02:39:28, kojii wrote: > Weren't we using setVerbose to"Report an exception"? What's different here? We realized there's a subtlety to setVerbose: It only reports if the exception originates from within *JavaScript*. You actually have to dig into execution.cc and see that there's a ReportPendingMessages call--I think object creation doesn't call this, for example, but function calling does.
I looked at other implementations that use ErrorEvents and realized that it's common to dispatch ErrorEvents synchronously. LGTM. https://codereview.chromium.org/2161003002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp (right): https://codereview.chromium.org/2161003002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:238: const String& message = "custom element constructors must call super() first and must " I want to create a helper method that runs line 238 - 255. ScriptCustomElementDefinition::fireErrorEvent() or something.
https://codereview.chromium.org/2161003002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp (right): https://codereview.chromium.org/2161003002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:253: V8ErrorHandler::storeExceptionOnErrorEventWrapper(m_scriptState.get(), event, tryCatch.Exception(), m_scriptState->context()->Global()); BTW, is this line needed? e.g., Lazy event listeners are not setting the exception on their error events. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...
The spec says to throw, catch and report the exception so I think it is preferable to do it. I realize this way of setting the property on the wrapper looks a bit hacky but I think this is the way it is done. If there was a helper for this we'd love to use that though. On Jul 26, 2016 8:46 PM, <haraken@chromium.org> wrote: > > > https://codereview.chromium.org/2161003002/diff/140001/third_party/WebKit/Sou... > File > > third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp > (right): > > > https://codereview.chromium.org/2161003002/diff/140001/third_party/WebKit/Sou... > > third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:253: > V8ErrorHandler::storeExceptionOnErrorEventWrapper(m_scriptState.get(), > event, tryCatch.Exception(), m_scriptState->context()->Global()); > > BTW, is this line needed? > > e.g., Lazy event listeners are not setting the exception on their error > events. > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/... > > https://codereview.chromium.org/2161003002/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
The spec says to throw, catch and report the exception so I think it is preferable to do it. I realize this way of setting the property on the wrapper looks a bit hacky but I think this is the way it is done. If there was a helper for this we'd love to use that though. On Jul 26, 2016 8:46 PM, <haraken@chromium.org> wrote: > > > https://codereview.chromium.org/2161003002/diff/140001/third_party/WebKit/Sou... > File > > third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp > (right): > > > https://codereview.chromium.org/2161003002/diff/140001/third_party/WebKit/Sou... > > third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:253: > V8ErrorHandler::storeExceptionOnErrorEventWrapper(m_scriptState.get(), > event, tryCatch.Exception(), m_scriptState->context()->Global()); > > BTW, is this line needed? > > e.g., Lazy event listeners are not setting the exception on their error > events. > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/... > > https://codereview.chromium.org/2161003002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by dominicc@chromium.org to run a CQ dry run
Sending this to trybots...
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_gyp_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gyp_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
PTL
The CQ bit was checked by dominicc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, dominicc@chromium.org Link to the patchset: https://codereview.chromium.org/2161003002/#ps180001 (title: "workflow commit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
eHi Davaa, It looks like this might have broke one of our C++ unit tests, so the bots rejected it. This is actually a good and normal thing--you can examine what happened at your own pace. You can try to reproduce the failure locally by building: ninja -C out/Default webkit_unit_tests And then run out/Default/webkit_unit_tests --gtest_filter=CustomElementsRegistryFrameTest.* If you want, you can debug tests with gdb, just start gdb and do file /path/to/out/Default/webkit_unit_tests set args --gtest_filter=CustomElementsRegistryFrameTest.attributeChangedCallback run There's no need to attach, signal SIGUSR1, etc. You can 'run' to restart; you only need to file and set args once. You can look through the bot log to see whichever tests failed, or run them locally (leave out the gtest_filter flag to run all of the tests) and see if any fail. After this you will have seen a bit of our C++ unit test code. Feel free to browse around. C++ unit tests are a bit harder to write than layout tests, because you need to pull together just the parts of Chrome that you need, and sometimes that's a considerable subset of a whole browser! (We have a set of tests called browsertests which basically do this!) But it is a very powerful way to test the internals of blink, because you've got access to all of the native objects right there. They're also quite fast because you can just run the parts you need--we have a lot of custom elements tests which don't even start the JavaScript engine, for example. (Relatively) lean and mean. If you are writing C++ code in future and want to test it with C++ unit test, go right ahead. I expect you to have lots of questions. By the way, you will have noticed people are CCing you on bugs and assigning bugs to you. This is great! It's a sign people recognize you're an expert in a certain area. Don't feel pressure about having bugs assigned to you; most people on the project have a ton of bugs assigned to them (look at crbug.com and search for owner:dominicc@chromium.org or owner:kojii@chromium.org for example.) Being bug owner is really recognition that you're the most competent person to handle a particular bug, but anyone can take it, but they may want to ask your advice first. It's a good thing! Dominic ---------- Forwarded message ---------- From: commit-bot@chromium.org via codereview.chromium.org < reply@chromiumcodereview-hr.appspotmail.com> Date: Mon, Aug 1, 2016 at 9:16 PM Subject: Re: CustomElements: upgrade element C++ patch (issue 2161003002 by davaajav@google.com) To: davaajav@google.com, domenic@chromium.org, dominicc@chromium.org, haraken@chromium.org, kojii@chromium.org, commit-bot@chromium.org Cc: blink-reviews@chromium.org, blink-reviews-bindings@chromium.org, chromium-reviews@chromium.org Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... ) https://codereview.chromium.org/2161003002/ -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
eHi Davaa, It looks like this might have broke one of our C++ unit tests, so the bots rejected it. This is actually a good and normal thing--you can examine what happened at your own pace. You can try to reproduce the failure locally by building: ninja -C out/Default webkit_unit_tests And then run out/Default/webkit_unit_tests --gtest_filter=CustomElementsRegistryFrameTest.* If you want, you can debug tests with gdb, just start gdb and do file /path/to/out/Default/webkit_unit_tests set args --gtest_filter=CustomElementsRegistryFrameTest.attributeChangedCallback run There's no need to attach, signal SIGUSR1, etc. You can 'run' to restart; you only need to file and set args once. You can look through the bot log to see whichever tests failed, or run them locally (leave out the gtest_filter flag to run all of the tests) and see if any fail. After this you will have seen a bit of our C++ unit test code. Feel free to browse around. C++ unit tests are a bit harder to write than layout tests, because you need to pull together just the parts of Chrome that you need, and sometimes that's a considerable subset of a whole browser! (We have a set of tests called browsertests which basically do this!) But it is a very powerful way to test the internals of blink, because you've got access to all of the native objects right there. They're also quite fast because you can just run the parts you need--we have a lot of custom elements tests which don't even start the JavaScript engine, for example. (Relatively) lean and mean. If you are writing C++ code in future and want to test it with C++ unit test, go right ahead. I expect you to have lots of questions. By the way, you will have noticed people are CCing you on bugs and assigning bugs to you. This is great! It's a sign people recognize you're an expert in a certain area. Don't feel pressure about having bugs assigned to you; most people on the project have a ton of bugs assigned to them (look at crbug.com and search for owner:dominicc@chromium.org or owner:kojii@chromium.org for example.) Being bug owner is really recognition that you're the most competent person to handle a particular bug, but anyone can take it, but they may want to ask your advice first. It's a good thing! Dominic ---------- Forwarded message ---------- From: commit-bot@chromium.org via codereview.chromium.org < reply@chromiumcodereview-hr.appspotmail.com> Date: Mon, Aug 1, 2016 at 9:16 PM Subject: Re: CustomElements: upgrade element C++ patch (issue 2161003002 by davaajav@google.com) To: davaajav@google.com, domenic@chromium.org, dominicc@chromium.org, haraken@chromium.org, kojii@chromium.org, commit-bot@chromium.org Cc: blink-reviews@chromium.org, blink-reviews-bindings@chromium.org, chromium-reviews@chromium.org Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... ) https://codereview.chromium.org/2161003002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by davaajav@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, dominicc@chromium.org Link to the patchset: https://codereview.chromium.org/2161003002/#ps200001 (title: "RegistryTest update, removed setting element`s state to custom in the test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== CustomElements: upgrade element patch, setting custom element state at the right time HTMLConstructor should change status to "custom" only when the definitions construction stack is empty, for example, when the constructor is invoked with new. If return value from the constructor is different from the custom element, upgrade should throw "InvalidStateError" DOMException. BUG=594918 ========== to ========== CustomElements: upgrade element patch, setting custom element state at the right time HTMLConstructor should change status to "custom" only when the definitions construction stack is empty, for example, when the constructor is invoked with new. If return value from the constructor is different from the custom element, upgrade should throw "InvalidStateError" DOMException. BUG=594918 Committed: https://crrev.com/d41cf519e902ba24e51317004dc36e14af64fc50 Cr-Commit-Position: refs/heads/master@{#409445} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/d41cf519e902ba24e51317004dc36e14af64fc50 Cr-Commit-Position: refs/heads/master@{#409445} |
