[spec] Support saved queries for selectURL #188

pythagoraskitty · 2024-09-10T19:07:13Z

We update the spec to support saved queries, as a followup to #176.

pythagoraskitty · 2024-09-10T20:14:18Z

@xyaoinum Could you please take a first pass? I won't land until after #176 lands and after I have a spec mentor review also. Thanks!

xyaoinum

LGTM % a nit

spec.bs

pythagoraskitty · 2024-09-12T18:02:40Z

@wanderview Please take a look, thanks!

spec.bs

wanderview · 2024-09-12T20:55:42Z

spec.bs

+    1. Let |savedQueryName| be |options|["`savedQuery`"].
+    1. If |savedQueryName| is nonempty, then:
+        1. Let |site| be the result of running [=obtain a site=] on |workletDataOrigin|.
+        1. Let |savedIndex| be the result of running [=get the index for a saved query=] on |navigable|, |site|, and |savedQueryName|.


It appears that you could use the same name for two completely different queries and this would return the value of the first query. Is this the desired behavior?

For example, maybe we should return an error if the parameters passed to selectUrl in the second call differ from the first call? Or should the second call override the parameters from the first selectUrl by creating a new saved query?

This is kind of stemming from the awkardness of adding a parameter to an existing function call. It seems like what you might want is something more like how SQL has separate "prepare statement" and "execute statement" calls. You could consider a prepareSelectUrl() that takes all params and assigns a name. Then a separate selectUrl(name) call that operates only on the name without any other params.

If we want to keep a single function call that is overloaded I think we should just be intentional if the site calls it twice with different arguments, but the same name.

It appears that you could use the same name for two completely different queries and this would return the value of the first query. Is this the desired behavior?

The short answer is: probably yes.

The long answer is that I did forget to take into account the operation name, and I may not want to ignore that. I am thinking about that some more offline.

But we deliberately want the array of URLs to be able to be different, and note that we do handle the error that happens if we end up with an out of range index because of this.

We would ignore any passed data for options in the case that we are able to retrieve a saved query result. The other options parameters are still handled.

So there shouldn't be a need for a separate prepareSelectUrl() statement. I just need to decide whether or not operation names should be incorporated into the map, etc.

In that case I recommend returning an error if the operation name doesn't match the previously used operation name for the same saved query. Its probably a developer mistake. Allowing the same saved query name for different operations sounds like a footgun.

After further contemplation and discussion with the team, we've decided two things.

Saved queries will be stored on a per-origin (instead of per-site) basis. I have updated this spec PR accordingly.

We do indeed want names of saved queries to be independent of operation names, and for the storing and lookup of saved queries to ignore the operation names. So two calls to selectURL() that have different operation names could still access the same saved query result if they use the same savedQuery parameter. While this could lead to some developers making mistakes, it also allows for greater flexibility and simplicity. We would need to be sure to document this properly, of course, including in the current explainer PR (where the effect of differing operation names is currently not mentioned).

Just to double-check, if you start with being more restrictive where you throw an error for same-savedQuery and different-operation, then you can relax that later if needed. If you do what you propose and then find developers are having trouble you can't safely make it more restrictive in a backward compatible way.

I'm not going to block the PR on this, but I want to double-check because I think in web standards we generally want to be more restrictive unless there is a compelling reason to do otherwise.

I don't have particularly strong feelings here. It seems unlikely to be a footgun to me, but at the same time I can't think of a good reason to use the same saved-query on two different operations. So, let's say that we go ahead with your proposal here as it seems reasonable.

So now the proposal is that a saved-query key is <origin, operation_name, saved_query_name>.

For similar reasons, what would you think about also including the worklet script url as part of the key?

e.g., <origin, operation_name, worklet_script_url, saved_query_name>

Like operations, presumably different scripts could have different behavior, even if the operation name is the same. This gets a little funky in that urls aren't actually const resources, but still.

OK, I have updated the spec so that queries are keyed by <origin, operation_name, saved_query_name>.

