Last Comment Bug 716207 - Cleanup in nsTextEditRules
: Cleanup in nsTextEditRules
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: :Ms2ger
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-07 02:25 PST by :Ms2ger
Modified: 2012-01-11 05:00 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part a: DidUndo (1.87 KB, patch)
2012-01-07 02:25 PST, :Ms2ger
ehsan: review+
Details | Diff | Review
Part b: RemoveRedundantTrailingBR (6.81 KB, patch)
2012-01-07 02:26 PST, :Ms2ger
ehsan: review+
Details | Diff | Review

Description :Ms2ger 2012-01-07 02:25:59 PST
Created attachment 586652 [details] [diff] [review]
Part a: DidUndo
Comment 1 :Ms2ger 2012-01-07 02:26:20 PST
Created attachment 586653 [details] [diff] [review]
Part b: RemoveRedundantTrailingBR
Comment 2 :Ehsan Akhgari (busy, don't ask for review please) 2012-01-09 12:40:00 PST
Comment on attachment 586653 [details] [diff] [review]
Part b: RemoveRedundantTrailingBR

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

::: editor/libeditor/text/nsTextEditRules.cpp
@@ +1077,4 @@
>  
> +  // Rather than deleting this node from the DOM tree we should instead
> +  // morph this br into the bogus node
> +  elem->UnsetAttr(kNameSpaceID_None, nsGkAtoms::type, /* ? */ true);

Why the doubt?  :-)
Comment 3 :Ms2ger 2012-01-10 00:24:05 PST
Because the SetAttr right below it doesn't notify
Comment 4 :Ehsan Akhgari (busy, don't ask for review please) 2012-01-10 07:23:58 PST
(In reply to Ms2ger from comment #3)
> Because the SetAttr right below it doesn't notify

Sure it does: <http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsGenericElement.cpp#2575>
Comment 5 :Ms2ger 2012-01-10 07:26:37 PST
Comment on attachment 586653 [details] [diff] [review]
Part b: RemoveRedundantTrailingBR

Yeah, the current code does notify, but I was wondering about this:

>+  // give it the bogus node attribute
>+  elem->SetAttr(kNameSpaceID_None, kMOZEditorBogusNodeAttrAtom,
>+                kMOZEditorBogusNodeValue, false);
Comment 6 :Ehsan Akhgari (busy, don't ask for review please) 2012-01-10 10:02:32 PST
Yeah.  I don't know if we specifically _need_ to notify for removing the attribute (I think we don't) but I'd prefer this patch to not change that behavior.  I would be more than happy to review another patch in a new bug which turns off the notification.  That way we can deal with any possible future regressions better.  :-)

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