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

Status: NEW

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

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

Comment 1

[Emilio Cobos Álvarez (:emilio)]

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

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Try run with the flush and checks removed (hasn't finished, just for reference): https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6d8527406fff98f5393e124d8791326404ce4de

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 5

[Emilio Cobos Álvarez (:emilio)]

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

Comment 6

[Emilio Cobos Álvarez (:emilio)]

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?

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Those test cases -> The test cases over bug 302561, which is what that test claims to be testing.

Comment 8

[Bobby Holley (:bholley)]

Is this still an issue? If so, does it block shipping?

Comment 9

[Emilio Cobos Álvarez (:emilio)]

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