Bug 161071 - Web Inspector: Combine similar SVG files for Styles sidebar
Summary: Web Inspector: Combine similar SVG files for Styles sidebar
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-08-22 21:06 PDT by Devin Rousso
Modified: 2016-09-11 20:31 PDT (History)
8 users (show)

See Also:


Attachments
Patch (69.93 KB, patch)
2016-08-23 14:58 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (70.15 KB, patch)
2016-09-11 17:44 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (145.59 KB, patch)
2016-09-11 18:45 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2016-08-22 21:06:30 PDT
Based on https://bugs.webkit.org/show_bug.cgi?id=160893#c8
Comment 1 Radar WebKit Bug Importer 2016-08-22 21:06:52 PDT
<rdar://problem/27961661>
Comment 2 Devin Rousso 2016-08-23 14:58:35 PDT
Created attachment 286783 [details]
Patch

So I tried doing this, but it seems like trying to add extra styling based on a <use> doesn't want to work.  All the icons just appear as black rounded rectangles...
Comment 3 Joseph Pecoraro 2016-08-23 15:09:15 PDT
Darn. At a quick glance this looks correct to me.
Comment 4 Devin Rousso 2016-09-11 17:44:29 PDT
Created attachment 288540 [details]
Patch
Comment 5 Joseph Pecoraro 2016-09-11 17:56:36 PDT
Comment on attachment 288540 [details]
Patch

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

r- to handle the GTK images, but otherwise the patch looks good!

> Source/WebInspectorUI/UserInterface/Images/StyleRule.svg:41
> +            --background: rgb(224, 224, 224);
> +            --border: rgb(178, 178, 178);
> +            --outline: rgb(191, 191, 191);

Using variables here is not just clever it was apparently necessary (can't just do #id .class { fill: ... }).
You should have a sentence explaining this in the ChangeLog, because it is non-trivial to someone reading the file.

> Source/WebInspectorUI/UserInterface/Views/StyleRuleIcons.css:61
>  .inherited-style-rule-icon .icon {
> -    content: url(../Images/StyleRuleInherited.svg);
> +    content: url(../Images/StyleRule.svg#inherited);
>  }

This is awesome! But it does means you have to handle the GTK images as well, to either update them to work with this or an override:

    :not(.mac-platform, .windows-platform) .inherited-style-rule-icon .icon { ... }

I'd prefer if we can update their images as well, it would keep this CSS slim.
Comment 6 Devin Rousso 2016-09-11 18:45:59 PDT
Created attachment 288545 [details]
Patch
Comment 7 Joseph Pecoraro 2016-09-11 19:12:19 PDT
Comment on attachment 288545 [details]
Patch

r=me
Comment 8 WebKit Commit Bot 2016-09-11 20:31:09 PDT
Comment on attachment 288545 [details]
Patch

Clearing flags on attachment: 288545

Committed r205793: <http://trac.webkit.org/changeset/205793>
Comment 9 WebKit Commit Bot 2016-09-11 20:31:13 PDT
All reviewed patches have been landed.  Closing bug.