Skip to content
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

Avoid redefining secureConnectionStart #106

Merged
merged 3 commits into from Jul 15, 2019
Merged

Avoid redefining secureConnectionStart #106

merged 3 commits into from Jul 15, 2019

Conversation

yoavweiss
Copy link
Contributor

@yoavweiss yoavweiss commented Apr 23, 2019 •
edited by pr-preview bot

Closes #84

Preview | Diff

@yoavweiss yoavweiss requested a review from npm1 April 23, 2019 12:11
npm1
npm1 approved these changes Apr 23, 2019
View reviewed changes
Copy link
Contributor

@npm1 npm1 left a comment

Choose a reason for hiding this comment

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

LGTM, modulo questions:

  • Do we need to special-case for when the document does not have an associated resource?
  • It looks like this changes the definition of this attribute because it was not identical to what was specified in ResourceTiming. Is this covered by a test, or do we need to add one?

@yoavweiss
Copy link
Contributor Author

yoavweiss commented Jul 15, 2019 •
edited

LGTM

Thanks for reviewing! :)

  • Do we need to special-case for when the document does not have an associated resource?

Yeah, makes sense to do that. (even if handwavy...)

  • It looks like this changes the definition of this attribute because it was not identical to what was specified in ResourceTiming. Is this covered by a test, or do we need to add one?

Yeah, I'll add a test around connection reuse (which is the piece that's changed)

@yoavweiss yoavweiss force-pushed the secure_connection_align_with_rt branch from e080b7a to 41ed1ae Compare July 15, 2019 10:24
@yoavweiss
Copy link
Contributor Author

Tests added at https://chromium-review.googlesource.com/c/chromium/src/+/1702022

@chromium-wpt-export-bot chromium-wpt-export-bot mentioned this pull request Jul 15, 2019
Merged
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 15, 2019
@chromium-wpt-export-bot
[Navigation timing] Add secure connection start reuse test
bf0cc8c 
During the review of w3c/navigation-timing#106
it was noted that secureConnectionStart's behavior with regards to
connection reuse and navigation timing is not well-tested.
This CL adds such a test.

Change-Id: I91d7cbfe4f1ee58a72b4a647a38bd58d4ffca693
Bug: 977519
@yoavweiss yoavweiss mentioned this pull request Jul 15, 2019
Closed
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 15, 2019
@chromium-wpt-export-bot
[Navigation timing] Add secure connection start reuse test
a0b04e4 
During the review of w3c/navigation-timing#106
it was noted that secureConnectionStart's behavior with regards to
connection reuse and navigation timing is not well-tested.
This CL adds such a test.

Change-Id: I91d7cbfe4f1ee58a72b4a647a38bd58d4ffca693
Bug: 977519
@yoavweiss
Copy link
Contributor Author

  • Do we need to special-case for when the document does not have an associated resource?

Yeah, makes sense to do that. (even if handwavy...)

Took a second look and it seems like RT's secureConnectionStart is using [https://w3c.github.io/resource-timing/#sec-timing-allow-origin](Timing allow check) which will fail for resources which didn't result in network requests. So, I think we're good for now, at least until we better define what an "associated resource" is...

@yoavweiss yoavweiss merged commit b438573 into w3c:gh-pages Jul 15, 2019
@npm1
Copy link
Contributor

npm1 commented Jul 15, 2019

Took a second look and it seems like RT's secureConnectionStart is using [https://w3c.github.io/resource-timing/#sec-timing-allow-origin](Timing allow check) which will fail for resources which didn't result in network requests. So, I think we're good for now, at least until we better define what an "associated resource" is...

Well, there is no associated resource in this case, though? So calling algorithms which assume the existence of a resource is not well-defined.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 15, 2019
@chromium-wpt-export-bot
[Navigation timing] Add secure connection start reuse test
8d4b8ec 
During the review of w3c/navigation-timing#106
it was noted that secureConnectionStart's behavior with regards to
connection reuse and navigation timing is not well-tested.
This CL adds such a test.

Change-Id: I91d7cbfe4f1ee58a72b4a647a38bd58d4ffca693
Bug: 977519
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 15, 2019
@chromium-wpt-export-bot
[Navigation timing] Add secure connection start reuse test
7c4d7d3 
During the review of w3c/navigation-timing#106
it was noted that secureConnectionStart's behavior with regards to
connection reuse and navigation timing is not well-tested.
This CL adds such a test.

Change-Id: I91d7cbfe4f1ee58a72b4a647a38bd58d4ffca693
Bug: 977519
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1702022
Commit-Queue: Yoav Weiss <yoavweiss@chromium.org>
Reviewed-by: Nicolás Peña Moreno <npm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#677521}
Hexcles pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 16, 2019
@Hexcles
[Navigation timing] Add secure connection start reuse test
391c8f1 
During the review of w3c/navigation-timing#106
it was noted that secureConnectionStart's behavior with regards to
connection reuse and navigation timing is not well-tested.
This CL adds such a test.

Change-Id: I91d7cbfe4f1ee58a72b4a647a38bd58d4ffca693
Bug: 977519
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1702022
Commit-Queue: Yoav Weiss <yoavweiss@chromium.org>
Reviewed-by: Nicolás Peña Moreno <npm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#677521}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jul 24, 2019
@jgraham
Bug 1566056 [wpt PR 17832] - [Navigation timing] Add secure connectio…
682e181 
…n start reuse test, a=testonly

