Implement cloneNode for Document

Implement cloneNode for Document

When cloning HTMLDocument and SVGDocument we need to ensure that we
create a new HTMLDocument and SVGDocument respectively.

http://dom.spec.whatwg.org/#dom-node-clonenode

BUG=258146
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=159835

[arv (Not doing code reviews), 2013-08-26 20:48:26]

Second try.

Main difference from first try is that:

1. It used DocumentInit(url()) for the URL.
2. It uses a new TextResourceDecoder.
3. It also inherits the registrationContext for (X)HTML documents.

registrationContext, securityOrigin and contextFeatures follow the exact same
code paths as DOMImplementation::create(HTML)Document.

[abarth-chromium, 2013-08-26 21:04:54]

https://codereview.chromium.org/23437003/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/23437003/diff/1/Source/core/dom/Document.cpp#...
Source/core/dom/Document.cpp:3113: setSecurityOrigin(other.securityOrigin());
You should make a copy of the security origin.  The security origin object is
mutable, which means mutations will affect both documents.

https://codereview.chromium.org/23437003/diff/1/Source/core/dom/Document.cpp#...
Source/core/dom/Document.cpp:3116:
setDecoder(TextResourceDecoder::create(other.suggestedMIMEType(),
other.encoding()));
This doesn't make a lot of sense...  There is no text resource to decode.

jamesr is in the process of removing the TextResourceDecoder member from
Document, which will make this easier.

What about the HTMLImports state?  How do we decide which state to clone here?

[abarth-chromium, 2013-08-26 21:05:22]

+jamesr for the interaction with TextResourceDecoder

[jamesr, 2013-08-26 21:07:51]

https://codereview.chromium.org/23437003/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/23437003/diff/1/Source/core/dom/Document.cpp#...
Source/core/dom/Document.cpp:3116:
setDecoder(TextResourceDecoder::create(other.suggestedMIMEType(),
other.encoding()));
On 2013/08/26 21:04:55, abarth wrote:
> This doesn't make a lot of sense...  There is no text resource to decode.
> 
> jamesr is in the process of removing the TextResourceDecoder member from
> Document, which will make this easier.

Have you tested that the exposed encoding on the new document should match the
old one?  If so, then we just need to call setEncoding(other.encoding()) once
https://codereview.chromium.org/22909053/ lands.  I'm not sure that makes sense,
though, since the cloned document wasn't generated by decoding an encoded
resource.

[arv (Not doing code reviews), 2013-08-26 21:48:29]

On 2013/08/26 21:07:51, jamesr wrote:
> https://codereview.chromium.org/23437003/diff/1/Source/core/dom/Document.cpp
> File Source/core/dom/Document.cpp (right):
> 
>
https://codereview.chromium.org/23437003/diff/1/Source/core/dom/Document.cpp#...
> Source/core/dom/Document.cpp:3116:
> setDecoder(TextResourceDecoder::create(other.suggestedMIMEType(),
> other.encoding()));
> On 2013/08/26 21:04:55, abarth wrote:
> > This doesn't make a lot of sense...  There is no text resource to decode.
> > 
> > jamesr is in the process of removing the TextResourceDecoder member from
> > Document, which will make this easier.
> 
> Have you tested that the exposed encoding on the new document should match the
> old one?  If so, then we just need to call setEncoding(other.encoding()) once
> https://codereview.chromium.org/22909053/ lands.  I'm not sure that makes
sense,
> though, since the cloned document wasn't generated by decoding an encoded
> resource.

There is a test that compares the characterSet are the same of the two
documents.

The spec requires that characterSet be cloned. I can't think of any case where
this would be required so maybe we can get the spec to change?

I'm also fine waiting on your patch to land since it makes things simpler no
matter what.

[arv (Not doing code reviews), 2013-08-26 21:52:36]

On 2013/08/26 21:04:54, abarth wrote:
> https://codereview.chromium.org/23437003/diff/1/Source/core/dom/Document.cpp
> File Source/core/dom/Document.cpp (right):
> 
>
https://codereview.chromium.org/23437003/diff/1/Source/core/dom/Document.cpp#...
> Source/core/dom/Document.cpp:3113: setSecurityOrigin(other.securityOrigin());
> You should make a copy of the security origin.  The security origin object is
> mutable, which means mutations will affect both documents.
> 
>
https://codereview.chromium.org/23437003/diff/1/Source/core/dom/Document.cpp#...
> Source/core/dom/Document.cpp:3116:
> setDecoder(TextResourceDecoder::create(other.suggestedMIMEType(),
> other.encoding()));
> This doesn't make a lot of sense...  There is no text resource to decode.
> 
> jamesr is in the process of removing the TextResourceDecoder member from
> Document, which will make this easier.
> 
> What about the HTMLImports state?

HTMLImports (like other <link> related state) gets cloned when the link element
gets cloned.

> How do we decide which state to clone here?

The spec defines:

http://dom.spec.whatwg.org/#concept-node-clone

"Its encoding, content type, URL, its mode (quirks mode, limited quirks mode, or
no-quirks mode), and its type (XML document or HTML document)."

At the moment I don't see a reason to why we would need to copy the encoding.
I'll ask on #whatwg what the intent was.

[abarth-chromium, 2013-08-26 22:24:41]

On 2013/08/26 21:52:36, arv wrote:
> On 2013/08/26 21:04:54, abarth wrote:
> > What about the HTMLImports state?
> 
> HTMLImports (like other <link> related state) gets cloned when the link
element
> gets cloned.

I meant if this document was imported by another document.

[jamesr, 2013-08-26 23:18:22]

