1068381 - Prettyprint button should be an icon, not a label

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Sebastian Zartner [:sebo]]

The pretty-print button within the Debugger panel has a label instead of an icon, which makes styling of it difficult.

So the button should have an icon assigned via CSS like the other buttons within the devtools UI.

FWIW this is needed for Firebug.[1]

Sebastian

[1] https://github.com/firebug/firebug.next/issues/63

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Brian, this is simple to fix, but we need an icon...
Any tips what icon we could use?

Honza

Comment 2

[Brian Grinstead [:bgrins]]

(In reply to Jan Honza Odvarko [:Honza] from comment #1)
> Brian, this is simple to fix, but we need an icon...
> Any tips what icon we could use?
> 
> Honza

Stephen, I don't see any pretty print icon in the zip attached to Bug 837188.  It's the '{}' button as seen here: https://hacks.mozilla.org/wp-content/uploads/2013/12/pretty-print-minified-JS.png.  We are using text for the button right now, but that makes it harder to style and less consistent across operating systems.  It'd be more consistent if we could use a 1x and 2x png for this, just like debugger-blackbox.png.  Can you help?

Comment 3

[Sebastian Zartner [:sebo]]

> It'd be more consistent if we could use a 1x and 2x png for this
General question: Why did you decide for PNGs instead of using SVG icons like Firebug does since version 2.0?

Sebastian

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to Sebastian Zartner from comment #3)
> > It'd be more consistent if we could use a 1x and 2x png for this
> General question: Why did you decide for PNGs instead of using SVG icons
> like Firebug does since version 2.0?
> 
> Sebastian

I started requesting SVG for new icons when we switched to 2x in the blockers for Bug 837188, but for a lot of them it was just easier to stick with png since we already had the images in that format.  I wish we were using SVG for everything because it cleans up the CSS a lot.  For this case SVG would be great, but I think we would also want to do all of the other debugger buttons at the same time if we were to make the switch.

Comment 5

[Sebastian Zartner [:sebo]]

> but for a lot of them it was just easier to stick with png since we already had the images in that format.
FWIW we also had our icons in PNG format earlier but we took the time to switch everything to SVG.[1]

> I wish we were using SVG for everything because it cleans up the CSS a lot.  For this case SVG would 
> be great, but I think we would also want to do all of the other debugger buttons at the same time if 
> we were to make the switch.
So should I file a bug for switching all icons to SVG?

Sebastian

[1] https://code.google.com/p/fbug/issues/detail?id=7000

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Sebastian Zartner from comment #5)
> So should I file a bug for switching all icons to SVG?

Sure, probably the most up to date collection of images that need to be converted is from the repository (https://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/images/), rather than one of the zips from a previous bug, as we've added and modified quite a few images as one-offs in the meantime.

Comment 7

[Sebastian Zartner [:sebo]]

Attachment: debugger-prettyprint.png

Normal resolution prettyprint icon

Comment 8

[Sebastian Zartner [:sebo]]

Attachment: [e-mail]

High resolution prettyprint icon

Comment 9

[Sebastian Zartner [:sebo]]

Attachment: prettyprint.patch

Replace the label by the icon

Comment 10

[Sebastian Zartner [:sebo]]

This is my very first patch against Mozilla code, so Brian, could you please review and check if I did it right and guide me with the next steps to do?

Note that I had to add my name to the patch manually. Not sure how to get Mercurial to do that for me.

Sebastian

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to Sebastian Zartner from comment #10)
> This is my very first patch against Mozilla code, so Brian, could you please
> review and check if I did it right and guide me with the next steps to do?

Sure!  The patch applies fine, but please add the new image files to the patch with `hg add`.  Also, when you reupload the patch, add r=bgrins at the end of the commit message and upload with me in the r? field.

> Note that I had to add my name to the patch manually. Not sure how to get
> Mercurial to do that for me.

See this document https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F, it will help you get that set up.

Comment 12

[Sebastian Zartner [:sebo]]

Attachment: prettyprint2.patch

>> Note that I had to add my name to the patch manually. Not sure how to get
>> Mercurial to do that for me.
>
> See this document https://developer.mozilla.org/en-US
> /docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F, it will help
> you get that set up.

I read that before I uploaded my first patch, though it didn't work. I guess my problem could be related to bug 1077326 as I got the same errors.

Anyway, I hope it works for you now.

Sebastian

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8515125 [details] [diff] [review]
prettyprint2.patch

Review of attachment 8515125 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!  Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=2eac4b0444ff

Comment 14

[Sebastian Zartner [:sebo]]

If I interpret those test results correctly, the failing tests are unrelated. (It's a minor layout change, anyway.)
So what's next? Somehow I need to request a check-in, right?

Sebastian

Comment 15

[Brian Grinstead [:bgrins]]

(In reply to Sebastian Zartner from comment #14)
> If I interpret those test results correctly, the failing tests are
> unrelated. (It's a minor layout change, anyway.)
> So what's next? Somehow I need to request a check-in, right?
> 
> Sebastian

Yeah, you can add the checkin-needed keyword to the bug

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/e11192d5f965

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e11192d5f965