If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

stylo: remove HasStateDependentStyle warnings

RESOLVED FIXED in Firefox 57

Status

()

Core
CSS Parsing and Computation
P4
normal
RESOLVED FIXED
2 months ago
15 days ago

People

(Reporter: heycam, Assigned: bradwerth)

Tracking

(Blocks: 1 bug)

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 months ago
They're just noise.
Comment hidden (mozreview-request)
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
(Reporter)

Comment 3

2 months ago
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.
(Reporter)

Updated

2 months ago
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)
(Reporter)

Updated

2 months ago
Priority: -- → P4
Comment hidden (mozreview-request)
(Assignee)

Comment 7

23 days ago
Attachment 8902429 [details] attempts to implement the approach in comment 2.
Assignee: nobody → bwerth
(Assignee)

Updated

23 days ago
Attachment #8891904 - Attachment is obsolete: true
(Assignee)

Updated

23 days ago
Attachment #8902429 - Flags: review?(bobbyholley)

Comment 8

23 days ago
mozreview-review
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-
Comment hidden (mozreview-request)
(Assignee)

Comment 10

23 days ago
Attachment 8902429 [details] attempts the approach in comment 8.
Flags: needinfo?(cam)

Comment 11

23 days ago
mozreview-review
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+
Comment hidden (mozreview-request)

Comment 13

22 days ago
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
Last Resolved: 22 days ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Can we remove ServoStyleSet::HasStateDependentStyle()? Or we do still need to implement it in the future?
(Assignee)

Comment 16

16 days ago
(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.