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

IPC calls for promise #35

Closed
marcoscaceres opened this issue Jun 24, 2019 · 9 comments
Closed

IPC calls for promise #35

marcoscaceres opened this issue Jun 24, 2019 · 9 comments

Comments

@marcoscaceres
Copy link
Member

marcoscaceres commented Jun 24, 2019

Because there is much IPC going on when setting a badge, I think this API might need to return promises and only allow setting the badge “one at a time”... so more like a request to set the badge. We might need to retain state, and only allow setting once a setting operation has completed. Clear()ing would probably want to abort ongoing set() operations, then behave as set()ing to 0.

Also, as already stated in another bug, we need to disassociate this API from installation IMO (and “Web Applications”, as that’s fairly meaningless). And we definitely need to restrict this to top level browsing contexts only and same origin iframes. Would third party contexts ever be allowed to set the badge? Do we need a feature policy?

@fallaciousreasoning
Copy link
Collaborator

I think this API might need to return promises and only allow setting the badge "one at a time"...

There are two potential races here:

  1. The same page sets the badge twice in quick succession
  2. Two different pages (in future maybe a service worker) set the badge at the same time.

Promises would let developers manage the first, though we could get into a similar situation if they didn't wait for the promise to resolve. I don't think this buys us much we couldn't get with some kind of batching on the renderer side of the IPC pipe (wait for a response from the browser before sending the next value, only store the most recently 'set'd' value).

I don't think promises help us at all with 2), though I'm not sure we need to worry about it. I suspect most of the time both pages will be setting the same value (in the case of a new message) and if they aren't, they have bigger problems (there's only one place to display a badge and how do they know which one badged first?).

we need to disassociate this API from installation IMO

I kind of like the idea of this, but I'm a bit less clear on how it should work. For example, how do we determine the scope of what to badge, without a web app manifest? We could require that all set calls take a scope, but it could behave pretty strangely with different windows/tabs badging different scopes. If we want to go down this route, we might need quite a different API.

And we definitely need to restrict this to top level browsing contexts only and same origin iframes

Agreed. Do you think the same origin iframes are required?

Would third party contexts ever be allowed to set the badge? Do we need a feature policy?

I'm leaning towards no. If we change our minds, I imagine we can add a feature policy in the future.

@marcoscaceres
Copy link
Member Author

I don't think this buys us much we couldn't get with some kind of batching on the renderer side of the IPC pipe (wait for a response from the browser before sending the next value, only store the most recently 'set'd' value).

Yeah, something like that could work... run to completion; take the last synchronously set value; send it over the IPC pipe. Rinse and repeat. My working assumption is independent of app installation, so could be racy across multiple browser tabs (and workers, as you point out).

I guess this might be worth prototyping a bit.

For example, how do we determine the scope of what to badge, without a web app manifest?

Yeah, that's a good question... I wonder if it should be origin bound, and "scoped" if the app is installed.

@fallaciousreasoning
Copy link
Collaborator

Yeah, that's a good question... I wonder if it should be origin bound, and "scoped" if the app is installed.

Perhaps we should be per-tab (client?) scoping things by default? I feel like origin scoping could be a bit restrictive as a default.

Consider Github PRs
Per 'PR' badge icon ('tick' when everything is good, 'X' when checks fail). It'd be pretty strange to apply that to all tabs/windows.

I feel like there are really two APIs here.
The 'Status' case, where the badge shows a specific per-page piece of information.
The 'Notifications' case, where the badge shows some app-wide count of notifications.

I had a bit of a look at #1, where we discuss some stuff that seems pretty similar.

Another potential problem is that we have a lot more control over what gets badged in the favicon than what gets badged in the OS. It would be a shame to introduce a new API for badging the favicon that's limited because we want something consistent with badging the OS.

@raymeskhoury
Copy link

I agree that there are 2 different use cases

  1. app-wide badging
  2. tab/window-specific badging

I think this API is really solving (1). We could extend it to solve (2) but it's unclear that there is a need. If a site isn't installed, we could possibly still use the manifest linked to the page to determine app scope?

@fallaciousreasoning
Copy link
Collaborator

fallaciousreasoning commented Jul 2, 2019

Do you think updating the spec to explicitly state that only the most recent badge value is used would fix the IPC issue? If so, I'll submit a PR for that and close this issue (I think the App vs Tab debate belongs in #1 and #12 )

@raymeskhoury
Copy link

@fallaciousreasoning that makes sense to me. Fire and forget seems best here.

@marcoscaceres
Copy link
Member Author

Random thought: If we are going to use the "origin scope model", and if we have N tabs calling .set() at the same time, then it's definitely going to be racy.

@fallaciousreasoning
Copy link
Collaborator

fallaciousreasoning commented Aug 1, 2019

If the pages want to ensure synchronization, they can go through a service worker.

In addition, if there are multiple pages setting different badges for the same scope it implies some of them are setting the wrong badge (or at the very least, a badge that's out of date), as each scope can only have a single badge. If they are setting the same badge, it's not a problem.

I don't think we need to worry too much, but definitely good to think about :)

@marcoscaceres
Copy link
Member Author

closed based on discussion in #35.... we are returning promises.

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

3 participants