Bug 150602 – Web Inspector: Remove image generators and use SVG directly

• Component: Web Inspector
• Status: RESOLVED FIXED

[Timothy Hatcher]

We can remove the rest of the generators, image cache database and switch to SVG directly.

[Radar WebKit Bug Importer]

<rdar://problem/23282583>

[Timothy Hatcher]

Created attachment 264158 [details]
Patch

[Blaze Burg]

Comment on attachment 264158 [details]
Patch

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

Looks pretty good overall. I would like some explanations of the changes as noted.

> Source/WebInspectorUI/UserInterface/Base/ImageUtilities.js:31
> +    var svgElement = document.createElementNS(svgNamespace, "svg");

Nit (throughout): let instead of var.

> Source/WebInspectorUI/UserInterface/Base/ImageUtilities.js:52
> +    callback(wrapper);

We can defer this to later to avoid getting stuck, but is there any point to using a callback interface if it's always synchronously invoked?

> Source/WebInspectorUI/UserInterface/Images/Checkers.svg:4
> +    <rect fill="rgb(204, 204, 204)" width="3" height="3"/>

Perhaps we could this later, but it would be nice to consistently use hsl() for colors, like we do in CSS.

> Source/WebInspectorUI/UserInterface/Images/Close.svg:-4
> -    <path class="stroked" fill="none" stroke="black" stroke-linecap="square" d="M 4.5 4.5 L 11.5 11.5 M 4.5 11.5 L 11.5 4.5"/>

was .stroked used for anything?

> Source/WebInspectorUI/UserInterface/Images/Console.svg:4
> +    <rect fill="none" stroke="currentColor" x="0.5" y="0.5" width="15" height="15" rx="2"/>

If you could spend 5 minutes to document the preferred order of attributes on our style guide page (here, putting paths last), it would be helpful.

> Source/WebInspectorUI/UserInterface/Images/FontStyleItalic.svg:4
> +    <path fill="currentColor" d="M 4.55712891 8.35449219 L 4.18359375 5.50244141 L 2.53564453 8.35449219 L 4.55712891 8.35449219 Z M 3.93310547 4.54443359 L 4.921875 4.54443359 L 5.89306641 11 L 4.93505859 11 L 4.68896484 9.06640625 L 2.13574219 9.06640625 L 1.02832031 11 L 0.131835938 11 L 3.93310547 4.54443359 Z M 7.89697266 4.52246094 L 8.66601562 4.52246094 L 8.16943359 6.86474609 C 8.38916125 6.63915903 8.63159047 6.46704161 8.89672852 6.34838867 C 9.16186656 6.22973573 9.43505719 6.17041016 9.71630859 6.17041016 C 10.302249 6.17041016 10.735106 6.37182416 11.0148926 6.7746582 C 11.2946791 7.17749225 11.3510751 7.77148045 11.184082 8.55664062 C 11.0258781 9.30078497 10.7138695 9.91894285 10.2480469 10.4111328 C 9.78222423 10.9033228 9.22998366 11.1494141 8.59130859 11.1494141 C 8.23388493 11.1494141 7.95117292 11.0629891 7.74316406 10.8901367 C 7.61718687 10.7875971 7.49560605 10.6235363 7.37841797 10.3979492 L 7.25097656 11 L 6.52148437 11 L 7.89697266 4.52246094 Z M 10.3666992 8.59619141 C 10.4721685 8.10107174 10.4531257 7.69091959 10.3095703 7.36572266 C 10.1660149 7.04052572 9.88769738 6.87792969 9.47460938 6.87792969 C 9.11425601 6.87792969 8.77002117 7.01122914 8.44189453 7.27783203 C 8.11376789 7.54443493 7.88525455 7.98388366 7.75634766 8.59619141 C 7.66259719 9.03857643 7.64208958 9.39745956 7.69482422 9.67285156 C 7.79150439 10.1914088 8.1240206 10.4506836 8.69238281 10.4506836 C 9.12011933 10.4506836 9.4760728 10.2807634 9.76025391 9.94091797 C 10.044435 9.60107252 10.2465814 9.15283481 10.3666992 8.59619141 L 10.3666992 8.59619141 Z M 15.9609375 7.87548828 L 15.1918945 7.87548828 C 15.206543 7.58544777 15.1508795 7.34448338 15.0249023 7.15258789 C 14.8989252 6.9606924 14.651369 6.86474609 14.2822266 6.86474609 C 13.7783178 6.86474609 13.365236 7.11083738 13.0429688 7.60302734 C 12.8349599 7.92236488 12.6811528 8.31640391 12.581543 8.78515625 C 12.4790034 9.2568383 12.4936517 9.65380698 12.6254883 9.97607422 C 12.7573249 10.2983415 13.0371072 10.4594727 13.4648438 10.4594727 C 13.7929704 10.4594727 14.0742176 10.3591319 14.3085938 10.1584473 C 14.5429699 9.95776267 14.7333977 9.68310721 14.8798828 9.33447266 L 15.6489258 9.33447266 C 15.4291981 9.95849921 15.112795 10.4147935 14.699707 10.7033691 C 14.286619 10.9919448 13.8061551 11.1362305 13.2583008 11.1362305 C 12.6430633 11.1362305 12.1999525 10.9113792 11.9289551 10.4616699 C 11.6579576 10.0119606 11.5942376 9.45019868 11.737793 8.77636719 C 11.9135751 7.95019118 12.2512182 7.30713121 12.7507324 6.84716797 C 13.2502466 6.38720473 13.8105438 6.15722656 14.4316406 6.15722656 C 14.9619167 6.15722656 15.3654771 6.28613152 15.642334 6.54394531 C 15.9191908 6.8017591 16.0253909 7.24560232 15.9609375 7.87548828 L 15.9609375 7.87548828 Z"/>

