Last Comment Bug 711862 - nsHTMLEditor::RemoveStyleInside may process the passed-in node even when aChildrenOnly is set
: nsHTMLEditor::RemoveStyleInside may process the passed-in node even when aChi...
Status: RESOLVED FIXED
[good first bug][mentor=ehsan]
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Isaac Aggrey [:isaac]
:
: Makoto Kato [:m_kato]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-18 12:16 PST by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2012-08-23 10:46 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Don't process node when aChildrenOnly is true (2.25 KB, patch)
2012-08-20 19:57 PDT, Isaac Aggrey [:isaac]
ehsan: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-18 12:16:33 PST
In bug 711799 dholbert notes that some editor logic looks a bit dodgy.  Specifically, nsHTMLEditor::RemoveStyleInside's current half-neglect of parentheses in conditions may obscure bugginess, in that an argument "aChildrenOnly" seems to not quite be used as its name would indicate.  Is there a bug here?

Even if there's no bug, there's still a pretty awful lack of disambiguating parentheses here that needs fixing.
Comment 1 Daniel Holbert [:dholbert] 2011-12-18 14:58:10 PST
(for reference, see bug 711799 comment 12)
Comment 2 :Ehsan Akhgari 2012-08-14 07:09:09 PDT
See bug 711799 comment 12 for an analysis of the problem.  The condition here <http://hg.mozilla.org/mozilla-central/annotate/22288130fea2/editor/libeditor/html/nsHTMLEditorStyle.cpp#l802> should be modified such that if aChildrenOnly is true, the entire condition always evaluates to false.
Comment 3 Isaac Aggrey [:isaac] 2012-08-20 19:39:00 PDT
I'd like to tackle this as my first bug, could someone assign it to me? I'll follow up with a (proposed) patch very soon.
Comment 4 Isaac Aggrey [:isaac] 2012-08-20 19:57:18 PDT
Created attachment 653633 [details] [diff] [review]
Don't process node when aChildrenOnly is true

I've used Ehsan's comments as a reference, so when aChildrenOnly is true, the entire condition is false.

As I was taking a look at the code, it also seems like the conditional here <https://hg.mozilla.org/mozilla-central/annotate/22288130fea2/editor/libeditor/html/nsHTMLEditorStyle.cpp#l879> leads to processing some nodes -- could aChildrenOnly also be applicable here? In other words, if aChildrenOnly is set, should it simply return NS_OK?

I'm not sure what testing needs to be done or if a simple test needs to be made, but I ran the tests in the mozilla-central/editor folder.

I welcome any constructive feedback or suggestions on where I should go from here if there are improvements to be made.
Comment 5 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-08-20 20:30:03 PDT
Comment on attachment 653633 [details] [diff] [review]
Don't process node when aChildrenOnly is true

Isaac, for future reference setting a flag on your patch (press the Details link on the attachment) is the preferred way of asking for specific feedback/review.
Comment 6 :Ehsan Akhgari 2012-08-21 07:43:00 PDT
Comment on attachment 653633 [details] [diff] [review]
Don't process node when aChildrenOnly is true

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

Thanks Isaac for your patch!  It looks good.

(In reply to Isaac Aggrey from comment #4)
> As I was taking a look at the code, it also seems like the conditional here
> <https://hg.mozilla.org/mozilla-central/annotate/22288130fea2/editor/
> libeditor/html/nsHTMLEditorStyle.cpp#l879> leads to processing some nodes --
> could aChildrenOnly also be applicable here? In other words, if
> aChildrenOnly is set, should it simply return NS_OK?

Yeah, that's probably wrong as well.  Would you please make all of that conditional on aChildrenOnly too and run the tests in editor/ to see if anything obvious breaks?  If it doesn't, please file a new bug for that and attach your other patch on that bug and ask me for review.  Thanks!

> I'm not sure what testing needs to be done or if a simple test needs to be
> made, but I ran the tests in the mozilla-central/editor folder.

I usually run those tests locally when making editor changes.  When they pass, the usual practice is to submit the patch to the try server <https://wiki.mozilla.org/ReleaseEngineering/TryServer> which is a testing environment which can build Firefox with your patches and run all of our tests on it to make sure that the tests pass on all platforms.  I have submitted your patch to the try server, and you can find a link to a page which shows the test results on all platforms as they become available here: <https://tbpl.mozilla.org/?tree=Try&rev=0c2358a37ae1>.  When this patch passes all of the tests, it's ready to be checked in!
Comment 7 :Ehsan Akhgari 2012-08-21 11:32:58 PDT
The try server job looked fairly green, so I went ahead and checked in your patch!

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a520dc23009

Congratulations on your first patch to Mozilla!  :-)
Comment 8 Mozilla RelEng Bot 2012-08-21 12:00:30 PDT
Try run for 0c2358a37ae1 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=0c2358a37ae1
Results (out of 205 total builds):
    exception: 2
    success: 159
    warnings: 19
    failure: 25
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-0c2358a37ae1
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-08-21 19:09:03 PDT
https://hg.mozilla.org/mozilla-central/rev/9a520dc23009
Comment 10 Isaac Aggrey [:isaac] 2012-08-21 22:00:09 PDT
(In reply to Josh Matthews [:jdm] from comment #5)
> Isaac, for future reference setting a flag on your patch (press the Details
> link on the attachment) is the preferred way of asking for specific
> feedback/review.

Thanks for that, Jason!

(In reply to Ehsan Akhgari [:ehsan] from comment #6)
> Yeah, that's probably wrong as well.  Would you please make all of that
> conditional on aChildrenOnly too and run the tests in editor/ to see if
> anything obvious breaks?  If it doesn't, please file a new bug for that and
> attach your other patch on that bug and ask me for review.  Thanks!

I sure can; I'll keep you posted.

(In reply to Ehsan Akhgari [:ehsan] from comment #7)
> Congratulations on your first patch to Mozilla!  :-)

Fantastic! I'm looking forward to contribute in even bigger ways. Thanks for all of your help, Ehsan!
Comment 11 :Ehsan Akhgari 2012-08-22 12:28:25 PDT
(In reply to comment #10)
> (In reply to Ehsan Akhgari [:ehsan] from comment #7)
> > Congratulations on your first patch to Mozilla!  :-)
> 
> Fantastic! I'm looking forward to contribute in even bigger ways. Thanks for
> all of your help, Ehsan!

Cool!  Let me know if you're looking for more stuff to work on.  I've got a number of cleanup ideas which I'll file bugs for shortly.
Comment 12 Isaac Aggrey [:isaac] 2012-08-23 10:46:38 PDT
(In reply to Isaac Aggrey from comment #10)
> (In reply to Josh Matthews [:jdm] from comment #5)
> > Isaac, for future reference setting a flag on your patch (press the Details
> > link on the attachment) is the preferred way of asking for specific
> > feedback/review.
> 
> Thanks for that, Jason!
> 

Oops. Sorry, I meant Josh! I have no idea where that came from.

(In reply to Ehsan Akhgari [:ehsan] from comment #11) 
> Cool!  Let me know if you're looking for more stuff to work on.  I've got a
> number of cleanup ideas which I'll file bugs for shortly.

Yes, very interested. I'll touch bases with you again via email since my comment is kinda irrelevant to the bug now. ;-) Thanks again.

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