1009145 - HDPI support for Debugger icons

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Comment 1

[Tim Nguyen :ntim]

Attachment: Patch v1

Here's what this patch does :
- Fixes the blank sidebar button :active state on Windows
- Adds HDPI support for the debugger

What I haven't done yet :
- Add HDPI support the source editor icons (inside the debugger)
- Add HDPI support for the toggle breakpoints button (as there is no asset)

Comment 2

[Tim Nguyen :ntim]

Attachment: Patch v1.1

Now with r=bgrins in the commit message.

Comment 3

[Tim Nguyen :ntim]

Attachment: Patch v1.2

Now with debugger toggle breakpoint HDPI icon (found it in original implementation bug)

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen [:ntim] from comment #3)
> Created attachment 8423379 [details] [diff] [review]
> Patch v1.2
> 
> Now with debugger toggle breakpoint HDPI icon (found it in original
> implementation bug)

How did you find that :)?  I looked around for it in Bug 837188 and couldn't find it.

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8423379 [details] [diff] [review]
Patch v1.2

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

I'm getting an error applying the patch: `1 out of 5 hunks FAILED -- saving rejects to file browser/themes/shared/devtools/debugger.inc.css.rej`.  Can you pull down the latest code and rebase?

Comment 6

[Tim Nguyen :ntim]

Attachment: Patch v2

Rebased and now includes new HDPI source editor icons (breakpoint and debug-location).

Comment 7

[Tim Nguyen :ntim]

Attachment: Patch v2.1

Fixed small issue on Variables view

Comment 8

[Brian Grinstead [:bgrins]]

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

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

Hmm, now I'm getting rejects on the jar.mn files.  Looks like an easy fix, but would you mind updating the patch?

Comment 9

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #8)
> Comment on attachment 8423420 [details] [diff] [review]
> Patch v2.1
> 
> Review of attachment 8423420 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hmm, now I'm getting rejects on the jar.mn files.  Looks like an easy fix,
> but would you mind updating the patch?

Sorry, didn't mention it, you need to apply it on top of bug 1009002 (which will land soon).

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen [:ntim] from comment #9)
> (In reply to Brian Grinstead [:bgrins] from comment #8)
> > Comment on attachment 8423420 [details] [diff] [review]
> > Patch v2.1
> > 
> > Review of attachment 8423420 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Hmm, now I'm getting rejects on the jar.mn files.  Looks like an easy fix,
> > but would you mind updating the patch?
> 
> Sorry, didn't mention it, you need to apply it on top of bug 1009002 (which
> will land soon).

Ah, got it.  No problem

Comment 11

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #4)
> (In reply to Tim Nguyen [:ntim] from comment #3)
> > Created attachment 8423379 [details] [diff] [review]
> > Patch v1.2
> > 
> > Now with debugger toggle breakpoint HDPI icon (found it in original
> > implementation bug)
> 
> How did you find that :)?  I looked around for it in Bug 837188 and couldn't
> find it.
It was in Bug 815280 ;)

Comment 12

[Brian Grinstead [:bgrins]]

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

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

Make sure all the 2x files are added.  I'm seeing this when building: RuntimeError: File "../shared/devtools/images/[e-mail]"

Comment 13

[Brian Grinstead [:bgrins]]

Also, I'm curious - are you checking these locally with 2x display as you work on these patches?

Comment 14

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #13)
> Also, I'm curious - are you checking these locally with 2x display as you
> work on these patches?

Well, I don't have a retina display. But I do use one of these techniques (on Windows) :
- about:config layout.css.devPixelsPerPx
- Simply zoom in inside the toolbox (it also emulates the retina assets at 200%)

Comment 15

[Tim Nguyen :ntim]

Attachment: Patch v2.2

Fixed missing images.

Hopefully this one is final ;)

Comment 16

[Brian Grinstead [:bgrins]]

Comment on attachment 8423945 [details] [diff] [review]
Patch v2.2

Asking victor for review since this is affecting the debugger ui.

Comment 17

[Victor Porof [:vporof][:vp]]

Comment on attachment 8423945 [details] [diff] [review]
Patch v2.2

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

::: browser/themes/shared/devtools/debugger.inc.css
@@ +585,5 @@
>    min-height: 1em !important;
>    padding: 0 !important;
>  }
>  
> +#debugger-toolbar .devtools-toolbarbutton > .toolbarbutton-icon,

This rule should also be 
#debugger-toolbar .devtools-toolbarbutton:not([label]) > .toolbarbutton-icon,
right?

@@ +661,5 @@
>  #instruments-pane-toggle[pane-collapsed] {
>    list-style-image: url(debugger-expand.png);
>  }
>  
> +#instruments-pane-toggle:hover {

Why was this changed to :hover?

::: browser/themes/windows/devtools/debugger.css
@@ -12,5 @@
> -  -moz-image-region: rect(0px,32px,16px,16px);
> -}
> -
> -#instruments-pane-toggle:hover:active {
> -  -moz-image-region: rect(0px,48px,16px,32px);

Aren't these changed in a different patch?

Comment 18

[Brian Grinstead [:bgrins]]

Comment on attachment 8423945 [details] [diff] [review]
Patch v2.2

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

::: browser/themes/windows/devtools/debugger.css
@@ -12,5 @@
> -  -moz-image-region: rect(0px,32px,16px,16px);
> -}
> -
> -#instruments-pane-toggle:hover:active {
> -  -moz-image-region: rect(0px,48px,16px,32px);

Actually, yeah.  The same thing is just being removed in Bug 1011727

Comment 19

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #18)
> Comment on attachment 8423945 [details] [diff] [review]
> Patch v2.2
> 
> Review of attachment 8423945 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/windows/devtools/debugger.css
> @@ -12,5 @@
> > -  -moz-image-region: rect(0px,32px,16px,16px);
> > -}
> > -
> > -#instruments-pane-toggle:hover:active {
> > -  -moz-image-region: rect(0px,48px,16px,32px);
> 
> Actually, yeah.  The same thing is just being removed in Bug 1011727

Can you revert the change for windows/debugger.css?  This will be handled in another bug

Comment 20

[Tim Nguyen :ntim]

(In reply to Victor Porof [:vporof][:vp] from comment #17)
> Comment on attachment 8423945 [details] [diff] [review]
> Patch v2.2
> 
> Review of attachment 8423945 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/shared/devtools/debugger.inc.css
> @@ +585,5 @@
> >    min-height: 1em !important;
> >    padding: 0 !important;
> >  }
> >  
> > +#debugger-toolbar .devtools-toolbarbutton > .toolbarbutton-icon,
> 
> This rule should also be 
> #debugger-toolbar .devtools-toolbarbutton:not([label]) > .toolbarbutton-icon,
> right?
Is there even a font icon in #debugger-toolbar ? The :not([label]) is for the prettify icon.
Or should I do the change anyway ?

Comment 21

[Tim Nguyen :ntim]

Attachment: Patch v2.3 (r=vporof)

Went ahead with the :not([label]) change as it won't affect anything.

Also rebased my patch on top of Bug 1011727

Comment 22

[Tim Nguyen :ntim]

Attachment: Patch v2.4 (r=vporof)

Didn't realize one change didn't get applied.

Comment 23

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen [:ntim] from comment #22)
> Created attachment 8424205 [details] [diff] [review]
> Patch v2.4 (r=vporof)
> 
> Didn't realize one change didn't get applied.

You will need a push to try.  Do you have access to the try server?

Comment 24

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #23)
> (In reply to Tim Nguyen [:ntim] from comment #22)
> > Created attachment 8424205 [details] [diff] [review]
> > Patch v2.4 (r=vporof)
> > 
> > Didn't realize one change didn't get applied.
> 
> You will need a push to try.  Do you have access to the try server?

Nope :( I was gonna ask for access, although, I have no idea how to create an SSH key.

Comment 25

[Tim Nguyen :ntim]

Can you provide a try link for me please ? Thanks :)
And if needed, can you also do it for bug 1011249 please ?

Comment 26

[Brian Grinstead [:bgrins]]

https://tbpl.mozilla.org/?tree=Try&rev=925acb579a1b

Comment 27

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen [:ntim] from comment #24)
> (In reply to Brian Grinstead [:bgrins] from comment #23)
> > (In reply to Tim Nguyen [:ntim] from comment #22)
> > > Created attachment 8424205 [details] [diff] [review]
> > > Patch v2.4 (r=vporof)
> > > 
> > > Didn't realize one change didn't get applied.
> > 
> > You will need a push to try.  Do you have access to the try server?
> 
> Nope :( I was gonna ask for access, although, I have no idea how to create
> an SSH key.

You should request access to for try so you can push these.  There is a tutorial for creating the keys on windows here: https://help.github.com/articles/generating-ssh-keys#platform-windows.  Then follow the instructions here to request commit access level 1: https://www.mozilla.org/hacking/committer/ - I will vouch for you.

Comment 28

[Carsten Book [:Tomcat]]

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

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/79d74ac58388