Last Comment Bug 684187 - dom::Element instead of nsIDOMElement for nsEditor::mRootElement
: dom::Element instead of nsIDOMElement for nsEditor::mRootElement
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Fabien Cazenave [:kaze]
:
Mentors:
Depends on: 690372
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-02 02:41 PDT by Fabien Cazenave [:kaze]
Modified: 2012-02-01 14:00 PST (History)
5 users (show)
Ms2ger: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch proposal (46.79 KB, patch)
2011-09-02 02:41 PDT, Fabien Cazenave [:kaze]
no flags Details | Diff | Splinter Review
patch proposal (46.79 KB, patch)
2011-09-02 02:43 PDT, Fabien Cazenave [:kaze]
no flags Details | Diff | Splinter Review
patch proposal (41.50 KB, patch)
2011-10-12 09:13 PDT, Fabien Cazenave [:kaze]
no flags Details | Diff | Splinter Review
patch proposal (43.10 KB, patch)
2011-10-19 07:25 PDT, Fabien Cazenave [:kaze]
ehsan: review+
Details | Diff | Splinter Review
patch proposal (44.12 KB, patch)
2011-10-27 09:56 PDT, Fabien Cazenave [:kaze]
no flags Details | Diff | Splinter Review
patch proposal (44.27 KB, patch)
2011-10-27 09:58 PDT, Fabien Cazenave [:kaze]
no flags Details | Diff | Splinter Review
patch proposal (43.31 KB, patch)
2011-11-29 07:17 PST, Fabien Cazenave [:kaze]
no flags Details | Diff | Splinter Review
patch proposal (43.39 KB, patch)
2011-11-30 11:32 PST, Fabien Cazenave [:kaze]
Ms2ger: checkin+
Details | Diff | Splinter Review

Description Fabien Cazenave [:kaze] 2011-09-02 02:41:05 PDT
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
Comment 1 Fabien Cazenave [:kaze] 2011-09-02 02:43:56 PDT
Created attachment 557788 [details] [diff] [review]
patch proposal

forgot to include a reference to the bug number in the patch…
Comment 2 Fabien Cazenave [:kaze] 2011-09-02 07:32:49 PDT
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 (⌚ UTC+1/+2) 2011-09-02 07:37:47 PDT
If you're going to change its type, please use nsRefPtr<dom::Element>
Comment 4 :Ehsan Akhgari (away Aug 1-5) 2011-09-02 08:23:12 PDT
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 [:bz] 2011-09-02 08:47:03 PDT
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 :Ehsan Akhgari (away Aug 1-5) 2011-09-02 09:06:45 PDT
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] 2011-09-07 09:10:45 PDT
In that case, we probably want GetActiveEditingHost() to return a dom::Element* instead of an nsIContent*, right?
Comment 8 :Ehsan Akhgari (away Aug 1-5) 2011-09-07 09:12:31 PDT
Yes.
Comment 9 Fabien Cazenave [:kaze] 2011-10-12 09:13:05 PDT
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. :-(
Comment 10 Fabien Cazenave [:kaze] 2011-10-12 09:15:40 PDT
https://tbpl.mozilla.org/?tree=Try&rev=9c6b58e6176f
Comment 11 Boris Zbarsky [:bz] 2011-10-12 10:33:55 PDT
> 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] 2011-10-19 07:25:17 PDT
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.
Comment 13 Fabien Cazenave [:kaze] 2011-10-19 07:46:07 PDT
https://tbpl.mozilla.org/?tree=Try&rev=907654f23f5e
Comment 14 :Ehsan Akhgari (away Aug 1-5) 2011-10-25 19:16:17 PDT
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 (⌚ UTC+1/+2) 2011-10-26 06:17:12 PDT
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] 2011-10-27 09:56:46 PDT
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?
Comment 17 Fabien Cazenave [:kaze] 2011-10-27 09:58:35 PDT
Created attachment 570017 [details] [diff] [review]
patch proposal

Sorry, forgot to `hg qrefresh'. :-/
Comment 18 :Ms2ger (⌚ UTC+1/+2) 2011-10-27 11:23:49 PDT
(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] 2011-10-27 14:31:16 PDT
Thanks again for your explanations, you’ve helped me a lot here.
Comment 20 Fabien Cazenave [:kaze] 2011-10-28 05:32:04 PDT
tbpl looks OK:
https://tbpl.mozilla.org/?tree=Try&rev=646d8a408916
Comment 21 :Ehsan Akhgari (away Aug 1-5) 2011-11-21 11:53:54 PST
This depends on bug 690372, which hasn't been fixed yet.
Comment 22 :Ms2ger (⌚ UTC+1/+2) 2011-11-22 00:02:00 PST
And has now.
Comment 23 :Ms2ger (⌚ UTC+1/+2) 2011-11-24 12:03:02 PST
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] 2011-11-26 11:46:24 PST
The patch doesn't seem to apply cleanly anymore.
Comment 25 Fabien Cazenave [:kaze] 2011-11-29 07:17:28 PST
Created attachment 577605 [details] [diff] [review]
patch proposal

Rebased patch, still passes TBPL:
https://tbpl.mozilla.org/?tree=Try&rev=216ae5f40cfe
Comment 26 Ed Morley [:emorley] 2011-11-30 05:22:04 PST
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] 2011-11-30 11:32:22 PST
Created attachment 578023 [details] [diff] [review]
patch proposal

https://tbpl.mozilla.org/?tree=Try&rev=15747dd18a79
Comment 28 :Ms2ger (⌚ UTC+1/+2) 2011-12-04 14:06:57 PST
https://hg.mozilla.org/mozilla-central/rev/b45ac1c3ab89

Note You need to log in before you can comment on or make changes to this bug.