Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2156523003:
[DevTools] Fix links for JSFrame records in timeline (Closed)
|
Created:
4 years, 5 months ago by kozy Modified:
4 years, 5 months ago Reviewers:
alph 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, sergeyv+blink_chromium.org, pfeldman, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[DevTools] Fix links for JSFrame records in timeline
R=alph@chromium.org
Committed: https://crrev.com/d7433a9450809dcbf1d8bda2a2cd6c483135849f
Cr-Commit-Position: refs/heads/master@{#406203}
Patch Set 1 #
Total comments: 1
Patch Set 2 : better solution #Patch Set 3 #
Total comments: 6
Patch Set 4 : addressed comments #Patch Set 5 : rebased #
Depends on Patchset: Messages
Total messages: 30 (16 generated)
Description was changed from ========== [DevTools] Make tracing event from CPU profiler 1-based Tracing events are currently 1-based. CPU profiler node was moved to 0-based. We need to transform it back. R=alph@chromium.org ========== to ========== [DevTools] Tracing event from CPU profiler should be 1-based Tracing events are currently 1-based. CPU profiler node was moved to 0-based. We need to transform it back. R=alph@chromium.org ==========
Please take a look.
https://codereview.chromium.org/2156523003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/timeline_model/TimelineJSProfile.js (right): https://codereview.chromium.org/2156523003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/timeline_model/TimelineJSProfile.js:39: functionName: node.functionName, You're creating a bunch of objects here. There were just a few nodes per profile, but you duplicating them for each sample. Moreover you are making RuntimeAgent.CallFrame's with a 1-based numbers that breaks the contract. I don't like this solution.
Better solution uploaded. Please take a look!
Description was changed from ========== [DevTools] Tracing event from CPU profiler should be 1-based Tracing events are currently 1-based. CPU profiler node was moved to 0-based. We need to transform it back. R=alph@chromium.org ========== to ========== [DevTools] JSFrame records on timeline are 0-based Linkify JSFrame record correctly in timeline. R=alph@chromium.org ==========
Description was changed from ========== [DevTools] JSFrame records on timeline are 0-based Linkify JSFrame record correctly in timeline. R=alph@chromium.org ========== to ========== [DevTools] Fix links for JSFrame records in timeline R=alph@chromium.org ==========
The CQ bit was checked by kozyatinskiy@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...
https://codereview.chromium.org/2156523003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js (right): https://codereview.chromium.org/2156523003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js:619: return frame ? linkifier.linkifyConsoleCallFrameForTracing(target, frame, "timeline-details") : null; I think this can be a "tracing" frame as well as a "profiler" frame.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2156523003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js (right): https://codereview.chromium.org/2156523003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js:619: return frame ? linkifier.linkifyConsoleCallFrameForTracing(target, frame, "timeline-details") : null; On 2016/07/16 01:33:36, alph wrote: > I think this can be a "tracing" frame as well as a "profiler" frame. I think that all frames from profiler have JSFrame type. linkifyTopCallFrame is called once in default case of switch that contains separate case for recordType.JSFrame.
lgtm https://codereview.chromium.org/2156523003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/Linkifier.js (right): https://codereview.chromium.org/2156523003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:218: // TODO(kozyatinskiy): remove this when tracing will migrate to 0-based lineNumber and columnNumber. I's prefer that once an object is claimed to be a Runtime.CallFrame it already had 0-based numbers. But we can address it later. https://codereview.chromium.org/2156523003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/timeline/TimelineTreeView.js (right): https://codereview.chromium.org/2156523003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/timeline/TimelineTreeView.js:87: linkifyLocationForTracing: function(frame) nit: Make it private.
thanks! all done. https://codereview.chromium.org/2156523003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/Linkifier.js (right): https://codereview.chromium.org/2156523003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:218: // TODO(kozyatinskiy): remove this when tracing will migrate to 0-based lineNumber and columnNumber. On 2016/07/18 19:49:21, alph wrote: > I's prefer that once an object is claimed to be a Runtime.CallFrame it already > had 0-based numbers. But we can address it later. Acknowledged. https://codereview.chromium.org/2156523003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/timeline/TimelineTreeView.js (right): https://codereview.chromium.org/2156523003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/timeline/TimelineTreeView.js:87: linkifyLocationForTracing: function(frame) On 2016/07/18 19:49:21, alph wrote: > nit: Make it private. Done.
The CQ bit was checked by kozyatinskiy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alph@chromium.org Link to the patchset: https://codereview.chromium.org/2156523003/#ps60001 (title: "addressed comments")
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
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by kozyatinskiy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alph@chromium.org Link to the patchset: https://codereview.chromium.org/2156523003/#ps80001 (title: "rebased")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2157263002 Patch 1). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by kozyatinskiy@chromium.org
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 ========== [DevTools] Fix links for JSFrame records in timeline R=alph@chromium.org ========== to ========== [DevTools] Fix links for JSFrame records in timeline R=alph@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== [DevTools] Fix links for JSFrame records in timeline R=alph@chromium.org ========== to ========== [DevTools] Fix links for JSFrame records in timeline R=alph@chromium.org Committed: https://crrev.com/d7433a9450809dcbf1d8bda2a2cd6c483135849f Cr-Commit-Position: refs/heads/master@{#406203} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/d7433a9450809dcbf1d8bda2a2cd6c483135849f Cr-Commit-Position: refs/heads/master@{#406203} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
