[webkit-reviews] review canceled: [Bug 222384] REGRESSION (r266288): Web Inspector: ::marker shows on every element now : [Attachment 421999] Patch v2.0 - Backend Fix, No Test Yet

[bugzilla-daemon at webkit.org]

Patrick Angle <pangle at apple.com> has canceled Patrick Angle
<pangle at apple.com>'s request for review:
Bug 222384: REGRESSION (r266288): Web Inspector: ::marker shows on every
element now
https://bugs.webkit.org/show_bug.cgi?id=222384

Attachment 421999: Patch v2.0 - Backend Fix, No Test Yet

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




--- Comment #4 from Patrick Angle <pangle at apple.com> ---
Comment on attachment 421999
  --> https://bugs.webkit.org/attachment.cgi?id=421999
Patch v2.0 - Backend Fix, No Test Yet

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

>> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:1106
>> +	if (!inspectorStyleSheet || (inspectorStyleSheet->origin() ==
Protocol::CSS::StyleSheetOrigin::UserAgent && rule->selectorText() ==
"::marker"_s && element.computedStyle()->display() != DisplayType::ListItem))
> 
> NIT: I'd recommend splitting this out into two different `if` so that it's
easier to read (less parenthesis/grouping)
> 
> NIT: Is there really a benefit to showing even non-UserAgent `::marker` if
not `display: list-item`?

To the second NIT: I was ready to say yes, but after typing my reason for
saying yes I realize that it wasn't a valid reason, so here is my reason for
saying no now: If the author defines a blanket `::marker` selector, they would
expect it to apply only where markers could apply. If they define a silly
selector like `table::marker`, it would still be shown after removing the
origin check anyways because its selector no exactly matches `::marker`.