Bug 29714

Summary: apply a style change to scope variables as they change from step to step
Product: WebKit Reporter: Patrick Mueller <pmuellr>
Component: Web InspectorAssignee: Nobody <webkit-unassigned>
Status: RESOLVED INVALID    
Severity: Enhancement CC: aroben, burg, graouts, joepeck, kmccullough, me, pfeldman, sandipchitale, timothy, vivekg, webkit-bug-importer
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 109908    
Bug Blocks:    
Attachments:
Description Flags
Possible patch to implement the functionality
none
Improved patch. Now computes diffs relative to the nodes that were shown when the debugger was suspended last.
none
step 1 screenshot
none
step 2 screenshot
none
step 3 screenshot
none
step 4 screenshot
none
step 5 screenshot
none
This patch incorporates the formatting feedback.
none
The previous patch broke the Watch Expressions pane. This fixes it.
none
This is a patch against the latest code none

Description Patrick Mueller 2009-09-24 07:46:07 PDT
As the user is stepping through their code in Web Inspector, it would be handy to be able to highlight values which have changed from before they made a step to the next pause state.

Lots of thoughts immediately come to mind:

- how do you tell when the user has just 'stepped" vs hitting a breakpoint after a run?  Presumably it only makes sense to show items that have changed when you're stepping, rather than since the last pause state to the current pause state.

- how deeply do we check for changes?  Just the top level properties in the various sections?  The leafs in the expanded sections?

- presumably new properties/variables would get marked as "changes" - as well as a normal property change; what about property/variable deletions?  I'm imagining showing some ghostly or strike-out representation of the deleted property/variable

- how do we visually indicate "changed".  Note this also needs to take into account property value styling as per bug 27235 .  Namely, we can't use the same style for both.  I'm thinking a background color - maybe yellow-ish - could be used to indicate "changed".  Rounded corners on the background of course!  This might preclude using background colors for value styling, which we might want to do for 'special' values such as exceptions, null, undefined, NaN, Infinity, etc.  Though if those values used a dark background, the combination of backgrounds might be distinctive enough to be able to easily tell that both characteristics are applicable.  Alternatively, the "changed" styling background might extend to the left of the usual margin for the property value, so you could visually distinguish that as well.
Comment 1 Sandip Chitale 2013-02-14 23:47:49 PST
I would like this feature also.

I was trying to implement a patch for it, but ran into an issue of stability of the runtime.RemoteObject.objectId. I appears that this id is not the same for the same object (say window object) between successive stopping of the debugee. This prevents the frontend based implementation of diff computation. May be the backend should simply set and additional property of runtime.RemoteObject, say state to one of added or changed which can be used by the frontend...more precisely near this code in DevTools.js:

