Closed Bug 1012829 Opened 6 years ago Closed 6 years ago

HDPI support for developer toolbar

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 32

People

(Reporter: ntim, Assigned: ntim)

References

Details

Attachments

(1 file)

No description provided.
Blocks: 837188
Brian, is this worth fixing ? We're gonna get rid of the developer toolbar soon according to what you said.
Flags: needinfo?(bgrinstead)
I don't think we necessarily need to worry about it.  The current icons are simple enough that they will scale alright to 2x, so we should be fine in the meantime.
Flags: needinfo?(bgrinstead)
Assignee: nobody → ntim007
Status: NEW → ASSIGNED
Attached patch Patch v1Splinter Review
Tested, no regressions/issues appearing.
Attachment #8432725 - Flags: review?(bgrinstead)
Comment on attachment 8432725 [details] [diff] [review]
Patch v1

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

The UI looks good in 2x, but I'm getting an error applying the patch to the jar.mn files

::: browser/themes/shared/devtools/commandline.inc.css
@@ +157,5 @@
>  
> +.gclitoolbar-input-node::before {
> +  content: "";
> +  display: inline-block;
> +  -moz-box-ordinal-group: 0;

Why does this property need to be set here?
Attachment #8432725 - Flags: review?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #5)
> Comment on attachment 8432725 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 8432725 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The UI looks good in 2x, but I'm getting an error applying the patch to the
> jar.mn files
Yeah, there are 2 HDPI related bugs awaiting to land that changed the jar.mn files (bug 1017890 and bug 1018809)

> ::: browser/themes/shared/devtools/commandline.inc.css
> @@ +157,5 @@
> >  
> > +.gclitoolbar-input-node::before {
> > +  content: "";
> > +  display: inline-block;
> > +  -moz-box-ordinal-group: 0;
> 
> Why does this property need to be set here?
I don't think it's needed, but it's nice to have in case a new children gets added inside the textbox. I'll remove it.
Comment on attachment 8432725 [details] [diff] [review]
Patch v1

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

Joe, from a usage standpoint this patch is good (I've checked in 2x and things look nice and sharp).  But it'd be good if you could review the changes in commandline.inc.css
Attachment #8432725 - Flags: review?(jwalker)
Comment on attachment 8432725 [details] [diff] [review]
Patch v1

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

Code looks good to me. I'd like to try it out. But that will have to wait until tomorrow
Comment on attachment 8432725 [details] [diff] [review]
Patch v1

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

LGTM. Thanks.
Attachment #8432725 - Flags: review?(jwalker) → review+
Try push in comment 4.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/52f6d0a67991
Status: ASSIGNED → RESOLVED
Closed: 6 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.