Bug 91704 – ShadowRoot.cloneNode() must always throw a DATA_CLONE_ERR exception.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Shinya Kawanaka]

According to the spec, ShadowRoot.cloneNode() must always throw a DATA_CLONE_ERR exception.
Currently NULL is returned.

[Shinya Kawanaka]

In DOM Level 2, cloneNode() does not throw an exception. 
In DOM 4, I'm not sure cloneNode() can throw an exception.

http://www.w3.org/TR/domcore/#dom-domexception-data_clone_err

But under the current implementation, we have to change all cloneNode() signature in various nodes, since nodeNode() does not have ExceptionCode&, maybe...

[Shinya Kawanaka]

Created attachment 159001 [details]
Patch

[Shinya Kawanaka]

Created attachment 159025 [details]
Patch

[Hajime Morrita]

Comment on attachment 159025 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159025&action=review

> LayoutTests/fast/dom/shadow/shadowroot-clonenode.html:6
> +<script>

Needs description() here.

> LayoutTests/fast/dom/shadow/shadowroot-clonenode.html:17
> +var successfullyParsed = true

finishJSTest() instead of "successfullyParsed = true" is the latest trend.

[Shinya Kawanaka]

Created attachment 159317 [details]
Patch

[WebKit Review Bot]

Comment on attachment 159317 [details]
Patch

Attachment 159317 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13528791

New failing tests:
http/tests/cache/post-redirect-get.php
http/tests/cache/post-with-cached-subresources.php

[WebKit Review Bot]

Created attachment 159322 [details]
Archive of layout-test-results from gce-cr-linux-07

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Shinya Kawanaka]

I don't think these failing tests are related to my patch.

[WebKit Review Bot]

Comment on attachment 159317 [details]
Patch

Clearing flags on attachment: 159317

Committed r126127: <http://trac.webkit.org/changeset/126127>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Xan Lopez]

