Push state updates further out across beforesetattr/aftersetattr

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM
P2
normal
RESOLVED FIXED
7 years ago
3 months ago

People

(Reporter: bz, Assigned: bz)

Tracking

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(5 attachments)

See the FIXME comments bug 598833 is adding.
OS: Mac OS X → All
Hardware: x86 → All
Taking for now, but please feel free to steal!
Assignee: nobody → bzbarsky
Priority: -- → P2
Blocks: 1347640
Created attachment 8847950 [details] [diff] [review]
part 1.  Remove the generic attr preparsing mechanism from BeforeSetAttr and just preparse class attributes directly in the one place that needs to do it

This removes the requirement that BeforeSetAttr comes before AttributeWillChange
(which needs the preparsed new value).

MozReview-Commit-ID: 87C6Mjc7ARh
Attachment #8847950 - Flags: review?(bugs)
Created attachment 8847951 [details] [diff] [review]
part 2.  Move calls to BeforeSetAttr to after AttributeWillChange

This means that implementations of BeforeSetAttr no longer need to UpdateState.
Those UpdateState calls will be removed in a bit.

MozReview-Commit-ID: 1yEg5D4garD
Attachment #8847951 - Flags: review?(bugs)
Created attachment 8847952 [details] [diff] [review]
part 3.  Remove UpdateState calls in BeforeSetAttr

This is safe because we've already made our AttributeWillChange call, so no one
should be caring exactly what our state is at the end of BeforeSetAttr.  We will
still ensure we UpdateState before AttributeChanged.

MozReview-Commit-ID: BQOPVgHyC0H
Attachment #8847952 - Flags: review?(bugs)
Created attachment 8847953 [details] [diff] [review]
part 4.  Move calls to AfterSetAttr to before UpdateState when manipulating attributes

In particular, this lets us remove UpdateState() calls from AfterSetAttr.  That
change is next.

MozReview-Commit-ID: CFeft0E9o8m
Attachment #8847953 - Flags: review?(bugs)
Created attachment 8847954 [details] [diff] [review]
part 5.  Remove UpdateState calls from AfterSetAttr, since they are no longer needed there

MozReview-Commit-ID: DmCf6Ndno2i
Attachment #8847954 - Flags: review?(bugs)
Olli, I tagged you for review on this because it's pretty finicky, but if you think we should have someone else do them so they learn their way around this code, I'm up for that too.  Just redirect as needed.
nsAttrValueOrString::TakeParsedValue was rather confusing. Apparently it does the opposite of other Take* methods.
Attachment #8847950 - Flags: review?(bugs) → review+
Comment on attachment 8847953 [details] [diff] [review]
part 4.  Move calls to AfterSetAttr to before UpdateState when manipulating attributes

I like this. Having nsIMutationObserver stuff before and after the internal attribute change handling
Attachment #8847953 - Flags: review?(bugs) → review+
Attachment #8847954 - Flags: review?(bugs) → review+
Attachment #8847952 - Flags: review?(bugs) → review+
Comment on attachment 8847952 [details] [diff] [review]
part 3.  Remove UpdateState calls in BeforeSetAttr

So, could you explain this a bit.
blame isn't useful here since it misses bug# for the commit which added the relevant code (before it was changed to UpdateState).

I can't think of anything needing the state though. Was the idea that PresShell and others would need to get the state update information before AttributeWillChange?
Attachment #8847951 - Flags: review?(bugs) → review+
So the r+ for part 3 is conditional. Please explain it a bit.
> Was the idea that PresShell and others would need to get the state update information before AttributeWillChange?

The way this has worked has changed a lot over the years.  The most important invariant presshell relies on to get restyling right is that any time the state actually changes internally there will be a corresponding ContentStatesChanged sent immediately, without allowing other changes (e.g. to an attr value) to happen in between the internal state change and the notification.  UpdateState provides this invariant automatically: it both updates the internal state and sends the notification.

So from that point of view, the only requirement is that after we do something that could change the state we guarantee that we do an UpdateState.  That was already guaranteed on the attr change codepaths (modulo OOM early returns, but slightly incorrect rendering on OOM seems ok).

But I don't know whether there are other mutation observers that somehow rely on the actual element state reflecting the state of the world.  Given that after part 2 the mutation observers don't run between BeforeSetAttr and the UpdateState() calls in SetAttrAndNotify/UnsetAttr, this is no longer a worry.  So the can simply remove UpdateState calls in BeforeSetAttr.

I'll put this in the commit message.  Thank you for pointing out it needs it.
Thanks, that is a good explanation.

Comment 14

9 months ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c5daa842570
part 1.  Remove the generic attr preparsing mechanism from BeforeSetAttr and just preparse class attributes directly in the one place that needs to do it.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a15b3d3bf8e
part 2.  Move calls to BeforeSetAttr to after AttributeWillChange.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/8adbe06476ae
part 3.  Remove UpdateState calls in BeforeSetAttr.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a0e59a45020
part 4.  Move calls to AfterSetAttr to before UpdateState when manipulating attributes.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/4960128d3dc3
part 5.  Remove UpdateState calls from AfterSetAttr, since they are no longer needed there.  r=smaug

Comment 15

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0c5daa842570
https://hg.mozilla.org/mozilla-central/rev/2a15b3d3bf8e
https://hg.mozilla.org/mozilla-central/rev/8adbe06476ae
https://hg.mozilla.org/mozilla-central/rev/2a0e59a45020
https://hg.mozilla.org/mozilla-central/rev/4960128d3dc3
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Blocks: 1324500
You need to log in before you can comment on or make changes to this bug.