[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
bugzilla-daemon at webkit.org
Tue Mar 2 15:37:50 PST 2021
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`.
More information about the webkit-reviews
mailing list