Last Comment Bug 724229 - Briefly flash the variables that changed between pauses
: Briefly flash the variables that changed between pauses
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: Trunk
: All All
: P3 enhancement (vote)
: Firefox 15
Assigned To: Victor Porof [:vporof][:vp]
:
Mentors:
Depends on: 723062 758208
Blocks: minotaur 752853 753651
  Show dependency treegraph
 
Reported: 2012-02-04 03:30 PST by Panos Astithas [:past]
Modified: 2012-05-26 10:21 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (6.34 KB, patch)
2012-03-15 06:27 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Review
v2 (9.33 KB, patch)
2012-04-28 13:09 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Review
v3 (11.07 KB, patch)
2012-05-04 02:29 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Review
v3.1 (15.62 KB, patch)
2012-05-08 03:51 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Review
v3.2 (15.63 KB, patch)
2012-05-08 05:00 PDT, Victor Porof [:vporof][:vp]
rcampbell: review+
Details | Diff | Review
v3.3 (15.58 KB, patch)
2012-05-09 09:07 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Review

Description Panos Astithas [:past] 2012-02-04 03:30:04 PST
It would be nice to be able to see at a glance which variables changed between two pauses (two consecutive breakpoints or after stepping). My current favorite idea is to add a red background to variables that are no longer in scope, a green background to newly-added variables and a yellow background to modified ones (value or descriptor attributes). Keeping these decorations for a couple of seconds seems reasonable, I think. After this timeout the red variables are removed and the rest are set to the normal background again.

This would help the user quickly pinpoint the stuff that changed, which would often be what he wants to inspect.
Comment 1 Victor Porof [:vporof][:vp] 2012-02-07 01:24:51 PST
I can take this.
Comment 2 Victor Porof [:vporof][:vp] 2012-03-15 06:27:53 PDT
Created attachment 606185 [details] [diff] [review]
v1

Totally work in progress.
Comment 3 Victor Porof [:vporof][:vp] 2012-04-28 13:09:42 PDT
Created attachment 619333 [details] [diff] [review]
v2

This actually works. Needs CSS in the right files etc.
Comment 4 Victor Porof [:vporof][:vp] 2012-05-04 02:29:19 PDT
Created attachment 620987 [details] [diff] [review]
v3
Comment 5 Victor Porof [:vporof][:vp] 2012-05-08 03:51:29 PDT
Created attachment 621929 [details] [diff] [review]
v3.1