(In reply to comment #1)
> In DOM Level 2, cloneNode() does not throw an exception. 
> In DOM 4, I'm not sure cloneNode() can throw an exception.
> 
> http://www.w3.org/TR/domcore/#dom-domexception-data_clone_err
> 
> But under the current implementation, we have to change all cloneNode() signature in various nodes, since nodeNode() does not have ExceptionCode&, maybe...

Sorry if this is stupid, but if this change is not reflected in the spec is it OK for us to just go ahead and change it?

This patch broke the GObject DOM bindings, so I'm trying to figure out how to solve that, and I'd like to understand why we think this change makes sense.

[Hajime Morrita]

> 
> Sorry if this is stupid, but if this change is not reflected in the spec is it OK for us to just go ahead and change it?
> 
> This patch broke the GObject DOM bindings, so I'm trying to figure out how to solve that, and I'd like to understand why we think this change makes sense.

I'm sorry to hear that. I wasn't aware this change affects GObject bindings.
In JS, it matters less what signature says and what matters is that
it actually throws an exception. This is due to JS's dynamic nature.
And I was thinking only about JS while reviewing this change.
This is why I thought this change made sense.

Considering GObject, which bakes exception type into the signature and 
has no overload mechanism, it might be better not to change the signature.

I'll talk with some binding expert how to deal with this.
Let me revert until for now.

[Hajime Morrita]

Reverted at http://trac.webkit.org/changeset/127228
Need to figure out gobject-friendly way to do this.

[Xan Lopez]

(In reply to comment #12)
> I'm sorry to hear that. I wasn't aware this change affects GObject bindings.
> In JS, it matters less what signature says and what matters is that
> it actually throws an exception. This is due to JS's dynamic nature.
> And I was thinking only about JS while reviewing this change.
> This is why I thought this change made sense.
> 
> Considering GObject, which bakes exception type into the signature and 
> has no overload mechanism, it might be better not to change the signature.

Hey,

thanks a lot. I wasn't necessarily asking to have this reverted, although in the short term it surely makes my life easier. As you well say the issue is that while this change does not change the JS API it does change the GObject one, since in GObject exceptions are implemented by having GError parameters.

If we really want this change in the IDL I think we can go ahead and do it, I'd just need to have a custom cloneNode method that calls the new C++ API (so basically #ifdef out the IDL method for GObject). My question was more along the lines of: do we really want to diverge from the upstream spec? Since it does not seem to contain this change.

> 
> I'll talk with some binding expert how to deal with this.
> Let me revert until for now.

Great. Please keep me on the loop, I should be able to comment on whether any change will have a negative impact in our port.

[Dimitri Glazkov (Google)]

(In reply to comment #14)
> (In reply to comment #12)
> > I'm sorry to hear that. I wasn't aware this change affects GObject bindings.
> > In JS, it matters less what signature says and what matters is that
> > it actually throws an exception. This is due to JS's dynamic nature.
> > And I was thinking only about JS while reviewing this change.
> > This is why I thought this change made sense.
> > 
> > Considering GObject, which bakes exception type into the signature and 
> > has no overload mechanism, it might be better not to change the signature.
> 
> Hey,
> 
> thanks a lot. I wasn't necessarily asking to have this reverted, although in the short term it surely makes my life easier. As you well say the issue is that while this change does not change the JS API it does change the GObject one, since in GObject exceptions are implemented by having GError parameters.
> 
> If we really want this change in the IDL I think we can go ahead and do it, I'd just need to have a custom cloneNode method that calls the new C++ API (so basically #ifdef out the IDL method for GObject). My question was more along the lines of: do we really want to diverge from the upstream spec? Since it does not seem to contain this change.

I'll ask Anne to make the change to DOM Core spec to add the throws exception clause to cloneNode().

[Dimitri Glazkov (Google)]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #12)
> > > I'm sorry to hear that. I wasn't aware this change affects GObject bindings.
> > > In JS, it matters less what signature says and what matters is that
> > > it actually throws an exception. This is due to JS's dynamic nature.
> > > And I was thinking only about JS while reviewing this change.
> > > This is why I thought this change made sense.
> > > 
> > > Considering GObject, which bakes exception type into the signature and 
> > > has no overload mechanism, it might be better not to change the signature.
> > 
> > Hey,
> > 
> > thanks a lot. I wasn't necessarily asking to have this reverted, although in the short term it surely makes my life easier. As you well say the issue is that while this change does not change the JS API it does change the GObject one, since in GObject exceptions are implemented by having GError parameters.
> > 
> > If we really want this change in the IDL I think we can go ahead and do it, I'd just need to have a custom cloneNode method that calls the new C++ API (so basically #ifdef out the IDL method for GObject). My question was more along the lines of: do we really want to diverge from the upstream spec? Since it does not seem to contain this change.
> 
> I'll ask Anne to make the change to DOM Core spec to add the throws exception clause to cloneNode().

I was confused. Web IDL doesn't even have "throws exception" clause :). Since this is not really a spec question, I don't see why we need to bother anyone with this.

[Hajime Morrita]

Yeah, this is more API compatiblity issue and what we need to do is
keeping GObject API compatible. My optimistic expectation is that
we can do this by tweaking the IDL compiler a bit.

[Hajime Morrita]

Created attachment 162208 [details]
Patch

[Kentaro Hara]

Comment on attachment 162208 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162208&action=review

> Source/WebCore/dom/ShadowRoot.idl:42
> +#if defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT

I'm not sure if this is the best way to keep the GObject API compatible, but it looks reasonable.

> LayoutTests/fast/dom/shadow/shadowroot-clonenode.html:21
> +var exceptionCode = null;
> +try {
> +    shadowRoot.cloneNode()
> +} catch (e) {
> +    exceptionCode = e.code;
> +}
> +
> +shouldBe('exceptionCode', 'DOMException.DATA_CLONE_ERR')

You can use shouldThrow('shadowRoot.cloneNode()', ...).

> LayoutTests/fast/dom/shadow/shadowroot-clonenode.html:23
> +finishJSTest();

Remove this.

[Hajime Morrita]

Created attachment 162211 [details]
Patch for landing

[Hajime Morrita]

Hi Haraken, thanks for taking a look!
> > Source/WebCore/dom/ShadowRoot.idl:42
> > +#if defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT
Me either. Any suggestions are welcome.

[Xan Lopez]

As I said in comment #14 if we really want to change the signature of the base Node method we can do so, we just need to provide a custom wrapper for GObject in WebCore/bindings/gobject/WebKitDOMCustom.[cpp,h] and #ifdef out the IDL method for GObject. My only concern there was whether it is OK to do changes to the IDL files not reflected in the specs.

As for the current patch, that seems fine as far as GObject is concerned of course. Not sure either whether it's the best approach (seems simpler than the previous patch though?).

[WebKit Review Bot]

Comment on attachment 162211 [details]
Patch for landing

Clearing flags on attachment: 162211

Committed r127580: <http://trac.webkit.org/changeset/127580>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.