912260 - Make Scratchpad use CodeMirror

Status: RESOLVED FIXED

(DevTools Graveyard :: Scratchpad, defect, P2)

Description

[Anton Kovalyov (:anton)]

This is a bug 816756 spin-off.

Comment 1

[Anton Kovalyov (:anton)]

Attachment: WIP 4

Comment 2

[Anton Kovalyov (:anton)]

Attachment: WIP 5

Comment 3

[Anton Kovalyov (:anton)]

Attachment: WIP 6

These tests still failing:

    browser_scratchpad_bug650345_find_ui.js
    browser_scratchpad_bug684546_reset_undo.js
    browser_scratchpad_bug714942_goto_line_ui.js
    browser_scratchpad_bug_650760_help_key.js
    browser_scratchpad_bug_653427_confirm_close.js
    browser_scratchpad_bug_660560_tab.js
    browser_scratchpad_bug_669612_unsaved.js
    browser_scratchpad_bug_699130_edit_ui_updates.js
    browser_scratchpad_bug_751744_revert_to_saved.js

Comment 4

[Anton Kovalyov (:anton)]

Attachment: WIP 7

These tests remain to be fixed or deleted:

    browser_scratchpad_bug714942_goto_line_ui.js
    browser_scratchpad_bug_650760_help_key.js
    browser_scratchpad_bug_660560_tab.js
    browser_scratchpad_bug_669612_unsaved.js
    browser_scratchpad_bug_699130_edit_ui_updates.js
    browser_scratchpad_bug_751744_revert_to_saved.js

Comment 5

[Anton Kovalyov (:anton)]

Attachment: WIP 8

Down to three failing tests. I think all of them involve XUL + CodeMirror integration.

  browser_scratchpad_bug714942_goto_line_ui.js
  browser_scratchpad_bug_650760_help_key.js
  browser_scratchpad_bug_699130_edit_ui_updates.js

Comment 6

[Anton Kovalyov (:anton)]

Attachment: WIP 9

Down to the last failing tests:

  browser_scratchpad_bug714942_goto_line_ui.js

Also will need to make sure all context/menu items work as expected.

Comment 7

[Anton Kovalyov (:anton)]

Attachment: WIP 10

0:15.72 Browser Chrome Test Summary
0:15.72 	Passed: 296
0:15.72 	Failed: 0
0:15.72 	Todo: 0

Now I need to make sure all context/menu items work as expected.

Comment 8

[Anton Kovalyov (:anton)]

Attachment: WIP 11

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

This patch is ready for feedback meaning that everything seems to be working and all Scratchpad tests pass.


## Patch statistics:

m-c (codemirror) ☭ git diff fx-team --stat
 browser/devtools/jar.mn                                                         |    7 +
 browser/devtools/scratchpad/scratchpad.js                                       |  235 +-
 browser/devtools/scratchpad/scratchpad.xul                                      |  148 +-
 browser/devtools/scratchpad/test/Makefile.in                                    |    1 -
 browser/devtools/scratchpad/test/browser_scratchpad_bug650345_find_ui.js        |   97 -
 browser/devtools/scratchpad/test/browser_scratchpad_bug684546_reset_undo.js     |    6 +-
 browser/devtools/scratchpad/test/browser_scratchpad_bug714942_goto_line_ui.js   |   23 +-
 browser/devtools/scratchpad/test/browser_scratchpad_bug_660560_tab.js           |   23 +-
 browser/devtools/scratchpad/test/browser_scratchpad_bug_669612_unsaved.js       |    6 +-
 .../devtools/scratchpad/test/browser_scratchpad_bug_699130_edit_ui_updates.js   |   12 +-
 .../devtools/scratchpad/test/browser_scratchpad_bug_751744_revert_to_saved.js   |    9 +-
 browser/devtools/scratchpad/test/browser_scratchpad_contexts.js                 |   17 +-
 browser/devtools/scratchpad/test/browser_scratchpad_execute_print.js            |   56 +-
 browser/devtools/scratchpad/test/browser_scratchpad_files.js                    |    4 +-
 browser/devtools/sourceeditor/codemirror/codemirror.css                         |  258 ++
 browser/devtools/sourceeditor/codemirror/codemirror.js                          | 5798 +++++++++++++++++++++++++++++++++++++
 browser/devtools/sourceeditor/codemirror/dialog/dialog.css                      |   32 +
 browser/devtools/sourceeditor/codemirror/dialog/dialog.js                       |   80 +
 browser/devtools/sourceeditor/codemirror/goto.js                                |   11 +
 browser/devtools/sourceeditor/codemirror/javascript.js                          |  479 +++
 browser/devtools/sourceeditor/codemirror/search/match-highlighter.js            |   88 +
 browser/devtools/sourceeditor/codemirror/search/search.js                       |  131 +
 browser/devtools/sourceeditor/codemirror/search/searchcursor.js                 |  143 +
 browser/devtools/sourceeditor/editor-commands.xul                               |  135 +
 browser/devtools/sourceeditor/editor.js                                         |  343 +++
 browser/devtools/sourceeditor/moz.build                                         |    3 +
 26 files changed, 7767 insertions(+), 378 deletions(-)
m-c (codemirror) ☭ 

Note that this patch touches neither Orion nor original SourceEditor code. Once I port all our tools and delete Orion and co. I expect the overall amount of code to actually go down.


## Architecture

I removed most of the abstraction code. As we learned, the underlying editor implementation is too important and swapping editors didn't work for us. Some methods, as you will see in the sourceeditor/editor.js, are directly mapped from CodeMirror onto the wrapping object. Most of the time I tried to stick to the CodeMirror API structure. This way I didn't have to write too many wrapper functions that do nothing but shuffle parameters around.

I decided not to use the overlay XUL files because we don't use them anywhere except for Scratchpad. If I'm wrong and I find some other tool that uses menu bar along the way it's easy to extract common items into a separate XUL file.


## UI

Since I decided to use CodeMirror addons for search and replace, the UI is not very Firefox-ey. We can fix that in the follow up bugs because it's just HTML. The advantage is that we will have a more consistent UI for search/replace/jump-to-line everywhere we use our editor.


## Feedback?

Please go through the code and leave comments on overall architecture and so on (but no nits for now!). In the meantime, I will be cleaning up, writing comments and, once done, will ask for a more official review.

Comment 9

[Anton Kovalyov (:anton)]

Attachment: WIP 12

Oops, left unused goto.js file in there. This patch is without it. Everything else including Try push is the same.

Comment 10

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

Comment on attachment 802711 [details] [diff] [review]
WIP 12

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

overall I think this looks good. Currently this breaks the debugger UI though. If you run a full mochitest browser chrome run, I expect it will fail.

::: browser/devtools/jar.mn
@@ +34,5 @@
> +    content/browser/devtools/cm/javascript.js                          (sourceeditor/codemirror/javascript.js)
> +    content/browser/devtools/cm/searchcursor.js                        (sourceeditor/codemirror/search/searchcursor.js)
> +    content/browser/devtools/cm/search.js                              (sourceeditor/codemirror/search/search.js)
> +    content/browser/devtools/cm/dialog.js                              (sourceeditor/codemirror/dialog/dialog.js)
> +    content/browser/devtools/cm/dialog.css                             (sourceeditor/codemirror/dialog/dialog.css)

should we expand the directory from cm to codemirror? makes it longer, but clearer, imo.

::: browser/devtools/scratchpad/scratchpad.js
@@ +15,5 @@
>  "use strict";
>  
> +const Cu = Components.utils;
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;

you could use destructuring.

const { Cu, Cc, Ci } = Components;

Though in this case I expect it's going to try to redeclare them and fail. You probably already have Cc and friends visible in this scope via the XULWindow, right?

(require("chrome") was probably unnecessary before, hence your removal?)

@@ -184,5 @@
> -   *        Optional, the end offset, zero based, where you want to stop
> -   *        replacing text in the editor.
> -   */
> -  setText: function SP_setText(aText, aStart, aEnd)
> -  {

removing setText? That seemed like a useful API for Scratchpads to have.

@@ +189,2 @@
>  
> +    if (!clean || !this._saved)

do you find this easier to understand than this.editor && this.editor.dirty ?

DeMorgan's rule is a harsh mistress.

I guess you need to change the logic to incorporate the !this._saved check.

@@ +269,4 @@
>      return this._sidebar;
>    },
>  
> +  setText: function SP_setText(...args)

ok, you moved it.

what values can setText accept as paramaters?

@@ +1281,2 @@
>  
> +      this.editor.editor.on("change", this._onChanged);

editor.editor.editor... :)

::: browser/devtools/scratchpad/scratchpad.xul
@@ +356,5 @@
> +    <menuitem id="se-cMenu-selectAll"
> +          label="&selectAllCmd.label;"
> +          key="key_selectAll"
> +          accesskey="&selectAllCmd.accesskey;"
> +          command="se-cmd-selectAll"/>

nice cleanups in this file.

Comment 11

[Anton Kovalyov (:anton)]

(In reply to Rob Campbell [:rc] (:robcee) from comment #10)
> Comment on attachment 802711 [details] [diff] [review]
> WIP 12
> 
> Review of attachment 802711 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> overall I think this looks good. Currently this breaks the debugger UI
> though. If you run a full mochitest browser chrome run, I expect it will
> fail.

It doesn't break debugger neither on my computer nor on Try (there are failures but they're from commandline/gcli; I'll investigate). Are you sure its not your rebase or something?

> 
> ::: browser/devtools/jar.mn
> @@ +34,5 @@
> > +    content/browser/devtools/cm/javascript.js                          (sourceeditor/codemirror/javascript.js)
> > +    content/browser/devtools/cm/searchcursor.js                        (sourceeditor/codemirror/search/searchcursor.js)
> > +    content/browser/devtools/cm/search.js                              (sourceeditor/codemirror/search/search.js)
> > +    content/browser/devtools/cm/dialog.js                              (sourceeditor/codemirror/dialog/dialog.js)
> > +    content/browser/devtools/cm/dialog.css                             (sourceeditor/codemirror/dialog/dialog.css)
> 
> should we expand the directory from cm to codemirror? makes it longer, but
> clearer, imo.

CM is just a URI, actual directory is still codemirror. Or were you talking about URIs? My plan is to get rid off 'cm' or 'codemirror' in the URIs once Orion is gone. 

> @@ -184,5 @@
> > -   *        Optional, the end offset, zero based, where you want to stop
> > -   *        replacing text in the editor.
> > -   */
> > -  setText: function SP_setText(aText, aStart, aEnd)
> > -  {
> 
> removing setText? That seemed like a useful API for Scratchpads to have.

setText was changed. I removed start/end parameters in favor of separate methods replace/appendText. Name makes the intention clearer (it is not clear to me what setText("foo", 10, 12) does just from reading the function call). I also changed it to take { line, ch } objects instead of character counts. Tests become way easier to read when you use appendText("foo", { line: 1, ch: 10 }) instead of setText("foo", 10).

> 
> @@ +189,2 @@
> >  
> > +    if (!clean || !this._saved)
> 
> do you find this easier to understand than this.editor && this.editor.dirty ?
> 
> DeMorgan's rule is a harsh mistress.
> 
> I guess you need to change the logic to incorporate the !this._saved check.

I'm not particularly happy with this and will probably change it before submitting for a review. I needed to change that because I wanted to use CodeMirror's versioned history system and its different from simply setting a 'dirty' flag.

> 
> @@ +269,4 @@
> >      return this._sidebar;
> >    },
> >  
> > +  setText: function SP_setText(...args)
> 
> ok, you moved it.
> 
> what values can setText accept as paramaters?

See above.

> 
> @@ +1281,2 @@
> >  
> > +      this.editor.editor.on("change", this._onChanged);
> 
> editor.editor.editor... :)

Yeah, I'll change similar things before sending for a review.

> 
> ::: browser/devtools/scratchpad/scratchpad.xul
> @@ +356,5 @@
> > +    <menuitem id="se-cMenu-selectAll"
> > +          label="&selectAllCmd.label;"
> > +          key="key_selectAll"
> > +          accesskey="&selectAllCmd.accesskey;"
> > +          command="se-cmd-selectAll"/>
> 
> nice cleanups in this file.

Comment 12

[Anton Kovalyov (:anton)]

Attachment: WIP 13

I'm actually not sure if those failures were mine or due to the fact that I pushed to try on a closed tree. Pushed again, just for OS X this time, to test the waters: https://tbpl.mozilla.org/?tree=Try&rev=5340ca355820

This WIP is a small fix—forgot to remove a reference to goto.js from editor.js.

Comment 13

[Anton Kovalyov (:anton)]

Attachment: WIP 14

Changed the editor.js code to not to expose the CodeMirror instance. This way we can be sure that no external code is accessing CodeMirror directly which makes the overall architecture more stable.

Comment 14

[Anton Kovalyov (:anton)]

Attachment: WIP 15

The patch is ready for review! Rob, I went back to using 'dirty' but now its a property of the Scratchpad object and not the editor. Editor still uses CodeMirror's versioned system. The code is clearer now.

Keep in mind that there's one more thing that needs to be done: integrating CM's tests with our codebase. I'll be working on that while you're reviewing the rest of my changes.

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

Comment 15

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

awesome. Thanks for the explanations. I'm fine with leaving the /cm/ path in the uri for now as long as we're planning on removing it down the road.

to the review then!

Comment 16

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

Comment on attachment 803442 [details] [diff] [review]
WIP 15

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

this looks great to me. Couple questions below about the extra editor-commands overlay and how you're using it. I did only the most cursory review of the codemirror code (mostly I tried to ignore it).

editor.js contains a few clever things. maybe too clever.

I'm still getting errors when trying to load the Debugger on the startpage.

JavaScript error: chrome://browser/content/devtools/debugger-controller.js, line 21: NS_ERROR_FILE_NOT_FOUND: Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]
JavaScript error: chrome://browser/content/devtools/debugger-view.js, line 719: Heritage is not defined
JavaScript error: chrome://browser/content/devtools/debugger-toolbar.js, line 339: Heritage is not defined
JavaScript error: chrome://browser/content/devtools/debugger-panes.js, line 32: Heritage is not defined
JavaScript error: resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/debugger/debugger-panel.js, line 18: this._controller is undefined


the source-line from above is:
Cu.import("resource:///modules/source-editor.jsm");

Same goes for Style Editor.

So this patch breaks this. Not sure why or how the debugger unittests are passing.

::: browser/devtools/scratchpad/scratchpad.js
@@ +226,4 @@
>        filename: this.filename,
>        text: this.getText(),
>        executionContext: this.executionContext,
> +      saved: this.dirty === false

do you prefer this to just !this.dirty?

@@ +1389,3 @@
>        let ps = Services.prompt;
>        let flags = ps.BUTTON_POS_0 * ps.BUTTON_TITLE_SAVE +
>                    ps.BUTTON_POS_1 * ps.BUTTON_TITLE_CANCEL +

nice.

::: browser/devtools/sourceeditor/editor-commands.xul
@@ +1,2 @@
> +<?xml version="1.0"?>
> +

a little fuzzy on what this is being used for. afaict this isn't being included anywhere. You could use this to define your context menu in scratchpad.xul but don't.

::: browser/devtools/sourceeditor/editor.js
@@ +41,5 @@
> +  "      html, body { height: 100%; }" +
> +  "      body { margin: 0; overflow: hidden; }" +
> +  "      .CodeMirror { width: 100%; height: 100% !important; }" +
> +  "    </style>" +
> +[ "    <link rel='stylesheet' href='" + style + "'>" for each (style in CM_STYLES) ].join("\n") +

augh

@@ +83,5 @@
> + * of helper methods to make our use of CodeMirror easier and
> + * another method, appendTo, to actually create and append
> + * the CodeMirror instance.
> + *
> + * Note that Editor doesn't expose CodeMirror instane to the

typonit: "instance".

@@ +113,5 @@
> +    extraKeys:   {},
> +    indentWithTabs: useTabs,
> +  };
> +
> +  Object.keys(config).forEach((key) => this.config[key] = config[key]);

these are not Keyboard keys. They're object keys.

@@ +114,5 @@
> +    indentWithTabs: useTabs,
> +  };
> +
> +  Object.keys(config).forEach((key) => this.config[key] = config[key]);
> +  this.config.extraKeys[ctrl("J")] = (cm) => this.jumpToLine();

these are Keyboard keys.

I feel like you're trolling me.

Comment 17

[Mihai Sucan [:msucan]]

Comment on attachment 803442 [details] [diff] [review]
WIP 15

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

Glad to see this patch! Thanks for your work!

General comments:
- general editor usage seems fine (/me likes).
- for some reason the Edit menu items display no keyboard shortcuts associated, even if the actually work: I can press Ctrl-Z/Y/C/X/V/F/G for their respective actions.
- *select a few lines of text and press Tab/Shift-Tab. These should indent/unindent the selection. I found that Ctrl-[ and Ctrl-] do what the previous shortcuts did. Is this intentional?
- *select a few lines of text and press Ctrl-/. This should comment/uncomment the code.
- *no highlighting of matching parenthesis.
- *no keyboard shortcut to jump to the matching parenthesis - we have Ctrl-[ and Ctrl-] for that.
- I haven't tested on Macs, but are the default keyboard bindings working? Those Emacs-like native bindings that you can use with every textarea. [1] If these do not work, they might be an important regression for Mac users.
- Do you include a license file for codemirror? A README with version information? An UPGRADE file with info on how to upgrade codemirror? Any Mozila-specific patches applied to codemirror? Did you update the about:license page to include the codemirror license?
- Do you include Brian's work on accessibility?
- Is editor-commands.xul used at all? If not, do you plan to use it?

(* for things that I consider regressions, but you can open follow-up bug reports if you do not have time to fix these issues in these patches.) 

The context menu in Scratchpad doesn't show:

JavaScript error: resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/sourceeditor/editor.js, line 309: pos is not defined


I cannot open the Debugger:

[22:08:01.786] NS_ERROR_FILE_NOT_FOUND: Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import] @ chrome://browser/content/devtools/debugger-controller.js:21

I cannot open the Style Editor either:

[22:08:03.080] NS_ERROR_FILE_NOT_FOUND: Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import] @ resource:///modules/devtools/StyleSheetEditor.jsm:19

Canceling the review request - please address the issues raised. Also see further comments below. Thank you!

[1] http://www.hcs.harvard.edu/~jrus/Site/System%20Bindings.html

::: browser/devtools/jar.mn
@@ +30,5 @@
>      content/browser/devtools/fontinspector/font-inspector.css          (fontinspector/font-inspector.css)
>      content/browser/devtools/orion.js                                  (sourceeditor/orion/orion.js)
> +    content/browser/devtools/codemirror.js                             (sourceeditor/codemirror/codemirror.js)
> +    content/browser/devtools/codemirror.css                            (sourceeditor/codemirror/codemirror.css)
> +    content/browser/devtools/cm/javascript.js                          (sourceeditor/codemirror/javascript.js)

Why not put all of the codemirror files in the devtools/codemirror/ path? Why "cm/"?

::: browser/devtools/scratchpad/scratchpad.js
@@ +37,3 @@
>  const escodegen = require("escodegen/escodegen");
> +const Editor    = require("devtools/sourceeditor/editor");
> +const TargetFactory = require("devtools/framework/target").TargetFactory;

nit: not much point trying to align the = on a few lines only. no alignment is better than inconsistency.

@@ +195,4 @@
>    setFilename: function SP_setFilename(aFilename)
>    {
>      this.filename = aFilename;
> +    Scratchpad._updateTitle();

Is this change needed?

@@ +288,4 @@
>      return this._sidebar;
>    },
>  
> +  setText: function SP_setText(...args)

This method is missing a description.

::: browser/devtools/scratchpad/scratchpad.xul
@@ +180,5 @@
> +      <menuitem id="se-menu-undo"
> +          label="&undoCmd.label;"
> +          key="key_undo"
> +          accesskey="&undoCmd.accesskey;"
> +          command="se-cmd-undo"/>

You might to import the #editMenuKeys from the editMenuOverlay.xul. This is probably why the keyboard shortcuts are not showing for me.

You may also want to reuse/import the edit menu items themselves from editMenuOverlay.xul - so you do not need different strings in scratchpad dtd.

@@ +332,4 @@
>  <popupset id="scratchpad-popups">
>    <menupopup id="scratchpad-text-popup"
>               onpopupshowing="goUpdateSourceEditorMenuItems()">
> +    <menuitem id="se-cMenu-cut"

Ditto.

::: browser/devtools/sourceeditor/editor.js
@@ +41,5 @@
> +  "      html, body { height: 100%; }" +
> +  "      body { margin: 0; overflow: hidden; }" +
> +  "      .CodeMirror { width: 100%; height: 100% !important; }" +
> +  "    </style>" +
> +[ "    <link rel='stylesheet' href='" + style + "'>" for each (style in CM_STYLES) ].join("\n") +

nit: for (let style of CM_STYLES) ?

@@ +63,5 @@
> +  "openDialog"
> +];
> +
> +const CM_JUMP_DIALOG = [
> +  "Jump to line: <input type=text style='width: 10em'/>"

This needs localization.

@@ +83,5 @@
> + * of helper methods to make our use of CodeMirror easier and
> + * another method, appendTo, to actually create and append
> + * the CodeMirror instance.
> + *
> + * Note that Editor doesn't expose CodeMirror instane to the

typo: s/instane/instance/

@@ +140,5 @@
> +   * This method is asynchronous and returns a promise.
> +   */
> +  appendTo: function (el) {
> +    let def = promise.defer();
> +    let cm  = editors.get(this);

nit: I found this confusing. Maybe you should go for "gEditors" or something that points to the fact it's a global.

(not binding for the review, just noting what I found odd while reading...)

Comment 18

[Girish Sharma [:Optimizer]]

(In reply to Mihai Sucan [:msucan] from comment #17)
> - *no highlighting of matching parenthesis.

There is an addon that does this http://codemirror.net/addon/edit/matchbrackets.js

> - *no keyboard shortcut to jump to the matching parenthesis - we have Ctrl-[
> and Ctrl-] for that.

The shortcut is Ctrl + Alt + [/] now.

Comment 19

[Anton Kovalyov (:anton)]

(In reply to Mihai Sucan [:msucan] from comment #17)
> Comment on attachment 803442 [details] [diff] [review]
> WIP 15
> 
> - I haven't tested on Macs, but are the default keyboard bindings working?
> Those Emacs-like native bindings that you can use with every textarea. [1]
> If these do not work, they might be an important regression for Mac users.

Yep. CodeMirror detects the OS and picks the right modifier key (ctrl or cmd) based on that.

> - Do you include a license file for codemirror? A README with version
> information? An UPGRADE file with info on how to upgrade codemirror? Any
> Mozila-specific patches applied to codemirror? Did you update the
> about:license page to include the codemirror license?

Ugh no. Will do.

> - Do you include Brian's work on accessibility?

Nope, he will land his work separately. Given that Orion doesn't work with screen readers (as Brian confirmed) it's not a regression but a followup bug.

> - Is editor-commands.xul used at all? If not, do you plan to use it?

Nope, forgot to remove that file.

> 
> ::: browser/devtools/jar.mn
> @@ +30,5 @@
> >      content/browser/devtools/fontinspector/font-inspector.css          (fontinspector/font-inspector.css)
> >      content/browser/devtools/orion.js                                  (sourceeditor/orion/orion.js)
> > +    content/browser/devtools/codemirror.js                             (sourceeditor/codemirror/codemirror.js)
> > +    content/browser/devtools/codemirror.css                            (sourceeditor/codemirror/codemirror.css)
> > +    content/browser/devtools/cm/javascript.js                          (sourceeditor/codemirror/javascript.js)
> 
> Why not put all of the codemirror files in the devtools/codemirror/ path?
> Why "cm/"?

Less characters to type. In any case, once Orion is gone I'll remove the /cm/ or /codemirror/ part of the path entirely.

> 
> @@ +195,4 @@
> >    setFilename: function SP_setFilename(aFilename)
> >    {
> >      this.filename = aFilename;
> > +    Scratchpad._updateTitle();
> 
> Is this change needed?

I think a test was breaking without this change. I'll re-check.

I'll address other comments in a followup WIPs.

Comment 20

[Anton Kovalyov (:anton)]

Attachment: WIP 16

This patch mostly addresses breakage and comments about redundant files. Other edits will come in followup WIPs.

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

Comment 21

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Anton Kovalyov (:anton) from comment #19)
> (In reply to Mihai Sucan [:msucan] from comment #17)
> > Comment on attachment 803442 [details] [diff] [review]
> > WIP 15
> > 
> > - I haven't tested on Macs, but are the default keyboard bindings working?
> > Those Emacs-like native bindings that you can use with every textarea. [1]
> > If these do not work, they might be an important regression for Mac users.
> 
> Yep. CodeMirror detects the OS and picks the right modifier key (ctrl or
> cmd) based on that.

This isn't about cmd vs. ctrl, its about how OSX provides emacs-style keybindings in all text inputs.

For example, Ctrl-A goes to the start of the line, Ctrl-E goes to the end, Ctrl-D deletes one character forward, Ctrl-F moves cursor forwards one character, Ctrl-B moves cursor backwards one character.

Orion doesn't break these bindings, does our code mirror integration break the bindings? It definitely shouldn't. I rely on them heavily, as do most other OSX && emacs users, which is a not an insignificant portion of developers.

Comment 22

[Anton Kovalyov (:anton)]

(In reply to Nick Fitzgerald [:fitzgen] from comment #21)
> Orion doesn't break these bindings, does our code mirror integration break
> the bindings? It definitely shouldn't. I rely on them heavily, as do most
> other OSX && emacs users, which is a not an insignificant portion of
> developers.

I just tested and they all seem to work. That said, if there's any other OS specific stuff we missed they'll be filed as follow-ups and fixed during the next Nightly cycle (since I'll be merging this in the beginning of the cycle we'll have lots of time).

Comment 23

[Mihai Sucan [:msucan]]

(In reply to Anton Kovalyov (:anton) from comment #19)
> (In reply to Mihai Sucan [:msucan] from comment #17)
> > Comment on attachment 803442 [details] [diff] [review]
> > WIP 15
> > 
> > - I haven't tested on Macs, but are the default keyboard bindings working?
> > Those Emacs-like native bindings that you can use with every textarea. [1]
> > If these do not work, they might be an important regression for Mac users.
> 
> Yep. CodeMirror detects the OS and picks the right modifier key (ctrl or
> cmd) based on that.

Nick's comment points to what I meant.


> > - Do you include a license file for codemirror? A README with version
> > information? An UPGRADE file with info on how to upgrade codemirror? Any
> > Mozila-specific patches applied to codemirror? Did you update the
> > about:license page to include the codemirror license?
> 
> Ugh no. Will do.

Thanks!

> > - Do you include Brian's work on accessibility?
> 
> Nope, he will land his work separately. Given that Orion doesn't work with
> screen readers (as Brian confirmed) it's not a regression but a followup bug.

Cool. Do you have a bug number?


> > ::: browser/devtools/jar.mn
> > @@ +30,5 @@
> > >      content/browser/devtools/fontinspector/font-inspector.css          (fontinspector/font-inspector.css)
> > >      content/browser/devtools/orion.js                                  (sourceeditor/orion/orion.js)
> > > +    content/browser/devtools/codemirror.js                             (sourceeditor/codemirror/codemirror.js)
> > > +    content/browser/devtools/codemirror.css                            (sourceeditor/codemirror/codemirror.css)
> > > +    content/browser/devtools/cm/javascript.js                          (sourceeditor/codemirror/javascript.js)
> > 
> > Why not put all of the codemirror files in the devtools/codemirror/ path?
> > Why "cm/"?
> 
> Less characters to type. In any case, once Orion is gone I'll remove the
> /cm/ or /codemirror/ part of the path entirely.

browser/devtools/codemirror/foo makes more sense rather than putting all of the codemirror files directly in browser/devtools. I'd like the virtual chrome path to be as similar as possible to the path on the disk - to avoid unneeded confusion. Is there a reason not to do this change now?


> > @@ +195,4 @@
> > >    setFilename: function SP_setFilename(aFilename)
> > >    {
> > >      this.filename = aFilename;
> > > +    Scratchpad._updateTitle();
> > 
> > Is this change needed?
> 
> I think a test was breaking without this change. I'll re-check.

If that's the case, there's a bug that needs a different fix.

> I'll address other comments in a followup WIPs.

Thank you!

Comment 24

[Anton Kovalyov (:anton)]

Attachment: WIP 17

56ee9c3 Localize jump to line
8e9aeed Added description to setText and changed for each to for (of)
2bed845 Reused menu items from editMenuOverlay.xul
32b234f Import editMenuKeys instead of sourceEditorKeys

Should localization of a search plugin be a followup?

Comment 25

[Anton Kovalyov (:anton)]

Attachment: WIP18

a79304a Add licensing information
c24179e Comment/uncomment selected lines on ctrl-/
d602f0b Highlight matching parens

Okay, I implemented most comments. One thing though:

Ctrl-[ and Ctrl-] toggles indentation in most editor I know. I propose to not to re-implement the jumping to matching parens feature, at least not with these shortcuts. I, for one, use Ctrl-[] to indent code all the time. I also think we should abandon Tab/Shift-Tab in favor of one shortcut per feature. That said, if people *really* want this, implementing Tab/Shift-Tab is trivial.

Also, as I said in previous WIPs search and jump-to-line features will need some UX and localization love but we can do that in followup bugs.

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

Comment 26

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Anton Kovalyov (:anton) from comment #25)
> Ctrl-[ and Ctrl-] toggles indentation in most editor I know. I propose to
> not to re-implement the jumping to matching parens feature, at least not
> with these shortcuts. I, for one, use Ctrl-[] to indent code all the time. I
> also think we should abandon Tab/Shift-Tab in favor of one shortcut per
> feature. That said, if people *really* want this, implementing Tab/Shift-Tab
> is trivial.

I'll add my vote in support of Tab / Shift-Tab...  I would have assumed that is more familiar than Ctrl-[], but at the same time I've never used an editor where Ctrl-[] did anything, so it's not part of my keystroke memory.

Comment 27

[Girish Sharma [:Optimizer]]

(In reply to Anton Kovalyov (:anton) from comment #25)
> Ctrl-[ and Ctrl-] toggles indentation in most editor I know. I propose to
> not to re-implement the jumping to matching parens feature, at least not
> with these shortcuts. I, for one, use Ctrl-[] to indent code all the time. I
> also think we should abandon Tab/Shift-Tab in favor of one shortcut per
> feature. That said, if people *really* want this, implementing Tab/Shift-Tab
> is trivial.

I don't know why my comment was ignored previously, but .. Ctrl + [ / ] do not do anything now in scratchpad (or in general , source editor) because they were conflicting with the toolbox tab switch shortcut. The new shortcut for matching paren jumping is Ctrl + Alt + [ / ]

Comment 28

[Anton Kovalyov (:anton)]

Attachment: codemirror-scratchpad.patch

7434879 Disable ctrl-[ and ctrl-] and add support for tab/shift-tab

Not implemented jumping to parens shortcut because I don't know if anyone uses it. Cmd-Alt-[ and Cmd-Alt-] are super awkward on Macs.

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

Comment 29

[Anton Kovalyov (:anton)]

Attachment: WIP 20

Added CodeMirror tests to our test suite (see browser_codemirror.js, codemirror.html and cm_* files in the sourceeditor/test directory). It was surprisingly easy to do. Also update README and license files to reflect these changes.

Comment 30

[Anton Kovalyov (:anton)]

New Try (with CodeMirror tests): https://tbpl.mozilla.org/?tree=Try&rev=d6b547b435cc

Comment 31

[Anton Kovalyov (:anton)]

There seems to be a test failure on Windows and Linux platforms, I'm installing Ubuntu to investigate it. Shouldn't stop reviewers from reviewing though!

Comment 32

[Anton Kovalyov (:anton)]

Attachment: WIP 21

Removed forgotten console.log.

Comment 33

[Anton Kovalyov (:anton)]

Okay, I think I know where the Linux/Windows single test failure is coming from. Will fix in the morning, once I get back to the office.

Comment 34

[Anton Kovalyov (:anton)]

Attachment: WIP 22

Okay, fixed Linux (and hopefully Windows) failures. Try: https://tbpl.mozilla.org/?tree=Try&rev=15998ea3d6cd

Comment 35

[Mihai Sucan [:msucan]]

Comment on attachment 806437 [details] [diff] [review]
WIP 21

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

General comments:

- nit: I cannot select whole lines using the gutter by shift-clicking/double-clicking. It seems to be inert.
- Really glad to see the codemirror tests in our codebase. Maybe you should put the codemirror test files in a subfolder. Also, how do you report if any errors happen in that page? Does the mochitest extract meaningful info from the page? Such that we can see the failures in the logs.
- in sourceeditor/editor.js you have a mix between var and let, and I would prefer jsdoc comments for all the methods - |@param type varName description|, and |@return type description|. These help with reading the signature of functions. Prose is harder to skim-through.

Thanks for all the updates. This is looking good, r+. Please address any of these comments as you see fit.

::: browser/devtools/scratchpad/scratchpad.js
@@ +182,4 @@
>     */
>    getText: function SP_getText(aStart, aEnd)
>    {
> +    var value = this.editor.getText();

nit: var versus let consistency through the patch.

@@ -198,4 @@
>    setFilename: function SP_setFilename(aFilename)
>    {
>      this.filename = aFilename;
> -    this._updateTitle();

if setFilename() is called you do not need to update the scratchpad window title anymore?

Comment 36

[Mihai Sucan [:msucan]]

Comment on attachment 806437 [details] [diff] [review]
WIP 21

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

r+ for the patch
(mid-air collision)

::: browser/devtools/scratchpad/scratchpad.js
@@ +182,4 @@
>     */
>    getText: function SP_getText(aStart, aEnd)
>    {
> +    var value = this.editor.getText();

nit: var versus let consistency through the patch.

@@ -198,4 @@
>    setFilename: function SP_setFilename(aFilename)
>    {
>      this.filename = aFilename;
> -    this._updateTitle();

if setFilename() is called you do not need to update the scratchpad window title anymore?

Comment 37

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

Comment on attachment 806437 [details] [diff] [review]
WIP 21

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

looks good overall.

Sadly, devtools do not open with this patch applied.

JavaScript error: resource://gre/modules/devtools/Loader.jsm, line 86: NS_ERROR_FILE_UNRECOGNIZED_PATH: Component returned failure code: 0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH) [nsILocalFile.initWithPath]
JavaScript error: resource://gre/modules/devtools/Loader.jsm, line 86: NS_ERROR_FILE_UNRECOGNIZED_PATH: Component returned failure code: 0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH) [nsILocalFile.initWithPath]
JavaScript error: resource://gre/modules/devtools/Loader.jsm, line 86: NS_ERROR_FILE_UNRECOGNIZED_PATH: Component returned failure code: 0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH) [nsILocalFile.initWithPath]

::: browser/devtools/scratchpad/scratchpad.js
@@ +15,5 @@
>  "use strict";
>  
> +const Cu = Components.utils;
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;

still no like destructuring?

::: browser/devtools/scratchpad/scratchpad.xul
@@ +3,4 @@
>  <!-- This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +

by removing these you can remove the preprocessing directive in devtools/scratchpad's jar.mn file.

::: browser/devtools/scratchpad/test/browser_scratchpad_bug650345_find_ui.js
@@ -92,5 @@
> -
> -  Services.prompt = oldPrompt;
> -
> -  finish();
> -}

bye bye!

::: browser/devtools/sourceeditor/test/Makefile.in
@@ +28,2 @@
>  		head.js \
> +		codemirror.html \

does this actually work?

Do we get all these tests running that easily?

I'm guessing not since they don't have the browser_ prefix.

If you just need these to be included in your test directory, you would normally put them in the MOCHITEST_BROWSER_PAGES section of the Makefile.

::: browser/devtools/sourceeditor/test/cm_comment_test.js
@@ +33,5 @@
> +  // }, "html {\n  border: none;\n}", "/* html {\n  border: none;\n} */");
> +
> +  // test("fallbackToLine", "ruby", function(cm) {
> +  //   cm.blockComment(Pos(0, 0), Pos(1));
> +  // }, "def blah()\n  return hah\n", "# def blah()\n#   return hah\n");

do these commented tests eventually need to be uncommented? Maybe a TODO or do we not care about ruby?

::: browser/devtools/sourceeditor/test/codemirror.html
@@ +70,5 @@
> +    <script src="../mode/xml/xml.js"></script>
> +    <script src="../mode/htmlmixed/htmlmixed.js"></script>
> +    <script src="../mode/ruby/ruby.js"></script>
> +    <script src="../mode/haml/haml.js"></script>
> +    <script src="../mode/haml/test.js"></script>

we're not testing haml??

Comment 38

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

I see there was another patch uploaded. I'll try it out later.

Comment 39

[Anton Kovalyov (:anton)]

Thanks for the review, Mihai!

(In reply to Mihai Sucan [:msucan] from comment #35)
> Comment on attachment 806437 [details] [diff] [review]
> WIP 21
> 
> Review of attachment 806437 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> General comments:
> 
> - nit: I cannot select whole lines using the gutter by
> shift-clicking/double-clicking. It seems to be inert.

I didn't even know we had this functionality! I'll do it as a followup in the same time as Debugger/CodeMirror patch.

> - Really glad to see the codemirror tests in our codebase. Maybe you should
> put the codemirror test files in a subfolder. Also, how do you report if any
> errors happen in that page? Does the mochitest extract meaningful info from
> the page? Such that we can see the failures in the logs.

I tried but couldn't make Makefile.in recognize files inside the directory. :-\ As for, extracting the info, right now it only reports where all CM tests passed or any of them failed. I'll look into making more granular reporting.

> - in sourceeditor/editor.js you have a mix between var and let, and I would
> prefer jsdoc comments for all the methods - |@param type varName
> description|, and |@return type description|. These help with reading the
> signature of functions. Prose is harder to skim-through.

I'll add them for more complex functions. Things like `setText(value)` are self-explanatory I think.

> 
> @@ -198,4 @@
> >    setFilename: function SP_setFilename(aFilename)
> >    {
> >      this.filename = aFilename;
> > -    this._updateTitle();
> 
> if setFilename() is called you do not need to update the scratchpad window
> title anymore?

Oops, will fix.

(In reply to Rob Campbell [:rc] (:robcee) from comment #37)
> Comment on attachment 806437 [details] [diff] [review]
> WIP 21
> 
> Review of attachment 806437 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> looks good overall.
> 
> Sadly, devtools do not open with this patch applied.
> 
> JavaScript error: resource://gre/modules/devtools/Loader.jsm, line 86:
> NS_ERROR_FILE_UNRECOGNIZED_PATH: Component returned failure code: 0x80520001
> (NS_ERROR_FILE_UNRECOGNIZED_PATH) [nsILocalFile.initWithPath]
> JavaScript error: resource://gre/modules/devtools/Loader.jsm, line 86:
> NS_ERROR_FILE_UNRECOGNIZED_PATH: Component returned failure code: 0x80520001
> (NS_ERROR_FILE_UNRECOGNIZED_PATH) [nsILocalFile.initWithPath]
> JavaScript error: resource://gre/modules/devtools/Loader.jsm, line 86:
> NS_ERROR_FILE_UNRECOGNIZED_PATH: Component returned failure code: 0x80520001
> (NS_ERROR_FILE_UNRECOGNIZED_PATH) [nsILocalFile.initWithPath]

I just did a new build with freshly rebased fx-team and it worked fine on Mac. Other platforms worked fine on Try servers. Can you pinpoint which path is not being recognized?

> ::: browser/devtools/scratchpad/scratchpad.js
> @@ +15,5 @@
> >  "use strict";
> >  
> > +const Cu = Components.utils;
> > +const Cc = Components.classes;
> > +const Ci = Components.interfaces;
> 
> still no like destructuring?

How will destructuring work here? Shouldn't left-hand variable and right-hand object properties have same names?

> ::: browser/devtools/scratchpad/scratchpad.xul
> @@ +3,4 @@
> >  <!-- This Source Code Form is subject to the terms of the Mozilla Public
> >     - License, v. 2.0. If a copy of the MPL was not distributed with this
> >     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> > +
> 
> by removing these you can remove the preprocessing directive in
> devtools/scratchpad's jar.mn file.

OK.

> 
> ::: browser/devtools/scratchpad/test/browser_scratchpad_bug650345_find_ui.js
> @@ -92,5 @@
> > -
> > -  Services.prompt = oldPrompt;
> > -
> > -  finish();
> > -}
> 
> bye bye!
> 
> ::: browser/devtools/sourceeditor/test/Makefile.in
> @@ +28,2 @@
> >  		head.js \
> > +		codemirror.html \
> 
> does this actually work?
> 
> Do we get all these tests running that easily?
> 
> I'm guessing not since they don't have the browser_ prefix.
> 
> If you just need these to be included in your test directory, you would
> normally put them in the MOCHITEST_BROWSER_PAGES section of the Makefile.

Test browser_codemirror.js opens codemirror.html and run CodeMirror tests as part of mochitests. So yeah we got all these tests running that easily. Mostly because CM doesn't use any external framework.

> ::: browser/devtools/sourceeditor/test/cm_comment_test.js
> @@ +33,5 @@
> > +  // }, "html {\n  border: none;\n}", "/* html {\n  border: none;\n} */");
> > +
> > +  // test("fallbackToLine", "ruby", function(cm) {
> > +  //   cm.blockComment(Pos(0, 0), Pos(1));
> > +  // }, "def blah()\n  return hah\n", "# def blah()\n#   return hah\n");
> 
> do these commented tests eventually need to be uncommented? Maybe a TODO or
> do we not care about ruby?

CSS and HTML modes—yes, once I start working on integrating CodeMirror with other tools. Languages like Ruby—no.

> ::: browser/devtools/sourceeditor/test/codemirror.html
> @@ +70,5 @@
> > +    <script src="../mode/xml/xml.js"></script>
> > +    <script src="../mode/htmlmixed/htmlmixed.js"></script>
> > +    <script src="../mode/ruby/ruby.js"></script>
> > +    <script src="../mode/haml/haml.js"></script>
> > +    <script src="../mode/haml/test.js"></script>
> 
> we're not testing haml??

>_<

Comment 40

[Anton Kovalyov (:anton)]

Attachment: WIP 23

Let's see if this version fixes mochitest failure. Try (mochitests only): https://tbpl.mozilla.org/?tree=Try&rev=dce744d75042

Comment 41

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

Damn, fresh tree, new build environment (I updated all the pythons!) and I'm still getting this loader.jsm error.

JavaScript error: resource://gre/modules/devtools/Loader.jsm, line 87: NS_ERROR_FILE_UNRECOGNIZED_PATH: Component returned failure code: 0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH) [nsILocalFile.initWithPath]

Going to dig into what's happening there. Will report back soon.

Comment 42

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

Comment on attachment 807442 [details] [diff] [review]
WIP 23

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

it's fine. You did alright.

Comment 43

[Anton Kovalyov (:anton)]

Attachment: WIP 24

Let's try to land this thing.

Comment 44

[Anton Kovalyov (:anton)]

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

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8db3d638ccd1

Comment 46

[Gervase Markham [:gerv]]

Hi,

For future reference, please run all imports of 3rd party libraries past the licensing team ([e-mail] or file a mozilla.org::Licensing bug).

Also, the entries in about:licence are in alphabetical order (see bug 923157) - but you shouldn't be adding to it without my review.

Thanks :-)

Gerv

Comment 47

[Anton Kovalyov (:anton)]

Thanks, will do. Is there a wiki page describing the process? We plan to ship more third-party libraries in the near future and I'd like to put this on our next meeting's agenda so that we're all on the same page.

Comment 48

[Gervase Markham [:gerv]]

anton: http://www.mozilla.org/MPL/license-policy.html#Licensing_of_Third_Party_Code ?

Gerv