Closed Bug 684187 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: DOM: Editor, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: kaze, Assigned: kaze)

References

Details

Attachments

(1 file, 7 obsolete files)

Attached patch patch proposal (obsolete) — Splinter Review
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
Attachment #557786 - Flags: review?(ehsan)
Attached patch patch proposal (obsolete) — Splinter Review
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)
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!
Summary: nsIContent instead of nsIDOMElement for nsEditor::mRootElement → dom::Element instead of nsIDOMElement for nsEditor::mRootElement
In that case, we probably want GetActiveEditingHost() to return a dom::Element* instead of an nsIContent*, right?
Yes.
Depends on: 690372
Attached patch patch proposal (obsolete) — Splinter Review
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
> 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?
Attached patch patch proposal (obsolete) — Splinter Review
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
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?
Attached patch patch proposal (obsolete) — Splinter Review
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
Attached patch patch proposal (obsolete) — Splinter Review
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.
Thanks again for your explanations, you’ve helped me a lot here.
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
Attached patch patch proposal (obsolete) — Splinter Review
Rebased patch, still passes TBPL:
https://tbpl.mozilla.org/?tree=Try&rev=216ae5f40cfe
Attachment #570017 - Attachment is obsolete: true
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
}
Attached patch patch proposalSplinter Review
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
Closed: 8 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Attachment #578023 - Flags: checkin?(ehsan) → checkin+
You need to log in before you can comment on or make changes to this bug.