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

RESOLVED FIXED in mozilla11

Status

()

Core
Editor
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: kaze, Assigned: kaze)

Tracking

Trunk
mozilla11
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 557786 [details] [diff] [review]
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
(Assignee)

Updated

6 years ago
Attachment #557786 - Flags: review?(ehsan)
(Assignee)

Comment 1

6 years ago
Created attachment 557788 [details] [diff] [review]
patch proposal

forgot to include a reference to the bug number in the patch…
Assignee: nobody → kaze
Attachment #557786 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #557786 - Flags: review?(ehsan)
Attachment #557788 - Flags: review?(ehsan)
(Assignee)

Comment 2

6 years ago
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. :-(
Attachment #557788 - Flags: review?(ehsan)
If you're going to change its type, please use nsRefPtr<dom::Element>
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.
Yeah, we should probably do that.  As it is, people QI to dom::Element every so often, which does totally the wrong thing...
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!
(Assignee)

Updated

6 years ago
Summary: nsIContent instead of nsIDOMElement for nsEditor::mRootElement → dom::Element instead of nsIDOMElement for nsEditor::mRootElement
(Assignee)

Comment 7

6 years ago
In that case, we probably want GetActiveEditingHost() to return a dom::Element* instead of an nsIContent*, right?
Yes.
(Assignee)

Updated

6 years ago
Depends on: 690372
(Assignee)

Comment 9

6 years ago
Created attachment 566562 [details] [diff] [review]
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. :-(
Attachment #557788 - Attachment is obsolete: true
(Assignee)

Comment 10

6 years ago
https://tbpl.mozilla.org/?tree=Try&rev=9c6b58e6176f
> 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?
(Assignee)

Comment 12

6 years ago
Created attachment 568043 [details] [diff] [review]
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.
Attachment #566562 - Attachment is obsolete: true
(Assignee)

Comment 13

6 years ago
https://tbpl.mozilla.org/?tree=Try&rev=907654f23f5e
(Assignee)

Updated

6 years ago
Attachment #568043 - Flags: review?(ehsan)
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?
Attachment #568043 - Flags: review?(ehsan) → review+
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?
(Assignee)

Comment 16

6 years ago
Created attachment 570016 [details] [diff] [review]
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?
Attachment #568043 - Attachment is obsolete: true
(Assignee)

Comment 17

6 years ago
Created attachment 570017 [details] [diff] [review]
patch proposal

Sorry, forgot to `hg qrefresh'. :-/
Attachment #570016 - Attachment is obsolete: true
(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.
(Assignee)

Comment 19

6 years ago
Thanks again for your explanations, you’ve helped me a lot here.
(Assignee)

Comment 20

6 years ago
tbpl looks OK:
https://tbpl.mozilla.org/?tree=Try&rev=646d8a408916
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
This depends on bug 690372, which hasn't been fixed yet.
Keywords: checkin-needed
And has now.
Keywords: checkin-needed
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.)
The patch doesn't seem to apply cleanly anymore.
Keywords: checkin-needed
(Assignee)

Comment 25

6 years ago
Created attachment 577605 [details] [diff] [review]
patch proposal

Rebased patch, still passes TBPL:
https://tbpl.mozilla.org/?tree=Try&rev=216ae5f40cfe
Attachment #570017 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #577605 - Flags: checkin?(ehsan)
Even with the new patch, applying to inbound tip gives:
{
patching file editor/libeditor/base/nsEditor.cpp
Hunk #10 FAILED at 3554
}
(Assignee)

Comment 27

6 years ago
Created attachment 578023 [details] [diff] [review]
patch proposal

https://tbpl.mozilla.org/?tree=Try&rev=15747dd18a79
Attachment #577605 - Attachment is obsolete: true
Attachment #577605 - Flags: checkin?(ehsan)
Attachment #578023 - Flags: checkin?(ehsan)
https://hg.mozilla.org/mozilla-central/rev/b45ac1c3ab89
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla11

Updated

6 years ago
Attachment #578023 - Flags: checkin?(ehsan) → checkin+
You need to log in before you can comment on or make changes to this bug.