[webkit-reviews] review granted: [Bug 229666] [CSS Cascade Layers] Computer order correctly for late added sublayers : [Attachment 436764] patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Antti Koivisto
<koivisto at iki.fi>'s request for review:
Bug 229666: [CSS Cascade Layers] Computer order correctly for late added
sublayers
https://bugs.webkit.org/show_bug.cgi?id=229666

Attachment 436764: patch

https://bugs.webkit.org/attachment.cgi?id=436764&action=review




--- Comment #3 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 436764
  --> https://bugs.webkit.org/attachment.cgi?id=436764
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=436764&action=review

> Source/WebCore/ChangeLog:3
> +	   [CSS Cascade Layers] Computer order correctly for late added
sublayers

Compute order

> Source/WebCore/style/RuleSet.cpp:86
> +void RuleSet::addRule(const StyleRule& rule, unsigned selectorIndex,
unsigned selectorListIndex, unsigned cascadeLayerIdentifier,
MediaQueryCollector* mediaQueryCollector)

unsigned -> CascadeLayerIdentifier ?

> Source/WebCore/style/RuleSet.cpp:93
> +	   std::fill(m_cascadeLayerIdentifierForRulePosition.begin() + oldSize,
m_cascadeLayerIdentifierForRulePosition.end(), 0);

Shame they don't init to 0 automatically

> Source/WebCore/style/RuleSet.cpp:405
> +	   CascadeLayerIdentifier identifier = 0;

Oh there is a typedef! Perhaps it should be a type which can initialize itself
to 0.

> Source/WebCore/style/RuleSet.cpp:470
> +    auto compareCascadeLayers = [&](CascadeLayerIdentifier a,
CascadeLayerIdentifier b) {
> +	   while (a && b) {
> +	       // Identifiers are in parse order which almost corresponds to
the layer priority order.
> +	       // The only exception is when a sublayer gets added to a layer
after adding other non-sublayers.
> +	       // To resolve this we need look for a shared ancestor layer.
> +	       auto aParent =
ruleSet->cascadeLayerForIdentifier(a).parentIdentifier;
> +	       auto bParent =
ruleSet->cascadeLayerForIdentifier(b).parentIdentifier;
> +	       if (aParent == bParent || aParent == b || bParent == a)
> +		   break;
> +	       if (aParent > bParent)
> +		   a = aParent;
> +	       else
> +		   b = bParent;
> +	   }
> +	   return a < b;
> +    };
> +
> +    auto updateCascadeLayerOrder = [&] {
> +	   Vector<CascadeLayerIdentifier> orderVector;
> +	   for (CascadeLayerIdentifier identifier = 1; identifier <=
ruleSet->m_cascadeLayers.size(); ++identifier)
> +	       orderVector.append(identifier);
> +	   std::sort(orderVector.begin(), orderVector.end(),
compareCascadeLayers);
> +	   for (unsigned i = 0; i < orderVector.size(); ++i)
> +	       ruleSet->cascadeLayerForIdentifier(orderVector[i]).order = i +
1;
> +    };
> +
> +    if (mode == Mode::Normal && !ruleSet->m_cascadeLayers.isEmpty())
> +	   updateCascadeLayerOrder();

Maybe put this in its own function

> Source/WebCore/style/RuleSet.h:193
> +    CascadeLayer& cascadeLayerForIdentifier(CascadeLayerIdentifier
identifier) { return m_cascadeLayers[identifier - 1]; }
> +    const CascadeLayer& cascadeLayerForIdentifier(CascadeLayerIdentifier
identifier) const { return m_cascadeLayers[identifier - 1]; }

Maybe assert that identifier != 0