929766 - Remove Orion from the SourceEditor component

Status: RESOLVED FIXED

(DevTools :: Source Editor, defect, P2)

Description

[Anton Kovalyov (:anton)]

Once all other components are converted we need to write some more tests for our CodeMirror integration and remove Orion and all Orion specific tests from the tree.

Comment 1

[Anton Kovalyov (:anton)]

Attachment: WIP 1

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

Took me longer than expected because of the command dispatcher bug (see bug 933015).

Comment 2

[Anton Kovalyov (:anton)]

Attachment: WIP 2

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

Comment 3

[Anton Kovalyov (:anton)]

Attachment: WIP 3

Removed all traces of Orion (except for maybe about:license). Need to write tests for the Editor object now.

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

Comment 4

[Anton Kovalyov (:anton)]

Attachment: WIP 4

Started adding tests. Try: https://tbpl.mozilla.org/?tree=Try&rev=bdb0c0befe87

Comment 5

[Anton Kovalyov (:anton)]

Attachment: WIP 5

Added tests, removed Orion. There are three untested methods, they are all associated with line alignment so I'll file a bug for Optimizer to write tests for them after this lands. (Since he implemented those methods)

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

Comment 6

[Mihai Sucan [:msucan]]

Comment on attachment 831833 [details] [diff] [review]
WIP 5

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

This is looking good. Thanks for the patch! r+

I only have one concern: need moar tests. We had source editor tests for cut/copy/paste, middle-click paste, drag and drop, and more. I'm sure the codemirror tests themselves include a lot of API testing - that's good, but the ones I mentioned are not testable from web-based code. For example, mochitests for drag and drop in our editor integration would be useful.

::: browser/devtools/scratchpad/test/browser_scratchpad_bug_699130_edit_ui_updates.js
@@ +5,3 @@
>  "use strict";
>  
>  let tempScope = {};

Do you need to keep tempScope here?

::: browser/devtools/sourceeditor/test/browser.ini
@@ +16,2 @@
>  [browser_codemirror.js]
>  skip-if = os == "linux"

Any reason why the codemirror tests are disabled on linux? Is there a bug open for this problem?

::: browser/devtools/sourceeditor/test/head.js
@@ +35,3 @@
>  }
>  
> +function ch(exp, act, label) {

What does ch stand for?

When I first saw this function being used in a test I was confusd and thought it's a new function for mochitests -- is, ise, isnot, ok, ch, ...

Please rename this function to something less cryptic.

Comment 7

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

Comment on attachment 831833 [details] [diff] [review]
WIP 5

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

can we remove devtools.editor.component from firefox.js too?

Comment 8

[Anton Kovalyov (:anton)]

Thanks!

(In reply to Mihai Sucan [:msucan] from comment #6)
> Comment on attachment 831833 [details] [diff] [review]
> WIP 5
> 
> Review of attachment 831833 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is looking good. Thanks for the patch! r+
> 
> I only have one concern: need moar tests. We had source editor tests for
> cut/copy/paste, middle-click paste, drag and drop, and more. I'm sure the
> codemirror tests themselves include a lot of API testing - that's good, but
> the ones I mentioned are not testable from web-based code. For example,
> mochitests for drag and drop in our editor integration would be useful.

I agree. I'll be filing bugs and writing more tests. Some things (context menu, for example) are tested from other components.

> :::
> browser/devtools/scratchpad/test/
> browser_scratchpad_bug_699130_edit_ui_updates.js
> @@ +5,3 @@
> >  "use strict";
> >  
> >  let tempScope = {};
> 
> Do you need to keep tempScope here?

You're right, I do not.

> ::: browser/devtools/sourceeditor/test/browser.ini
> @@ +16,2 @@
> >  [browser_codemirror.js]
> >  skip-if = os == "linux"
> 
> Any reason why the codemirror tests are disabled on linux? Is there a bug
> open for this problem?

Yeah, there's an intermittent that happens only on Linux. Bug 920570.

> ::: browser/devtools/sourceeditor/test/head.js
> @@ +35,3 @@
> >  }
> >  
> > +function ch(exp, act, label) {
> 
> What does ch stand for?
> 
> When I first saw this function being used in a test I was confused and
> thought it's a new function for mochitests -- is, ise, isnot, ok, ch, ...

That's the idea! It's like 'is', 'isnot', etc. but for CodeMirror's character objects. It's in head.js because it doesn't make sense to make it any more generic.

(In reply to Rob Campbell [:rc] (:robcee) from comment #7)
> Comment on attachment 831833 [details] [diff] [review]
> WIP 5
> 
> Review of attachment 831833 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> can we remove devtools.editor.component from firefox.js too?

Good catch.

Comment 9

[Anton Kovalyov (:anton)]

Attachment: WIP 6

Comment 10

[Anton Kovalyov (:anton)]

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

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in  https://hg.mozilla.org/integration/fx-team/rev/d1bc3673a721 for a browser-chrome test failure:
https://tbpl.mozilla.org/php/getParsedLog.php?id=30727358&tree=Fx-Team

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Though these are also probably that failure and more: https://tbpl.mozilla.org/php/getParsedLog.php?id=30727259&tree=Fx-Team

Comment 13

[Anton Kovalyov (:anton)]

Fixed all test failures and verified by running all browser/devtools mochitests locally. Fingers crossed.

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

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/464f36a51d3e