688600 - Remove the iQ dependencies from the Remote Debugger

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Victor Porof [:vporof][:vp]]

Attachment: 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]]

Attachment: The necessary DebuggerView object used in debugger.js

Comment 2

[Victor Porof [:vporof][:vp]]

Attachment: iQ removal in debugger.js and updated debugger-view.js file

Comment 3

[Dave Camp (:dcamp)]

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)]

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]]

(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]]

Attachment: iQ removal (keeping the "Load more frames" button)

Comment 7

[Victor Porof [:vporof][:vp]]

Attachment: iQ removal (replacing the "Load more frames" button with scroll and resize events)

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

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]]

(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]]

Attachment: iQ removal (replacing the "Load more frames" button with scroll and resize events) + fixed tests

Comment 11

[Dave Camp (:dcamp)]

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)]

(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)]

Attachment: 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 (he/him) [:past] (please ni?)]

(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 (he/him) [:past] (please ni?)]

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]]

(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)]

Attachment: small fixes

I guess I can qrefresh if you guys want to be sticklers about it.

Comment 18

[Victor Porof [:vporof][:vp]]

Attachment: iQ removal + small fixes

Comment 19

[Dave Camp (:dcamp)]

Comment on attachment 563766 [details] [diff] [review]
iQ removal + small fixes

Will land today.

Comment 20

[Dave Camp (:dcamp)]

http://hg.mozilla.org/users/dcamp_campd.org/remote-debug/rev/95536ee07897