move webapp event listeners/handlers from GeckoAppShell to webapp/ class

RESOLVED FIXED in Firefox 29

Status

()

defect
P2
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: myk, Assigned: mhaigh)

Tracking

unspecified
Firefox 29
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

We should move webapp event listeners and handlers in GeckoAppShell into a class in the webapp/ package.
Assignee: nobody → mhaigh
Only one event listener & handler which I couldn't see a way of moving, this is due to the return value and the asyn nature of the listener.

https://github.com/mykmelez/gecko-dev/compare/5fe4a16...467d1f4
Posted patch Proposed patch (obsolete) — Splinter Review
Attachment #8357113 - Flags: feedback?(myk)
Attachment #8357113 - Flags: feedback?(jhugman)
Comment on attachment 8357113 [details] [diff] [review]
Proposed patch

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

Nice. Overall: DELETE MOAR CODEZ.

::: mobile/android/base/GeckoApp.java
@@ +691,4 @@
>                      message.optString("className"), message.optString("action"), message.optString("title"));
>              } else if (event.equals("Locale:Set")) {
>                  setLocale(message.getString("locale"));
> +            } else if (!AppConstants.MOZ_ANDROID_SYNTHAPKS && event.equals("WebApps:PreInstall")) {

WebApps:PreInstall was originally used to find a profile directory. This is now done before AutoInstall is called.

In its current form, any reference to WebApps:PreInstall can be removed.

::: mobile/android/base/webapp/WebAppShell.java
@@ +90,5 @@
> +    }
> +  }
> +
> +
> +  // The old implementation of preInstallWebApp.  Not used by MOZ_ANDROID_SYNTHAPKS.

If this isn't used by MOZ_ANDROID_SYNTHAPKS is there any reason we should keep it around?

@@ +97,5 @@
> +    GeckoProfile profile = GeckoProfile.get(GeckoAppShell.getContext(), "webapp" + index);
> +    return profile.getDir();
> +  }
> +
> +  // The old implementation of postInstallWebApp.  Not used by MOZ_ANDROID_SYNTHAPKS.

Ditto.

@@ +113,5 @@
> +    WebAppAllocator allocator = WebAppAllocator.getInstance(GeckoAppShell.getContext());
> +    int index = allocator.findOrAllocatePackage(aPackageName);
> +    allocator.putOrigin(index, aOrigin);
> +  }
> +

Nit. Too much whitespace between methods.

@@ +126,5 @@
> +        int index;
> +        if (AppConstants.MOZ_ANDROID_SYNTHAPKS) {
> +          index = WebAppAllocator.getInstance(GeckoAppShell.getContext()).releaseIndexForApp(uniqueURI);
> +        } else {
> +          index = org.mozilla.gecko.WebAppAllocator.getInstance(GeckoAppShell.getContext()).releaseIndexForApp(uniqueURI);

128 and 130 are the same line. Is there any need to check MOZ_ANDROID_SYNTHAPKS.

@@ +154,5 @@
> +      }
> +    });
> +}
> +
> +

Nit. Too much whitespace between methods.
Attachment #8357113 - Flags: feedback?(jhugman) → feedback+
Posted patch revised patch (obsolete) — Splinter Review
Attachment #8357113 - Attachment is obsolete: true
Attachment #8357113 - Flags: feedback?(myk)
Attachment #8357777 - Flags: feedback?(myk)
Attachment #8357777 - Flags: feedback?(jhugman)
Comment on attachment 8357777 [details] [diff] [review]
revised patch

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

::: mobile/android/base/webapp/WebAppShell.java
@@ +32,5 @@
> +import java.util.List;
> +
> +public class WebAppShell implements GeckoEventListener {
> +
> +  private static final String LOGTAG = "WebAppShell";

The tiniest of nits that I should've spotted earlier – please could you prefix the logtag with "Gecko", so I can grep for all gecko logtags in logcat.
Attachment #8357777 - Flags: feedback?(jhugman) → feedback+
Posted patch revised 957070 patch (obsolete) — Splinter Review
Attachment #8357777 - Attachment is obsolete: true
Attachment #8357777 - Flags: feedback?(myk)
Attachment #8358423 - Flags: feedback?(myk)
Attachment #8358423 - Flags: feedback?(jhugman)
Priority: -- → P2
Comment on attachment 8358423 [details] [diff] [review]
revised 957070 patch

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

(In reply to James Hugman [:jhugman] [@jhugman] from comment #3)
> In its current form, any reference to WebApps:PreInstall can be removed.> > +  // The old implementation of preInstallWebApp.  Not used by MOZ_ANDROID_SYNTHAPKS.
> 
> If this isn't used by MOZ_ANDROID_SYNTHAPKS is there any reason we should
> keep it around?

We must keep around the entire old implementation until we enable the new implementation and then decide to remove the old implementation, at which point we should remove it in its entirety.


> @@ +126,5 @@
> > +        int index;
> > +        if (AppConstants.MOZ_ANDROID_SYNTHAPKS) {
> > +          index = WebAppAllocator.getInstance(GeckoAppShell.getContext()).releaseIndexForApp(uniqueURI);
> > +        } else {
> > +          index = org.mozilla.gecko.WebAppAllocator.getInstance(GeckoAppShell.getContext()).releaseIndexForApp(uniqueURI);
> 
> 128 and 130 are the same line. Is there any need to check
> MOZ_ANDROID_SYNTHAPKS.

The first line references org.mozilla.gecko.webapps.WebAppAllocator (the new implementation), while the other one references org.mozilla.gecko.WebAppAllocator (the old implementation), and we need to check MOZ_ANDROID_SYNTHAPKS to reference the correct one.  (If Java had a preprocessor, then we'd preprocess the other one out at build time; alternately, if Java had class aliases, then we'd assign WebAppAllocator to the appropriate one at load time; but it has neither, so we have to import one as WebAppAllocator, reference the other by its fully-qualified name, and determine which to call at runtime).


Overall, this looks good, except that we need to keep around the old implementation for now, and "WebAppShell" doesn't seem like a good description of the code being moved into the new file.  It seems more like a WebAppEventListener or a part of InstallHelper.

::: mobile/android/base/GeckoApp.java
@@ +1550,4 @@
>          registerEventListener("Intent:GetHandlers");
>          registerEventListener("Locale:Set");
>  
> +        mWebAppShell= new WebAppShell();

Nit: space between variable name and assignment operator.
Attachment #8358423 - Flags: feedback?(myk) → feedback+
Attachment #8358423 - Attachment is obsolete: true
Attachment #8358423 - Flags: feedback?(jhugman)
Attachment #8359258 - Flags: review?(myk)
Attachment #8359258 - Flags: review?(jhugman)
Comment on attachment 8359258 [details] [diff] [review]
New patch with revisions based on feedback

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

This version reintroduces WebAppEventListener.preInstallWebApp, but it doesn't register a listener for WebApps:PreInstall.  It needs to do so, as we can't get rid of any aspect of the pre-existing non-synthetic implementation until we've enabled and shipped synthetic APKs (such that there is no possibility that we will disable the feature on a stabilization or release branch).

Otherwise, just a few issues, see below.  I haven't looked closely at the functions that moved, i.e. preInstallWebApp, postInstallWebApp (x2), uninstallWebApp, and installApk, on the presumption that they're straight copies.  But me know if that isn't the case and I should look at them.

::: mobile/android/base/webapp/WebAppEventListener.java
@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +package org.mozilla.gecko.webapp;
> +
> +

Nit: extra blank line.

@@ +12,5 @@
> +import org.mozilla.gecko.GeckoProfile;
> +import org.mozilla.gecko.util.EventDispatcher;
> +import org.mozilla.gecko.util.GeckoEventListener;
> +import org.mozilla.gecko.util.ActivityResultHandler;
> +import org.mozilla.gecko.util.ThreadUtils;

Nit: order the org.mozilla.gecko imports alphabetically (but keep them above the other imports).

@@ +28,5 @@
> +import org.json.JSONException;
> +import org.json.JSONObject;
> +
> +import java.io.File;
> +import java.util.List;

Nit: order the rest of the imports alphabetically (but keep them grouped by most significant part and below the org.mozilla.gecko imports).

@@ +32,5 @@
> +import java.util.List;
> +
> +public class WebAppEventListener implements GeckoEventListener {
> +
> +  private static final String LOGTAG = "GeckoWebAppEventListener";

Use four-space indentation.

@@ +113,5 @@
> +    int index = allocator.findOrAllocatePackage(aPackageName);
> +    allocator.putOrigin(index, aOrigin);
> +  }
> +
> +  public static void uninstallWebApp(final String uniqueURI) {

uninstallWebApp also needs to be removed from GeckoAppShell.
Attachment #8359258 - Flags: review?(myk) → feedback+
One more thing: the older classes like WebAppImpl, WebAppAllocator, and WebAppDispatcher all have "WebApp" prefixes, but the newer ones do not, and it seems better to leave off the prefix, given that they're all in the "webapp" subdirectory now.
I've removed the WebApp prefix from the files in the 'webapp' directory, aside from WebAppImpl which needs to have the prefix retained because of the file WebApp.java.in.

Preinstall has been hooked up to the event listeners again, but it's being listened for in GeckoApp.java because of the requirement to pass back data to the JS.
Attachment #8359258 - Attachment is obsolete: true
Attachment #8359258 - Flags: review?(jhugman)
Attachment #8361048 - Flags: feedback?(myk)
Comment on attachment 8361048 [details] [diff] [review]
Webapp prefixes removed from files in webapp directory and reinstates calls to preinstall

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

A couple of notes and a nit, but otherwise this looks great!  Let's get wesj to review it.

::: mobile/android/base/GeckoApp.java
@@ +29,4 @@
>  import org.mozilla.gecko.util.ThreadUtils;
>  import org.mozilla.gecko.util.UiAsyncTask;
>  import org.mozilla.gecko.webapp.UninstallListener;
> +import org.mozilla.gecko.webapp.EventListener;

I guess the one danger with removing the "WebApp" prefix from these class names is import clashes (f.e. with org.mozilla.gecko.AnotherFeature.EventListener).  But let's burn that bridge when we come to it (or if wesj complains).

@@ +640,5 @@
>                  String name = message.getString("name");
>                  String manifestURL = message.getString("manifestURL");
>                  String origin = message.getString("origin");
>                  // preInstallWebapp will return a File object pointing to the profile directory of the webapp
> +                mCurrentResponse = mWebAppEventListener.preInstallWebApp(name, manifestURL, origin).toString();                

Nit: trailing whitespace.

::: mobile/android/base/GeckoAppShell.java
@@ +712,5 @@
>      public static Intent getWebAppIntent(String aURI, String aOrigin, String aTitle, Bitmap aIcon) {
>          Intent intent;
>  
>          if (AppConstants.MOZ_ANDROID_SYNTHAPKS) {
> +            org.mozilla.gecko.webapp.Allocator slots = org.mozilla.gecko.webapp.Allocator.getInstance(getContext());

Now that the new implementation's allocator's class name is different from the old one's, you could actually import org.mozilla.gecko.webapp.Allocator and reference it via simply Allocator here.
Attachment #8361048 - Flags: feedback?(myk) → feedback+
Attachment #8361048 - Flags: review?(wjohnston)
Comment on attachment 8361048 [details] [diff] [review]
Webapp prefixes removed from files in webapp directory and reinstates calls to preinstall

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

Mostly nits.

::: mobile/android/base/GeckoApp.java
@@ +29,4 @@
>  import org.mozilla.gecko.util.ThreadUtils;
>  import org.mozilla.gecko.util.UiAsyncTask;
>  import org.mozilla.gecko.webapp.UninstallListener;
> +import org.mozilla.gecko.webapp.EventListener;

Heh. I've actually started doing this a lot, and wondering when it will bite my ass. Until it does, continue on! Heck, you could just use Listener here....

@@ +639,1 @@
>              } else if (!AppConstants.MOZ_ANDROID_SYNTHAPKS && event.equals("WebApps:PreInstall")) {

Is there a reason to leave this code outside here? I keep reading about how we want to kill it, but I didn't see why it had to stay in GeckoApp...

@@ +1556,5 @@
>          registerEventListener("Intent:GetHandlers");
>          registerEventListener("Locale:Set");
> +        registerEventListener("WebApps:PreInstall");
> +
> +        mWebAppEventListener = new EventListener();

For these types of objects, I usually prefer to just make them singletons. i.e. can we just call

EventListener.getInstance().init()

We do a bunch of those in GeckoApplication for things that might want to run even if the Activity (but not Gecko) is killed:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApplication.java#66

::: mobile/android/base/webapp/Dispatcher.java
@@ +18,5 @@
> +
> +        Allocator allocator = Allocator.getInstance(getApplicationContext());
> +
> +        if (bundle == null) {
> +            bundle = getIntent().getExtras();

According to http://developer.android.com/reference/android/content/Intent.html#getExtras%28%29 this may still be null after this. Maybe that's a crazy error condition and we should crash anyway though :)

::: mobile/android/base/webapp/EventListener.java
@@ +72,5 @@
> +                    String name = message.getString("name");
> +                    String manifestURL = message.getString("manifestURL");
> +                    String iconURL = message.getString("iconURL");
> +                    String originalOrigin = message.getString("originalOrigin");
> +                    String origin = message.getString("origin");

Lets try to add some whitespace here so this code is more readable. Or pull things out into private methods.

@@ +90,5 @@
> +            Log.e(LOGTAG, "Exception handling message \"" + event + "\":", e);
> +        }
> +    }
> +
> +    // The old implementation of preInstallWebApp.  Not used by MOZ_ANDROID_SYNTHAPKS.

Lets avoid words like 'old' and 'new', since they're quickly outdated. Just "Not used by MOZ_ANDROID_SYNTHAPKS" is probably good.

@@ +92,5 @@
> +    }
> +
> +    // The old implementation of preInstallWebApp.  Not used by MOZ_ANDROID_SYNTHAPKS.
> +    public static File preInstallWebApp(String aTitle, String aURI, String aOrigin) {
> +        int index = org.mozilla.gecko.WebAppAllocator.getInstance(GeckoAppShell.getContext()).findAndAllocateIndex(aOrigin, aTitle, (String) null);

Like Myk said, You could import WebAppAllocator now :)

@@ +101,5 @@
> +    // The old implementation of postInstallWebApp.  Not used by MOZ_ANDROID_SYNTHAPKS.
> +    public static void postInstallWebApp(String aTitle, String aURI, String aOrigin, String aIconURL, String aOriginalOrigin) {
> +        org.mozilla.gecko.WebAppAllocator allocator = org.mozilla.gecko.WebAppAllocator.getInstance(GeckoAppShell.getContext());
> +        int index = allocator.getIndexForApp(aOriginalOrigin);
> +        assert index != -1 && aIconURL != null;

I like these asserts a lot. Do they actually get built? I don't see any -ea arguments in our Makefile...

@@ +104,5 @@
> +        int index = allocator.getIndexForApp(aOriginalOrigin);
> +        assert index != -1 && aIconURL != null;
> +        Bitmap icon = BitmapUtils.getBitmapFromDataURI(aIconURL);
> +        allocator.updateAppAllocation(aOrigin, index, icon);
> +        GeckoAppShell.createShortcut(aTitle, aURI, aOrigin, icon, "webapp");

Same coment about needing some whitespace in here

::: mobile/android/base/webapp/WebAppAllocator.java
@@ -15,5 @@
> -import java.util.ArrayList;
> -
> -import android.util.Log;
> -
> -public class WebAppAllocator {

You should probably just hg move these to preserve them and blame. I'd just manually edit the patch file to put this back in and remove the new file, and then hg move it, and then replace it with the new code at the top of this patch.

::: mobile/android/base/webapp/WebAppDispatcher.java
@@ -8,5 @@
> -import android.app.Activity;
> -import android.content.Intent;
> -import android.os.Bundle;
> -
> -public class WebAppDispatcher extends Activity {

Same with this.
Attachment #8361048 - Flags: review?(wjohnston) → review+
Refactored slightly and addressed nits.

> For these types of objects, I usually prefer to just make them singletons.
> i.e. can we just call
>
> EventListener.getInstance().init()

I've converted over to a singleton design, but have left off an initialisation step in GeckoApplication as it doesn't need one. 


> Is there a reason to leave this code outside here? I keep reading about how 
> we want to kill it, but I didn't see why it had to stay in GeckoApp...

We can't completely move the preinstall code into the EventListener class as it returns a value which is picked up by another mechanism within GeckoApp.java.  I couldn't see a way of being able to return a value without some presence in GeckoApp. 

> According to http://developer.android.com/reference/android/content/Intent.html#getExtras%28%29 this may still be null after this. Maybe that's a crazy error condition and we should crash anyway though :)

This should never happen! (as I've stated in the new log message) and as such a crash is appropriate at this stage

> I like these asserts a lot. Do they actually get built? I don't see any -ea arguments in our Makefile...

I'm afraid I don't know - I would imagine they are stripped out in a non-debug build

>You should probably just hg move these to preserve them and blame. I'd just manually edit the patch file to put this back in and remove the new file, and then hg move it, and then replace it with the new code at the top of this patch.

I believe this patch is in the correct format!
Attachment #8361048 - Attachment is obsolete: true
Attachment #8365064 - Flags: feedback?(wjohnston)
Attachment #8365064 - Flags: feedback?(myk)
Comment on attachment 8365064 [details] [diff] [review]
Move webapp event handlers in to own class and refactor surrounding code

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

::: mobile/android/base/webapp/EventListener.java
@@ +58,5 @@
> +        registerEventListener("WebApps:PreInstall");
> +        registerEventListener("WebApps:InstallApk");
> +        registerEventListener("WebApps:PostInstall");
> +        registerEventListener("WebApps:Open");
> +        registerEventListener("WebApps:Uninstall");

You might be better to just throw these in a const array that we'd automatically loop over. We forget to unregister thiings sometimes, so having one list instead of two would be nicer :)

@@ +81,5 @@
> +                } else {
> +                    postInstallWebApp(message.getString("name"), 
> +                                      message.getString("manifestURL"), 
> +                                      message.getString("origin"), 
> +                                      message.getString("iconURL"), 

nit - whitespace after there (and below)

@@ +89,5 @@
> +                Intent intent = GeckoAppShell.getWebAppIntent(message.getString("manifestURL"), 
> +                                                              message.getString("origin"), 
> +                                                              "", null);
> +                if (intent == null) {
> +                    return;

When the new event responder stuff lands (in 30) we'll have an easier way to send back errors (asynchronously). We should show some sort of "Error launching!" toast then...
Attachment #8365064 - Flags: feedback?(wjohnston) → feedback+
Comment on attachment 8365064 [details] [diff] [review]
Move webapp event handlers in to own class and refactor surrounding code

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

I *think* mhaigh actually meant to ask wesj for review on this patch, so I'm requesting it.

::: mobile/android/base/webapp/Dispatcher.java
@@ +22,5 @@
>          if (bundle == null) {
>              bundle = getIntent().getExtras();
>          }
>  
> +        if(bundle == null) {

Nit: space between "if" and opening parenthesis.
Attachment #8365064 - Flags: review?(wjohnston)
Attachment #8365064 - Flags: feedback?(myk)
Attachment #8365064 - Flags: feedback+
Comment on attachment 8365064 [details] [diff] [review]
Move webapp event handlers in to own class and refactor surrounding code

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

Heh. I'll let my feedback stand as a review. If you want to do the String[] thing, feel free to carry the r+.
Attachment #8365064 - Flags: review?(wjohnston) → review+
(In reply to Wesley Johnston (:wesj) from comment #17)
> Heh. I'll let my feedback stand as a review. If you want to do the String[]
> thing, feel free to carry the r+.

I ended up leaving the patch alone, except to fix the nits and touch CLOBBER (without which ProGuard killed my depend build on "Warning: org.mozilla.gecko.GeckoAppShell$18: can't find enclosing method 'void installApk(android.app.Activity,java.lang.String,java.lang.String)' in class org.mozilla.gecko.GeckoAppShell" after which "there were 1 unresolved references to program class members" and then "java.io.IOException: Please correct the above warnings first").
https://hg.mozilla.org/mozilla-central/rev/6638c6ade38c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Blocks: 957066
You need to log in before you can comment on or make changes to this bug.