1279301 - Move notification classes to a separate 'notification' package

Status: RESOLVED FIXED

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

Description

[Jonathan Almeida [:jonalmeida]]

We should put all our notification classes into one package to clean it up.

See comments in bug 1075476 comment 50.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

@jonalmeida: Is this straight-forward? Maybe a "good first bug" you could mentor?

Comment 2

[Jonathan Almeida [:jonalmeida]]

Sure I can.

To extract what mcomella was saying in the related bug: we have a lot of Notification* classes in the mobile/android/base directory[1] and it would be nice if we could move this to a separate package to organise it a bit more.

A simple way to do this would be to setup your environment first so you can build with IntelliJ/Android Studio[2]. When that's working, you can simply move the Notification classes from `org.mozilla.gecko` to `org.mozilla.gecko.notifications` using the IDE's project explorer. IntelliJ/AS comes in handy by refactoring all references to those classes to correctly point to the new package namespace.

Alternatively, if using IntelliJ/AS turns into a hassle, you can stick with building using mach and then manually changing each reference to the notification classes. This might be a bit tedious so I highly recommend the first approach instead.

The last step required, is to edit the m/a/b/moz.build file[3] to change the directory of the Notification classes to be under `notifications/` similar to how it is for the `WhatsNewReceiver.java`[4]. This is essential for allowing the mach build system to continue working.

After this, go ahead and try cleaning your build with `mach clobber` and trying a fresh build from scratch with `mach build`. If it builds successfully, that should be it!

You can post your patch here after that and then I can assign the bug to you.

If you get stuck with anything, you can reply to this bug or message me on IRC in the #mobile channel - my nick is 'jonalmeida'. If you need IRC setup instructions, see https://wiki.mozilla.org/IRC

Thanks! :)

[1]: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko
[2]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_for_Android_build#Developing_Firefox_for_Android_in_Android_Studio_or_IDEA_IntelliJ
[3]: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/moz.build?q=mobile%2Fandroid%2Fbase%2Fmoz.build&redirect_type=direct#484
[4]: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/moz.build?q=mobile%2Fandroid%2Fbase%2Fmoz.build&redirect_type=direct#488

Comment 3

[Jonathan Almeida [:jonalmeida]]

Just a small clarification to the previous comment that broderick mentioned on irc: the classes to be moved are the ones that begin with 'Notification' and include `AppNotificationClient` and `ServiceNotificationClient`.

Comment 4

[Broderick Stadden]

@jonalmeida: I sent this to you on IRC but it was late.  In case I'm not on IRC, I'll add it here.

GeckoAppShell(org.mozilla.gecko) creates a static NotificationClient which NotificationHelper(org.mozilla.gecko.notifications) uses but no longer can access it because NotificationHelper is moved to the notifications package. The specific error is “Error:(306, 22) error: notificationClient is not public in GeckoAppShell; cannot be accessed from outside package”  How would you recommend going about that?

Comment 5

[Horatiu Lazu]

Hi, I would like to try out fixing this bug. Can I try?

Comment 6

[Jonathan Almeida [:jonalmeida]]

(In reply to Broderick Stadden from comment #4)
> @jonalmeida: I sent this to you on IRC but it was late.  In case I'm not on
> IRC, I'll add it here.
> 
> GeckoAppShell(org.mozilla.gecko) creates a static NotificationClient which
> NotificationHelper(org.mozilla.gecko.notifications) uses but no longer can
> access it because NotificationHelper is moved to the notifications package.
> The specific error is “Error:(306, 22) error: notificationClient is not
> public in GeckoAppShell; cannot be accessed from outside package”  How would
> you recommend going about that?

Sorry I missed your message earlier. Consider adding a public getter for the `SensorEventListener` in GeckoAppShell[1] for accessing the static `NotificationClient` instance. Let me know if you run into other issues as well.


(In reply to Horatiu from comment #5)
> Hi, I would like to try out fixing this bug. Can I try?

Hi Horatiu, welcome! It looks like Broderick is currently working on this issue right now. You could take a peek at other bugs with the tag 'good first bug' as well, or maybe check back later to see if this bug still requires work?

[1]: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoAppShell.java?q=GeckoAppShell.java&redirect_type=direct#664

Comment 7

[Horatiu Lazu]

Awesome, thanks! I'll look at other bugs.

Comment 8

[Jonathan Almeida [:jonalmeida]]

Clearing NI for Horatiu.

Comment 9

[Jonathan Almeida [:jonalmeida]]

(In reply to Jonathan Almeida (:jonalmeida) from comment #6)
> 
> Sorry I missed your message earlier. Consider adding a public getter for the
> `SensorEventListener` in GeckoAppShell[1] for accessing the static
> `NotificationClient` instance. Let me know if you run into other issues as
> well.


I made a mistake in my comment that probably would cause some confusion. I meant to say that you should consider creating a getter function similar to how one was created for the SensorEventListener. Thanks broderick!

Comment 10

[Broderick Stadden]

Attachment: Bug 1279301 fix - Moved notification classes to notfication folder;

Moved the 6 notification classes in org.mozilla.gecko to separate
org.mozilla.gecko.notification.

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

Comment 11

[Broderick Stadden]

Attachment: Bug 1279301 Moved notification classes to notification folder, refactored;

Added getter in GeckoAppShell and changed NotificationHelper to
use them.

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

Comment 12

[Broderick Stadden]

Hey Jon, in the second one, my gradle was updated so mercurial picked it up and I removed it from the commit.  If that causes any issues, I can redo the review push with clean commits.

Comment 13

[Jonathan Almeida [:jonalmeida]]

https://reviewboard.mozilla.org/r/66666/#review63382

Hey Broderick, this is a good first patch! There are a few things that can be changed though before we have this checked in:

- Try rebasing your changes to the latest fx-team branch - I'm having trouble applying your patch to the latest version of fx-team.
- There are a few imports that were added which don't seem to be used. I've added comments below to all the places that I've come across.
- Regarding your previous comment, it would be better if you could make another review without the build.gradle being removed.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
(Diff revision 1)
>  import java.io.ByteArrayOutputStream;
>  import java.io.File;
>  import java.io.FileNotFoundException;
>  import java.io.IOException;
>  import java.lang.reflect.Method;
> -import java.net.MalformedURLException;

Were these imports removed by mistake? I'm having trouble importing this patch locally. It looks like hg rejects the patch because of this hunk of the patch.

After you've updated your patch, try pulling the latest changes with `hg pull fx-team` and then rebase your patch with `hg rebase -r <rev> -d fx-team`. If it fails, go ahead and follow the instructions to fix any conflicts.

::: mobile/android/base/java/org/mozilla/gecko/GeckoAppShell.java
(Diff revision 1)
>  import android.net.NetworkInfo;
>  import android.net.Uri;
>  import android.os.Bundle;
>  import android.os.Environment;
>  import android.os.Looper;
> -import android.os.SystemClock;

It might be best to not include automatically removed imports in a commit to avoid confusion, assuming that's what happened here. For now it's fine.

::: mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java:24
(Diff revision 1)
>  import org.mozilla.gecko.db.LocalBrowserDB;
>  import org.mozilla.gecko.dlc.DownloadContentService;
>  import org.mozilla.gecko.home.HomePanelsManager;
>  import org.mozilla.gecko.lwt.LightweightTheme;
>  import org.mozilla.gecko.mdns.MulticastDNSManager;
> +import org.mozilla.gecko.notifications.NotificationHelper;

Unnecessary imports added.

::: mobile/android/base/java/org/mozilla/gecko/GeckoService.java:18
(Diff revision 1)
>  import android.os.IBinder;
>  import android.util.Log;
>  
>  import java.io.File;
>  
> +import org.mozilla.gecko.notifications.ServiceNotificationClient;

Unnecessary imports added.

::: mobile/android/base/java/org/mozilla/gecko/notifications/NotificationHandler.java:16
(Diff revision 1)
>  import android.app.PendingIntent;
>  import android.content.Context;
>  import android.support.v4.app.NotificationCompat;
>  import android.support.v4.app.NotificationManagerCompat;
> +
> +import org.mozilla.gecko.R;

Unnecessary imports added.

::: mobile/android/base/java/org/mozilla/gecko/notifications/NotificationHelper.java:15
(Diff revision 1)
>  
>  import org.json.JSONArray;
>  import org.json.JSONException;
>  import org.json.JSONObject;
> +import org.mozilla.gecko.AppConstants;
> +import org.mozilla.gecko.EventDispatcher;

Unnecessary imports added.

::: mobile/android/base/java/org/mozilla/gecko/notifications/NotificationReceiver.java:8
(Diff revision 1)
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> -package org.mozilla.gecko;
> +package org.mozilla.gecko.notifications;
>  
> +import org.mozilla.gecko.GeckoThread;

Unused imports.

Comment 14

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8774025 [details]
Bug 1279301 Moved notification classes to notification folder, refactored;

https://reviewboard.mozilla.org/r/66670/#review63736

Comment 15

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8774024 [details]
Bug 1279301 fix - Moved notification classes to notfication folder;

https://reviewboard.mozilla.org/r/66668/#review63738

Comment 16

[Jonathan Almeida [:jonalmeida]]

After you've made the appropriate edits, you can go ahead and add me as a reviewer again.

Comment 17

[Broderick Stadden]

(In reply to Jonathan Almeida (:jonalmeida) from comment #16)
> After you've made the appropriate edits, you can go ahead and add me as a
> reviewer again.

I'll go ahead and make the changes. I'm not sure what happened with the imports considering I didn't manually edit them but I'll clean all that up before I resubmit. Do you know if there is something that is in Android studio that might've added them so I can make sure it doesn't happen again?

I don't think I was on the fx team branch so I'll redo it from there and I'll double check the diffs before I send it in. I'll let you know if I have any problems.

Comment 18

[Jonathan Almeida [:jonalmeida]]

(In reply to Broderick Stadden from comment #17)
> (In reply to Jonathan Almeida (:jonalmeida) from comment #16)
> > After you've made the appropriate edits, you can go ahead and add me as a
> > reviewer again.
> 
> I'll go ahead and make the changes. I'm not sure what happened with the
> imports considering I didn't manually edit them but I'll clean all that up
> before I resubmit. Do you know if there is something that is in Android
> studio that might've added them so I can make sure it doesn't happen again?

I took a quick look at StackOverflow and it might be this[1] setting that is enabled for you. Ideally, it would be helpful to have it enabled, but so you don't leave extra unused imports by mistake. It may be good practice to also check your commit either before or after pushing it to reviewboard.

To check before pushing to reviewboard (rb), you can try looking at the diff of your changeset in your terminal with `hg diff -c <rev>`. If you prefer checking it after (since rb can be nicer to work with than your terminal), you can push your change to rb as normal with `hg push review` and then go to the rb link to check it before you hit the publish button.

> I don't think I was on the fx team branch so I'll redo it from there and
> I'll double check the diffs before I send it in. I'll let you know if I have
> any problems.

I recommend building against fx-team, but mozilla-central works as well. Just make sure to pull the latest changes maybe once a week, and rebase on top of that before sending it to review. There are fewer chances of merge conflicts then.

Clearing NI for you as well.

[1]: http://stackoverflow.com/a/36079543/936067

Comment 19

[Broderick Stadden]

This time through I did a closer inspection and many of the imports, specifically in the Notification* classes, that import org.mozilla.gecko.* are being used within the class and creates errors when removed.  The ones that seemed to make no sense(like the systemclock and malformedexception changes) didn't occur this time around and may have been because I was on a different branch.

One example is GeckoService imports the package "org.mozilla.gecko.notifications.ServiceNotificationClient" because it creates a ServiceNotificationClient object.  Another would be NotificationHandler importing "org.mozilla.gecko.R" because it accesses "R.drawable.ic_status_logo" (Is there another way to access that resource without the package?).

My question is, do you think that they are okay with importing the packages or should I go through and try to see what can be done with each one?

Comment 20

[Jonathan Almeida [:jonalmeida]]

Adding those imports seems fair then since we've moving the notification classes to a different package. Yes, the MalformedURLException and SystemClock would be the only issues that you would need to fix then.

Comment 21

[Jonathan Almeida [:jonalmeida]]

For your next patch, you could also consider flattening your two commits into one. My preferred way to do that is with the hg histedit extension but there are other ways[1] to do it as well.

It's a small nitpick, but it makes reviewing and looking at a hg blame easier in the future. It's good to keep each commit concise so that all the changes in it are regarding one thing.

[1]: http://stackoverflow.com/questions/1725607/can-i-squash-commits-in-mercurial

Comment 22

[Broderick Stadden]

Attachment: Bug 1279301 fix - Added imports to Gecko classes requiring access to notifications;

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

Comment 23

[Broderick Stadden]

Comment on attachment 8774024 [details]
Bug 1279301 fix - Moved notification classes to notfication folder;

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

Comment 24

[Broderick Stadden]

Comment on attachment 8774025 [details]
Bug 1279301 Moved notification classes to notification folder, refactored;

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

Comment 25

[Broderick Stadden]

 (In reply to Jonathan Almeida (:jonalmeida) from comment #21)
> For your next patch, you could also consider flattening your two commits
> into one. My preferred way to do that is with the hg histedit extension but
> there are other ways[1] to do it as well.
> 
> It's a small nitpick, but it makes reviewing and looking at a hg blame
> easier in the future. It's good to keep each commit concise so that all the
> changes in it are regarding one thing.
> 
> [1]:
> http://stackoverflow.com/questions/1725607/can-i-squash-commits-in-mercurial

Sorry, I didn't catch this comment before submitting. It is still split. I can redo it if needed.

Comment 26

[Broderick Stadden]

Hey sorry about this Jon but can you refuse the patch.  There's a build error I didn't catch.

"make[5]: *** No rule to make target `java/org/mozilla/gecko/AppNotificationClient.java', needed by `gecko-browser.jar'.  Stop."

Not sure how it didn't come up before but how would I go about fixing this?

Comment 27

[Broderick Stadden]

Ignore that, it didn't have the change to moz.build.  I'm going to push a new patch.  For now, ignore patch 2.  Sorry about all this mix up.

Comment 28

[Broderick Stadden]

Comment on attachment 8774024 [details]
Bug 1279301 fix - Moved notification classes to notfication folder;

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

Comment 29

[Jonathan Almeida [:jonalmeida]]

https://reviewboard.mozilla.org/r/66666/#review64622

This patch looks great! I was able to apply and build it locally, as well as push it to our treeherder build system[1] as well. Notice though that there is one red mark on it for the 'lint' section. If you click on the 'Job Details' tab for it, and then click on the 'lint-results-automationDebug.html'[2] you can see that the linter picked up the registered class `org.mozilla.gecko.NotificationReceiver` as missing in the AndroidManifest.xml.

Fixing that last issue should be all thats left to land this patch! \o/

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=553fd8f28f34c8107f161653ed63904bab89c9fe&selectedJob=24615483
[2]: https://public-artifacts.taskcluster.net/JmzgKgZpRSCZbU5DQgkA5A/0/public/android/lint/lint-results-automationDebug.html#MissingRegistered

Comment 30

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8774024 [details]
Bug 1279301 fix - Moved notification classes to notfication folder;

https://reviewboard.mozilla.org/r/66668/#review64628

Comment 31

[Broderick Stadden]

Comment on attachment 8774024 [details]
Bug 1279301 fix - Moved notification classes to notfication folder;

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

Comment 32

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8774024 [details]
Bug 1279301 fix - Moved notification classes to notfication folder;

https://reviewboard.mozilla.org/r/66668/#review65174

This patch looks good to me. I pushed it to treeherder[1] and it seems to build fine. There is a failure on a taskgraph test, but this seems unrelated to the patch. 

When a patch receives an r+, you can then add the keyword 'checkin-needed' so the sheriffs can know to pick it up during the next merge (I've done that now for this bug).

Congrats on getting your first bug fixed! \o/

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f57ff2f6b4d2e283a67569d0fd7cd0f4d272be8

Comment 33

[Jonathan Almeida [:jonalmeida]]

Broderick, were you interested in working on other bugs or something similar? Since we didn't have any specific tests for notifications, it would be a good idea to have tests written to avoid breaking basic functionality of notifications - I created bug 1290557 to track this. If you're interested in working on it, feel free to pick it up.

Comment 34

[Broderick Stadden]

(In reply to Jonathan Almeida (:jonalmeida) from comment #33)
> Broderick, were you interested in working on other bugs or something
> similar? Since we didn't have any specific tests for notifications, it would
> be a good idea to have tests written to avoid breaking basic functionality
> of notifications - I created bug 1290557 to track this. If you're interested
> in working on it, feel free to pick it up.

I will definitely look into other bugs.  I'm relatively new to programming(<1 year) so there are some more things I need to learn before going into some of the deeper bugs.  I don't have much knowledge on testing yet so I don't know how much help I would be with that next bug.

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4760f549a020
Move notification classes to notification folder. r=jonalmeida

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4760f549a020