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
:
Mentors:
Depends on: 767684
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-05 06:39 PDT by :Ms2ger
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
ehsan: review+
Details | Diff | Splinter Review
Part b: RelativeFontChangeOnNode (8.21 KB, patch)
2012-05-05 06:40 PDT, :Ms2ger
ehsan: review+
Details | Diff | Splinter Review
Part c: RelativeFontChange (3.90 KB, patch)
2012-05-05 06:40 PDT, :Ms2ger
ehsan: review+
Details | Diff | Splinter Review

Description :Ms2ger 2012-05-05 06:39:05 PDT
Created attachment 621289 [details] [diff] [review]
Part a: RelativeFontChangeHelper
Comment 1 :Ms2ger 2012-05-05 06:40:18 PDT
Created attachment 621290 [details] [diff] [review]
Part b: RelativeFontChangeOnNode
Comment 2 :Ms2ger 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 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.