589686 - Fix uses of ContentStatesChanged and MOZ_AUTO_DOC_UPDATE in content/html/content/*.cpp

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Mounir Lamouri (:mounir)]

See: https://bugzilla.mozilla.org/show_bug.cgi?id=558788#c29

Comment 1

[Mounir Lamouri (:mounir)]

This has to be fixed before we ship.

Comment 2

[Mounir Lamouri (:mounir)]

Looks like nsGenericHTMLElement.cpp also has some bad uses so let have this bug fixing all of them.

Comment 3

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 471646 [details] [diff] [review]
Patch v1

>+++ b/content/html/content/src/nsGenericHTMLElement.cpp
> MakeContentDescendantsEditable(nsIContent *aContent, nsIDocument *aDocument)
>+    MOZ_AUTO_DOC_UPDATE(aDocument, UPDATE_CONTENT_STATE, PR_TRUE);

Since this function is recursive, we could end up with tons of update begin/ends here in the common case.  Why not just do a single update around the call in nsGenericHTMLElement::ChangeEditableState?

And maybe file a followup bug to make this iterative instead of recursive?

r=bzbarsky with that.

Comment 5

[Mounir Lamouri (:mounir)]

I guess I can add MOZ_AUTO_DOC_UPDATE(document, UPDATE_CONTENT_STATE, PR_TRUE); before the initial MakeContentDescendantEditable call. This might be useless if |aDocument && stateBefore != aContent->IntrinsicState()| is always false but I guess it's better than having it called at each recursion step.

I will open a derecursification bug too.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> but I guess it's better than having it called at each recursion step.

Right.  And I would be surprised if it's always false.

Comment 7

[Mounir Lamouri (:mounir)]

(In reply to comment #5)
> I will open a derecursification bug too.

bug 594472

Comment 8

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/b4cbd426c547