808369 - Use the VariablesView in Scratchpad

Status: RESOLVED FIXED

(DevTools Graveyard :: Scratchpad, defect, P3)

Description

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

See bug 794823.

Comment 2

[Brandon Benvie [:benvie]]

Attachment: proof of concept WIP

A very rough first pass at using a VariablesView sidebar for inspecting objects in the Scratchpad. It actually works!

Comment 3

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

w00t!

Comment 4

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

Comment on attachment 736072 [details] [diff] [review]
proof of concept WIP

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

::: browser/devtools/scratchpad/scratchpad.js
@@ +25,5 @@
>  Cu.import("resource:///modules/source-editor.jsm");
>  Cu.import("resource:///modules/devtools/LayoutHelpers.jsm");
>  Cu.import("resource:///modules/devtools/scratchpad-manager.jsm");
> +Cu.import("resource:///modules/devtools/Sidebar.jsm");
> +Cu.import("resource:///modules/devtools/VariablesView.jsm");

Might want to lazy load these. I know this is WIP, but I'm just dumping a thought here :)

Comment 5

[Brandon Benvie [:benvie]]

Indeed, good point! I had thought about it and was remembering what Dave said last week. Something to the effect of don't lazy load stuff that will definitely be used. I think you're right here since it's not a guarantee that the side panel will be opened.

Comment 6

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

(In reply to Brandon Benvie [:bbenvie] from comment #5)

The way I see it is: if lazyloading something speeds up startup, lazyload it. Even if something will surely be used, but after a couple of milliseconds/seconds, it's still a good idea to lazyload.

Comment 7

[Brandon Benvie [:benvie]]

Attachment: WIP2 cleaned up and commented

* Adds comments to ScratchpadSidebar
* Adds scratchpad.css and ceases relying on webconsole.css
* Removes unneeded classes from xul elements
* Various cleanup

Comment 8

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

hg add scratchpad.css, I want to play with it :)

Comment 9

[Brandon Benvie [:benvie]]

Attachment: Add scratchpad.css

Oops forgot to add that.

Comment 10

[Brandon Benvie [:benvie]]

Attachment: WIP3 - handle primitives

Since the VariablesView doesn't handle inspection of primitives, and there's really no useful representation of primitives anyway, I have changed it so primitive values are simply displayed as a comment (same as if you inspected them).

Comment 11

[Brandon Benvie [:benvie]]

Attachment: WIP4 - fix handling of functions

The last patch caused functions to be treated like primitives and printed as a comment instead of inspected.

Comment 12

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

