[webkit-reviews] review granted: [Bug 195123] Web Inspector: Styles: `::-webkit-scrollbar*` rules aren't shown : [Attachment 364313] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 195123: Web Inspector: Styles: `::-webkit-scrollbar*` rules aren't shown
https://bugs.webkit.org/show_bug.cgi?id=195123

Attachment 364313: Patch

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




--- Comment #13 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 364313
  --> https://bugs.webkit.org/attachment.cgi?id=364313
Patch

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

r=me

>>>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:
295
>>> +			 section.__pseudoId = pseudoId;
>> 
>> `__publicProperty` looks like a dirty hack. Can we just modify
WI.SpreadsheetCSSStyleDeclarationSection? Either by adding a parameter to the
constructor or introducing a getter&setter.
> 
> We do this pretty often, for "bookkeeping" that the underlying object/class
doesn't really need to know about.  The only reason why this is needed is so
that we can relate a header to a style when filtering.	Unless there's an
active filter, the property won't do anything or even be used for anything. 
`WI.SpreadsheetCSSStyleDeclarationSection` doesn't need to know about what
`pseudoId` it correlates to at all, just like how it doesn't need to know what
header it's under.  Alternatively, we could maintain another `Map` that holds
`section` => `header`, but that requires a lot more work to keep in sync, and
is really overkill for something this small.

Yeah this is a situation where we would normally do the __property trick.

> LayoutTests/inspector/css/getMatchedStylesForNode.html:45
> +	   function replacer(key, value) {
> +	       if (key === "ruleId" || key === "styleId" || key === "range" ||
key === "sourceLine" || key === "sourceURL")
> +		   return "<filtered>";
> +	       return value;
> +	   }

You could completely skip keys you don't care about. For example some these you
could probably just hide to simplify the output, including:

    if (key === "matchingSelectors" || key === "shorthandEntries")
	return undefined;

> LayoutTests/inspector/css/getMatchedStylesForNode.html:50
> +	   ProtocolTest.log(JSON.stringify(matchedCSSRules, replacer, 2));

We have `*Test.json(...)` which should allow you to simplify this:

    ProtocolTest.log(JSON.stringify(matchedCSSRules, replacer, 2));

to just:

    ProtocolTest.json(matchedCSSRules, replacer);