Single Promise, options object, revamped processing model by yoavweiss · Pull Request #70 · WICG/ua-client-hints

yoavweiss · 2020-03-02T15:38:44Z

This addresses the IDL review in #51, feedback from @mikewest on https://chromium-review.googlesource.com/c/chromium/src/+/2019369 as well as #56

Preview | Diff

…ing model

index.bs

Co-Authored-By: Domenic Denicola <d@domenic.me>

mikewest

This looks reasonable to me. Small comments.

index.bs

yoavweiss · 2020-03-03T13:23:36Z

@domenic @mikewest PTAL?

yoavweiss · 2020-03-03T14:14:11Z

index.bs

+
+    6. [=Queue a task=] to [=resolve=] |p| with |uaData|.
+
+ISSUE: Add a specific task source this is queued on.


Thoughts on the task queue for this?

What do you plan to implement in Chrome?

There's always the option of giving maximum flexibility by creating an entirely new task source.

The closest counterpart I can think of off the top of my head is createImageBitmap. Unfortunately that doesn't do this right either :(. whatwg/html#5329

This CL aligns the UA-CH implementation with PR#46[1], #48[2] and #70[3]. [1] WICG/ua-client-hints#46 [2] WICG/ua-client-hints#48 [3] WICG/ua-client-hints#70 Change-Id: I8221d8a967213180a1aa1d9ef23f17e6f95718b7

domenic

Looking pretty good; only major issue left is the frozen array initialization.

index.bs

domenic · 2020-03-03T17:33:21Z

index.bs

 --------------
-<dfn method for="NavigatorUA"><code>getUserAgent()</code></dfn> method MUST run these steps:
+
+On getting, the {{NavigatorUAData/uaList}} attribute MUST run these steps:


As written, this will return a new frozen array each time. That's not great. In particular navigator.userAgentData.uaList !== navigator.userAgentData.uaList.

The way to do this is to have an associated <dfn for="NavigatorUAData">UA list</dfn> and say that it must be initialized following this algorithm, plus at the end, a call to https://heycam.github.io/webidl/#dfn-create-frozen-array. Then, the getting algorithm for {{NavigatorUAData/uaList}} returns this's UA list.

You can see an example of this in https://wicg.github.io/origin-policy/#monkeypatch-html-windoworworkerglobalscope. Here you can get away without an explicit initialization point (i.e. no need to monkeypatch navigation or similar to set the UA list) since the algorithm is the same for all NavigatorUAData instances.

Oh, that raises a new point. Do you want to allow the answer to vary between NavigatorUAData instances, or do you need to maintain consistency across some scope? E.g. across the UA, or across Windows that can reach each other synchronously, or...

Thanks! Did the above, and did patch WindowOrWorkerGlobalScope, to make sure consistency is maintained across them.

I don't have a strong opinion about consistency across the UA or across Windows - it seems important that the algorithm would end up with the same values for them (especially with headers, to reduce caching variance), but not super important (to me) that objects would ===.
At the same time, if there's an easy way to make that happen in spec and implementation, I'm happy to do that.

Oh, I don't think the objects should be ===; that'd be a bit strange. I was more concerned about whether the values should be consistent. I'll re-review from that perspective.

index.bs

domenic · 2020-03-03T17:35:50Z

index.bs

+
+    6. [=Queue a task=] to [=resolve=] |p| with |uaData|.
+
+ISSUE: Add a specific task source this is queued on.


What do you plan to implement in Chrome?

There's always the option of giving maximum flexibility by creating an entirely new task source.

The closest counterpart I can think of off the top of my head is createImageBitmap. Unfortunately that doesn't do this right either :(. whatwg/html#5329

This CL aligns the UA-CH implementation with PR#46[1], #48[2] and #70[3]. [1] WICG/ua-client-hints#46 [2] WICG/ua-client-hints#48 [3] WICG/ua-client-hints#70 Change-Id: I8221d8a967213180a1aa1d9ef23f17e6f95718b7

Co-Authored-By: Domenic Denicola <d@domenic.me>

index.bs

Co-Authored-By: Domenic Denicola <d@domenic.me>

…s into revamp_promises

