Launched manifest bookmarks should have their own taskswitcher entry

VERIFIED FIXED in Firefox 55

Status

()

Firefox for Android
General
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: daleharvey, Assigned: daleharvey)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 55
Points:
---

Firefox Tracking Flags

(firefox55 verified)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

a year ago
Blocks: 1285858
Do you really want a separate OS process or just a separate entry in "recent apps"?
(Assignee)

Comment 2

a year ago
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
(Assignee)

Updated

a year ago
Summary: Launch Manifest Bookmarks in own process → Launched manifest bookmarks should have their own taskswitcher entry
(Assignee)

Updated

a year ago
Assignee: nobody → dale
(Assignee)

Comment 3

a year ago
Created attachment 8841933 [details] [diff] [review]
Open webapps in new activity
Attachment #8841933 - Flags: review?(s.kaspari)
(Assignee)

Comment 4

a year ago
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
(Assignee)

Comment 5

a year ago
Created attachment 8841953 [details] [diff] [review]
Open webapps in new activity

Sorry, forgot to add the layout to this
Attachment #8841933 - Attachment is obsolete: true
Attachment #8841933 - Flags: review?(s.kaspari)
Attachment #8841953 - Flags: review?(s.kaspari)
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.
Attachment #8841953 - Flags: review?(s.kaspari) → feedback+
(Assignee)

Comment 7

a year ago
Created attachment 8844067 [details] [diff] [review]
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
Attachment #8841953 - Attachment is obsolete: true
Flags: needinfo?(s.kaspari)
(Assignee)

Comment 8

a year ago
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
Clearing NI: We talked on IRC.
Flags: needinfo?(s.kaspari)
(Assignee)

Comment 10

a year ago
Created attachment 8846707 [details] [diff] [review]
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
Attachment #8844067 - Attachment is obsolete: true
Attachment #8846707 - Flags: review?(s.kaspari)
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.*(...)
Attachment #8846707 - Flags: review?(s.kaspari) → review+

Comment 12

a year ago
Pushed by dharvey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4168b9960850
Open webapps in new activity. r=sebastian
(Assignee)

Comment 13

a year ago
> 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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4168b9960850
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
(Assignee)

Comment 15

a year ago
Created attachment 8848038 [details] [diff] [review]
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
Attachment #8848038 - Flags: review?(s.kaspari)
Attachment #8848038 - Flags: review?(s.kaspari) → review+

Comment 16

a year ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/dd2f2db1f87a
follow up to fix lint. r=sebastian a=tomcat

Comment 17

a year ago
Verified as fixed on Nightly 55.0a2 (2017-03-30). 
Devices: LG G4 (Android 5.1) & Huawei Honor 8 (Android 6.0)
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified

Comment 18

a year ago
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.
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

a year ago
ah I see, thanks. It works now, but adding to the homescreen crashes fennec on https://snapdrop.net
(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

a year ago
(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
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?
Flags: needinfo?(oana.horvath)

Comment 24

a year ago
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)
Flags: needinfo?(oana.horvath)
You need to log in before you can comment on or make changes to this bug.