656197 - Push state updates further out across beforesetattr/aftersetattr

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

See the FIXME comments bug 598833 is adding.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Taking for now, but please feel free to steal!

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: 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

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: 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

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: 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

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: 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

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Remove UpdateState calls from AfterSetAttr, since they are no longer needed there

MozReview-Commit-ID: DmCf6Ndno2i

Comment 7

[Boris Zbarsky [:bzbarsky]]

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.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

nsAttrValueOrString::TakeParsedValue was rather confusing. Apparently it does the opposite of other Take* methods.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

So the r+ for part 3 is conditional. Please explain it a bit.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Thanks, that is a good explanation.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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