Closed Bug 612978 Opened 9 years ago Closed 9 years ago

GetDPI on Maemo should use the true DPI when known

Categories

(Core :: Graphics, defect)

All
Maemo
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0b3+ ---

People

(Reporter: mbrubeck, Assigned: mbrubeck)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
+++ This bug was initially created as a clone of Bug #590777 +++

In bug 530931, we removed code to return the true display density on Maemo, because densities of >200dpi were bad for web compatibility in Gecko 1.9.2.

With the new DPI system (bug 537890), this is not a problem any more.  We can make GetDPI return an accurate value without affecting standard CSS units.  This will allow Fennec to use the new "mozmm" physical unit in its chrome styles (bug 590817) and adapt various settings to the true display DPI (bug 590777).

This patch restores code removed by bug 530931.  Fortunately the new DPI system makes this much simpler than it was.
Attachment #491284 - Flags: review?(roc)
Blocks: 590777
No longer blocks: 575403
+        if (NS_FAILED(rv))
+            NS_ASSERTION(infoService, "Could not find a system info service");

Why check NS_FAILED(rv)?

+            if (deviceType.EqualsLiteral("Nokia N900"))
+                sDPI = 265.0f;
+            else if (deviceType.EqualsLiteral("Nokia N8xx"))
+                sDPI = 225.0f;

{} around the bodies

+        if (!sDPI) {
+            // Fall back to something sane.
+            return 96.0f;
+        }

Can't you set sDPI to 96.0f here? Or does getting the property fail sometimes but succeed later?
Comment on attachment 491284 [details] [diff] [review]
patch


>+        // X on Maemo does not report true DPI: https://bugs.maemo.org/show_bug.cgi?id=4825
>+        nsresult rv;
>+        nsCOMPtr<nsIPropertyBag2> infoService = do_GetService("@mozilla.org/system-info;1", &rv);
>+        if (NS_FAILED(rv))
>+            NS_ASSERTION(infoService, "Could not find a system info service");
>+

No need to use rv here or have that NS_FAILED.  Just assert infoService.

>+        nsCString deviceType;
>+        rv = infoService->GetPropertyAsACString(NS_LITERAL_STRING("device"), deviceType);
>+        if (NS_SUCCEEDED(rv)) {

You probably don't even need this test.  And instead just directly start doing your EqualsLiteral.  It is extremely unlikely that rv will ever be failure.

>+            if (deviceType.EqualsLiteral("Nokia N900"))
>+                sDPI = 265.0f;
>+            else if (deviceType.EqualsLiteral("Nokia N8xx"))
>+                sDPI = 225.0f;
>+        }

without the check for rv above, this just becomes |else sDPI = 96.0f|

toss up one more patch, and I think we are good here!
Attachment #491284 - Flags: review?(roc) → review-
Attached patch patch v2Splinter Review
This addresses review comments, and also moves the static variable (and the #ifdef) inside the function body.

(In reply to comment #1)
> +        if (NS_FAILED(rv))
> +            NS_ASSERTION(infoService, "Could not find a system info service");
> 
> Why check NS_FAILED(rv)?

Removed.

> {} around the bodies

Fixed.

> +        if (!sDPI) {
> +            // Fall back to something sane.
> +            return 96.0f;
> +        }
> 
> Can't you set sDPI to 96.0f here?

Yes - fixed.

(In reply to comment #2)
> >+        rv = infoService->GetPropertyAsACString(NS_LITERAL_STRING("device"), deviceType);
> >+        if (NS_SUCCEEDED(rv)) {
> 
> You probably don't even need this test.  And instead just directly start doing
> your EqualsLiteral.  It is extremely unlikely that rv will ever be failure.
> 
> without the check for rv above, this just becomes |else sDPI = 96.0f|

Fixed.
Attachment #491284 - Attachment is obsolete: true
Attachment #491539 - Flags: review?(roc)
Attachment #491539 - Flags: review?(doug.turner)
Comment on attachment 491539 [details] [diff] [review]
patch v2

note: we don't need to test for do_GetService because we know that it is, and always will be, implemented on at least MAEMO.

This looks fine.  The only suggestion I would make is to add a NS_WARNING() in the default case.  This would be a nice reminder, when bringing up another nokia device, that we have to add a new DPI value.
Attachment #491539 - Flags: review?(doug.turner) → review+
(In reply to comment #4)
> This looks fine.  The only suggestion I would make is to add a NS_WARNING() in
> the default case. 

Done, and pushed: http://hg.mozilla.org/mozilla-central/rev/d18bdf583d87
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.