Setup parent stylesheet for tree boundary crossing rules.

Setup parent stylesheet for tree boundary crossing rules.

The purpose of the patch is to store information about parent stylesheets for
rules that are crossing tree boundary. Currently, this information is missing,
thus DevTools do not allow to edit them.

To achieve this, the patch modifies TreeBoundaryCrossingRules data structure from
storing <ScopingNode, RuleSet> to storing <ScopingNode, CSSStyleSheet, RuleSet>. 
Iteration over this new data structure is somewhat tricky, so logic related to
collecting matched rules is moved in TreeBoundaryCrossingRules class as well.

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

[lushnikov, 2014-03-22 15:07:48]

This is a draft implementation of the idea we discussed on Friday evening; I'm
still not sure if it doesn't break css scoring.

[apavlov, 2014-03-24 14:05:12]

https://codereview.chromium.org/206043009/diff/1/Source/core/css/resolver/Mat...
File Source/core/css/resolver/MatchRequest.h (right):

https://codereview.chromium.org/206043009/diff/1/Source/core/css/resolver/Mat...
Source/core/css/resolver/MatchRequest.h:36: MatchRequest(RuleSet* ruleSet, bool
includeEmptyRules = false, const ContainerNode* scope = 0,  const CSSStyleSheet*
cssSheet = 0, bool elementApplyAuthorStyles = true, unsigned styleSheetIndex =
0)
The argument order usually agrees with the field order. Please fix that + the
ctor initializer order

[lushnikov, 2014-03-25 13:31:32]

Addressed comment; preserved order of rules in collecting.

https://codereview.chromium.org/206043009/diff/1/Source/core/css/resolver/Mat...
File Source/core/css/resolver/MatchRequest.h (right):

https://codereview.chromium.org/206043009/diff/1/Source/core/css/resolver/Mat...
Source/core/css/resolver/MatchRequest.h:36: MatchRequest(RuleSet* ruleSet, bool
includeEmptyRules = false, const ContainerNode* scope = 0,  const CSSStyleSheet*
cssSheet = 0, bool elementApplyAuthorStyles = true, unsigned styleSheetIndex =
0)
On 2014/03/24 14:05:12, apavlov wrote:
> The argument order usually agrees with the field order. Please fix that + the
> ctor initializer order

Done.

[vsevik, 2014-03-26 08:33:44]

https://codereview.chromium.org/206043009/diff/50001/Source/core/css/TreeBoun...
File Source/core/css/TreeBoundaryCrossingRules.h (right):

https://codereview.chromium.org/206043009/diff/50001/Source/core/css/TreeBoun...
Source/core/css/TreeBoundaryCrossingRules.h:47: typedef HashMap<const
CSSStyleSheet*, OwnPtrWillBeMember<RuleSet> > CSSStyleSheetRuleSubSet;
Can we use Vector of pairs instead? This should be faster/simpler.

https://codereview.chromium.org/206043009/diff/50001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResolver.cpp (left):

https://codereview.chromium.org/206043009/diff/50001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:403: inline void
StyleResolver::collectTreeBoundaryCrossingRules(Element* element,
ElementRuleCollector& collector, bool includeEmptyRules)
I am not sure if it is OK that this method is not inline anymore.

https://codereview.chromium.org/206043009/diff/50001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/206043009/diff/50001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:249: void
StyleResolver::addTreeBoundaryCrossingRules(const
WillBeHeapVector<MinimalRuleData>& rules, ContainerNode* scope, CSSStyleSheet*
parentStyleSheet)
Looks like this could be moved to TreeBoundaryCrossingRules (and inlined there).

[lushnikov, 2014-03-26 14:04:35]

addressed comments

https://codereview.chromium.org/206043009/diff/50001/Source/core/css/TreeBoun...
File Source/core/css/TreeBoundaryCrossingRules.h (right):

https://codereview.chromium.org/206043009/diff/50001/Source/core/css/TreeBoun...
Source/core/css/TreeBoundaryCrossingRules.h:47: typedef HashMap<const
CSSStyleSheet*, OwnPtrWillBeMember<RuleSet> > CSSStyleSheetRuleSubSet;
On 2014/03/26 08:33:44, vsevik wrote:
> Can we use Vector of pairs instead? This should be faster/simpler.

Done.

