Closed Bug 1009002 Opened 7 years ago Closed 7 years ago

HDPI web console image support

Categories

(DevTools :: Console, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 32

People

(Reporter: ntim, Assigned: ntim)

References

Details

Attachments

(2 files, 2 obsolete files)

No description provided.
Attached patch Patch v1 (obsolete) — Splinter Review
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)
Attachment #8421167 - Flags: review?(bgrinstead)
As far as I checked, there are no regressions. I'm gonna check if there are some alignment issues.
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/commandline-icon@2x.png file?
(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/commandline-icon@2x.png file?

"I updated the command line icon to use the new asset included in the zip (lighter in colors and in size)"
(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/commandline-icon@2x.png 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 commandline-icon@2x.png?
Component: Developer Tools → Developer Tools: Console
(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/commandline-icon@2x.png 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
> commandline-icon@2x.png?

Yeah, you're right, forgot to hg add. Gonna post an updated patch now.
Attached patch Patch v1.1 (obsolete) — Splinter Review
Added forgotten commandline-icon@2x.png file.
Attachment #8421167 - Attachment is obsolete: true
Attachment #8421167 - Flags: review?(bgrinstead)
Attachment #8421203 - Flags: review?(bgrinstead)
(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
(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 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/commandline-icon@2x.png'), 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.
Attachment #8421203 - Flags: review?(bgrinstead)
Attached patch Patch v2Splinter Review
Addressed comments
Attachment #8421203 - Attachment is obsolete: true
Attachment #8421738 - Flags: review?(bgrinstead)
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.
Attachment #8421738 - Flags: review?(bgrinstead) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3afac5d337b6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 32
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.