Closed
Bug 999680
Opened 10 years ago
Closed 10 years ago
HardwareUtils.isTelevision is a footgun
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 32
People
(Reporter: nalexander, Assigned: alexandru.deaconu)
References
Details
(Whiteboard: [mentor=nalexander][lang=java][good first bug])
Attachments
(1 file, 2 obsolete files)
1.35 KB,
patch
|
Details | Diff | Splinter Review |
HardwareUtils.isTelevision has a hidden API requirement: it only works on v14 and later. Luckily the only caller has the check in place: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#2170 This should really be part of isTelevision. Another win for Eclipse's context aware lint!
Reporter | ||
Comment 1•10 years ago
|
||
This is a good first bug. You'll need a working build, and preferably a device or two to test on. To correct this, fold an API check into isTelevision, just like at http://hg.mozilla.org/mozilla-central/file/1ab07aa4d004/mobile/android/base/util/HardwareUtils.java#l70 The single caller should stay as it is.
Whiteboard: [mentor=nalexander][lang=java][good first bug]
Assignee | ||
Comment 2•10 years ago
|
||
Hi I have successfully build the Fennec project, I'm using Eclipse as an IDE and I would like to start working on this issue. Thanks Alex.
Assignee | ||
Comment 3•10 years ago
|
||
The PackageManager.FEATURE_TELEVISION flag used in HardwareUtils.isTelevision was added in API 16. We should probably check in the HardwareUtils.isTelevision that the Build.VERSION.SDK_INT is grater than 16
Flags: needinfo?(nalexand)
Reporter | ||
Comment 5•10 years ago
|
||
(In reply to Alex Deaconu from comment #2) > Hi I have successfully build the Fennec project, I'm using Eclipse as an IDE > and I would like to start working on this issue. Hey Alex, that's awesome! I wrote most of the Fennec/Eclipse IDE integration, so I'd love to know how it's working out for you. It's great that you have Fennec building (and running on your device, I assume). For this ticket, your next step is to look at https://developer.mozilla.org/en-US/docs/Introduction -- you're at step 3, and you've correctly identified what needs to be done. So implement that locally, and then you're ready for step 4, attaching a patch to this ticket. There won't be much testing involved, here, since most of us don't have Android TVs. We'll check that Eclipse's lint warning goes away, though. Thanks for pitching in!
Flags: needinfo?(nalexander)
Assignee | ||
Comment 6•10 years ago
|
||
The 'PackageManager.FEATURE_TELEVISION' flag requires API 16 or greater. Added checking in 'HardwareUtils.isTelevision' that API is >= 16
Comment 7•10 years ago
|
||
Comment on attachment 8413274 [details]
checking SDK version >=16 before using PackageManager.FEATURE_TELEVISION
Alex, it looks like a bunch of extraneous stuff has made it into this patch file -- take a look?
Regarding your real change itself:
public static boolean isTelevision() {
if (sIsTelevision == null) {
- sIsTelevision = sContext.getPackageManager().hasSystemFeature(PackageManager.FEATURE_TELEVISION);
+ sIsTelevision = Build.VERSION.SDK_INT >= 16 &&
+ sContext.getPackageManager().hasSystemFeature(PackageManager.FEATURE_TELEVISION);
}
return sIsTelevision;
}
I'd prefer this as a short-cut at the start of the method:
public static boolean isTelevision() {
if (Build.VERSION.SDK_INT < Build.VERSION_CODES.JELLY_BEAN) {
// Too early to support this system feature.
return false;
}
if (sIsTelevision == null) {
....
Attachment #8413274 -
Flags: feedback+
Assignee | ||
Comment 8•10 years ago
|
||
Hi Richard, I've added by mistake in the patch changes that were not related to this bug. Sorry about that I was quite desparate to remove them. Thanks for the feedback I'll implement the new changes. Is there any way of deleting the old patch or updating it or do I have to attach a new patch?
Reporter | ||
Comment 9•10 years ago
|
||
> Is there any way of deleting the old patch or updating it or do I have to
> attach a new patch?
You have to attach a new patch, but you can mark the old as obsolete.
Assignee | ||
Updated•10 years ago
|
Attachment #8413274 -
Attachment is obsolete: true
Attachment #8413274 -
Attachment is patch: false
Assignee | ||
Comment 10•10 years ago
|
||
Flag 'PackageManager.FEATURE_TELEVISION' was added in API 16. Checking at the start of 'HardwareUtils.isTelevision' that API level is >=16
Assignee | ||
Comment 11•10 years ago
|
||
Comment on attachment 8413284 [details] [diff] [review] Checking API level >= 16 inside 'HardwareUtils.isTelevision' Review of attachment 8413284 [details] [diff] [review]: ----------------------------------------------------------------- Hey Nick, 1. Regarding Eclipse. It has been really helpful and I haven't encountered issues so far. Currently I am using Eclipse version which ships with the adt-bundle-linux-x86_64-20131030 and the last ADT plugin version. It's true, I'm new to Fennec and I've been using Ecipse for just 1 week, but it's eased the work, especially while searching for classes and references. 2. Regarding bug 999680 While working on this issue, I've seen that the lint was not showing any errors and I had to add the lint view to the Java Perspective and press the refresh button within in order to update the lint warnings. Currently there is still a lint warning on the line sIsTelevision = sContext.getPackageManager().hasSystemFeature(PackageManager.FEATURE_TELEVISION); which dissapears after cleaning the project.
Assignee | ||
Comment 12•10 years ago
|
||
Comment on attachment 8413284 [details] [diff] [review] Checking API level >= 16 inside 'HardwareUtils.isTelevision' Review of attachment 8413284 [details] [diff] [review]: ----------------------------------------------------------------- Hey Nick, 1. Regarding Eclipse. It has been really helpful and I haven't encountered issues so far. Currently I am using Eclipse version which ships with the adt-bundle-linux-x86_64-20131030 and the last ADT plugin version. It's true, I'm new to Fennec and I've been using Ecipse for just 1 week, but it's eased the work, especially while searching for classes and references. 2. Regarding bug 999680 While working on this issue, I've seen that the lint was not showing any errors and I had to add the lint view to the Java Perspective and press the refresh button within in order to update the lint warnings. Currently there is still a lint warning on the line sIsTelevision = sContext.getPackageManager().hasSystemFeature(PackageManager.FEATURE_TELEVISION); which dissapears after cleaning the project.
Attachment #8413284 -
Flags: review?(nalexander)
Reporter | ||
Comment 13•10 years ago
|
||
Comment on attachment 8413284 [details] [diff] [review] Checking API level >= 16 inside 'HardwareUtils.isTelevision' Review of attachment 8413284 [details] [diff] [review]: ----------------------------------------------------------------- Alex: looking good! I've asked for some tiny changes. Then post a new patch with the commit comment: Bug 999680 - Check API level >= 16 in HardwareUtils.isTelevision. r=nalexander Finally, add the keyword checkin-needed to the ticket. That'll tell a sheriff to land your change for you. I think this is your first bug; congratulations! I have a bunch of bugs around the Synced Tabs panel that I'm thinking of mentoring; are you interested in helping me with some of them? See the dependent tickets of Bug 1002573. ::: mobile/android/base/util/HardwareUtils.java @@ +73,5 @@ > return sIsSmallTablet; > } > > public static boolean isTelevision() { > + if (Build.VERSION.SDK_INT < Build.VERSION_CODES.JELLY_BEAN) { This file uses numeric codes throughout, so let's use < 16 to be consistent, and say something like "Android versions before Jelly Bean don't support this system feature." in the comment below. @@ +77,5 @@ > + if (Build.VERSION.SDK_INT < Build.VERSION_CODES.JELLY_BEAN) { > + // Too early to support this system feature. > + return false; > + } > + nit: delete trailing whitespace.
Attachment #8413284 -
Flags: review?(nalexander) → review+
Reporter | ||
Comment 14•10 years ago
|
||
> 1. Regarding Eclipse.
>
> It has been really helpful and I haven't encountered issues so far.
> Currently I am using Eclipse version which ships with the
> adt-bundle-linux-x86_64-20131030 and the last ADT plugin version. It's true,
> I'm new to Fennec and I've been using Ecipse for just 1 week, but it's eased
> the work, especially while searching for classes and references.
I'm so glad to hear this. Thanks for the feedback!
Reporter | ||
Comment 15•10 years ago
|
||
> I think this is your first bug; congratulations! I have a bunch of bugs > around the Synced Tabs panel that I'm thinking of mentoring; are you > interested in helping me with some of them? See the dependent tickets of > Bug 1002573. We also issue a digital badge to contributors (see https://badges.mozilla.org/en-US/badges/badge/Friends-of-the-Mobile-Team); if you're interested, let me know! margaret: can you add Alex to our weekly notes? Thanks!
Flags: needinfo?(margaret.leibovic)
Assignee | ||
Comment 16•10 years ago
|
||
- using numeric code to check API level - updated comment - removed white space after first 'if' condition
Assignee | ||
Updated•10 years ago
|
Attachment #8413284 -
Attachment is obsolete: true
Assignee | ||
Comment 17•10 years ago
|
||
Nick : Thanks, I'd like to help on the Synced Tabs panel. I'll look into the code to get more familiar. We can chat more about it on irc. My nickname on irc is: adeac
Assignee | ||
Comment 18•10 years ago
|
||
keyword checkin-needed
Reporter | ||
Comment 19•10 years ago
|
||
(In reply to Alex Deaconu from comment #18) > keyword checkin-needed This is a special bugzilla flag; I've set it for you. Updated patch looks great!
Keywords: checkin-needed
Comment 20•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/221dd25fe11c
Assignee: nobody → alexandru.deaconu
Keywords: checkin-needed
Whiteboard: [mentor=nalexander][lang=java][good first bug] → [mentor=nalexander][lang=java][good first bug][fixed-in-fx-team]
Comment 21•10 years ago
|
||
(In reply to Nick Alexander :nalexander from comment #15) > > I think this is your first bug; congratulations! I have a bunch of bugs > > around the Synced Tabs panel that I'm thinking of mentoring; are you > > interested in helping me with some of them? See the dependent tickets of > > Bug 1002573. > > We also issue a digital badge to contributors (see > https://badges.mozilla.org/en-US/badges/badge/Friends-of-the-Mobile-Team); > if you're interested, let me know! > > margaret: can you add Alex to our weekly notes? Thanks! Definitely! Thanks so much for your contribution!
Flags: needinfo?(margaret.leibovic)
Comment 22•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/221dd25fe11c
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=nalexander][lang=java][good first bug][fixed-in-fx-team] → [mentor=nalexander][lang=java][good first bug]
Target Milestone: --- → Firefox 32
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•