Hey Tim, Tried to pick up where you left off. I pixelsnapped some additional toolbars in addition to cleaning up the debugger icons (step-over, step-in, etc.), the command-no-autohide button (bug 1260560), and a few other odds and ends.

The new icons are a definite improvement. I really the perf icon :) I'm seeing some sizing inconsistencies as highlighted in the screenshot, and some shape inconsistencies as well (the shape of both breakpoint icons is different, as well as the { } icon).

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #8) > Created attachment 8769766 [details] > critique helen's icons-1.png > > The new icons are a definite improvement. I really the perf icon :) I really like*

Comment on attachment 8769748 [details] [diff] [review] thin-icons.patch Review of attachment 8769748 [details] [diff] [review]: ----------------------------------------------------------------- The code changes look good so far. ::: devtools/client/themes/images/itemToggle.svg @@ +1,4 @@ > +<!-- This Source Code Form is subject to the terms of the Mozilla Public > + - License, v. 2.0. If a copy of the MPL was not distributed with this > + - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > +<svg width="16" height="16" viewBox="0 0 16 16" xmlns="http://www.w3.org/2000/svg" fill="whitesmoke"> You might want to put this icon in sync with debugger-blackbox.svg, since it's supposed to be the same eye icon. ::: devtools/client/themes/images/power.svg @@ +1,5 @@ > +<?xml version="1.0" encoding="UTF-8" standalone="no"?> > +<svg width="16px" height="16px" viewBox="0 0 16 16" fill="whitesmoke" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" xmlns:sketch="http://www.bohemiancoding.com/sketch/ns"> > + <path d="M8.5,14 C5.46243388,14 3,11.5375661 3,8.5 C3,5.46243388 5.46243388,3 8.5,3 C11.5375661,3 14,5.46243388 14,8.5 C14,11.5375661 11.5375661,14 8.5,14 L8.5,14 Z M8.5,13 C10.9852814,13 13,10.9852814 13,8.5 C13,6.01471863 10.9852814,4 8.5,4 C6.01471863,4 4,6.01471863 4,8.5 C4,10.9852814 6.01471863,13 8.5,13 L8.5,13 Z M10.6892468,4.56750356 L11.3722285,3.80863499 L11.3722285,3.80863499 C10.5362548,3.29572747 9.55266283,3 8.5,3 C7.44733717,3 6.46374519,3.29572747 5.62777149,3.80863499 L6.3107532,4.56750356 C6.95873288,4.20599601 7.70530997,4 8.5,4 C9.29469003,4 10.0412671,4.20599601 10.6892468,4.56750356 L10.6892468,4.56750356 L10.6892468,4.56750356 Z" id="Triangle" sketch:type="MSShapeGroup"></path> > + <path d="M8,8 L9,8 L9,2 L8,2 L8,8 Z" id="Shape" sketch:type="MSShapeGroup"></path> > + </svg> \ No newline at end of file This SVG needs cleanup ::: devtools/client/themes/images/tool-profiler.svg @@ +1,5 @@ > <!-- This Source Code Form is subject to the terms of the Mozilla Public > - License, v. 2.0. If a copy of the MPL was not distributed with this > - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > +<svg width="16" height="16" viewBox="0 0 16 16" xmlns="http://www.w3.org/2000/svg" fill="whitesmoke"> > + <g fill-rule="evenodd"> the <g> tag can be removed, and fill-rule="evenodd" can be set on <svg> ::: devtools/client/themes/images/tool-scratchpad.svg @@ +1,4 @@ > +<svg width="16" height="16" viewBox="0 0 16 16" xmlns="http://www.w3.org/2000/svg"> > + <title> > + tool-scratchpad-expanded > + </title> The title can be removed @@ +1,5 @@ > +<svg width="16" height="16" viewBox="0 0 16 16" xmlns="http://www.w3.org/2000/svg"> > + <title> > + tool-scratchpad-expanded > + </title> > + <g fill="whitesmoke" fill-rule="evenodd"> Same comment about the g tag.

I know you marked the last patch as r+, but would you mind taking a second look just to be safe? I ended up updating a few more icons, couldn't help myself. (In reply to Tim Nguyen :ntim (use needinfo?) from comment #8) > The new icons are a definite improvement. I really the perf icon :) That icon is all Bryan Bell, can't take credit for it :D (In reply to Tim Nguyen :ntim (use needinfo?) from comment #8) > Created attachment 8769766 [details] > critique helen's icons-1.png > I'm seeing some sizing inconsistencies as highlighted in the screenshot, and > some shape inconsistencies as well (the shape of both breakpoint icons is > different, as well as the { } icon). I updated the {} icon to be the same. The debugger icon and the toggle breakpoints icon is purposefully different, however. Updated the shader editor icon, it was a little blurry. Made the commandnoautohide icon a little larger, but wasn't sure what to do about webaudio and scratchpad—on scratchpad the lines aren't centered if I give it one pixel of height, and when I give it two pixels of height it looks kinda large. Since both it and webaudio aren't /super/ used, I think it's okay to leave them. (In reply to Tim Nguyen :ntim (use needinfo?) from comment #10) > Created attachment 8769770 [details] > critique-2.png I think the stroke seeming large on the filter icon is visual trickery—I bumped down the size of the "liquid" inside the filter and I think it helps, let me know what you think. (In reply to Tim Nguyen :ntim (use needinfo?) from comment #11) > Created attachment 8769772 [details] > feedback-3.png Updated!

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #13) > Created attachment 8769853 [details] [diff] [review] > thin-icons.patch > > I know you marked the last patch as r+, but would you mind taking a second > look just to be safe? I ended up updating a few more icons, couldn't help > myself. Sure, taking a look. > (In reply to Tim Nguyen :ntim (use needinfo?) from comment #8) > > Created attachment 8769766 [details] > > critique helen's icons-1.png > > I'm seeing some sizing inconsistencies as highlighted in the screenshot, and > > some shape inconsistencies as well (the shape of both breakpoint icons is > > different, as well as the { } icon). > > I updated the {} icon to be the same. The debugger icon and the toggle > breakpoints icon is purposefully different, however. Looks better. > Updated the shader editor icon, it was a little blurry. Looks better as well. > Made the commandnoautohide icon a little larger, but wasn't sure what to do > about webaudio and scratchpad—on scratchpad the lines aren't centered if I > give it one pixel of height, and when I give it two pixels of height it > looks kinda large. Since both it and webaudio aren't /super/ used, I think > it's okay to leave them. I thought the old scratchpad icon looked better sizing-wise, any reason why we can't keep it/pixel-snap it ? > (In reply to Tim Nguyen :ntim (use needinfo?) from comment #10) > > Created attachment 8769770 [details] > > critique-2.png > I think the stroke seeming large on the filter icon is visual trickery—I > bumped down the size of the "liquid" inside the filter and I think it helps, > let me know what you think. It looks a bit better, but the stroke still feels a bit thick. I've opened the icon with sketch, and it looks like the bottom part of the icon isn't pixel snapped, which is making the stroke blurry. > (In reply to Tim Nguyen :ntim (use needinfo?) from comment #11) > > Created attachment 8769772 [details] > > feedback-3.png > Updated! Thanks, these look good!

Comment on attachment 8769853 [details] [diff] [review] thin-icons.patch Review of attachment 8769853 [details] [diff] [review]: ----------------------------------------------------------------- r+ for the code changes. f+ for UI. Also, I've just realised you've missed 2 toolbar icons: rewind.png and fast-forward.png. Can you update them as well ? I guess you should be able to fork the play icon. ::: devtools/client/themes/images/diff.svg @@ +3,5 @@ > - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > +<svg height="16" width="16" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 16 16" fill="whitesmoke"> > + <path d="M10.2 4.1c-.6 0-1.2.2-1.8.4-.6-.4-1.4-.6-2.2-.6-2.5 0-4.6 2.1-4.6 4.6s2.1 4.6 4.6 4.6c.9 0 1.6-.2 2.3-.7.5.2 1.1.4 1.7.4 2.4 0 4.3-1.9 4.3-4.3.1-2.4-1.9-4.4-4.3-4.4zm-4 7.9c-1.9 0-3.5-1.6-3.5-3.5S4.2 5 6.2 5c.3 0 .7 0 1 .1-.8.9-1.4 2.1-1.4 3.4 0 1.3.6 2.5 1.5 3.3-.4.1-.8.2-1.1.2zm2.1-.7c-.9-.6-1.4-1.6-1.4-2.8 0-1.1.6-2.1 1.4-2.8.8.6 1.4 1.6 1.4 2.8 0 1.1-.6 2.1-1.4 2.8z"/> > + <path d="M7.6 8c-.2 0-.3-.1-.4-.2-.1-.2 0-.4.2-.5l1.1-.6c.2-.1.4 0 .5.2.1.2 0 .4-.2.5l-1.1.5c0 .1-.1.1-.1.1zM7.6 9.1c-.1 0-.3-.1-.4-.2-.1-.2 0-.4.2-.5l1.1-.6c.3-.1.5 0 .6.2.1.2 0 .4-.2.5l-1.1.6h-.2zM7.8 10.3c-.1 0-.3-.1-.4-.2-.1-.2 0-.4.2-.5L8.8 9c.2-.1.4 0 .5.2.1.2 0 .4-.2.5l-1.1.6h-.2z"/> > + </svg> \ No newline at end of file nit: leading whitespace ::: devtools/client/themes/images/filter.svg @@ +1,4 @@ > <!-- This Source Code Form is subject to the terms of the Mozilla Public > - License, v. 2.0. If a copy of the MPL was not distributed with this > - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > +<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16" fill="whitesmoke"> The fill was intentionally set to #aaa for this icon, to prevent it from being invisible in the light theme filter boxes (where we can't apply the invert filter) If you want both colours to be supported, you can use a location hash to designate the wanted colour. You can see how it's done here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/images/commandline-icon.svg Let me know if you'd like me to do it.

The play icon is offcenter. This is not noticeable in the debugger, but it looks a bit odd in the animation inspector.

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #14) > I thought the old scratchpad icon looked better sizing-wise, any reason why > we can't keep it/pixel-snap it ? I made it a little larger—think it's looking pretty similar. We can fudge it another pixel if you still don't like it. > > (In reply to Tim Nguyen :ntim (use needinfo?) from comment #10) > It looks a bit better, but the stroke still feels a bit thick. I've opened > the icon with sketch, and it looks like the bottom part of the icon isn't > pixel snapped, which is making the stroke blurry. I think this is fixed now, take a gander. (In reply to Tim Nguyen :ntim (use needinfo?) from comment #16) > Created attachment 8769910 [details] > off centre icon.png > > The play icon is offcenter. This is not noticeable in the debugger, but it > looks a bit odd in the animation inspector. Fixed! (In reply to Tim Nguyen :ntim (use needinfo?) from comment #15) > Also, I've just realised you've missed 2 toolbar icons: rewind.png and > fast-forward.png. Can you update them as well ? I guess you should be able > to fork the play icon. Added these. Didn't see the fast-forward icon used anywhere in dxr, but this might be worth double-checking. > ::: devtools/client/themes/images/filter.svg > @@ +1,4 @@ > > <!-- This Source Code Form is subject to the terms of the Mozilla Public > > - License, v. 2.0. If a copy of the MPL was not distributed with this > > - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > > +<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16" fill="whitesmoke"> > > The fill was intentionally set to #aaa for this icon, to prevent it from > being invisible in the light theme filter boxes (where we can't apply the > invert filter) > > If you want both colours to be supported, you can use a location hash to > designate the wanted colour. You can see how it's done here: > https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/images/ > commandline-icon.svg > > Let me know if you'd like me to do it. I just set this back to #aaa. I'd like to change some icon colors at some point (I think your point in bug 1268591 is a good one) but I'd rather do it in another patch.

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #17) > Created attachment 8770245 [details] [diff] [review] > thin-icons.patch > > (In reply to Tim Nguyen :ntim (use needinfo?) from comment #14) > > I thought the old scratchpad icon looked better sizing-wise, any reason why > > we can't keep it/pixel-snap it ? > I made it a little larger—think it's looking pretty similar. We can fudge it > another pixel if you still don't like it. It looks better, but I still feel it's smaller than the other ones, maybe it's because the DOM icon next to it is too big ? > > > (In reply to Tim Nguyen :ntim (use needinfo?) from comment #10) > > It looks a bit better, but the stroke still feels a bit thick. I've opened > > the icon with sketch, and it looks like the bottom part of the icon isn't > > pixel snapped, which is making the stroke blurry. > I think this is fixed now, take a gander. Looks much better, thanks ! > (In reply to Tim Nguyen :ntim (use needinfo?) from comment #16) > > Created attachment 8769910 [details] > > off centre icon.png > > > > The play icon is offcenter. This is not noticeable in the debugger, but it > > looks a bit odd in the animation inspector. > Fixed! Thanks! Looks like the pause icon is also a bit off centre, but I'm fine with leaving it that way. > (In reply to Tim Nguyen :ntim (use needinfo?) from comment #15) > > Also, I've just realised you've missed 2 toolbar icons: rewind.png and > > fast-forward.png. Can you update them as well ? I guess you should be able > > to fork the play icon. > Added these. Didn't see the fast-forward icon used anywhere in dxr, but this > might be worth double-checking. Thanks! the fast-forward icon doesn't seem used, but it doesn't hurt updating it :) Anyway, I've noticed a pixel snapping issue with the rewind icon.

Also, little detail, it would be nice if the | part of the rewind icon has the same size as the | of the pause icon.

> > (In reply to Tim Nguyen :ntim (use needinfo?) from comment #16) > Thanks! Looks like the pause icon is also a bit off centre, but I'm fine > with leaving it that way. Fixed! > > (In reply to Tim Nguyen :ntim (use needinfo?) from comment #15) > Anyway, I've noticed a pixel snapping issue with the rewind icon. Fixed this too. > > (In reply to Tim Nguyen :ntim (use needinfo?) from comment #14) > It looks better, but I still feel it's smaller than the other ones, maybe > it's because the DOM icon next to it is too big ? Would you like to do another review cycle on this patch for this, or should we file a new bug? I'm kinda okay with this sizing difference personally, but it's up to you. (In reply to Tim Nguyen :ntim (use needinfo?) from comment #20) > Also, little detail, it would be nice if the | part of the rewind icon has > the same size as the | of the pause icon. Added this as well, I agree.

The scratchpad and webaudio tools are not frequently used, so I'm fine with a new bug.

Thanks for adressing my issues!

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #22) > The scratchpad and webaudio tools are not frequently used, so I'm fine with > a new bug. Okeydoke, filed Bug 1286624. (In reply to Tim Nguyen :ntim (use needinfo?) from comment #23) > Thanks for adressing my issues! Thanks for doing all of these reviews!

Pushed by ntim.bugs@gmail.com: https://hg.mozilla.org/integration/fx-team/rev/105d425837e7 pixelsnap more toolbar icons and use thin style, r=ntim

I have reproduced this on firefox Nightly according to(2016-03-29) Fixing bug is verified on Latest Developer Edition-- Build ID: (20160831004001),User Agent: Mozilla/5.0 (Windows NT 10.0; rv:50.0) Gecko/20100101 Firefox/50.0 Tested OS-- Windows10 32bit