Last Comment Bug 688600 - Remove the iQ dependencies from the Remote Debugger
: Remove the iQ dependencies from the Remote Debugger
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: x86 All
: -- normal (vote)
: ---
Assigned To: Victor Porof [:vporof][:vp]
:
: J. Ryan Stinnett [:jryans] (use ni?)
Mentors:
Depends on: 690615
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-22 13:41 PDT by Victor Porof [:vporof][:vp]
Modified: 2011-09-30 11:34 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
iQ-removal-688600-0 (18.18 KB, patch)
2011-09-22 13:41 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
The necessary DebuggerView object used in debugger.js (10.31 KB, text/plain)
2011-09-22 13:45 PDT, Victor Porof [:vporof][:vp]
no flags Details
iQ removal in debugger.js and updated debugger-view.js file (28.74 KB, patch)
2011-09-23 09:42 PDT, Victor Porof [:vporof][:vp]
dcamp: review-
Details | Diff | Splinter Review
iQ removal (keeping the "Load more frames" button) (30.31 KB, patch)
2011-09-26 04:20 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
iQ removal (replacing the "Load more frames" button with scroll and resize events) (30.27 KB, patch)
2011-09-26 04:21 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
iQ removal (replacing the "Load more frames" button with scroll and resize events) + fixed tests (38.45 KB, patch)
2011-09-29 02:08 PDT, Victor Porof [:vporof][:vp]
dcamp: review-
Details | Diff | Splinter Review
small fixes (71 bytes, patch)
2011-09-29 18:15 PDT, Dave Camp (:dcamp)
no flags Details | Diff | Splinter Review
small fixes (2.18 KB, patch)
2011-09-30 07:27 PDT, Dave Camp (:dcamp)
no flags Details | Diff | Splinter Review
iQ removal + small fixes (38.52 KB, patch)
2011-09-30 10:00 PDT, Victor Porof [:vporof][:vp]
dcamp: review+
Details | Diff | Splinter Review

Description Victor Porof [:vporof][:vp] 2011-09-22 13:41:40 PDT
Created attachment 561867 [details] [diff] [review]
iQ-removal-688600-0

The browser/devtools/debugger/content/debugger.js implementation uses unnecessary iQ dependencies which can be easily translated to simple dom manipulation.
Comment 1 Victor Porof [:vporof][:vp] 2011-09-22 13:45:28 PDT
Created attachment 561870 [details]
The necessary DebuggerView object used in debugger.js
Comment 2 Victor Porof [:vporof][:vp] 2011-09-23 09:42:55 PDT
Created attachment 562068 [details] [diff] [review]
iQ removal in debugger.js and updated debugger-view.js file
Comment 3 Dave Camp (:dcamp) 2011-09-23 10:05:01 PDT
Comment on attachment 562068 [details] [diff] [review]
iQ removal in debugger.js and updated debugger-view.js file

Review of attachment 562068 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, mostly style nits.

r- for replacing manual style manipulation with classList, but this looks good!

