898044 - GeckoApp.onSaveInstanceState has an unnecessary null check for the bundle

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

http://hg.mozilla.org/mozilla-central/file/8196c86355bc/mobile/android/base/GeckoApp.java#l491

I believe the null check is unnecessary. However, if the Bundle can be null, then creating a new Bundle is pointless because the new bundle will never be saved. outState is not actually an out-variable like a C pointer; it is a Java reference and the calling code will never know about the created Bundle.

Code was originally added in http://hg.mozilla.org/mozilla-central/rev/a9c28fe6b23f and doesn't look like it was added in response to a specific NPE so I'd like to just remove it.

Comment 1

[Ming]

Hi Kartikaya,

This is Ming and I am new to Firefox Android. I have looked at this bug and it seems to be quite easy. I am wondering whether I can take it? I think this bug would be a great start for me to begin learning the code base.

I am following the Fennec building wiki right now to get the source code. I am wondering whether I need to have an android phone/tablet to work with it? Any suggestions would be very helpful.

Thanks.

Comment 2

[Ming]

Hi Kartikaya,

This is Ming and I am new to Firefox Android. I have looked at this bug and it seems to be quite easy. I am wondering whether I can take it? I think this bug would be a great start for me to begin learning the code base.

I am following the Fennec building wiki right now to get the source code. I am wondering whether I need to have an android phone/tablet to work with it? Any suggestions would be very helpful.

Thanks.

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #0)
> http://hg.mozilla.org/mozilla-central/file/8196c86355bc/mobile/android/base/
> GeckoApp.java#l491
> 
> I believe the null check is unnecessary. However, if the Bundle can be null,
> then creating a new Bundle is pointless because the new bundle will never be
> saved. outState is not actually an out-variable like a C pointer; it is a
> Java reference and the calling code will never know about the created Bundle.
> 
> Code was originally added in
> http://hg.mozilla.org/mozilla-central/rev/a9c28fe6b23f and doesn't look like
> it was added in response to a specific NPE so I'd like to just remove it.

Comment 3

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

Hi Ming,

The first thing to do, which you mentioned, is to follow the steps on the wiki page at https://wiki.mozilla.org/Mobile/Fennec/Android#Building_Fennec to get your fennec build environment set up. In general when working on Fennec it is useful to have an actual android device, but this specific bug is simple enough that you should be able to just use the emulator.

There are instructions on how to set up the emulator so that Fennec runs on it at https://wiki.mozilla.org/Mobile/Fennec/Android/Emulator

If you have any trouble you can always ask questions in #mobile on IRC or post here.

Comment 4

[Ming]

Attachment: name.patch: delete the null check in GeckoApp.onSaveInstanceState

Comment 5

[Ming]

Hi Kartikaya,

I have generate the patch with "hg qnew name.patch" and uploaded here. Since there are too much info on the wiki page, I cannot figure out what to do next for this bug. I am wondering what test should i run for this change? Also, I currently have a nexus 7 but it is not rooted. The wiki page mentioned that it is troublesome to run test with an unrooted device. Is there other ways to run the required test?

Also, some suggestion on how to begin learning the code base would be very helpful :)

Thanks,
Ming


