Closed Bug 755346 Opened 12 years ago Closed 12 years ago

Global variables are not displayed in the debugger frontend

Categories

(DevTools :: Debugger, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 15

People

(Reporter: past, Assigned: past)

References

Details

Attachments

(3 files, 10 obsolete files)

6.10 KB, patch
past
: review+
Details | Diff | Splinter Review
59.52 KB, patch
Details | Diff | Splinter Review
879 bytes, patch
vporof
: review+
Details | Diff | Splinter Review
Even though the Debugger API supports it, we don't seem to show the global variables in the UI.
Attached patch Patch v1 (obsolete) — Splinter Review
We were missing the code to populate the global variables pane, and a couple of fixes to the handling of parent environments, mandated by the recent refactoring. This patch needs tests and to figure out what to display where, when the current frame is in the global environment.
Attached patch Patch v2 (obsolete) — Splinter Review
This is now ready for review.
Attachment #624079 - Attachment is obsolete: true
Attachment #624390 - Flags: review?(rcampbell)
Comment on attachment 624390 [details] [diff] [review]
Patch v2

Actually, scratch that. I now realize that I had the protocol spec for global environments wrong, even though the user experience was correct. I'll fix that and I'll throw in the fix for |with| scope as a bonus.
Attachment #624390 - Flags: review?(rcampbell)
Attached patch Patch v3 (obsolete) — Splinter Review
Working global scope and |with| scope, fixed protocol implementation. I still need to add tests for the |with| scope and make sure corner cases are handled correctly (e.g. |with| in global scope, or |with| inside |with|).
Attachment #624390 - Attachment is obsolete: true
Attached patch Patch v4 (obsolete) — Splinter Review
Added many more tests.
Attachment #624501 - Attachment is obsolete: true
Attached patch Patch v5 (obsolete) — Splinter Review
Some minor actor changes after bug 755778.
Attachment #624685 - Attachment is obsolete: true
Attached patch Patch v6 (obsolete) — Splinter Review
After changing my mind a couple of times already, I'm now certain that this is what we should be doing in the variables view. There is no point to having a static set of scopes and populating them. This may be better than what Firebug does, but is far less than what WebKit does. Frames can have any number of nested scopes and we must present them all, if we are to do a meaningful job. This expands the scope of the original patch, but I can't believe we were restricting the display of scope information, essentially throwing away a lot of what the debugger server was sending our way. This was all my fault, alas.

I also made sure that the environment bindings is now in accordance to the spec for all types of environments and added new tests. This display now allows us to do more advanced stuff, like detect overriden variables and display them with a strike-through font, like the rule view. Victor, you should take a thorough look into my DebuggerView changes, to see if I missed anything.
Attachment #624721 - Attachment is obsolete: true
Attachment #624825 - Flags: review?(vporof)
Attachment #624825 - Flags: review?(rcampbell)
Attached patch Patch v7 (obsolete) — Splinter Review
Moar tests! Coincidentally, these uncovered some obscure bugs that I had to fix as well. I promise, this is the last version of the patch. Scout's honor.
Attachment #624825 - Attachment is obsolete: true
Attachment #624825 - Flags: review?(vporof)
Attachment #624825 - Flags: review?(rcampbell)
Attachment #625086 - Flags: review?(vporof)
Attachment #625086 - Flags: review?(rcampbell)
Attached patch v6-v7 diff (obsolete) — Splinter Review
If you were already half way through the previous patch, you can use this diff to see what has changed.
Attached patch Patch v8 (obsolete) — Splinter Review
Rebased on top of bug 752770 and simplified accordingly. If you are already midway through reviewing v7, just ignore this.
firstly, has this been through try yet?
secondly, I'm getting the weird startup and shutdown bug every time I try to launch the debugger with this patch.

This is on the Error Console:

Timestamp: 2012-05-24 15:56:17
Error: [Exception... "Component returned failure code: 0x80470002 (NS_BASE_STREAM_CLOSED) [nsIOutputStream.write]"  nsresult: "0x80470002 (NS_BASE_STREAM_CLOSED)"  location: "JS frame :: chrome://global/content/devtools/dbg-transport.js :: DT_onOutputStreamReady :: line 94"  data: no]
Source File: chrome://global/content/devtools/dbg-transport.js
Line: 94

I'll try activating it with debugdump enabled.
attempted with debug logging and it worked fine on first open. However, on shutdown, I received this:

2012-05-24 15:58:05.631 firefox[29177:507] *** Assertion failure in -[mozRootAccessible didReceiveFocus], /Volumes/Data/FXTEAM/fx-team-working/accessible/src/mac/mozAccessible.mm:584
2012-05-24 15:58:05.632 firefox[29177:507] Mozilla has caught an Obj-C exception [NSInternalInconsistencyException: trying to set focus to ignored element! ((0x150bb31c0) AXUnknown)]
DBG-SERVER: Cleaning up connection.
DBG-SERVER: Cleaning up connection.
DBG-SERVER: Cleaning up connection.

the three Cleaning up connection messages are from three separate attempts to launch the debugger.
Attached patch Patch v9 (obsolete) — Splinter Review
Sorry, the recent landings seriously bitrotted this patch. I've updated it and now everything works as expected. Unfortunately this patch regresses bug 724229 (brief variable flashing) and it doesn't look like it's a trivial amount of work for me to fix that. Victor, any help would be appreciated, but I think that a different strategy may be required, now that there are no static scopes between pauses.

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=5e9c023ee947
Attachment #625086 - Attachment is obsolete: true
Attachment #625088 - Attachment is obsolete: true
Attachment #625630 - Attachment is obsolete: true
Attachment #625086 - Flags: review?(vporof)
Attachment #625086 - Flags: review?(rcampbell)
Attachment #627133 - Flags: review?(rcampbell)
(In reply to Panos Astithas [:past] from comment #16)
> Created attachment 627133 [details] [diff] [review]
> Patch v9
> 
> Sorry, the recent landings seriously bitrotted this patch. I've updated it
> and now everything works as expected. Unfortunately this patch regresses bug
> 724229 (brief variable flashing) and it doesn't look like it's a trivial
> amount of work for me to fix that. Victor, any help would be appreciated,
> but I think that a different strategy may be required, now that there are no
> static scopes between pauses.
> 
> Try run:
> https://tbpl.mozilla.org/?tree=Try&rev=5e9c023ee947

I'll take a look.
(In reply to Panos Astithas [:past] from comment #16)
> Created attachment 627133 [details] [diff] [review]
> 

+            objClient.getPrototypeAndProperties(function SF_getProps(aResponse) {
+              this._addScopeVariables(aResponse.ownProperties, scope);
+              // Signal that variables have been fetched.
+              DebuggerController.dispatchEvent("Debugger:FetchedVariables");
+            }.bind(this));

This makes me a bit worried because there's another signal at the end of the function. Therefore, we get multiple FetchedVariables events dispatched, but somebody has no idea what scope they refer to, or if they actually mean "I got all the variables from all the scopes, you can start testing now".

-   * Removes all elements from the stackframes container, and adds a child node
+   * Removes all elements from the variables container, and adds a child node

I don't think you want to change that there.
The comments are the other way around: stackframes for DVF_emptyText and variables for DVP_emptyText.

-  this._addScope = this._addScope.bind(this);
+  this.addScope = this._addScope.bind(this);
+  this.clearScopes = this._clearScopes.bind(this);

No need to bind clearScopes to _clearScopes. Just make it directly public in the prototype, since it's always called from an instance.
I'd also like it renamed to empty, to preserve the same interface with StackframesView, since most of their functions have similar naming.

+  /**
+   * Removes all elements from the stackframes container, and adds a child node
+   * with an empty text note attached.
+   */
+  emptyText: function DVP_emptyText() {

Same thing.
s/stackframes/properties

+    // Generate a unique id for the element, if not specified.
+    aId = aId || aName.toLowerCase().trim().replace(" ", "-") + this._idCount++;
 
Should now be
aId = aId || aName.toLowerCase().trim().replace(/\s+/g, "-") + this._idCount++;
since we can have more than one space and the naming can be more volatile.
I'll take care of this in the followup.

+        // Construct the scope name.
+        let name = env.type.substring(0, 1).toUpperCase() +
+                   env.type.substring(1);

How about
let name = env.type.charAt(0).toUpperCase() + env.type.slice(1);
Fits on one line :)

-    this._localScope = this._addScope(L10N.getStr("localScope")).expand();
-    this._withScope = this._addScope(L10N.getStr("withScope")).hide();
-    this._closureScope = this._addScope(L10N.getStr("closureScope")).hide();
-    this._globalScope = this._addScope(L10N.getStr("globalScope"));

Should've also removed
this._saveHierarchy({ element: this._localScope, store: this._currHierarchy });
and friends from DVP_createHierarchyStore, but I'll take care of that in the followup patch.


+++ b/browser/devtools/debugger/test/browser_dbg_propertyview-01.js

This test does absolutely nothing now :) Maybe remove it and rename the others? Or add something in it, I don't know.
Attached patch v9, part2Splinter Review
Attachment #627307 - Flags: review?(past)
Comment on attachment 627307 [details] [diff] [review]
v9, part2

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

Perfect, thanks! I had to resort myself to nitpicking.

::: browser/devtools/debugger/debugger-view.js
@@ +811,5 @@
>  
> +    /**
> +     * @see DebuggerView.Properties.addScopeToHierarchy
> +     */
> +    element.addToHierarchy = this.addScopeToHierarchy.bind(this, element);

I'm not used to seeing this type of comment not preceding function declarations.

@@ +1684,3 @@
>  
> +  /**
> +   * Creates a designated hierarchy holder for a scope.

s/designated// (too wordy?)
Attachment #627307 - Flags: review?(past) → review+
(In reply to Victor Porof from comment #18)
> (In reply to Panos Astithas [:past] from comment #16)
> > Created attachment 627133 [details] [diff] [review]
> > 
> 
> +            objClient.getPrototypeAndProperties(function
> SF_getProps(aResponse) {
> +              this._addScopeVariables(aResponse.ownProperties, scope);
> +              // Signal that variables have been fetched.
> +              DebuggerController.dispatchEvent("Debugger:FetchedVariables");
> +            }.bind(this));
> 
> This makes me a bit worried because there's another signal at the end of the
> function. Therefore, we get multiple FetchedVariables events dispatched, but
> somebody has no idea what scope they refer to, or if they actually mean "I
> got all the variables from all the scopes, you can start testing now".

Since this event was for use by tests only, I opted for triggering one after every protocol response was received, and let the test make sure it got all it needed. I see now that you are counting on it in SF_onFetchedVars, which I hadn't anticipated. It seems to me that making sure FetchedVariables is fired only once can be done in a followup, without serious problems. What do you think?

> -   * Removes all elements from the stackframes container, and adds a child
> node
> +   * Removes all elements from the variables container, and adds a child
> node
> 
> I don't think you want to change that there.
> The comments are the other way around: stackframes for DVF_emptyText and
> variables for DVP_emptyText.

Oops!

> -  this._addScope = this._addScope.bind(this);
> +  this.addScope = this._addScope.bind(this);
> +  this.clearScopes = this._clearScopes.bind(this);
> 
> No need to bind clearScopes to _clearScopes. Just make it directly public in
> the prototype, since it's always called from an instance.
> I'd also like it renamed to empty, to preserve the same interface with
> StackframesView, since most of their functions have similar naming.

OK, done.

> +  /**
> +   * Removes all elements from the stackframes container, and adds a child
> node
> +   * with an empty text note attached.
> +   */
> +  emptyText: function DVP_emptyText() {
> 
> Same thing.
> s/stackframes/properties

Done.

> +    // Generate a unique id for the element, if not specified.
> +    aId = aId || aName.toLowerCase().trim().replace(" ", "-") +
> this._idCount++;
>  
> Should now be
> aId = aId || aName.toLowerCase().trim().replace(/\s+/g, "-") +
> this._idCount++;
> since we can have more than one space and the naming can be more volatile.
> I'll take care of this in the followup.

I don't think there can ever be more than one consecutive spaces, but OK.

> +        // Construct the scope name.
> +        let name = env.type.substring(0, 1).toUpperCase() +
> +                   env.type.substring(1);
> 
> How about
> let name = env.type.charAt(0).toUpperCase() + env.type.slice(1);
> Fits on one line :)

Tasty! Done.

> -    this._localScope = this._addScope(L10N.getStr("localScope")).expand();
> -    this._withScope = this._addScope(L10N.getStr("withScope")).hide();
> -    this._closureScope = this._addScope(L10N.getStr("closureScope")).hide();
> -    this._globalScope = this._addScope(L10N.getStr("globalScope"));
> 
> Should've also removed
> this._saveHierarchy({ element: this._localScope, store: this._currHierarchy
> });
> and friends from DVP_createHierarchyStore, but I'll take care of that in the
> followup patch.
> 
> 
> +++ b/browser/devtools/debugger/test/browser_dbg_propertyview-01.js
> 
> This test does absolutely nothing now :) Maybe remove it and rename the
> others? Or add something in it, I don't know.

It still tests the shortening of script labels, so I figured it did something useful. I renamed the test function to testScriptLabelShortening, in order for that to be more evident.
(In reply to Panos Astithas [:past] from comment #21)
> (In reply to Victor Porof from comment #18)
> > (In reply to Panos Astithas [:past] from comment #16)
> > > Created attachment 627133 [details] [diff] [review]
> > > 
> > 
> > +            objClient.getPrototypeAndProperties(function
> > SF_getProps(aResponse) {
> > +              this._addScopeVariables(aResponse.ownProperties, scope);
> > +              // Signal that variables have been fetched.
> > +              DebuggerController.dispatchEvent("Debugger:FetchedVariables");
> > +            }.bind(this));
> > 
> > This makes me a bit worried because there's another signal at the end of the
> > function. Therefore, we get multiple FetchedVariables events dispatched, but
> > somebody has no idea what scope they refer to, or if they actually mean "I
> > got all the variables from all the scopes, you can start testing now".
> 
> Since this event was for use by tests only, I opted for triggering one after
> every protocol response was received, and let the test make sure it got all
> it needed. I see now that you are counting on it in SF_onFetchedVars, which
> I hadn't anticipated. It seems to me that making sure FetchedVariables is
> fired only once can be done in a followup, without serious problems. What do
> you think?

It doesn't affect me that much for now, because I only flash variables in the single local scope (because all others are collapsed after a frame step). But I'll need this for bug 751836. Followup.

> > +    // Generate a unique id for the element, if not specified.
> > +    aId = aId || aName.toLowerCase().trim().replace(" ", "-") +
> > this._idCount++;
> >  
> > Should now be
> > aId = aId || aName.toLowerCase().trim().replace(/\s+/g, "-") +
> > this._idCount++;
> > since we can have more than one space and the naming can be more volatile.
> > I'll take care of this in the followup.
> 
> I don't think there can ever be more than one consecutive spaces, but OK.
> 

"Local scope [Something]" has two spaces :)

> > +        // Construct the scope name.
> > +        let name = env.type.substring(0, 1).toUpperCase() +
> > +                   env.type.substring(1);
> > 
> > How about
> > let name = env.type.charAt(0).toUpperCase() + env.type.slice(1);
> > Fits on one line :)
> 
> Tasty! Done.
> 

^_^

> > 
> > +++ b/browser/devtools/debugger/test/browser_dbg_propertyview-01.js
> > 
> > This test does absolutely nothing now :) Maybe remove it and rename the
> > others? Or add something in it, I don't know.
> 
> It still tests the shortening of script labels, so I figured it did
> something useful. I renamed the test function to testScriptLabelShortening,
> in order for that to be more evident.

Ah, ok
(In reply to Victor Porof from comment #22)
> (In reply to Panos Astithas [:past] from comment #21)
> > (In reply to Victor Porof from comment #18)
> > > (In reply to Panos Astithas [:past] from comment #16)
> > > > Created attachment 627133 [details] [diff] [review]
> > > > 
> > > 
> > > +            objClient.getPrototypeAndProperties(function
> > > SF_getProps(aResponse) {
> > > +              this._addScopeVariables(aResponse.ownProperties, scope);
> > > +              // Signal that variables have been fetched.
> > > +              DebuggerController.dispatchEvent("Debugger:FetchedVariables");
> > > +            }.bind(this));
> > > 
> > > This makes me a bit worried because there's another signal at the end of the
> > > function. Therefore, we get multiple FetchedVariables events dispatched, but
> > > somebody has no idea what scope they refer to, or if they actually mean "I
> > > got all the variables from all the scopes, you can start testing now".
> > 
> > Since this event was for use by tests only, I opted for triggering one after
> > every protocol response was received, and let the test make sure it got all
> > it needed. I see now that you are counting on it in SF_onFetchedVars, which
> > I hadn't anticipated. It seems to me that making sure FetchedVariables is
> > fired only once can be done in a followup, without serious problems. What do
> > you think?
> 
> It doesn't affect me that much for now, because I only flash variables in
> the single local scope (because all others are collapsed after a frame
> step). But I'll need this for bug 751836. Followup.

Filed bug 759045.

> > > +    // Generate a unique id for the element, if not specified.
> > > +    aId = aId || aName.toLowerCase().trim().replace(" ", "-") +
> > > this._idCount++;
> > >  
> > > Should now be
> > > aId = aId || aName.toLowerCase().trim().replace(/\s+/g, "-") +
> > > this._idCount++;
> > > since we can have more than one space and the naming can be more volatile.
> > > I'll take care of this in the followup.
> > 
> > I don't think there can ever be more than one consecutive spaces, but OK.
> > 
> 
> "Local scope [Something]" has two spaces :)

But not *consecutive* spaces :-)
Comment on attachment 627133 [details] [diff] [review]
Patch v9

send it through the try servers!
Attachment #627133 - Flags: review?(rcampbell) → review+
one thing I have noticed while playtesting this, the Globals section of the property view do not persist their expanded state across pauses.
(In reply to Rob Campbell [:rc] (:robcee) from comment #25)
> one thing I have noticed while playtesting this, the Globals section of the
> property view do not persist their expanded state across pauses.

Right, this is a general issue. We will stop collapsing expanded nodes in bug 751836.
Attached patch Patch v10Splinter Review
The patch in bug 758208 broke one test that I fixed. Victor, your patch breaks a bunch more, can you take a look?
Attachment #627133 - Attachment is obsolete: true
(In reply to Panos Astithas [:past] from comment #27)
> The patch in bug 758208 broke one test that I fixed. Victor, your patch
> breaks a bunch more, can you take a look?

This fixes it for me. Try run:

https://tbpl.mozilla.org/?tree=Try&rev=cc55e4fd061c
Attachment #627780 - Flags: review?(vporof)
(In reply to Panos Astithas [:past] from comment #28)
> Created attachment 627780 [details] [diff] [review]
> Fix for test breakage
> 
> (In reply to Panos Astithas [:past] from comment #27)
> > The patch in bug 758208 broke one test that I fixed. Victor, your patch
> > breaks a bunch more, can you take a look?
> 
> This fixes it for me. Try run:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=cc55e4fd061c

Looks ok.

For speed, instead of "let prevVar = prevScope && prevScope.children[v];", do a "if (!prevScope) continue;" before the inner loop.
Attachment #627780 - Flags: review?(vporof) → review+
(In reply to Victor Porof from comment #29)
> (In reply to Panos Astithas [:past] from comment #28)
> > Created attachment 627780 [details] [diff] [review]
> > Fix for test breakage
> > 
> > (In reply to Panos Astithas [:past] from comment #27)
> > > The patch in bug 758208 broke one test that I fixed. Victor, your patch
> > > breaks a bunch more, can you take a look?
> > 
> > This fixes it for me. Try run:
> > 
> > https://tbpl.mozilla.org/?tree=Try&rev=cc55e4fd061c
> 
> Looks ok.
> 
> For speed, instead of "let prevVar = prevScope && prevScope.children[v];",
> do a "if (!prevScope) continue;" before the inner loop.

OK, I did that, but I forgot to refresh the patch, so it landed separately:

https://hg.mozilla.org/integration/fx-team/rev/d4834de824a9
https://hg.mozilla.org/integration/fx-team/rev/59954c646e78
https://hg.mozilla.org/integration/fx-team/rev/a55eca18fd52
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/d4834de824a9
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 15
# LOCALIZATION NOTE (scopeLabel): The text that is displayed in the variables
# pane as a header for each variable scope (e.g. "Global scope, "With scope",
# etc.).
scopeLabel=%S scope

I'm a bit lost here. What are the possible values of %S here? I find only a string (globalScopeLabel) in the current files.
(In reply to Francesco Lodolo [:flod] from comment #34)
> # LOCALIZATION NOTE (scopeLabel): The text that is displayed in the variables
> # pane as a header for each variable scope (e.g. "Global scope, "With scope",
> # etc.).
> scopeLabel=%S scope
> 
> I'm a bit lost here. What are the possible values of %S here? I find only a
> string (globalScopeLabel) in the current files.

All possible JavaScript scope types, like 'With scope', 'Function scope' or Block scope'.
(In reply to Panos Astithas [:past] from comment #35)
> All possible JavaScript scope types, like 'With scope', 'Function scope' or
> Block scope'.

And these "with", "function", etc. are taken from?
(In reply to Francesco Lodolo [:flod] from comment #36)
> (In reply to Panos Astithas [:past] from comment #35)
> > All possible JavaScript scope types, like 'With scope', 'Function scope' or
> > Block scope'.
> 
> And these "with", "function", etc. are taken from?

They are specified in the remote debugging protocol:
https://wiki.mozilla.org/Remote_Debugging_Protocol#Lexical_Environments
Which means they'll remain untranslated, isn't it?

Thanks a lot for the explanation, I think this is worth adding to the current localization notes ;-)
(In reply to Francesco Lodolo [:flod] from comment #38)
> Which means they'll remain untranslated, isn't it?

That's what I expected, yes. See also the localization note in the top of the file, which mentions that most of these wouldn't make sense if localized, at least in most locales.

> Thanks a lot for the explanation, I think this is worth adding to the
> current localization notes ;-)

OK, I'll make a note to add it in a forthcoming patch that touches this file.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: