Issue 2334223005: 2nd arg of document.createElement should be an object

Anton Obzhirov

Description was changed from ========== 2nd arg of document.createElement should be an object This patch ...

4 years, 3 months ago (2016-09-13 22:22:28 UTC) #1

Anton Obzhirov

a.obzhirov@samsung.com changed reviewers: + dominicc@chromium.org

4 years, 3 months ago (2016-09-13 22:22:28 UTC) #2

dominicc (has gone to gerrit)

Thanks for working on this! This is shaping up very nicely. Could you update the ...

4 years, 3 months ago (2016-09-15 01:19:24 UTC) #3

Thanks for working on this! This is shaping up very nicely.

Could you update the layout tests to pass a hash, and just put one test that
passing a string still works?

Put a TODO somewhere (maybe on ElementCreationOptionsOrString) to replace it
with ElementCreationOptions when passing a string is removed, and mention the
use counter you're adding.

https://codereview.chromium.org/2334223005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2334223005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.cpp:684: if
(optionsOrString.isString() && !optionsOrString.getAsString().isEmpty()) {
I wonder if the existing code does toString on non-string arguments. Could you
check, maybe by writing tests, with an object argument that does not have
extends but toString returns something to extend?

For example, I think this alerts true, but your change will send it down the
'is' path:

http://jsbin.com/mikuyomogu/edit?html,output

<!DOCTYPE html>
<script>
FooBar = document.registerElement('foo-bar', {extends: 'button'});
var elem = document.createElement('button', {toString: () => 'foo-bar'});
alert(elem.constructor === FooBar);
</script>

https://codereview.chromium.org/2334223005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/Deprecation.cpp (right):

https://codereview.chromium.org/2334223005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/Deprecation.cpp:379: case
UseCounter::DocumentCreateElement2ndArg:
Maybe give this a more descriptive name, because createElement having a second
argument isn't deprecated; it's the future! Maybe mention ...StringHandling or
something in the name.

https://codereview.chromium.org/2334223005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/Deprecation.cpp:380: return
"Document.createElement second argument string will be replaced in near future
with the dictionary. See
https://dom.spec.whatwg.org/#dom-document-createelement.";
Maybe make this more explicit: Change to passing a dictionary with 'is' *now*,
because that will work, and passing a string will stop being supported soon.

Yuki

yukishiino@chromium.org changed reviewers: + yukishiino@chromium.org

4 years, 3 months ago (2016-09-15 08:01:38 UTC) #4

Yuki

+cc: bashi@ https://codereview.chromium.org/2334223005/diff/1/third_party/WebKit/Source/core/dom/Document.idl File third_party/WebKit/Source/core/dom/Document.idl (right): https://codereview.chromium.org/2334223005/diff/1/third_party/WebKit/Source/core/dom/Document.idl#newcode178 third_party/WebKit/Source/core/dom/Document.idl:178: [CustomElementCallbacks, PerWorldBindings, RaisesException] Element createElement(DOMString localName, (ElementCreationOptions ...

4 years, 3 months ago (2016-09-15 08:01:39 UTC) #5

Anton Obzhirov

Hi, thanks for checking, will address all the issues. https://codereview.chromium.org/2334223005/diff/1/third_party/WebKit/Source/core/dom/Document.cpp File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2334223005/diff/1/third_party/WebKit/Source/core/dom/Document.cpp#newcode684 third_party/WebKit/Source/core/dom/Document.cpp:684: ...

4 years, 3 months ago (2016-09-15 09:25:20 UTC) #6

Anton Obzhirov

On 2016/09/15 09:25:20, Anton Obzhirov wrote: > Hi, thanks for checking, will address all the ...

4 years, 3 months ago (2016-09-16 19:10:59 UTC) #7

Anton Obzhirov

On 2016/09/16 19:10:59, Anton Obzhirov wrote: > On 2016/09/15 09:25:20, Anton Obzhirov wrote: > > ...

4 years, 3 months ago (2016-09-19 20:20:44 UTC) #8

On 2016/09/16 19:10:59, Anton Obzhirov wrote:
> On 2016/09/15 09:25:20, Anton Obzhirov wrote:
> > Hi, thanks for checking, will address all the issues.
> > 
> >
>
https://codereview.chromium.org/2334223005/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/dom/Document.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2334223005/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/dom/Document.cpp:684: if
> > (optionsOrString.isString() && !optionsOrString.getAsString().isEmpty()) {
> > On 2016/09/15 01:19:24, dominicc wrote:
> > > I wonder if the existing code does toString on non-string arguments. Could
> you
> > > check, maybe by writing tests, with an object argument that does not have
> > > extends but toString returns something to extend?
> > > 
> > > For example, I think this alerts true, but your change will send it down
the
> > > 'is' path:
> > > 
> > > http://jsbin.com/mikuyomogu/edit?html,output
> > > 
> > > <!DOCTYPE html>
> > > <script>
> > > FooBar = document.registerElement('foo-bar', {extends: 'button'});
> > > var elem = document.createElement('button', {toString: () => 'foo-bar'});
> > > alert(elem.constructor === FooBar);
> > > </script>
> > 
> > Will check, good point, will need to figure out how to handle this case :)
> 
> Quick update about toString conversion. So existing code does call toString on
> non-string elements.
> With the change it goes down to Object 'is' path and since the dictionary
> doesn't have 'is' it doesn't set the element extension. So 
> alert(elem.constructor === FooBar) returns fail. Which will fail probably some
> legacy tests. I will try to find a solution for this.

The solution I've found so far is to define 
[CallWith=ScriptState, CustomElementCallbacks, PerWorldBindings,
RaisesException] Element createElement(DOMString localName, (DOMString or
Dictionary) options);
and in case of Dictionary get 'is' property or call 'toString'. I couldn’t find
way to customize in IDL file so if it is not ElementCreationOptions call
toString on the dictionary . If there is one plz let me know.

Anton Obzhirov

https://codereview.chromium.org/2334223005/diff/1/third_party/WebKit/Source/core/dom/Document.idl File third_party/WebKit/Source/core/dom/Document.idl (right): https://codereview.chromium.org/2334223005/diff/1/third_party/WebKit/Source/core/dom/Document.idl#newcode178 third_party/WebKit/Source/core/dom/Document.idl:178: [CustomElementCallbacks, PerWorldBindings, RaisesException] Element createElement(DOMString localName, (ElementCreationOptions or DOMString) ...

4 years, 3 months ago (2016-09-21 15:05:12 UTC) #10

Anton Obzhirov

Hi guys, plz check the new patch. I've changed the approach to support toString case ...

4 years, 3 months ago (2016-09-21 18:16:13 UTC) #11

dominicc (has gone to gerrit)

The CQ bit was checked by dominicc@chromium.org to run a CQ dry run

4 years, 2 months ago (2016-09-29 01:17:44 UTC) #12

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2334223005/20001

4 years, 2 months ago (2016-09-29 01:18:23 UTC) #13

dominicc (has gone to gerrit)

This LGTM. Thanks for working on this! There's one more thing I want to ensure, ...

4 years, 2 months ago (2016-09-29 01:20:57 UTC) #14

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 2 months ago (2016-09-29 01:22:00 UTC) #15

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clang_dbg_recipe/builds/136659) android_n5x_swarming_rel on ...

