Closed
Bug 1009145
Opened 10 years ago
Closed 10 years ago
HDPI support for Debugger icons
Categories
(DevTools :: Debugger, defect)
DevTools
Debugger
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 32
People
(Reporter: ntim, Assigned: ntim)
References
Details
Attachments
(1 file, 7 obsolete files)
44.21 KB,
patch
|
ntim
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•10 years ago
|
Component: Developer Tools → Developer Tools: Debugger
OS: Windows 8.1 → All
Hardware: x86_64 → All
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → ntim007
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•10 years ago
|
||
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)
Attachment #8423314 -
Flags: feedback?(bgrinstead)
Assignee | ||
Comment 2•10 years ago
|
||
Now with r=bgrins in the commit message.
Attachment #8423314 -
Attachment is obsolete: true
Attachment #8423314 -
Flags: feedback?(bgrinstead)
Attachment #8423339 -
Flags: feedback?(bgrinstead)
Assignee | ||
Comment 3•10 years ago
|
||
Now with debugger toggle breakpoint HDPI icon (found it in original implementation bug)
Attachment #8423339 -
Attachment is obsolete: true
Attachment #8423339 -
Flags: feedback?(bgrinstead)
Attachment #8423379 -
Flags: review?(bgrinstead)
Comment 4•10 years ago
|
||
(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•10 years ago
|
||
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?
Attachment #8423379 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 6•10 years ago
|
||
Rebased and now includes new HDPI source editor icons (breakpoint and debug-location).
Attachment #8423379 -
Attachment is obsolete: true
Attachment #8423417 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 7•10 years ago
|
||
Fixed small issue on Variables view
Attachment #8423417 -
Attachment is obsolete: true
Attachment #8423417 -
Flags: review?(bgrinstead)
Attachment #8423420 -
Flags: review?(bgrinstead)
Comment 8•10 years ago
|
||
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?
Assignee | ||
Comment 9•10 years ago
|
||
(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).
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(bgrinstead)
Comment 10•10 years ago
|
||
(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
Depends on: 1009002
Flags: needinfo?(bgrinstead)
Assignee | ||
Comment 11•10 years ago
|
||
(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•10 years ago
|
||
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"
Attachment #8423420 -
Flags: review?(bgrinstead)
Comment 13•10 years ago
|
||
Also, I'm curious - are you checking these locally with 2x display as you work on these patches?
Assignee | ||
Comment 14•10 years ago
|
||
(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%)
Assignee | ||
Comment 15•10 years ago
|
||
Fixed missing images. Hopefully this one is final ;)
Attachment #8423420 -
Attachment is obsolete: true
Attachment #8423945 -
Flags: review?(bgrinstead)
Comment 16•10 years ago
|
||
Comment on attachment 8423945 [details] [diff] [review] Patch v2.2 Asking victor for review since this is affecting the debugger ui.
Attachment #8423945 -
Flags: review?(bgrinstead) → review?(vporof)
Comment 17•10 years ago
|
||
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?
Attachment #8423945 -
Flags: review?(vporof) → review+
Comment 18•10 years ago
|
||
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•10 years ago
|
||
(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
Assignee | ||
Comment 20•10 years ago
|
||
(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 ?
Flags: needinfo?(vporof)
Assignee | ||
Comment 21•10 years ago
|
||
Went ahead with the :not([label]) change as it won't affect anything. Also rebased my patch on top of Bug 1011727
Attachment #8423945 -
Attachment is obsolete: true
Attachment #8424203 -
Flags: review+
Flags: needinfo?(vporof)
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 22•10 years ago
|
||
Didn't realize one change didn't get applied.
Attachment #8424203 -
Attachment is obsolete: true
Attachment #8424205 -
Flags: review+
Comment 23•10 years ago
|
||
(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?
Keywords: checkin-needed
Assignee | ||
Comment 24•10 years ago
|
||
(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.
Assignee | ||
Comment 25•10 years ago
|
||
Can you provide a try link for me please ? Thanks :) And if needed, can you also do it for bug 1011249 please ?
Flags: needinfo?(bgrinstead)
Comment 26•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=925acb579a1b
Flags: needinfo?(bgrinstead)
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 27•10 years ago
|
||
(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•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/79d74ac58388
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/79d74ac58388
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 32
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•