Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2899023003:
buffer paint entries so they can queried with getEntries (Closed)
DescriptionBuffer paint entries so they can be queried with getEntries
BUG=657826, 657825
Review-Url: https://codereview.chromium.org/2899023003
Cr-Commit-Position: refs/heads/master@{#474096}
Committed: https://chromium.googlesource.com/chromium/src/+/472fbf0ccd5b03bf09f4616d437ad65a8ca62f2b
Patch Set 1 #
Total comments: 2
Patch Set 2 : update test #
Total comments: 4
Messages
Total messages: 24 (10 generated)
panicker@chromium.org changed reviewers: + skobes@chromium.org, tdresser@chromium.org
PTAL.
rs lgtm
Needs a test. Does this work with the |buffered| parameter when creating the observer? (https://w3c.github.io/performance-timeline/#dom-performanceobserverinit-buffered) Or do we not support |buffered| at all? https://codereview.chromium.org/2899023003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right): https://codereview.chromium.org/2899023003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:420: // Always buffer First Paint & First Contentful Paint Missing trailing .
On 2017/05/23 17:20:34, tdresser wrote: > Needs a test. Updated (was in the midst of it) > > Does this work with the |buffered| parameter when creating the observer? > (https://w3c.github.io/performance-timeline/#dom-performanceobserverinit-buffered) > > Or do we not support |buffered| at all? We don't support buffered flag yet, we should do so soon though. > > https://codereview.chromium.org/2899023003/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right): > > https://codereview.chromium.org/2899023003/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/timing/PerformanceBase.cpp:420: // Always buffer > First Paint & First Contentful Paint > Missing trailing .
https://codereview.chromium.org/2899023003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right): https://codereview.chromium.org/2899023003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:420: // Always buffer First Paint & First Contentful Paint On 2017/05/23 17:20:34, tdresser wrote: > Missing trailing . Done.
The CQ bit was checked by panicker@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Do we have a bug for supporting the buffered option? Might be a good task for Max. https://codereview.chromium.org/2899023003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/observable.html (right): https://codereview.chromium.org/2899023003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/observable.html:31: // Check buffer I think we try for tailing "."'s in JS too, don't we?
LGTM with above nit.
I filed a bug for the buffered flag. https://codereview.chromium.org/2899023003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/observable.html (right): https://codereview.chromium.org/2899023003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/observable.html:31: // Check buffer On 2017/05/23 17:33:57, tdresser wrote: > I think we try for tailing "."'s in JS too, don't we? Is this in the style guide somewhere?
https://codereview.chromium.org/2899023003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/observable.html (right): https://codereview.chromium.org/2899023003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/observable.html:31: // Check buffer On 2017/05/23 17:44:11, panicker wrote: > On 2017/05/23 17:33:57, tdresser wrote: > > I think we try for tailing "."'s in JS too, don't we? > > Is this in the style guide somewhere? JS style guide says follow C++ guide for comments https://google.github.io/styleguide/javascriptguide.xml?showone=Comments#Comm... C++ guide, uh, hmmm... I was pretty sure this was the case, but I can't find any reference to it other than https://www.chromium.org/developers/learning-your-way-around-the-code. It may have ceased to be a rule at some point.
On 2017/05/23 18:11:14, tdresser wrote: > C++ guide, uh, hmmm... I was pretty sure this was the case, but I can't find any > reference to it other than > https://www.chromium.org/developers/learning-your-way-around-the-code. https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_G... "Comments should be as readable as narrative text, with proper capitalization and punctuation."
On 2017/05/23 18:44:13, skobes wrote: > On 2017/05/23 18:11:14, tdresser wrote: > > C++ guide, uh, hmmm... I was pretty sure this was the case, but I can't find > any > > reference to it other than > > https://www.chromium.org/developers/learning-your-way-around-the-code. > > https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_G... > > "Comments should be as readable as narrative text, with proper capitalization > and punctuation." BTW I think this should apply to change descriptions too. :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== buffer paint entries so they can queried with getEntries BUG=657826,657825 ========== to ========== Buffer paint entries so they can be queried with getEntries BUG=657826,657825 ==========
https://codereview.chromium.org/2899023003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/observable.html (right): https://codereview.chromium.org/2899023003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/observable.html:31: // Check buffer On 2017/05/23 18:11:14, tdresser wrote: > On 2017/05/23 17:44:11, panicker wrote: > > On 2017/05/23 17:33:57, tdresser wrote: > > > I think we try for tailing "."'s in JS too, don't we? > > > > Is this in the style guide somewhere? > > JS style guide says follow C++ guide for comments > https://google.github.io/styleguide/javascriptguide.xml?showone=Comments#Comm... > > C++ guide, uh, hmmm... I was pretty sure this was the case, but I can't find any > reference to it other than > https://www.chromium.org/developers/learning-your-way-around-the-code. > > It may have ceased to be a rule at some point. not sure "proper punctuation" entails a period for two-word comment, but added anyway :)
The CQ bit was checked by panicker@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skobes@chromium.org Link to the patchset: https://codereview.chromium.org/2899023003/#ps20001 (title: "update test")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1495580283859590,
"parent_rev": "a1ebacdb1bbacbf534ef9f8dc763bc77273fdfd0", "commit_rev":
"472fbf0ccd5b03bf09f4616d437ad65a8ca62f2b"}
Message was sent while issue was closed.
Description was changed from ========== Buffer paint entries so they can be queried with getEntries BUG=657826,657825 ========== to ========== Buffer paint entries so they can be queried with getEntries BUG=657826,657825 Review-Url: https://codereview.chromium.org/2899023003 Cr-Commit-Position: refs/heads/master@{#474096} Committed: https://chromium.googlesource.com/chromium/src/+/472fbf0ccd5b03bf09f4616d437a... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/472fbf0ccd5b03bf09f4616d437a... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
