stylo: The hover generation is never updated, and we seem to be fine with it.

NEW
Unassigned

Status

()

defect
P3
normal
2 years ago
2 years ago

People

(Reporter: emilio, Unassigned)

Tracking

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 wontfix, firefox58 affected)

Details

So, I was looking at removing the super-noisy HasStateDependentStyle warning on stylo (and cleaning up that code a bit, since is handled correctly via the snapshot stuff), when I realized of an interesting condition:

  if (aStateMask.HasState(NS_EVENT_STATE_HOVER) && *aOutRestyleHint != 0) {
    IncrementHoverGeneration();
  }

*aOutRestyleHint is always zero for stylo, so that condition is effectively unreachable for stylo.

Seems like the only place it's checked is in the PresShell[1].

It's not totally clear to me yet why that recursive reflow is necessary, but seems like something we should sort out.

[1]: http://searchfox.org/mozilla-central/rev/77b256dc98efb93f1d118db456f442a09bba77c2/layout/base/PresShell.cpp#3749
So that was added in bug 302561, and seems not to be needed anymore? At least stylo definitely passes test_hover.html, and the test-cases in that bug don't have any unexpected behaviour...
Try run with the flush and checks removed (hasn't finished, just for reference): https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6d8527406fff98f5393e124d8791326404ce4de
What you're probably seeing is because test_hover was broken by changes to the test such that it doesn't test what it was intending to test anymore.  There's some discussion of that in bug 881832 comment 46 etc. and comment 58 etc., but I think there's also a separate bug on it that I can't find.  (comment 59 references a patch I had that I don't see there)

I might be able to search more later but don't have time right now.
I guess I was planning to fix it myself, but never got to figuring out how to finish landing the patch, which is still in my queue at:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/00ed9abbd0dc/test-hover-oscillate
And this leds me to realise that we're probably not even testing stylo with that test anyway, given it's a chrome test, so it'll use Gecko's style system...
That being said, that test fix of course makes the tests fail with my patch above (on Gecko), so it seems it's really needed... 

We'll have to figure out a way of fixing that I guess... Though Stylo's behaviour is pretty much the same as Gecko's and Blink's on those test-cases... Is the difference more subtle than that?
Those test cases -> The test cases over bug 302561, which is what that test claims to be testing.
Is this still an issue? If so, does it block shipping?
Flags: needinfo?(emilio+bugs)
> Is this still an issue? If so, does it block shipping?

I think it is... But not sure it blocks shipping. I'd say not, per comment 6, but maybe David has a different opinion.
Flags: needinfo?(emilio+bugs)
Priority: -- → P3
Blocks: 1393109
No longer blocks: 1393109
You need to log in before you can comment on or make changes to this bug.