Update GetDefaultScaleInternal() again to enable FxOS for devices beyond retina

RESOLVED FIXED in 1.3 Sprint 5 - 11/22

Status

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: timdream, Assigned: mchang)

Tracking

unspecified
1.3 Sprint 5 - 11/22
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor-lang=zh][mentor=timdream])

Attachments

(1 attachment, 2 obsolete attachments)

+++ This bug was initially created as a clone of Bug #845182 +++

So I made a wrong assumption about dppx: The function in bug #878029 does not return any value more than 2, but as of 2013, we already have some ddpx=3 porting target out there.

https://github.com/ResponsiveImagesCG/picture-element/wiki/Display-technologies:-beyond-Retina

It would be a really easy patch enabling people to port FxOS to these device by adopt the the |floor()| part on our Android backend here.

http://hg.mozilla.org/mozilla-central/file/e5b2e2ee91e0/content/base/src/nsContentUtils.cpp#l4868
blocking-b2g: hd+ → ---

Comment 1

5 years ago
Created attachment 783547 [details] [diff] [review]
updateDefaultScale.patch

Hi Tim,

I'm new to this and thought I'd tackle this bug. Thanks for your comment, it seems like a simple fix. I have attached a patch that's a first stab at this based on your comments. It seems like a simple enough patch. I built the B2G Desktop environment on OS X 10.8.2 and it compiled and ran. I don't have a phone to try it on though. Is there anything else I should try?

Thanks!
Whiteboard: [good first bug] → [good first bug][mentor-lang=zh][mentor=timdream]
Attachment #783547 - Flags: review?(mwu)
Mason,

Your code was left unattended because you have not set anyone for review. Please ensure there is always someone being marked review?, feedback?, needinfo? to ensure the bug don't get left out (like what I do to you now).
Flags: needinfo?(maschang85)
Assignee: nobody → maschang85

Comment 3

5 years ago
Tim, this patch makes it so that dpi between 200 and 300 are 1.5x scaled, rather than between 200 and 280. Is that ok with you? It makes things match the linked Android logic, at least, but we should change the code for the 1.5x scaling part to make things obvious.
Flags: needinfo?(timdream)
(In reply to Michael Wu [:mwu] from comment #3)
> Tim, this patch makes it so that dpi between 200 and 300 are 1.5x scaled,
> rather than between 200 and 280. Is that ok with you? It makes things match
> the linked Android logic, at least, but we should change the code for the
> 1.5x scaling part to make things obvious.

Agree. It is unclear to me if we would have a device within that dpi range (280-300) though.
Flags: needinfo?(timdream)

Comment 5

5 years ago
Comment on attachment 783547 [details] [diff] [review]
updateDefaultScale.patch

Ok. Please fix the 1.5x scaling case and I'll rubberstamp this.
Attachment #783547 - Flags: review?(mwu) → feedback+
(Assignee)

Updated

5 years ago
Assignee: maschang85 → mchang
(Assignee)

Comment 6

5 years ago
Created attachment 8335495 [details] [diff] [review]
updateDefaultScale.patch

Updated to reflect the same dpi as Android backend.
Attachment #783547 - Attachment is obsolete: true
Attachment #8335495 - Flags: review?(mwu)
Flags: needinfo?(maschang85)

Updated

5 years ago
Attachment #8335495 - Flags: review?(mwu) → review+
Comment on attachment 8335495 [details] [diff] [review]
updateDefaultScale.patch

># HG changeset patch
># Parent 597287004ff59cd132e0b74fb1a19b18b61a3e86
>
>diff -r 597287004ff5 widget/gonk/nsWindow.cpp
>--- a/widget/gonk/nsWindow.cpp	Wed Nov 20 15:16:32 2013 +0100
>+++ b/widget/gonk/nsWindow.cpp	Wed Nov 20 12:06:10 2013 -0800
>     }
>-    return 2.0; // xhdpi devices.
>+    return floor(dpi / 150.0); // xhdpi devices.
s/xhdpi devices/xhdpi devices and beyond/
(Assignee)

Comment 8

5 years ago
Created attachment 8336186 [details] [diff] [review]
updateDefaultScale.patch

Carrying over r+ from mwu. Modified comment as stated in Comment 7.

Successful try server:
https://tbpl.mozilla.org/?tree=Try&rev=e8c4085694ca
Attachment #8335495 - Attachment is obsolete: true
Attachment #8336186 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4a006106c65c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3 Sprint 5 - 11/22
You need to log in before you can comment on or make changes to this bug.