Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TPAC Discussion Result #55

Closed
fallaciousreasoning opened this issue Sep 20, 2019 · 12 comments
Closed

TPAC Discussion Result #55

fallaciousreasoning opened this issue Sep 20, 2019 · 12 comments

Comments

@fallaciousreasoning
Copy link
Collaborator

fallaciousreasoning commented Sep 20, 2019

The Badging API was discussed at TPAC 2019-09.

After some discussion a new shape for the API was decided on:

navigator.setAppBadge(number);
navigator.setClientBadge(number, { client });

typedef ClientOrClientId = WindowClient or DOMString;

dictionary ClientBadgeOptions {
    ClientOrClientId client = null;
};

partial interface navigator {
    Promise<void> setAppBadge(optional integer content);
    Promise<void> clearAppBadge();

    Promise<void> setClientBadge(optional integer content, optional ClientBadgeOptions options);
    Promise<void> clearClientBadge(optional ClientBadgeOptions options);
}

partial interface workernavigator {
    Promise<void> setAppBadge(optional integer content);
    Promise<void> clearAppBadge();

    Promise<void> setClientBadge(optional integer content, optional ClientBadgeOptions options);
    Promise<void> clearClientBadge(optional ClientBadgeOptions options);
}

setXXXBadge methods are exposed on the navigator on the ServiceWorker and Window. Calling setClientBadge from the window without specifying a client or client id will result in the current client being badged.

calling setAppBadge from the window will result in the app with the most specific scope containing the current document being badged.

calling setAppBadge from the ServiceWorker will result in the app whose scope exactly matches the service worker (<-- to be discussed, could be above scope, could be within scope) being badged.

@fallaciousreasoning
Copy link
Collaborator Author

@mgiuca let's discuss this when I get back. I know there's no scope setting the app badge from the ServiceWorker, but it is kind of gross :P

@raymeskhoury
Copy link

calling setAppBadge from the ServiceWorker will result in the app whose scope exactly matches the service worker (<-- to be discussed, could be above scope, could be within scope) being badged.

We discussed this a bit in the meeting and I believe it would set the badge for any app that is within the scope of the SW.

@mgiuca
Copy link
Collaborator

mgiuca commented Sep 20, 2019

OK, I had a chat with Raymes. I'm on board with this mostly.

