646524 - Workspace: cache the sandboxes

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Mihai Sucan [:msucan]]

In the new Workspace code we need to cache the sandbox objects for the current content and chrome windows.

Follow up from bug 642176.

Comment 1

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

I'm not sure how aggressive we need to be with these caches. I'm inclined to think that the simplest solution is the best one.

Cache the current sandbox until the user switches contexts and runs something elsewhere (i.e., creates a new sandbox to run code in a different tab / window).

Comment 2

[Mihai Sucan [:msucan]]

Yep, agreed.

Comment 3

[Shawn Wilsher :sdwilsh]

What is the cost of creating a sandbox?  What other benefits do we gain by caching them?

Comment 4

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

I believe the cost of sandbox creation is minimal. The main purpose for this is one of workflow.

1. Open a content workspace on a webpage.
2. Evaluate some code, define some variables.
3. Evaluate more code later on...

In step 3, without cached workspaces, any variables you defined in step 2 are lost (if they're not redefined when you evaluate your next bit of code). With cached sandboxes, variables have a lifetime and you can keep playing with them.

Comment 5

[Shawn Wilsher :sdwilsh]

(In reply to comment #4)
> In step 3, without cached workspaces, any variables you defined in step 2 are
> lost (if they're not redefined when you evaluate your next bit of code). With
> cached sandboxes, variables have a lifetime and you can keep playing with them.
What needs to happen between step 2 and 3 to make the variables go away?

Comment 6

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

oh, I'm saying that keeping those variables around is a desirable outcome. Right now the variables are thrown away between each execution.

What needs to happen between steps 2 and 3 to keep the variable bindings is caching the sandbox between executions. Right now, we create a new sandbox every time we evaluate a piece of code.

I think we can be pretty lazy about this. As I say in Comment 1, I think we can hold onto the sandbox for as long as the workspace is evaluating against a given tab (or for chrome). We could hang onto it indefinitely, I suppose, keeping a list of current contexts but that feels too heavy. Having an option to "reset" our sandbox might be a good idea in any case.

Comment 7

[Shawn Wilsher :sdwilsh]

(In reply to comment #6)
> oh, I'm saying that keeping those variables around is a desirable outcome.
> Right now the variables are thrown away between each execution.
Alright.  I'm OK with this being a follow-up as long as this is fixed on the dev-tools branch before that branch is merged with mozilla-central.  It is my opinion that this feature is not done enough to be merged to mozilla-central without this bug.

Comment 8

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

I kind of disagree. Workspaces as they are now in the add-on (which doesn't have this) are very usable.

Comment 9

[Kevin Dangoor]

I agree with comment 8 that Workspaces are useful without this feature, but I do think we should strive to get this caching in soon as well as it will make the feature that much nicer.

Comment 10

[Shawn Wilsher :sdwilsh]

(In reply to comment #8)
> I kind of disagree. Workspaces as they are now in the add-on (which doesn't
> have this) are very usable.
I wasn't saying it wasn't usable, but I don't think the experience would be complete.  For example, Panorama was usable when it first landed, but we could not have shipped it that way because the experience was not complete.

Comment 11

[Mihai Sucan [:msucan]]

Attachment: proposed patch

Since this bug is becoming important I worked on a patch.

I really like working with a cached sandbox. I tend to agree now with Shawn that this patch is very much desirable in the initial Workspace feature we give developers at large.

Looking forward to your feedback!

Please note that this patch requires the following patches from bugs: 642176, 636725 and 646070. (in the given order)

Comment 12

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

Comment on attachment 526362 [details] [diff] [review]
proposed patch

in workspace.dtd

+<!ENTITY resetContext.label           "Reset the context global object">

This is a somewhat ungainly-sounding menu item. How about just "Reset"? It's already in a "Context" menu so there is some extra hinting there.

Also, you should add a localization note explaining what "Reset" does to make it clearer for translators.

+<!ENTITY resetContext.accesskey       "R">

I think this is going to work fine. Thanks!

Comment 13

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

after some discussion about what's best for this label, and tossing around some ideas (one of which included adding some helpful text to the statusbar in the workspace on menu item hover), we decided to add a Help menu which links to the soon-to-be-created MDN page.

I still think it's best to keep the menu item's label to "Reset" and provide supporting text, either in the status-line or in the startup comment to explain it.

Comment 14

[Shawn Wilsher :sdwilsh]

Some food for thought: how does this interact with save?  The author might create a script that works, but, unknowingly to the author, it requires the use of a variable that isn't in the script.  The author saves it and tries to use it later, only to find that it no longer works.

Comment 15

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

(In reply to comment #14)
> Some food for thought: how does this interact with save?  The author might
> create a script that works, but, unknowingly to the author, it requires the use
> of a variable that isn't in the script.  The author saves it and tries to use
> it later, only to find that it no longer works.

Let's focus on this as a followup bug, and get the Integrate Workspaces finished up first, please. This is all further down-stream work that can happen after we get the first chunk landed. While I agree that it's possible for a developer to get themselves into some context-loaded state, I don't think we need to worry about saving that state between restarts or file+save/open cycles.

A nice thing about workspaces is that you should always be able to recreate your context by re-evaluating the text, possibly in some different order than the way it's written. Maybe we could annotate parts of the workspace with some fragments that indicate which blocks of text were evaluated in which order? Definitely a future feature worthy of a separate bug.

A developer is also going to find themselves in a "lost context" situation if they switch tabs, do something in the workspace and then go back to the original tab. While workspaces are great for prototyping, I don't think we should over-engineer them. They are essentially transient places to develop a bit of code before moving it to a more permanent location. For reusable bits of code, you can save the file as js.

Comment 16

[Shawn Wilsher :sdwilsh]

(In reply to comment #15)
> Let's focus on this as a followup bug, and get the Integrate Workspaces
> finished up first, please. This is all further down-stream work that can happen
> after we get the first chunk landed. While I agree that it's possible for a
> developer to get themselves into some context-loaded state, I don't think we
> need to worry about saving that state between restarts or file+save/open
> cycles.
Absolutely.  I just wanted to make sure it was on the radar.

Comment 17

[Kevin Dangoor]

(In reply to comment #15)
> A developer is also going to find themselves in a "lost context" situation if
> they switch tabs, do something in the workspace and then go back to the
> original tab. While workspaces are great for prototyping, I don't think we
> should over-engineer them. They are essentially transient places to develop a
> bit of code before moving it to a more permanent location. For reusable bits of
> code, you can save the file as js.

This comment is exactly why I was agreeing on this being a non-blocker. Workspaces are transient.

(In reply to comment #14)
> Some food for thought: how does this interact with save?  The author might
> create a script that works, but, unknowingly to the author, it requires the use
> of a variable that isn't in the script.  The author saves it and tries to use
> it later, only to find that it no longer works.

It's definitely time to get some more workflow and use cases docs up for all of the tools and their various combinations.

The idea here is that what's saved in a workspace isn't really "a script". It's just a collection of code that the user wants to re-evaluate and re-inspect from time-to-time and in specific circumstances.

This particular bug definitely reflects fixing something that doesn't work the way users would expect. I'm not so sure that there's something the Open/Save should do differently (unless we don't have good error reporting!)


sorry for getting off-topic for this bug... I'll work on resolving this question in this feature page:

https://wiki.mozilla.org/DevTools/Features/WorkspacesRefined

Comment 18

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

(In reply to comment #17)
> The idea here is that what's saved in a workspace isn't really "a script". It's
> just a collection of code that the user wants to re-evaluate and re-inspect
> from time-to-time and in specific circumstances.

That's a really important distinction that we haven't really broadcasted very well. Of course you *can* use Workspaces to build up re-runnable, full-functioning scripts and execute them, but that's almost a secondary side-effect of them. They're really for interactively trying out smaller pieces of code or prototyping a function.

Comment 19

[Mihai Sucan [:msucan]]

Attachment: updated patch

Updated the menuitem string as requested. Thanks for the feedback+!

Asking for review from David.

Comment 20

[Mihai Sucan [:msucan]]

Comment on attachment 526788 [details] [diff] [review]
updated patch

Thanks for the r+, David!

Asking for additional review from Shawn.

Comment 21

[Mihai Sucan [:msucan]]

Attachment: rebased patch

Rebased the patch on top of the latest "upstream" code.

Comment 22

[Shawn Wilsher :sdwilsh]

Comment on attachment 527217 [details] [diff] [review]
rebased patch

Can you also add a test for the following:
- the context is reset when switching between chrome and content
- the new menuitem added invokes the right API method

r=sdwilsh with those changes

Comment 23

[Mihai Sucan [:msucan]]

Attachment: [checked-in][in-devtools] patch update 2

Updated the patch as requested.

Thanks for the review+!

Comment 24

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

Comment on attachment 527498 [details] [diff] [review]
[checked-in][in-devtools] patch update 2

http://hg.mozilla.org/projects/devtools/rev/01f21f5be88d

Comment 25

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

Comment on attachment 527498 [details] [diff] [review]
[checked-in][in-devtools] patch update 2

http://hg.mozilla.org/mozilla-central/rev/8227473480f5

Comment 26

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

http://hg.mozilla.org/mozilla-central/rev/01f21f5be88d