Last Comment Bug 657353 - Stop sending BeginUpdate/EndUpdate on content state changes
: Stop sending BeginUpdate/EndUpdate on content state changes
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
:
Mentors:
Depends on: 691215
Blocks: 653887 598833
  Show dependency treegraph
 
Reported: 2011-05-16 07:45 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2011-10-02 18:29 PDT (History)
7 users (show)
bzbarsky: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1. Switch the XML prettyprinter to using a script runner instead of EndUpdate to determine when to drop prettyprinting. (4.69 KB, patch)
2011-05-16 07:46 PDT, Boris Zbarsky [:bz] (still a bit busy)
jonas: review+
Details | Diff | Splinter Review
part 2. Switch content state updates away from calling Begin/EndUpdate. (41.28 KB, patch)
2011-05-16 07:47 PDT, Boris Zbarsky [:bz] (still a bit busy)
jonas: review+
Details | Diff | Splinter Review
The test fix that I pushed (858 bytes, patch)
2011-05-31 21:21 PDT, Boris Zbarsky [:bz] (still a bit busy)
mounir: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (still a bit busy) 2011-05-16 07:45:38 PDT
It's slow, we don't need it.  See bug 653887.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2011-05-16 07:46:11 PDT
Created 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.
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2011-05-16 07:47:31 PDT
Created attachment 532636 [details] [diff] [review]
part 2.  Switch content state updates away from calling Begin/EndUpdate.
Comment 3 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-05-27 17:58:36 PDT
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 [:bz] (still a bit busy) 2011-05-27 19:31:17 PDT
> No need to handle the failure case here.

Excellent.  Fixed.
Comment 5 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-05-31 14:21:00 PDT
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 [:bz] (still a bit busy) 2011-05-31 14:39:12 PDT
> 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 [:bz] (still a bit busy) 2011-05-31 21:20:47 PDT
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 [:bz] (still a bit busy) 2011-05-31 21:21:21 PDT
Created attachment 536519 [details] [diff] [review]
The test fix that I pushed
Comment 10 Mounir Lamouri (:mounir) 2011-06-01 01:38:25 PDT
and:
http://hg.mozilla.org/mozilla-central/rev/963cd7a109be
Comment 11 Mounir Lamouri (:mounir) 2011-06-01 02:49:01 PDT
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] 2011-08-23 01:26:13 PDT
Could you give me a testcase on how i can verify that the issue was fixed?
Thanks.
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2011-08-23 01:34:31 PDT
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.

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