Bug 91704

Summary: ShadowRoot.cloneNode() must always throw a DATA_CLONE_ERR exception.
Product: WebKit Reporter: Shinya Kawanaka <shinyak>
Component: DOMAssignee: Shinya Kawanaka <shinyak>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, cmarcelo, dglazkov, dominicc, haraken, hayato, morrita, ojan, tasak, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 94568    
Bug Blocks: 72352    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from gce-cr-linux-07
none
Patch
none
Patch for landing none

Description Shinya Kawanaka 2012-07-18 19:19:17 PDT
According to the spec, ShadowRoot.cloneNode() must always throw a DATA_CLONE_ERR exception.
Currently NULL is returned.
Comment 1 Shinya Kawanaka 2012-07-18 19:59:03 PDT
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...
Comment 2 Shinya Kawanaka 2012-08-16 22:40:17 PDT
Created attachment 159001 [details]
Patch
Comment 3 Shinya Kawanaka 2012-08-17 00:26:35 PDT
Created attachment 159025 [details]
Patch
Comment 4 Hajime Morrita 2012-08-17 00:48:34 PDT
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.
Comment 5 Shinya Kawanaka 2012-08-19 18:47:43 PDT
Created attachment 159317 [details]
Patch
Comment 6 WebKit Review Bot 2012-08-19 20:00:42 PDT
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
Comment 7 WebKit Review Bot 2012-08-19 20:00:47 PDT
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
Comment 8 Shinya Kawanaka 2012-08-20 00:16:33 PDT
I don't think these failing tests are related to my patch.
Comment 9 WebKit Review Bot 2012-08-20 20:22:22 PDT
Comment on attachment 159317 [details]
Patch

Clearing flags on attachment: 159317

Committed r126127: <http://trac.webkit.org/changeset/126127>
Comment 10 WebKit Review Bot 2012-08-20 20:22:28 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Xan Lopez 2012-08-27 11:17:47 PDT
(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.
Comment 12 Hajime Morrita 2012-08-30 18:04:06 PDT
> 
> 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.
Comment 13 Hajime Morrita 2012-08-30 21:40:27 PDT
Reverted at http://trac.webkit.org/changeset/127228
Need to figure out gobject-friendly way to do this.
Comment 14 Xan Lopez 2012-08-31 04:51:07 PDT
(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.
Comment 15 Dimitri Glazkov (Google) 2012-08-31 08:45:02 PDT
(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().
Comment 16 Dimitri Glazkov (Google) 2012-08-31 09:11:47 PDT
(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.
Comment 17 Hajime Morrita 2012-09-02 20:32:42 PDT
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.
Comment 18 Hajime Morrita 2012-09-05 03:39:17 PDT
Created attachment 162208 [details]
Patch
Comment 19 Kentaro Hara 2012-09-05 03:47:28 PDT
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.
Comment 20 Hajime Morrita 2012-09-05 03:56:38 PDT
Created attachment 162211 [details]
Patch for landing
Comment 21 Hajime Morrita 2012-09-05 03:57:25 PDT
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.
Comment 22 Xan Lopez 2012-09-05 04:39:55 PDT
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?).
Comment 23 WebKit Review Bot 2012-09-05 05:08:45 PDT
Comment on attachment 162211 [details]
Patch for landing

Clearing flags on attachment: 162211

Committed r127580: <http://trac.webkit.org/changeset/127580>
Comment 24 WebKit Review Bot 2012-09-05 05:08:50 PDT
All reviewed patches have been landed.  Closing bug.