:
WebInspector.ObjectPropertyTreeElement.prototype = {
:
:
:
update: function()
{
:
:
this.nameElement.textContent = this.property.name;
if ('added' == this.property.value.state) {
    // add a class to the to row to indicate new property
} else if ('changed' == this.property.value.changed) {
    // add a class to the to row to indicate change property
}
:
:
Comment 2 Sandip Chitale 2013-02-24 01:26:37 PST
Created attachment 189962 [details]
Possible patch to implement the functionality

This is my first attempt at implementing the functionality of applying a style change to scope variables as they change from step to step. The idea is simple - keep track of values that the user saw in last iteration for any expanded nodes. Next time compare, if different apply a different style class.
Comment 3 Sandip Chitale 2013-02-24 01:29:26 PST
Just added a possible patch. I still need to refine it further.
Comment 4 Sandip Chitale 2013-02-26 00:49:57 PST
Created attachment 190223 [details]
Improved patch. Now computes diffs relative to the nodes that were shown when the debugger was suspended last.
Comment 5 Sandip Chitale 2013-02-26 00:51:24 PST
Created attachment 190224 [details]
step 1 screenshot
Comment 6 Sandip Chitale 2013-02-26 00:51:54 PST
Created attachment 190225 [details]
step 2 screenshot
Comment 7 Sandip Chitale 2013-02-26 00:52:27 PST
Created attachment 190226 [details]
step 3 screenshot
Comment 8 Sandip Chitale 2013-02-26 00:52:49 PST
Created attachment 190228 [details]
step 4 screenshot
Comment 9 Sandip Chitale 2013-02-26 00:53:20 PST
Created attachment 190229 [details]
step 5 screenshot
Comment 10 Pavel Feldman 2013-02-26 00:57:49 PST
> - how deeply do we check for changes?  Just the top level properties in the various sections?  The leafs in the expanded sections?

We should compare primitive values and rely on object identity.

> 
> - presumably new properties/variables would get marked as "changes" - as well as a normal property change; what about property/variable deletions?  I'm imagining showing some ghostly or strike-out representation of the deleted property/variable

We should not go deep. New properties in the scope are just new scope variables.
Comment 11 Vivek Galatage 2013-02-26 01:00:41 PST
Comment on attachment 190223 [details]
Improved patch. Now computes diffs relative to the nodes that were shown when the debugger was suspended last.

View in context: https://bugs.webkit.org/attachment.cgi?id=190223&action=review

> Source/WebCore/inspector/front-end/ObjectPropertiesSection.js:106
> +        if (this.pane) {

We usually omit the braces for single line statements in an if block. Here and below all the occurrences should remove the braces for a single line statements.

> Source/WebCore/inspector/front-end/ObjectPropertiesSection.js:243
> +        {

The opening brace is placed after if() {
Comment 12 Sandip Chitale 2013-02-26 15:41:31 PST
Created attachment 190377 [details]
This patch incorporates the formatting feedback.
Comment 13 Sandip Chitale 2013-02-26 15:43:55 PST
Aside from incorporating the formatting feedback this patch adds the following:

- Detect when the type of the property changes
- When a new property is detected also highlights the name. For modified properties only highlights the value.
Comment 14 Sandip Chitale 2013-02-27 01:57:04 PST
Created attachment 190471 [details]
The previous patch broke the Watch Expressions pane. This fixes it.

Had to make changes to WatchExpressionsSidebarPane.js to fix the Watch Expressions window.
Comment 15 Sandip Chitale 2014-12-09 13:33:16 PST
How was this fixed? Did the patch work? When will be available?
Comment 16 Brian Burg 2014-12-09 14:07:13 PST
(In reply to comment #15)
> How was this fixed? Did the patch work? When will be available?

Hi Sandip, we have been closing out old bugs filed against the "Web Inspector (Deprecated)" component. I closed this because there's been no activity in over a year. However it is not "fixed".

If you intend to keep working on it, I'm happy to migrate it to the correct component and reopen it. You will probably need to start over with a new patch, as the posted patches are based on the old inspector UI.
Comment 17 Sandip Chitale 2014-12-09 23:48:17 PST
Hi Brian,

Please migrate the bug. I will submit the patch again.

Thanks,
Sandip
Comment 18 Brian Burg 2014-12-10 13:45:36 PST
Reopening..
Comment 19 Sandip Chitale 2014-12-11 08:10:05 PST
Created attachment 243123 [details]
This is a patch against the latest code

This is a patch against the latest code at: https://chromium.googlesource.com/chromium/blink
Comment 20 Brian Burg 2014-12-11 09:17:37 PST
(In reply to comment #19)
> Created attachment 243123 [details]
> This is a patch against the latest code
> 
> This is a patch against the latest code at:
> https://chromium.googlesource.com/chromium/blink

Blink is a separate codebase with a different inspector UI implementation. Thus, this patch is not reviewable. Please see the above comments.
Comment 21 Sandip Chitale 2014-12-11 09:28:47 PST
Hi Brian,

What is the code base against which I should create the patch. I mean the git or SVN repository?

Thanks,
Sandip
Comment 22 Joseph Pecoraro 2014-12-11 12:03:51 PST
(In reply to comment #21)
> What is the code base against which I should create the patch. I mean the
> git or SVN repository?

Report bugs and attach patches to the Chromium bug tracker:
https://code.google.com/p/chromium/issues/entry
Comment 23 Sandip Chitale 2014-12-11 15:19:16 PST
Done.

https://code.google.com/p/chromium/issues/detail?id=441374