1321320 - Ability to determine whether a url is within an installed app scope

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Comment 1

[Dale Harvey (:daleharvey)]

Support bug for https://bugzilla.mozilla.org/show_bug.cgi?id=1285858, spec is @ https://developer.mozilla.org/en-US/docs/Web/Manifest#scope + https://www.w3.org/TR/appmanifest/#navigation-scope)

One of the requirements for support of standalone mode in web apps is to be able to determine whether a url is within the app scope of a previously installed application, ie you have installed the twitter.com web app and you visit https://twitter.com/daleharvey, or from twitter.com you navigate to https://support.twitter.com/ is that within the pwa or not?

An important point is one url can be within the scope of many web apps that have different display modes, also 'scope' is a relatively unused feature so we need to both provide sensible defaults and to ensure that if the user navigates outside of an app scope they have the ability to being their chrome back so they do not get stuck.

When we install an application somewhere we will need to maintain a list of those installed urls + their scope fields and we can perform a lookup on navigation

Comment 2

[Dale Harvey (:daleharvey)]

Hey Marcos, writing this up now and realising the scope issue is somewhat underdefined (also noticing open issues on the spec for example).

Now for a basic example, I can visit https://mobile.twitter.com/marcosc in chrome, "install" it (add to homescreen) and open in standalone mode, the standalone mode persists for any tweet I visit / user profile I see so for that example I infer that any url that starts with "https://mobile.twitter.com" is within the default scope. Defining the default scope seems a little tricky though, it cant be the page installed from (twitter.com/tweet/atweet would only have 1 tween within scope), We could use the manifest url but it seems like people doing example.com/static/manifest.json could be popular. I cant see any downsides to using the start_url as the default scope though?

As for scopes 

  1. There is no limitation on scope right? so example.com can include "https://login.google.com" within its scope (to handle oauth etc)

  2. Is there any root on the scope? say I install "http://example.com/myapp/manifest.json" with a scope of "/data", is "http://example.com/data/" or "http://example.com/myapp/data/" in scope (and does scope: ["data/"] work?)

Comment 3

[Marcos Caceres [:marcosc]]

