Use invalidation sets for :hover, :active, and :focus.

Use invalidation sets for :hover, :active, and :focus.

Store an invalidation set for each of :hover, :active, and :focus and use
those instead of SubtreeStyleChange when the state changes for elements which
have childrenAffectedBy*.

R=esprehn@chromium.org,chrishtr@chromium.org
BUG=351613
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=172703

[rune, 2014-04-01 12:29:26]

This works, but I think it needs some cleanup first.

[rune, 2014-04-02 11:39:05]

Ready for review now.

I couldn't decide if it was better to have a single method taking a pseudo class
type, or one for each of :hover/:active/:focus, so I'm currently duplicating
methods for the pseudo classes.

Got rid of the long if-test before calling invalidationSetForSelector since the
same if-test was done there. Could be less performant. Let me hear what you
think.

[esprehn, 2014-04-04 22:48:26]

https://codereview.chromium.org/220943002/diff/50001/Source/core/css/Affected...
File Source/core/css/AffectedByFocusTest.cpp (right):

https://codereview.chromium.org/220943002/diff/50001/Source/core/css/Affected...
Source/core/css/AffectedByFocusTest.cpp:296: "<div></div>"
Can we just write the normal layout tests like we've been doing for this
feature? I'd rather not have the tests in so many places.

https://codereview.chromium.org/220943002/diff/50001/Source/core/css/HoverUpd...
File Source/core/css/HoverUpdateTest.cpp (right):

https://codereview.chromium.org/220943002/diff/50001/Source/core/css/HoverUpd...
Source/core/css/HoverUpdateTest.cpp:61: "<span></span>"
Why not a regular layout test like we did for all the other ones?

https://codereview.chromium.org/220943002/diff/50001/Source/core/css/RuleFeat...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/220943002/diff/50001/Source/core/css/RuleFeat...
Source/core/css/RuleFeature.cpp:179: return &ensureHoverInvalidationSet();
Lets use a HashMap<CSSSelector::PseudoType, DescendantInvalidationSet> instead
and get rid of all the duplication. We probably want to expand this to many more
things than just :hover, :active and :focus.

[rune, 2014-04-08 16:28:25]

https://codereview.chromium.org/220943002/diff/50001/Source/core/css/HoverUpd...
File Source/core/css/HoverUpdateTest.cpp (right):

https://codereview.chromium.org/220943002/diff/50001/Source/core/css/HoverUpd...
Source/core/css/HoverUpdateTest.cpp:61: "<span></span>"
On 2014/04/04 22:48:27, esprehn wrote:
> Why not a regular layout test like we did for all the other ones?

Because hover/active/focus changes are synchronously recalculated and cannot be
caught by internals.updateStyleAndReturnAffectedElementCount(). I'm looking at
how to fix the synchronous update.

https://codereview.chromium.org/220943002/diff/50001/Source/core/css/RuleFeat...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/220943002/diff/50001/Source/core/css/RuleFeat...
Source/core/css/RuleFeature.cpp:179: return &ensureHoverInvalidationSet();
On 2014/04/04 22:48:27, esprehn wrote:
> Lets use a HashMap<CSSSelector::PseudoType, DescendantInvalidationSet> instead
> and get rid of all the duplication. We probably want to expand this to many
more
> things than just :hover, :active and :focus.

Done.

[rune, 2014-04-10 12:42:12]

https://codereview.chromium.org/220943002/diff/50001/Source/core/css/Affected...
File Source/core/css/AffectedByFocusTest.cpp (right):

https://codereview.chromium.org/220943002/diff/50001/Source/core/css/Affected...
Source/core/css/AffectedByFocusTest.cpp:296: "<div></div>"
On 2014/04/04 22:48:27, esprehn wrote:
> Can we just write the normal layout tests like we've been doing for this
> feature? I'd rather not have the tests in so many places.

Style update for focus still happens synchronously for some reason. I haven't
looked into that. Do you want me to remove this?

https://codereview.chromium.org/220943002/diff/50001/Source/core/css/HoverUpd...
File Source/core/css/HoverUpdateTest.cpp (right):

https://codereview.chromium.org/220943002/diff/50001/Source/core/css/HoverUpd...
Source/core/css/HoverUpdateTest.cpp:61: "<span></span>"
On 2014/04/08 16:28:25, rune - CET wrote:
> On 2014/04/04 22:48:27, esprehn wrote:
> > Why not a regular layout test like we did for all the other ones?
> 
> Because hover/active/focus changes are synchronously recalculated and cannot
be
> caught by internals.updateStyleAndReturnAffectedElementCount(). I'm looking at
> how to fix the synchronous update.

I've landed removal of the synchronous update for hover/active now and added a
layout test for hover. Do you want me to remove this?

[rune, 2014-04-11 20:05:23]

@esprehn PTAL

[esprehn, 2014-04-11 20:23:00]

lgtm, but why do we need the combinator check at all? Can't we just do it for
:hover on right most side as well?

https://codereview.chromium.org/220943002/diff/110001/Source/core/css/HoverUp...
File Source/core/css/HoverUpdateTest.cpp (right):

https://codereview.chromium.org/220943002/diff/110001/Source/core/css/HoverUp...
Source/core/css/HoverUpdateTest.cpp:72: unsigned accessCount =
document->styleEngine()->resolverAccessCount() - startCount;
Can't this be a regular layout test now that we don't do the update
synchronously?

https://codereview.chromium.org/220943002/diff/110001/Source/core/css/RuleFea...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/220943002/diff/110001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:391:
ensurePseudoInvalidationSet(static_cast<CSSSelector::PseudoType>(it->key)).combine(*it->value);
Why do you need the cast?

[rune, 2014-04-11 22:34:36]

