752210 - Use nsIContent in nsHTMLEditor::RelativeFontChange

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

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

Attachment: Part a: RelativeFontChangeHelper

Comment 1

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

Attachment: Part b: RelativeFontChangeOnNode

Comment 2

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

Attachment: Part c: RelativeFontChange

Comment 3

[(no longer active)]

Comment on attachment 621289 [details] [diff] [review]
Part a: RelativeFontChangeHelper

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

::: editor/libeditor/html/nsHTMLEditorStyle.cpp
@@ +1689,5 @@
> +    // Cycle through children and adjust relative font size.
> +    for (nsIContent* child = aNode->GetLastChild();
> +         child;
> +         child = child->GetPreviousSibling()) {
> +      nsresult rv = RelativeFontChangeOnNode(aSizeChange, child->AsDOMNode());

Hmm, why are you calling the nsIDOMNode* version of this method here?  You can just call the nsINode* version directly, like you do below.

Comment 4

[(no longer active)]

Comment on attachment 621290 [details] [diff] [review]
Part b: RelativeFontChangeOnNode

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

::: editor/libeditor/html/nsHTMLEditorStyle.cpp
@@ +1688,5 @@
>      // Cycle through children and adjust relative font size.
>      for (nsIContent* child = aNode->GetLastChild();
>           child;
>           child = child->GetPreviousSibling()) {
> +      nsresult rv = RelativeFontChangeOnNode(aSizeChange, child);

Hmm, heh, this is effectively addressing my comment on the first patch!  :-)

Comment 5

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

(In reply to Ehsan Akhgari [:ehsan] from comment #3)
> Comment on attachment 621289 [details] [diff] [review]
> Part a: RelativeFontChangeHelper
> 
> Review of attachment 621289 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: editor/libeditor/html/nsHTMLEditorStyle.cpp
> @@ +1689,5 @@
> > +    // Cycle through children and adjust relative font size.
> > +    for (nsIContent* child = aNode->GetLastChild();
> > +         child;
> > +         child = child->GetPreviousSibling()) {
> > +      nsresult rv = RelativeFontChangeOnNode(aSizeChange, child->AsDOMNode());
> 
> Hmm, why are you calling the nsIDOMNode* version of this method here?  You
> can just call the nsINode* version directly, like you do below.

Because it doesn't yet exist in this patch :)

Comment 6

[(no longer active)]

(In reply to Ms2ger from comment #5)
> (In reply to Ehsan Akhgari [:ehsan] from comment #3)
> > Comment on attachment 621289 [details] [diff] [review]
> > Part a: RelativeFontChangeHelper
> > 
> > Review of attachment 621289 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: editor/libeditor/html/nsHTMLEditorStyle.cpp
> > @@ +1689,5 @@
> > > +    // Cycle through children and adjust relative font size.
> > > +    for (nsIContent* child = aNode->GetLastChild();
> > > +         child;
> > > +         child = child->GetPreviousSibling()) {
> > > +      nsresult rv = RelativeFontChangeOnNode(aSizeChange, child->AsDOMNode());
> > 
> > Hmm, why are you calling the nsIDOMNode* version of this method here?  You
> > can just call the nsINode* version directly, like you do below.
> 
> Because it doesn't yet exist in this patch :)

Hehe, that's a good reason!  ;-)

Feel free to land the patches as they are

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/25025cc03fb3
https://hg.mozilla.org/mozilla-central/rev/e0110ef13989
https://hg.mozilla.org/mozilla-central/rev/c795f1a41dae

Comment 8

[[e-mail]]

Comment on attachment 621290 [details] [diff] [review]
Part b: RelativeFontChangeOnNode

>   // none of the above?  then cycle through the children.
>   // MOOSE: we should group the children together if possible
>   // into a single "big" or "small".  For the moment they are
>   // each getting their own.  
>-  nsCOMPtr<nsIDOMNodeList> childNodes;
>-  res = aNode->GetChildNodes(getter_AddRefs(childNodes));
>-  NS_ENSURE_SUCCESS(res, res);
>-  if (childNodes)
>-  {
>-    PRInt32 j;
>-    PRUint32 childCount;
>-    childNodes->GetLength(&childCount);
>-    for (j=childCount-1; j>=0; j--)
>-    {
>-      nsCOMPtr<nsIDOMNode> childNode;
>-      res = childNodes->Item(j, getter_AddRefs(childNode));
>-      if ((NS_SUCCEEDED(res)) && (childNode))
>-      {
>-        res = RelativeFontChangeOnNode(aSizeChange, childNode);
>-        NS_ENSURE_SUCCESS(res, res);
>-      }
>-    }
>+  for (nsIContent* child = aNode->GetLastChild();
>+       child;
>+       child = child->GetPreviousSibling()) {
>+    nsresult rv = RelativeFontChangeOnNode(aSizeChange, child);
>+    NS_ENSURE_SUCCESS(rv, rv);
>   }
>-  return res;
>+
>+  return NS_OK;
> }
This changes the meaning of the code, in particular you can't iterate over nodes that you're mutating...