Last Comment Bug 703239 - fix build Warnings in editor/
: fix build Warnings in editor/
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Atul Aggarwal
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-17 03:55 PST by Atul Aggarwal
Modified: 2011-12-24 22:16 PST (History)
2 users (show)
philringnalda: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Warnings (2.65 KB, text/plain)
2011-11-17 03:55 PST, Atul Aggarwal
no flags Details
Patch v1 (13.71 KB, patch)
2011-11-17 04:05 PST, Atul Aggarwal
ehsan: review+
Details | Diff | Review
Patch v1.10 (13.43 KB, patch)
2011-11-29 06:31 PST, Atul Aggarwal
ehsan: review+
Details | Diff | Review
Patch v1.50 (12.72 KB, patch)
2011-12-22 10:18 PST, Atul Aggarwal
no flags Details | Diff | Review

Description Atul Aggarwal 2011-11-17 03:55:45 PST
Created attachment 575140 [details]
Warnings

Patch coming shortly.
Comment 1 Atul Aggarwal 2011-11-17 04:05:35 PST
Created attachment 575141 [details] [diff] [review]
Patch v1
Comment 2 :Ehsan Akhgari (busy, don't ask for review please) 2011-11-25 11:21:49 PST
Comment on attachment 575141 [details] [diff] [review]
Patch v1

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

This is great!  r=me with the comments below.

::: editor/composer/src/nsComposerController.cpp
@@ +44,5 @@
>  
>  #define NS_REGISTER_ONE_COMMAND(_cmdClass, _cmdName)                    \
>    {                                                                     \
>      _cmdClass* theCmd = new _cmdClass();                                \
> +    NS_ENSURE_TRUE(theCmd, NS_ERROR_OUT_OF_MEMORY);                     \

Now that you're here, can you please remove this NS_ENSURE_TRUE (and others below like it) too?  operator new is infallible these days.

Thanks!

::: editor/libeditor/html/nsHTMLEditor.cpp
@@ +935,5 @@
>  {
>    if (!aNode)
>    {
>      NS_NOTREACHED("null node passed to GetBlockNodeParent()");
> +    return nsnull;

Ouch!

::: editor/libeditor/html/nsHTMLEditorStyle.cpp
@@ +672,1 @@
>          (!aProperty && NodeIsProperty(aNode)))  // or node is any prop and we asked for that

Please remove the double parenthesis after if.  Also, remove the trailing space after '(' on the first line.
Comment 3 Atul Aggarwal 2011-11-29 06:31:18 PST
Created attachment 577594 [details] [diff] [review]
Patch v1.10
Comment 4 Atul Aggarwal 2011-11-29 06:34:41 PST
(In reply to Ehsan Akhgari [:ehsan] from comment #2)
> Comment on attachment 575141 [details] [diff] [review] [diff] [details] [review]
> Patch v1
> 
> Review of attachment 575141 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> This is great!  r=me with the comments below.
> 
> ::: editor/composer/src/nsComposerController.cpp
> Now that you're here, can you please remove this NS_ENSURE_TRUE (and others
> below like it) too?  operator new is infallible these days.
> 
Done.
> 
> ::: editor/libeditor/html/nsHTMLEditor.cpp
> @@ +935,5 @@
> >  {
> >    if (!aNode)
> >    {
> >      NS_NOTREACHED("null node passed to GetBlockNodeParent()");
> > +    return nsnull;
> 
> Ouch!
This was intentional. Did I miss something?
> 
> ::: editor/libeditor/html/nsHTMLEditorStyle.cpp
> @@ +672,1 @@
> >          (!aProperty && NodeIsProperty(aNode)))  // or node is any prop and we asked for that
> 
> Please remove the double parenthesis after if.  Also, remove the trailing
> space after '(' on the first line.

Removed the trailing space but double parenthesis was intentional to shut compiler from throwing warnings.

Please review so that I can mark this bug checkin-needed.

Thanks !
Comment 5 :Ehsan Akhgari (busy, don't ask for review please) 2011-12-21 14:32:32 PST
Comment on attachment 577594 [details] [diff] [review]
Patch v1.10

The "ouch" was because we'd missed that case all this time!  Great work, thanks a lot!
Comment 6 Atul Aggarwal 2011-12-21 21:13:40 PST
Thanks for reviewing :).
Comment 7 Dão Gottwald [:dao] 2011-12-22 02:50:22 PST
patching file editor/libeditor/html/nsHTMLEditor.cpp
Hunk #1 FAILED at 932

:(
Comment 8 Atul Aggarwal 2011-12-22 10:18:54 PST
Created attachment 583847 [details] [diff] [review]
Patch v1.50

rebase on current code.
Comment 10 Phil Ringnalda (:philor) 2011-12-24 22:16:41 PST
https://hg.mozilla.org/mozilla-central/rev/1f0062478a1f

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