Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1514483002:
DevTools: brush up new details cards on timeline. (Closed)
|
Created:
5 years ago by pfeldman Modified:
5 years 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, sergeyv+blink_chromium.org, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: brush up new details cards on timeline.
NOTRY=true
Committed: https://crrev.com/0942804bd0d8d11de74e95af317baf569b5af828
Cr-Commit-Position: refs/heads/master@{#363965}
Patch Set 1 #
Total comments: 5
Patch Set 2 : review comments addressed. #
Total comments: 5
Patch Set 3 : #Patch Set 4 : #
Messages
Total messages: 23 (9 generated)
pfeldman@chromium.org changed reviewers: + alph@chromium.org
pfeldman@chromium.org changed reviewers: + caseq@chromium.org
Screenshot?
On 2015/12/09 00:39:31, caseq wrote: > Screenshot? No user-noticeable changes here :P
On 2015/12/09 00:39:56, pfeldman wrote: > On 2015/12/09 00:39:31, caseq wrote: > > Screenshot? > > No user-noticeable changes here :P Why add over 50 lines of code then?
https://codereview.chromium.org/1514483002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js (right): https://codereview.chromium.org/1514483002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js:607: contentHelper.element.classList.add("timeline-details-lineup"); What does this do?
https://codereview.chromium.org/1514483002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js (right): https://codereview.chromium.org/1514483002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js:752: contentHelper = new WebInspector.TimelineDetailsContentHelper(model.target(), linkifier, relatedNodesMap); What is this meant to do? What happens to the content of previous helper?
https://codereview.chromium.org/1514483002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js (right): https://codereview.chromium.org/1514483002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js:697: contentHelper.appendTextRow(WebInspector.UIString("Nodes That Need Layout"), beginData["dirtyObjects"]); may be keep total, e.g.: 16 of 33. https://codereview.chromium.org/1514483002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js:1607: result.appendChild(contentHelper.element); doing it twice. https://codereview.chromium.org/1514483002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/timeline/timelinePanel.css (right): https://codereview.chromium.org/1514483002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/timeline/timelinePanel.css:686: padding-left: 10px; I guess it also needs some right padding.
PTAL
https://codereview.chromium.org/1514483002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js (right): https://codereview.chromium.org/1514483002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js:757: contentHelper.appendElementRow(relatedNodeLabel || WebInspector.UIString("Related Node"), WebInspector.DOMPresentationUtils.linkifyNodeReference(relatedNode)); So this will go into the same section with previewElement in case one is present and to the "details" section if preview is missing? I don't think this is expected behavior here. https://codereview.chromium.org/1514483002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js:1939: this.element = createElementWithClass("div", "timeline-details-view-block"); s/element/_currentElement/ (note this needs to be private)
lgtm https://codereview.chromium.org/1514483002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js (right): https://codereview.chromium.org/1514483002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js:764: if (event.stackTrace || (event.initiator && event.initiator.stackTrace) || event.invalidationTrackingEvents) { drop {} https://codereview.chromium.org/1514483002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js:770: contentHelper.addSection(WebInspector.UIString("Aggregated Time")); could you turn it into the element name? https://codereview.chromium.org/1514483002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js:973: contentHelper.appendStackTrace(stackLabel || WebInspector.UIString("Stack Trace"), event.stackTrace); Two titles "call stacks" and "stack trace" look strange.
The CQ bit was checked by pfeldman@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/1514483002/#ps60001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1514483002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1514483002/60001
Description was changed from ========== DevTools: brush up new details cards on timeline. ========== to ========== DevTools: brush up new details cards on timeline. NOTRY=true ==========
The CQ bit was unchecked by pfeldman@chromium.org
The CQ bit was checked by pfeldman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1514483002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1514483002/60001
Message was sent while issue was closed.
Description was changed from ========== DevTools: brush up new details cards on timeline. NOTRY=true ========== to ========== DevTools: brush up new details cards on timeline. NOTRY=true ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== DevTools: brush up new details cards on timeline. NOTRY=true ========== to ========== DevTools: brush up new details cards on timeline. NOTRY=true Committed: https://crrev.com/0942804bd0d8d11de74e95af317baf569b5af828 Cr-Commit-Position: refs/heads/master@{#363965} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/0942804bd0d8d11de74e95af317baf569b5af828 Cr-Commit-Position: refs/heads/master@{#363965} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