https://codereview.chromium.org/206043009/diff/50001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/206043009/diff/50001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:249: void
StyleResolver::addTreeBoundaryCrossingRules(const
WillBeHeapVector<MinimalRuleData>& rules, ContainerNode* scope, CSSStyleSheet*
parentStyleSheet)
On 2014/03/26 08:33:44, vsevik wrote:
> Looks like this could be moved to TreeBoundaryCrossingRules (and inlined
there).

Done.

[lushnikov, 2014-03-26 19:04:50]

The patch adds memory overhead proportional to the size of the datastructure. I
believe this kind of rules is going to be a pretty rare case, so this seems to
be bearable. @ojan, @esprehn - what would you say?

[esprehn, 2014-03-26 19:16:03]

On 2014/03/26 19:04:50, lushnikov wrote:
> The patch adds memory overhead proportional to the size of the datastructure.
I
> believe this kind of rules is going to be a pretty rare case, so this seems to
> be bearable. @ojan, @esprehn - what would you say?

These rules are not very rare in polymer applications.

https://codereview.chromium.org/206043009/diff/120001/Source/core/css/TreeBou...
File Source/core/css/TreeBoundaryCrossingRules.h (right):

https://codereview.chromium.org/206043009/diff/120001/Source/core/css/TreeBou...
Source/core/css/TreeBoundaryCrossingRules.h:52: typedef
Vector<CSSStyleSheetRules> CSSStyleSheetRuleSubSet;
This doesn't make sense to me, tree boundary crossing rules are already owned by
a CSSStyleSheet from somewhere.

[esprehn, 2014-03-26 19:59:25]

lgtm, I think this is probably fine. We should fix addTreeBoundaryCrossingRules
to not do so many duplicate hash lookups by using add() properly, but we can do
that in a follow up patch.

[vsevik, 2014-03-27 09:01:21]

DevTools-wise lgtm.

[lushnikov, 2014-03-27 12:32:38]

The CQ bit was checked by lushnikov@chromium.org