This CL aligns the UA-CH implementation with PR#46[1], #48[2] and #70[3]. [1] WICG/ua-client-hints#46 [2] WICG/ua-client-hints#48 [3] WICG/ua-client-hints#70 Change-Id: I8221d8a967213180a1aa1d9ef23f17e6f95718b7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2019369 Reviewed-by: Mike West <mkwst@chromium.org> Reviewed-by: Daniel Vogelheim <vogelheim@chromium.org> Reviewed-by: Aaron Tagliaboschi <aarontag@chromium.org> Reviewed-by: Avi Drissman <avi@chromium.org> Commit-Queue: Yoav Weiss <yoavweiss@chromium.org> Cr-Commit-Position: refs/heads/master@{#747644}

domenic

Great, this LGTM! Very clear and correct now about issues like whether the same list is returned, etc.

I left a few nits but upon reflection they're pretty general (probably extending to sections of the document which are not touched by this PR), and you should feel welcome to fix them separately, at a much lower-priority.

index.bs

Co-Authored-By: Domenic Denicola <d@domenic.me>

This CL aligns the UA-CH implementation with PR#46[1], #48[2] and #70[3]. [1] WICG/ua-client-hints#46 [2] WICG/ua-client-hints#48 [3] WICG/ua-client-hints#70 Change-Id: I8221d8a967213180a1aa1d9ef23f17e6f95718b7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2019369 Reviewed-by: Mike West <mkwst@chromium.org> Reviewed-by: Daniel Vogelheim <vogelheim@chromium.org> Reviewed-by: Aaron Tagliaboschi <aarontag@chromium.org> Reviewed-by: Avi Drissman <avi@chromium.org> Commit-Queue: Yoav Weiss <yoavweiss@chromium.org> Cr-Commit-Position: refs/heads/master@{#747644}

…ith PR#46, #48 & #70, a=testonly Automatic update from web-platform-tests [UA client hints] Align implementation with PR#46, #48 & #70 This CL aligns the UA-CH implementation with PR#46[1], #48[2] and #70[3]. [1] WICG/ua-client-hints#46 [2] WICG/ua-client-hints#48 [3] WICG/ua-client-hints#70 Change-Id: I8221d8a967213180a1aa1d9ef23f17e6f95718b7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2019369 Reviewed-by: Mike West <mkwst@chromium.org> Reviewed-by: Daniel Vogelheim <vogelheim@chromium.org> Reviewed-by: Aaron Tagliaboschi <aarontag@chromium.org> Reviewed-by: Avi Drissman <avi@chromium.org> Commit-Queue: Yoav Weiss <yoavweiss@chromium.org> Cr-Commit-Position: refs/heads/master@{#747644} -- wpt-commits: e0be414fa1d9f3b4e841ca74bfc43ee3d7586fc4 wpt-pr: 21428

…ith PR#46, #48 & #70, a=testonly Automatic update from web-platform-tests [UA client hints] Align implementation with PR#46, #48 & #70 This CL aligns the UA-CH implementation with PR#46[1], #48[2] and #70[3]. [1] WICG/ua-client-hints#46 [2] WICG/ua-client-hints#48 [3] WICG/ua-client-hints#70 Change-Id: I8221d8a967213180a1aa1d9ef23f17e6f95718b7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2019369 Reviewed-by: Mike West <mkwstchromium.org> Reviewed-by: Daniel Vogelheim <vogelheimchromium.org> Reviewed-by: Aaron Tagliaboschi <aarontagchromium.org> Reviewed-by: Avi Drissman <avichromium.org> Commit-Queue: Yoav Weiss <yoavweisschromium.org> Cr-Commit-Position: refs/heads/master{#747644} -- wpt-commits: e0be414fa1d9f3b4e841ca74bfc43ee3d7586fc4 wpt-pr: 21428 UltraBlame original commit: 222962fe639e45b4d3f8f769e49446edf4133207

…ith PR#46, #48 & #70, a=testonly Automatic update from web-platform-tests [UA client hints] Align implementation with PR#46, #48 & #70 This CL aligns the UA-CH implementation with PR#46[1], #48[2] and #70[3]. [1] WICG/ua-client-hints#46 [2] WICG/ua-client-hints#48 [3] WICG/ua-client-hints#70 Change-Id: I8221d8a967213180a1aa1d9ef23f17e6f95718b7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2019369 Reviewed-by: Mike West <mkwst@chromium.org> Reviewed-by: Daniel Vogelheim <vogelheim@chromium.org> Reviewed-by: Aaron Tagliaboschi <aarontag@chromium.org> Reviewed-by: Avi Drissman <avi@chromium.org> Commit-Queue: Yoav Weiss <yoavweiss@chromium.org> Cr-Commit-Position: refs/heads/master@{#747644} -- wpt-commits: e0be414fa1d9f3b4e841ca74bfc43ee3d7586fc4 wpt-pr: 21428

