856756 - Extract a HardwareUtils class for tablet/television/other checking

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Attachment: 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

Comment 1

[Richard Newman [:rnewman]]

How about `isLargeFormat()` == `isTablet() || isTelevision()`?

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

Comment 2

[Kartikaya Gupta (email:[e-mail])]

(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?

Comment 3

[Richard Newman [:rnewman]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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 4

[Mark Finkle (:mfinkle) (use needinfo?)]

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?

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Attachment: Move hasPermanentMenuKey

Good call on moving this too.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e1503eecb2e4
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4d0e624c2ba

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e1503eecb2e4
https://hg.mozilla.org/mozilla-central/rev/e4d0e624c2ba