A summary of the (rather extensive) changes (just based on Jay's post) from what I had proposed in #51:

  1. Removal of the Badge namespace; methods appear on Navigator and WorkerNavigator.
  2. Changing setForScope to setAppBadge, with a focus explicitly on badging apps (no more "handle" contexts, it's explicit to apps).
  3. Changing setForDocument to setClientBadge.
  4. Removal of "scope", with the implicit scope being "the app surrounding this document" or "all apps inside this service worker".
  5. Addition of the ability to call setClientBadge from a SW, and being able to specify a client to go along with it.
  6. Returning a promise, rather than void.

Some thoughts:

  1. No strong opinion (though I feel this will pollute the semi-global Navigator namespace a fair amount).
  2. I am OK with focusing on apps specifically and not a generic "handle" (which turned out to be very difficult to write about).
  3. OK.
  4. See below.
  5. It doesn't make sense for a Client to be supplied when this is called from a document, and it doesn't make sense to not supply a Client when called from a worker. I think this needs some thought.
  6. I agree with Raymes; I don't see the point of returning a promise unless we have some failure cases defined (currently it's designed as "fire and forget"). (In fact I'm kind of regretting adding a promise to Web Share since you don't really need to know if it succeeded and it caused a lot of troubles.) I think we should start with just void and add a promise if there are some defined failure conditions that the caller needs to know about.

The big one: removing scope.

The main technical drawback of this is that if you have multiple apps governed by a single SW, you can't badge them individually. Apparently, y'all discussed this and decided that was a use case we want to discourage. I'm hesitant about this, since I don't think the Badge API is the right place to make a stand on this: if we really think that sites should not have multiple apps per SW (i.e., each app should have a SW at the same scope as the app scope), then we should officially (in the Manifest spec) state that this is expected, and not having a SW at the same scope as the manifest is deprecated, and future APIs may not support multiple apps per SW. However, I'm happy for the Badge API to have this deficiency which we can later rectify by either a) adding scope as an option, or b) making the above deprecation announcement in the Manifest spec.

Aside from this, there is a big conceptual problem with the proposal: it has basically the exact opposite behaviour when called from a document vs called from a SW. When called from a document, the user agent searches up the URL hierarchy looking for an app scope to badge. When called from a SW, the user agent searches down the URL hierarchy, looking for any nested app scopes to badge. That's potentially quite confusing.

The way I found to rationalize this, though, is as follows: when called from a document, it searches up the URL hierarchy looking for an app scope to badge. When called from a SW, it applies that algorithm to each URL governed by the SW scope, which causes it to search up the hierarchy looking for an app scope. I still feel uneasy about it.

It has an advantage over the current scope proposal of having a better default in the case where you have multiple apps in different paths, each with their own SW, on the same origin. The default now isolates the badge to the "current app" without having to specify a scope. That's pretty nice.

@mgiuca
Copy link
Collaborator

mgiuca commented Sep 23, 2019

Follow-up discussion with @raymeskhoury and @fallaciousreasoning :

  1. Resolved that when setAppBadge is called from a document, we badge the app whose manifest is <link>ed from that page, if-and-only-if the page is also within scope of the manifest. (As opposed to searching upwards for any manifest in scope.) Rationale: That way, if https://example.com has a manifest that is installed and https://example.com/foo has one that is not installed, calling setAppBadge from https://example.com/foo will not (inappropriately) set a badge on the top-level app, because its pages won't be linking to that manifest. We can reconsider this; the downside is that the badge API won't work if you aren't linking to your manifest on every page. Edit: Actually we're leaning towards not doing this, and just relying on a future "carve-out" mechanism for scopes.
  2. Resolved that when setAppBadge is called from a SW, badge all apps whose scope is "within scope" (even though I don't think SW spec uses that language) of the SW. We specifically will use the definition of scope matching for this, so that if scope matching becomes more finessed in the future (e.g., allowing carve-out sub-URLs that are out of scope) we would respect that.

@fallaciousreasoning
Copy link
Collaborator Author

See also w3c/manifest#782 for results of TPAC discussion.

@mgiuca
Copy link
Collaborator

mgiuca commented Sep 27, 2019

  1. I agree with Raymes; I don't see the point of returning a promise unless we have some failure cases defined (currently it's designed as "fire and forget"). (In fact I'm kind of regretting adding a promise to Web Share since you don't really need to know if it succeeded and it caused a lot of troubles.) I think we should start with just void and add a promise if there are some defined failure conditions that the caller needs to know about.

OK OK we just discussed this and now I understand the reason why we need to make a promise from the get-go.

Say we made it void return, then any errors (e.g., TypeErrors caused by bad inputs) will throw an exception synchronously. If we later change it to return a Promise<void>, then these errors (even those detected synchronously) will now reject the promise rather than throwing. So that will break the existing API. So if we want to leave aside the possibility of returning a Promise in the future, we need to start out returning a Promise<void>.

aarongable pushed a commit to chromium/chromium that referenced this issue Oct 1, 2019
This is the result of some discussion at TPAC. Previously the
BadgeManager applied badges to scopes. This CL changes the BadgeManager
to work with AppIds instead (as the web facing API has been changed).

Previous CLs have changed the API exposed to the Web, so this change
only affects how Chromium stores/applies the badges.

TPAC Discussion Summary:
w3c/badging#55

Related CLs:
https://crrev.com/c/1817999
https://crrev.com/c/1818892
https://crrev.com/c/1816002

Bug: 1006665
Change-Id: I24d18bdad5a8a0a92b0d1b5ac14052b9de8913dc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1830485
Commit-Queue: Jay Harris <harrisjay@chromium.org>
Reviewed-by: Alan Cutter <alancutter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#701449}
@mgiuca
Copy link
Collaborator

mgiuca commented Oct 10, 2019

  1. Resolved that when setAppBadge is called from a document, we badge the app whose manifest is <link>ed from that page, if-and-only-if the page is also within scope of the manifest. (As opposed to searching upwards for any manifest in scope.) Rationale: That way, if https://example.com has a manifest that is installed and https://example.com/foo has one that is not installed, calling setAppBadge from https://example.com/foo will not (inappropriately) set a badge on the top-level app, because its pages won't be linking to that manifest. We can reconsider this; the downside is that the badge API won't work if you aren't linking to your manifest on every page. Edit: Actually we're leaning towards not doing this, and just relying on a future "carve-out" mechanism for scopes.

Long discussion about this with @fallaciousreasoning . We had tentatively decided this as shown in the quoted paragraph, but now we're committing to that decision. Summary:

Choice of two options:

  1. setAppBadge from a document searches upwards for the nearest containing installed app.
  2. setAppBadge from a document gets the <link rel=manifest> in the current document, and badges the associated app.

Decision: Option 1.

Rationale: Option 1 has the downside discussed above when you have apps with nested scopes. If you have an app scope https://example.com/ and an app scope https://example.com/foo/, if you call setAppBadge from https://example.com/foo/, it will badge the /foo/ app if it is installed, but if /foo/ is not installed but / is, it will badge /. It's impossible to control which app gets badged because you don't know which apps the user has installed.

However, it was discussed at TPAC that we do not want to explicitly cater for the nested app use case. On the horizon, we may define a way for scopes to exclude sub-paths which would give you more control (then /foo/ might not match the scope /).

On the other hand, Option 2 has an even more difficult problem: it requires us to finally solve the long-standing issue of specifying the identity of an app (#586). Because we would have to say "follow the <link rel=manifest> for the current document, and get the associated app and badge it. We would need to explicitly define a way to find an associated installed app, given a manifest. We have never explicitly defined this: is it the manifest URL? The start URL? The scope URL? We've always wanted to answer this but it's hard (even within Chrome, our desktop/Android implementations disagree about what the app key is), and whatever we pick makes it impossible to migrate some aspect of your app. Ideally, we would define an explicit app_id field so you can migrate the app without changing its identity.

So we don't want to block on #586. Therefore, we're going with Option 1. However, if we do add app_id to manifest in the future, then we could add an optional parameter to the Badge API to specify an app_id to badge, which means you can avoid accidentally badging an outer-scope app.

@marcoscaceres
Copy link
Member

When an app is installed, its identity should be clear in that it's isolated from the web browser (it has its own cache, permissions, cookie jar, URL scope, etc)... i.e., it becomes an OS level concern, no? setAppBadge() applies to "that installed thing" 📲 - being option 2.

@marcoscaceres
Copy link
Member

I guess the point being that it's unlikely we will ever agree what the identity of an installed app is (or how that's actually implemented).... just that we know what the qualities are, and that there is a "app installed on the OS, as far as the user is concerned" - but the rest is details.

@mgiuca
Copy link
Collaborator

mgiuca commented Oct 11, 2019

I guess the point being that it's unlikely we will ever agree what the identity of an installed app is (or how that's actually implemented)....

