Closed Bug 636723 Opened 9 years ago Closed 9 years ago

Turn Workspaces into a Jetpack

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dangoor, Assigned: msucan)

Details

(Whiteboard: [workspaces])

The Workspaces add-on started life as a traditional add-on. We should turn it into a Jetpack and experiment with making the code evaluation routines into e10s-ready Jetpackified APIs that can be promoted to the devtools SDK.
One other note: the addon should change to use Jetpack Panels rather than XUL panels.
I wanted to add that we need to make the workspace embeddable as well. Perhaps we allow the construction of a workspace with it's containing node passed into the constructor?

myEmbedWorkspace = new Workspace({parentNode: aNode});

I know I will want a workspace sitting always to the left of my console, making the best use of my small laptop screen.
Yeah, that's definitely something we need to be able to support.

Currently I am working on the initial jetpack prototype. First we'll need to decide on some devtools SDK API stuff that I am using. Will tell more about what I mean ... when the code is ready. ;)
(In reply to comment #2)
> I wanted to add that we need to make the workspace embeddable as well. Perhaps
> we allow the construction of a workspace with it's containing node passed into
> the constructor?
> 
> myEmbedWorkspace = new Workspace({parentNode: aNode});
> 
> I know I will want a workspace sitting always to the left of my console, making
> the best use of my small laptop screen.

This is off-topic for this bug. Also, I don't think this is necessarily the best solution to what is probably a more general problem. We should discuss the options there!
(In reply to comment #4)
> (In reply to comment #2)
> > I wanted to add that we need to make the workspace embeddable as well. Perhaps
> > we allow the construction of a workspace with it's containing node passed into
> > the constructor?
> > 
> > myEmbedWorkspace = new Workspace({parentNode: aNode});
> > 
> > I know I will want a workspace sitting always to the left of my console, making
> > the best use of my small laptop screen.
> 
> This is off-topic for this bug. Also, I don't think this is necessarily the
> best solution to what is probably a more general problem. We should discuss the
> options there!

That kind of extensibility seems a key feature here, since it provides the platform for novelty that has historically been the cornerstone of our success. I'd prefer a modular workspace component that doesn't depend on any specific UI approach.

Embed-ability of the workspace ensures that all UI approaches are available for experimentation by developers that are not us. The question of floating panels vs Firebug-style in-app panels becomes irrelevant, since any UI approach can easily be prototyped.
A Jetpack-specific comment: The high-level APIs shouldn't use the browser's DOM directly, since at some point they'll all run out of the main process.

When designing the various modules, should try to keep the code that touches the browser internals as abstracted as possible from the Jetpack stuff that creates user-facing UI.
Robert: I have the first prototype ready to show.

https://github.com/mihaisucan/workspace-jetpack

- the workspace UI is not split from the workspace API as requested by David, at the moment - that's on purpose actually. We can't make a bite that's too big. Let's have something to discuss, to start with. Just like the Workspace extension is tied to its UI.

I like the idea of separating the code from the UI, such that we can have different UIs, in different contexts. We must do that, but we need to properly address it. I think we need a separate bug for it, as Kevin seems to suggest (?).

- the UI now uses a pristine jetpack panel implementation, which means we have no title bar, no panel resizer, uhm, nothing. we can't even properly anchor the panel to the jetpack widget (icon) we have in the addon bar. see bug 638142.

- we definitely need a fix for bug 637291 - we need title bars and resizability.

- we need to be able to check if a panel is open or not. see bug 638131.

- we have problems with <select> elements and tooltips not showing in xul:iframes inside xul:panels.

- the main.js code deals with the workspace panel and widget. It provides the ability to evaluate code and inspect objects. The panel implements its own UI code in data/panel.js. It sends the selected code to the main addon code for evaluation/inspection, depending on the selected option.

- main.js uses sdk.js which is just a placeholder for what we really want in the future. main.js is coded such that it doesn't use *any* low level API - we just require("sdk").

- the sdk provides two methods to retrieve the active content window object or the active chrome window object.

Currently, the situation "sucks". Jetpack offers API to retrieve the active window object and the active tab object, but no way to actually access the chrome window object, or the tab content window object. While that is very much expected - it would create e10s issues... we'd still need some lower-level API to give us just that.

We can't pass a window/tab object that we get from jetpack to the sandbox, for example. :)

Anyhow, this is something we need to change. We obviously cannot return the real chrome/content window objects, to the main addon code. We need to use "wrappers" - perhaps based on window IDs?

The current code in main.js doesn't "care" what kind of window object is returned, so we are safe in this way. We just pass it to the Sandbox SDK abstraction.

- the sdk abstracts away the Sandbox as well, and it takes a plain DOM Window object, but if we are going the way of abstracting window objects using their IDs, then no problem. The Sandbox abstraction is constructed such that we evaluate code in an async manner. Once the evaluation result is available, a user-provided callback is invoked with the result.

I expect things are pretty safe in this regard.

- the current PropertyPanel we have is definitely not the way to go, but I abstracted it away from the main.js code. It takes an sdk.Sandbox object, such that the Update button works. This will obviously be replaced by the future/upcoming property panel, that will be jetpack-based and so on.

Other things to mention, problems I found when working on this code:

- cfx init with hidden files/folders fails to work. See bug 635169.

- you can't use capital letters in the addon name. Capital letters also cause problems with module names (require() fun). Both issues are known in #jetpack and they are going to be solved. (no bug numbers)

Moving on:

- I'd like a review/comments on the code, things you want me to change.
- What shall I work on, next?

Thanks!
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #2)
> > > I wanted to add that we need to make the workspace embeddable as well. Perhaps
> > > we allow the construction of a workspace with it's containing node passed into
> > > the constructor?
> > > 
> > > myEmbedWorkspace = new Workspace({parentNode: aNode});
> > > 
> > > I know I will want a workspace sitting always to the left of my console, making
> > > the best use of my small laptop screen.
> > 
> > This is off-topic for this bug. Also, I don't think this is necessarily the
> > best solution to what is probably a more general problem. We should discuss the
> > options there!
> 
> That kind of extensibility seems a key feature here, since it provides the
> platform for novelty that has historically been the cornerstone of our success.
> I'd prefer a modular workspace component that doesn't depend on any specific UI
> approach.

I want a modular workspace as well. I'd like to be able to drop them into object inspectors and have them inherit that object as its scope. There are likely many other use cases for this type of pluggability that we can't think of.

We're still prototyping though, so I expect this to come as we iterate on it.
mihai: I agree with you 100% in comment 7 so I'm not going to repeat it all here and reply point-by-point. These are the issues we need to figure out if we're going to stick with this.

From a usability perspective, the workspace is a close approximation of the prototype (http://antennasoft.net/robcee/workspace/), but it really highlights the limitations of the current panel API. As you mention, we need features like those in bug 637291 to make it feel more complete. I think anchoring ceases to be an issue with a panel with a titlebar in it.

Another important bug is creating context (popup) menus for Addons. This differs from the Context module in that it would be good to create a complete popups for use in other bits of UI (non content, panels or windows).

Looking at the code, I'm not a fan of the | const $self = require("self.js"); | notation. I'd sooner see | const SELF | (still ugly) or | const Self | if you really need to differentiate from local variables.

The PropertyPanel references in sdk.js should probably be broken out into a separate Addon-SDK-based implementation of the PropertyPanel which we're going to need anyway if we want to beef it up. (TODO: file this bug!)

in (main.js):
   let window = getWindowForContext(aContext);

we know we're cheating here.

references to the Sandbox are also going to change.

otherwise, this is fine, as long as we're aware that requiring the chrome module is a "cheat" until we nail down how we want to manage cross process boundary object marshaling and messaging.
(In reply to comment #9)
> mihai: I agree with you 100% in comment 7 so I'm not going to repeat it all
> here and reply point-by-point. These are the issues we need to figure out if
> we're going to stick with this.

If we are going to use jetpack ... we kinda are required to stick with this. Let's just "whip the code" into a good shape.


> From a usability perspective, the workspace is a close approximation of the
> prototype (http://antennasoft.net/robcee/workspace/), but it really highlights
> the limitations of the current panel API. As you mention, we need features like
> those in bug 637291 to make it feel more complete. I think anchoring ceases to
> be an issue with a panel with a titlebar in it.

Agreed.

> Another important bug is creating context (popup) menus for Addons. This
> differs from the Context module in that it would be good to create a complete
> popups for use in other bits of UI (non content, panels or windows).

Agreed, as well. I looked into the jetpack docs, and I didn't find any nice solution (or any at all?).

> Looking at the code, I'm not a fan of the | const $self = require("self.js"); |
> notation. I'd sooner see | const SELF | (still ugly) or | const Self | if you
> really need to differentiate from local variables.

Yeah, will change the code wrt. the notation.


> The PropertyPanel references in sdk.js should probably be broken out into a
> separate Addon-SDK-based implementation of the PropertyPanel which we're going
> to need anyway if we want to beef it up. (TODO: file this bug!)

Yeah, we'll do these things as the pieces of the puzzle fall into place. That's pretty much a placeholder for any upcoming PropertyPanel implementation.

> in (main.js):
>    let window = getWindowForContext(aContext);
> 
> we know we're cheating here.
> 
> references to the Sandbox are also going to change.
> 
> otherwise, this is fine, as long as we're aware that requiring the chrome
> module is a "cheat" until we nail down how we want to manage cross process
> boundary object marshaling and messaging.

Agreed. We have to decide on the API we want and on what kind of objects we marshal across processes.

The main.js code does not have any chrome refs. The devtools SDK (the sdk.js) will inevitably need access to chrome - as we previously discussed.

Thanks for your reply!
(In reply to comment #5)
> That kind of extensibility seems a key feature here, since it provides the
> platform for novelty that has historically been the cornerstone of our success.
> I'd prefer a modular workspace component that doesn't depend on any specific UI
> approach.
> 
> Embed-ability of the workspace ensures that all UI approaches are available for
> experimentation by developers that are not us. The question of floating panels
> vs Firebug-style in-app panels becomes irrelevant, since any UI approach can
> easily be prototyped.

OK, I'll buy that. Extensibility, especially for devtools, is a good thing as long as it doesn't add too much complexity. Regardless, I think that's something for a separate bug.
WRT to the Jetpack Panel API, it'd be awesome to have some patches on those bugs. FWIW, a large number of the Jetpack high-level APIs were written by Fx-team people in addition to their normal Firefox work. Contributions from Devtools to help lift up Jetpack are welcome as well!

Comment #7 is exactly right in summarizing the suckiness of Jetpack for something like this, and here's why: The Jetpack SDK is targeted at the casual add-on developer. It's not intended to be as powerful and flexible as having the entire guts of Firefox at your fingertips. Because of this, the typical process for building Jetpack APIs involves a lot of iteration on a given API design to see whether it's even possible to do that thing in a way that doesn't require deep and constant low-level platform access. Seems like you've hit this from the UI end first, finding problems as you try to work your way from the outside in.

Is there a spec for what the use-cases are, and what the API might look like, for what's described in comment #0? That might help clarify whether Jetpack is the right tool. It also might highlight some patterns that'll make for clean high- and low-level interactions.
Might be a good idea to mark the bugs you encounter in the whiteboard somehow, so we can track the kinds of problems you hit while trying to use Jetpack to build developer tools.
(In reply to comment #12)
> Comment #7 is exactly right in summarizing the suckiness of Jetpack for
> something like this, and here's why: The Jetpack SDK is targeted at the casual
> add-on developer. It's not intended to be as powerful and flexible as having
> the entire guts of Firefox at your fingertips. Because of this, the typical
> process for building Jetpack APIs involves a lot of iteration on a given API
> design to see whether it's even possible to do that thing in a way that doesn't
> require deep and constant low-level platform access. Seems like you've hit this
> from the UI end first, finding problems as you try to work your way from the
> outside in.

I think we're in complete agreement, and I hope Mihai's comment came across in the vein in which it was intended: not as a criticism of Jetpack but rather a reflection on the work we've got to do to make Jetpack a nice platform for building devtools.

From our perspective, we (the devtools team) don't expect the Jetpack SDK to provide all of the low-level access to the platform. We expect to have to create high-level APIs that do things that are useful for developer tools, using low-level mechanisms (either require("chrome") or code that we land on mozilla-central directly).

Where it makes sense to do so, we absolutely will submit patches for the Jetpack SDK.

> 
> Is there a spec for what the use-cases are, and what the API might look like,
> for what's described in comment #0? That might help clarify whether Jetpack is
> the right tool. It also might highlight some patterns that'll make for clean
> high- and low-level interactions.

I just spoke with Mihai about this, and we'll try to work out those APIs in a separate bug. The trick there (one that I know you're familiar with!) is to anticipate e10s and make APIs that assume the process boundaries that will be there real soon now.
(In reply to comment #14)
> I hope Mihai's comment came across in
> the vein in which it was intended: not as a criticism of Jetpack but rather a
> reflection on the work we've got to do to make Jetpack a nice platform for
> building devtools.

Yeah, no worries, I wear bulletproof pants ;) I just wanted to make sure the expectations were correct. Happens all the time with Jetpack that experienced XUL devs feel like they're trying to hammer a nail with jello.
Dietrich, we are absolutely wearing the peril sensitive sunglasses with this stuff and are fully aware we're kind of exceeding the specs with what we want to do. We're going to be playing the "Should this go in Jetpack or in our Devtools SDK" game with every bump we hit. Expectations are tuned accordingly. :)

(In reply to comment #13)
> Might be a good idea to mark the bugs you encounter in the whiteboard somehow,
> so we can track the kinds of problems you hit while trying to use Jetpack to
> build developer tools.

we can add a [devtools] tag to the whiteboard if that helps.
marking this fixed. proof is here:

https://github.com/mihaisucan/workspace-jetpack
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.