Closed Bug 1385789 Opened 4 years ago Closed 4 years ago

stylo: remove HasStateDependentStyle warnings

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: heycam, Assigned: bradwerth)

References

Details

Attachments

(1 file, 1 obsolete file)

They're just noise.
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
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.
Flags: needinfo?(cam)
Attachment #8891904 - Flags: review?(xidorn+moz) → review?(bobbyholley)
I'm not sure why the warnings were put and whether the reason still applies, so redirecting to bholley.
Comment on attachment 8891904 [details]
Bug 1385789 - stylo: Remove HasStateDependentStyle warnings.

Canceling review per comment 2.
Attachment #8891904 - Flags: review?(bobbyholley)
Priority: -- → P4
Attachment 8902429 [details] attempts to implement the approach in comment 2.
Assignee: nobody → bwerth
Attachment #8891904 - Attachment is obsolete: true
Attachment #8902429 - Flags: review?(bobbyholley)
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.
Attachment #8902429 - Flags: review?(bobbyholley) → review-
Attachment 8902429 [details] attempts the approach in comment 8.
Flags: needinfo?(cam)
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.
Attachment #8902429 - Flags: review?(bobbyholley) → review+
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/194d51097e24
Refactor RestyleManager::ContentStateChangedInternal to move nsRestyleHint calculation out to GeckoRestyleManager. r=bholley
https://hg.mozilla.org/mozilla-central/rev/194d51097e24
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Can we remove ServoStyleSet::HasStateDependentStyle()? Or we do still need to implement it in the future?
(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.
Flags: needinfo?(cam)
(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?
Flags: needinfo?(cam) → needinfo?(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.
Flags: needinfo?(botond)
You need to log in before you can comment on or make changes to this bug.