Skip to content

Add Worklet to JavaScript agent clusters #4069

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Oct 15, 2018
Merged

Add Worklet to JavaScript agent clusters #4069

merged 8 commits into from
Oct 15, 2018

Conversation

hoch
Copy link
Contributor

@hoch hoch commented Oct 8, 2018 •
edited by pr-preview bot
Loading

Fixes w3c/css-houdini-drafts#380. Mostly it expands the current text to include Worklet agent to the clusters.

  • Define what other agents and worklet agents can share memory with.
  • Remove the red box about worklet agent.
  • Add an example to the example box.

This PR was drafted from the discussion here.

cc @domenic

/infrastructure.html ( diff )
/webappapis.html ( diff )

domenic
domenic requested changes Oct 8, 2018
View reviewed changes
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.

Right direction, a few things to fix.

source Outdated
@@ -88624,14 +88624,14 @@ import "https://example.com/foo/../module2.mjs";</code></pre>
set</span> contains an item whose <span data-x="concept-relevant-realm">relevant Realm</span>
belongs to <var>agent</var>.</p>

<p>A <span>similar-origin window agent</span> can share memory with any <span>worklet agent
</span> whose single <span data-x="JavaScript realm">realm</span>'s
Copy link
Member

Choose a reason for hiding this comment

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

No space and newline before the </span>. We're linking "worklet agent", not "worklet agent\n ".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

source Outdated
@@ -88624,14 +88624,14 @@ import "https://example.com/foo/../module2.mjs";</code></pre>
set</span> contains an item whose <span data-x="concept-relevant-realm">relevant Realm</span>
belongs to <var>agent</var>.</p>

<p>A <span>similar-origin window agent</span> can share memory with any <span>worklet agent
</span> whose single <span data-x="JavaScript realm">realm</span>'s
<span data-x="concept-realm-global">global object</span>'s <span>owner set</span>'s
Copy link
Member

Choose a reason for hiding this comment

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

WorkletGlobalScopes don't have owner sets; they have owner documents. You'll want to change the text, and introduce a cross-spec reference around line 4125ish ("The following feature is defined in the Worklets specification").

See https://github.com/whatwg/wattsi/blob/master/Syntax.md#cross-specification-cross-references for more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

Choose a reason for hiding this comment

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

The definition should be in the definitions section, and the reference to it should be here. I'll push a commit so you can see what I mean for next time :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to know. Thanks for the review!

@domenic domenic added topic: multiple globals needs tests Moving the issue forward requires someone to write tests labels Oct 8, 2018
hoch
hoch commented Oct 8, 2018
View reviewed changes
Copy link
Contributor Author

@hoch hoch left a comment

Choose a reason for hiding this comment

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

I addressed your comments. Thanks!

Still unsure how to write a web-platform-test for this. WDYT @binji?

@binji
Copy link

binji commented Oct 8, 2018

A success test should be pretty close to the repro here: https://bugs.chromium.org/p/chromium/issues/detail?id=892067. Not sure about a failure test, though.

domenic
domenic approved these changes Oct 9, 2018
View reviewed changes
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 pending tests!

@hoch
Copy link
Contributor Author

hoch commented Oct 9, 2018

Not sure about the order of tasks. The implementation change needs the spec change, but the spec change here is waiting for tests (which will be a part of the CL)? Or do we need another reviewer to come and take a look for the actual merge?

@domenic
Copy link
Member

domenic commented Oct 9, 2018

You can land implementation and test changes based off of spec PRs, not just final specs. Especially if they're approved spec PRs :).

annevk
annevk reviewed Oct 10, 2018
View reviewed changes
Copy link
Member

@annevk annevk 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 that nit. This also needs implementer bugs, though maybe only Chrome and Firefox are somewhat affected here?

source Outdated
@@ -88624,14 +88625,15 @@ import "https://example.com/foo/../module2.mjs";</code></pre>
set</span> contains an item whose <span data-x="concept-relevant-realm">relevant Realm</span>
belongs to <var>agent</var>.</p>

<p>A <span>similar-origin window agent</span> can share memory with any <span>worklet agent</span>
Copy link
Member

Choose a reason for hiding this comment

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

This should link "can share memory with", no?

@hoch
Copy link
Contributor Author

hoch commented Oct 10, 2018 •
edited
Loading

@annevk You meant that the bug should be noted somewhere in the spec? FWIW, Chrome's tracking bug and the patch with a test.

@domenic
Copy link
Member

domenic commented Oct 10, 2018

@hoch as part of https://whatwg.org/working-mode#changes we require filing bugs on all affected implementers, either before or shortly after merging. (@annevk tends to prefer before.) But, I'm not sure which implementers are affected---do others besides Chrome implement AudioWorklet? If so we should file a bug on their tracker; see https://github.com/whatwg/meta/blob/master/MAINTAINERS.md#handling-pull-requests for more info.

@hoch
Copy link
Contributor Author

hoch commented Oct 10, 2018 •
edited
Loading

@annevk @domenic Thanks for the pointer. Only Chrome has the Audio Worklet implementation at this moment, so I think we got that covered in the crbug entry above. If this is not enough, please let me know.

TimothyGu
TimothyGu reviewed Oct 10, 2018
View reviewed changes
annevk
annevk approved these changes Oct 11, 2018
View reviewed changes
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Thanks, looks good! (And no need for a Firefox bug. Guess it's just tests at this point.)

@hoch
Copy link
Contributor Author

hoch commented Oct 15, 2018

I think we flipped all the bits. Can we merge this PR now?

@domenic domenic removed the needs tests Moving the issue forward requires someone to write tests label Oct 15, 2018
@domenic domenic merged commit bddeb25 into whatwg:master Oct 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimothyGu TimothyGu TimothyGu left review comments

@domenic domenic domenic approved these changes

@annevk annevk annevk approved these changes

Assignees
No one assigned
Labels
topic: multiple globals
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hoch @binji @domenic @TimothyGu @annevk