1385789 - stylo: remove HasStateDependentStyle warnings

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P4)

Description

[Cameron McCormack (:heycam)]

They're just noise.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: Bug 1385789 - stylo: Remove HasStateDependentStyle warnings.

Review commit: https://reviewboard.mozilla.org/r/162920/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162920/

Comment 2

[Bobby Holley (:bholley)]

Hm, are we going to do the right thing for [1]?

It seems like we'd ideally fix callers to avoid calling these functions in the stylo case and then mark them unreachable.

[1] http://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/gfx/layers/apz/util/ActiveElementManager.cpp#173

Comment 3

[Cameron McCormack (:heycam)]

I wasn't aware of that call. :-(  Seems like no, and we should have a mechanism for querying whether we have rules that depend on :active.  I'll make us actively skip the HasStateDependentStyle() calls in stylo (from RestyleManager::ContentStateChangedInternal) and leave the remaining ones.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

I'm not sure why the warnings were put and whether the reason still applies, so redirecting to bholley.

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 8891904 [details]
Bug 1385789 - stylo: Remove HasStateDependentStyle warnings.

Canceling review per comment 2.

Comment 6

[Brad Werth [:bradwerth]]

Attachment: Bug 1385789: Refactor RestyleManager::ContentStateChangedInternal to move nsRestyleHint calculation out to GeckoRestyleManager.

Review commit: https://reviewboard.mozilla.org/r/174012/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174012/

Comment 7

[Brad Werth [:bradwerth]]

Attachment 8902429 [details] attempts to implement the approach in comment 2.

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 8902429 [details]
Bug 1385789: Refactor RestyleManager::ContentStateChangedInternal to move nsRestyleHint calculation out to GeckoRestyleManager.

https://reviewboard.mozilla.org/r/174012/#review179242

Seems like we should instead move all the restyle hint handling in this function into GeckoRestyleManager, and only leave the primaryFrame handling in the common code. We can also remove emilio's big comment at the callsite in ServoRestyleManager (since we're definitely committed to the snapshot approach at this point), and remove the unneeded hint arguments to the shared function.

Comment 9

[Brad Werth [:bradwerth]]

Comment on attachment 8902429 [details]
Bug 1385789: Refactor RestyleManager::ContentStateChangedInternal to move nsRestyleHint calculation out to GeckoRestyleManager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174012/diff/1-2/

Comment 10

[Brad Werth [:bradwerth]]

Attachment 8902429 [details] attempts the approach in comment 8.

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 8902429 [details]
Bug 1385789: Refactor RestyleManager::ContentStateChangedInternal to move nsRestyleHint calculation out to GeckoRestyleManager.

https://reviewboard.mozilla.org/r/174012/#review179316

Looks good, thanks.

::: layout/base/GeckoRestyleManager.cpp:299
(Diff revision 2)
>  
>    nsChangeHint changeHint;
> +  ContentStateChangedInternal(aElement, aStateMask, &changeHint);
> +
> +  // Assemble what we'll need to calculate the nsRestyleHint.
>    nsRestyleHint restyleHint;

Nit: Move this down right before the conditional block where it's initialized.

::: layout/base/GeckoRestyleManager.cpp:307
(Diff revision 2)
> +  if (primaryFrame) {
> +    pseudoType = primaryFrame->StyleContext()->GetPseudoType();
> +  }
> +
> +  StyleSetHandle styleSet = PresContext()->StyleSet();
> +  NS_ASSERTION(styleSet, "couldn't get style set");

I think we can remove this assertion, or at least make it a description-less MOZ_ASSERT.

Comment 12

[Brad Werth [:bradwerth]]

Comment on attachment 8902429 [details]
Bug 1385789: Refactor RestyleManager::ContentStateChangedInternal to move nsRestyleHint calculation out to GeckoRestyleManager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174012/diff/2-3/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/194d51097e24
Refactor RestyleManager::ContentStateChangedInternal to move nsRestyleHint calculation out to GeckoRestyleManager. r=bholley

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/194d51097e24

Comment 15

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Can we remove ServoStyleSet::HasStateDependentStyle()? Or we do still need to implement it in the future?

Comment 16

[Brad Werth [:bradwerth]]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #15)
> Can we remove ServoStyleSet::HasStateDependentStyle()? Or we do still need
> to implement it in the future?

I don't know. That's a question for Cameron.

Comment 17

[Bobby Holley (:bholley)]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #15)
> Can we remove ServoStyleSet::HasStateDependentStyle()? Or we do still need
> to implement it in the future?

So I think ideally we would just move this API to GeckoStyleSet.

However, there is one caller, for APZ:

http://searchfox.org/mozilla-central/rev/f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/gfx/layers/apz/util/ActiveElementManager.cpp#165

Right now stylo is always returning false from this API. Botond, will this cause problems with APZ?

Comment 18

[Botond Ballo [:botond]]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #17)
> Right now stylo is always returning false from this API. Botond, will this
> cause problems with APZ?

To provide some background for why APZ checks this:

When you click on an element like a button with a mouse, some nonzero amount of time elapses between the mouse-down and the mouse-up, during which time the element is in the :active state.

With touch events, however, when you tap an element with your finger, we generate synthetic mouse-down and mouse-up events in sequence after having seen the touch-up (we can't proactively generate a mouse-down on a touch-down, because the touch-down could be the start of scrolling instead of a tap). If we don't insert a delay between the synthetic mouse events, the element won't spend any noticeable amount of time in the :active state, and so the user won't see any corresponding visual feedback. To address this, we added a 10 ms (configurable via the 
"ui.touch_activation.duration_ms" pref) delay to ensure the element spends a bit of time in the :active state.

As an optimization, we omit this delay if the element isn't actually styled differently in the :active state. 

This optimization was added back in the B2G days because the delay was in the critical path of app startup times (bug 1083818). I'm not sure how important it is today - 10 ms seems like a very small delay that we could probably live with for every tap.

So, the preferable behaviour from APZ's point of view would be for HasStateDependentStyle() to return *true* if it can't provide an accurate answer. (Or we could use a True/False/DontKnow enum, and let the caller decide how to handle "DontKnow"). Or, if Stylo is never going to implement this, and going forward we will use Stylo everywhere, we could just remove the optimization (and thus the call site) from APZ.