(In reply to Brandon Benvie [:bbenvie] from comment #10)
> Created attachment 737636 [details] [diff] [review]
> WIP3 - handle primitives
> 
> Since the VariablesView doesn't handle inspection of primitives, and there's
> really no useful representation of primitives anyway, I have changed it so
> primitive values are simply displayed as a comment (same as if you inspected
> them).

VariablesView can handle primitives easily, however the rawObject setter isn't smart enough to use them (hence also the setter name). It would be pretty trivial to add another setter (rawValue maybe) (or even add some more brainz into rawObject) to handle such cases.

The rawObject setter was added mostly just as a quick and dirty way of testing out how things look in a variables view. Since most things need to be remoteable, a different approach would be needed later anyway (see https://gist.github.com/victorporof/4609119 for some examples).

Comment 13

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

(In reply to Brandon Benvie [:bbenvie] from comment #11)
> Created attachment 737646 [details] [diff] [review]
> WIP4 - fix handling of functions
> 
> The last patch caused functions to be treated like primitives and printed as
> a comment instead of inspected.

IMHO primitives should also be somehow shown in the variables view, but I don't feel strongly about it.

Comment 14

[Brandon Benvie [:benvie]]

Attachment: WIP5 - add sidebar.show(), sidebar.hide()

Also initialize sidebar._sidebar in the constructor.

Comment 15

[Brandon Benvie [:benvie]]

Attachment: WIP5

Forgot to mark it as a patch.

Comment 16

[Brandon Benvie [:benvie]]

(In reply to Victor Porof [:vp] from comment #13)
> IMHO primitives should also be somehow shown in the variables view, but I
> don't feel strongly about it.

I have no disagreement, just didn't see a convenient way to do this. I may have overlooked it though, I will check. If there isn't a way currently, do you have a suggestion for the best way to add the capability?

Comment 17

[Brandon Benvie [:benvie]]

Attachment: WIP6

* remove `variablesView` accessor since it depends on asynchronous state
* add `visible` property to ScratchpadSidebar to track state

Comment 18

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

(In reply to Brandon Benvie [:bbenvie] from comment #16)
> 
> I have no disagreement, just didn't see a convenient way to do this. I may
> have overlooked it though, I will check. If there isn't a way currently, do
> you have a suggestion for the best way to add the capability?

I wouldn't worry too much about it since you'll have to reimplement everything anyway after bug 825039. At that point you'll be required to use the API I pointed you to (the gist) so handling primitives shouldn't be a problem (as in: you'll need to take care of it just like everything else).

Comment 19

[Brandon Benvie [:benvie]]

Attachment: WIP7

* Changes `sidebar._update` to return a promise (since it will be async in the future)
* Streamline `sidebar.open`

Comment 20

[Brandon Benvie [:benvie]]

Attachment: WIP8

* Fix scratchpad.inspect returning the Deferred instead of the Promise
* Fix the inspect test

Is there a better way to do the inspect test? What I'm doing now is really ugly, but it works.

Comment 21

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

Comment on attachment 738262 [details] [diff] [review]
WIP8

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

::: browser/devtools/scratchpad/test/browser_scratchpad_inspect.js
@@ +31,2 @@
>  
> +    outer: for (let [, scope] of sidebar.variablesView._store) {

You can skip the ._store and go for (let [, scope] of sidebar.variablesView). Same goes with everything else (scopes, variables, properties). Take a look at the __iterator__s defined in VariablesView.jsm. Alternatively, webconsole defines some nice helpers like findVariableViewProperties and matchVariablesViewProperty that you may be able to use.

Comment 22

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

(In reply to Victor Porof [:vp] from comment #21)
> You can skip the ._store and go for (let [, scope] of

s/of/in/

Comment 23

[Brandon Benvie [:benvie]]

(In reply to Victor Porof [:vp] from comment #21)
> You can skip the ._store and go for (let [, scope] of
> sidebar.variablesView). Same goes with everything else (scopes, variables,
> properties). Take a look at the __iterator__s defined in VariablesView.jsm.

I had tried originally to use them directly as iterables, but it didn't work. It appears that the iterator protocol in SpiderMonkey has been changed:

    let iterable = { __iterator__: function(){ yield 5 } };
    for (let val of iterable){} // Exception: Object is not iterable

    let iterable = { iterator: function(){ yield 5 } };
    for (let val of iterable){} // works

Comment 24

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

(In reply to Brandon Benvie [:bbenvie] from comment #23)
>     let iterable = { __iterator__: function(){ yield 5 } };
>     for (let val of iterable){} // Exception: Object is not iterable
> 

Does for (let val in iterable){} work? (*in*, not *of*).

Comment 25

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

(In reply to Victor Porof [:vp] from comment #24)
> (In reply to Brandon Benvie [:bbenvie] from comment #23)
> >     let iterable = { __iterator__: function(){ yield 5 } };
> >     for (let val of iterable){} // Exception: Object is not iterable
> > 
> 
> Does for (let val in iterable){} work? (*in*, not *of*).

Apparently not anymore..

Comment 26

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

(In reply to Victor Porof [:vp] from comment #25)
> 
> Apparently not anymore..

Scratch that, it works, like it always has.

Comment 27

[Brandon Benvie [:benvie]]

(In reply to Victor Porof [:vp] from comment #26)
> (In reply to Victor Porof [:vp] from comment #25)
> > 
> > Apparently not anymore..
> 
> Scratch that, it works, like it always has.

Yeah my bad. I didn't realize that `__iterator__` worked with for-in. So eventually we should switch to `iterator` and for-of but for now I'll use for-in in this test (or matchVariablesViewProperty).

Comment 28

[Brandon Benvie [:benvie]]

Attachment: WIP9

Update the test to use for-in.

Comment 29

[Mihai Sucan [:msucan]]

Comment on attachment 738564 [details] [diff] [review]
WIP9

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

This looks really good!

General comments:

- I would suggest moving scratchpad.css to browser/themes/shared/devtools.
- Please remove all of the trailing whitespaces.

::: browser/devtools/scratchpad/scratchpad.js
@@ +453,5 @@
>     */
>    inspect: function SP_inspect()
>    {
> +    let deferred = Promise.defer();
> +    let reject = () => deferred.reject();

No reject reason. Why not pass the original reject reason from execute() or from sidebar.open()?

@@ +1496,5 @@
> +   */
> +  _splitter: null,
> +
> +  /*
> +   * The VariablesView for this sidebar

nit: there's inconsistency between using a dot at the end of comment or not. Please make sure this is consistent with the rest of the file.

@@ +1521,5 @@
> +  {
> +    this.show();
> +
> +    if (this.variablesView) {
> +      return this._update(aObject);

This is slightly confusing.

Below you assume that the sidebar may have multiple tabs and you try to select the right one, then onTabReady you always create a new instance of vview.

I suggest you remove this check from here, and below, in onTabReady, you create the new vview only if needed.

Even better would be to simplify the function and always assume a single tab in the sidebar.

@@ +1529,5 @@
> +
> +    let onTabReady = () => {
> +      let window = this._sidebar.getWindowForTab("variablesview");
> +      let container = window.document.querySelector("#variables");
> +      this.variablesView = new VariablesView(container);

Depending on what you decide you may want to do the creation of a new vview only if !this.variablesView.

@@ +1598,5 @@
> +
> +/**
> + * Check whether a value is non-primitive
> + */
> +function isObject(value)

Once you switch to the remote debugging protocol you will need to use VariablesView.isPrimitive() instead of this new function.

::: browser/devtools/scratchpad/test/browser_scratchpad_inspect.js
@@ +31,2 @@
>  
> +    outer: for (let [, scope] in sidebar.variablesView) {

I wrote a bunch of helpers for searching in vview, for the web console tests, see head.js in browser/devtools/webconsole/test. We should reuse those in here.

Maybe you don't need to do this right now, but you will need those functions once you switch to the remote protocol.

You can move the functions you need from head.js into a new jsm specific for variables view testing - say VariablesViewTestHelpers.jsm. You can make this jsm only available for tests:

https://developer.mozilla.org/en/docs/Mozilla/QA/Developing_tests#JavaScript_test-only_modules

Comment 30

[Brandon Benvie [:benvie]]

Attachment: WIP10

* Have `sidebar.open` assume there's only one tab
* Simplify `sidebar.open` to only have on execution path
* Have `Scratchpad.inspect` pass along failure reason
* Fix periods at the end of sidebar property descriptions

I'm going to leave the test as is for now and tackle moving the utility functions as part of bug843091.

Comment 31

[Brandon Benvie [:benvie]]

Attachment: WIP11

This patch moves the css to themes. However, it doesn't work and I'm not quite sure why.

Comment 32

[Girish Sharma [:Optimizer]]

(In reply to Brandon Benvie [:bbenvie] from comment #31)
> Created attachment 741473 [details] [diff] [review]
> WIP11
> 
> This patch moves the css to themes. However, it doesn't work and I'm not
> quite sure why.

Did you miss out on adding the theme css to the patch ? As far as I can tell, the patch does not contain a theme scratchpad.css, it contains a content scratchpad.css. And if by chance the content css was the only css that you wanted as theme css, It does not require a '*' in front (no pre-processing required in the file).

Comment 33

[Brandon Benvie [:benvie]]

Attachment: WIP12

Adds the missing css files (woops).

Comment 34

[Brandon Benvie [:benvie]]

Attachment: WIP13

Fixed it thanks to Optimizer. Issue was there needs to be two entries in windows/jar.mn (one normal and one aero).

Comment 35

[Rob Campbell [:rc] (:robcee)]

\o/!

What's next? Getting tests working probably.

Comment 36

[Brandon Benvie [:benvie]]

Tests are already fixed. This is done. Needs a tryrun, a review, and then a land.

Comment 37

[Brandon Benvie [:benvie]]

Try is all green https://tbpl.mozilla.org/?tree=Try&rev=f1a381803e48

Comment 38

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

Comment on attachment 741499 [details] [diff] [review]
WIP13

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

::: browser/devtools/scratchpad/test/browser_scratchpad_inspect.js
@@ +31,2 @@
>  
> +    outer: for (let [, scope] in sidebar.variablesView) {

This changed in bug 864802, since scopes can't have unique ids, do:
> for (let scope in sidebar.variablesView)

Comment 39

[Mihai Sucan [:msucan]]

Comment on attachment 741499 [details] [diff] [review]
WIP13

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

This is good stuff. Thanks for the updates.

Please note that the patch still has trailing whitespaces in scratchpad.js. Also see Victor's comment.

::: browser/devtools/jar.mn
@@ +13,5 @@
>      content/browser/devtools/netmonitor-view.js       (netmonitor/netmonitor-view.js)
>      content/browser/NetworkPanel.xhtml            (webconsole/NetworkPanel.xhtml)
>      content/browser/devtools/webconsole.js        (webconsole/webconsole.js)
>      content/browser/devtools/webconsole.xul       (webconsole/webconsole.xul)
> +    content/browser/scratchpad.css                (scratchpad/scratchpad.css)

Is this needed anymore? You have scratchpad.css in theme folders. Also scratchpad.xul doesn't seem to use this css.

::: browser/devtools/scratchpad/scratchpad.css
@@ +1,1 @@
> +/* vim:set ts=2 sw=2 sts=2 et: */

I like and use vim too, but this file's really small and I'm not sure if we want this modeline here.

::: browser/devtools/scratchpad/test/browser_scratchpad_inspect.js
@@ +11,5 @@
>      gBrowser.selectedBrowser.removeEventListener("load", onLoad, true);
>      openScratchpad(runTests);
>    }, true);
>  
> +  content.location = "data:text/html,<title></title>" +

Please add ;charset=utf8 here.

This page always causes a missing charset warning when loaded.

Comment 40

[Brandon Benvie [:benvie]]

Attachment: WIP14

I believe the cause of the character encoding warning was caused by either having a <title> or because the <p> in the datauri wasn't closed. Instead of adding a charset, I've removed the <title> and fixed the <p> (none of the other Scratchpad tests have charsets specified).

* remove redundent scratchpad.css from content
* remove EOL whitespace from scratchpad.js
* make the license comment in scratchpad.inc.css match the other .inc.css files (conditional compilation, no modeline)
* fix datauri in the inspect test
* update iteration of scopes sidebar.variablesView to reflect change from bug 864802

Comment 41

[Brandon Benvie [:benvie]]

https://tbpl.mozilla.org/?tree=Try&rev=f946213f7c99

Comment 42

[Mihai Sucan [:msucan]]

Comment on attachment 742434 [details] [diff] [review]
WIP14

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

The inspect.js timeouts for me now.

Also, I see:

TypeError: this.editor is null
Source File: chrome://browser/content/scratchpad.js
Line: 141

... in the error console, repeated several times, while the inspect.js test is stuck.

I applied the patch on top of the latest fx-team. I see the try push is green, maybe this needs a small rebase?

::: browser/devtools/scratchpad/test/browser_scratchpad_inspect.js
@@ +11,5 @@
>      gBrowser.selectedBrowser.removeEventListener("load", onLoad, true);
>      openScratchpad(runTests);
>    }, true);
>  
> +  content.location = "data:text/html,<p>test inspect() in Scratchpad</p>";

nit: if you load this data URI in the browser, while you have the browser console or the error console open, you will see a warning about the missing charset. It doesn't break the test, but simply adding ;charset=utf8 makes tests have less (irrelevant) output.

Comment 43

[Brandon Benvie [:benvie]]

Attachment: WIP15

I just pulled and rebased to latest fx-team. Also added `;charset=utf8` to the inspector test. https://tbpl.mozilla.org/?tree=Try&rev=4814e88df22d

Comment 44

[Brandon Benvie [:benvie]]

Forgot to note that all Scratchpad tests did pass for me.

Comment 45

[Brandon Benvie [:benvie]]

Attachment: WIP16

This should fix it. Switches to using require for ToolSidebar since the change to the new loader landed and Sidebar.jsm was changed to Sidebar.js.

https://tbpl.mozilla.org/?tree=Try&rev=b88a59bd1253

Comment 46

[Brandon Benvie [:benvie]]

The try push accidentally includes another patch for some reason, but my patch file doesn't so it shouldn't matter.

Comment 47

[Mihai Sucan [:msucan]]

Comment on attachment 743223 [details] [diff] [review]
WIP16

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

Thanks for the updates! Really good stuff.

Comment 48

[Brandon Benvie [:benvie]]

Woo awesome! Thanks for reviewing this for me!

Comment 49

[Brandon Benvie [:benvie]]

Attachment: WIP17

Rebase to bug 867485

Comment 50

[Brandon Benvie [:benvie]]

bug 867485 got backed out. Land WIP16 while it's backed out, WIP17 if it's relanded. /blame dcamp

Comment 51

[Rob Campbell [:rc] (:robcee)]

Landed WIP16.

https://hg.mozilla.org/integration/fx-team/rev/dfebbc839392

Comment 52

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/dfebbc839392