The default bug view has changed. See this FAQ.

Use nsIContent in nsHTMLEditor::RelativeFontChange

RESOLVED FIXED in mozilla15

Status

()

Core
Editor
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Tracking

Trunk
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

5 years ago
Created attachment 621289 [details] [diff] [review]
Part a: RelativeFontChangeHelper
Attachment #621289 - Flags: review?(ehsan)
(Assignee)

Comment 1

5 years ago
Created attachment 621290 [details] [diff] [review]
Part b: RelativeFontChangeOnNode
Attachment #621290 - Flags: review?(ehsan)
(Assignee)

Comment 2

5 years ago
Created attachment 621291 [details] [diff] [review]
Part c: RelativeFontChange
Attachment #621291 - Flags: review?(ehsan)
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.
Attachment #621289 - Flags: review?(ehsan) → review+
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!  :-)
Attachment #621290 - Flags: review?(ehsan) → review+

Updated

5 years ago
Attachment #621291 - Flags: review?(ehsan) → review+
(Assignee)

Comment 5

5 years ago
(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 :)
(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
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/mozilla-central/rev/25025cc03fb3
https://hg.mozilla.org/mozilla-central/rev/e0110ef13989
https://hg.mozilla.org/mozilla-central/rev/c795f1a41dae
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15

Updated

5 years ago
Depends on: 767684

Comment 8

5 years ago
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...

Updated

5 years ago
Depends on: 780035
No longer depends on: 780035
You need to log in before you can comment on or make changes to this bug.