Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2346133002:
Generate long task timing entries when long task is reported. (Closed)
|
Created:
4 years, 3 months ago by panicker Modified:
4 years, 3 months ago CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, kozyatinskiy+blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionGenerate long task timing entries when long task is reported.
Depends on https://codereview.chromium.org/2343553005/
(haven't figured out how to show this as diffbase)
BUG=635596
Committed: https://crrev.com/a836bd29e3ca8f3c40a3a4108613a61a2a82f5e7
Cr-Commit-Position: refs/heads/master@{#419849}
Patch Set 1 #Patch Set 2 : add basic unittest #
Total comments: 8
Patch Set 3 : review comments and rebase to CL 2343553005 #
Total comments: 14
Patch Set 4 : address review comments #
Total comments: 8
Patch Set 5 : address review comments #Patch Set 6 : sync to base CL 2343553005 #
Messages
Total messages: 27 (14 generated)
Description was changed from ========== Generate long task timing entries when long task is reported. BUG=635596 ========== to ========== Generate long task timing entries when long task is reported. Depends on https://codereview.chromium.org/2343553005/ (haven't figured out how to show this as diffbase) BUG=635596 ==========
panicker@chromium.org changed reviewers: + caseq@chromium.org
PTAL. This should be good enough for next week's demo (at TPAC) so your review attention is much appreciated :) I will keep refining the attribution (cross origin cases) and beefing up tests when I return. Thanks! This CL depends on the other CL you are reviewing: https://codereview.chromium.org/2343553005/
dgozman@chromium.org changed reviewers: + dgozman@chromium.org
https://codereview.chromium.org/2346133002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp (right): https://codereview.chromium.org/2346133002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:26: if (securityOrigin1->toRawString() == securityOrigin2->toRawString()) { I don't think this is a correct way to compare origins -- this bypasses a lot of logic within SecurityOrigin. Perhaps just securityOrigin1->canAccess(securityOrigin2)? Also, we prefer "return condition" to "if (condition) return true; else return false;" https://codereview.chromium.org/2346133002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:97: if (frameContextLocations.size() == 1) { redundant check -- just convert it to DCHECK()? https://codereview.chromium.org/2346133002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h (right): https://codereview.chromium.org/2346133002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h:44: String getSanitizedLongTaskName( - no get prefixes in blink, please! - this actually passes a set by value -- i.e. copies it. https://codereview.chromium.org/2346133002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgentTest.cpp (right): https://codereview.chromium.org/2346133002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgentTest.cpp:28: String getSanitizedLongTaskName( remove "get"
PTAL https://codereview.chromium.org/2346133002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp (right): https://codereview.chromium.org/2346133002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:26: if (securityOrigin1->toRawString() == securityOrigin2->toRawString()) { On 2016/09/17 01:57:50, caseq wrote: > I don't think this is a correct way to compare origins -- this bypasses a lot of > logic within SecurityOrigin. Perhaps just > securityOrigin1->canAccess(securityOrigin2)? > > Also, we prefer "return condition" to "if (condition) return true; else return > false;" Good question. not 100% sure if canAccess is preferable over maybe isSameSchemeHostPort (stricter) but sounds good to start with. I will confirm as followup, I'm actively nailing down security considerations now. Feel free to follow along here: https://docs.google.com/document/d/1tIMI1gau_q6X5EBnjDNiFS5NWV9cpYJ5KKA7xPd3V... https://codereview.chromium.org/2346133002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:97: if (frameContextLocations.size() == 1) { On 2016/09/17 01:57:50, caseq wrote: > redundant check -- just convert it to DCHECK()? Done. https://codereview.chromium.org/2346133002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h (right): https://codereview.chromium.org/2346133002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h:44: String getSanitizedLongTaskName( On 2016/09/17 01:57:50, caseq wrote: > - no get prefixes in blink, please! > - this actually passes a set by value -- i.e. copies it. Done. https://codereview.chromium.org/2346133002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgentTest.cpp (right): https://codereview.chromium.org/2346133002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgentTest.cpp:28: String getSanitizedLongTaskName( On 2016/09/17 01:57:51, caseq wrote: > remove "get" Done.
https://youtu.be/ZB29OFBS2w8 On Sat, Sep 17, 2016 at 5:52 AM, <panicker@chromium.org> wrote: > Reviewers: caseq > CL: https://codereview.chromium.org/2346133002/ > > Message: > PTAL. > This should be good enough for next week's demo (at TPAC) so your review > attention is much appreciated :) > I will keep refining the attribution (cross origin cases) and beefing up > tests > when I return. > Thanks! > > This CL depends on the other CL you are reviewing: > https://codereview.chromium.org/2343553005/ > > Description: > Generate long task timing entries when long task is reported. > > Depends on https://codereview.chromium.org/2343553005/ > (haven't figured out how to show this as diffbase) > > BUG=635596 > > Affected files (+189, -2 lines): > M third_party/WebKit/Source/core/BUILD.gn > M third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h > M third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp > M third_party/WebKit/Source/core/inspector/InspectorWebPerfAgentTest.cpp > M third_party/WebKit/Source/core/timing/Performance.h > M third_party/WebKit/Source/core/timing/Performance.cpp > M third_party/WebKit/Source/core/timing/PerformanceBase.h > M third_party/WebKit/Source/core/timing/PerformanceBase.cpp > A third_party/WebKit/Source/core/timing/PerformanceTest.cpp > > > -- > You received this message because you are subscribed to the Google Groups > "DevTools Reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to devtools-reviews+unsubscribe@chromium.org. > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://youtu.be/ZB29OFBS2w8 On Sat, Sep 17, 2016 at 5:52 AM, <panicker@chromium.org> wrote: > Reviewers: caseq > CL: https://codereview.chromium.org/2346133002/ > > Message: > PTAL. > This should be good enough for next week's demo (at TPAC) so your review > attention is much appreciated :) > I will keep refining the attribution (cross origin cases) and beefing up > tests > when I return. > Thanks! > > This CL depends on the other CL you are reviewing: > https://codereview.chromium.org/2343553005/ > > Description: > Generate long task timing entries when long task is reported. > > Depends on https://codereview.chromium.org/2343553005/ > (haven't figured out how to show this as diffbase) > > BUG=635596 > > Affected files (+189, -2 lines): > M third_party/WebKit/Source/core/BUILD.gn > M third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h > M third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp > M third_party/WebKit/Source/core/inspector/InspectorWebPerfAgentTest.cpp > M third_party/WebKit/Source/core/timing/Performance.h > M third_party/WebKit/Source/core/timing/Performance.cpp > M third_party/WebKit/Source/core/timing/PerformanceBase.h > M third_party/WebKit/Source/core/timing/PerformanceBase.cpp > A third_party/WebKit/Source/core/timing/PerformanceTest.cpp > > > -- > You received this message because you are subscribed to the Google Groups > "DevTools Reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to devtools-reviews+unsubscribe@chromium.org. > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
If you make git branch A be upstream for a branch B, only the diff from A to B will be uploaded to B's code review, and it will automatically show "depends on A". https://codereview.chromium.org/2346133002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp (right): https://codereview.chromium.org/2346133002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:87: blink::DOMWindow* domWindow = m_inspectedFrames->root()->domWindow(); No need in blink:: prefix. https://codereview.chromium.org/2346133002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:88: blink::Performance* performance = DOMWindowPerformance::performance(*domWindow); I believe domWindow could be null here. https://codereview.chromium.org/2346133002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:103: return "multiple-contexts"; Are these user-visible? https://codereview.chromium.org/2346133002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:110: return String(culpritLocation->protocol()) + "//" curlpritLocation->href() ? https://codereview.chromium.org/2346133002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:113: // TODO(panicker): THIS IS NOT OKAY. Scrub this to the first Is this code under a flag or something? It's not okay to land something like this to be accessible in canary. https://codereview.chromium.org/2346133002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:117: if (culpritLocation->frame()->tree().isDescendantOf(rootFrame)) { It must be a descendant due to how InspectorInstrumentation works. Let's add a DCHECK instead. https://codereview.chromium.org/2346133002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:124: return ""; Unreachable code. MSVS will be disappointed :-)
PTAL https://codereview.chromium.org/2346133002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp (right): https://codereview.chromium.org/2346133002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:87: blink::DOMWindow* domWindow = m_inspectedFrames->root()->domWindow(); On 2016/09/19 18:09:49, dgozman wrote: > No need in blink:: prefix. Done. https://codereview.chromium.org/2346133002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:88: blink::Performance* performance = DOMWindowPerformance::performance(*domWindow); On 2016/09/19 18:09:49, dgozman wrote: > I believe domWindow could be null here. Added check. Although curious - can you elaborate for which cases? https://codereview.chromium.org/2346133002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:103: return "multiple-contexts"; On 2016/09/19 18:09:49, dgozman wrote: > Are these user-visible? they will be visible to performance observer in JS. Why do you ask? https://codereview.chromium.org/2346133002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:110: return String(culpritLocation->protocol()) + "//" On 2016/09/19 18:09:49, dgozman wrote: > curlpritLocation->href() ? Done. https://codereview.chromium.org/2346133002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:113: // TODO(panicker): THIS IS NOT OKAY. Scrub this to the first On 2016/09/19 18:09:49, dgozman wrote: > Is this code under a flag or something? It's not okay to land something like > this to be accessible in canary. Yes, it's behind this flag: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/Runti... Did you mean it's not okay even behind a flag? I've trimmed it down just to be safe. all the interesting stuff is here :) https://codereview.chromium.org/2346133002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:117: if (culpritLocation->frame()->tree().isDescendantOf(rootFrame)) { On 2016/09/19 18:09:49, dgozman wrote: > It must be a descendant due to how InspectorInstrumentation works. Let's add a > DCHECK instead. Acknowledged. https://codereview.chromium.org/2346133002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:124: return ""; On 2016/09/19 18:09:49, dgozman wrote: > Unreachable code. MSVS will be disappointed :-) Done.
caseq@chromium.org changed reviewers: - vinodsonkusare77@gmail.com
lgtm https://codereview.chromium.org/2346133002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp (right): https://codereview.chromium.org/2346133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:101: return "unknown"; nit: define some constant for this. https://codereview.chromium.org/2346133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:105: return "multiple-contexts"; ditto. https://codereview.chromium.org/2346133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:109: Location* culpritLocation = *frameContextLocations.begin().get(); nit: I think get() is redundant here. https://codereview.chromium.org/2346133002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h (right): https://codereview.chromium.org/2346133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h:48: const HeapHashSet<Member<Location>>& frameContextLocations, Frame* rootFrame); nit: no need to wrap like this in blink land, just keep the long line.
Again: will try to submit soon. If you have further comments feel free to add them, and I'll address in a follow-up CL. Thanks for the review! https://codereview.chromium.org/2346133002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp (right): https://codereview.chromium.org/2346133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:101: return "unknown"; On 2016/09/19 23:09:51, caseq wrote: > nit: define some constant for this. Done. https://codereview.chromium.org/2346133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:105: return "multiple-contexts"; On 2016/09/19 23:09:51, caseq wrote: > ditto. Done. https://codereview.chromium.org/2346133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:109: Location* culpritLocation = *frameContextLocations.begin().get(); On 2016/09/19 23:09:51, caseq wrote: > nit: I think get() is redundant here. Done. https://codereview.chromium.org/2346133002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h (right): https://codereview.chromium.org/2346133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h:48: const HeapHashSet<Member<Location>>& frameContextLocations, Frame* rootFrame); On 2016/09/19 23:09:51, caseq wrote: > nit: no need to wrap like this in blink land, just keep the long line. Done. Although what about wrapping for better readability? Is it actually recommended to not wrap? (nothing in the style guide about this)
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...
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
> Done. > Although what about wrapping for better readability? Is it actually recommended > to not wrap? > (nothing in the style guide about this) There's nothing that forbids wrap either, but I think the common approach is not to wrap moderately long lines, and when we need to wrap them, do so lazily, i.e. in this example, wrap before "Frame *".
The CQ bit was checked by panicker@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org Link to the patchset: https://codereview.chromium.org/2346133002/#ps100001 (title: "sync to base CL 2343553005")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Generate long task timing entries when long task is reported. Depends on https://codereview.chromium.org/2343553005/ (haven't figured out how to show this as diffbase) BUG=635596 ========== to ========== Generate long task timing entries when long task is reported. Depends on https://codereview.chromium.org/2343553005/ (haven't figured out how to show this as diffbase) BUG=635596 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Generate long task timing entries when long task is reported. Depends on https://codereview.chromium.org/2343553005/ (haven't figured out how to show this as diffbase) BUG=635596 ========== to ========== Generate long task timing entries when long task is reported. Depends on https://codereview.chromium.org/2343553005/ (haven't figured out how to show this as diffbase) BUG=635596 Committed: https://crrev.com/a836bd29e3ca8f3c40a3a4108613a61a2a82f5e7 Cr-Commit-Position: refs/heads/master@{#419849} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/a836bd29e3ca8f3c40a3a4108613a61a2a82f5e7 Cr-Commit-Position: refs/heads/master@{#419849} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
