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

Editorial: restructure ownership of workers to parent-owners #2520

Merged
merged 3 commits into from
Apr 14, 2017

Conversation

annevk
Copy link
Member

@annevk annevk commented Apr 11, 2017

Instead of each worker being owned by a document, have it instead be
owned by one or more of its parents (more only in the case of
SharedWorkerGlobalScope).

This makes it possible to define JavaScript agent clusters (#2260) and
also helps with allowing service workers to have nested workers (#411).

This is marked editorial as it should have no normative impact.

This also removes a step that should have been removed as part of
4e2b006.

Instead of each worker being owned by a document, have it instead be
owned by one or more of its parents (more only in the case of
SharedWorkerGlobalScope).

This makes it possible to define JavaScript agent clusters (#2260) and
also helps with allowing service workers to have nested workers (#411).

This is marked editorial as it should have no normative impact.

This also removes a step that should have been removed as part of
4e2b006.
@annevk
Copy link
Member Author

annevk commented Apr 11, 2017

@wanderview @bakulf I'd appreciate review. I didn't go for changing the owner from document to Window at this time, but this makes such a change easier to evaluate going forward.

The main reason I'm making this change is to unblock SharedArrayBuffer.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with some nits, thanks so much.

source Outdated
@@ -79134,8 +79135,8 @@ callback <dfn>FrameRequestCallback</dfn> = void (<span>DOMHighResTimeStamp</span
</ol>

<p class="note">Whenever a <code>Document</code> object is <span data-x="discard a
Document">discarded</span>, it is also removed from the list of <span>the worker's
<code>Document</code>s</span> of each worker whose list contains that <code>Document</code>.</p>
Document">discarded</span>, it is also removed from the list of <span>the worker's owners</span>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I think better linking text would be "owners" here, although I recognize you're just going with the existing pattern. But as seen in the agents PR, I think it gets really ugly the more it's used.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I could make these all more clearly slot-like things of the WorkerGlobalScope instead of the double naming. But then we'd also have to change "the worker's ports" and maybe some other things too. I went for a smaller change, but happy to go more ambitious.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see. I guess being inconsistent would be not great. Up to you; we can leave it for later. In which case ignore a lot of my reviews here and in the other PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that I did set up exporting for the term I expect we'll eventually settle on.

source Outdated

<p>Each <code>WorkerGlobalScope</code> object also has a <span>set</span> of <dfn
id="the-worker's-workers" data-export="" data-for="WorkerGlobalScope" data-lt="workers">the
worker's workers</dfn>. Initially this set is empty; it is populated when the worker creates or
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading this sentence made the grammar situation clear for me:

  • Workers have owners/workers
  • WorkerGlobalScopes have "the worker's owners"/"the worker's workers"

I apologize if I gave feedback on the agents PR that didn't fit with the above framework.

source Outdated
owners</dfn>. Initially this set is empty; it is populated when the worker is created or
obtained.</p>

<p class="note">It is a list to accomodate <code>SharedWorkerGlobalScope</code> objects.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/list/set, instead of a single owner,/ maybe

source Outdated
Otherwise, <var>o</var> specifies a <span data-x="concept-settings-object-global">global
object</span> that is a <code>Window</code> object, and the relevant <code>Document</code> is just
the <span>responsible document</span> specified by <var>o</var>.</p>
it must be removed from the list of <span>the worker's owners</span> of each worker whose list
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: so here this can be "from the owners of each worker..."

source Outdated
<p>A worker is said to be a <dfn>permissible worker</dfn> if its list of <span>the worker's
<code>Document</code>s</span> is not empty, or if its list has been empty for no more than a short
<p>A worker is said to be a <dfn>permissible worker</dfn> if its set of <span>the worker's
owners</span> is not empty, or if its list has been empty for no more than a short
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: "set of owners"

source Outdated

<hr>

<p>A worker is said to be a <dfn>permissible worker</dfn> if its list of <span>the worker's
<code>Document</code>s</span> is not empty, or if its list has been empty for no more than a short
<p>A worker is said to be a <dfn>permissible worker</dfn> if its set of <span>the worker's
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While here refactoring this to a bulleted list ("all of the following are true") would greatly improve clarity

source Outdated
<p>A worker is said to be an <dfn>active needed worker</dfn> if any of the <code>Document</code>
objects in <span>the worker's <code>Document</code>s</span> are <span>fully active</span>.</p>
<p>A worker is said to be an <dfn>active needed worker</dfn> if any of <span>the worker's
owners</span> are either <code>Document</code> objects that are <span>fully active</span> or
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: "its owners"

source Outdated
settings</var>.</p></li>
<li><p><span data-x="set append">Append</span> the <span>relevant owner to add</span> given
<var>outside settings</var> to <var>worker global scope</var>'s set of <span>the worker's
owners</span>.</p></li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't moving from a list of relevant documents to add to a single owner a normative change? (One that aligns with reality, I imagine.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. Before it could be a list because an ancestor owner might be a SharedWorkerGlobal object with multiple document owners and those documents would just trickle down. Now we go through the hierarchy.

@annevk
Copy link
Member Author

annevk commented Apr 13, 2017

I went ahead and renamed them them "owner set" and "worker set" anyway. I found out that if I moved them into the WorkerGlobalScope section that would not look so weird and "the worker's ports" just remains as an item for future cleanup in the lifetime section (and probably also needs some further clarification since it's not a set that's explicitly being added too at the moment, though that might be rather easy to change if we define MessagePort allocation).

@domenic
Copy link
Member

domenic commented Apr 13, 2017

I pushed a small commit that links to and uses more Infra terms while we're in the area. LGTM!

@annevk annevk merged commit 59a4750 into master Apr 14, 2017
@annevk annevk deleted the annevk/worker-ownership branch April 14, 2017 05:48
sideshowbarker added a commit to w3c/ServiceWorker that referenced this pull request Apr 19, 2017
sideshowbarker added a commit to w3c/ServiceWorker that referenced this pull request Jun 11, 2017
jungkees pushed a commit to w3c/ServiceWorker that referenced this pull request Jun 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants