Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
826713005:
DevTools: Highlight changed scope variables as they change (Closed)
|
Created:
5 years, 11 months ago by paulirish Modified:
5 years, 5 months ago CC:
aandrey+blink_chromium.org, apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, devtools-reviews_chromium.org, eustas+blink_chromium.org, loislo+blink_chromium.org, lushnikov+blink_chromium.org, malch+blink_chromium.org, paulirish+reviews_chromium.org, pfeldman+blink_chromium.org, sergeyv+blink_chromium.org, vsevik+blink_chromium.org, yurys+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionHighlight changed scope variables as they change in
Sources' Scope Variables and Watch Expressions panes
Patch by Sandip Chitale
BUG=441374
Patch Set 1 #
Total comments: 1
Patch Set 2 : addressing feedback #
Total comments: 2
Patch Set 3 : addressing feedback. settings label. #
Total comments: 12
Patch Set 4 : now an experiment. logic in memento. comment #23 #Patch Set 5 : addressing feedback. v good points. simplified #
Messages
Total messages: 24 (5 generated)
paulirish@chromium.org changed reviewers: + aandrey@chromium.org
Sandip will monitor the review and I'll ferry the patches up for this one.
aandrey@chromium.org changed reviewers: + yurys@chromium.org
https://codereview.chromium.org/826713005/diff/1/Source/devtools/front_end/co... File Source/devtools/front_end/components/ObjectPropertiesSection.js (right): https://codereview.chromium.org/826713005/diff/1/Source/devtools/front_end/co... Source/devtools/front_end/components/ObjectPropertiesSection.js:47: this.pane = {}; // set in ScopeChainSidebarPane This should be better encapsulated. The ObjectPropertiesSection should not depend on ScopeChainSidebarPane in any way, even implicitly.
On 2015/01/12 14:50:36, aandrey wrote: > https://codereview.chromium.org/826713005/diff/1/Source/devtools/front_end/co... > File Source/devtools/front_end/components/ObjectPropertiesSection.js (right): > > https://codereview.chromium.org/826713005/diff/1/Source/devtools/front_end/co... > Source/devtools/front_end/components/ObjectPropertiesSection.js:47: this.pane = > {}; // set in ScopeChainSidebarPane > This should be better encapsulated. The ObjectPropertiesSection should not > depend on ScopeChainSidebarPane in any way, even implicitly. I did this because the Closure compiler complained that the pane property was not set. Do you have any suggestions on how to fix this?
On 2015/01/12 15:30:10, sandipchitale wrote: > On 2015/01/12 14:50:36, aandrey wrote: > > > https://codereview.chromium.org/826713005/diff/1/Source/devtools/front_end/co... > > File Source/devtools/front_end/components/ObjectPropertiesSection.js (right): > > > > > https://codereview.chromium.org/826713005/diff/1/Source/devtools/front_end/co... > > Source/devtools/front_end/components/ObjectPropertiesSection.js:47: this.pane > = > > {}; // set in ScopeChainSidebarPane > > This should be better encapsulated. The ObjectPropertiesSection should not > > depend on ScopeChainSidebarPane in any way, even implicitly. > > I did this because the Closure compiler complained that the pane property was > not set. Do you have any suggestions on how to fix this? It appears that ScopeChainSidebarPane injects itself into ObjectPropertiesSection on line 133 section.pane = this; of Source/devtools/front_end/sources/ScopeChainSidebarPane.js
Latest patch set is applied.
https://codereview.chromium.org/826713005/diff/20001/Source/devtools/front_en... File Source/devtools/front_end/components/ObjectPropertiesSection.js (right): https://codereview.chromium.org/826713005/diff/20001/Source/devtools/front_en... Source/devtools/front_end/components/ObjectPropertiesSection.js:48: this.pane = sidebarPane; Should not depend on SidebarPane, should not have this line at all. https://codereview.chromium.org/826713005/diff/20001/Source/devtools/front_en... Source/devtools/front_end/components/ObjectPropertiesSection.js:112: this.pane._propertyIdentifiers = {}; this is a no-go
On 2015/01/20 05:05:02, aandrey wrote: > https://codereview.chromium.org/826713005/diff/20001/Source/devtools/front_en... > File Source/devtools/front_end/components/ObjectPropertiesSection.js (right): > > https://codereview.chromium.org/826713005/diff/20001/Source/devtools/front_en... > Source/devtools/front_end/components/ObjectPropertiesSection.js:48: this.pane = > sidebarPane; > Should not depend on SidebarPane, should not have this line at all. > > https://codereview.chromium.org/826713005/diff/20001/Source/devtools/front_en... > Source/devtools/front_end/components/ObjectPropertiesSection.js:112: > this.pane._propertyIdentifiers = {}; > this is a no-go Is it possible to invert the control, so that ObjectPropertiesSection just provides information in a general way what nodes are currently expanded? Say, saveExpandedState() & restoreExpandedState() w/o introducing new dependencies. Otherwise, if a SidebarPane has several ObjectPropertiesSection's this approach won't work.
Latest patchset is applied.
pfeldman@chromium.org changed reviewers: + pfeldman@chromium.org
I don't think this is right conceptually. ObjectPropertiesSection / TreeElements just render remote objects. These should not diff anything, diffing utility should be external and should rely upon object identity, not the path. Diffing by description is also not formally correct. https://codereview.chromium.org/826713005/diff/40001/Source/devtools/front_en... File Source/devtools/front_end/common/Settings.js (right): https://codereview.chromium.org/826713005/diff/40001/Source/devtools/front_en... Source/devtools/front_end/common/Settings.js:87: this.highlightChangedProperties = this.createSetting("highlightChangedProperties", false); This should not be a setting, but rather an experiment. https://codereview.chromium.org/826713005/diff/40001/Source/devtools/front_en... File Source/devtools/front_end/components/ObjectPropertiesSection.js (right): https://codereview.chromium.org/826713005/diff/40001/Source/devtools/front_en... Source/devtools/front_end/components/ObjectPropertiesSection.js:48: this.memento = memento; Should be private. https://codereview.chromium.org/826713005/diff/40001/Source/devtools/front_en... Source/devtools/front_end/components/ObjectPropertiesSection.js:100: // Delete the cached last descriptions of all propertyIdentifiers No need for comments. https://codereview.chromium.org/826713005/diff/40001/Source/devtools/front_en... Source/devtools/front_end/components/ObjectPropertiesSection.js:157: WebInspector.ObjectPropertiesMemento = function() { Here and below, { on the next line. https://codereview.chromium.org/826713005/diff/40001/Source/devtools/front_en... Source/devtools/front_end/components/ObjectPropertiesSection.js:193: getLastDescriptionForPropertyPath: function(propertyPath) { no get prefixes in Blink https://codereview.chromium.org/826713005/diff/40001/Source/devtools/front_en... Source/devtools/front_end/components/ObjectPropertiesSection.js:276: var lastDesription = this.treeOutline.section.memento.getLastDescriptionForPropertyPath(this.propertyPath()) This code can't be a part of the ObjectPropertyTreeElement which is just a tree that renders remote objects. You can try decorating it externally, but baking this functionality into it is wrong.
sandipchitale@gmail.com changed reviewers: + sandipchitale@gmail.com
https://codereview.chromium.org/826713005/diff/40001/Source/devtools/front_en... File Source/devtools/front_end/common/Settings.js (right): https://codereview.chromium.org/826713005/diff/40001/Source/devtools/front_en... Source/devtools/front_end/common/Settings.js:87: this.highlightChangedProperties = this.createSetting("highlightChangedProperties", false); On 2015/01/21 10:41:25, pfeldman wrote: > This should not be a setting, but rather an experiment. Acknowledged. https://codereview.chromium.org/826713005/diff/40001/Source/devtools/front_en... File Source/devtools/front_end/components/ObjectPropertiesSection.js (right): https://codereview.chromium.org/826713005/diff/40001/Source/devtools/front_en... Source/devtools/front_end/components/ObjectPropertiesSection.js:48: this.memento = memento; On 2015/01/21 10:41:25, pfeldman wrote: > Should be private. Acknowledged. https://codereview.chromium.org/826713005/diff/40001/Source/devtools/front_en... Source/devtools/front_end/components/ObjectPropertiesSection.js:100: // Delete the cached last descriptions of all propertyIdentifiers On 2015/01/21 10:41:25, pfeldman wrote: > No need for comments. Acknowledged. https://codereview.chromium.org/826713005/diff/40001/Source/devtools/front_en... Source/devtools/front_end/components/ObjectPropertiesSection.js:157: WebInspector.ObjectPropertiesMemento = function() { On 2015/01/21 10:41:25, pfeldman wrote: > Here and below, { on the next line. Acknowledged. https://codereview.chromium.org/826713005/diff/40001/Source/devtools/front_en... Source/devtools/front_end/components/ObjectPropertiesSection.js:193: getLastDescriptionForPropertyPath: function(propertyPath) { On 2015/01/21 10:41:25, pfeldman wrote: > no get prefixes in Blink Acknowledged. https://codereview.chromium.org/826713005/diff/40001/Source/devtools/front_en... Source/devtools/front_end/components/ObjectPropertiesSection.js:276: var lastDesription = this.treeOutline.section.memento.getLastDescriptionForPropertyPath(this.propertyPath()) On 2015/01/21 10:41:25, pfeldman wrote: > This code can't be a part of the ObjectPropertyTreeElement which is just a tree > that renders remote objects. You can try decorating it externally, but baking > this functionality into it is wrong. Will work on this.
new patchset is up. https://code.google.com/p/chromium/issues/detail?id=441374#c23 - Treat this as a Experiment as opposed to Setting - Fix tabs - Move special logic into Memento - Fixed { - Removed comments - Removed get prefix
This seems to be going the right direction, yet there is still area for improvement. First, imagine that ObjectPropertySection is a black box component. What is the bare minimum set of generic changes that you need to apply to make it do what you want? 1) You need to teach it how to decorate rendered values. That's where you are going to animate / highlight individual values. 2) You need to make it retain expanded paths based on some object identity the rest of the functionality could be implemented as an external class. (1) requires a thin ObjectPropertySection delegate, while (2) could be either implemented using delegate or via making actual object ids persistent. @yurys: where are we with the persistent ids?
On 2015/01/22 09:55:17, pfeldman wrote: > This seems to be going the right direction, yet there is still area for > improvement. > > First, imagine that ObjectPropertySection is a black box component. What is the > bare minimum set of generic changes that you need to apply to make it do what > you want? > > 1) You need to teach it how to decorate rendered values. That's where you are > going to animate / highlight individual values. > 2) You need to make it retain expanded paths based on some object identity > > the rest of the functionality could be implemented as an external class. > > (1) requires a thin ObjectPropertySection delegate, while (2) could be either > implemented using delegate or via making actual object ids persistent. @yurys: > where are we with the persistent ids? Stable ids support is tracked by crbug.com/437416. I've taken several attempts to implement that but naive approaches didn't work out and I decided to postpone it. I'll try to summarize my findings in that bug report. If there is an interest in that being implemented we may raise its priority.
New patch set is up.
PTAL
PTAL
Sandip, my understanding is that this patch must use proper object tracking at the V8 level. That work was attempted but stalled here: https://code.google.com/p/chromium/issues/detail?id=437416#c3 It appears this patch here will be stalled until issue 437416 is resolved.
On 2015/02/16 18:45:27, paulirish wrote: > Sandip, my understanding is that this patch must use proper object tracking at > the V8 level. > That work was attempted but stalled here: > https://code.google.com/p/chromium/issues/detail?id=437416#c3 > > It appears this patch here will be stalled until issue 437416 is resolved. Hi Paul, I am not sure if my implementation is dependent on the fix related to proper object tracking at the V8 level. The object tracking only helps in distinguishing the following: 1 function Person(firstName) { 2 this.firstName = firstName; 3 } 4 var john = new Person("John"); 5 john = new Person("John"); As you can see variable john was assigned a new instance of Person again even though the instance has exact same properties as the instance on line 4. This should ideally be highlighted as a change for the reference john. This is the only case that is not handled by my implementation. That is because it can only be detected by comparing the "stable" remote object id. Other than this use case my implementation works perfectly. But I understand if it needs to wait for the other fix to come in. Regards, Sandip
On 2015/02/16 19:45:17, sandipchitale wrote: > On 2015/02/16 18:45:27, paulirish wrote: > > Sandip, my understanding is that this patch must use proper object tracking at > > the V8 level. > > That work was attempted but stalled here: > > https://code.google.com/p/chromium/issues/detail?id=437416#c3 > > > > It appears this patch here will be stalled until issue 437416 is resolved. > > Hi Paul, > > I am not sure if my implementation is dependent on the fix related to proper > object tracking at the V8 level. > > The object tracking only helps in distinguishing the following: > > 1 function Person(firstName) { > 2 this.firstName = firstName; > 3 } > 4 var john = new Person("John"); > 5 john = new Person("John"); > > As you can see variable john was assigned a new instance of Person again even > though the instance has exact same properties as the instance on line 4. This > should ideally be highlighted as a change for the reference john. This is the > only case that is not handled by my implementation. That is because it can only > be detected by comparing the "stable" remote object id. Other than this use case > my implementation works perfectly. > > But I understand if it needs to wait for the other fix to come in. > > Regards, > Sandip In the mean time, to try out this feature (and other enhancements by me), folks can use the devtools at: http://chrome-developer-tools.googlecode.com/git-history/enhanced/devtools-fr... by loading it in DevTools App: https://chrome.google.com/webstore/detail/dev-tools-app/eichfopopofffkbhjgbab... Read details here: http://sandipchitale.blogspot.com/2015/02/single-devtools-with-all-my-enhance... via Chrome DevTools App.
pfeldman@chromium.org changed reviewers: - pfeldman@chromium.org |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
