642176 - Integrate Workspace extension into the browser

Status: VERIFIED FIXED

(DevTools :: General, defect)

Description

[Mihai Sucan [:msucan]]

We need to pull the Workspace extension code into the mozilla-central browser code base.

See:
https://github.com/robcee/workspace

Comment 1

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

thanks for filing

Comment 3

[Mihai Sucan [:msucan]]

Attachment: wip 1

This is the first WIP.

Merged Workspace into the browser. Did some changes for localization. Please let me know if these changes and the rest of the changes are fine.

Pending work: add unit tests before we can ask for review from a browser peer. Is there anything else beyond that? We'll also have to ask for l10n review.

Thanks!

Comment 4

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

as mentioned in irc, there are some MPL boilerplate issues in the xul file.

Otherwise, I thought this looked pretty good on a first-pass. I'll give a more formal review when the license files are cleaned up, then we can check strings.

Thanks for the patch!

Comment 5

[Mihai Sucan [:msucan]]

Attachment: wip 2

Fixed the XUL license.

Comment 6

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

Comment on attachment 520208 [details] [diff] [review]
wip 2

in browser.js:

+let Workspace = {

Let's use | var Workspace | here.

in workspace.xul:
- The Initial Developer of the Original Code is
- Mozilla Corporation.

Initial developer is always The Mozilla Foundation.

in workspace.js, line 82:

get sandbox() {
// need to cache sandboxes if currentBrowser == previousBrowser

This comment should probably be changed to a TODO with a bug filed. It's future work, so I'm not sure the comment's really relevant here.

We're also going to need javadoc style comments for each of the methods in this file. I don't mind writing them if you're tired of typing. (and yes, I realize you just copied this straight over from the extension. My fault for not writing them in the first place)

We should also convert the functions to Mozilla local code-style:

funname: function PRE_funname()
{
}

I'm ok with internal bracing styles in this file, but new-line after function declaration seems to be an important one around here.

in browser.dtd, line 208:
<!ENTITY workspace.execute.key         "e">

Let's change that to "t". Ctrl-E is overloaded on unix to end of line and we want to preserve that. Cmd/Ctrl-T should be free to use in a workspace window.

line 211,
<!ENTITY workspace.placeholder         "// Enter some JavaScript, select it, right click and choose your weapon.">

Let's change that to something a little less violent. It was ok in the add-on context, but we should be a little nicer here. How about:

"// Enter some JavaScript, select it, right click and select Execute, Inspect or Print.">

Less catchy, I know.

Otherwise, I think is good stuff. F+ with above addressed.

Comment 7

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

oh, and one other thing. This needs a pref to be able to hide the menu options.

devtools.workspace.enabled and some code in browser.js' delayedStartup() function. Menu item defaults should be hidden and disabled.

Comment 8

[Mihai Sucan [:msucan]]

Attachment: proposed patch

Thanks Robert for your feedback+!

Patch changes:

- changed from let Workspace to var Workspace, in browser.js.
- changed licenses to say Mozilla Foundation as initial developer, instead of Mozilla Corporation.
- reported bug 646524 as a follow up and updated workspace.js to mention it (see get sandbox()).
- added javadoc comments to all Workspace methods.
- changed the code to match even more the Mozilla code style.
- changed workspace.execute.key from "e" to "t".
- updated workspace.placeholder to have the string recommended by Robert.
- added the devtools.workspace.enabled pref.
- added code to browser.js delayedStartup() which enables/disables Workspace based on the new pref.
- added code to Workspace.hasClipboard() which checks if there's content in the clipboard. this method was empty.

These changes should address Rob's feedback.

(will work on unit tests in bug 636725.)

Comment 9

[David Dahl :ddahl]

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


>diff --git a/browser/base/content/workspace.js b/browser/base/content/workspace.js
>new file mode 100644
>--- /dev/null
>+++ b/browser/base/content/workspace.js
>@@ -0,0 +1,480 @@
>+#ifdef 0
Not sure if I would #ifdef this out as this file will no doubt grow and debugging will be more convoluted

>+/* vim:set ts=2 sw=2 sts=2 et:
>+ * ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
...
>+ *
>+ * ***** END LICENSE BLOCK *****/
>+#endif

>+/**
>+ * The workspace object handles the Workspace window functionality.
>+ */
>+Workspace = {
'var Workspace' - as I think this will warn each time it is launched


>+
>+  updateEditUIVisibility: function WS_updateEditUIVisibility()
>+  {
>+    if (this.hasSelection) {
>+      document.getElementById("ws-menu-cut").removeAttribute("disabled");
>+      document.getElementById("ws-menu-copy").removeAttribute("disabled");
>+    } else {
nit: else on newline

>+
>+  /**
>+   * Evaluate a string in the active tab content window.
>+   *
>+   * @param string aString
>+   *        The script you want evaluated.
>+   * @return the script evaluation result.
>+   */
>+  evalInSandbox: function WS_evalInSandbox(aString)
>+  {
>+    return Cu.evalInSandbox(aString, this.sandbox, "1.8", "Workspace", 1);
I think you should add a try/catch here and if you throw do Cu.reportError(ex) + Cu.reportError(ex.stack) (in the case of a chrome-scoped sandbox) then bring the error console into focus.

In the case of a content-scoped sandbox, open the web console, then do window.console.error(ex) + window.console.error(ex.stack)

This will really help in usability if you do not already have either console open - you may just sit there and wonder why there is "no feedback" from the Workspace.


>+    // If there is a evalString passed to this function, then add a `Update`
>+    // button to the panel so that the evalString can be reexecuted to update
>+    // the content of the panel.
>+    if (aEvalString !== null) {
>+      buttons.push({
>+        label: this.strings.
>+               GetStringFromName("propertyPanel.updateButton.label"),
>+        accesskey: this.strings.
>+                   GetStringFromName("propertyPanel.updateButton.accesskey"),
>+        oncommand: function () {
>+          try {
>+            let result = self.evalForContext(aEvalString);
>+
>+            if (result !== undefined)
>+              propPanel.treeView.data = result;
>+          } catch (ex) {
nit: catch on newline

This looks great. The feedback is very important here - I think the exceptions get swallowed by the sandbox and are nowhere to be seen otherwise.

Where are the tests?

Comment 10

[Mihai Sucan [:msucan]]

Attachment: updated patch

Thanks for your review David!

I have updated the patch and hopefully I have addressed your requests.

Changes:

- add Tools > Web Console to Workspace menu. It really belongs there together with the Error Console. Really feels like a nice addition. ;)

- added that 'var Workspace' - which I had in the unit tests patch: bug 636725 attachment 523405 [details] [diff] [review].

- also added some other minor changes from the unit tests patch.

- added some comments to methods I forgot last time.

- added error logging when some code fails to evaluate in the chrome and content sandboxes. These open the Web Console or Error Console window, as needed.


That's all. Looking forward for your updated review comments, thanks!

Comment 11

[Mihai Sucan [:msucan]]

The use of console.error() when exceptions occur is a good idea, so we can show them in the Web Console.

However, it just came to me that we can create an instance of nsIScriptError2 (IIANM) with the content window ID, to report the error properly. This would allow a better display of the error, and perhaps better control. Thoughts?

Comment 12

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

mihai, I think that's a great idea. If it's not too invasive and doesn't add a lot of code, I say do it.

Comment 13

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

requesting l10n review as well.

Comment 14

[David Dahl :ddahl]

(In reply to comment #12)
> mihai, I think that's a great idea. If it's not too invasive and doesn't add a
> lot of code, I say do it.

++

Comment 15

[Mihai Sucan [:msucan]]

Attachment: patch update 2

Updated the patch to use an nsIScriptError2 to report exceptions when code is executed in the content context. Looks better than console.error().

Comment 16

[David Dahl :ddahl]

Comment on attachment 524708 [details] [diff] [review]
patch update 2

Nice Job!

Comment 17

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

beauty. Adding sdwilsh for browser review.

Comment 18

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

also, since we should really get an l10n review on this because of the new strings, it's unlikely we'll get this in for fx5, though it pains me deeply.

Comment 19

[Shawn Wilsher :sdwilsh]

I'm not going to be able to finish the review on this by today so it will make Firefox 5.  The good news is that we'll be merging into Aurora in six weeks, so we'll get more widespread feedback soon enough!

Comment 20

[Shawn Wilsher :sdwilsh]

Comment on attachment 524708 [details] [diff] [review]
patch update 2

>+++ b/browser/base/content/browser.js
>+  // Enable Workspace?
This could stand to be a better comment.

>+var Workspace = {
>+  prefEnabledName: "devtools.workspace.enabled",
>+
>+  openWorkspace: function WS_openWorkspace() {
>+    const WORKSPACE_WINDOW_URL = "chrome://browser/content/workspace.xul";
I think, since you have the toolbar enabled, you want to use Mossop's new protocol for UI stuff (like the add-ons manager).  I'm told there isn't a bug on file for it yet though, so we may need to do this in a follow-up.  Please poke him.

>+++ b/browser/base/content/workspace.js
>+  /**
>+   * Retrieve the xul:textbox DOM element.
>+   */
>+  get textbox() document.getElementById("workspace-textbox"),
>+
>+  /**
>+   * Retrieve the xul:statusbar DOM element.
>+   */
>+  get statusbar() document.getElementById("workspace-statusbar"),
>+
>+  /**
>+   * Retrieve the xul:statusbarpanel DOM element.
>+   */
>+  get statusbarStatus() document.getElementById("workspace-status"),
These comments are not terribly useful since the getters are pretty self describing.  To make them more useful, indicating what would be expected to be placed in them (like in the case of statusbarStatus) would be nice.

>+  get selectedText()
>+  {
>+    let text = this.textbox.value;
>+    let selectionStart = this.textbox.selectionStart;
>+    let selectionEnd = this.textbox.selectionEnd;
>+    if (selectionStart != selectionEnd)
>+      return text.substring(selectionStart, selectionEnd);
global-nit: brace one line ifs

>+  get sandbox()
>+  {
>+    // TODO: bug 646524 - need to cache sandboxes if
>+    // currentBrowser == previousBrowser
I've asked some questions in that bug.  Not yet sure if this is follow-up worthy or should be fixed now.

>+  /**
>+   * Get the Cu.Sandbox object for the most recently active navigator:browser
>+   * chrome window object.
>+   */
>+  get chromeSandbox()
>+  {
>+    return new Cu.Sandbox(this.browserWindow,
>+                          { sandboxPrototype: this.browserWindow,
>+                            wantXrays: false });
browserWindow can be null.  What happens in that case?

>+    let str = new Object();
>+    let strLength = new Object();
It is generally considered to be better practice to do |let str = {};| instead.

>+  /**
>+   * Update the Edit UI visibility.
Stating what it updates would be more useful (cut, copy, paste).

>+  /**
>+   * Drop the textbox selection.
>+   *
>+   * @return this
>+   *         Returns the Workspace object itself.
>+   */
>+  deselect: function WS_deselect()
>+  {
>+    this.textbox.selectionEnd = this.textbox.selectionStart;
>+    return this;
>+  },
There is no benefit to returning |this| here, and no call site uses it.  Also update the header.

>+  /**
>+   * Execute the selected text (if any) or the entire textbox content in the
>+   * current context.
>+   */
>+  execute: function WS_execute()
>+  {
>+    let selection = this.selectedText || this.textbox.value;
>+    this.evalForContext(selection);
>+    this.deselect();
Why are we deselecting after the selected text is executed?  I'm not sure this is the right user experience, but I'm open to being wrong!

>+  /**
>+   * Execute the selected text (if any) or the entire textbox content in the
>+   * current context. The resulting object is opened up in the Property Panel
>+   * for inspection.
>+   */
>+  inspect: function WS_inspect()
>+  {
>+    let selection = this.selectedText || this.textbox.value;
>+    let result = this.evalForContext(selection);
>+
>+    if (result)
>+      this.openPropertyPanel(selection, result);
I actually think it makes sense for execute (above) to return [selection, result] and then inspect just calls it.  I can see it being useful to more than just inspect to know what execute ran, and what the result was.

>+  /**
>+   * Execute the selected text (if any) or the entire textbox content in the
>+   * current context. The code evaluation result is "printed" in the textbox.
>+   */
Describing in more detail how this is "printed" when having a text selection would be nice.  The code is a bit difficult to reason about, so having a textual description would be nice.

>+  print: function WS_print()
>+  {
>+    let selection = this.selectedText || this.textbox.value;
>+    let selectionStart = this.textbox.selectionStart;
>+    let selectionEnd = this.textbox.selectionEnd;
>+    if (selectionStart == selectionEnd)
>+      selectionEnd = this.textbox.value.length;
>+    let result = this.evalForContext(selection);
Ditto to the above re: having execute return [selection, result].

>+  exportToFile: function WS_exportToFile(aFile, aNoConfirmation)
>+  {
>+    if (!aNoConfirmation && aFile.exists() &&
>+        !window.confirm(this.strings.
>+                        GetStringFromName("export.fileOverwriteConfirmation")))
>+      return;
>+
>+    let fs = Cc["@mozilla.org/network/file-output-stream;1"].
>+              createInstance(Ci.nsIFileOutputStream);
>+    let modeFlags = 0x02 | 0x08 | 0x20;
>+    fs.init(aFile, modeFlags, 0644, 0);
>+    fs.write(this.textbox.value, this.textbox.value.length);
>+    fs.close();
NOOOOOOOOOOOOOOOOOO!  Please use NetUtil.asyncCopy here, and you should be passing in fs.DEFER_OPEN as a flag to the file stream.

>+  importFromFile: function WS_importFromFile(aFile)
>+  {
>+    let fs = Cc["@mozilla.org/network/file-input-stream;1"].
>+                createInstance(Ci.nsIFileInputStream);
>+    fs.init(aFile, -1, -1, 0);
>+    let sis = Cc["@mozilla.org/scriptableinputstream;1"].
>+                 createInstance(Ci.nsIScriptableInputStream);
>+    sis.init(fs);
>+    this.textbox.value = sis.read(sis.available());
>+    sis.close();
>+    fs.close();
Ooof.  I'm still out of breath from that last no, but same thing here.  You can actually use NetUtil.asyncFetch here, but you need to generate a channel first and set the contentType so the mime service doesn't go and try to figure it out (and then do disk I/O).

>+++ b/browser/base/content/workspace.xul
>+<window id="main-window"
>+        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+        title="&workspace.title;"
>+        windowtype="devtools:workspace"
>+        screenX="4" screenY="4"
>+        width="640" height="480"
This is often localized since strings can be different lengths in different locales, although I'm not sure it is needed in this case.

>+++ b/browser/locales/en-US/chrome/browser/browser.dtd
>+<!ENTITY workspace.title               "Workspace">
>+<!ENTITY workspace.label               "Workspace">
>+<!ENTITY workspace.keycode             "VK_F4">
>+<!ENTITY workspace.keytext             "F4">
>+<!ENTITY workspace.newwindow.label     "New Window">
>+<!ENTITY workspace.newwindow.accesskey "N">
>+<!ENTITY workspace.newwindow.key       "n">
>+<!ENTITY workspace.savefile.label      "Save">
>+<!ENTITY workspace.savefile.accesskey  "S">
>+<!ENTITY workspace.savefile.key        "s">
>+<!ENTITY workspace.saveas.label        "Save Asâ¦">
>+<!ENTITY workspace.saveas.accesskey    "A">
>+<!ENTITY workspace.execute.label       "Execute">
>+<!ENTITY workspace.execute.accesskey   "E">
>+<!ENTITY workspace.execute.key         "t">
>+<!ENTITY workspace.inspect.label       "Inspect">
>+<!ENTITY workspace.inspect.accesskey   "I">
>+<!ENTITY workspace.inspect.key         "i">
>+<!ENTITY workspace.print.label         "Print">
>+<!ENTITY workspace.print.accesskey     "p">
>+<!ENTITY workspace.print.key           "r">
>+<!ENTITY workspace.context.label       "Context">
>+<!ENTITY workspace.context.accesskey   "C">
>+<!ENTITY workspace.chrome.label        "Chrome">
>+<!ENTITY workspace.chrome.accesskey    "H">
>+<!ENTITY workspace.content.label       "Content">
>+<!ENTITY workspace.content.accesskey   "C">
>+<!ENTITY workspace.placeholder         "// Enter some JavaScript, select it, right click and select Execute, Inspect or Print.">
We may want to steal some of these from the browser (things like New Window and Save) and not redefine them here.  However, it's been a while since I've had to deal with l10n like this, so I'm going to defer to Pike on that.

This is only r- for the synchronous I/O issues.  I'll see if I can review the tests tomorrow.

Comment 21

[Shawn Wilsher :sdwilsh]

(In reply to comment #9)
> >+++ b/browser/base/content/workspace.js
> >@@ -0,0 +1,480 @@
> >+#ifdef 0
> Not sure if I would #ifdef this out as this file will no doubt grow and
> debugging will be more convoluted
I generally frown on doing this for this very reason, but it's your team's file, so your folks call.

Comment 22

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

(In reply to comment #20)
> Comment on attachment 524708 [details] [diff] [review]
> >+  /**
> >+   * Execute the selected text (if any) or the entire textbox content in the
> >+   * current context.
> >+   */
> >+  execute: function WS_execute()
> >+  {
> >+    let selection = this.selectedText || this.textbox.value;
> >+    this.evalForContext(selection);
> >+    this.deselect();
> Why are we deselecting after the selected text is executed?  I'm not sure this
> is the right user experience, but I'm open to being wrong!

It is. At minimum, it's a visual indicator that you've done something. Rerunning the same block you'll have to reselect (or run the entire contents). This is part of the way a Workspace is supposed to work.

> >+  exportToFile: function WS_exportToFile(aFile, aNoConfirmation)
> >+  {
> >+    if (!aNoConfirmation && aFile.exists() &&
> >+        !window.confirm(this.strings.
> >+                        GetStringFromName("export.fileOverwriteConfirmation")))
> >+      return;
> >+
> >+    let fs = Cc["@mozilla.org/network/file-output-stream;1"].
> >+              createInstance(Ci.nsIFileOutputStream);
> >+    let modeFlags = 0x02 | 0x08 | 0x20;
> >+    fs.init(aFile, modeFlags, 0644, 0);
> >+    fs.write(this.textbox.value, this.textbox.value.length);
> >+    fs.close();
> NOOOOOOOOOOOOOOOOOO!  Please use NetUtil.asyncCopy here, and you should be
> passing in fs.DEFER_OPEN as a flag to the file stream.

oops. No yelly! :)

> >+  importFromFile: function WS_importFromFile(aFile)
> >+  {
> >+    let fs = Cc["@mozilla.org/network/file-input-stream;1"].
> >+                createInstance(Ci.nsIFileInputStream);
> >+    fs.init(aFile, -1, -1, 0);
> >+    let sis = Cc["@mozilla.org/scriptableinputstream;1"].
> >+                 createInstance(Ci.nsIScriptableInputStream);
> >+    sis.init(fs);
> >+    this.textbox.value = sis.read(sis.available());
> >+    sis.close();
> >+    fs.close();
> Ooof.  I'm still out of breath from that last no, but same thing here.  You can
> actually use NetUtil.asyncFetch here, but you need to generate a channel first
> and set the contentType so the mime service doesn't go and try to figure it out
> (and then do disk I/O).

I sorry. My fault. I made Shawn cry. :(

...

thanks for the review, Shawn!

Comment 23

[Axel Hecht [:Pike]]

Reusing strings is bad in general, I'd not do that.

Why are the workspace strings in browser.dtd? I don't see an overlap between the two xul documents, so this looks like an unnecessary clutter.

Is there a screenshot to look at? That'd help on the sizing question. Also, ux might have an opinion on the units (px vs em or ex) or aspect ratio. Is the window resizable?

Comment 24

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

(In reply to comment #23)
> Reusing strings is bad in general, I'd not do that.
> 
> Why are the workspace strings in browser.dtd? I don't see an overlap between
> the two xul documents, so this looks like an unnecessary clutter.

good idea. browser.dtd -> workspace.dtd

> Is there a screenshot to look at? That'd help on the sizing question. Also, ux
> might have an opinion on the units (px vs em or ex) or aspect ratio. Is the
> window resizable?

You can see an earlier screenshot here:

http://antennasoft.net/robcee/workspace/

It's pretty basic, just a text editor in a window. The window is resizable.

Comment 25

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

Initial size should probably be smallish, I think, and I like 4:3 as a ratio, hence 400x300px.

We can certainly ask for some ux review on this.

Comment 26

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

Mihai's generated a try build:

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-c11860ebbc86/

cc'ing limi for ux review.

Comment 27

[Mihai Sucan [:msucan]]

(In reply to comment #23)
> Reusing strings is bad in general, I'd not do that.
> 
> Why are the workspace strings in browser.dtd? I don't see an overlap between
> the two xul documents, so this looks like an unnecessary clutter.

What I understand is you want us to make a new workspace.dtd that we use in workspace.xul, and we should no longer reuse anything from browser.dtd inside workspace.xul. Is this correct?

(I tend to agree with this idea since a translator working on browser.dtd can cause unexpected changes in the Workspace UI. It's better to keep things separate.)

Thanks for looking into the patch!

Comment 28

[Mihai Sucan [:msucan]]

Attachment: patch update 3

Updated the patch based on comment 20. Changes:

- improved code comments.
- braced one line ifs.
- added some missing strings I forgot in the previous patch iteration.
- also added new strings for new error messages: file read/write, etc.
- dealt with the browserWindow is null case.
- no longer returning |this| in deselect().
- execute() now returns [selection, result].
- inspect() and print() reuse execute().
- print() now properly selects the evaluation result.
- using async file read and write.

Will deal with bug 646524 separately.

Will update the patch for l10n review, once I have a better understanding of what needs to be done. Looking forward Axel's reply.

Thanks to both of you!

Comment 29

[Mihai Sucan [:msucan]]

Attachment: patch update 4

Updated the patch again.

Changes:

- moved all the Workspace UI strings from browser.dtd to a new workspace.dtd, as agreed with robcee, based on comments from Axel.

- removed Workspace.hasSelection, hasClipboard and updateEditUIVisibility. This is some clean up we should've done since the move from the extension to the in-browser code happen. Basically, the Edit > Cut/Copy/Paste/Undo/Redo menuitems are nicely handled by the editMenuOverlay.xul file - no need to duplicate its attempts. The code removed didn't really work well.

- made changes to exportToFile() and importFromFile() for unit tests. (incoming updated patch!)

- commented out unimplemented functionality: File > Print and Edit > Find. Reported bug 650340 and bug 650345.

Looking forward to your reviews. Thanks to all of you!

Comment 30

[Mihai Sucan [:msucan]]

Pushed this patch and its dependencies to the try server.

Green results:

http://tbpl.mozilla.org/?tree=MozillaTry&rev=ccf60a31b861

Builds and logs:

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-ccf60a31b861/

Comment 31

[Shawn Wilsher :sdwilsh]

Comment on attachment 526355 [details] [diff] [review]
patch update 4

>+++ b/browser/base/content/workspace.js
>+  get selectedText()
>+  {
>+    let text = this.textbox.value;
>+    let selectionStart = this.textbox.selectionStart;
>+    let selectionEnd = this.textbox.selectionEnd;
>+
>+    return selectionStart != selectionEnd ?
>+           text.substring(selectionStart, selectionEnd) : "";
substring already returns an empty string if the indexes are the same, so no reason to do that check on your own:
https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/String/substring

>+  /**
>+   * Get the Cu.Sandbox object for the active tab content window object.
>+   */
>+  get sandbox()
I think it would be better to call this contentSandbox so it's obvious everywhere we use it which sandbox we have.  I should have caught this last time; my apologies.

>+  evalInSandbox: function WS_evalInSandbox(aString)
Likewise, this should be evalInContentSandbox

>+  inspect: function WS_inspect()
>+  {
>+    let [selection, result] = this.execute();
>+
>+    if (result) {
>+      this.openPropertyPanel(selection, result);
>+    }
>+
>+    this.deselect();
execute calls deselect, so this isn't needed.

>+  print: function WS_print()
>+  {
>+    let selectionStart = this.textbox.selectionStart;
>+    let selectionEnd = this.textbox.selectionEnd;
>+    if (selectionStart == selectionEnd) {
>+      selectionEnd = this.textbox.value.length;
>+    }
>+
>+    let [selection, result] = this.execute();
>+    if (!result) {
>+      return;
>+    }
>+
>+    let firstPiece = this.textbox.value.slice(0, selectionEnd);
>+    let lastPiece = this.textbox.value.
>+                    slice(selectionEnd + 1, this.textbox.value.length);
>+
>+    this.textbox.value = firstPiece + "\n/* " + result.toString() + " */\n" +
>+                         lastPiece;
>+
>+    this.selectRange(selectionEnd + 1,
>+                     selectionEnd + 7 + result.toString().length);
This needs a comment.  I have no idea what you are trying to select here, and the magic number (7) isn't helping (you add eight characters above).  Consider adding some constants here to help explain things.

>+  /**
>+   * Export the textbox content to a file.
>+   *
>+   * @param nsILocalFile aFile
>+   *        The file where you want to save the textbox content.
>+   *
>+   * @param boolean aNoConfirmation
>+   *        If the file already exists, ask for confirmation?
>+   *
>+   * @param boolean aSilentError
>+   *        True if you do not want to display an error when file save fails,
>+   *        false otherwise.
>+   *
>+   * @param function aCallback
>+   *        Optional function you want to call when file save completes.
>+   */
global-nit: you seem to do this in a few places (but not everywhere), but there shouldn't be newlines between @param and/or @return lines.

Please document what arguments aCallback gets when it is called.  See https://hg.mozilla.org/mozilla-central/annotate/a6467a88b056/netwerk/base/src/NetUtil.jsm#l131 for an example (but ignore the fact that it says two and lists three please).

>+  /**
>+   * Read the content of a file and put it into the textbox.
>+   *
>+   * @param nsILocalFile aFile
>+   *        The file you want to save the textbox content into.
>+   *
>+   * @param boolean aSilentError
>+   *        True if you do not want to display an error when file load fails,
>+   *        false otherwise.
>+   *
>+   * @param function aCallback
>+   *        Optional function you want to call when file load completes.
>+   */
Ditto here.

>+  importFromFile: function WS_importFromFile(aFile, aSilentError, aCallback)
>+  {
>+    // Prevent file type detection.
>+    let channel = NetUtil.newChannel(aFile);
>+    channel.contentType = "application/javascript";
>+
>+    let self = this;
>+    NetUtil.asyncFetch(channel, function(aInputStream, aStatus) {
>+      if (Components.isSuccessCode(aStatus)) {
>+        self.textbox.value =
>+          NetUtil.readInputStreamToString(aInputStream,
>+                                          aInputStream.available());;
>+      }
>+      else if (!aSilentError) {
>+        window.alert(self.strings.GetStringFromName("openFile.failed"));
>+      }
>+
>+      if (aCallback) {
>+        aCallback.call(self, aStatus);
>+      }
I think it would be useful for the callback to also get a copy of the data that was read.

r=sdwilsh

Comment 32

[Mihai Sucan [:msucan]]

Attachment: [in-devtools] patch update 5

Updated the patch as requested by Shawn. Thanks for the review+!

Comment 33

[Mihai Sucan [:msucan]]

Pushed all the Workspace patches to the try server.

Green results:

http://tbpl.mozilla.org/?tree=MozillaTry&rev=85e57381da8b

Builds and logs:

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-85e57381da8b

Comment 34

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

http://hg.mozilla.org/projects/devtools/rev/037508c3bf0c

Comment 35

[Axel Hecht [:Pike]]

Comment on attachment 527213 [details] [diff] [review]
[in-devtools] patch update 5

r=me, I have a few nits, though:

browser-menubar.inc doesn't include an accesskey for the menuitem at all.
Going through the other menu items, I found it hard to read if they had accesskeys, as the attributes moved around merrily inside the elements. I'd stick to one style there.

One additional thought, do we need to use "Workspace" as a name? I'd encourage you to open that name up for comments in .l10n before landing, or at least add a rich localization note describing why that word works in en-US, and how you came up with it. Well, Rob did, same thing. In German at least, the translations sound like boilersuit or just suit, and not like fun. Don't know English enough to comment on wether that'd be different.

Comment 36

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

(In reply to comment #35)
> Comment on attachment 527213 [details] [diff] [review]
> [in-devtools] patch update 5
> 
> r=me, I have a few nits, though:
> 
> browser-menubar.inc doesn't include an accesskey for the menuitem at all.
> Going through the other menu items, I found it hard to read if they had
> accesskeys, as the attributes moved around merrily inside the elements. I'd
> stick to one style there.

ok, we can (and should) add one of those. bug 651872.

> One additional thought, do we need to use "Workspace" as a name? I'd encourage
> you to open that name up for comments in .l10n before landing, or at least add
> a rich localization note describing why that word works in en-US, and how you
> came up with it. Well, Rob did, same thing. In German at least, the
> translations sound like boilersuit or just suit, and not like fun. Don't know
> English enough to comment on wether that'd be different.

We can do that. There are issues with the name in that most people who don't have a Smalltalk background expect something completely different than what it is. I appropriated the name directly from there. I am a little concerned about opening up a huge bikeshed on this.

I kinda like Boilersuit though. ;)

Comment 37

[u60234]

Comment on attachment 527213 [details] [diff] [review]
[in-devtools] patch update 5

>+++ b/browser/locales/en-US/chrome/browser/workspace.dtd

>+<!ENTITY print.label                  "Print">
>+<!ENTITY print.accesskey              "p">
>+<!ENTITY print.key                    "r">

Is not Ctrl+R an unusual command for "Print"? And "P" is already used as access key for "Paste". Did they get mixed up?

Of course, "R" as access key is no good either. It is already used for "Redo".

Comment 38

[Axel Hecht [:Pike]]

I'd not expect a bikeshed from discussing this in .l10n, take it as opening up to other cultural influences. The point is, localize the name-finding process instead of the name. We have good experience with panorama, that was brought up by pascal in a l10n discussion, too.

And yeah, what Hasse said.

Comment 39

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

(In reply to comment #37)
> Comment on attachment 527213 [details] [diff] [review]
> [in-devtools] patch update 5
> 
> >+++ b/browser/locales/en-US/chrome/browser/workspace.dtd
> 
> >+<!ENTITY print.label                  "Print">
> >+<!ENTITY print.accesskey              "p">
> >+<!ENTITY print.key                    "r">
> 
> Is not Ctrl+R an unusual command for "Print"? And "P" is already used as access
> key for "Paste". Did they get mixed up?

This isn't "Print" as in "Print this page on a printer". This is "Evaluate and Print the result in the workspace". Maybe this menu option should be "Display" or "Inline" or something else to avoid confusion.

> Of course, "R" as access key is no good either. It is already used for "Redo".

Right. Maybe "N" or "T" for access key.

Comment 40

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

(In reply to comment #38)
> I'd not expect a bikeshed from discussing this in .l10n, take it as opening up
> to other cultural influences. The point is, localize the name-finding process
> instead of the name. We have good experience with panorama, that was brought up
> by pascal in a l10n discussion, too.

Discussion's here: https://groups.google.com/forum/#!topic/mozilla.dev.l10n/eYZU4uaLOrQ

Also the cross-posting to dev.apps.firefox generated some good replies as well.

https://groups.google.com/forum/#!topic/mozilla.dev.apps.firefox/eYZU4uaLOrQ

I think the current leading favorite is the somewhat clinical sounding "JavaScript Environment".

The reply in the l10n post suggesting Eclipse's equivalent of "Scrapbook" is not bad, but probably subject to the same criticisms as "workspace" for localization.

Comment 41

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

oh, also, bz and a blog commenter suggested "Scratchpad" as an alternative.

http://antennasoft.net/robcee/2011/04/21/workspace-fixed-in-devtoolsl10n/#comments

Comment 42

[Paul [pwd]]

In terms of connotation 'Scratch-pad' is certainly the most fitting for me. Workspaces makes me think of something similar to Panorama.

Comment 43

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

We had another +1 for Scratchpad (I'd recommend without the hyphen) in dev.apps.firefox. It's a good name and it suggests a good alternate use I've always liked about workspaces: you can use them to keep blobs of any text, not just Javascript.

Thanks for the comment, Paul.

Comment 44

[Erik Vold]

Why was I removed as a contributor to workspace.xul ?

Comment 45

[Erik Vold]

(In reply to comment #44)
> Why was I removed as a contributor to workspace.xul ?

https://github.com/robcee/workspace/commits/master/content/workspace.xul

Comment 46

[Mihai Sucan [:msucan]]

(In reply to comment #45)
> (In reply to comment #44)
> > Why was I removed as a contributor to workspace.xul ?
> 
> https://github.com/robcee/workspace/commits/master/content/workspace.xul

I apologize for that, it is an entirely unintentional mistake. The file had no credits. See:

https://github.com/robcee/workspace/blob/96bb01fadbc3b433721b81067eacebd4d62f5836/content/workspace.xul

I added them and I didn't know you also contributed changes to workspace.xul.

Will make a patch to add your name as well. Thanks!

Comment 47

[Mihai Sucan [:msucan]]

Attachment: [checked-in][in-devtools] update contributors list for workspace.xul

Adding Erik Vold to the workspace.xul contributors list.

Comment 48

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

Comment on attachment 529538 [details] [diff] [review]
[checked-in][in-devtools] update contributors list for workspace.xul

r+++++

Comment 49

[Mihai Sucan [:msucan]]

Comment on attachment 529538 [details] [diff] [review]
[checked-in][in-devtools] update contributors list for workspace.xul

Pushed:

http://hg.mozilla.org/projects/devtools/rev/7a6e57bf1b85

Comment 50

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

Comment on attachment 529538 [details] [diff] [review]
[checked-in][in-devtools] update contributors list for workspace.xul

http://hg.mozilla.org/mozilla-central/rev/037508c3bf0c

Comment 51

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

http://hg.mozilla.org/mozilla-central/rev/7a6e57bf1b85

Comment 52

[AndreiD[QA]]

VERIFIED FIXED ON:
Mozilla/5.0 (Windows NT 6.1; rv:6.0a1) Gecko/20110511 Firefox/6.0a1
Mozilla/5.0 (X11; Linux i686; rv:6.0a1) Gecko/20110512 Firefox/6.0a1
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a1) Gecko/20110511 Firefox/6.0a1