I hope eventually we will agree (but I don't want to block on this).

Yes. Right now, "identity" is a user-agent-specific concept. It doesn't really come up at all unless you consider cross-device syncing and/or automatic updating, which aren't part of a spec.

What I mean by identity is: what aspect of the website would have to change in order for the user agent to consider this "a different app". If it's the manifest URL, that means if you move the manifest URL, now the user agent may offer to install a new app because you don't already have that one installed. But it could also be the scope URL, which means you can change your manifest URL as long as you keep the scope the same, and the user agent will continue to view it as the same app.

If we specced the Badge API as "badge the app associated with the currently linked manifest", then that question of "how does a manifest URL / contents relate to an actual app object installed in the UA's app database" becomes part of the spec for the first time.

aarongable pushed a commit to chromium/chromium that referenced this issue Oct 11, 2019
ExperimentalBadge.set ==> navigator.setExperimentalAppBadge
ExperimentalBadge.clear ==> navigator.clearExperimentalAppBadge

Note: This also removes the (unused) scope parameter from the
IDL files and returns a Promise<void> from both APIs, in case
we ever want to promisify the API in future (see
w3c/badging#55 (comment)).

TPAC Discussion Summary:
w3c/badging#55

Summary of IDL Changes:

interface ExperimentalBadge {
  static void set(optional unsigned long long contents);
  static void clear();
}

changed to:

partial interface Navigator {
  Promise<void> setExperimentalAppBadge(optional unsigned long long contents);
  Promise<void> clearExperimentalAppBadge();
}

Bug: 719176
Change-Id: Ied825cb1a58df674d5c7de59278cbf8b6e3b32f9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1816002
Commit-Queue: Jay Harris <harrisjay@chromium.org>
Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Matt Giuca <mgiuca@chromium.org>
Cr-Commit-Position: refs/heads/master@{#704926}
@marcoscaceres
Copy link
Member

If we specced the Badge API as "badge the app associated with the currently linked manifest", then that question of "how does a manifest URL / contents relate to an actual app object installed in the UA's app database" becomes part of the spec for the first time.

Manifests and installed apps are conceptually separate things: a web app could be installed without any manifest at all (i.e., they are somewhat orthogonal things, though manifests help - but it's UA dependent).

@mgiuca mgiuca closed this as completed in 8508df0 Oct 17, 2019
@mgiuca
Copy link
Collaborator

mgiuca commented Oct 17, 2019

We've landed the changes proposed at the top of this issue (to the explainer at least). Spec changes coming in #45.

mgiuca added a commit to mgiuca/badging that referenced this issue Oct 24, 2019
mgiuca added a commit to mgiuca/badging that referenced this issue Nov 1, 2019
pdigennaro pushed a commit to washezium/washezium that referenced this issue Nov 4, 2019
This reverts the (most obvious) new behavior added in
https://crrev.com/692412 while making
minimal changes (so as to ease merging).

This is the result of some discussion at TPAC here:
w3c/badging#55

Changes that this will not revert:
* Inheriting the badge of a parent app.

(cherry picked from commit 725a3df)

Bug: 1006665
Change-Id: I845f56f166f00dd3c7cc76e323be27cb90881e3e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1817999
Commit-Queue: Jay Harris <harrisjay@chromium.org>
Reviewed-by: Matt Giuca <mgiuca@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#700117}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1832185
Reviewed-by: Jay Harris <harrisjay@chromium.org>
Cr-Commit-Position: refs/branch-heads/3904@{#541}
Cr-Branched-From: 675968a-refs/heads/master@{#693954}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants