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

Move name up from SharedWorkerGlobalScope to WorkerGlobalScope, add corresponding argument to Worker() #2477

Closed
hober opened this Issue Mar 28, 2017 · 10 comments

Comments

6 participants
@hober
Collaborator

hober commented Mar 28, 2017

Since websites may spawn multiple dedicated workers with the same script URL, it would be nice if each could be given a name so they can be usefully disambiguated in debugging tools. See the related WebKit bug.

@hober

This comment has been minimized.

Show comment
Hide comment
@hober
Collaborator

hober commented Mar 28, 2017

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Mar 29, 2017

Member

That would also end up affecting service workers. Probably does not make sense there, @wanderview @jungkees?

The other thing is that we recently added WorkerOptions as second argument for dedicated workers and third argument for shared workers. If we added name as member there for dedicated workers that would make the APIs somewhat asymmetric. WorkerOptions is perhaps not implemented though (not sure anyone is doing that part of JavaScript modules) so maybe we can still change things around.

Member

annevk commented Mar 29, 2017

That would also end up affecting service workers. Probably does not make sense there, @wanderview @jungkees?

The other thing is that we recently added WorkerOptions as second argument for dedicated workers and third argument for shared workers. If we added name as member there for dedicated workers that would make the APIs somewhat asymmetric. WorkerOptions is perhaps not implemented though (not sure anyone is doing that part of JavaScript modules) so maybe we can still change things around.

@wanderview

This comment has been minimized.

Show comment
Hide comment
@wanderview

wanderview Mar 29, 2017

Member

Overall seems reasonable to me.

In regards to the webkit issue's example code:

    var worker1 = new Worker("worker.js")
    worker1.name = "Foo Worker";

    var worker2 = new Worker("worker.js");
    worker2.name = "Bar Worker";

It might be a bit nicer to make this a constructor option and then a readonly attribute. Immutable data is a bit easier to work with in our worker implementation.

In regards to service workers, we have internally actually mapped the service worker scope to the worker name. We don't expose that to script, though. Not sure if that is worth doing as a default name.

Member

wanderview commented Mar 29, 2017

Overall seems reasonable to me.

In regards to the webkit issue's example code:

    var worker1 = new Worker("worker.js")
    worker1.name = "Foo Worker";

    var worker2 = new Worker("worker.js");
    worker2.name = "Bar Worker";

It might be a bit nicer to make this a constructor option and then a readonly attribute. Immutable data is a bit easier to work with in our worker implementation.

In regards to service workers, we have internally actually mapped the service worker scope to the worker name. We don't expose that to script, though. Not sure if that is worth doing as a default name.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Apr 13, 2017

Member

@mattto any thoughts from Blink? @travisleithead Edge?

Constructor option (as part of the WorkerOptions dictionary) sounds like a good idea to me.

Not sure what to do here for service workers; I'll leave that in the hands of the service worker spec people.

Member

domenic commented Apr 13, 2017

@mattto any thoughts from Blink? @travisleithead Edge?

Constructor option (as part of the WorkerOptions dictionary) sounds like a good idea to me.

Not sure what to do here for service workers; I'll leave that in the hands of the service worker spec people.

@mattto

This comment has been minimized.

Show comment
Hide comment
@mattto

mattto Apr 17, 2017

Contributor

I don't have a strong opinion but this sounds reasonable. @kinu @nhiroki

Should the API enforce the uniqueness of the name?

For service workers, the spec calls for an id per service worker which additionally is unique within a scope. However there is talk of removing that from the spec w3c/ServiceWorker#1076

It feels like this should be aligned with the service worker spec somehow. @jakearchibald

Contributor

mattto commented Apr 17, 2017

I don't have a strong opinion but this sounds reasonable. @kinu @nhiroki

Should the API enforce the uniqueness of the name?

For service workers, the spec calls for an id per service worker which additionally is unique within a scope. However there is talk of removing that from the spec w3c/ServiceWorker#1076

It feels like this should be aligned with the service worker spec somehow. @jakearchibald

@kinu

This comment has been minimized.

Show comment
Hide comment
@kinu

kinu Apr 17, 2017

Sounds reasonable to me too, and constructor option looks good.

Agreed that it'd be nice to have an aligned story for Service Workers, we can expose something as a default (like 'id' thing), while a question there would be if we want to allow developers to specify other names (and if so how).

kinu commented Apr 17, 2017

Sounds reasonable to me too, and constructor option looks good.

Agreed that it'd be nice to have an aligned story for Service Workers, we can expose something as a default (like 'id' thing), while a question there would be if we want to allow developers to specify other names (and if so how).

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Apr 17, 2017

Member

Hmm, OK, we should probably coordinate with service workers indeed. I filed w3c/ServiceWorker#1121. I hope we can get people to weigh in there quickly so as to unblock this approach.

One concern I had here is that people might expect the semantics of name for dedicated workers to be similar to those of shared worker, where if you supply the same name, it somehow shares the worker. I guess it's probably fine though? I dunno, should we throw an error if you supply a non-unique name for dedicated workers?

The other thing is that we recently added WorkerOptions as second argument for dedicated workers and third argument for shared workers. If we added name as member there for dedicated workers that would make the APIs somewhat asymmetric. WorkerOptions is perhaps not implemented though (not sure anyone is doing that part of JavaScript modules) so maybe we can still change things around.

I also noticed this. I think if we do this we should add an overload to SharedWorker that makes it (url, options) where options contains a name, and get rid of (url, name, options).

Member

domenic commented Apr 17, 2017

Hmm, OK, we should probably coordinate with service workers indeed. I filed w3c/ServiceWorker#1121. I hope we can get people to weigh in there quickly so as to unblock this approach.

One concern I had here is that people might expect the semantics of name for dedicated workers to be similar to those of shared worker, where if you supply the same name, it somehow shares the worker. I guess it's probably fine though? I dunno, should we throw an error if you supply a non-unique name for dedicated workers?

The other thing is that we recently added WorkerOptions as second argument for dedicated workers and third argument for shared workers. If we added name as member there for dedicated workers that would make the APIs somewhat asymmetric. WorkerOptions is perhaps not implemented though (not sure anyone is doing that part of JavaScript modules) so maybe we can still change things around.

I also noticed this. I think if we do this we should add an overload to SharedWorker that makes it (url, options) where options contains a name, and get rid of (url, name, options).

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Apr 18, 2017

Member

And whoever gets to answer the question about uniqueness, please qualify it with scope if it needs to be unique: unique within an agent cluster or unique within the browser.

Member

annevk commented Apr 18, 2017

And whoever gets to answer the question about uniqueness, please qualify it with scope if it needs to be unique: unique within an agent cluster or unique within the browser.

domenic added a commit that referenced this issue May 10, 2017

@domenic domenic self-assigned this May 10, 2017

@domenic domenic closed this in #2664 May 12, 2017

domenic added a commit that referenced this issue May 12, 2017

Add a name to dedicated workers, for debugging
Closes #2477. This also changes the SharedWorker constructor to allow
passing the name via an options bag, e.g. SharedWorker(url, { name }),
in addition to directly as a string. This new url-plus-options form
supersedes the previous signature of SharedWorker(url, name, options),
where previously options was only used for module-worker-related
options, which haven't shipped in implementations yet.

Tests: web-platform-tests/wpt#5880
@wanderview

This comment has been minimized.

Show comment
Hide comment
@wanderview

wanderview Jun 22, 2017

Member

We have a compat problem with our implementation of this:

https://bugzilla.mozilla.org/show_bug.cgi?id=1375457

It seems like we need to make the property [replaceable].

Member

wanderview commented Jun 22, 2017

We have a compat problem with our implementation of this:

https://bugzilla.mozilla.org/show_bug.cgi?id=1375457

It seems like we need to make the property [replaceable].

domenic added a commit that referenced this issue Jun 22, 2017

Make self.name in workers [Replaceable]
This is a follow-up to #2477, to
address the compatibility problem discovered in
https://bugzilla.mozilla.org/show_bug.cgi?id=1375457.
@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Jun 22, 2017

Member

I posted a spec fix: #2783.

Member

domenic commented Jun 22, 2017

I posted a spec fix: #2783.

domenic added a commit that referenced this issue Jun 22, 2017

Make self.name in workers [Replaceable]
This is a follow-up to #2477, to
address the compatibility problem discovered in
https://bugzilla.mozilla.org/show_bug.cgi?id=1375457.

hubot pushed a commit to WebKit/webkit that referenced this issue Aug 21, 2018

yusukesuzuki@slowstart.org
Support "name" option for dedicated workers
https://bugs.webkit.org/show_bug.cgi?id=188779

Reviewed by Joseph Pecoraro.

LayoutTests/imported/w3c:

* web-platform-tests/workers/interfaces.worker-expected.txt:
* web-platform-tests/workers/name-property-expected.txt:

Source/WebCore:

This patch adds `new Worker(url, { name: "Worker Name" })` option support[1].
This name can be accessible from `self.name` of DedicatedWorkerGlobalScope.
It is useful for debugging dedicated workers if the inspector can show the
names of the workers. This enhancement is tracked by [2].

[1]: whatwg/html#2477
[2]: https://bugs.webkit.org/show_bug.cgi?id=164678

* workers/DedicatedWorkerGlobalScope.cpp:
(WebCore::DedicatedWorkerGlobalScope::create):
(WebCore::DedicatedWorkerGlobalScope::DedicatedWorkerGlobalScope):
Hold `name` member.

* workers/DedicatedWorkerGlobalScope.h:
* workers/DedicatedWorkerGlobalScope.idl:
Add `name` attribute.

* workers/DedicatedWorkerThread.cpp:
(WebCore::DedicatedWorkerThread::DedicatedWorkerThread):
(WebCore::DedicatedWorkerThread::createWorkerGlobalScope):
* workers/DedicatedWorkerThread.h:
* workers/Worker.cpp:
(WebCore::Worker::Worker):
(WebCore::Worker::create):
(WebCore::Worker::notifyFinished):
* workers/Worker.h:
* workers/Worker.idl:
Add WorkerOptions for dedicated worker creation.

* workers/WorkerGlobalScopeProxy.h:
* workers/WorkerMessagingProxy.cpp:
(WebCore::WorkerMessagingProxy::startWorkerGlobalScope):
* workers/WorkerMessagingProxy.h:
* workers/WorkerThread.cpp:
(WebCore::WorkerThreadStartupData::WorkerThreadStartupData):
Isolate copy the given `name` to pass the worker thread.

(WebCore::WorkerThread::WorkerThread):
(WebCore::WorkerThread::workerThread):
* workers/WorkerThread.h:
* workers/service/context/ServiceWorkerThread.cpp:
(WebCore::ServiceWorkerThread::ServiceWorkerThread):
(WebCore::ServiceWorkerThread::createWorkerGlobalScope):
* workers/service/context/ServiceWorkerThread.h:

LayoutTests:

* http/wpt/workers/name-property-enhanced-expected.txt: Added.
* http/wpt/workers/name-property-enhanced.html: Added.
* http/wpt/workers/support/name.js: Added.
(test):
* http/wpt/workers/support/no-name.js: Added.
(test):

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@235128 268f45cc-cd09-0410-ab3c-d52691b4dbfc

hubot pushed a commit to WebKit/webkit that referenced this issue Aug 22, 2018

yusukesuzuki@slowstart.org
Support "name" option for dedicated workers
https://bugs.webkit.org/show_bug.cgi?id=188779

Reviewed by Joseph Pecoraro.

LayoutTests/imported/w3c:

* web-platform-tests/workers/interfaces.worker-expected.txt:
* web-platform-tests/workers/name-property-expected.txt:

Source/WebCore:

This patch adds `new Worker(url, { name: "Worker Name" })` option support[1].
This name can be accessible from `self.name` of DedicatedWorkerGlobalScope.
It is useful for debugging dedicated workers if the inspector can show the
names of the workers. This enhancement is tracked by [2].

[1]: whatwg/html#2477
[2]: https://bugs.webkit.org/show_bug.cgi?id=164678

Tests: http/wpt/workers/name-property-enhanced.html
       http/wpt/workers/name-property-no-name.html

* workers/DedicatedWorkerGlobalScope.cpp:
(WebCore::DedicatedWorkerGlobalScope::create):
(WebCore::DedicatedWorkerGlobalScope::DedicatedWorkerGlobalScope):
Hold `name` member.

* workers/DedicatedWorkerGlobalScope.h:
* workers/DedicatedWorkerGlobalScope.idl:
Add `name` attribute.

* workers/DedicatedWorkerThread.cpp:
(WebCore::DedicatedWorkerThread::DedicatedWorkerThread):
(WebCore::DedicatedWorkerThread::createWorkerGlobalScope):
* workers/DedicatedWorkerThread.h:
* workers/Worker.cpp:
(WebCore::Worker::Worker):
(WebCore::Worker::create):
(WebCore::Worker::notifyFinished):
* workers/Worker.h:
* workers/Worker.idl:
Add WorkerOptions for dedicated worker creation.

* workers/WorkerGlobalScopeProxy.h:
* workers/WorkerMessagingProxy.cpp:
(WebCore::WorkerMessagingProxy::startWorkerGlobalScope):
* workers/WorkerMessagingProxy.h:
* workers/WorkerThread.cpp:
(WebCore::WorkerThreadStartupData::WorkerThreadStartupData):
Isolate copy the given `name` to pass the worker thread.

(WebCore::WorkerThread::WorkerThread):
(WebCore::WorkerThread::workerThread):
* workers/WorkerThread.h:
* workers/service/context/ServiceWorkerThread.cpp:
(WebCore::ServiceWorkerThread::ServiceWorkerThread):
(WebCore::ServiceWorkerThread::createWorkerGlobalScope):
* workers/service/context/ServiceWorkerThread.h:

LayoutTests:

* http/wpt/workers/name-property-enhanced-expected.txt: Added.
* http/wpt/workers/name-property-enhanced.html: Added.
* http/wpt/workers/name-property-no-name-expected.txt: Added.
* http/wpt/workers/name-property-no-name.html: Added.
* http/wpt/workers/support/name.js: Added.
(test):
* http/wpt/workers/support/no-name.js: Added.
(test):

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@235159 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment