Bug 144329 - [GTK] Web Inspector: New Images added in r183339, r183338, r183332, r183323, r182660, and r182186
Summary: [GTK] Web Inspector: New Images added in r183339, r183338, r183332, r183323, ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andres Gomez Garcia
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-04-28 07:24 PDT by Manuel Rego Casasnovas
Modified: 2015-05-05 11:09 PDT (History)
12 users (show)

See Also:


Attachments
Patch (20.63 KB, patch)
2015-04-30 10:44 PDT, Andres Gomez Garcia
no flags Details | Formatted Diff | Diff
Patch for landing (19.97 KB, patch)
2015-05-05 07:26 PDT, Andres Gomez Garcia
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Manuel Rego Casasnovas 2015-04-28 07:24:41 PDT
Several new images has been added in http://trac.webkit.org/changeset/183323

Specially annoying the missing icon for "Show Details Sidebar" (Images/ToggleRightSidebar.svg) which is the place to change the style of an element.
Comment 1 Radar WebKit Bug Importer 2015-04-28 07:24:54 PDT
<rdar://problem/20726829>
Comment 2 Andres Gomez Garcia 2015-04-28 07:26:31 PDT
Taking ...
Comment 3 Timothy Hatcher 2015-04-28 09:42:08 PDT
More images were added in r183339, r183338, r183332, r183323, r182660, and r182186.
Comment 4 Andres Gomez Garcia 2015-04-30 10:44:23 PDT
Created attachment 252069 [details]
Patch
Comment 5 Joseph Pecoraro 2015-04-30 12:23:43 PDT
Comment on attachment 252069 [details]
Patch

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

rs=me, but consider the comments to simplify even more

> Source/WebInspectorUI/UserInterface/Images/gtk/Console.svg:1
> +<?xml version="1.0" encoding="UTF-8" standalone="no"?>

I had never heard of standalone. Reading up on it:
http://www.xmlplease.com/xml/xmlquotations/standalone#s1.

Says:
Only if the XML document has a DTD, the "standalone" pseudo-attribute could be of importance.
If the XML declaration has no "standalone" pseudo-attribute it defaults to standalone="no".
It is never necessary to use standalone="no" explicitly.

You should be able to remove this attribute from all of these SVGs.

> Source/WebInspectorUI/UserInterface/Images/gtk/Console.svg:5
> +  <path style="block-progression:tb;color:#bebebe" d="m2.1875 0.5c-1.2055 0-2.1875 1.0167-2.1875 2.2188v10.562c0 1.202 0.98197 2.219 2.1875 2.219h11.625c1.206 0 2.188-1.017 2.188-2.219v-10.562c0-1.2023-0.982-2.219-2.188-2.219h-11.625zm0 2h11.625c0.123 0 0.188 0.0809 0.188 0.2188v10.562c0 0.138-0.065 0.219-0.188 0.219h-11.625c-0.1223 0-0.187-0.081-0.187-0.219v-10.562c0-0.1381 0.0647-0.219 0.1875-0.219z"/>

I had never heard of block-progression. It seems to deal with text layout / writing modes. I don't think it is needed here.

You should be able to remove this attribute in all cases that don't apply to text.

> Source/WebInspectorUI/UserInterface/Images/gtk/NewTabPlus.svg:5
> +  <text style="word-spacing:0px;letter-spacing:0px" font-weight="bold" xml:space="preserve" transform="scale(.99917 1.0008)" line-height="125%" font-size="18.288px" y="15.371576" x="0.45220301" font-family="Cantarell" fill="#000000"><tspan y="15.371576" x="0.45220301" fill="#000000">+</tspan></text>

Now here is a case where you are using text! That said, I wonder if it might be more appropriate to use a couple pathes, instead of text. What if the font differs between two GTK machines?
Comment 6 Andres Gomez Garcia 2015-05-05 06:40:42 PDT
(In reply to comment #5)
> > Source/WebInspectorUI/UserInterface/Images/gtk/Console.svg:1
> > +<?xml version="1.0" encoding="UTF-8" standalone="no"?>
> 
> I had never heard of standalone. Reading up on it:
> http://www.xmlplease.com/xml/xmlquotations/standalone#s1.
> 
> Says:
> Only if the XML document has a DTD, the "standalone" pseudo-attribute could
> be of importance.
> If the XML declaration has no "standalone" pseudo-attribute it defaults to
> standalone="no".
> It is never necessary to use standalone="no" explicitly.
> 
> You should be able to remove this attribute from all of these SVGs.

Removing ...

> > Source/WebInspectorUI/UserInterface/Images/gtk/Console.svg:5
> > +  <path style="block-progression:tb;color:#bebebe" d="m2.1875 0.5c-1.2055 0-2.1875 1.0167-2.1875 2.2188v10.562c0 1.202 0.98197 2.219 2.1875 2.219h11.625c1.206 0 2.188-1.017 2.188-2.219v-10.562c0-1.2023-0.982-2.219-2.188-2.219h-11.625zm0 2h11.625c0.123 0 0.188 0.0809 0.188 0.2188v10.562c0 0.138-0.065 0.219-0.188 0.219h-11.625c-0.1223 0-0.187-0.081-0.187-0.219v-10.562c0-0.1381 0.0647-0.219 0.1875-0.219z"/>
> 
> I had never heard of block-progression. It seems to deal with text layout /
> writing modes. I don't think it is needed here.
> 
> You should be able to remove this attribute in all cases that don't apply to
> text.

Removing all the unneeded text related attributes ...

> > Source/WebInspectorUI/UserInterface/Images/gtk/NewTabPlus.svg:5
> > +  <text style="word-spacing:0px;letter-spacing:0px" font-weight="bold" xml:space="preserve" transform="scale(.99917 1.0008)" line-height="125%" font-size="18.288px" y="15.371576" x="0.45220301" font-family="Cantarell" fill="#000000"><tspan y="15.371576" x="0.45220301" fill="#000000">+</tspan></text>
> 
> Now here is a case where you are using text! That said, I wonder if it might
> be more appropriate to use a couple pathes, instead of text. What if the
> font differs between two GTK machines?

This is clearly a mistake that slipped among my fingers :(

Obviously, everything should have been converted to shapes. Thanks to this, now I've found another file that uses also a font (the one from which a derived this one). I will fix that in the optimizing patch that I'm already cooking.

Thanks a lot for your review Joseph (Joe?) :) !
Comment 7 Andres Gomez Garcia 2015-05-05 07:26:20 PDT
Created attachment 252383 [details]
Patch for landing
Comment 8 WebKit Commit Bot 2015-05-05 08:21:56 PDT
Comment on attachment 252383 [details]
Patch for landing

Clearing flags on attachment: 252383

Committed r183806: <http://trac.webkit.org/changeset/183806>
Comment 9 WebKit Commit Bot 2015-05-05 08:22:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Joseph Pecoraro 2015-05-05 11:09:16 PDT
> Thanks a lot for your review Joseph (Joe?) :) !

Joe, JoePeck, Joseph, all work for me =)