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

Consistency of ServiceWorkerClient fields #500

Closed
dmurph opened this issue Oct 1, 2014 · 12 comments
Closed

Consistency of ServiceWorkerClient fields #500

dmurph opened this issue Oct 1, 2014 · 12 comments
Milestone

Comments

@dmurph
Copy link
Collaborator

dmurph commented Oct 1, 2014

I'm looking at implementing these fields, and I'm wondering, is there any consistency guarantees w/ client properties? Especially regarding the URL, which can be changed via javascript w/o loading a new page (and making a new client).

Interface here:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#service-worker-client-interface

@dmurph
Copy link
Collaborator Author

dmurph commented Oct 1, 2014

Or, to clarify, should I be updating these properties dynamically? Or should they be constant once the 'getAll' is called from ServiceWorkerClients.

The static version would be much easier to implement, and avoids us needing to send a whole bunch of ipcs whenever those properties change (performance issue)

@annevk
Copy link
Member

annevk commented Oct 1, 2014

Which are dynamic other than a document's URL? (Although this already seems problematic. Not sure what a good API for that would be :/)

@dmurph
Copy link
Collaborator Author

dmurph commented Oct 1, 2014

I'm assuming 'hidden' and 'focused' are dynamic, I'm not sure about the frame context.

Keeping all of these up to date seems like it would be a lot of coding if we wanted to do this efficiently. Naive solution is to have messages fire whenever this changes to the SW if there is one, but that is unneeded and someone spamming changing any of these properties would make lots of IPCs happen possibly for no reason if no one was listening.

The best approach, if we want dynamic updating, would be to only send the state change IPCs if the worker is alive and listening, but that involves more code (this goes from renderer -> browser -> renderer). Also, we could easily add onchange support for these properties if these are dynamically updating.

Granted, we have to do something like this for the 'ready' Promise, but that wiring is (I believe) mostly there already.

I don't think it's unreasonable to have the ServiceWorkerClient be an immutable snapshot (other than the 'ready', and have the worker re-call getAll to get the current state again. But that's obviously not as nice for the end user.

@annevk
Copy link
Member

annevk commented Oct 1, 2014

Can't we make them all methods that always return a fresh promise?

@dmurph
Copy link
Collaborator Author

dmurph commented Oct 1, 2014

That would work, but I think it reverses the inefficiency, and the best case solution still involves doing some sort of optional smart syncing of the sw client state

to clarify on reversing the inefficiency, we can now send the IPC train whenever the SW asks for any of these variables, but the state might have not changed, so we waste that.

I do think this (make them promises, and sending an IPC every time the SW asks) is better than the other way (sending IPCs on all state changes if we have a SW) if we want to make it dynamic, because it no longer effects the world that doesn't use this API. Also, I don't think the frequency of this request would come close to the frequency of the state changes (unless you can think of something).

Also, what's wrong with making the whole internal state:
readonly attribute boolean hidden;
readonly attribute boolean focused;
readonly attribute ScalarValueString url;
readonly attribute ContextFrameType frameType;
in a Promise?

@annevk
Copy link
Member

annevk commented Oct 2, 2014

Getting all that state with a single promise is probably better, yes.

@KenjiBaheux
Copy link
Collaborator

Currently out of scope for blink => no impact (blink).

@KenjiBaheux KenjiBaheux added this to the Version 2 milestone Oct 14, 2014
@KenjiBaheux
Copy link
Collaborator

I think it's time to start using Version 2 labels to avoid forgetting about these "V1 => no impact (blink)" issues.

@jakearchibald
Copy link
Contributor

In the ts I listed hidden, focused, url, frameType as being set on object creation and not updating. Is there any issue with this?

The developer would call getAll, look at all the clients, & decide what to do. Eg:

  • There's an appropriate tab focused & visible, no need to show a notification
  • There's a tab with a particular url already open, I'll navigate/message that rather than opening something new

@annevk
Copy link
Member

annevk commented Oct 20, 2014

I think you're right @jakearchibald. We don't want developers to create code that keeps objects around anyway to get fresh state as the service worker might be killed at any moment.

@dmurph
Copy link
Collaborator Author

dmurph commented Nov 11, 2014

I like that conclusion.

@mfalken
Copy link
Member

mfalken commented Sep 15, 2019

The conclusion is Client is a snapshot and doesn't get updated. It'd be good to have tests for this. I'll look into filing a new issue to add tests.

@mfalken mfalken closed this as completed Sep 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants