Add support for element ids in TargetedStyleRecalc

Add support for element ids in TargetedStyleRecalc.

BUG=318468
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170719

[rune, 2014-04-01 06:58:25]

lgtm

I don't know how common it is to change id on elements compared to
classes/attributes, but I guess it's worth it? (worth the extra storage needed).

https://codereview.chromium.org/220123004/diff/40002/Source/core/css/RuleFeat...
File Source/core/css/RuleFeature.h (right):

https://codereview.chromium.org/220123004/diff/40002/Source/core/css/RuleFeat...
Source/core/css/RuleFeature.h:85: return m_idInvalidationSets.get(idValue);

You assert on the emptiness of class and attribute above. Would it make sense to
do the same for id?

[chrishtr, 2014-04-01 16:25:16]

Regarding use cases: the primary motivation for this CL is for its use in
invalidation for adding style sheets, not mutating id.

https://codereview.chromium.org/220123004/diff/40002/Source/core/css/RuleFeat...
File Source/core/css/RuleFeature.h (right):

https://codereview.chromium.org/220123004/diff/40002/Source/core/css/RuleFeat...
Source/core/css/RuleFeature.h:85: return m_idInvalidationSets.get(idValue);
On 2014/04/01 06:58:26, rune - CET wrote:
> 
> You assert on the emptiness of class and attribute above. Would it make sense
to
> do the same for id?

I think it can be empty, if the element has no id. There are also checks in
scheduleStyleInvalidationForIdChange for empty.

[esprehn, 2014-04-01 23:11:19]

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

https://codereview.chromium.org/220123004/diff/110001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.h:85: return m_idInvalidationSets.get(idValue);
This can still be contains right? Same with the above .hasSelectorForClass

https://codereview.chromium.org/220123004/diff/110001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.h:97: return m_idInvalidationSets.size() > 0;
This function should really return a bool too

https://codereview.chromium.org/220123004/diff/110001/Source/core/css/invalid...
File Source/core/css/invalidation/StyleInvalidator.cpp (right):

https://codereview.chromium.org/220123004/diff/110001/Source/core/css/invalid...
Source/core/css/invalidation/StyleInvalidator.cpp:84: if (id == *it)
You don't need this loop,

if (m_invalidationIds.contains(element.idForStyleResolution()))
  return true;

[chrishtr, 2014-04-01 23:20:08]

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

https://codereview.chromium.org/220123004/diff/110001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.h:85: return m_idInvalidationSets.get(idValue);
On 2014/04/01 23:11:19, esprehn wrote:
> This can still be contains right? Same with the above .hasSelectorForClass

Done.

https://codereview.chromium.org/220123004/diff/110001/Source/core/css/RuleFea...
Source/core/css/RuleFeature.h:97: return m_idInvalidationSets.size() > 0;
On 2014/04/01 23:11:19, esprehn wrote:
> This function should really return a bool too

Done.

https://codereview.chromium.org/220123004/diff/110001/Source/core/css/invalid...
File Source/core/css/invalidation/StyleInvalidator.cpp (right):

https://codereview.chromium.org/220123004/diff/110001/Source/core/css/invalid...
Source/core/css/invalidation/StyleInvalidator.cpp:84: if (id == *it)
On 2014/04/01 23:11:19, esprehn wrote:
> You don't need this loop,
> 
> if (m_invalidationIds.contains(element.idForStyleResolution()))
>   return true;

Good point! Fixed.

[esprehn, 2014-04-02 23:06:45]

lgtm

[chrishtr, 2014-04-02 23:06:57]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-04-02 23:07:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/chrishtr@chromium.org/220123004/180001

[commit-bot: I haz the power, 2014-04-03 00:17:28]

Change committed as 170719