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 takeRecords() to PerformanceObserver #97

Closed
wants to merge 2 commits into from
Closed

Conversation

npm1
Copy link
Contributor

@npm1 npm1 commented Nov 28, 2017

Copy link
Contributor

@tdresser tdresser left a comment

Choose a reason for hiding this comment

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

Shubhie, what's the process for landing this?

Should we send a message to the web perf wg linking this pull request?

index.html Outdated
<section>
<h2><dfn>takeRecords()</dfn> method</h2>
<p>The <a>takeRecords()</a> method returns a copy of the <a>observer
buffer</a> and empties the observer buffer.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs to be the context object's observer buffer.

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

index.html Outdated
@@ -364,6 +365,11 @@ <h2><dfn>disconnect()</dfn> method</h2>
<a>relevant global object</a>, and also empty <a>context object</a>'s
<a>observer buffer</a>.</p>
</section>
<section>
<h2><dfn>takeRecords()</dfn> method</h2>
<p>The <a>takeRecords()</a> method returns a copy of the <a>observer
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the observer buffer can be directly returned.

Can we phrase this in terms of step 4.2?

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. Looks almost equivalent to me

@npm1
Copy link
Contributor Author

npm1 commented Nov 28, 2017

I created a W3C Account but after doing the confirmation I got on my email it still said it requires some sort of verification (maybe manually by someone in the group, not sure). If that happens then I can link that account to Github and the lpr check will stop failing I think

@spanicker
Copy link

Since Ilya will be back next week, makes sense to let him review.

@igrigorik
Copy link
Member

Apologies about the delay. FWIW, I have an existing WIP PR that added takeRecords: c29d569

My preference is to spell out the algorithm as in above PR. WDYT?

We can break apart the above PR and land takeRecords + other editorial updates in that PR, and move #entryTypes and buffer logic into separate PR.

@npm1
Copy link
Contributor Author

npm1 commented Dec 8, 2017

The change from c29d569 looks good to me. Would you like to submit a PR to gh-pages with those changes? Or do you want me to modify this PR to describe takeRecords as in c29d569?

@igrigorik
Copy link
Member

Opened #98, let's continue there.

@igrigorik igrigorik closed this Dec 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants