Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(17)

Issue 826713005: DevTools: Highlight changed scope variables as they change (Closed)

Created:
5 years, 6 months ago by paulirish
Modified:
5 years, 1 month 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.

Description

Highlight 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+106 lines, -17 lines) Patch
M Source/devtools/front_end/components/ObjectPropertiesSection.js View 1 2 3 4 7 chunks +87 lines, -8 lines 0 comments Download
M Source/devtools/front_end/main/Main.js View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M Source/devtools/front_end/sources/ScopeChainSidebarPane.js View 1 2 4 chunks +7 lines, -7 lines 0 comments Download
M Source/devtools/front_end/sources/WatchExpressionsSidebarPane.js View 1 2 3 4 3 chunks +11 lines, -2 lines 0 comments Download

Messages

Total messages: 24 (5 generated)
paulirish
Sandip will monitor the review and I'll ferry the patches up for this one.
5 years, 6 months ago (2015-01-12 04:31:56 UTC) #2
aandrey
https://codereview.chromium.org/826713005/diff/1/Source/devtools/front_end/components/ObjectPropertiesSection.js File Source/devtools/front_end/components/ObjectPropertiesSection.js (right): https://codereview.chromium.org/826713005/diff/1/Source/devtools/front_end/components/ObjectPropertiesSection.js#newcode47 Source/devtools/front_end/components/ObjectPropertiesSection.js:47: this.pane = {}; // set in ScopeChainSidebarPane This should ...
5 years, 6 months ago (2015-01-12 14:50:36 UTC) #4
sandipchitale
On 2015/01/12 14:50:36, aandrey wrote: > https://codereview.chromium.org/826713005/diff/1/Source/devtools/front_end/components/ObjectPropertiesSection.js > File Source/devtools/front_end/components/ObjectPropertiesSection.js (right): > > https://codereview.chromium.org/826713005/diff/1/Source/devtools/front_end/components/ObjectPropertiesSection.js#newcode47 > ...
5 years, 6 months ago (2015-01-12 15:30:10 UTC) #5
sandipchitale
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/components/ObjectPropertiesSection.js ...
5 years, 6 months ago (2015-01-12 23:28:42 UTC) #6
paulirish1
Latest patch set is applied.
5 years, 6 months ago (2015-01-13 08:01:20 UTC) #7
aandrey
https://codereview.chromium.org/826713005/diff/20001/Source/devtools/front_end/components/ObjectPropertiesSection.js File Source/devtools/front_end/components/ObjectPropertiesSection.js (right): https://codereview.chromium.org/826713005/diff/20001/Source/devtools/front_end/components/ObjectPropertiesSection.js#newcode48 Source/devtools/front_end/components/ObjectPropertiesSection.js:48: this.pane = sidebarPane; Should not depend on SidebarPane, should ...
5 years, 6 months ago (2015-01-20 05:05:02 UTC) #8
aandrey
On 2015/01/20 05:05:02, aandrey wrote: > https://codereview.chromium.org/826713005/diff/20001/Source/devtools/front_end/components/ObjectPropertiesSection.js > File Source/devtools/front_end/components/ObjectPropertiesSection.js (right): > > https://codereview.chromium.org/826713005/diff/20001/Source/devtools/front_end/components/ObjectPropertiesSection.js#newcode48 > ...
5 years, 6 months ago (2015-01-20 05:13:37 UTC) #9
paulirish1
Latest patchset is applied.
5 years, 6 months ago (2015-01-21 07:41:49 UTC) #10
pfeldman
I don't think this is right conceptually. ObjectPropertiesSection / TreeElements just render remote objects. These ...
5 years, 6 months ago (2015-01-21 10:41:25 UTC) #12
sandipchitale
https://codereview.chromium.org/826713005/diff/40001/Source/devtools/front_end/common/Settings.js File Source/devtools/front_end/common/Settings.js (right): https://codereview.chromium.org/826713005/diff/40001/Source/devtools/front_end/common/Settings.js#newcode87 Source/devtools/front_end/common/Settings.js:87: this.highlightChangedProperties = this.createSetting("highlightChangedProperties", false); On 2015/01/21 10:41:25, pfeldman wrote: ...
5 years, 6 months ago (2015-01-22 07:46:34 UTC) #14
paulirish
new patchset is up. https://code.google.com/p/chromium/issues/detail?id=441374#c23 - Treat this as a Experiment as opposed to Setting ...
5 years, 6 months ago (2015-01-22 07:46:58 UTC) #15
pfeldman
This seems to be going the right direction, yet there is still area for improvement. ...
5 years, 6 months ago (2015-01-22 09:55:17 UTC) #16
yurys
On 2015/01/22 09:55:17, pfeldman wrote: > This seems to be going the right direction, yet ...
5 years, 6 months ago (2015-01-22 12:58:31 UTC) #17
sandipchitale
New patch set is up.
5 years, 6 months ago (2015-01-22 22:46:07 UTC) #18
sandipchitale
PTAL
5 years, 6 months ago (2015-01-28 14:45:55 UTC) #19
sandipchitale
PTAL
5 years, 5 months ago (2015-02-13 15:11:17 UTC) #20
paulirish
Sandip, my understanding is that this patch must use proper object tracking at the V8 ...
5 years, 5 months ago (2015-02-16 18:45:27 UTC) #21
sandipchitale
On 2015/02/16 18:45:27, paulirish wrote: > Sandip, my understanding is that this patch must use ...
5 years, 5 months ago (2015-02-16 19:45:17 UTC) #22
sandipchitale
5 years, 5 months ago (2015-02-16 19:49:06 UTC) #23
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.

Powered by Google App Engine
This is Rietveld 408576698