4 years, 2 months ago (2016-09-29 01:22:01 UTC) #16

Anton Obzhirov

Hi, thanks for the review. Will rebase the patch, address the comments and check the ...

4 years, 2 months ago (2016-09-29 13:48:25 UTC) #18

haraken

On 2016/09/29 13:48:25, Anton Obzhirov wrote: > Hi, thanks for the review. > Will rebase ...

4 years, 2 months ago (2016-09-29 16:16:12 UTC) #19

Anton Obzhirov

The CQ bit was checked by a.obzhirov@samsung.com to run a CQ dry run

4 years, 2 months ago (2016-09-30 12:05:20 UTC) #20

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2334223005/40001

4 years, 2 months ago (2016-09-30 12:05:31 UTC) #21

Anton Obzhirov

a.obzhirov@samsung.com changed reviewers: + jochen@chromium.org

4 years, 2 months ago (2016-09-30 12:11:11 UTC) #22

Anton Obzhirov

Hi, I've added the deprecation warning to some test expectations to pass the tests. Will ...

4 years, 2 months ago (2016-09-30 12:11:12 UTC) #23

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 2 months ago (2016-09-30 13:39:25 UTC) #24

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 2 months ago (2016-09-30 13:39:26 UTC) #25

dominicc (has gone to gerrit)

The CQ bit was checked by dominicc@chromium.org

4 years, 2 months ago (2016-10-03 06:01:48 UTC) #26

dominicc (has gone to gerrit)

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, dominicc@chromium.org Link to ...

4 years, 2 months ago (2016-10-03 06:01:49 UTC) #27

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2334223005/40001

4 years, 2 months ago (2016-10-03 06:02:01 UTC) #28

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 2 months ago (2016-10-03 06:11:38 UTC) #29

commit-bot: I haz the power

Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/271722)

4 years, 2 months ago (2016-10-03 06:11:39 UTC) #30

jochen (gone - plz use gerrit)

jochen@chromium.org changed reviewers: + haraken@chromium.org

4 years, 2 months ago (2016-10-04 14:44:37 UTC) #31

jochen (gone - plz use gerrit)

could we do an intent to ship anyways? it's easier for everybody to track those ...

4 years, 2 months ago (2016-10-04 14:44:38 UTC) #32

Anton Obzhirov

On 2016/10/04 14:44:38, jochen (slow) wrote: > could we do an intent to ship anyways? ...

4 years, 2 months ago (2016-10-05 09:25:16 UTC) #33

Anton Obzhirov

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, dominicc@chromium.org Link to ...

4 years, 1 month ago (2016-10-27 17:57:44 UTC) #35

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2334223005/60001

4 years, 1 month ago (2016-10-27 17:58:22 UTC) #36

commit-bot: I haz the power

Description was changed from ========== 2nd arg of document.createElement should be an object This patch ...

4 years, 1 month ago (2016-10-27 19:33:53 UTC) #37

commit-bot: I haz the power

Description was changed from ========== 2nd arg of document.createElement should be an object This patch ...

4 years, 1 month ago (2016-10-27 20:00:07 UTC) #39

commit-bot: I haz the power

4 years, 1 month ago (2016-10-27 20:00:08 UTC) #40

Message was sent while issue was closed.

Patchset 4 (id:??) landed as
https://crrev.com/fc17d505758c284f99624ac69b6e2fb72ea7b623
Cr-Commit-Position: refs/heads/master@{#428111}

Issue 2334223005: 2nd arg of document.createElement should be an object (Closed)

Description

Patch Set 1 #

Patch Set 2 : Next iteration, added new test and slightly different approach. #

Patch Set 3 : Updated after review + added warning in the tests expectations." #

Patch Set 4 : Remove deprecation warning for now. #

Messages