nsHTMLEditor::RemoveStyleInside may (still) process the passed-in node even when aChildrenOnly is set

RESOLVED FIXED in mozilla18

Status

()

--
trivial
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: isaac, Assigned: isaac)

Tracking

Trunk
mozilla18
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
Created attachment 654683 [details] [diff] [review]
Don't process any node when aChildrenOnly is true

See bug 711862 for background.

nsHTMLEditor::RemoveStyleInside appears to still process some nodes when the contextual code implies children should be processed first and no nodes processed afterwards.

With the attached patch, nsHTMLEditor::RemoveStyleInside will now only process children then the node itself only if aChildrenOnly is false (i.e., if it's true, it will simply skip processing the node itself and return NS_OK).

Aside: For clarity, it may even be preferable to have a single conditional checking for aChildrenOnly being false rather than three separate conditionals that include aChildrenOnly, but that's more of a style choice.
Attachment #654683 - Flags: review?(ehsan)

Comment 1

6 years ago
Comment on attachment 654683 [details] [diff] [review]
Don't process any node when aChildrenOnly is true

Thanks!

Do you have try server access?  If not, can you please file a bug for that and CC me on it to vouch for you?  Once you get try server access, you should push this patch to the try server to make sure that it doesn't break anything!
Attachment #654683 - Flags: review?(ehsan) → review+
(Assignee)

Comment 2

6 years ago
Not yet, but I'm on it!

Comment 3

6 years ago
Try run for d992868f83ca is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d992868f83ca
Results (out of 84 total builds):
    success: 74
    warnings: 9
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/isaac.aggrey@gmail.com-d992868f83ca
(Assignee)

Comment 4

6 years ago
(In reply to Ehsan Akhgari [:ehsan] from comment #1)
> Once you get try server access, you should push this patch to the try 
> server to make sure that it doesn't break anything!

It looks like the try server job is mighty green, so it doesn't like I broke anything!
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

6 years ago
Assignee: nobody → isaac.aggrey

Comment 5

6 years ago
Try run for d992868f83ca is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d992868f83ca
Results (out of 85 total builds):
    success: 74
    warnings: 9
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/isaac.aggrey@gmail.com-d992868f83ca

Comment 6

6 years ago
Thanks again for your patch, and sorry for the delay in landing it.  I need to get better at staying on top of my bugmail!  :-)

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9c3f1cd372a
Target Milestone: --- → mozilla18

Comment 7

6 years ago
https://hg.mozilla.org/mozilla-central/rev/f9c3f1cd372a
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.