1009002 - HDPI web console image support

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Comment 1

[Tim Nguyen :ntim]

Attachment: Patch v1

Here's what I did in the patch :
- I converted -moz-image-rects into background-position (so I won't have to rewrite all the rules twice)
- I used ::before for the icon display since otherwise you would see some extra unwanted icon parts
- I added HDPI support for console icons (of course)
- I updated the command line icon to use the new asset included in the zip (lighter in colors and in size)

Comment 2

[Tim Nguyen :ntim]

As far as I checked, there are no regressions. I'm gonna check if there are some alignment issues.

Comment 3

[Tim Nguyen :ntim]

Attachment: Fancy screenshot of patch 1 (on Windows)

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8421167 [details] [diff] [review]
Patch v1

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

Why am I seeing 'Modified Binary File: browser/themes/shared/devtools/images/commandline-icon.png' here?  Shouldn't that file have stayed the same, but instead added the new browser/themes/shared/devtools/images/[e-mail] file?

Comment 5

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #4)
> Comment on attachment 8421167 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 8421167 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why am I seeing 'Modified Binary File:
> browser/themes/shared/devtools/images/commandline-icon.png' here?  Shouldn't
> that file have stayed the same, but instead added the new
> browser/themes/shared/devtools/images/[e-mail] file?

"I updated the command line icon to use the new asset included in the zip (lighter in colors and in size)"

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen [:ntim] from comment #5)
> (In reply to Brian Grinstead [:bgrins] from comment #4)
> > Comment on attachment 8421167 [details] [diff] [review]
> > Patch v1
> > 
> > Review of attachment 8421167 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Why am I seeing 'Modified Binary File:
> > browser/themes/shared/devtools/images/commandline-icon.png' here?  Shouldn't
> > that file have stayed the same, but instead added the new
> > browser/themes/shared/devtools/images/[e-mail] file?
> 
> "I updated the command line icon to use the new asset included in the zip
> (lighter in colors and in size)"

Ah, fair enough.  Shouldn't there also be a new file added for [e-mail]?

Comment 7

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #6)
> (In reply to Tim Nguyen [:ntim] from comment #5)
> > (In reply to Brian Grinstead [:bgrins] from comment #4)
> > > Comment on attachment 8421167 [details] [diff] [review]
> > > Patch v1
> > > 
> > > Review of attachment 8421167 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Why am I seeing 'Modified Binary File:
> > > browser/themes/shared/devtools/images/commandline-icon.png' here?  Shouldn't
> > > that file have stayed the same, but instead added the new
> > > browser/themes/shared/devtools/images/[e-mail] file?
> > 
> > "I updated the command line icon to use the new asset included in the zip
> > (lighter in colors and in size)"
> 
> Ah, fair enough.  Shouldn't there also be a new file added for
> [e-mail]?

Yeah, you're right, forgot to hg add. Gonna post an updated patch now.

Comment 8

[Tim Nguyen :ntim]

Attachment: Patch v1.1

Added forgotten [e-mail] file.

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen [:ntim] from comment #2)
> As far as I checked, there are no regressions. I'm gonna check if there are
> some alignment issues.

FYI this page has easy ways to test all sorts of different console output: https://mihaisucan.github.io/mozilla-work/test.html

Comment 10

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #9)
> (In reply to Tim Nguyen [:ntim] from comment #2)
> > As far as I checked, there are no regressions. I'm gonna check if there are
> > some alignment issues.
> 
> FYI this page has easy ways to test all sorts of different console output:
> https://mihaisucan.github.io/mozilla-work/test.html

Thanks for the tip :) I checked again, and there are no regressions happening.

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 8421203 [details] [diff] [review]
Patch v1.1

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

Looks very good - I will need to go through thoroughly to check each icon type just to make sure we don't have any regressions before r+, but if you make the minor changes suggested then add r=bgrins in the commit message I will take a look.

::: browser/themes/shared/devtools/webconsole.inc.css
@@ -39,4 @@
>    flex: 0 0 auto;
>    margin: 3px 6px 0 0;
>    padding: 0 4px;
>    width: 8px;

This width is not used anymore (it is set by the ::before).

@@ +348,5 @@
>  
> +@media (min-resolution: 2dppx) {
> +  .jsterm-input-node {
> +    background-image: -moz-image-rect(url('chrome://browser/skin/devtools/[e-mail]'), 0, 64, 32, 32);
> +    background-size: 16px 16px;

I've been moving the background-size stuff into the original rule just as a matter of convention.  Then break the background: rule in the original jsterm-input-node into background-image, background-repeat, background-size just like you did for the message icon.  Then here you can just override background-image.

Comment 12

[Tim Nguyen :ntim]

Attachment: Patch v2

Addressed comments

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8421738 [details] [diff] [review]
Patch v2

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

Looks good, icons appear unchanged on normal displays and have been updated with hidpi support.

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3afac5d337b6