Last Comment Bug 752210 - Use nsIContent in nsHTMLEditor::RelativeFontChange
: Use nsIContent in nsHTMLEditor::RelativeFontChange
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: :Ms2ger (⌚ UTC+1/+2)
:
Mentors:
Depends on: 767684
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-05 06:39 PDT by :Ms2ger (⌚ UTC+1/+2)
Modified: 2012-08-09 07:23 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part a: RelativeFontChangeHelper (5.00 KB, patch)
2012-05-05 06:39 PDT, :Ms2ger (⌚ UTC+1/+2)
ehsan: review+
Details | Diff | Splinter Review
Part b: RelativeFontChangeOnNode (8.21 KB, patch)
2012-05-05 06:40 PDT, :Ms2ger (⌚ UTC+1/+2)
ehsan: review+
Details | Diff | Splinter Review
Part c: RelativeFontChange (3.90 KB, patch)
2012-05-05 06:40 PDT, :Ms2ger (⌚ UTC+1/+2)
ehsan: review+
Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2012-05-05 06:39:05 PDT
Created attachment 621289 [details] [diff] [review]
Part a: RelativeFontChangeHelper
Comment 1 :Ms2ger (⌚ UTC+1/+2) 2012-05-05 06:40:18 PDT
Created attachment 621290 [details] [diff] [review]
Part b: RelativeFontChangeOnNode
Comment 2 :Ms2ger (⌚ UTC+1/+2) 2012-05-05 06:40:49 PDT
Created attachment 621291 [details] [diff] [review]
Part c: RelativeFontChange
Comment 3 :Ehsan Akhgari 2012-05-07 07:44:14 PDT
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 :Ehsan Akhgari 2012-05-07 07:46:25 PDT
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 (⌚ UTC+1/+2) 2012-05-13 02:54:13 PDT
(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 :Ehsan Akhgari 2012-05-14 10:42:25 PDT
(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 8 neil@parkwaycc.co.uk 2012-07-01 08:30:30 PDT
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...

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