Why is this a path now instead of text?

> Source/WebInspectorUI/UserInterface/Images/ReloadToolbar.svg:-3
> -<svg viewBox="0 0 16 16" version="1.1" xmlns="http://www.w3.org/2000/svg">

Why is the clip-path not necessary any more?

> Source/WebInspectorUI/UserInterface/Images/ReplayPauseButton.svg:-3
> -<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">

Oops, sorry about that :P

> Source/WebInspectorUI/UserInterface/Images/gtk/BackForwardArrows.svg:-13
> -        path {

I had no idea SVG could have style rules like this inside the actual SVG file. o_O

> Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:-409
> -        var parentSelector = "." + (this.constructor.StyleClassName || "navigation-bar");

What did this code do? It would be good to briefly explain the canvas identifier approach and the ramifications of moving away from it. Come to think of it... the ChangeLog needs a lot more explanation of "why?" for a patch this big. It's pretty much empty right now.

> Source/WebInspectorUI/UserInterface/Views/ScopeBar.css:75
> +    color: white;

Please also explain how the currentColor thing works. It is still kind of mysterious to me. is currentColor an alias for the CSS 'color' property value?

> Source/WebInspectorUI/UserInterface/Views/ToggleButtonNavigationItem.js:-33
> -        // FIXME: We could try overriding _canvasIdentifier() to return different identifiers. If we did that

Whew, that was bugging me.

> Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditorLink.js:53
> +        useSVGSymbol("Images/VisualStylePropertyUnlinked.svg", "unlinked-icon", null, function(wrapper) {

Might be a good time to switch useSVGSymbol over to the options={} approach, since it has a lot of args.

[Timothy Hatcher]

Created attachment 264231 [details]
Diff from last patch

[Blaze Burg]

Comment on attachment 264231 [details]
Diff from last patch

Very nice.

[Blaze Burg]

Comment on attachment 264158 [details]
Patch

r=me (with the interdiff applied)

[Timothy Hatcher]

http://trac.webkit.org/changeset/191693