1304145 - [GeckoView] Move o.m.g.notification package out of geckoview

Status: RESOLVED FIXED

(GeckoView :: General, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

Each GeckoView consumer should provide its own implementation of notifications, so Fennec's implementation in the o.m.g.notification package should move into Fennec sources. This involves lots of refactoring and untangling of dependencies.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 1. Restrict access for GeckoAppShell JNI methods (v1)

General cleanup patch: make JNI methods in GeckoAppShell private if
possible, because they're not meant to be used in Java from outside of
GeckoAppShell.

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 2. Change GeckoAppShell's notification interface (v1)

Instead of using NotificationClient directly from GeckoAppShell, add a
GeckoAppShell.NotificationListener interface, which NotificationClient
would implement. This isolates NotificationClient (and the notification
package) from GeckoAppShell and lets us move the notification package to
Fennec. It also makes a cleaner interface for GeckoView consumers to
implement notification support.

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 3. Move GeckoService and notification package to Fennec (v1)

GeckoService and the notification package have some interdependencies,
so if we want to move the notification package, we have to move
GeckoService also. With that said, it's good to move GeckoService in
any case, because it's a Fennec component just like GeckoApp.

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 4. Use string names instead of integer IDs for notifications (v1)

Use string names instead of integer IDs to identify notifications. The
integer IDs came from the hashes of the string names, so they are not
guaranteed to be unique. There can be unintentional collisions, or
worse, a site can intentionally make its notification collide with and
replace another site's notification.

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 5. Implement NotificationListener in NotificationClient (v1)

Provide Fennec's implementation of GeckoAppShell.NotificationListener in
NotificationClient. A lot of this code was removed in an earlier patch
from GeckoAppShell, so combined with this patch, we're essentially
moving code from GeckoAppShell to NotificationClient.

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 6. Update auto-generated bindings (v1)

Comment 7

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8793083 [details] [diff] [review]
1. Restrict access for GeckoAppShell JNI methods (v1)

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

Stamp!

Comment 8

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8793084 [details] [diff] [review]
2. Change GeckoAppShell's notification interface (v1)

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

This looks fine, although it's hard to verify good things have happened since you've removed code that you're about to re-add, just across (non-MozReview!) patches.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java
@@ +458,5 @@
>      /* package */ static native void onLocationChanged(double latitude, double longitude,
>                                                         double altitude, float accuracy,
>                                                         float bearing, float speed, long time);
>  
> +    public interface NotificationListener {

Consider making this top-level, or a sub-interface of GeckoView, like ChromeDelegate is.  Prefer to unify on Delegate instead of Listener, since that's what we started with.  Although now I see we have more listeners than delegates here.  Sigh.

@@ +624,5 @@
> +    public static NotificationListener getNotificationListener() {
> +        return sNotificationListener;
> +    }
> +
> +    public static void setNotificationListener(final NotificationListener listener) {

It's not obvious how to remove an _existing_ notification listener, given that `null` will cause crashes and `DEFAULT_LISTENERS` is private.

@@ +954,4 @@
>      @WrapForJNI(dispatchTo = "gecko")
>      private static native void notifyAlertListener(String name, String topic);
>  
> +    public static void onNotificationShow(final String name) {

nit: it's weird to me that you've swapped `showNotification` for `notificationShow`.  You wanted to group by notification in the name?  Do it through-out, or better yet, follow Gecko's event names through-out.

Comment 9

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8793086 [details] [diff] [review]
3. Move GeckoService and notification package to Fennec (v1)

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

These files should all move, since the Gradle build works by directory.

I'm surprised that you consider GeckoService a Fennec component.  I'm going to assume you are confident you can handle "normal" Android lifecycle events for GV-consuming applications without the service, although I don't understand the whole picture here.

Comment 10

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8793086 [details] [diff] [review]
3. Move GeckoService and notification package to Fennec (v1)

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

These files should all move, since the Gradle build works by directory.

I'm surprised that you consider GeckoService a Fennec component.  I'm going to assume you are confident you can handle "normal" Android lifecycle events for GV-consuming applications without the service, although I don't understand the whole picture here.

Comment 11

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8793087 [details] [diff] [review]
4. Use string names instead of integer IDs for notifications (v1)

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

I believe that this patch uses strings instead of integers, but I don't understand how it addresses your second concern: "a site can intentionally make its notification collide with and replace another site's notification".  I didn't dig hard, but I can't see where the notification IDs are namespaced to the site or origin.  Does this happen on the Gecko side?  (I hope it does!)

f+ 'cuz the code is fine; r- to address the question and to remind me to come back to this.

Comment 12

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8793088 [details] [diff] [review]
5. Implement NotificationListener in NotificationClient (v1)

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

See notes, but I'm fine with this.  No need for re-review.

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
@@ +2017,5 @@
>          }
> +
> +        // If Gecko isn't running yet, we ignore the notification. Note that
> +        // even if Gecko is running but it was restarted since the notification
> +        // was created, the notification won't be handled (bug 849653).

I was going to ask what happened switching integer IDs to strings across App upgrades, but that's a Gecko restart fo' sho' and we'll just lose those notifications too :(

@@ +2018,5 @@
> +
> +        // If Gecko isn't running yet, we ignore the notification. Note that
> +        // even if Gecko is running but it was restarted since the notification
> +        // was created, the notification won't be handled (bug 849653).
> +        if (GeckoThread.isRunning()) {

I find these GeckoThread checks surprising; I assume it's to protect your class cast, which might not be correct if the default listener is in place?

::: mobile/android/base/java/org/mozilla/gecko/notifications/NotificationClient.java
@@ +24,4 @@
>  /**
>   * Client for posting notifications through a NotificationHandler.
>   */
> +public abstract class NotificationClient implements GeckoAppShell.NotificationListener {

I feel like your earlier patch cast in a way that wouldn't actually work before this patch:
```
((NotificationClient) GeckoAppShell.getNotificationListener()).add(id, builder.build());
```
Are you confident that your intermediate steps are green?  If not, feel free to fold after review.  (The sushi-ing of the patch did make review easier, so thanks for that!)

Comment 13

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 2. Change GeckoAppShell's notification interface (v1.1)

Updated patch; carry over r+

(In reply to Nick Alexander :nalexander from comment #8)
> Comment on attachment 8793084 [details] [diff] [review]
> 2. Change GeckoAppShell's notification interface (v1)
> 
> Review of attachment 8793084 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Consider making this top-level, or a sub-interface of GeckoView, like
> ChromeDelegate is.  Prefer to unify on Delegate instead of Listener, since
> that's what we started with.  Although now I see we have more listeners than
> delegates here.  Sigh.

Made NotificationListener top-level. Going over the Android API, 'Listener'
seems to be the preferred term for simple interfaces, whereas 'Delegate' is
used for more heavyweight classes that are meant to be subclassed.

> It's not obvious how to remove an _existing_ notification listener, given
> that `null` will cause crashes and `DEFAULT_LISTENERS` is private.

It now resets to default on null. Also added same behavior to the other
listener setters.

> @@ +954,4 @@
> >      @WrapForJNI(dispatchTo = "gecko")
> >      private static native void notifyAlertListener(String name, String topic);
> >  
> > +    public static void onNotificationShow(final String name) {
> 
> nit: it's weird to me that you've swapped `showNotification` for
> `notificationShow`.  You wanted to group by notification in the name?  Do it
> through-out, or better yet, follow Gecko's event names through-out.

`onNotificationShow` is a callback for NotificationListener implementations. I
added some comments explaining that. For example, Gecko first calls
NotificationListener.showNotification. Then NotificationListener calls Gecko
back through GeckoAppShell.onNotificationShow. I opted to name the callbacks
using the Android convention of on<Noun><Verb>.

Comment 14

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8793086 [details] [diff] [review]
3. Move GeckoService and notification package to Fennec (v1)

(In reply to Nick Alexander :nalexander from comment #10)
> Comment on attachment 8793086 [details] [diff] [review]
> 3. Move GeckoService and notification package to Fennec (v1)
> 
> Review of attachment 8793086 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> These files should all move, since the Gradle build works by directory.

The patch does move the files, but I guess splinter didn't show that.

> I'm surprised that you consider GeckoService a Fennec component.  I'm going
> to assume you are confident you can handle "normal" Android lifecycle events
> for GV-consuming applications without the service, although I don't
> understand the whole picture here.

My thinking is that GeckoService is the main background component for Fennec, just as GeckoApp/BrowserApp is the main foreground component for Fennec. GeckoView itself shouldn't be running Android Services; IMO that's the GV consumer's job.

Comment 15

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8793086 [details] [diff] [review]
3. Move GeckoService and notification package to Fennec (v1)

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

Ah, splinter.  Stamp!

And I'm fine with Fennec owning the service, although I'm really curious as to how you'll be able to maintain a long-lived GeckoThread without an Android object encapsulating it's lifecycle.  It's worth noting that the Android system plays that role for WebViews.

Comment 16

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8793086 [details] [diff] [review]
3. Move GeckoService and notification package to Fennec (v1)

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

Ah, splinter.  Stamp!

And I'm fine with Fennec owning the service, although I'm really curious as to how you'll be able to maintain a long-lived GeckoThread without an Android object encapsulating it's lifecycle.  It's worth noting that the Android system plays that role for WebViews.

Comment 17

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 4. Use string names instead of integer IDs for notifications (v2)

(In reply to Nick Alexander :nalexander from comment #11)
> Comment on attachment 8793087 [details] [diff] [review]
> 4. Use string names instead of integer IDs for notifications (v1)
> 
> Review of attachment 8793087 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I believe that this patch uses strings instead of integers, but I don't
> understand how it addresses your second concern: "a site can intentionally
> make its notification collide with and replace another site's notification".
> I didn't dig hard, but I can't see where the notification IDs are namespaced
> to the site or origin.  Does this happen on the Gecko side?  (I hope it
> does!)

Yeah, for web notifications, the "notification name" comes from Gecko and is a
combination of the origin and a custom tag (if specified), so it's possible for
the site to manipulate the tag, so that the combined name has a hashcode that's
the same as another notification from another site or from Fennec itself.

Comment 18

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 5. Implement NotificationListener in NotificationClient (v1.1)

Updated patch; carry over r+

(In reply to Nick Alexander :nalexander from comment #12)
> Comment on attachment 8793088 [details] [diff] [review]
> 5. Implement NotificationListener in NotificationClient (v1)
> 
> Review of attachment 8793088 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I was going to ask what happened switching integer IDs to strings across App
> upgrades, but that's a Gecko restart fo' sho' and we'll just lose those
> notifications too :(

These notifications are temporary, so they are not affected by the switch.

> > +        // If Gecko isn't running yet, we ignore the notification. Note that
> > +        // even if Gecko is running but it was restarted since the notification
> > +        // was created, the notification won't be handled (bug 849653).
> > +        if (GeckoThread.isRunning()) {
> 
> I find these GeckoThread checks surprising; I assume it's to protect your
> class cast, which might not be correct if the default listener is in place?

I copy and pasted this block, but looks like the check is not actually needed
anymore; removed in new patch.

> Are you confident that your intermediate steps are green?  If not, feel free
> to fold after review.  (The sushi-ing of the patch did make review easier,
> so thanks for that!)

Yeah they're meant to be folded.

Comment 19

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8793415 [details] [diff] [review]
4. Use string names instead of integer IDs for notifications (v2)

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

Update the commit message to include that the message IDs include the origin from Gecko, so they're namespaced.  Onwards!

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/10ce1c1de6b7
1. Restrict access for GeckoAppShell JNI methods; r=nalexander
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cffc3494427
2. Change GeckoAppShell's notification interface; r=nalexander
https://hg.mozilla.org/integration/mozilla-inbound/rev/d676a611d3ac
3. Move GeckoService and notification package to Fennec; r=nalexander
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b6d1a72900b
4. Use string names instead of integer IDs for notifications; r=nalexander
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6b76d373a4a
5. Implement NotificationListener in NotificationClient; r=nalexander
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ead4233329b
6. Update auto-generated bindings; r=me

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/10ce1c1de6b7
https://hg.mozilla.org/mozilla-central/rev/4cffc3494427
https://hg.mozilla.org/mozilla-central/rev/d676a611d3ac
https://hg.mozilla.org/mozilla-central/rev/6b6d1a72900b
https://hg.mozilla.org/mozilla-central/rev/b6b76d373a4a
https://hg.mozilla.org/mozilla-central/rev/4ead4233329b

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/10ce1c1de6b7
https://hg.mozilla.org/mozilla-central/rev/4cffc3494427
https://hg.mozilla.org/mozilla-central/rev/d676a611d3ac
https://hg.mozilla.org/mozilla-central/rev/6b6d1a72900b
https://hg.mozilla.org/mozilla-central/rev/b6b76d373a4a
https://hg.mozilla.org/mozilla-central/rev/4ead4233329b