[webkit-reviews] review granted: [Bug 237532] [css-cascade] revert-layer should roll back to preshints : [Attachment 454702] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Oriol Brufau <obrufau at igalia.com>'s
request for review:
Bug 237532: [css-cascade] revert-layer should roll back to preshints
https://bugs.webkit.org/show_bug.cgi?id=237532

Attachment 454702: Patch

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




--- Comment #6 from Darin Adler <darin at apple.com> ---
Comment on attachment 454702
  --> https://bugs.webkit.org/attachment.cgi?id=454702
Patch

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

> Source/WebCore/style/ElementRuleCollector.cpp:129
> +inline void ElementRuleCollector::addElementStyleProperties(const
StyleProperties* propertySet, CascadeLayerPriority cascadeLayerPriority, bool
isCacheable, FromStyleAttribute fromStyleAttribute)

Since the type is so specific and strong typing prevents us from making obvious
mistakes, I think we could take the risk of just calling this argument
priority. Single-word names often make things easier to read.

> Source/WebCore/style/RuleSetBuilder.cpp:294
> +	   // Priority 0 is reserved for presentational hints.
> +	   auto priority = std::min<unsigned>(i + 1,
RuleSet::cascadeLayerPriorityForUnlayered - 1);

I find this comment confusing; it says some, but not all, of why the code below
does what it does and is correct. I think the issue is that priorities matter
only relative to each other, so we use "+1" to avoid clobbering the priority of
0. But that isn’t said, and the code below has "i + 1" and since I can see the
diff I know the comment is about the "+ 1" in that but also has
"RuleSet::cascadeLayerPriorityForUnlayered - 1" and it’s quite unclear why that
value is correct.