(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> Hi Ming,
> 
> The first thing to do, which you mentioned, is to follow the steps on the
> wiki page at https://wiki.mozilla.org/Mobile/Fennec/Android#Building_Fennec
> to get your fennec build environment set up. In general when working on
> Fennec it is useful to have an actual android device, but this specific bug
> is simple enough that you should be able to just use the emulator.
> 
> There are instructions on how to set up the emulator so that Fennec runs on
> it at https://wiki.mozilla.org/Mobile/Fennec/Android/Emulator
> 
> If you have any trouble you can always ask questions in #mobile on IRC or
> post here.

Comment 6

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

(In reply to Ming from comment #5)
> Hi Kartikaya,
> 
> I have generate the patch with "hg qnew name.patch" and uploaded here.

That looks good, thanks.

> Since
> there are too much info on the wiki page, I cannot figure out what to do
> next for this bug.

When you upload a patch that is ready for review, you should set the review flags on it. In the attachment details there is a "review" setting - set the value of that to "?" and put my name in the reviewer field (you can just type in ":kats" in the field and it should autocomplete it).

> I am wondering what test should i run for this change?
> Also, I currently have a nexus 7 but it is not rooted. The wiki page
> mentioned that it is troublesome to run test with an unrooted device. Is
> there other ways to run the required test?

Since this is a pretty simple change, just exercising the codepath on your device should be enough. Make a build with this change applied, put it on your N7 device, start Fennec, and then put fennec in the background using the home button. This should run through the onSaveInstanceState code and make sure that no NullPointerException is thrown. As long as you do that I'm satisfied that it shouldn't break anything.

> Also, some suggestion on how to begin learning the code base would be very
> helpful :)

Well the codebase is pretty large so I would say to pick concrete goals that you would like to do. If there are other bugs that you are interested in fixing then that's a great way to continue learning the codebase. If you want to implement a new feature or customization that's also a good way.

Comment 7

[Ming]

Hi Kartikaya,

Thanks for the help. I would prefer helping to fix some other simple bugs first in order to learn the codebase. Since I haven't done any android app development before, I guess the curve maybe a little bit difficult for me. 

I am wondering whether there is some document about the overall architecture of the codebase? I think maybe it's better for me to learn about the general work flow of this app before look into more details.

Thanks,
Ming

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)
> (In reply to Ming from comment #5)
> > Hi Kartikaya,
> > 
> > I have generate the patch with "hg qnew name.patch" and uploaded here.
> 
> That looks good, thanks.
> 
> > Since
> > there are too much info on the wiki page, I cannot figure out what to do
> > next for this bug.
> 
> When you upload a patch that is ready for review, you should set the review
> flags on it. In the attachment details there is a "review" setting - set the
> value of that to "?" and put my name in the reviewer field (you can just
> type in ":kats" in the field and it should autocomplete it).
> 
> > I am wondering what test should i run for this change?
> > Also, I currently have a nexus 7 but it is not rooted. The wiki page
> > mentioned that it is troublesome to run test with an unrooted device. Is
> > there other ways to run the required test?
> 
> Since this is a pretty simple change, just exercising the codepath on your
> device should be enough. Make a build with this change applied, put it on
> your N7 device, start Fennec, and then put fennec in the background using
> the home button. This should run through the onSaveInstanceState code and
> make sure that no NullPointerException is thrown. As long as you do that I'm
> satisfied that it shouldn't break anything.
> 
> > Also, some suggestion on how to begin learning the code base would be very
> > helpful :)
> 
> Well the codebase is pretty large so I would say to pick concrete goals that
> you would like to do. If there are other bugs that you are interested in
> fixing then that's a great way to continue learning the codebase. If you
> want to implement a new feature or customization that's also a good way.

Comment 8

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

Comment on attachment 782222 [details] [diff] [review]
name.patch: delete the null check in GeckoApp.onSaveInstanceState

Review of attachment 782222 [details] [diff] [review]:
-----------------------------------------------------------------

The patch itself is fine, but the commit message should indicate what the patch is doing rather than re-stating the title of the bug. In this case the commit message should be something like "Remove unnecessary null check from GeckoApp.onSaveInstanceState".

Please upload a new patch with the updated commit message. You can carry forward this r+ but just setting the review flag to "+" on your new patch, and add the "checkin-needed" keyword to the bug to get it committed by one of the sheriffs.

Comment 9

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

(In reply to Ming from comment #7)
> Hi Kartikaya,
> 
> Thanks for the help. I would prefer helping to fix some other simple bugs
> first in order to learn the codebase. Since I haven't done any android app
> development before, I guess the curve maybe a little bit difficult for me. 
> 

Well you're doing great so far! You can find other simple bugs by looking around on Bugs Ahoy - http://www.joshmatthews.net/bugsahoy/. Just find a bug that looks simple and interests you, and comment on the bug much like you did on this one to get started!

> I am wondering whether there is some document about the overall architecture
> of the codebase? I think maybe it's better for me to learn about the general
> work flow of this app before look into more details.

There isn't a whole lot of documentation, unfortunately. There's some scraps at https://wiki.mozilla.org/Mobile/Fennec/Android#Hacking which might be useful but keep in mind that some of it might be out of date.

Comment 10

[Ming]

Attachment: Remove the unnecessary null check in GeckoApp.onSaveInstanceState

Comment 11

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

Sorry, I should have been more clear - you should also keep the bug number in the commit message. But that's ok, I added the bug number and landed the patch for you:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d9b658f6840

Thanks!

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7d9b658f6840