Cammie Smith Barnes (Gerrit)

unread,

Jun 20, 2024, 5:27:21 PM (9 days ago) Jun 20

to Yao Xiao, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, Kentaro Hara, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, haoliu...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org, speed-metr...@chromium.org

Attention needed from Yao Xiao

Cammie Smith Barnes added 1 comment

Patchset-level comments

File-level comment, Patchset 4 (Latest):

Cammie Smith Barnes . resolved

yaoxia@, PTAL, thanks!

Open in Gerrit

Related details

Attention is currently required from:

Yao Xiao

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Yao Xiao (Gerrit)

unread,

Jun 20, 2024, 6:44:48 PM (9 days ago) Jun 20

to Cammie Smith Barnes, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, Kentaro Hara, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, haoliu...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org, speed-metr...@chromium.org

Attention needed from Cammie Smith Barnes

Yao Xiao voted and added 4 comments

Votes added by Yao Xiao

Code-Review

+1

4 comments

Patchset-level comments

File-level comment, Patchset 5 (Latest):

Yao Xiao . resolved

LGTM % comments

File third_party/blink/renderer/modules/shared_storage/shared_storage.cc

Line 883, Patchset 5 (Latest): credentials_mode_only_options, exception_state,

Yao Xiao . unresolved

I think you can pass `options` as before, as it will implicitly convert to the base class type (i.e. in the generated C++ code, `SharedStorageWorkletOptions` also inherits `WorkletOptions`).

File third_party/blink/renderer/modules/shared_storage/shared_storage_worklet.cc

Line 194, Patchset 5 (Latest):

    UseCounter::Count(
        To<LocalDOMWindow>(*execution_context).document(),

Yao Xiao . unresolved

Can we do `execution_context->CountUse(...)`?

Line 200, Patchset 5 (Latest): // TODO(348445878): Make the `dataOrigin` option (which is parsed into

Yao Xiao . unresolved

Should this be `crbug.com/348445878`? Or is your style also supported? (I haven't seen one)

Open in Gerrit

Related details

Attention is currently required from:

Cammie Smith Barnes

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Cammie Smith Barnes (Gerrit)

unread,

Jun 20, 2024, 7:10:02 PM (9 days ago) Jun 20

to Yao Xiao, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, Kentaro Hara, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, haoliu...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org, speed-metr...@chromium.org

Cammie Smith Barnes added 3 comments

File third_party/blink/renderer/modules/shared_storage/shared_storage.cc

Line 883, Patchset 5: credentials_mode_only_options, exception_state,

Yao Xiao . resolved

I think you can pass `options` as before, as it will implicitly convert to the base class type (i.e. in the generated C++ code, `SharedStorageWorkletOptions` also inherits `WorkletOptions`).

Cammie Smith Barnes

Done

File third_party/blink/renderer/modules/shared_storage/shared_storage_worklet.cc

Line 194, Patchset 5:

    UseCounter::Count(
        To<LocalDOMWindow>(*execution_context).document(),

Yao Xiao . resolved

Can we do `execution_context->CountUse(...)`?

Cammie Smith Barnes

Handy, thanks!

Line 200, Patchset 5: // TODO(348445878): Make the `dataOrigin` option (which is parsed into

Yao Xiao . resolved

Should this be `crbug.com/348445878`? Or is your style also supported? (I haven't seen one)

Cammie Smith Barnes

Done

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Cammie Smith Barnes (Gerrit)

unread,

Jun 20, 2024, 7:12:40 PM (9 days ago) Jun 20

to Sun Yueru, Yao Xiao, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, Kentaro Hara, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, haoliu...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org, speed-metr...@chromium.org

Attention needed from Sun Yueru

Cammie Smith Barnes added 1 comment

Patchset-level comments

File-level comment, Patchset 7 (Latest):

Cammie Smith Barnes . resolved

yrsun@, PTAL at ukm_features.cc, thanks!

Open in Gerrit

Related details

Attention is currently required from:

Sun Yueru

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Yao Xiao (Gerrit)

unread,

Jun 20, 2024, 7:17:36 PM (9 days ago) Jun 20

to Cammie Smith Barnes, Sun Yueru, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, Kentaro Hara, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, haoliu...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org, speed-metr...@chromium.org

Attention needed from Cammie Smith Barnes and Sun Yueru

Yao Xiao voted and added 2 comments

Votes added by Yao Xiao

Code-Review

+1

2 comments

File third_party/blink/renderer/modules/shared_storage/shared_storage.cc

Line 166, Patchset 5: return SharedStorageDataOrigin::kInvalid;

Yao Xiao . unresolved

Should this be `NOTREACHED_NORETURN`? I *think* IDL will convert to the default value if invalid values are provided. It'd be good to verify this though.

File third_party/blink/renderer/modules/shared_storage/shared_storage.idl

Line 76, Patchset 5: Exposed=Window,

Yao Xiao . unresolved

Suggest rebasing to get rid of this.

Open in Gerrit

Related details

Attention is currently required from:

Cammie Smith Barnes
Sun Yueru

Submit Requirements:

Code-Coverage
Code-Review

No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Cammie Smith Barnes (Gerrit)

unread,

Jun 20, 2024, 7:25:07 PM (9 days ago) Jun 20

to Sun Yueru, Yao Xiao, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, Kentaro Hara, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, haoliu...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org, speed-metr...@chromium.org

Attention needed from Cammie Smith Barnes and Sun Yueru

Cammie Smith Barnes added 1 comment

Patchset-level comments

File-level comment, Patchset 7 (Latest):

Cammie Smith Barnes . resolved

yrsun@, I decided to split into 2 CLs, will move you off of review here and then onto review for the other one

Open in Gerrit

Related details

Attention is currently required from:

Cammie Smith Barnes
Sun Yueru

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Cammie Smith Barnes (Gerrit)

unread,

Jun 20, 2024, 7:45:33 PM (9 days ago) Jun 20

to Sun Yueru, Yao Xiao, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, Kentaro Hara, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, haoliu...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org, speed-metr...@chromium.org

Attention needed from Sun Yueru and Yao Xiao

Cammie Smith Barnes added 1 comment

File third_party/blink/renderer/modules/shared_storage/shared_storage.cc

Line 166, Patchset 5: return SharedStorageDataOrigin::kInvalid;

Yao Xiao . unresolved

Should this be `NOTREACHED_NORETURN`? I *think* IDL will convert to the default value if invalid values are provided. It'd be good to verify this though.

Cammie Smith Barnes

Revised.

I will also add a web test for this specifically, still working on that.

Open in Gerrit

Related details

Attention is currently required from:

Sun Yueru
Yao Xiao

Submit Requirements:

Code-Coverage
Code-Review

No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Cammie Smith Barnes (Gerrit)

unread,

Jun 20, 2024, 8:51:52 PM (9 days ago) Jun 20

to AyeAye, Sun Yueru, Yao Xiao, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, Kentaro Hara, blink-revie...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, haoliu...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org, speed-metr...@chromium.org

Attention needed from Sun Yueru and Yao Xiao

Cammie Smith Barnes voted and added 3 comments

Votes added by Cammie Smith Barnes

Commit-Queue

+1

3 comments

Patchset-level comments

File-level comment, Patchset 13 (Latest):

Cammie Smith Barnes . resolved

yaoxia@, Could you please take a look at my new web tests?

Also, I split the UKM feature to a separate CL. Thanks.

File third_party/blink/renderer/modules/shared_storage/shared_storage.cc

Line 166, Patchset 5: return SharedStorageDataOrigin::kInvalid;

Yao Xiao . resolved

Should this be `NOTREACHED_NORETURN`? I *think* IDL will convert to the default value if invalid values are provided. It'd be good to verify this though.

Cammie Smith Barnes

Revised.
I will also add a web test for this specifically, still working on that.

Cammie Smith Barnes

It turns out that an error is thrown if an invalid value is entered. See my new web tests.

This is desirable, however, for feature detection: https://github.com/WICG/shared-storage/pull/158#issuecomment-2168827866

Marking as resolved, but please re-open if you wish to discuss further. 😊

File third_party/blink/renderer/modules/shared_storage/shared_storage.idl

Line 76, Patchset 5: Exposed=Window,

Yao Xiao . resolved

Suggest rebasing to get rid of this.

Cammie Smith Barnes

Done

Open in Gerrit

Related details

Attention is currently required from:

Sun Yueru
Yao Xiao

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Yao Xiao (Gerrit)

unread,

Jun 20, 2024, 9:01:30 PM (9 days ago) Jun 20

to Cammie Smith Barnes, AyeAye, Sun Yueru, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, Kentaro Hara, blink-revie...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, haoliu...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org, speed-metr...@chromium.org

Attention needed from Cammie Smith Barnes and Sun Yueru

Yao Xiao voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Cammie Smith Barnes
Sun Yueru

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Blink W3C Test Autoroller (Gerrit)

unread,

Jun 20, 2024, 9:07:34 PM (9 days ago) Jun 20

to Cammie Smith Barnes, Yao Xiao, AyeAye, Sun Yueru, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, Kentaro Hara, blink-revie...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, haoliu...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org, speed-metr...@chromium.org

Attention needed from Cammie Smith Barnes and Sun Yueru

Message from Blink W3C Test Autoroller

Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/46853.

When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.

WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process

Open in Gerrit

Related details

Attention is currently required from:

Cammie Smith Barnes
Sun Yueru

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Sun Yueru (Gerrit)

unread,

Jun 21, 2024, 11:30:33 AM (8 days ago) Jun 21

to Cammie Smith Barnes, Blink W3C Test Autoroller, Yao Xiao, AyeAye, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, Kentaro Hara, blink-revie...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, haoliu...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org, speed-metr...@chromium.org

Attention needed from Cammie Smith Barnes

Sun Yueru added 1 comment

Patchset-level comments

File-level comment, Patchset 7:

Cammie Smith Barnes . resolved

yrsun@, I decided to split into 2 CLs, will move you off of review here and then onto review for the other one

Sun Yueru

Sg, approved the other one.

Open in Gerrit

Related details

Attention is currently required from:

Cammie Smith Barnes

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Cammie Smith Barnes (Gerrit)

unread,

Jun 21, 2024, 12:46:43 PM (8 days ago) Jun 21

to Blink W3C Test Autoroller, Yao Xiao, AyeAye, Sun Yueru, Tricium, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, Kentaro Hara, blink-revie...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, haoliu...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org, speed-metr...@chromium.org

Cammie Smith Barnes voted Commit-Queue+2

Commit-Queue

+2

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

Jun 21, 2024, 12:50:34 PM (8 days ago) Jun 21

to Cammie Smith Barnes, Blink W3C Test Autoroller, Yao Xiao, AyeAye, Sun Yueru, Tricium, Chromium Metrics Reviews, chromium...@chromium.org, Kentaro Hara, blink-revie...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, haoliu...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org, speed-metr...@chromium.org

Chromium LUCI CQ submitted the change

Change information

Commit message:

Shared Storage: Add non-live dataOption to createWorklet for use counter

We proposed a breaking change to sharedStorage.createWorklet() in
https://github.com/WICG/shared-storage/pull/158.

We add a use counter for how frequently createWorklet is called in a
non-forward-compatible way according to the proposal (i.e. with a
cross-origin script but without the option dataOrigin: "script-
origin"). This will help us determine what the impact of this change
would be.

Bug: 348445878

Change-Id: I3b882a9ec859beb5265ba31d3169fb5e1239cac4

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5645568

Reviewed-by: Yao Xiao <yao...@chromium.org>

Commit-Queue: Cammie Smith Barnes <cam...@chromium.org>

Cr-Commit-Position: refs/heads/main@{#1318000}

Files:

M third_party/blink/public/mojom/use_counter/metrics/web_feature.mojom
M third_party/blink/renderer/bindings/generated_in_modules.gni
M third_party/blink/renderer/bindings/idl_in_modules.gni
M third_party/blink/renderer/modules/shared_storage/shared_storage.cc
M third_party/blink/renderer/modules/shared_storage/shared_storage.h
M third_party/blink/renderer/modules/shared_storage/shared_storage.idl
M third_party/blink/renderer/modules/shared_storage/shared_storage_worklet.cc
M third_party/blink/renderer/modules/shared_storage/shared_storage_worklet.h
A third_party/blink/renderer/modules/shared_storage/shared_storage_worklet_options.idl
M third_party/blink/renderer/modules/shared_storage/util.h
A third_party/blink/web_tests/external/wpt/shared-storage/cross-origin-create-worklet-data-origin-option.tentative.https.sub.html
M third_party/blink/web_tests/external/wpt/shared-storage/resources/util.sub.js
A third_party/blink/web_tests/external/wpt/shared-storage/same-origin-create-worklet-data-origin-option.tentative.https.sub.html
M tools/metrics/histograms/enums.xml

Change size: M

Delta: 14 files changed, 194 insertions(+), 14 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Yao Xiao

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Blink W3C Test Autoroller (Gerrit)

unread,

Jun 21, 2024, 1:49:31 PM (8 days ago) Jun 21

to Cammie Smith Barnes, Chromium LUCI CQ, Yao Xiao, AyeAye, Sun Yueru, Tricium, Chromium Metrics Reviews, chromium...@chromium.org, Kentaro Hara, blink-revie...@chromium.org, asvitkine...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, bmcquad...@chromium.org, csharris...@chromium.org, haoliu...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org, speed-metr...@chromium.org

Message from Blink W3C Test Autoroller

The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/46853

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Reply all

Reply to author

Forward

Shared Storage: Add non-live dataOption to createWorklet for use counter [chromium/src : main]

Cammie Smith Barnes (Gerrit)

Cammie Smith Barnes added 1 comment

Related details

Yao Xiao (Gerrit)

Yao Xiao voted and added 4 comments

Votes added by Yao Xiao

4 comments

Related details

Cammie Smith Barnes (Gerrit)

Cammie Smith Barnes added 3 comments

Related details

Cammie Smith Barnes (Gerrit)

Cammie Smith Barnes added 1 comment

Related details

Yao Xiao (Gerrit)

Yao Xiao voted and added 2 comments

Votes added by Yao Xiao

2 comments

Related details

Cammie Smith Barnes (Gerrit)

Cammie Smith Barnes added 1 comment

Related details

Cammie Smith Barnes (Gerrit)

Cammie Smith Barnes added 1 comment

Related details

Cammie Smith Barnes (Gerrit)

Cammie Smith Barnes voted and added 3 comments

Votes added by Cammie Smith Barnes

3 comments

Related details

Yao Xiao (Gerrit)

Yao Xiao voted Code-Review+1

Related details

Blink W3C Test Autoroller (Gerrit)

Message from Blink W3C Test Autoroller

Related details

Sun Yueru (Gerrit)

Sun Yueru added 1 comment

Related details

Cammie Smith Barnes (Gerrit)

Cammie Smith Barnes voted Commit-Queue+2

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change

Change information

Blink W3C Test Autoroller (Gerrit)

Message from Blink W3C Test Autoroller

Related details