1336355 - Launched manifest bookmarks should have their own taskswitcher entry

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Do you really want a separate OS process or just a separate entry in "recent apps"?

Comment 2

[Dale Harvey (:daleharvey)]

Yup I was being vague about wether it would be a full on process / APK or just its own item in the task switcher waiting on whether it would be possible to do the full APK thing, but yeh its own entry in the task switcher is the best we can go for now

Comment 3

[Dale Harvey (:daleharvey)]

Attachment: Open webapps in new activity

Comment 4

[Dale Harvey (:daleharvey)]

This creates a new activity to handle the opening of webapps, its a plain activity with no title or toolbar and sets the android status bar to the theme_color defined in the manifest. https://bugzilla.mozilla.org/show_bug.cgi?id=1337341 will deal with the follow on work of what happens when the user navigates from the web app

Comment 5

[Dale Harvey (:daleharvey)]

Attachment: Open webapps in new activity

Sorry, forgot to add the layout to this

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8841953 [details] [diff] [review]
Open webapps in new activity

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

Overall this looks good to me. My only concern is codify manifest values into the intent.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
@@ +2033,5 @@
>                      .decodeDataURI(getContext(), message.getString("icon"))
>                      .getBestBitmap(GeckoAppShell.getPreferredIconSize());
> +                final String themeColor = message.getString("theme_color");
> +
> +                createAppShortcut(name, startUrl, icon, themeColor);

I wouldn't store this (and other values from the manifest) in the Intent: We can't update them. If the manifest changes then we are still stuck with those values. The same is true for the "name", but there's no workaround for that.

Shouldn't we read those values on startup to use whatever latest value was in the manifest the last time we fetched it? However I'm not sure how we are going to update it anyways? Is this defined in the spec? What do you think?

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
@@ +2041,5 @@
>          // After shortcut is created, show the mobile desktop.
>          ActivityUtils.goToHomeScreen(this);
>      }
>  
> +    public Integer parseColor(final String color) {

This should go into ColorUtil.

::: mobile/android/base/java/org/mozilla/gecko/webapps/WebAppActivity.java
@@ +42,5 @@
> +        if (themeColor == NO_COLOR) {
> +            return;
> +        }
> +        final Window window = getWindow();
> +        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) {

Please use a constant from AppConstants for that.

Comment 7

[Dale Harvey (:daleharvey)]

Attachment: Open webapps in new activity

Addressed all the nits, however I think there is a little more I want to do here, on the webappactivity starting I send a message to browser.js, browser.js then reads some data (including the icon over the network) then sends the data back, it works however does cause an extra network request and causes delay on the UI.

At the very least the manifest icon needs to be cached somewhere, either it can be done on the js side when first fetched or when java first sees it. As a bonus we may want to cache the manifest information on the java side, but I think this is low priority atm

One thing I was thinking it that we store the manifest information as json in the profile, we could add the icon to that data easily (either as base64 in the current json, or just as a straight blob) and Java can read that data from disk directly, as long as Java is only ever readonly we dont need to worry multithreaded disk access, it avoids all the message passing and encoding which adds complexity/time.

If there is a big reason to not do that then I am thinking we cache the base64 icon in the existing manifest json file. 

What do you think is best Sebastian

Comment 8

[Dale Harvey (:daleharvey)]

Also while testing I am noticing a race condition in this code for when its asking gecko data before we set the 'Tab:selected', we dont want to be waiting for gecko so this launch data is definitely going to be needed java side

So its either reading the manifest data from java directly, or storing a cached version in browserdb somewhere

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Clearing NI: We talked on IRC.

Comment 10

[Dale Harvey (:daleharvey)]

Attachment: Open webapps in new activity

So this has Java read the contents of the manifest directly, this means we do not need to duplicate stored data to be accessed from java / js, the manifest logic remains accessible to gecko which is going to be needed for future features and there is no delay in waiting for gecko to boot to show the UI to android not message passing back and forth. The Java code accessing the manifest is and only ever will be read only and there are no consistency issues.

(in future if we are worried about Java needing to own these files, I think we should be looking at allowing Java to hook in at the JSONFile level)

The rest of the comments are addressed, cheers

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8846707 [details] [diff] [review]
Open webapps in new activity

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

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
@@ +2000,5 @@
>          final Tab selectedTab = Tabs.getInstance().getSelectedTab();
>          final String manifestUrl = selectedTab.getManifestUrl();
>  
>          if (manifestUrl != null) {
> +            Log.w(LOGTAG, "Attempting to install with manifest: " + manifestUrl);

We avoid logging URLs and other sensitive data. Earlier versions of Android allow apps (with those permissions) to read the logs of other apps. So this potentially can leak browsing data to other apps.

::: mobile/android/base/java/org/mozilla/gecko/util/ColorUtil.java
@@ +37,5 @@
> +            return Color.argb(
> +                              255,
> +                              Integer.valueOf(color.substring(1, 3), 16),
> +                              Integer.valueOf(color.substring(3, 5), 16),
> +                              Integer.valueOf(color.substring(5, 7), 16));

nit: The style of this is a bit unusual. We just use the default wrapping of Android Studio which should look something like this:

> return Color.argb(255,
>         Integer.valueOf(color.substring(1, 3), 16),
>         Integer.valueOf(color.substring(3, 5), 16),
>         Integer.valueOf(color.substring(5, 7), 16));

Anyways, just make sure to run "mach gradle checkstyle" before pushing :)

::: mobile/android/base/java/org/mozilla/gecko/webapps/WebAppActivity.java
@@ +44,5 @@
> +    private static final String LOGTAG = "WebAppActivity";
> +
> +    @Override
> +    public void onCreate(Bundle savedInstanceState) {
> +

nit: empty line (and in the try block in updateFromManifest(). Nothing critical, just looks "different" :)

@@ +50,5 @@
> +
> +        String manifestPath = getIntent().getStringExtra(WebAppActivity.MANIFEST_PATH);
> +        if (manifestPath != null) {
> +            updateFromManifest(manifestPath);
> +        }

Should we file a bug for handling error cases (no path in intent, manifest file not on disk). Right now this looks like we just show an empty activity in those situations.

@@ +63,5 @@
> +        try {
> +
> +            final JSONParser parser = new JSONParser();
> +            final JSONObject manifest = (JSONObject) parser.parse(new FileReader(manifestPath));
> +            final JSONObject manifestField = (JSONObject) manifest.get("manifest");

If possible try to use org.json.* and not org.json.simple.*. The simple library is something we included for sync historically.  We want to get rid of it eventually (bug 1204559).

You might notice that reading from a file with org.json requires a lot of boilerplate code. You'll find a helper for that in FileUtils (readJSONObjectFromFile).

@@ +75,5 @@
> +            updateStatusBarColor(color);
> +            setTaskDescription(taskDescription);
> +
> +        } catch (IOException | ParseException e) {
> +            e.printStackTrace();

Please use an explicit log statement: Log.*(...)

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4168b9960850
Open webapps in new activity. r=sebastian

Comment 13

[Dale Harvey (:daleharvey)]

> Should we file a bug for handling error cases (no path in intent, 
> manifest file not on disk). Right now this looks like we just show an 
> empty activity in those situations.

This shouldnt happen (of course, error conditions never should), but if it does we only fail to set the theme color and the icon + name, the content still loads and the rest of the chrome behaves the same way, the activity will just have the firefox generic theming so not sure there is much better we could actually do in this case.

Addressed the rest of the comments, cheers for the FileUtils tip, and landed, thanks so much :)

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4168b9960850

Comment 15

[Dale Harvey (:daleharvey)]

Attachment: Fix linting error

Apologies, I didnt check for lint failures before landing, this was referencing an id from the custom tabs layout we arent using. Tomcat said he will land on m-c directly

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/dd2f2db1f87a
follow up to fix lint. r=sebastian a=tomcat

Comment 17

[Oana Horvath [:ohorvath]]

Verified as fixed on Nightly 55.0a2 (2017-03-30). 
Devices: LG G4 (Android 5.1) & Huawei Honor 8 (Android 6.0)

Comment 18

[Jovan Gerodetti]

I installed the latest nightly fenne, but can't get this to work. I installed a coupple of PWAs including Dales test app, but they are always loaded inside the main activity.

Comment 19

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Did you go to about:config and set pref("manifest.install.enabled", true) ?

(In reply to Jovan Gerodetti from comment #18)
> I installed the latest nightly fenne, but can't get this to work. I
> installed a coupple of PWAs including Dales test app, but they are always
> loaded inside the main activity.

Comment 20

[Jovan Gerodetti]

ah I see, thanks. It works now, but adding to the homescreen crashes fennec on https://snapdrop.net

Comment 21

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

(In reply to Jovan Gerodetti from comment #20)
> ah I see, thanks. It works now, but adding to the homescreen crashes fennec
> on https://snapdrop.net
It's working fine from me using the latest nightly. Any specific step to reproduce?

Comment 22

[Jovan Gerodetti]

(In reply to Wesley Huang [:wesley_huang] (EPM) (NI me) from comment #21)
> (In reply to Jovan Gerodetti from comment #20)
> > ah I see, thanks. It works now, but adding to the homescreen crashes fennec
> > on https://snapdrop.net
> It's working fine from me using the latest nightly. Any specific step to
> reproduce?

I'm using Android 5.0.2 on Sony Z3+ with Nightly 55.0a1 2017-04-10.

1. navigate to https://snapdrop.net
2. choose add to homescreen
3. nightly crashes and crash reporter appears

this is one of the crash reports:
https://crash-stats.mozilla.com/report/index/89cbd842-836c-4aae-8f96-1dadb2170410

Comment 23

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

thanks for the info. I have the same android version 5.0.2 running on z3c but still couldn't reproduce.
Oana, could you check please?

Comment 24

[Oana Horvath [:ohorvath]]

I could reproduce this on Huawei Honor 5X (Android 5.1.1), with the latest Nightly build.
The crash is already logged in Bug 1355676.

Doesn't reproduce on any device, as I've also verified on:
-Samsung Galaxy Note 4 (Android 5.0.1)
-LG G4 (Android 5.1)