Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1978323002:
DevTools: reveal object path in console tooltip (Closed)
|
Created:
4 years, 7 months ago by luoe Modified:
4 years, 7 months ago Reviewers:
dgozman 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. |
DescriptionDevTools: reveal object path in console tooltip
BUG=607691
Committed: https://crrev.com/552f0cca1e282832f21f591d503be714eb237911
Cr-Commit-Position: refs/heads/master@{#394801}
Patch Set 1 #Patch Set 2 : Always bracket notation #Patch Set 3 : Use regexes for approximate cases #Patch Set 4 : Removed console logs #
Total comments: 9
Patch Set 5 : Address comments in patch 4 #
Total comments: 6
Patch Set 6 : Address patch 5 comments #Patch Set 7 : Move option to bottom of list #Patch Set 8 : Remove uiutils and rebase #
Total comments: 3
Patch Set 9 : Add beforeShow #
Total comments: 4
Patch Set 10 : Address comments in patch 9 #
Messages
Total messages: 28 (10 generated)
Description was changed from ========== DevTools: reveal object path in console tooltip BUG=607691 ========== to ========== DevTools: reveal object path in console tooltip BUG=607691 ==========
luoe@chromium.org changed reviewers: + dgozman@chromium.org
The CQ bit was checked by luoe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1978323002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1978323002/60001
https://codereview.chromium.org/1978323002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js (right): https://codereview.chromium.org/1978323002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:330: if (this.title) Why this check? https://codereview.chromium.org/1978323002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:336: var parentPath = this.parent.nameElement ? this.parent.nameElement.title : "this"; Could there be no parent? https://codereview.chromium.org/1978323002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:345: _contextMenuFired: function(value, event) Let's annotate parameters. https://codereview.chromium.org/1978323002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:348: if (value && value.classList && value.classList.contains("name")) How can value be null? Or value.classList? https://codereview.chromium.org/1978323002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (right): https://codereview.chromium.org/1978323002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:663: return WebInspector.UIString.capitalize("Copy ^property ^path"); Just inline this at callsite.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/1978323002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js (right): https://codereview.chromium.org/1978323002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:330: if (this.title) On 2016/05/17 02:09:50, dgozman wrote: > Why this check? In the case when a property is a getter, clicking on the "..." to invoke the getter makes another call to .updatePropertyPath(), even though it was already called earlier. Although calling it twice won't hurt, this check just avoids the unnecessary call. Aah, I meant to use this.nameElement.title https://codereview.chromium.org/1978323002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:336: var parentPath = this.parent.nameElement ? this.parent.nameElement.title : "this"; On 2016/05/17 02:09:50, dgozman wrote: > Could there be no parent? Currently, ObjectPropertiesSection.js is the only file creating instances of ObjectPropertyTreeElement, and we always create it under a WebInspector.ObjectPropertiesSection.RootElement. Also, update() is called after onAttach(), so it should always have a parent. https://codereview.chromium.org/1978323002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:345: _contextMenuFired: function(value, event) On 2016/05/17 02:09:50, dgozman wrote: > Let's annotate parameters. Done. https://codereview.chromium.org/1978323002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:348: if (value && value.classList && value.classList.contains("name")) On 2016/05/17 02:09:50, dgozman wrote: > How can value be null? Or value.classList? Refactored to avoid this check
https://codereview.chromium.org/1978323002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js (right): https://codereview.chromium.org/1978323002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:347: _nameContextMenuFired: function(property, event) Did you preserve "Store as Global Variable" item? https://codereview.chromium.org/1978323002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:360: _valueContextMenuFired: function(property, event) I think we should display a single context menu both for name and value. https://codereview.chromium.org/1978323002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (right): https://codereview.chromium.org/1978323002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:663: return WebInspector.UIString.capitalize("Copy ^property ^path"); I still think you should inline this at callsite.
https://codereview.chromium.org/1978323002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js (right): https://codereview.chromium.org/1978323002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:347: _nameContextMenuFired: function(property, event) On 2016/05/18 00:46:14, dgozman wrote: > Did you preserve "Store as Global Variable" item? Yes, "Store as Global Variable" and others like "Show Function Definition", and "Show Generator Location" are preserved. https://codereview.chromium.org/1978323002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:360: _valueContextMenuFired: function(property, event) On 2016/05/18 00:46:14, dgozman wrote: > I think we should display a single context menu both for name and value. Sure, I'm fine with changing that. If these two are merged, "Copy property path" will be the first option in the menu, but I get the feeling that "Store as global variable" is used more often, and should be first. Do we have data on this? https://codereview.chromium.org/1978323002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (right): https://codereview.chromium.org/1978323002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:663: return WebInspector.UIString.capitalize("Copy ^property ^path"); On 2016/05/18 00:46:14, dgozman wrote: > I still think you should inline this at callsite. Sorry, I missed this in the last patch. Will do.
Okay, Copy property path is on the bottom now. At first, I was trying to add an "addLater" flag to appendItem, appendCustomItem, appendAction, appendSubMenuItem, appendCheckboxItem but felt this was complicated. Does a callback on .show() make sense?
Almost there! https://codereview.chromium.org/1978323002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js (right): https://codereview.chromium.org/1978323002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:354: contextMenu.show(function() { We disallow anonymous functions except for arrow functions. This would be a good arrow function. https://codereview.chromium.org/1978323002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/ui/ContextMenu.js (right): https://codereview.chromium.org/1978323002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/ui/ContextMenu.js:356: show: function(populatedCallback) Let's instead have ContextMenu.beforeShow(callback), which will execute |callback| right before |_innerShow|.
Comments addressed?
lgtm https://codereview.chromium.org/1978323002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js (right): https://codereview.chromium.org/1978323002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:354: contextMenu.beforeShow(() => { nit: make it a one-liner. https://codereview.chromium.org/1978323002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/ui/ContextMenu.js (right): https://codereview.chromium.org/1978323002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/ui/ContextMenu.js:404: this._beforeShow(); delete this._beforeShow;
https://codereview.chromium.org/1978323002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js (right): https://codereview.chromium.org/1978323002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:354: contextMenu.show(function() { On 2016/05/18 23:05:31, dgozman wrote: > We disallow anonymous functions except for arrow functions. This would be a good > arrow function. Done. https://codereview.chromium.org/1978323002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js (right): https://codereview.chromium.org/1978323002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:354: contextMenu.beforeShow(() => { On 2016/05/18 23:56:32, dgozman wrote: > nit: make it a one-liner. I made it a one liner, but the line was super long so I moved the handler function into a variable. Is this alright? https://codereview.chromium.org/1978323002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/ui/ContextMenu.js (right): https://codereview.chromium.org/1978323002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/ui/ContextMenu.js:404: this._beforeShow(); On 2016/05/18 23:56:32, dgozman wrote: > delete this._beforeShow; Done.
> I made it a one liner, but the line was super long so I moved the handler > function into a variable. Is this alright? Yes, that's good.
The CQ bit was checked by luoe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1978323002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1978323002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by luoe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/1978323002/#ps180001 (title: "Address comments in patch 9")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1978323002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1978323002/180001
Message was sent while issue was closed.
Description was changed from ========== DevTools: reveal object path in console tooltip BUG=607691 ========== to ========== DevTools: reveal object path in console tooltip BUG=607691 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== DevTools: reveal object path in console tooltip BUG=607691 ========== to ========== DevTools: reveal object path in console tooltip BUG=607691 Committed: https://crrev.com/552f0cca1e282832f21f591d503be714eb237911 Cr-Commit-Position: refs/heads/master@{#394801} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/552f0cca1e282832f21f591d503be714eb237911 Cr-Commit-Position: refs/heads/master@{#394801} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