On 2013/08/26 21:48:29, arv wrote:
> The spec requires that characterSet be cloned. I can't think of any case where
> this would be required so maybe we can get the spec to change?
> 
> I'm also fine waiting on your patch to land since it makes things simpler no
> matter what.

I think it's worth checking what the desired behavior is and if it's desired to
preserve the attribute value to wait for my Document::setEncoding patch to land.
 I hope to have it fixed up soon (i.e. today or tomorrow).

[arv (Not doing code reviews), 2013-08-27 00:06:15]

On 2013/08/26 23:18:22, jamesr wrote:
> On 2013/08/26 21:48:29, arv wrote:
> > The spec requires that characterSet be cloned. I can't think of any case
where
> > this would be required so maybe we can get the spec to change?
> > 
> > I'm also fine waiting on your patch to land since it makes things simpler no
> > matter what.
> 
> I think it's worth checking what the desired behavior is and if it's desired
to
> preserve the attribute value to wait for my Document::setEncoding patch to
land.
>  I hope to have it fixed up soon (i.e. today or tomorrow).

According to Anne the characterSet is observable through URLs and <form>. I'll
add tests for these.

No rush. This is low priority for me so I would not mind waiting a week or two
if that is what you need.

[arv (Not doing code reviews), 2013-08-29 19:31:15]

Updated to use encoding/setEncoding.

PTAL

[abarth-chromium, 2013-08-30 06:56:41]

LGTM

Once comment below about making a copy of the security origin.  I looked at the
messages on this CL and I couldn't find your reply.

You can test the difference as follows:

1) Clone a document.
2) Set the document.domain property of the original document.
3) Observe that the document.domain property of the cloned document doesn't
change.

With the CL as written, the cloned document's domain property will change as
well and the two will be coupled together, which seems wrong.  Please add a test
for this case as well.

https://codereview.chromium.org/23437003/diff/13001/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/23437003/diff/13001/Source/core/dom/Document....
Source/core/dom/Document.cpp:3114: setSecurityOrigin(other.securityOrigin());
Shouldn't this be a copy of the security origin?  Maybe you replied to this
comment already.

https://codereview.chromium.org/23437003/diff/13001/Source/core/html/HTMLDocu...
File Source/core/html/HTMLDocument.cpp (right):

https://codereview.chromium.org/23437003/diff/13001/Source/core/html/HTMLDocu...
Source/core/html/HTMLDocument.cpp:221: return
create(DocumentInit(url()).withRegistrationContext(registrationContext()));
Nice.

[arv (Not doing code reviews), 2013-08-30 14:33:45]

Re document.domain: Ouch. We have the same bug in
document.implementation.createHTMLDocument.

> document.domain
"chrome.google.com"
> doc = document.implementation.createHTMLDocument('title')
#document
> doc.domain
"chrome.google.com"
> document.domain = 'google.com'
"google.com"
> document.domain
"google.com"
> doc.domain
"google.com"

https://code.google.com/p/chromium/issues/detail?id=282415



On Fri, Aug 30, 2013 at 2:56 AM, <abarth@chromium.org> wrote:

> LGTM
>
> Once comment below about making a copy of the security origin.  I looked
> at the
> messages on this CL and I couldn't find your reply.
>
> You can test the difference as follows:
>
> 1) Clone a document.
> 2) Set the document.domain property of the original document.
> 3) Observe that the document.domain property of the cloned document doesn't
> change.
>
> With the CL as written, the cloned document's domain property will change
> as
> well and the two will be coupled together, which seems wrong.  Please add
> a test
> for this case as well.
>
>
> https://codereview.chromium.**org/23437003/diff/13001/**
>
Source/core/dom/Document.cpp<https://codereview.chromium.org/23437003/diff/13001/Source/core/dom/Document.cpp>
> File Source/core/dom/Document.cpp (right):
>
> https://codereview.chromium.**org/23437003/diff/13001/**
>
Source/core/dom/Document.cpp#**newcode3114<https://codereview.chromium.org/23437003/diff/13001/Source/core/dom/Document.cpp#newcode3114>
> Source/core/dom/Document.cpp:**3114:
> setSecurityOrigin(other.**securityOrigin());
> Shouldn't this be a copy of the security origin?  Maybe you replied to
> this comment already.
>
> https://codereview.chromium.**org/23437003/diff/13001/**
>
Source/core/html/HTMLDocument.**cpp<https://codereview.chromium.org/23437003/diff/13001/Source/core/html/HTMLDocument.cpp>
> File Source/core/html/HTMLDocument.**cpp (right):
>
> https://codereview.chromium.**org/23437003/diff/13001/**
>
Source/core/html/HTMLDocument.**cpp#newcode221<https://codereview.chromium.org/23437003/diff/13001/Source/core/html/HTMLDocument.cpp#newcode221>
> Source/core/html/HTMLDocument.**cpp:221: return
> create(DocumentInit(url()).**withRegistrationContext(**
> registrationContext()));
> Nice.
>
>
https://codereview.chromium.**org/23437003/<https://codereview.chromium.org/2...
>



-- 
erik

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[arv (Not doing code reviews), 2013-10-16 16:09:53]

Adam, now that document.domain has been resolved for DOMImplementation
create{HTML}Document I applied the same fix to this. I'm now using
SecurityContext::isolatedCopy and I also added another tests for changing domain
in different clones.

PTAL

[abarth-chromium, 2013-10-17 00:59:48]

lgtm

The set of things cloned seems a bit ad-hoc, but as long as it matches the spec,
this seems fine.

[commit-bot: I haz the power, 2013-10-17 01:26:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/arv@chromium.org/23437003/20001

[commit-bot: I haz the power, 2013-10-17 07:40:53]

Change committed as 159835