Stop sending BeginUpdate/EndUpdate on content state changes

RESOLVED FIXED in mozilla7

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Blocks: 1 bug)

Trunk
mozilla7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

6 years ago
It's slow, we don't need it.  See bug 653887.
(Assignee)

Comment 1

6 years ago
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.
(Assignee)

Updated

6 years ago
Attachment #532635 - Flags: review?(jonas)
(Assignee)

Comment 2

6 years ago
Created attachment 532636 [details] [diff] [review]
part 2.  Switch content state updates away from calling Begin/EndUpdate.
Attachment #532636 - Flags: review?(jonas)
(Assignee)

Updated

6 years ago
Blocks: 598833
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)
Attachment #532635 - Flags: review?(jonas) → review+
(Assignee)

Comment 4

6 years ago
> No need to handle the failure case here.

Excellent.  Fixed.
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
Attachment #532636 - Flags: review?(jonas) → review+
(Assignee)

Comment 6

6 years ago
> 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.
(Assignee)

Comment 7

6 years ago
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.
Flags: in-testsuite-
Whiteboard: [need review] → fixed-in-cedar
Target Milestone: --- → mozilla7
(Assignee)

Comment 8

6 years ago
Created attachment 536519 [details] [diff] [review]
The test fix that I pushed
Attachment #536519 - Flags: review?(mounir.lamouri)
Pushed:
http://hg.mozilla.org/mozilla-central/rev/d47a10acc66d
http://hg.mozilla.org/mozilla-central/rev/acd4a2d5d3d1
Status: NEW → RESOLVED
Last Resolved: 6 years ago
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
and:
http://hg.mozilla.org/mozilla-central/rev/963cd7a109be
Status: RESOLVED → UNCONFIRMED
Ever confirmed: false
OS: All → Mac OS X
Hardware: All → x86
Resolution: FIXED → ---
Status: UNCONFIRMED → RESOLVED
Last Resolved: 6 years ago6 years ago
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Comment on attachment 536519 [details] [diff] [review]
The test fix that I pushed

Review of attachment 536519 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #536519 - Flags: review?(mounir.lamouri) → review+
Could you give me a testcase on how i can verify that the issue was fixed?
Thanks.
(Assignee)

Comment 13

6 years ago
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.

Updated

6 years ago
Depends on: 691215
You need to log in before you can comment on or make changes to this bug.