According to the spec, ShadowRoot.cloneNode() must always throw a DATA_CLONE_ERR exception. Currently NULL is returned.
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...
Created attachment 159001 [details] Patch
Created attachment 159025 [details] Patch
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.
Created attachment 159317 [details] Patch
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
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
I don't think these failing tests are related to my patch.
Comment on attachment 159317 [details] Patch Clearing flags on attachment: 159317 Committed r126127: <http://trac.webkit.org/changeset/126127>
All reviewed patches have been landed. Closing bug.
(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.
> > 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.
Reverted at http://trac.webkit.org/changeset/127228 Need to figure out gobject-friendly way to do this.
(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.
(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().
(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.
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.
Created attachment 162208 [details] Patch
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.
Created attachment 162211 [details] Patch for landing
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.
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 on attachment 162211 [details] Patch for landing Clearing flags on attachment: 162211 Committed r127580: <http://trac.webkit.org/changeset/127580>