Added a way of avoiding redraw flicker after stepping (aka "don't force the UI to always be emptied on framescleared and rebuilt on framesadded).
Comment 6 Victor Porof [:vporof][:vp] 2012-05-08 05:00:10 PDT
Created attachment 621938 [details] [diff] [review]
v3.2

Rebased.
Comment 7 Rob Campbell [:rc] (:robcee) 2012-05-09 03:15:36 PDT
Comment on attachment 621938 [details] [diff] [review]
v3.2

+const FRAME_STEP_CACHE_DURATION = 100; // ms

   _onFramesCleared: function SF__onFramesCleared() {
-    DebuggerView.StackFrames.emptyText();
+    // After each frame step (in, over, out), framescleared is fired, which
+    // forces the UI to be emptied and rebuilt on framesadded. Most of the times
+    // this is not necessary, and will result in a brief redraw flicker.
+    // To avoid it, invalidate the UI only after a short time if necessary.
+    window.setTimeout(this._afterFramesCleared, FRAME_STEP_CACHE_DURATION);

100ms seems high (also arbitrary).

and setTimeout? Really? You're killin' me.

debugger-view.js

+   * Remember a simple hierarchy of parent->element->children.
+   */
+  _saveHierarchy: function DVP__saveHierarchy(aProperties) {

@param aProperties?

+  createHierarchyStore: function DVP_createHierarchyStore() {
+    this._prevHierarchy = this._currHierarchy;
+    this._currHierarchy = {};
+
+    this._saveHierarchy({ element: this._globalScope, store: this._currHierarchy });
+    this._saveHierarchy({ element: this._localScope, store: this._currHierarchy });
+    this._saveHierarchy({ element: this._withScope, store: this._currHierarchy });
+    this._saveHierarchy({ element: this._closureScope, store: this._currHierarchy });

That's a lot of _saveHierarchy.

+  commitHierarchy: function DVS_commitHierarchy() {
+    for (let i in this._currHierarchy) {
+      let currScope = this._currHierarchy[i];
+      let prevScope = this._prevHierarchy[i];
+
+      for (let v in currScope.children) {
+        let currVar = currScope.children[v];
+        let prevVar = prevScope.children[v];

you could use for each ... in loops instead of for in to access curr/prevScope and curr/prevVar directly.

+          window.setTimeout(function() {
+           currVar.element.removeAttribute(action);
+         }, PROPERTY_VIEW_FLASH_DURATION);

Now that's how you use a setTimeout!

+
+.variable[added] {
+  -moz-transition-duration: 0.5s;
+  background: rgba(0, 255, 0, 0.15);
 }

pink!

do we want to add a transition function? ease-out, to add a bit of a tail to the fade?
Comment 8 Victor Porof [:vporof][:vp] 2012-05-09 06:02:38 PDT
(In reply to Rob Campbell [:rc] (:robcee) from comment #7)
> Comment on attachment 621938 [details] [diff] [review]
> v3.2
> 
> +const FRAME_STEP_CACHE_DURATION = 100; // ms
> 
>    _onFramesCleared: function SF__onFramesCleared() {
> -    DebuggerView.StackFrames.emptyText();
> +    // After each frame step (in, over, out), framescleared is fired, which
> +    // forces the UI to be emptied and rebuilt on framesadded. Most of the
> times
> +    // this is not necessary, and will result in a brief redraw flicker.
> +    // To avoid it, invalidate the UI only after a short time if necessary.
> +    window.setTimeout(this._afterFramesCleared, FRAME_STEP_CACHE_DURATION);
> 
> 100ms seems high (also arbitrary).
> 
> and setTimeout? Really? You're killin' me.
> 

I tried for loops but they didn't seem to work..

> debugger-view.js
> 
> +   * Remember a simple hierarchy of parent->element->children.
> +   */
> +  _saveHierarchy: function DVP__saveHierarchy(aProperties) {
> 
> @param aProperties?
> 
> +  createHierarchyStore: function DVP_createHierarchyStore() {
> +    this._prevHierarchy = this._currHierarchy;
> +    this._currHierarchy = {};
> +
> +    this._saveHierarchy({ element: this._globalScope, store:
> this._currHierarchy });
> +    this._saveHierarchy({ element: this._localScope, store:
> this._currHierarchy });
> +    this._saveHierarchy({ element: this._withScope, store:
> this._currHierarchy });
> +    this._saveHierarchy({ element: this._closureScope, store:
> this._currHierarchy });
> 
> That's a lot of _saveHierarchy.
> 

I think we need a followup to remove withScope and closureScope, since we don't use them anywhere.

> +  commitHierarchy: function DVS_commitHierarchy() {
> +    for (let i in this._currHierarchy) {
> +      let currScope = this._currHierarchy[i];
> +      let prevScope = this._prevHierarchy[i];
> +
> +      for (let v in currScope.children) {
> +        let currVar = currScope.children[v];
> +        let prevVar = prevScope.children[v];
> 
> you could use for each ... in loops instead of for in to access
> curr/prevScope and curr/prevVar directly.
> 

I need both values at the same time. They're referenced by the same key. How would for each work better here?

> +          window.setTimeout(function() {
> +           currVar.element.removeAttribute(action);
> +         }, PROPERTY_VIEW_FLASH_DURATION);
> 
> Now that's how you use a setTimeout!
> 

I knew it!

> +
> +.variable[added] {
> +  -moz-transition-duration: 0.5s;
> +  background: rgba(0, 255, 0, 0.15);
>  }
> 
> pink!
> 

I will NOT fall for that!

> do we want to add a transition function? ease-out, to add a bit of a tail to
> the fade?

Ok.
Comment 9 Rob Campbell [:rc] (:robcee) 2012-05-09 06:55:51 PDT
(In reply to Victor Porof from comment #8)
> (In reply to Rob Campbell [:rc] (:robcee) from comment #7)
> > Comment on attachment 621938 [details] [diff] [review]
> > v3.2
> > 
> > +const FRAME_STEP_CACHE_DURATION = 100; // ms
> > 
> >    _onFramesCleared: function SF__onFramesCleared() {
> > -    DebuggerView.StackFrames.emptyText();
> > +    // After each frame step (in, over, out), framescleared is fired, which
> > +    // forces the UI to be emptied and rebuilt on framesadded. Most of the
> > times
> > +    // this is not necessary, and will result in a brief redraw flicker.
> > +    // To avoid it, invalidate the UI only after a short time if necessary.
> > +    window.setTimeout(this._afterFramesCleared, FRAME_STEP_CACHE_DURATION);
> > 
> > 100ms seems high (also arbitrary).
> > 
> > and setTimeout? Really? You're killin' me.
> 
> I tried for loops but they didn't seem to work..

darn.

> > That's a lot of _saveHierarchy.
> 
> I think we need a followup to remove withScope and closureScope, since we
> don't use them anywhere.

Are they intended for future versions? I could see a world where we explicitly show closure scope, though with scope will likely just go away over time. I hope.

> > +  commitHierarchy: function DVS_commitHierarchy() {
> > +    for (let i in this._currHierarchy) {
> > +      let currScope = this._currHierarchy[i];
> > +      let prevScope = this._prevHierarchy[i];
> > +
> > +      for (let v in currScope.children) {
> > +        let currVar = currScope.children[v];
> > +        let prevVar = prevScope.children[v];
> > 
> > you could use for each ... in loops instead of for in to access
> > curr/prevScope and curr/prevVar directly.
> > 
> 
> I need both values at the same time. They're referenced by the same key. How
> would for each work better here?

oh of course. Silly me.

> > pink!
> 
> I will NOT fall for that!

are you sure?
Comment 10 Victor Porof [:vporof][:vp] 2012-05-09 09:07:55 PDT
Created attachment 622388 [details] [diff] [review]
v3.3

Addressed comments.

(In reply to Rob Campbell [:rc] (:robcee) from comment #9)
> > > pink!
> > 
> > I will NOT fall for that!
> 
> are you sure?

Green is the colour.
Comment 11 Rob Campbell [:rc] (:robcee) 2012-05-10 08:04:00 PDT
(In reply to Victor Porof from comment #10)
> Created attachment 622388 [details] [diff] [review]
> > > > pink!
> > > 
> > > I will NOT fall for that!
> > 
> > are you sure?
> 
> Green is the colour.

ok.
Comment 12 Panos Astithas [:past] 2012-05-24 06:09:59 PDT
https://hg.mozilla.org/integration/fx-team/rev/e7a2ef8a6c35
Comment 13 Rob Campbell [:rc] (:robcee) 2012-05-26 10:21:32 PDT
https://hg.mozilla.org/mozilla-central/rev/e7a2ef8a6c35

Note You need to log in before you can comment on or make changes to this bug.