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)

Now with r=bgrins in the commit message.

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

(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 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?

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

Fixed small issue on Variables view

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?

(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).

(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

(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 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/editor-breakpoint@2x.png"

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

(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%)

Fixed missing images. Hopefully this one is final ;)

Comment on attachment 8423945 [details] [diff] [review] Patch v2.2 Asking victor for review since this is affecting the debugger ui.

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 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

(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

(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 ?

Went ahead with the :not([label]) change as it won't affect anything. Also rebased my patch on top of Bug 1011727

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

(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?

(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.

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

(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.