657353 - Stop sending BeginUpdate/EndUpdate on content state changes

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

It's slow, we don't need it.  See bug 653887.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Switch the XML prettyprinter to using a script runner instead of EndUpdate to determine when to drop prettyprinting.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Switch content state updates away from calling Begin/EndUpdate.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 532635 [details] [diff] [review]
part 1.  Switch the XML prettyprinter to using a script runner instead of EndUpdate to determine when to drop prettyprinting.

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

r=me

::: content/xml/document/src/nsXMLPrettyPrinter.cpp
@@ +203,5 @@
>          mUnhookPending = PR_TRUE;
> +        if (!nsContentUtils::AddScriptRunner(
> +          NS_NewRunnableMethod(this, &nsXMLPrettyPrinter::Unhook))) {
> +            mUnhookPending = PR_FALSE;
> +        }

No need to handle the failure case here. AddScriptRunner no longer can fail due to nsTArray using infallible malloc.

(we should make it return void, but someone needs to write that patch)

Comment 4

[Boris Zbarsky [:bzbarsky]]

> No need to handle the failure case here.

Excellent.  Fixed.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 532636 [details] [diff] [review]
part 2.  Switch content state updates away from calling Begin/EndUpdate.

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

My money is on that a lot of these aren't needed, but better stay on the safe side. But do add an assertion to nsDocument::ContentStateChanged that checks that we're inside a scriptblocker so that it'll be easier to remove them in the future.

::: content/base/src/nsGenericElement.cpp
@@ +4735,5 @@
>  
>    if (aNotify) {
>      stateMask ^= IntrinsicState();
>      if (document && !stateMask.IsEmpty()) {
> +      nsAutoScriptBlocker scriptBlocker;

Shouldn't we already be in a scriptblocker at this point? Wanna change this into just an assertion?

@@ +4980,5 @@
>  
>    if (aNotify) {
>      stateMask ^= IntrinsicState();
>      if (document && !stateMask.IsEmpty()) {
> +      nsAutoScriptBlocker scriptBlocker;

Same here.

::: content/html/content/src/nsGenericHTMLElement.cpp
@@ +2671,5 @@
>        // Go ahead and notify on that change.
>        // Note: no need to notify on CanBeDisabled(), since type attr
>        // changes can't affect that.
>        if (doc && aNotify) {
> +        nsAutoScriptBlocker scriptBlocker;

This one also doesn't seem needed for the same reason.

@@ +2988,5 @@
>    aStates |= NS_EVENT_STATE_DISABLED | NS_EVENT_STATE_ENABLED;
>  
>    nsIDocument* doc = GetCurrentDoc();
>    if (doc) {
> +    nsAutoScriptBlocker scriptBlocker;

I *think* this one isn't needed either.

::: content/html/content/src/nsHTMLButtonElement.cpp
@@ +579,5 @@
>  
>      if (aNotify && !states.IsEmpty()) {
>        nsIDocument* doc = GetCurrentDoc();
>        if (doc) {
> +        nsAutoScriptBlocker scriptBlocker;

Not needed.

::: content/html/content/src/nsHTMLFormElement.cpp
@@ +384,5 @@
>      // Update all form elements states because they might be [no longer]
>      // affected by :-moz-ui-valid or :-moz-ui-invalid.
>      nsIDocument* doc = GetCurrentDoc();
>      if (doc) {
> +      nsAutoScriptBlocker scriptBlocker;

Not needed

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +953,5 @@
>          states |= NS_EVENT_STATE_MOZ_READONLY | NS_EVENT_STATE_MOZ_READWRITE;
>        }
>  
>        if (doc && !states.IsEmpty()) {
> +        nsAutoScriptBlocker scriptBlocker;

Not needed

::: content/html/content/src/nsHTMLTextAreaElement.cpp
@@ +1223,5 @@
>          states |= NS_EVENT_STATE_MOZ_READONLY | NS_EVENT_STATE_MOZ_READWRITE;
>        }
>  
>        if (doc && !states.IsEmpty()) {
> +        nsAutoScriptBlocker scriptBlocker;

Not needed

::: content/xul/content/src/nsXULElement.cpp
@@ +1470,5 @@
>  
>      if (aNotify) {
>          stateMask ^= IntrinsicState();
>          if (doc && !stateMask.IsEmpty()) {
> +            nsAutoScriptBlocker scriptBlocker;

Not needed

Comment 6

[Boris Zbarsky [:bzbarsky]]

> do add an assertion to nsDocument::ContentStateChanged 

Done.

> Shouldn't we already be in a scriptblocker at this point? 

Yes.  The nsDocument assert will catch it.  This used to not be the case when we had removable scriptblockers.

> I *think* this one isn't needed either.

It will be later; I'd rather keep it.

Removed the others.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Pushed:
  http://hg.mozilla.org/projects/cedar/rev/d47a10acc66d
  http://hg.mozilla.org/projects/cedar/rev/acd4a2d5d3d1

then pushed http://hg.mozilla.org/projects/cedar/rev/963cd7a109be to fix the resulting asserts.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: The test fix that I pushed

Comment 9

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/d47a10acc66d
http://hg.mozilla.org/mozilla-central/rev/acd4a2d5d3d1

Comment 10

[Mounir Lamouri (:mounir)]

and:
http://hg.mozilla.org/mozilla-central/rev/963cd7a109be

Comment 11

[Mounir Lamouri (:mounir)]

Comment on attachment 536519 [details] [diff] [review]
The test fix that I pushed

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

Comment 12

[Trif Andrei-Alin[:AlinT]]

Could you give me a testcase on how i can verify that the issue was fixed?
Thanks.

Comment 13

[Boris Zbarsky [:bzbarsky]]

You can't very easily, short of reading the code and verifying that the change was made.  This was just an architectural change laying the groundwork for other changes without a huge amount of user-visible impact.  Maybe a small performance win in some cases.