338954 - Make .innerHTML to work without DOM Ranges

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Currently .innerHTML getter and setter use DOM ranges. But if 
ranges are fixed to work only when nodes are in document/document fragment/attr .innerHTML stops working for elements which aren't in
the document.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Few tests

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: added one test for range.createContextualFragment

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: proposed patch

The biggest change here is to move the implementation of
CreateContextualFragment from nsRange to nsContentUtils.
The scariest change is to use docEncoder->SetContainerNode
(not ->SetRange())

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

The "scary" part is that dom gets serialized in a simpler way.
No need to handle start/endContainer of the range because they are
anyway always the same node (which is why there is no need to use range 
at all).

I tested the patch also with some test cases found in bugzilla and 
couldn't see any regressions.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 223023 [details] [diff] [review]
proposed patch

>+nsContentUtils::CreateContextualFragment(nsIDOMNode* aContextNode,
...
>+  while (parent && (parent != domDocument) && NS_SUCCEEDED(rv)) {

Please |break| in all places where rv could become an error and remove the last condition here. The rv variable is really volitile so it should be checked asap to avoid future code from clamping all over it.

>+    PRUint16 nodeType;
>+    parent->GetNodeType(&nodeType);
>+    if (nsIDOMNode::ELEMENT_NODE == nodeType) {
>+      nsAutoString tagName, uriStr;
>+      parent->GetNodeName(tagName);
>+
>+      // see if we need to add xmlns declarations
>+      nsCOMPtr<nsIContent> content = do_QueryInterface(parent, &rv);
>+      if (NS_FAILED(rv)) {
>+        break;
>+      }

Don't use nsresults with QI, instead just nullcheck the return.

>Index: content/base/src/nsDocumentEncoder.cpp

>   nsresult SerializeToStringRecursive(nsIDOMNode* aNode,
>-                                      nsAString& aStr);
>+                                      nsAString& aStr,
>+                                      PRBool aNodeIsContainer = PR_FALSE);

Something like |aDontSerializeRoot| or |aJustSerializeChildren| might be more descriptive, up to you.

Don't use default parameters though, please just change all call sites. Default params just make further signature changes harder.

With that, r=me

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 223895 [details] [diff] [review]
v2

sr=jst