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

review some initial tests for resource timing #4266

Merged
merged 5 commits into from Dec 6, 2016
Merged

review some initial tests for resource timing #4266

merged 5 commits into from Dec 6, 2016

Conversation

siusin
Copy link
Contributor

@siusin siusin commented Nov 30, 2016

review tests from pull request #402

  • RT Level 1 indicated that aborted requests or requests that don't return a response may be included as PerformanceResourceTiming objects in the Performance Timeline of the relevant context, but later versions would like to included these aborted requests in the Perf Timeline.

@wpt-pr-bot
Copy link
Collaborator

@siusin siusin changed the title review some initials tests for resource timing review some initial tests for resource timing Nov 30, 2016
</head>
<body>
<h1>Description</h1>
<p>This test validates that all of the initiator types are represented even when dynamically inserted.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: drop "all of the"? I don't think we want to enumerate all of the element types here, and implying all and testing a subset is slightly confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!


test_equals(entries.length, 2, 'There should be 2 PerformanceEntries');

var entry = entries[1];
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth adding a comment above, explaining that we're counting on second request of same resource to reuse the socket..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. Please look at commit #b1bbb10

@igrigorik
Copy link
Member

RT Level 1 indicated that aborted requests or requests that don't return a response may be included as PerformanceResourceTiming objects in the Performance Timeline of the relevant context, but later versions would like to included these aborted requests in the Perf Timeline.

Yikes, looks like we introduced some confusion here.

  • RT L1 (section 4.2) states: "Aborted requests or requests that don't return a response may be included as PerformanceResourceTiming objects in the Performance Timeline of the relevant context."
  • RT L2 changes via Clarify presence of requests that don't return a response w3c/resource-timing#12:
    1. w3c/resource-timing@0eb0f69 landed an update stating that aborted requests MUST be included, but it didn't update the processing section..
    2. After that was landed we re-opened discussion once more, concluded that it is (in fact) the correct behavior, and Wes started working on updating the processing section.
    3. At TPAC we concluded that we'll push Wes's update to L3, but we never backed out (i).

The end result, as of right now, is that L2 states that failed resources MUST be surfaced, but doesn't have the processing section updated.


  1. I think "MAY" for L1 is correct, and we shouldn't change that -- some browsers surface such requests, and that's good.
  2. We need to revisit how we want to treat this in L2:
    1. We can carry over MAY from L1, back out w3c/resource-timing@0eb0f69, and target that + processing update to L3.
    2. We can revisit our earlier decision to push MUST to L3...

I believe the reason we went with (i) at TPAC is that it would make existing L2 implementations (e.g. IE, which is not going to change?) incompatible... Which hints that we should stick with that.

@toddreifsteck @plehegar wdyt?

@plehegar
Copy link
Member

For L2, I'd rather use (i) since doing (ii) would probably require hooking ourselves into Fetch, which is for L3.

@toddreifsteck
Copy link
Contributor

I agree that we should go with (i) for L2 and push any further specification to L3.

@siusin
Copy link
Contributor Author

siusin commented Dec 1, 2016

Shall we drop the test case for L1 (resource_ignore_failures_level1.html) in that case? I'm not sure if a test case for a "MAY" will make sense, knowing that it produces different test results among the major browsers.

@plehegar
Copy link
Member

plehegar commented Dec 1, 2016

@siusin imho, we should drop it. Until we better define what it means in L3, it might be harmful to have a test related to resource failures in the test suite.

@igrigorik
Copy link
Member

igrigorik commented Dec 1, 2016

+1 to removing those tests. Also, opened w3c/resource-timing#85 for spec update, ptal.

@siusin other then removing the test, everything else looks good to me!

@siusin
Copy link
Contributor Author

siusin commented Dec 1, 2016

Ok, thanks!

igrigorik added a commit to w3c/resource-timing that referenced this pull request Dec 1, 2016
@plehegar
Copy link
Member

plehegar commented Dec 6, 2016

LGTM

@plehegar plehegar closed this Dec 6, 2016
@plehegar plehegar reopened this Dec 6, 2016
@plehegar plehegar merged commit 0592a96 into web-platform-tests:master Dec 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants