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

ServiceWorkerClient to Client #588

Closed
jakearchibald opened this issue Dec 12, 2014 · 29 comments
Closed

ServiceWorkerClient to Client #588

jakearchibald opened this issue Dec 12, 2014 · 29 comments

Comments

@jakearchibald
Copy link
Contributor

These shouldn't be specific to ServiceWorker, and should move into the Fetch spec.

These bits stay the same:

partial interface ServiceWorkerGlobalScope : WorkerGlobalScope {
  readonly attribute ServiceWorkerClients clients;
}

interface ServiceWorkerClients {
  Promise<sequence<Client>> getAll(optional ServiceWorkerClientQueryParams params);
}

dictionary ServiceWorkerClientQueryParams {
  // include clients on the origin that aren't controlled by this SW
  boolean includeUncontrolled;
}

Here's the different bits:

enum ClientType { "window", "worker", "sharedworker", "serviceworker" };

dictionary ClientOptions {
  ClientType type = "window";
};

[Constructor(USVString url, optional ClientOptions options), Exposed=Window, Worker]
interface Client {
  readonly attribute Promise<void> update;
  readonly attribute VisibilityState visibilityState;
  readonly attribute boolean focused;
  readonly attribute USVString url;
  readonly attribute ContextFrameType frameType;
  readonly attribute ClientType type;
  readonly attribute ServiceWorkerRegistration? serviceWorkerRegistration;
  void postMessage(any message, optional sequence<Transferable> transfer);
  Promise<void> focus();
}

The properties of a client do not change after construction unless update is called.

The constructor takes a url and options. The options are a bit useless at the moment, as it should throw if type is set to anything other than window. If type is window (the default), the user agent is requested to open a new tab/window. This may depend on a user-interaction event, as window.open does now.

update causes visibilityState, focused, url, and serviceWorkerRegistration to refresh to the client's current values. If the client hasn't yet succeeded/failed to open, it waits. It rejects if the client is no longer running, or failed to ever run. It doubles as a ready method. Question: Do we need a ready method regardless?

visibilityState is from Page Visibility. It reports hidden for non-window types.

focused is the result of hasFocus() from the focus management APIs. Question: Or should it be true if the browser window has focus and the containing tab is active? As in, should it be true for a top-level client if a contained client (such as an iframe) has focus? Should it be true for a frame in a frameset even if focus is in one of the other frames in the frameset? Should it be true for a top-level client if docked devtools has focus?

url is the current url of the client. This is the script url for workers, but can change throughout the life of a window via hash changes and pushState. Question: Or should this be the creation url, therefore static for windows too? Once we get clients.claim(), it'll be taking over clients based on creation url so pushState cannot allow pages to "jump scope" and be claimed by a registration they otherwise wouldn't be.

frameType comes from the fetch spec. It lets you tell iframes apart from top-level windows.

type lets you tell windows apart from different types of worker.

serviceWorkerRegistration lets you know if the client is controlled by a worker or not.

postMessage does what you'd expect.

focus rejects if the type is not "window". Calls window.focus() on the related window. Resolves if successful, rejects otherwise. This may depend on a user-interaction event. Question: Should this implicitly call update?

I thought about having separate constructors rather than a type attribute, such as WindowClient, WorkerClient etc etc, but that means we'd have more constructors that'd throw, and I know how y'all hate that!

@jakearchibald
Copy link
Contributor Author

/cc @annevk @mounirlamouri

@mounirlamouri, does implicit update after successful focus make you happier about the snapshot nature of these objects?

@annevk
Copy link
Member

annevk commented Dec 12, 2014

Also @domenic and @Hixie.

It seems like an interesting idea to make Client available everywhere, I had not actually considered that. I was mostly suggesting to remove the "ServiceWorker" prefix since they were scoped to service workers anyway. But this makes sense if we want the ability to open new windows using new Client elsewhere. Also, the postMessage() setup only works if these objects are in a service worker since the other side is expected to use the ServiceWorker object, no?

I'm not sure this satisfies the constructor camp as the user agent will still need to use the token approach to construct these objects in a way that works for user agents (e.g. otherwise there's no way to get one that returns "worker" for type). Given that window and worker are likely to grow apart further, overloading might not be the best way to go about this. We've been bitten by that in the past (e.g. <object>).

I don't really like update. I thought the idea was to require polling given the expected longevity of a service worker?

@domenic
Copy link
Contributor

domenic commented Dec 12, 2014

Out of curiosity, why are we allowing creation of window clients but not worker ones?

Why are we trying to squash windows and workers into a single class? visibilityState, focused, frameType, and focus() all seem inapplicable to workers. Seems better to move the rest of the properties to a base class and have a WindowClient, or similar.


Real Talk about constructors:

In general with constructors, all I want you to do is outline a clear story for how the objects the author sees came into being. If that can be "the browser called new Client(...) just like an author would", that's the best story. If it's "the browser called new Client(..., secretToken, secretOptions)", that's less satisfying, but sometimes necessary. In other words, how does getAll() actually work: if you say that it's impossible to construct Client instances that represent workers, then how did the browser do it? (Using a secret token/set of options, presumably. Why is only the browser allowed to do this? See below.)

I don't like the idea of new Client(...) causing side effects like opening a window. In general object construction should not have any real side effects IMO. For example that would mean that if calling getAll() returns a promise for 5 windows, you'd expect the browser to open 5 new windows before fulfilling the promise, since it had to create the objects before it could return them.

It seems more likely you want a design where Client's constructor is of the form new Client(informationGivingAPointerToImplementationWindowOrWorkerRepresentation). Since authors would never be able to create a well-formed pointer of this type, new Client will always throw for them. (So, in WebIDL terms, "there is no constructor".) Then when getAll() is called the implementation internally does new Client(pointerData) 5 times before fulfilling.

@annevk
Copy link
Member

annevk commented Dec 12, 2014

There is something else that irks me about this design. It does not considers workers fully (we've been seeing this a lot with service workers).

To create a worker, you use new Worker. This creates an environment DedicatedWorkerGlobalScope you can communicate with from your Worker object. In the service worker this is identified by a Client object with type set to "worker" (not by Worker, that would not work for messaging).

It seems weird therefore that for the equivalent case for Window (a worker's DedicatedWorkerGlobalScope), we would use Client for both constructing the environment and identifying it in service workers. And that does indeed fall apart the moment we'd allow Client outside of service workers (e.g. when a Window wants to create another Window), as Client works in conjunction with ServiceWorker objects in the environments only.

@jungkees
Copy link
Collaborator

These shouldn't be specific to ServiceWorker, and should move into the Fetch spec.

But this makes sense if we want the ability to open new windows using new Client elsewhere.

I thought the ServiceWorkerClient object would be just an object that captures a service worker's controlled (or can-be-controlled when selected a registration) client (document or worker) for authors to manipulate and communicate with it. And it's not a fetch event specific instance either IMO. When other functional events are fired at the service worker, the event listeners would still want to check out the existing windows and workers in the shape of the ServiceWorkerClient objects. In that sense, the ServiceWorkerClient here should be something that represents the client that interacts with the service worker. I'm not sure if the advantages the generalized Client can bring is much bigger and I'm missing that points?

Why are we trying to squash windows and workers into a single class? visibilityState, focused, frameType, and focus() all seem inapplicable to workers. Seems better to move the rest of the properties to a base class and have a WindowClient, or similar.

We can do subclassing but then we'd have to define DedicatedWorkerClient and SharedWorkerClient separately as well, and getAll() should resolve with a sequence holding a mix of those different types of objects. Would that be a better design for authors?

@jakearchibald
Copy link
Contributor Author

Before I respond to specific points, I really should have provided example usage code:

Responding to a push message:

self.addEventListener('push', function(event) {
  if (event.message.data == 'new-chat-message') {
    fetch('/latest-messages').then(r => r.json()).then(function(data) {
      clients.getAll().then(function(clients) {
        // filter clients to relevant urls
        clients = clients.filter(c => new URL(c.url).pathname.indexof('/chat/') === 0);
        // tell them about the new messages
        clients.forEach(c => c.postMessage(data));

        // if we don't have a focused & visible window, show notification
        if (!clients.find(c => !c.hidden && c.focused)) {
          registration.showNotification(data[0].username, {
            body: data[0].message,
            icon: data[0].avatarURL,
            tag: 'chat-' + data[0].username
          });
        }
      });
    });
  }
});

Responding to a notification activation:

self.addEventListener('notificationclick', function(event) {
  // chat notification clicked
  if (event.notification.tag.indexOf('chat-') === 0) {
    clients.getAll().then(function(clients) {
      // look for relevent window, or make a new one
      var client = clients.find(c => new URL(c.url).pathname.indexof('/chat/') === 0 && c.type == 'window')
        || new Client('/chat/');

      // focus it
      client.focus();
    });
  }
};

@jakearchibald
Copy link
Contributor Author

@annevk:

It seems like an interesting idea to make Client available everywhere, I had not actually considered that. I was mostly suggesting to remove the "ServiceWorker" prefix since they were scoped to service workers anyway. But this makes sense if we want the ability to open new windows using new Client elsewhere.

Ohh, I thought that's what you were asking for. Happy to keep it ServiceWorker specific.

Also, the postMessage() setup only works if these objects are in a service worker since the other side is expected to use the ServiceWorker object, no?

If we're making this ServiceWorker specific, can we make postMessageEvent.source point back to the ServiceWorker it originated from?

I don't really like update. I thought the idea was to require polling given the expected longevity of a service worker?

I was looking to solve this issue:

client.focused; // false
client.focus().then(function() {
  client.focused; // still false
});

But perhaps "it's a snapshot, deal with it" is good enough. The focus method resolving is indication enough.

@jakearchibald
Copy link
Contributor Author

@domenic

It seems more likely you want a design where Client's constructor is of the form new Client(informationGivingAPointerToImplementationWindowOrWorkerRepresentation).

Given this, I'd much rather go with this and introduce something like clients.openWindow(url).

@domenic
Copy link
Contributor

domenic commented Dec 15, 2014

We can do subclassing but then we'd have to define DedicatedWorkerClient and SharedWorkerClient separately as well,

Why? BasicClient and WindowClient or similar seems like it would work.

and getAll() should resolve with a sequence holding a mix of those different types of objects. Would that be a better design for authors?

Yes, definitely.

@jakearchibald
Copy link
Contributor Author

If getAll returns different types, given that workers are still pretty rare, I'm worried that code would appear to work, go live, then start failing because a worker was present.

interface BasicClient {
  readonly attribute USVString url;
  readonly attribute ServiceWorkerRegistration? serviceWorkerRegistration;
  void postMessage(any message, optional sequence<Transferable> transfer);
}

interface WindowClient : BasicClient {
  readonly attribute VisibilityState visibilityState;
  readonly attribute boolean focused;
  readonly attribute ContextFrameType frameType;
  Promise<void> focus();
}

Assuming you mean the above, it isn't too bad right now. If your code encounters a BasicClient for the first time, .visibilityState != "visible", .focused != true, frameType != "top-level". If you call .focus() you're going to throw an error, that won't often be different from rejecting, but I don't know how much money I'd bet on that.

Adding stuff to WindowClient in future may be tough.

Assuming we went with the above, what API would you prefer for opening a window, considering a constructor here is undesirable? clients.openWindow(url), WindowClient.open(url)?

@domenic
Copy link
Contributor

domenic commented Dec 15, 2014

If getAll returns different types, given that workers are still pretty rare, I'm worried that code would appear to work, go live, then start failing because a worker was present.

Good point. Very interesting.

Is there a scenario when getAll() is useful without filtering by type? Your examples seem like they would benefit from a filter on window. Perhaps clients.getWindows() + clients.getAll() would be helpful for avoiding this?

Assuming we went with the above, what API would you prefer for opening a window, considering a constructor here is undesirable? clients.openWindow(url), WindowClient.open(url)?

If I'm understanding correctly, any new window clients will be added to the collection of all clients, right? If so, then I think the former seems a bit nicer, since clients.openWindow modifying the return value of clients.getAll makes more sense than WindowClient.open doing so.

@jakearchibald
Copy link
Contributor Author

Is there a scenario when getAll() is useful without filtering by type? Your examples seem like they would benefit from a filter on window.

There's already .getAll({ includeUncontrolled: true }), adding a type option that defaults to window seems pretty sensible, with other values being worker, shared-worker or all.

(includeUncontrolled, ugh, maybe includeUnclaimed if we're going for clients.claim())

@jungkees
Copy link
Collaborator

Incorporated the result of the discussion up to the point: 868eaea.

Summary:

  • Defined BasicClient, WindowClient (having no constructors in WebIDL terms)
  • Added clients.openWindow(url)
  • Defined ContextFrameType enum and ClientType enum
  • Updated clients.getAll(options) algorithm as such
  • ServiceWorkerClient is exposed to service worker only.
  • ServiceWorkerClient is now the type name of the typedef (BasicClient or WindowClient) ServiceWorkerClient. Didn't change the name to Client due to the resolution gotten from Rename the Client interface #350

@jakearchibald
Copy link
Contributor Author

@domenic .focus() should reject if the window couldn't be focused right? This can happen due to permission or the window having closed.

It seems like a slam-dunk exceptional case, but window.focus() fails silently.

@annevk
Copy link
Member

annevk commented Dec 16, 2014

No need for a ServiceWorkerClient typedef. You can just use BasicClient instead. #350 is old hat and obsolete given recent discussion, let's rename BasicClient to Client.

@jakearchibald
Copy link
Contributor Author

sgtm

@domenic
Copy link
Contributor

domenic commented Dec 16, 2014

@jungkees

Updated clients.getAll(options) algorithm as such

Also make sure the default is to return only window clients.

@jakearchibald

.focus() should reject if the window couldn't be focused right? This can happen due to permission or the window having closed.

Sounds good to me, unless

window.focus() fails silently.

is for security reasons or something.

@annevk

No need for a ServiceWorkerClient typedef. You can just use BasicClient instead

Interesting; didn't know Web IDL was covariant.

@jungkees
Copy link
Collaborator

No need for a ServiceWorkerClient typedef. You can just use BasicClient instead. #350 is old hat and obsolete given recent discussion, let's rename BasicClient to Client.

Addressed: 04d5734.

Updated clients.getAll(options) algorithm

Also make sure the default is to return only window clients.

Yes it does so. The default (without options attribute) returns only controlled window clients indeed.

@jungkees
Copy link
Collaborator

/cc @irori @KenjiBaheux

@mounirlamouri
Copy link
Member

A few follow up questions.

  • We are fine with code such as:
client.focus().then(function() {
  client.focused === false;
});

It's very odd to have a method that can change the state of an object on an object that is meant to be a snapshot.

  • WindowClient.focus() will fail in Chromium if it is not called inside a notificationclick event or called more than once. Do we still want to throw in that case? I'm afraid that it might break when reaching some edge cases like:
clients.getAll().then(function(clients) {
  clients.forEach(function(c) {
    c.focus();
  });
});

If this code is called from inside a notificationclick and there is only one WindowClient, it will work fine. If there are multiple WindowClient, it will throw.

  • Clients.openWindow() only takes an url and no options. It is fairly limited. Should we imitate window.open() API or should we improve that later for something more powerful but saner than window.open()?

@annevk
Copy link
Member

annevk commented Dec 19, 2014

focused changing would not make much sense, indeed. We should make it a true snapshot with the methods forwarding to the underlying object.

@mounirlamouri
Copy link
Member

What about having:

partial interface Clients {
  Promise<void> focusWindow(WindowClient);
};

We could even return a new WindowClient in the Promise. It feels less odd to me to have the method outside of the snapshot object.

@jakearchibald
Copy link
Contributor Author

WindowClient.focus() will fail in Chromium if it is not called inside a notificationclick event or called more than once. Do we still want to throw in that case?

It should reject rather than throw.

Clients.openWindow() only takes an url and no options. It is fairly limited. Should we imitate window.open() API

God no :D

Let's keep it simple for now, we can add to it later.

@jakearchibald
Copy link
Contributor Author

@mounirlamouri

Had a quick chat with @annevk in IRC. Thinking about usability, I think people will look for methods that act on the client, on the client object. How about:

client.focus().then(function(updatedClient) {
  updatedClient.focued === true;
})

Basically, your idea of returning an updated snapshot, but leaving the focus method on client.

@jakearchibald
Copy link
Contributor Author

Just had a chat with @mounirlamouri. To clarify, .focus and .openWindow should reject if they fail to complete, which would happen in cases such as:

  • Window has already closed, so cannot focus
  • Browser does not grant permission

When it comes to permission, it'll probably be a MAY in the spec. As in "If the call is not within the context of an interaction event, the browser MAY reject"

@inexorabletash
Copy link
Member

For openWindow there are cases where the window may indeed open but the resulting window is not controlled - e.g. the initial URL passes all the validation checks but there's a server side redirect or some other reason that the SW making the call doesn't end up controlling it. When I prototyped the code I had those result in rejection as well.

It wasn't clear we wanted to do more there (e.g. try and abort the navigation once we realize it's not going to be controlled, etc). I erred on the side of thinking of it as just a window.open() call without the window; if the result ends up controlled, that's awesome!

@jakearchibald
Copy link
Contributor Author

So @mounirlamouri had a great use-case. A news aggregation app may wish to show a notification about breaking news, which when clicked, opens the news website with the story. This would be another origin.

For other-origin URLs we can resolve with null, rather than a client object.

I don't think there's any problem opening a window outside the scope of the SW & same origin, we can return a client object for that.

@mounirlamouri
Copy link
Member

Should we close that issue and open specific ones if needed?

@jakearchibald
Copy link
Contributor Author

Yeah, will continue in #602

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

7 participants