1316005 - [findbugs] [UMAC] Uncallable method defined in anonymous class

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Uncallable method org.mozilla.gecko.MediaPlayerManager$1.onRouteSelected(MediaRouter, int, MediaRouter$RouteInfo) defined in anonymous class

Uncallable method org.mozilla.gecko.MediaPlayerManager$1.onRouteUnselected(MediaRouter, int, MediaRouter$RouteInfo) defined in anonymous class

"This anonymous class defined a method that is not directly invoked and does not override a method in a superclass. Since methods in other classes cannot directly invoke methods declared in an anonymous class, it seems that this method is uncallable. The method might simply be dead code, but it is also possible that the method is intended to override a method declared in a superclass, and due to an typo or other error the method does not, in fact, override the method it is intended to."

Comment 2

[Owais Kazi]

Hi Sebastian!
I would like to work on this bug. Can you explain me in brief what i need to do given that this is my first bug.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

I just answered the same question in bug 1316023 - Let's start with this one. :)

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Anyways, for whoever picks this one up, ..

.. to start, set up a build environment - you can see the instructions here:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_for_Android_build

.. then, you'll need to upload a patch - see:
http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html

If you need any help, you can reply to this bug, or feel free to message me on IRC - my nick is "sebastian" and you can find me and other helpful folks in #mobile. If you need IRC setup instructions, see https://wiki.mozilla.org/IRC

Comment 5

[Tomislav Jurin]

I will take this one

Comment 6

[Tomislav Jurin]

I will take this one.

Comment 7

[Tomislav Jurin]

Attachment: Bug 1316005 - fixed unused methods, made methods use actual signature and added @Override;removed additional line

Review commit: https://reviewboard.mozilla.org/r/94220/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/94220/

Comment 8

[Tomislav Jurin]

I have made patch for the bug, what is the procedure now ?

Thanks.

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8812494 [details]
Bug 1316005 - fixed unused methods, made methods use actual signature and added @Override;removed additional line

https://reviewboard.mozilla.org/r/94220/#review94566

::: mobile/android/base/java/org/mozilla/gecko/MediaPlayerManager.java
(Diff revision 1)
> -            @SuppressWarnings("unused")
> -            public void onRouteSelected(MediaRouter router, int type, MediaRouter.RouteInfo route) {
> -                updatePresentation();
> -            }
> -
> -            // These methods aren't used by the support version Media Router
> -            @SuppressWarnings("unused")
> -            public void onRouteUnselected(MediaRouter router, int type, RouteInfo route) {
> -                updatePresentation();
> -            }

Looking at the current version of MediaRouter.Callback I think it would make sense to move to the actual signature (without 'int type'):

https://developer.android.com/reference/android/support/v7/media/MediaRouter.Callback.html#onRouteSelected(android.support.v7.media.MediaRouter,%20android.support.v7.media.MediaRouter.RouteInfo)

After that we can remove the SuppressWarnings annotation and add the Override annotation.

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to svezauzeto12 from comment #8)
> I have made patch for the bug, what is the procedure now ?

I just reviewed the current version of the patch. After it has passed review I'll push the patch to the test servers and if it passes all tests then we can land the patch (by adding the 'checkin-needed' keyword to the bug).

Comment 11

[Tomislav Jurin]

I will try to do what is written above and then commit.

Comment 12

[Tomislav Jurin]

Comment on attachment 8812494 [details]
Bug 1316005 - fixed unused methods, made methods use actual signature and added @Override;removed additional line

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94220/diff/1-2/

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8812494 [details]
Bug 1316005 - fixed unused methods, made methods use actual signature and added @Override;removed additional line

https://reviewboard.mozilla.org/r/94220/#review95490

LGTM.

::: mobile/android/base/java/org/mozilla/gecko/MediaPlayerManager.java:199
(Diff revision 2)
> -            @SuppressWarnings("unused")
> -            public void onRouteSelected(MediaRouter router, int type, MediaRouter.RouteInfo route) {
> +            @Override
> +            public void onRouteSelected(MediaRouter router, MediaRouter.RouteInfo route) {
>                  updatePresentation();
>              }
>  
> -            // These methods aren't used by the support version Media Router
> +

nit: Remove this additional empty line.

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

I pushed the patch to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=27a11491fe80

Comment 15

[Tomislav Jurin]

To remove this line I should just make new commit right ? Or should I make new commit and merge it with the previous one for this issue ? 

Thanks for information

Comment 16

[Tomislav Jurin]

I have removed additional line and squashed it with last commit, so you should have all changes in this commit now.

When you are happy with how this is done I would appreciate if you could suggest new bug for me to work on ( I would prefer something that maybe has some actually coding, but if it is too early for that I will accept anything )

Comment 17

[Tomislav Jurin]

Comment on attachment 8812494 [details]
Bug 1316005 - fixed unused methods, made methods use actual signature and added @Override;removed additional line

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94220/diff/2-3/

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e6459d80bf245d1f5e2f46bda884878548d58126
Bug 1316005 - fixed unused methods, made methods use actual signature and added @Override;  r=sebastian

Comment 19

[Sebastian Kaspari (:sebastian; :pocmo)]

Thank you for the patch!

(In reply to Tomislav Jurin from comment #16)
> When you are happy with how this is done I would appreciate if you could
> suggest new bug for me to work on ( I would prefer something that maybe has
> some actually coding, but if it is too early for that I will accept anything

Good question. We are running a bit low on mentor bugs currently. But looking through the bugs I filed recently. Maybe bug 1312686? It's still simple but you can actually change something that is visible to the user. :)

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e6459d80bf24