(In reply to Dale Harvey (:daleharvey) from comment #2)
> Hey Marcos, writing this up now and realising the scope issue is somewhat
> underdefined (also noticing open issues on the spec for example).
> 
> Now for a basic example, I can visit https://mobile.twitter.com/marcosc in
> chrome, "install" it (add to homescreen) and open in standalone mode, the
> standalone mode persists for any tweet I visit / user profile I see so for
> that example I infer that any url that starts with
> "https://mobile.twitter.com" is within the default scope.

It's "unbound", meaning you can navigate anywhere you want and the display mode will apply. However, Chrome will display an address bar if you leave the origin. 

We have opportunity to do better here: whatever we feel comfortable with. 

> Defining the
> default scope seems a little tricky though, it cant be the page installed
> from (twitter.com/tweet/atweet would only have 1 tween within scope), We
> could use the manifest url but it seems like people doing
> example.com/static/manifest.json could be popular. I cant see any downsides
> to using the start_url as the default scope though?

Yeah, that could also work. We can try that, specially if none was given. 

> As for scopes 
> 
>   1. There is no limitation on scope right? so example.com can include
> "https://login.google.com" within its scope (to handle oauth etc)

Scope are same origin, so no. For authentication, you would need something like the credential management API to display an authentication overlay. Or, you would need to open another window, go through the oauth flow, and navigate back to the PWA (same as today). 

>   2. Is there any root on the scope? say I install
> "http://example.com/myapp/manifest.json" with a scope of "/data", is
> "http://example.com/data/" or "http://example.com/myapp/data/" in scope (and
> does scope: ["data/"] work?)

The algorithm is: if url.startsWith(scope) then it's in scope. It's a simple string check. So:

Scope is: http://example.com/data

* is in scope: http://example.com/myapp/data/
* is not in scope: http://example.com/myapp/data/

About ["data/"], there is only 1 scope per app right now. We don't support arrays of scopes... but getting pressure to do so.

Comment 4

[Dale Harvey (:daleharvey)]

Attachment: 1321320-1.patch

Sebastian, this doesnt have any end user changes to Android yet, the changes to the android code is just to handle the changes to the manifest api

Marcos, so this has us store manifests locally as well as a registry of installed manifests, we currently only expect a single scope but multiple scopes can easily be supported

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8834357 [details] [diff] [review]
1321320-1.patch

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

Here's the thing I'm not sure about yet (what do you think?): Starting Gecko takes about 1-3 seconds depending on the device. So we are already displaying the app UI for quite some time until we actually have a browser engine running and can do things. This means it will take 1-3 seconds until we know about what's in the manifest (or do you want to write it from JS but read it from Java?) - Isn't this a problem if we want to start web apps in full screen right away, or show a splash screen until we get ready? For that we'd need to know the contents of the manifest just when the web app starts.

Comment 6

[Dale Harvey (:daleharvey)]

Yeh, having the homescreen bookmark open with its own entry in the task switcher will be worked on in https://bugzilla.mozilla.org/show_bug.cgi?id=1336355 and it will definitely need android to know about certain manifest items before gecko starts up (likely stored as activity metadata when the bookmark is created?).

This is for when the users are navigating, primarily it is for detecting when the user has entered/left the scope of any installed manifest and we can hide show chrome (will be done in https://bugzilla.mozilla.org/show_bug.cgi?id=1337341). In future it is likely gecko will need to know which manifests are installed as to give implicit permissions to installed sites.

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Dale Harvey (:daleharvey) from comment #6)
> Yeh, having the homescreen bookmark open with its own entry in the task
> switcher will be worked on in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1336355 and it will definitely
> need android to know about certain manifest items before gecko starts up
> (likely stored as activity metadata when the bookmark is created?).

This is one option. The downside is that we can't update this when the manifest changes.

Comment 8

[Marcos Caceres [:marcosc]]

Comment on attachment 8834357 [details] [diff] [review]
1321320-1.patch

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

Couple of stylistic/idiomatic things with the API - but I suggest we fix them now (particularly the getters on Manifest). r+ with those.

::: dom/manifest/Manifest.jsm
@@ +33,5 @@
> + * @note The generated hash is returned in base64 form.  Mind the fact base64
> + * is case-sensitive if you are going to reuse this code.
> + */
> +function generateHash(aString) {
> +  let cryptoHash = Cc["@mozilla.org/security/hash;1"]

nit: const, also for let below (as neither are reassigned)

@@ +57,5 @@
>  
> +/**
> + * Manifest object
> + */
> +function Manifest(browser, manifestUrl) {

Total nit (and feel free to ignore completely!) but could totally be:

class Manifest {
  constructor(browser, manifestUrl){
    // stuff from current Manifest function
  }
  
  async initialise() { ... }
   
  // other stuff...
}

@@ +89,5 @@
> +                                                this._store.data.manifest,
> +                                                expectedSize);
> +  },
> +
> +  scope() {

I think the following 4 should all be getters instead of methods. 

```
  get scope(){

  }

```

@@ +116,5 @@
> +
> +
> +// We maintain a list of scopes for installed webmanifests so we can determine
> +// whether a given url is within the scope of a previously installed manifest
> +const MANIFESTS_FILE = "manifest-scopes.json";

Maybe just inline this? It's only ever used once and it's not exported or reused anywhere.

Alternatively, move it up to where MANIFESTS_DIR is defined. That way they are visually grouped together.

@@ +134,5 @@
> +    this._store = new JSONFile({path: this._path});
> +    await this._store.load();
> +
> +    // If we dont have any existing data, initialise empty
> +    if (!("scopes" in this._store.data)) {

Maybe just: 

  !this._store.data.hasOwnProperty("scopes")

IIRC, using "in" walks the whole prototype chain, which I don't think we want here.

@@ +135,5 @@
> +    await this._store.load();
> +
> +    // If we dont have any existing data, initialise empty
> +    if (!("scopes" in this._store.data)) {
> +      this._store.data.scopes = {};

if data.scopes is going to be essentially Map<string, string>, then it might as well be a `new Map()` instead of an object literal.

@@ +153,5 @@
> +
> +  // Given a url, find if it is within an installed manifests scope and if so
> +  // return that manifests url
> +  findManifestUrl(url) {
> +    let matchedScope;

matchedScope is defined but never used. 

Also, maybe just:

```
return this._store.data.scopes
  .filter(scope => url.startsWith(scope))
  .find(scope => this._store.data.scopes[scope]) || null;
```

Returning `false` at the end is not great, as it's a little bit ambiguous and not very idiomatic.

@@ +171,5 @@
> +    // url of the client and see if it matches the scope of any installed
> +    // manifests
> +    if (!manifestUrl) {
> +      const url = stripQuery(browser.currentURI.spec);
> +      manifestUrl = Manifests.findManifestUrl(url);

saying "Manifests.findManifestUrl" implies that it's a static method; so maybe just use "this.findManifestUrl" here?

::: dom/manifest/test/browser_Manifest_install.js
@@ +29,5 @@
> +    let manifest = yield Manifests.getManifest(browser, manifestUrl);
> +    is(manifest.installed(), false, 'We havent installed this manifest yet');
> +
> +    yield manifest.install(browser);
> +    is(manifest.name(), 'hello World', 'Manifest has correct name');

Q: will you also make sure `icon`, `url`, and `start_url` are returning the correct things? or if that tested somewhere else?

@@ +41,5 @@
> +
> +    let foundManifest = Manifests.findManifestUrl("http://example.org/browser/dom/");
> +    is(foundManifest, manifestUrl, "Finds manifests within scope");
> +
> +    foundManifest = Manifests.findManifestUrl("http://example.org/");

Can Manifest.manifestInstalled() also be tested?

::: mobile/android/chrome/content/browser.js
@@ +2174,5 @@
> +    await manifest.install();
> +    const icon = await manifest.icon(iconSize);
> +    GlobalEventDispatcher.sendRequest({
> +      type: "Website:AppInstalled",
> +      icon: icon,

Nit: s/icon: icon/icon,

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ccda17e0776e
Track installed manifests. r=marcos, r=s.kaspari

Comment 10

[Dale Harvey (:daleharvey)]

Thanks for the reviews, Marcos I took all your changes, thanks, got a green try run @ https://treeherder.mozilla.org/#/jobs?repo=try&revision=e93294fad7680de57f850d9b440f73b36022c0ef and now on inbound

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ccda17e0776e

Comment 12

[Oana Horvath [:ohorvath]]

Changes made here were applied, we can close the bug as fixed.