-
Notifications
You must be signed in to change notification settings - Fork 76
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
Single Promise, options object, revamped processing model #70
Conversation
Co-Authored-By: Domenic Denicola <d@domenic.me>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks reasonable to me. Small comments.
|
||
6. [=Queue a task=] to [=resolve=] |p| with |uaData|. | ||
|
||
ISSUE: Add a specific task source this is queued on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on the task queue for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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
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
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking pretty good; only major issue left is the frozen array initialization.
index.bs
Outdated
}; | ||
Navigator includes NavigatorUA; | ||
|
||
</pre> | ||
|
||
Processing model {#processing} | ||
-------------- | ||
<dfn method for="NavigatorUA"><code>getUserAgent()</code></dfn> method MUST run these steps: | ||
|
||
On getting, the {{NavigatorUAData/uaList}} attribute MUST run these steps: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
6. [=Queue a task=] to [=resolve=] |p| with |uaData|. | ||
|
||
ISSUE: Add a specific task source this is queued on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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
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>
Co-Authored-By: Domenic Denicola <d@domenic.me>
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}
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}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Co-Authored-By: Domenic Denicola <d@domenic.me>
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 <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 <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 <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
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