stylo: remove HasStateDependentStyle warnings

RESOLVED FIXED in Firefox 57

Status

()

P4
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: heycam, Assigned: bradwerth)

Tracking

(Blocks: 1 bug)

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years 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 years 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 years 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 years ago
Priority: -- → P4
Comment hidden (mozreview-request)
(Assignee)

Comment 7

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

Updated

2 years ago
Attachment #8891904 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8902429 - Flags: review?(bobbyholley)

Comment 8

2 years 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

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

Comment 11

2 years 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

2 years 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: 2 years 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

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