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.

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

Add domain_lookup_start functionality #23310

Merged
merged 2 commits into from Jun 30, 2019

Conversation

@KaczuH
Copy link

KaczuH commented May 2, 2019

Added the domain_lookup_start functionality in http_loader.rs (http_redirect_fetch function)


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #21259 (GitHub issue number if applicable)
  • There are tests for these changes OR
  • These changes do not require tests because ___

This change is Reviewable

@highfive
Copy link

highfive commented May 2, 2019

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @asajeffrey (or someone else) soon.

@highfive
Copy link

highfive commented May 2, 2019

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/webidls/PerformanceResourceTiming.webidl, components/script/dom/performanceresourcetiming.rs
  • @KiChjang: components/script/dom/webidls/PerformanceResourceTiming.webidl, components/net_traits/lib.rs, components/script/dom/performanceresourcetiming.rs, components/net/http_loader.rs
components/net/http_loader.rs Outdated Show resolved Hide resolved
@jdm
Copy link
Member

jdm commented May 2, 2019

Looks good! Could you squash the commits into one single commit, too?

@KaczuH KaczuH force-pushed the KaczuH:add-domain-lookup-start branch from d4d00a1 to 77cabbb May 2, 2019
@KaczuH
Copy link
Author

KaczuH commented May 8, 2019

Ok, commits are sqashed.
I'm not sure why AppVeyor build was cancelled.

@CYBAI
Copy link
Collaborator

CYBAI commented May 8, 2019

@bors-servo try=wpt

bors-servo added a commit that referenced this pull request May 8, 2019
Add domain_lookup_start functionality

<!-- Please describe your changes on the following line: -->
Added the domain_lookup_start functionality in http_loader.rs (http_redirect_fetch function)

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #21259 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23310)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented May 8, 2019

Trying commit 77cabbb with merge 27f726c...

@CYBAI CYBAI removed the S-needs-squash label May 8, 2019
@bors-servo
Copy link
Contributor

bors-servo commented May 8, 2019

💔 Test failed - linux-rel-wpt

@CYBAI
Copy link
Collaborator

CYBAI commented May 8, 2019

Let's fix the FAIL one and label the latter one as PASS! 😄

{
    "status": "FAIL", 
    "group": "default", 
    "message": "assert_equals: wrong typeof object expected \"object\" but got \"undefined\"", 
    "stack": "IdlInterface.prototype.test_interface_of/<@http://web-platform.test:8000/resources/idlharness.js:2758:17\nTest.prototype.step@http://web-platform.test:8000/resources/testharness.js:1587:20\ntest@http://web-platform.test:8000/resources/testharness.js:544:21\nself.subsetTestByKey@http://web-platform.test:8000/resources/idlharness.js:54:23\nIdlInterface.prototype.test_interface_of@http://web-platform.test:8000/resources/idlharness.js:2755:13\nIdlInterface.prototype.test_object@http://web-platform.test:8000/resources/idlharness.js:2634:9\nIdlArray.prototype.test/<@http://web-platform.test:8000/resources/idlharness.js:840:17\nIdlArray.prototype.test@http://web-platform.test:8000/resources/idlharness.js:835:13\nidl_test/</<@http://web-platform.test:8000/resources/idlharness.js:3252:21\n", 
    "subtest": "PerformanceResourceTiming interface: resource must inherit property \"domainLookupStart\" with the proper type", 
    "test": "/resource-timing/idlharness.any.worker.html", 
    "line": 290996, 
    "action": "test_result", 
    "expected": "PASS"
}
{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "domainLookupStart should be 0 in cross-origin request.", 
    "test": "/resource-timing/resource_TAO_zero.htm", 
    "line": 344577, 
    "action": "test_result", 
    "expected": "FAIL"
}
@jdm
Copy link
Member

jdm commented May 8, 2019

Yes, that worker failure is caused by #21350 and should be fixed by #23322.

@CYBAI
Copy link
Collaborator

CYBAI commented May 8, 2019

Yes, that worker failure is caused by #21350 and should be fixed by #23322.

oh, thanks! didn't notice that! then just need to update expectation for the PASS one! thanks!

@highfive highfive removed the S-tests-failed label May 8, 2019
domain_lookup_end: 0.,
domain_lookup_start: 0.,

This comment has been minimized.

Copy link
@jdm

jdm May 17, 2019

Member

We need to adjust the from_resource_timing method below this to initialize the value to the stored one as well.

This comment has been minimized.

Copy link
@KaczuH

KaczuH May 18, 2019

Author

Ok, done :)

@jdm
Copy link
Member

jdm commented May 17, 2019

Sorry about the delay in reviewing this!

@jdm jdm removed the S-awaiting-review label May 17, 2019
Remove wpt tests for domainLookupStart

Set ResourceAttribute::DomainLookupTime

Move DomainLookupStart timing before HTTP request initialization

Change label of domainLookupStart TAO zero test to PASS

Adjust the from_resource_timing method to initialize domain_lookup_start value

Restore domainLookupsStart test
@KaczuH KaczuH force-pushed the KaczuH:add-domain-lookup-start branch from ef03584 to d84513c Jun 29, 2019
@KaczuH
Copy link
Author

KaczuH commented Jun 29, 2019

@jdm squash done

@jdm
Copy link
Member

jdm commented Jun 29, 2019

@bors-servo r+
Thanks!

@bors-servo
Copy link
Contributor

bors-servo commented Jun 29, 2019

📌 Commit d84513c has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Jun 29, 2019

Testing commit d84513c with merge fd7bc18...

bors-servo added a commit that referenced this pull request Jun 29, 2019
Add domain_lookup_start functionality

<!-- Please describe your changes on the following line: -->
Added the domain_lookup_start functionality in http_loader.rs (http_redirect_fetch function)

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #21259 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23310)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 29, 2019

💔 Test failed - linux-rel-wpt

@CYBAI
Copy link
Collaborator

CYBAI commented Jun 29, 2019

{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "PerformanceResourceTiming interface: resource must inherit property \"domainLookupStart\" with the proper type", 
    "test": "/resource-timing/idlharness.any.worker.html", 
    "line": 139027, 
    "action": "test_result", 
    "expected": "FAIL"
}
@jdm
Copy link
Member

jdm commented Jun 30, 2019

@bors-servo
Copy link
Contributor

bors-servo commented Jun 30, 2019

📌 Commit 98be2df has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Jun 30, 2019

Testing commit 98be2df with merge 489ff5e...

bors-servo added a commit that referenced this pull request Jun 30, 2019
Add domain_lookup_start functionality

<!-- Please describe your changes on the following line: -->
Added the domain_lookup_start functionality in http_loader.rs (http_redirect_fetch function)

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #21259 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23310)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 30, 2019

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: jdm
Pushing 489ff5e to master...

@bors-servo bors-servo merged commit 98be2df into servo:master Jun 30, 2019
4 checks passed
4 checks passed
Taskcluster (pull_request) TaskGroup: success
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@atouchet atouchet mentioned this pull request Jun 30, 2019
0 of 5 tasks complete
bors-servo added a commit that referenced this pull request Jun 30, 2019
Remove passing test

<!-- Please describe your changes on the following line: -->
This was added in #23310. Was that a mistake?

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23662)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Jun 30, 2019
Remove passing test

<!-- Please describe your changes on the following line: -->
This was added in #23310. Was that a mistake?

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23662)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.