[commit-bot: I haz the power, 2014-03-27 12:32:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lushnikov@chromium.org/206043009/160001

[commit-bot: I haz the power, 2014-03-27 13:05:45]

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

[commit-bot: I haz the power, 2014-03-27 13:05:46]

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

[esprehn, 2014-03-27 13:20:07]

Your failures look real:

fast/dom/shadow/content-pseudo-element-overridden.html

[lushnikov, 2014-03-28 17:48:04]

Could you guys please take another look on the patch due to non-trivial changes?

On 2014/03/27 13:20:07, esprehn wrote:
> Your failures look real:
> 
> fast/dom/shadow/content-pseudo-element-overridden.html

Indeed; it was a reasonable failure. The problem was that I added scoped nodes
into the treeboundarycrossingrules even though the rulesets didn't contain any
tree-boundary rules. I fixed this; trybots seem to be happy so far.

Since the last version of the patch I've rewritten addTreeBoundingRules method
and fixed oilpan transitioning types.

[lushnikov, 2014-04-01 12:08:57]

On 2014/03/28 17:48:04, lushnikov wrote:
> Could you guys please take another look on the patch due to non-trivial
changes?
> 
> On 2014/03/27 13:20:07, esprehn wrote:
> > Your failures look real:
> > 
> > fast/dom/shadow/content-pseudo-element-overridden.html
> 
> Indeed; it was a reasonable failure. The problem was that I added scoped nodes
> into the treeboundarycrossingrules even though the rulesets didn't contain any
> tree-boundary rules. I fixed this; trybots seem to be happy so far.
> 
> Since the last version of the patch I've rewritten addTreeBoundingRules method
> and fixed oilpan transitioning types.

ping?

[esprehn, 2014-04-03 09:57:16]

https://codereview.chromium.org/206043009/diff/220001/Source/core/css/TreeBou...
File Source/core/css/TreeBoundaryCrossingRules.cpp (right):

https://codereview.chromium.org/206043009/diff/220001/Source/core/css/TreeBou...
Source/core/css/TreeBoundaryCrossingRules.cpp:54: return;
How did the old code deal with this being empty? It doesn't seem to have this
check.

Ideally we'd check 

if (authorRules.treeBoundaryCrossingRules().isEmpty() &&
authorRules.shadowDistributedRules().isEmpty())
  return;

at the top of the function to avoid the malloc inside RuleSet::create() where
we're just going to discard it anyway. I suppose that means we still need the
check here otherwise that check needs to be more complex to deal with the
scopingNode && !scopingNode->isDocumentNode() check. Perhaps you want both
checks though. In the common case you're making us create the RuleSet above and
discard it.

https://codereview.chromium.org/206043009/diff/220001/Source/core/css/TreeBou...
Source/core/css/TreeBoundaryCrossingRules.cpp:56: if
(!m_treeBoundaryCrossingRuleSetMap.contains(scopingNode)) {
Can scopingNode actually be null? You check it above, but I don't think it's
safe to do contains(null) or add(null) on a HashMap.

https://codereview.chromium.org/206043009/diff/220001/Source/core/css/TreeBou...
Source/core/css/TreeBoundaryCrossingRules.cpp:111:
features.add(ruleSet->features());
I'd just do it->second->features() and make this one line and remove the braces.

https://codereview.chromium.org/206043009/diff/220001/Source/core/css/TreeBou...
Source/core/css/TreeBoundaryCrossingRules.cpp:119:
collectFeaturesFromRuleSubSet(ruleSubSet, features);
Same, this can be one line and remove the braces

[lushnikov, 2014-04-03 13:53:12]

https://codereview.chromium.org/206043009/diff/220001/Source/core/css/TreeBou...
File Source/core/css/TreeBoundaryCrossingRules.cpp (right):

https://codereview.chromium.org/206043009/diff/220001/Source/core/css/TreeBou...
Source/core/css/TreeBoundaryCrossingRules.cpp:54: return;
On 2014/04/03 09:57:17, esprehn wrote:
> How did the old code deal with this being empty? It doesn't seem to have this
> check.

The old code here was "addRule" method which was called from the body of
for-loop.
So it essentially added a scopingNode iff it had a crossing rule

> 
> Ideally we'd check 
> 
> if (authorRules.treeBoundaryCrossingRules().isEmpty() &&
> authorRules.shadowDistributedRules().isEmpty())
>   return;
> 
> at the top of the function to avoid the malloc inside RuleSet::create() where
> we're just going to discard it anyway. I suppose that means we still need the
> check here otherwise that check needs to be more complex to deal with the
> scopingNode && !scopingNode->isDocumentNode() check. Perhaps you want both
> checks though. In the common case you're making us create the RuleSet above
and
> discard it.

Added a check to avoid mallocing RuleSet if its not needed. This condition got
removed as well.

https://codereview.chromium.org/206043009/diff/220001/Source/core/css/TreeBou...
Source/core/css/TreeBoundaryCrossingRules.cpp:56: if
(!m_treeBoundaryCrossingRuleSetMap.contains(scopingNode)) {
On 2014/04/03 09:57:17, esprehn wrote:
> Can scopingNode actually be null? You check it above, but I don't think it's
> safe to do contains(null) or add(null) on a HashMap.

My bad; it cannot be null. Removed checking for null and fixed method signature
to receive
ref instead of pointer. Thanks.

https://codereview.chromium.org/206043009/diff/220001/Source/core/css/TreeBou...
Source/core/css/TreeBoundaryCrossingRules.cpp:111:
features.add(ruleSet->features());
On 2014/04/03 09:57:17, esprehn wrote:
> I'd just do it->second->features() and make this one line and remove the
braces.

Done.

https://codereview.chromium.org/206043009/diff/220001/Source/core/css/TreeBou...
Source/core/css/TreeBoundaryCrossingRules.cpp:119:
collectFeaturesFromRuleSubSet(ruleSubSet, features);
On 2014/04/03 09:57:17, esprehn wrote:
> Same, this can be one line and remove the braces

Done.

[esprehn, 2014-04-04 22:37:19]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2014-04-04 22:37:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lushnikov@chromium.org/206043009/240001

[commit-bot: I haz the power, 2014-04-04 22:49:24]

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

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

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

[lushnikov, 2014-04-05 07:34:32]

The CQ bit was checked by lushnikov@chromium.org

[commit-bot: I haz the power, 2014-04-05 07:34:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lushnikov@chromium.org/206043009/240001

[commit-bot: I haz the power, 2014-04-05 21:39:43]

Change committed as 170929