684187 - dom::Element instead of nsIDOMElement for nsEditor::mRootElement

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Fabien Cazenave [:kaze]]

Attachment: patch proposal

nsCOMPtr<nsIDOMElement> mRootElement → nsCOMPtr<nsIContent> mRootContent
+ nsEditor::GetRoot() now returns an nsIContent, hence a few changes and additional do_QueryInterface() calls.

TryServer looks ok:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=fd323f877cb0

Comment 1

[Fabien Cazenave [:kaze]]

Attachment: patch proposal

forgot to include a reference to the bug number in the patch…

Comment 2

[Fabien Cazenave [:kaze]]

Comment on attachment 557788 [details] [diff] [review]
patch proposal

Review of attachment 557788 [details] [diff] [review]:
-----------------------------------------------------------------

Ugh, this patch does not correspond to the one that has been pushed to Try. Sorry for the confusion. :-(

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

If you're going to change its type, please use nsRefPtr<dom::Element>

Comment 4

[(no longer active)]

While using dom::Element is better than nsIContent (to avoid checking IsElement() all the time), I don't like nsRefPtr here, since it would break the nsCOMPtr semantics such as do_QueryInterface.  Maybe we could give dom::Element an IID so that it can be used with nsCOMPtr?  Boris is the right person to answer this question I think.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Yeah, we should probably do that.  As it is, people QI to dom::Element every so often, which does totally the wrong thing...

Comment 6

[(no longer active)]

OK.  Fabien, can you please file a bug on that, and give dom::Element an IID?  Then, you can use nsCOMPtr<dom::Element> in this patch.  Thanks!

Comment 7

[Fabien Cazenave [:kaze]]

In that case, we probably want GetActiveEditingHost() to return a dom::Element* instead of an nsIContent*, right?

Comment 8

[(no longer active)]

Yes.

Comment 9

[Fabien Cazenave [:kaze]]

Attachment: patch proposal

Now using nsCOMPtr<dom::Element>.

(In reply to Boris Zbarsky (:bz) from comment #5)
> Yeah, we should probably do that. As it is, people QI to dom::Element every
> so often, which does totally the wrong thing...

Boris, you can help me here. :-)

This patch works with the current version of dom::Element… which means I’m probably doing “totally the wrong thing” when QI’ing to dom::Element in this patch. Can you explain a bit please?

More confusing to me: I’ve tried to add an IID to dom::Element in bug 690372 but that breaks this patch. E.g. in the following, do_QI returns a null pointer if dom::Element has an IID:
  dom::Element* myElement = GetRoot();
  nsCOMPtr<nsIDOMNode> myNode = do_QueryInterface(myRoot);
…but it seems to work fine when I leave dom::Element alone. I’m lost. :-(

Comment 10

[Fabien Cazenave [:kaze]]

https://tbpl.mozilla.org/?tree=Try&rev=9c6b58e6176f

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Can you explain a bit please?

Sure.  This code:

  void foo(nsIContent* bar) {
    nsCOMPtr<dom::Element> el = do_QueryInterface(bar);
  }

will currently always give you a non-null |el| if |bar| is not null, even if |bar| is not an element, becase NS_GET_IID(dom::Element) returns the IID of nsIContent so the QI is done to nsIContent and the resulting pointer is reinterpeted as a dom::Element*.

If you happen to know that |bar| is an element, there is no problem, of course.

> E.g. in the following, do_QI returns a null pointer if dom::Element has an IID:

Is myElement null to start with there?  As in, is the problem in the QI to nsIDOMNode or in the earlier QI to dom::Element when setting mRootElement?

Comment 12

[Fabien Cazenave [:kaze]]

Attachment: patch proposal

Boris, thanks for your explanation! I had this issue because an NS_INTERFACE_MAP_ENTRY was required in the patch I’ve submitted for bug 690372, hence the failing QI calls.

Comment 13

[Fabien Cazenave [:kaze]]

https://tbpl.mozilla.org/?tree=Try&rev=907654f23f5e

Comment 14

[(no longer active)]

Comment on attachment 568043 [details] [diff] [review]
patch proposal

Review of attachment 568043 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/base/nsEditor.cpp
@@ +1112,2 @@
>    nsCOMPtr<nsIDOMNode> child;
> +  NS_ENSURE_TRUE(node, NS_ERROR_NULL_POINTER); 

Nit: move this one line up please.

@@ +1984,2 @@
>    // Not ref counted
> +  nsIFrame *frame = aRoot->GetPrimaryFrame();

Ensure that aRoot is non-null.

@@ +2000,5 @@
>    *aWidget = nsnull;
>  
>    nsCOMPtr<nsIWidget> widget;
> +  dom::Element *rootElement = GetRoot();
> +  nsresult res = GetEditorContentWindow(rootElement, getter_AddRefs(widget));

Can't you just pass GetRoot() to GetEditorContentWindow directly?

@@ +2101,2 @@
>    NS_ADDREF(*aRootElement);
> +  //NS_IF_ADDREF(*aRootElement = rootContent);

Nit: remove the comment please.

@@ +2174,3 @@
>    nsCOMPtr<nsIDOMNode> p = aDestNode;
> +  nsCOMPtr<nsIDOMNode> rootNode = do_QueryInterface(GetRoot());
> +  NS_ENSURE_TRUE(rootNode, NS_ERROR_NULL_POINTER);

Nit: move this up to right after the comment, please.

@@ +3572,5 @@
>  nsEditor::IsDescendantOfBody(nsIDOMNode *inNode) 
>  {
>    NS_ENSURE_TRUE(inNode, false);
> +  nsCOMPtr<nsIDOMNode> root = do_QueryInterface(GetRoot());
> +  NS_ENSURE_TRUE(root, PR_FALSE);

Use false please.

::: editor/libeditor/html/nsHTMLEditRules.cpp
@@ +1013,5 @@
>      // in the parent hierarchy.
>      
>      // gather up info we need for test
>      nsCOMPtr<nsIDOMNode> parent, tmp, root;
> +    nsCOMPtr<nsIDOMElement> rootElem = do_QueryInterface(mHTMLEditor->GetRoot());

Can't you directly QI and set to "root" here?

@@ +7783,2 @@
>        NS_ENSURE_TRUE(rootElement, NS_ERROR_FAILURE);
>        nsCOMPtr<nsIDOMNode> rootNode(do_QueryInterface(rootElement));

Ditto here for rootNode.

::: editor/libeditor/html/nsHTMLEditor.cpp
@@ +541,5 @@
>    NS_ENSURE_SUCCESS(res, res);
>    NS_ENSURE_TRUE(selection, NS_ERROR_NOT_INITIALIZED);
>  
>    // Get the root element.
> +  //nsIDOMElement *rootElement = GetRoot();

Remove the comment please.

@@ +1798,5 @@
>    nsCOMPtr<nsISelection>selection;
>    nsresult res = GetSelection(getter_AddRefs(selection));
>    NS_ENSURE_SUCCESS(res, res);
>  
> +  //nsIDOMElement *bodyElement = GetRoot();

Ditto.

@@ +2447,3 @@
>    NS_ENSURE_TRUE(element, NS_ERROR_NULL_POINTER);
>  
>    return element->GetAttribute(styleName, aOutColor);

If you use GetAttr here, you should be able to save a QI...

@@ +3401,5 @@
>  
>    NS_ASSERTION(mDocWeak, "Missing Editor DOM Document");
>    
>    // Set the background color attribute on the body tag
> +  //nsIDOMElement *bodyElement = GetRoot();

Unneeded comment.

::: editor/libeditor/text/nsPlaintextEditor.cpp
@@ +1020,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>    if (docEmpty)
>      return NS_OK;
>  
> +  nsCOMPtr<nsIContent> rootContent = GetRoot();

Can't you make rootContent an Element* and save a QI?

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 568043 [details] [diff] [review]
patch proposal

And a couple of nits from me as well:

>--- a/editor/libeditor/base/nsEditor.cpp
>+++ b/editor/libeditor/base/nsEditor.cpp
> nsEditor::GetRootElement(nsIDOMElement **aRootElement)
> {
>   NS_ENSURE_ARG_POINTER(aRootElement);
>   NS_ENSURE_TRUE(mRootElement, NS_ERROR_NOT_AVAILABLE);
>-  *aRootElement = mRootElement;
>+  nsCOMPtr<nsIDOMElement> rootContent = do_QueryInterface(mRootElement);

rootContent sounds like an nsIContent, please use rootElement.

>+  *aRootElement = rootContent;
>   NS_ADDREF(*aRootElement);

And make this 'rootElement.forget(aRootElement);' to save an addref-release pair.

> nsEditor::IsDescendantOfBody(nsIDOMNode *inNode) 
> {
>   NS_ENSURE_TRUE(inNode, false);
>-  nsIDOMElement *rootElement = GetRoot();
>-  NS_ENSURE_TRUE(rootElement, false);
>-  nsCOMPtr<nsIDOMNode> root = do_QueryInterface(rootElement);
>+  nsCOMPtr<nsIDOMNode> root = do_QueryInterface(GetRoot());
>+  NS_ENSURE_TRUE(root, PR_FALSE);

'false'

> 
>   if (inNode == root.get()) return true;
>   
>   nsCOMPtr<nsIDOMNode> parent, node = do_QueryInterface(inNode);

Make this 'node = inNode' while you're here.

>-nsIDOMElement *
>+mozilla::dom::Element *

Drop the 'mozilla::'

>--- a/editor/libeditor/html/nsHTMLEditRules.cpp
>+++ b/editor/libeditor/html/nsHTMLEditRules.cpp
>@@ -7773,17 +7774,17 @@ nsHTMLEditRules::AdjustSelection(nsISele
>     if (bIsEmptyNode && mHTMLEditor->CanContainTag(selNode, NS_LITERAL_STRING("br")))
>     {
>-      nsIDOMElement *rootElement = mHTMLEditor->GetRoot();
>+      nsCOMPtr<nsIDOMElement> rootElement = do_QueryInterface(mHTMLEditor->GetRoot());
>       NS_ENSURE_TRUE(rootElement, NS_ERROR_FAILURE);
>       nsCOMPtr<nsIDOMNode> rootNode(do_QueryInterface(rootElement));

'rootNode = rootElement'

>--- a/editor/libeditor/html/nsHTMLEditor.cpp
>+++ b/editor/libeditor/html/nsHTMLEditor.cpp
>@@ -372,37 +372,39 @@ nsHTMLEditor::GetRootElement(nsIDOMEleme
>     // If there is no HTML body element,
>     // we should use the document root element instead.
>     nsCOMPtr<nsIDOMDocument> doc = do_QueryReferent(mDocWeak);
>     NS_ENSURE_TRUE(doc, NS_ERROR_NOT_INITIALIZED);
> 
>-    rv = doc->GetDocumentElement(getter_AddRefs(mRootElement));
>+    rv = doc->GetDocumentElement(getter_AddRefs(rootElement));

QI to nsIDocument and use GetRootElement()? Then you've got a dom::Element immediately.

>@@ -2434,17 +2438,17 @@ nsHTMLEditor::GetHTMLBackgroundColorStat
>-  element = GetRoot();
>+  element = do_QueryInterface(GetRoot());
>   NS_ENSURE_TRUE(element, NS_ERROR_NULL_POINTER);
> 
>   return element->GetAttribute(styleName, aOutColor);

Could use the nsIContent API

>--- a/editor/libeditor/html/nsHTMLInlineTableEditor.cpp
>+++ b/editor/libeditor/html/nsHTMLInlineTableEditor.cpp
>@@ -125,20 +126,17 @@ nsHTMLEditor::HideInlineTableEditingUI()
>+  nsCOMPtr<nsIContent> bodyContent( do_QueryInterface(GetRoot()) );

nsCOMPtr<nsIContent> bodyContent = GetRoot();

>--- a/editor/libeditor/text/nsPlaintextEditor.cpp
>+++ b/editor/libeditor/text/nsPlaintextEditor.cpp
>@@ -1099,17 +1099,17 @@ nsPlaintextEditor::SetWrapWidth(PRInt32 
>-  nsIDOMElement *rootElement = GetRoot();
>+  nsCOMPtr<nsIDOMElement> rootElement = do_QueryInterface(GetRoot());

nsIContent API?

Comment 16

[Fabien Cazenave [:kaze]]

Attachment: patch proposal

Thank you both for this extensive review!

(In reply to Ehsan Akhgari [:ehsan] from comment #14)
> @@ +1984,2 @@
> >    // Not ref counted
> > +  nsIFrame *frame = aRoot->GetPrimaryFrame();
> 
> Ensure that aRoot is non-null.

This is checked above, right?
  NS_ENSURE_TRUE(aRoot && aResult, NS_ERROR_NULL_POINTER);

(In reply to Ms2ger from comment #15)
> And make this 'rootElement.forget(aRootElement);' to save an addref-release
> pair.

Thanks! I wouldn’t have thought of this. Note to self:
https://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsCOMPtr.h#812

> QI to nsIDocument and use GetRootElement()? Then you've got a dom::Element
> immediately.

Hmmm, I’m not sure I see the point. That would be nice for mRootElement (which is a dom::Element) but nsHTMLEditor::GetRootElement has to return an nsIDOMElement, so we’d have to QI mRootElement to nsIDOMElement anyway.

That’s why I’ve kept the QI to nsIDOMDocument and I’ve applied the forget() trick instead. Is that correct?

Comment 17

[Fabien Cazenave [:kaze]]

Attachment: patch proposal

Sorry, forgot to `hg qrefresh'. :-/

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Fabien Cazenave (:kaze) from comment #16)
> > QI to nsIDocument and use GetRootElement()? Then you've got a dom::Element
> > immediately.
> 
> Hmmm, I’m not sure I see the point. That would be nice for mRootElement
> (which is a dom::Element) but nsHTMLEditor::GetRootElement has to return an
> nsIDOMElement, so we’d have to QI mRootElement to nsIDOMElement anyway.
> 
> That’s why I’ve kept the QI to nsIDOMDocument and I’ve applied the forget()
> trick instead. Is that correct?

Mm, yes, I missed that you need both a dom::Element and an nsIDOMElement.

Comment 19

[Fabien Cazenave [:kaze]]

Thanks again for your explanations, you’ve helped me a lot here.

Comment 20

[Fabien Cazenave [:kaze]]

tbpl looks OK:
https://tbpl.mozilla.org/?tree=Try&rev=646d8a408916

Comment 21

[(no longer active)]

This depends on bug 690372, which hasn't been fixed yet.

Comment 22

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

And has now.

Comment 23

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Should we expose something that returns dom::Element on nsIEditor, btw? Then we could get rid of GetEditorRootContent at <http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsGenericElement.cpp#277>. (In any case: followup.)

Comment 24

[Dão Gottwald [:dao]]

The patch doesn't seem to apply cleanly anymore.

Comment 25

[Fabien Cazenave [:kaze]]

Attachment: patch proposal

Rebased patch, still passes TBPL:
https://tbpl.mozilla.org/?tree=Try&rev=216ae5f40cfe

Comment 26

[Ed Morley [:emorley]]

Even with the new patch, applying to inbound tip gives:
{
patching file editor/libeditor/base/nsEditor.cpp
Hunk #10 FAILED at 3554
}

Comment 27

[Fabien Cazenave [:kaze]]

Attachment: patch proposal

https://tbpl.mozilla.org/?tree=Try&rev=15747dd18a79

Comment 28

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/b45ac1c3ab89