1012139 - Remaining DevTools HDPI work

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Tim Nguyen :ntim]

Here's what's left to do :
- Network sidebar toggle icon
- Stop black boxing this source button in Debugger (this uses the image="" attribute, so it's a bit harder)
- Style Editor and Shader Editor Eye icon.

I couldn't think of anything more.

Comment 1

[Tim Nguyen :ntim]

Attachment: Part 1 : Network monitor

I haven't done the eyeballs yet, since I'm getting conflicts when applying your patch, so I'm probably gonna wait for it to land on m-c.

And for the debugger stop black boxing source button, I couldn't figure out a good solution yet. Do you know what could be possible ? I thought of checking retina after load, then change the image="" attribute if it's a retina screen.

Comment 2

[Brian Grinstead [:bgrins]]

> And for the debugger stop black boxing source button, I couldn't figure out
> a good solution yet. Do you know what could be possible ? I thought of
> checking retina after load, then change the image="" attribute if it's a
> retina screen.

Are you talking about black-boxed-message-button?  If so, I'd say let's move that into CSS instead of debugger.xul.

Also, could you remove the file themes/devtools/shared/images/debugger-blackBoxMessageEye.png?  I don't think it is in use anymore.

Comment 3

[Brian Grinstead [:bgrins]]

Comment on attachment 8425859 [details] [diff] [review]
Part 1 : Network monitor

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

I'm getting rejects applying the patch - somehow :).  Also, I think this is actually doubling the size of the icon when in 2x mode after looking at it.

Comment 4

[Tim Nguyen :ntim]

Attachment: WIP v1

I haven't tested the patch yet unfortunatly, I had to do a full rebuild.
This patch addresses the following :
- Removes some references to background-noise-toolbar.png
- Adds HDPI support for the network monitor
- Adds HDPI support for the style and shader editors
- Removes the file you asked

It doesn't address the debugger black box button yet.

Comment 5

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #3)
> Comment on attachment 8425859 [details] [diff] [review]
> Part 1 : Network monitor
> 
> Review of attachment 8425859 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Also, I think this is actually doubling the size of the icon when in 2x mode after looking at it.

I can't seem to fix this issue. I tried to adjust the height and width, but no luck. Maybe I should switch to background-image ?

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen [:ntim] from comment #5)
> (In reply to Brian Grinstead [:bgrins] from comment #3)
> > Comment on attachment 8425859 [details] [diff] [review]
> > Part 1 : Network monitor
> > 
> > Review of attachment 8425859 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Also, I think this is actually doubling the size of the icon when in 2x mode after looking at it.
> 
> I can't seem to fix this issue. I tried to adjust the height and width, but
> no luck. Maybe I should switch to background-image ?

Yes, I would switch to background-image

Comment 7

[Brian Grinstead [:bgrins]]

(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 #3)
> > > Comment on attachment 8425859 [details] [diff] [review]
> > > Part 1 : Network monitor
> > > 
> > > Review of attachment 8425859 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Also, I think this is actually doubling the size of the icon when in 2x mode after looking at it.
> > 
> > I can't seem to fix this issue. I tried to adjust the height and width, but
> > no luck. Maybe I should switch to background-image ?
> 
> Yes, I would switch to background-image

If you'd rather stick with list-style-image - there are plenty of places in browser/themes using list-style-image in this context, so I know it is possible.  If you search for '2dppx' you will see them.

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8428231 [details] [diff] [review]
WIP v1

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

Not sure why, but I get an error applying to styleeditor: '1 out of 2 hunks FAILED -- saving rejects to file browser/themes/shared/devtools/styleeditor.css.rej'.

Otherwise, it looks good once the 2x icon for netmonitor.inc.css is worked out.

Comment 9

[Tim Nguyen :ntim]

Attachment: Patch v1

Comment 10

[Brian Grinstead [:bgrins]]

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

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

Looks good - were you also going to update black-boxed-message-button in this patch?

Comment 11

[Brian Grinstead [:bgrins]]

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

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

Cancelling review - it sounds like there is a plan for the black box icon

Comment 12

[Tim Nguyen :ntim]

Attachment: Patch v2

Haven't tested the patch yet, since it's building for an hour now.

Comment 13

[Brian Grinstead [:bgrins]]

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

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

I'm getting an error building: 'RuntimeError: File "../shared/devtools/images/debugger-blackbox-eye.png" not found'.  Looks like that file shouldn't be deleted.

Comment 14

[Tim Nguyen :ntim]

Attachment: Patch v2.1

Fixed missing image.

Comment 15

[Brian Grinstead [:bgrins]]

Comment on attachment 8431190 [details] [diff] [review]
Patch v2.1

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

These changes look good - push to try before marking checkin-needed

Comment 16

[Tim Nguyen :ntim]

Pushed to try : https://tbpl.mozilla.org/?tree=Try&rev=672b1905148b

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

[Phil Ringnalda (:philor)]

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