https://codereview.chromium.org/220943002/diff/110001/Source/core/css/HoverUp...
File Source/core/css/HoverUpdateTest.cpp (right):

https://codereview.chromium.org/220943002/diff/110001/Source/core/css/HoverUp...
Source/core/css/HoverUpdateTest.cpp:72: unsigned accessCount =
document->styleEngine()->resolverAccessCount() - startCount;
On 2014/04/11 20:23:01, esprehn wrote:
> Can't this be a regular layout test now that we don't do the update
> synchronously?

Will fix.

https://codereview.chromium.org/220943002/diff/110001/Source/core/css/RuleFea...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/220943002/diff/110001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.cpp:391:
ensurePseudoInvalidationSet(static_cast<CSSSelector::PseudoType>(it->key)).combine(*it->value);
On 2014/04/11 20:23:01, esprehn wrote:
> Why do you need the cast?

It's because of the use of IntHash:

  typedef HashMap<CSSSelector::PseudoType, RefPtr<DescendantInvalidationSet>,
WTF::IntHash<unsigned>, WTF::UnsignedWithZeroKeyHashTraits<unsigned> >
PseudoTypeInvalidationSetMap;

If I change to:

    typedef HashMap<CSSSelector::PseudoType, RefPtr<DescendantInvalidationSet> >
PseudoTypeInvalidationSetMap;

and add:


  namespace WTF {

  template<> struct DefaultHash<WebCore::CSSSelector::PseudoType> {
      typedef IntHash<unsigned> Hash;
  };

  template<> struct HashTraits<WebCore::CSSSelector::PseudoType> :
GenericHashTraits<WebCore::CSSSelector::PseudoType> {
      static const bool emptyValueIsZero = false;
      static const bool needsDestruction = false;
      static WebCore::CSSSelector::PseudoType emptyValue() { return
static_cast<WebCore::CSSSelector::PseudoType>(std::numeric_limits<unsigned>::max());
}
      static void constructDeletedValue(WebCore::CSSSelector::PseudoType& slot)
{ slot =
static_cast<WebCore::CSSSelector::PseudoType>(std::numeric_limits<unsigned>::max()
- 1); }
      static bool isDeletedValue(WebCore::CSSSelector::PseudoType value) {
return value == std::numeric_limits<unsigned>::max() - 1; }
  };

  } // namespace WTF


it works without the cast.

Please enlighten me if there is a simpler way.

[rune, 2014-04-11 22:43:03]

On 2014/04/11 20:23:00, esprehn wrote:
> lgtm, but why do we need the combinator check at all? Can't we just do it for
> :hover on right most side as well?

We don't look at the invalidation set when seeing a affectedByHover. There is no
need to create an invalidation set at all if there are only selectors with
:hover in the rightmost selector. On the other hand, html.css contains custom
pseudo rules including all of :hover/:focus/:active which will ensure that an
invalidation set would currently be created for all of them (because of the
implicit descendant type of combinator inferred by custom pseudo elements).
Should I get rid of the combinator check for simplicity?

[rune, 2014-04-25 11:56:27]

On 2014/04/11 22:43:03, rune wrote:
> On 2014/04/11 20:23:00, esprehn wrote:
> > lgtm, but why do we need the combinator check at all? Can't we just do it
for
> > :hover on right most side as well?
> 
> We don't look at the invalidation set when seeing a affectedByHover. There is
no
> need to create an invalidation set at all if there are only selectors with
> :hover in the rightmost selector. On the other hand, html.css contains custom
> pseudo rules including all of :hover/:focus/:active which will ensure that an
> invalidation set would currently be created for all of them (because of the
> implicit descendant type of combinator inferred by custom pseudo elements).
> Should I get rid of the combinator check for simplicity?

I've removed it now since it didn't have an effect in practice.

[rune, 2014-04-25 11:56:49]

https://codereview.chromium.org/220943002/diff/110001/Source/core/css/HoverUp...
File Source/core/css/HoverUpdateTest.cpp (right):

https://codereview.chromium.org/220943002/diff/110001/Source/core/css/HoverUp...
Source/core/css/HoverUpdateTest.cpp:72: unsigned accessCount =
document->styleEngine()->resolverAccessCount() - startCount;
On 2014/04/11 22:34:36, rune wrote:
> On 2014/04/11 20:23:01, esprehn wrote:
> > Can't this be a regular layout test now that we don't do the update
> > synchronously?
> 
> Will fix.

Done.

[rune, 2014-04-25 11:58:23]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2014-04-25 11:58:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rune@opera.com/220943002/170001

[rune, 2014-04-25 12:03:03]

@esprehn: I'm landing this based on your previous l*gtm. I've addressed the
issues apart from the enum cast for the HashMap iterator key. I explained why
further up, but you didn't respond. Please tell if you wanted the HashMap done
differently and I can fix that in a followup CL.

[commit-bot: I haz the power, 2014-04-25 12:37:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-25 12:37:54]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[rune, 2014-04-25 12:47:47]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2014-04-25 12:48:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rune@opera.com/220943002/170001

[commit-bot: I haz the power, 2014-04-25 13:39:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-25 13:39:49]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[rune, 2014-04-25 13:51:01]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2014-04-25 13:51:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rune@opera.com/220943002/170001

[commit-bot: I haz the power, 2014-04-25 14:38:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-25 14:38:46]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[rune, 2014-04-25 15:08:49]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2014-04-25 15:09:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rune@opera.com/220943002/170001

[commit-bot: I haz the power, 2014-04-25 15:43:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-25 15:43:12]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[rune, 2014-04-26 19:20:24]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2014-04-26 19:20:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rune@opera.com/220943002/170001

[commit-bot: I haz the power, 2014-04-26 20:06:48]

Change committed as 172703