::: browser/devtools/debugger/content/debugger-view.js
@@ +45,5 @@
> +/**
> + * Functions handling the html stackframes UI.
> + */
> +DebuggerView.Frames = {
> +

Might want to call it Stack or StackFrames?  Don't feel particularly strongly about that.

@@ +54,5 @@
> +  setStatus: function(mode) {
> +    var resume = document.getElementById("resume"),
> +      statusRunning = document.getElementById("status-running"),
> +      statusPaused = document.getElementById("status-paused");
> +

Generally prefer 'let' to 'var' where appropriate in functions.

@@ +93,5 @@
> +   * Removes all elements from the stackframes container, and adds a child node
> +   * with a specified text attached.
> +   *
> +   * @param {String} text: the text to be added to the emptied container
> +   */

Hrm, we don't seem to have a consistent prevailing style for javadoc comments in devtools.

@@ +116,5 @@
> +   * @param {String} frameNameText: the name to be displayed in the list
> +   * @return {Object} the newly created html node representing the added frame
> +   */
> +  addFrame: function(depth, frameIdText, frameNameText) {
> +    // make sure we don't duplicate anything

When adding methods, include a name:

addFrame: function DVF_addFrame(....)

It's ugly, but it means that you get something better than (anonymous) in stack traces.

@@ +148,5 @@
> +  /**
> +   * Selects a frame from the stackframe container.
> +   * @param {Number} depth: the frame depth specified by the debugger
> +   */
> +  setFrameSelected: function(depth) {

I don't like it, but we inherit the browser style, which is aDepth for parameter names.

@@ +164,5 @@
> +      if (classes[i] === "selected") {
> +        sel = true;
> +      }
> +    }
> +

You can use frame.classList.contains("selected") (and classList.add() later on).

https://developer.mozilla.org/en/DOM/element.classList

@@ +202,5 @@
> +  /**
> +   * The cached onClick listener for the stackframes container.
> +   */
> +  $onFramesClick: null,
> +

As discussed on IRC, _ is the prevailing private prefix.
Comment 4 Dave Camp (:dcamp) 2011-09-23 10:09:35 PDT
Actually, this doesn't yet fix the More Frames button - need to figure out a fix (or plan to fix) that before landing.
Comment 5 Victor Porof [:vporof][:vp] 2011-09-23 10:42:13 PDT
(In reply to Dave Camp (:dcamp) from comment #4)
> Actually, this doesn't yet fix the More Frames button - need to figure out a
> fix (or plan to fix) that before landing.

Indeed. Like we talked on the IRC, I need a solid test case for this.
Comment 6 Victor Porof [:vporof][:vp] 2011-09-26 04:20:03 PDT
Created attachment 562396 [details] [diff] [review]
iQ removal (keeping the "Load more frames" button)
Comment 7 Victor Porof [:vporof][:vp] 2011-09-26 04:21:30 PDT
Created attachment 562398 [details] [diff] [review]
iQ removal (replacing the "Load more frames" button with scroll and resize events)
Comment 8 Panos Astithas [:past] 2011-09-28 00:17:58 PDT
I haven't tried applying the patches, but from skimming the diffs I made a few observations that you may find useful.

From an aesthetics POV you might want to replace all the setFoo methods with proper setters. The new syntax for getters and setters is pervasive in the Firefox codebase. Also, setScriptsEmpty would be better called emptyScripts.

setFrameSelected and setFrameDeselected could use a rename, but more importantly they have two times the size of the succinct selectFrame, with no apparent reason.

On a similar note setClickListener should probably be addClickListener, (the same for setChangeListener) although it seems redundant. If the only reason you need this method is for getting a reference to StackFrames.onClick, why not move that into DebuggerView.Stackframes? Actually, why not merge these two objects altogether? Many methods from one just invoke their siblings in the latter. The same for SourceScripts and DebuggerView.Scripts. Is there a separation of responsibilities between them that I can't see?
Comment 9 Victor Porof [:vporof][:vp] 2011-09-29 02:06:16 PDT
(In reply to Panos Astithas [:past] from comment #8)
> From an aesthetics POV you might want to replace all the setFoo methods with
> proper setters. The new syntax for getters and setters is pervasive in the
> Firefox codebase.
I made a habit of using getters and setters only when accessing or modifying (usually private) members of an object, thus in a case where a setFoo function alters only the state of the view and does nothing to the members of the object literal, a setter may (arguably) create a certain degree of confusion (do I have a getter? is the state saved in a variable?). Most functions in the DebuggerView.Stackframes and DebuggerView.Scripts don't modify object members, but alter the underlying view.

> Also, setScriptsEmpty would be better called emptyScripts.
I agree. I admit that naming functions with a 'set' prefix can be misleading. I will do some proper renaming in the new patch.

> setFrameSelected and setFrameDeselected could use a rename, but more
> importantly they have two times the size of the succinct selectFrame, with
> no apparent reason.
The selectFrame function in debugger.js and setFrameSelected/Deselected functions in DebuggerView do two separate things. Again, I agree, the way I named them is prone to confusion. Functions in the DebuggerView object have a 'presenter' role, mediating visual changes between the debugger and the view. Therefore, while selectFrame sets the selected frame depth in the StackFrames object, setFrameSelected/Deselected in DebuggerView modifies the visual aspect of the corresponding list item. Fwiw, I combined and renamed some of them now.

> On a similar note setClickListener should probably be addClickListener, (the
> same for setChangeListener) although it seems redundant. If the only reason
> you need this method is for getting a reference to StackFrames.onClick, why
> not move that into DebuggerView.Stackframes? Actually, why not merge these
> two objects altogether? Many methods from one just invoke their siblings in
> the latter. The same for SourceScripts and DebuggerView.Scripts. Is there a
> separation of responsibilities between them that I can't see?
I agree on renaming, and yes: overly abstracting scenarios is clearly very bad idea. But, as stated in the previous note, the two objects handle two different responsibilities, debugger.js is the controller, and debugger-view.js is the presenter, and this was the whole purpose of removing iQ dependencies in the first place.
Also, the UI will likely change soon, and it makes more sense to modify only UI responsible code because of this, instead of changing code all over the place. This also makes the StackFrames and SourceScripts objects cleaner and easier to read, and modifying/enhancing the Debugger view easier in the feature.

I'll attach now a new patch with the necessary changes, and also fixed tests (without using iQ and replacing the "Add more frames" button with scroll functionality)
Comment 10 Victor Porof [:vporof][:vp] 2011-09-29 02:08:07 PDT
Created attachment 563342 [details] [diff] [review]
iQ removal (replacing the "Load more frames" button with scroll and resize events) + fixed tests
Comment 11 Dave Camp (:dcamp) 2011-09-29 18:08:59 PDT
Comment on attachment 563342 [details] [diff] [review]
iQ removal (replacing the "Load more frames" button with scroll and resize events) + fixed tests

Review of attachment 563342 [details] [diff] [review]:
-----------------------------------------------------------------

r- for the setTimeout, but I'm going to attach a fix for that (that depends on a patch in another bug).

If you want to stick with setStatus (see below), I can land this patch with my upcoming patch folded in.  Otherwise, go ahead and attach a new one with my patch (or a fix you prefer) folded in.

::: browser/devtools/debugger/content/debugger-view.js
@@ +52,5 @@
> +   *
> +   * @param {String} aMode
> +   *                 either "paused" or "attached"
> +   */
> +  setStatus: function DVF_setStatus(aMode) {

In our debugger code the "paused" or "attached" are generally referred to as "state" rather than "status", any reason not to use that terminology here?  And to avoid the setter/getter confusion, maybe this should be updateState() rather than setState().

::: browser/devtools/debugger/test/browser/browser_dbg_stack-03.js
@@ -50,7 +34,8 @@
> > -        gPane.activeThread.addOneTimeListener("framesadded", function () {
> > +      is(frames.querySelectorAll(".dbg-stackframe").length, pageSize,
> > -          // Now ask one more time, should get the remainder.
> > +        "Should have the max limit of frames.");
NaN more ...
> > +      is(childNodes.length, frames.querySelectorAll(".dbg-stackframe").length,
> > -          is(frames.find("#stack-more").length, 0,
> > +        "All children should be frames.");
> > -             "More button should be gone.");
NaN more ...

This timeout is hiding a pretty nasty bug.

The stack frame viewer is requesting new frames whenever the stack viewer scrolls, even if the debugger isn't currently paused.  In particular, after our initial attach/resume we're getting a request for frames from that scroll handler.

This should have been easier to catch, because the client object should be asserting that we don't make requests while the debugger is paused.  I filed bug 690615 with a patch waiting on review.
Comment 12 Dave Camp (:dcamp) 2011-09-29 18:10:31 PDT
(In reply to Dave Camp (:dcamp) from comment #11)

> ::: browser/devtools/debugger/test/browser/browser_dbg_stack-03.js
> @@ -50,7 +34,8 @@
> > > -        gPane.activeThread.addOneTimeListener("framesadded", function () {
> > > +      is(frames.querySelectorAll(".dbg-stackframe").length, pageSize,
> > > -          // Now ask one more time, should get the remainder.
> > > +        "Should have the max limit of frames.");
> NaN more ...
> > > +      is(childNodes.length, frames.querySelectorAll(".dbg-stackframe").length,
> > > -          is(frames.find("#stack-more").length, 0,
> > > +        "All children should be frames.");
> > > -             "More button should be gone.");
> NaN more ...

Splinter ate that review up, ignore this NaN stuff and pretend it quoted the settimeout in test -03.
Comment 13 Dave Camp (:dcamp) 2011-09-29 18:15:10 PDT
Created attachment 563620 [details] [diff] [review]
small fixes

This prevents the frame viewer from asking for new frames while the debugger isn't paused, and removes the setTimeout from the test.  Depends on the patch in bug 690615.

I suppose you could also play some shenanigans with _dirty as you pause and resume the thread.  Up to you how you want to prevent that, but it needs to be prevented :)
Comment 14 Panos Astithas [:past] 2011-09-30 06:30:48 PDT
(In reply to Dave Camp (:dcamp) from comment #13)
> Created attachment 563620 [details] [diff] [review] [diff] [details] [review]
> small fixes

This comes up empty for me (just two patch metadata lines at the top).
Comment 15 Panos Astithas [:past] 2011-09-30 06:52:55 PDT
Hey Victor, you might want to check this out for various code style nits people are usually complaining about during reviews:

https://developer.mozilla.org/En/Developer_Guide/Coding_Style
Comment 16 Victor Porof [:vporof][:vp] 2011-09-30 07:22:15 PDT
(In reply to Panos Astithas [:past] from comment #14)
> (In reply to Dave Camp (:dcamp) from comment #13)
> > Created attachment 563620 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > small fixes
> 
> This comes up empty for me (just two patch metadata lines at the top).

Yes, the diff is empty.
Comment 17 Dave Camp (:dcamp) 2011-09-30 07:27:05 PDT
Created attachment 563736 [details] [diff] [review]
small fixes

I guess I can qrefresh if you guys want to be sticklers about it.
Comment 18 Victor Porof [:vporof][:vp] 2011-09-30 10:00:59 PDT
Created attachment 563766 [details] [diff] [review]
iQ removal + small fixes
Comment 19 Dave Camp (:dcamp) 2011-09-30 11:09:37 PDT
Comment on attachment 563766 [details] [diff] [review]
iQ removal + small fixes

Will land today.

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