Modify API shape to a single promise & options object. Revamp process…

a426c13

…ing model

yoavweiss requested review from domenic and mikewest March 2, 2020 15:38

yoavweiss mentioned this pull request Mar 2, 2020

IDL/API surface review #51

Closed

domenic requested changes Mar 2, 2020

View reviewed changes

yoavweiss and others added 3 commits March 2, 2020 23:40

Fix up IDL

2cb13e0

Co-Authored-By: Domenic Denicola <d@domenic.me>

Responded to review comments

aaa06d6

Fix bikeshed error

1d11895

mikewest reviewed Mar 3, 2020

View reviewed changes

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

index.bs Show resolved Hide resolved

Switch to accept a sequence instead of an options object

29e16ea

yoavweiss mentioned this pull request Mar 3, 2020

WebIDL errors for default argument to dictionary input speced/bikeshed#1605

Closed

yoavweiss commented Mar 3, 2020

View reviewed changes

index.bs Outdated Show resolved Hide resolved

yoavweiss mentioned this pull request Mar 3, 2020

"UA" is being used for multiple things #73

Closed

yoavweiss commented Mar 3, 2020

View reviewed changes

Fix IDL

81478c4

chromium-wpt-export-bot mentioned this pull request Mar 3, 2020

[UA client hints] Align implementation with PR#46, #48 & #70 web-platform-tests/wpt#21428

Merged

domenic requested changes Mar 3, 2020

View reviewed changes

yoavweiss and others added 3 commits March 5, 2020 16:17

Tidy up IDL

b1e5ad5

Co-Authored-By: Domenic Denicola <d@domenic.me>

s/let/set/

dfe9870

Co-Authored-By: Domenic Denicola <d@domenic.me>

Revamp the frozen array creation

c9d6223

domenic reviewed Mar 5, 2020

View reviewed changes

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

Place variable in the method call

997ee19

Co-Authored-By: Domenic Denicola <d@domenic.me>

yoavweiss added 2 commits March 6, 2020 12:26

Attach UA list creation to UA

f23bfb8

Merge branch 'revamp_promises' of github.com:yoavweiss/ua-client-hint…

02ba201

…s into revamp_promises

yoavweiss mentioned this pull request Mar 6, 2020

Add missing "get" to method name #75

Closed

domenic approved these changes Mar 6, 2020

View reviewed changes

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

index.bs Show resolved Hide resolved

yoavweiss and others added 2 commits March 6, 2020 17:00

add user agent to UA list

9777b49

Co-Authored-By: Domenic Denicola <d@domenic.me>

[=user agent=]

e244cac

Co-Authored-By: Domenic Denicola <d@domenic.me>

yoavweiss merged commit 0b7359c into WICG:master Mar 6, 2020

yoavweiss deleted the revamp_promises branch March 6, 2020 16:05

yoavweiss mentioned this pull request Mar 6, 2020

Add refs to normative user agent mentions #78

Merged

yoavweiss mentioned this pull request Mar 9, 2020

Define getters for read-only attributes (Was: interface NavigatorUAData should be a dictionary) #19

Closed

This was referenced Mar 14, 2020

Interface for getUserAgent missing Promise #56

Closed

Should getUserAgent still be a function? #68

Closed

navigator.getUserAgent is missing on new Canary version #83

Closed

azu mentioned this pull request Mar 16, 2020

New User-Agent Client Hints faisalman/ua-parser-js#408

Closed

yoavweiss mentioned this pull request May 11, 2020

Sync attributes should also be exposed in async API (Was: Partially synchronous interface is odd) #108

Closed


		6. [=Queue a task=] to [=resolve=] \|p\| with \|uaData\|.

		ISSUE: Add a specific task source this is queued on.

Conversation

yoavweiss commented Mar 2, 2020 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mikewest left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yoavweiss commented Mar 3, 2020

Uh oh!

yoavweiss Mar 3, 2020

Choose a reason for hiding this comment

Uh oh!

domenic Mar 3, 2020

Choose a reason for hiding this comment

Uh oh!

domenic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

domenic Mar 3, 2020

Choose a reason for hiding this comment

Uh oh!

yoavweiss Mar 5, 2020

Choose a reason for hiding this comment

Uh oh!

domenic Mar 5, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

domenic Mar 3, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

domenic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yoavweiss commented Mar 2, 2020 •

edited by pr-preview bot

Loading