I think we should just not reuse the query (i.e. act as if it's the first time this query is seen) if there is different operation name, rather than throwing, to simplify things.

Just to be clear, keying on <origin, operation_name, saved_query_name> does your proposal above, right? It ignores mismatched operations because you end up with different slots in your map. To do what I was proposing I think the key needs to be <origin, saved_query_name> and then in the map value the operation_name is saved so you can compare it on lookup.

(Edit: Or maybe I am confused about the key here? Sorry if I am missing something.)

I'm not sure I understand enough about how different worklets can mix on a page to have an opinion on how to treat conflicting scripts URLs. If we assume folks are going to hash URLs for different versions of the same worklet, though, it does seem like a flow we should be concerned with.

Just to be clear, keying on <origin, operation_name, saved_query_name> does your proposal above, right? It ignores mismatched operations because you end up with different slots in your map. To do what I was proposing I think the key needs to be <origin, saved_query_name> and then in the map value the operation_name is saved so you can compare it on lookup.

Yes. The key would need to be <origin, saved_query_name> to do what you propose. But I think it would be best to key on <origin, operation_name, saved_query_name> instead because it seems to make more sense for us to simply narrow the scope for the saved queries further from the beginning, rather than to add additional errors. We wouldn't throw if there is a mismatch, but we wouldn't re-use a query in the case of a mismatch either.

wanderview

LGTM with one spec language improvement fixed. This changes the setting and getting of the saved query using tuples as the key.

spec.bs

pythagoraskitty · 2024-09-19T22:42:50Z

After further discussion with the team, we decided to also key on the script's URL. So now I have updated the spec so the the key is <origin, script URL, operation name, query name>.

Would you mind please taking one more look? @wanderview @xyaoinum

Thanks!

spec.bs

wanderview · 2024-09-20T16:23:48Z

After further discussion with the team, we decided to also key on the script's URL. So now I have updated the spec so the the key is <origin, script URL, operation name, query name>.

Would you mind please taking one more look? @wanderview @xyaoinum

Thanks!

I added one more comment, but otherwise LGTM.

xyaoinum · 2024-09-20T17:02:59Z

Still LGTM

SHA: 2951ea5 Reason: push, by pythagoraskitty Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

See explainer update at WICG/shared-storage#176 and spec updates at WICG/shared-storage#188 WICG/shared-storage#202 Currently the feature is disabled by default. We will enable by default after the I2S has been sent and approved. Bug: 367440966 Change-Id: I82d731a7e2d6bcc2368658a619a6eb1d4d751025 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5920781 Commit-Queue: Cammie Smith Barnes <cammie@chromium.org> Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org> Cr-Commit-Position: refs/heads/main@{#1374887}

We add support for saved queries, keyed by (`data_origin`, `script_url`, `operation_name`, `query_name`). When a named query is first invoked, the per page budgets are charged as usual, the index result is stored for later in case the query is reused from the same page. On query reuse, the previously calculated and stored index result will be used to select the URL at that index in the current list, and the per page budgets will not be re-charged. When the page is destroyed, all associated saved queries are also destroyed. See explainer update at WICG/shared-storage#176 and spec updates at WICG/shared-storage#188 WICG/shared-storage#202 Bug: 367440966 Change-Id: I89fd7e65cd14c9d60d6ac53906f95593a2259a37 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5868889 Reviewed-by: Ken Buchanan <kenrb@chromium.org> Reviewed-by: Yao Xiao <yaoxia@chromium.org> Commit-Queue: Cammie Smith Barnes <cammie@chromium.org> Cr-Commit-Position: refs/heads/main@{#1375507}

We add support for saved queries, keyed by (`data_origin`, `script_url`, `operation_name`, `query_name`). When a named query is first invoked, the per page budgets are charged as usual, the index result is stored for later in case the query is reused from the same page. On query reuse, the previously calculated and stored index result will be used to select the URL at that index in the current list, and the per page budgets will not be re-charged. When the page is destroyed, all associated saved queries are also destroyed. See explainer update at WICG/shared-storage#176 and spec updates at WICG/shared-storage#188 WICG/shared-storage#202 Bug: 367440966 Change-Id: I89fd7e65cd14c9d60d6ac53906f95593a2259a37 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5868889 Reviewed-by: Ken Buchanan <kenrb@chromium.org> Reviewed-by: Yao Xiao <yaoxia@chromium.org> Commit-Queue: Cammie Smith Barnes <cammie@chromium.org> Cr-Commit-Position: refs/heads/main@{#1375507} Co-authored-by: Camillia Smith Barnes <cammie@chromium.org>

… selectURL, a=testonly Automatic update from web-platform-tests Shared Storage: Support saved queries in selectURL (#48868) We add support for saved queries, keyed by (`data_origin`, `script_url`, `operation_name`, `query_name`). When a named query is first invoked, the per page budgets are charged as usual, the index result is stored for later in case the query is reused from the same page. On query reuse, the previously calculated and stored index result will be used to select the URL at that index in the current list, and the per page budgets will not be re-charged. When the page is destroyed, all associated saved queries are also destroyed. See explainer update at WICG/shared-storage#176 and spec updates at WICG/shared-storage#188 WICG/shared-storage#202 Bug: 367440966 Change-Id: I89fd7e65cd14c9d60d6ac53906f95593a2259a37 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5868889 Reviewed-by: Ken Buchanan <kenrb@chromium.org> Reviewed-by: Yao Xiao <yaoxia@chromium.org> Commit-Queue: Cammie Smith Barnes <cammie@chromium.org> Cr-Commit-Position: refs/heads/main@{#1375507} Co-authored-by: Camillia Smith Barnes <cammie@chromium.org> -- wpt-commits: 3c0bfa31dc842d0dbce0e377bfd39a4aa0ec4fd9 wpt-pr: 48868

… selectURL, a=testonly Automatic update from web-platform-tests Shared Storage: Support saved queries in selectURL (#48868) We add support for saved queries, keyed by (`data_origin`, `script_url`, `operation_name`, `query_name`). When a named query is first invoked, the per page budgets are charged as usual, the index result is stored for later in case the query is reused from the same page. On query reuse, the previously calculated and stored index result will be used to select the URL at that index in the current list, and the per page budgets will not be re-charged. When the page is destroyed, all associated saved queries are also destroyed. See explainer update at WICG/shared-storage#176 and spec updates at WICG/shared-storage#188 WICG/shared-storage#202 Bug: 367440966 Change-Id: I89fd7e65cd14c9d60d6ac53906f95593a2259a37 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5868889 Reviewed-by: Ken Buchanan <kenrbchromium.org> Reviewed-by: Yao Xiao <yaoxiachromium.org> Commit-Queue: Cammie Smith Barnes <cammiechromium.org> Cr-Commit-Position: refs/heads/main{#1375507} Co-authored-by: Camillia Smith Barnes <cammiechromium.org> -- wpt-commits: 3c0bfa31dc842d0dbce0e377bfd39a4aa0ec4fd9 wpt-pr: 48868 UltraBlame original commit: be7ca250d6301d51162a3b8b73ec9f0fa1beecf0

… selectURL, a=testonly Automatic update from web-platform-tests Shared Storage: Support saved queries in selectURL (#48868) We add support for saved queries, keyed by (`data_origin`, `script_url`, `operation_name`, `query_name`). When a named query is first invoked, the per page budgets are charged as usual, the index result is stored for later in case the query is reused from the same page. On query reuse, the previously calculated and stored index result will be used to select the URL at that index in the current list, and the per page budgets will not be re-charged. When the page is destroyed, all associated saved queries are also destroyed. See explainer update at WICG/shared-storage#176 and spec updates at WICG/shared-storage#188 WICG/shared-storage#202 Bug: 367440966 Change-Id: I89fd7e65cd14c9d60d6ac53906f95593a2259a37 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5868889 Reviewed-by: Ken Buchanan <kenrb@chromium.org> Reviewed-by: Yao Xiao <yaoxia@chromium.org> Commit-Queue: Cammie Smith Barnes <cammie@chromium.org> Cr-Commit-Position: refs/heads/main@{#1375507} Co-authored-by: Camillia Smith Barnes <cammie@chromium.org> -- wpt-commits: 3c0bfa31dc842d0dbce0e377bfd39a4aa0ec4fd9 wpt-pr: 48868

pythagoraskitty added 2 commits September 10, 2024 15:06

[spec] Support saved queries for selectURL

ce3e883

We update the spec to support saved queries, as a followup to #176.

tweak

251a621

Base automatically changed from cammie-branch6 to main September 10, 2024 20:09

pythagoraskitty requested a review from xyaoinum September 10, 2024 20:11

xyaoinum approved these changes Sep 11, 2024

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

Omit empty string default

b3ba8d0

pythagoraskitty requested a review from wanderview September 12, 2024 18:01

wanderview requested changes Sep 12, 2024

View reviewed changes

pythagoraskitty added 3 commits September 13, 2024 17:40

Use origin instead of site for saved queries

cc49d20

Revise use of nonempty

b9919b1

key on operation name also

5da1305

wanderview approved these changes Sep 18, 2024

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

pythagoraskitty added 3 commits September 18, 2024 14:42

Use tuples

41f604f

Also key on script's URL

8c7de15

Merge branch 'main' into cammie-branch7

bfb6d1a

wanderview reviewed Sep 20, 2024

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

name tuple members

fa8183f

pythagoraskitty merged commit 2951ea5 into main Sep 20, 2024
1 check passed

pythagoraskitty deleted the cammie-branch7 branch September 20, 2024 17:23

github-actions bot added a commit that referenced this pull request Sep 20, 2024

Merge pull request #188 from WICG/cammie-branch7

60ee24e

SHA: 2951ea5 Reason: push, by pythagoraskitty Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

chromium-wpt-export-bot mentioned this pull request Oct 29, 2024

Shared Storage: Support saved queries in selectURL web-platform-tests/wpt#48868

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[spec] Support saved queries for selectURL #188

[spec] Support saved queries for selectURL #188

pythagoraskitty commented Sep 10, 2024 •

edited by pr-preview bot

Loading

pythagoraskitty commented Sep 10, 2024

xyaoinum left a comment

pythagoraskitty commented Sep 12, 2024

wanderview Sep 12, 2024

pythagoraskitty Sep 12, 2024 •

edited

Loading

wanderview Sep 13, 2024

pythagoraskitty Sep 13, 2024

wanderview Sep 16, 2024

jkarlin Sep 16, 2024

pythagoraskitty Sep 16, 2024 •

edited

Loading

wanderview Sep 16, 2024 •

edited

Loading

pythagoraskitty Sep 16, 2024 •

edited

Loading

wanderview left a comment

pythagoraskitty commented Sep 19, 2024

wanderview commented Sep 20, 2024

xyaoinum commented Sep 20, 2024

[spec] Support saved queries for selectURL #188

[spec] Support saved queries for selectURL #188

Conversation

pythagoraskitty commented Sep 10, 2024 • edited by pr-preview bot Loading

pythagoraskitty commented Sep 10, 2024

xyaoinum left a comment

Choose a reason for hiding this comment

pythagoraskitty commented Sep 12, 2024

wanderview Sep 12, 2024

Choose a reason for hiding this comment

pythagoraskitty Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

wanderview Sep 13, 2024

Choose a reason for hiding this comment

pythagoraskitty Sep 13, 2024

Choose a reason for hiding this comment

wanderview Sep 16, 2024

Choose a reason for hiding this comment

jkarlin Sep 16, 2024

Choose a reason for hiding this comment

pythagoraskitty Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

wanderview Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

pythagoraskitty Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

wanderview left a comment

Choose a reason for hiding this comment

pythagoraskitty commented Sep 19, 2024

wanderview commented Sep 20, 2024

xyaoinum commented Sep 20, 2024

pythagoraskitty commented Sep 10, 2024 •

edited by pr-preview bot

Loading

pythagoraskitty Sep 12, 2024 •

edited

Loading

pythagoraskitty Sep 16, 2024 •

edited

Loading

wanderview Sep 16, 2024 •

edited

Loading

pythagoraskitty Sep 16, 2024 •

edited

Loading