Extract a HardwareUtils class for tablet/television/other checking

RESOLVED FIXED in Firefox 23

Status

()

Firefox for Android
General
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

unspecified
Firefox 23
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Created attachment 731982 [details] [diff] [review]
Patch

We have isTablet() functions in both GeckoApp and GeckoAppShell, which seems redundant. Code that calls these functions is not consistent. I also found PackageManager.hasSystemFeature to check for television devices and I wanted a good place to put that.

Note that this patch changes the quit button visibility condition to be isTelevision() rather than !isTouchDevice because I never really liked the isTouchDevice implementation that I wrote. I discovered PackageManager.hasSystemFeature(PackageManager.FEATURE_TOUCHSCREEN) which would be perfect, except that it returns true on the Ouya. I have brought that up in the Ouya forums at [1] but for now I figure using isTelevision() is safer.

I'm also on the fence about changing isTablet() to return false for television devices. I think that makes sense, but as it stands now I'd just be changing all the places that do isTablet() checks to be doing isTablet() || isTelevision() checks so it doesn't seem to be worth it really.

[1] http://forums.ouya.tv/discussion/1136/packagemanager-hassystemfeaturefeature_touchscreen-shouldnt-return-true
Attachment #731982 - Flags: review?(mark.finkle)
How about `isLargeFormat()` == `isTablet() || isTelevision()`?

And a note from Bug 856163: it seems like these should really be static methods.
(In reply to Richard Newman [:rnewman] from comment #1)
> How about `isLargeFormat()` == `isTablet() || isTelevision()`?

I don't quite like "isLargeFormat"; to me that would imply isLargeTablet() || isTelevision(). However I'm not opposed to adding something of this sort, as long as we have a good name for the function.

> And a note from Bug 856163: it seems like these should really be static
> methods.

All the things in HardwareUtils are static. Did you mean something else?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #2)

> I don't quite like "isLargeFormat"; to me that would imply isLargeTablet()
> || isTelevision(). However I'm not opposed to adding something of this sort,
> as long as we have a good name for the function.

Yeah, I don't have a vested interest in the name. Just seems that if we have some common category popping up in code, we should name it and make it easy to test for membership. That might be !isPhone(), who knows?


> > And a note from Bug 856163: it seems like these should really be static
> > methods.
> 
> All the things in HardwareUtils are static. Did you mean something else?

Sorry, I made an assumption based on this comment in that bug:

---
> And is there a reason why this method isn't static?

It calls isTablet(), which in turn calls isLargeTablet() and isSmallTablet() -- none of which are static. A workaround would be to call GeckoAppShell.isTablet(), but that seems kind of dumb since that method is just a static wrapper for the one in GeckoApp. I think we can clean all of this up in bug 856756.
---
Comment on attachment 731982 [details] [diff] [review]
Patch

LGTM.

We can make the isLargeFormat() (or whatever) decision as we get more focus on how we'll treat televisions in general.

Also, I think hasPermanentMenuKey() might be a good candidate for HardwareUtils. Followup?
Attachment #731982 - Flags: review?(mark.finkle) → review+
Created attachment 732305 [details] [diff] [review]
Move hasPermanentMenuKey

Good call on moving this too.
Attachment #732305 - Flags: review?(mark.finkle)
Attachment #732305 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/e1503eecb2e4
https://hg.mozilla.org/mozilla-central/rev/e4d0e624c2ba
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Depends on: 999680
You need to log in before you can comment on or make changes to this bug.