Closed Bug 999680 Opened 10 years ago Closed 10 years ago

HardwareUtils.isTelevision is a footgun

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

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)

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!
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]
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.
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)
Correcting needinfo
Flags: needinfo?(nalexand) → needinfo?(nalexander)
(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)
The 'PackageManager.FEATURE_TELEVISION' flag requires API 16 or greater. Added checking in 'HardwareUtils.isTelevision' that API is >= 16
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+
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?
> 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.
Attachment #8413274 - Attachment is obsolete: true
Attachment #8413274 - Attachment is patch: false
Flag 'PackageManager.FEATURE_TELEVISION' was added in API 16. Checking at the start of 'HardwareUtils.isTelevision' that API level is >=16
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.
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)
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+
> 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!
> 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)
- using numeric code to check API level
- updated comment
- removed white space after first 'if' condition
Attachment #8413284 - Attachment is obsolete: true
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
keyword checkin-needed
(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
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]
(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)
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
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.