Automatic update from web-platform-tests
[Navigation timing] Add secure connection start reuse test

During the review of w3c/navigation-timing#106
it was noted that secureConnectionStart's behavior with regards to
connection reuse and navigation timing is not well-tested.
This CL adds such a test.

Change-Id: I91d7cbfe4f1ee58a72b4a647a38bd58d4ffca693
Bug: 977519
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1702022
Commit-Queue: Yoav Weiss <yoavweiss@chromium.org>
Reviewed-by: Nicolás Peña Moreno <npm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#677521}

--

wpt-commits: 391c8f12e270cf0cadb26dc06b5d9984afe34a4c
wpt-pr: 17832
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Jul 25, 2019
@jgraham
Bug 1566056 [wpt PR 17832] - [Navigation timing] Add secure connectio…
ef3bcf0 
…n start reuse test, a=testonly

Automatic update from web-platform-tests
[Navigation timing] Add secure connection start reuse test

During the review of w3c/navigation-timing#106
it was noted that secureConnectionStart's behavior with regards to
connection reuse and navigation timing is not well-tested.
This CL adds such a test.

Change-Id: I91d7cbfe4f1ee58a72b4a647a38bd58d4ffca693
Bug: 977519
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1702022
Commit-Queue: Yoav Weiss <yoavweiss@chromium.org>
Reviewed-by: Nicolás Peña Moreno <npm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#677521}

--

wpt-commits: 391c8f12e270cf0cadb26dc06b5d9984afe34a4c
wpt-pr: 17832
natechapin pushed a commit to natechapin/wpt that referenced this pull request Aug 23, 2019
@natechapin
[Navigation timing] Add secure connection start reuse test
818f881 
During the review of w3c/navigation-timing#106
it was noted that secureConnectionStart's behavior with regards to
connection reuse and navigation timing is not well-tested.
This CL adds such a test.

Change-Id: I91d7cbfe4f1ee58a72b4a647a38bd58d4ffca693
Bug: 977519
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1702022
Commit-Queue: Yoav Weiss <yoavweiss@chromium.org>
Reviewed-by: Nicolás Peña Moreno <npm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#677521}
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 4, 2019
@marco-c
Bug 1566056 [wpt PR 17832] - [Navigation timing] Add secure connectio…
373ddbc 
…n start reuse test, a=testonly

Automatic update from web-platform-tests
[Navigation timing] Add secure connection start reuse test

During the review of w3c/navigation-timing#106
it was noted that secureConnectionStart's behavior with regards to
connection reuse and navigation timing is not well-tested.
This CL adds such a test.

Change-Id: I91d7cbfe4f1ee58a72b4a647a38bd58d4ffca693
Bug: 977519
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1702022
Commit-Queue: Yoav Weiss <yoavweisschromium.org>
Reviewed-by: Nicolás Peña Moreno <npmchromium.org>
Cr-Commit-Position: refs/heads/master{#677521}

--

wpt-commits: 391c8f12e270cf0cadb26dc06b5d9984afe34a4c
wpt-pr: 17832

UltraBlame original commit: 7d46cf3817a323c5047e4b8726c6708a1e2c9091
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 4, 2019
@marco-c
Bug 1566056 [wpt PR 17832] - [Navigation timing] Add secure connectio…
78efac5 
…n start reuse test, a=testonly

Automatic update from web-platform-tests
[Navigation timing] Add secure connection start reuse test

During the review of w3c/navigation-timing#106
it was noted that secureConnectionStart's behavior with regards to
connection reuse and navigation timing is not well-tested.
This CL adds such a test.

Change-Id: I91d7cbfe4f1ee58a72b4a647a38bd58d4ffca693
Bug: 977519
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1702022
Commit-Queue: Yoav Weiss <yoavweisschromium.org>
Reviewed-by: Nicolás Peña Moreno <npmchromium.org>
Cr-Commit-Position: refs/heads/master{#677521}

--

wpt-commits: 391c8f12e270cf0cadb26dc06b5d9984afe34a4c
wpt-pr: 17832

UltraBlame original commit: 7d46cf3817a323c5047e4b8726c6708a1e2c9091
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 4, 2019
@marco-c
Bug 1566056 [wpt PR 17832] - [Navigation timing] Add secure connectio…
c660050 
…n start reuse test, a=testonly

Automatic update from web-platform-tests
[Navigation timing] Add secure connection start reuse test

During the review of w3c/navigation-timing#106
it was noted that secureConnectionStart's behavior with regards to
connection reuse and navigation timing is not well-tested.
This CL adds such a test.

Change-Id: I91d7cbfe4f1ee58a72b4a647a38bd58d4ffca693
Bug: 977519
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1702022
Commit-Queue: Yoav Weiss <yoavweisschromium.org>
Reviewed-by: Nicolás Peña Moreno <npmchromium.org>
Cr-Commit-Position: refs/heads/master{#677521}

--

wpt-commits: 391c8f12e270cf0cadb26dc06b5d9984afe34a4c
wpt-pr: 17832

UltraBlame original commit: 7d46cf3817a323c5047e4b8726c6708a1e2c9091
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@npm1 npm1 npm1 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

secureConnectionStart definition is inconsistent with ResourceTiming
2 participants
@yoavweiss @npm1