So once CodeMirror 4.13 is out it'd be nice to upgrade to it since I've added support for CSS variables (which currently break parsing slightly), and fixed @supports and @-moz-document blocks so they highlight properly (bug 977665) I have a rough python script I'm attaching that handles most of the legwork for me (actually the first time I've written any python, so I apologise in advance. I think adding error handling to try and make using it friendlier made the code rather less friendly). I figure it might be nice to have on record/possibly reuseable in future. Currently the main issue is changes introduced in 4.7 break a number of tests TEST-UNEXPECTED-FAIL | Tern is (currently) defined on the window https://dxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/test/browser_editor_autocomplete_basic.js#31 #36, #51 https://dxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/test/browser_editor_autocomplete_js.js#26 TEST-UNEXPECTED-FAIL | (gutters|folderGutters) is correct https://dxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/test/browser_editor_prefs.js#63 #81, #92 I think there are also some failures of the CodeMirrors tests that I can/have fixed by updating imports, but I can worry about that more once these main issues are fixed.

> Currently the main issue is changes introduced in 4.7 break a number of tests > TEST-UNEXPECTED-FAIL | Tern is (currently) defined on the window > https://dxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/ > test/browser_editor_autocomplete_basic.js#31 > #36, #51 > https://dxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/ > test/browser_editor_autocomplete_js.js#26 > > TEST-UNEXPECTED-FAIL | (gutters|folderGutters) is correct > https://dxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/ > test/browser_editor_prefs.js#63 > #81, #92 > Just to narrow down the issue to see if it is test-only or if functionality is broken after the upgrade: 1) Does autocompletion work for JS (in scratchpad) with the new version applied? The easiest way to check is to press ctrl+space and see if you see a listing show up of normal window variables. 2) Do you see gutters for code folding in the style editor? If either of these aren't working, are you seeing any errors in the browser console when the editor opens up?

(In reply to Brian Grinstead [:bgrins] from comment #1) > Just to narrow down the issue to see if it is test-only or if functionality > is broken after the upgrade: > 1) Does autocompletion work for JS (in scratchpad) with the new version > applied? The easiest way to check is to press ctrl+space and see if you see > a listing show up of normal window variables. > 2) Do you see gutters for code folding in the style editor? > > If either of these aren't working, are you seeing any errors in the browser > console when the editor opens up? Well what do you know, both of those are working fine. And not an error in the console in sight. That was with 4.7, since that's the upgrade that was at the top of my patch queue, but I'll double check master works okay as well... yep, exactly the same results.

(In reply to Ian Moody [:Kwan] from comment #2) > Created attachment 8564380 [details] > workingOk.png > > (In reply to Brian Grinstead [:bgrins] from comment #1) > > Just to narrow down the issue to see if it is test-only or if functionality > > is broken after the upgrade: > > 1) Does autocompletion work for JS (in scratchpad) with the new version > > applied? The easiest way to check is to press ctrl+space and see if you see > > a listing show up of normal window variables. > > 2) Do you see gutters for code folding in the style editor? > > > > If either of these aren't working, are you seeing any errors in the browser > > console when the editor opens up? > Well what do you know, both of those are working fine. And not an error in > the console in sight. That was with 4.7, since that's the upgrade that was > at the top of my patch queue, but I'll double check master works okay as > well... yep, exactly the same results. So the functionality is working but the tests aren't? Or are the tests also working now with the latest patch?

(In reply to Brian Grinstead [:bgrins] from comment #3) > So the functionality is working but the tests aren't? Correct.

I assume we'll want to update to 5.0 rather than 4.13, since 5 seems to have the changes you want for bug 1088804. I cannot for the life of me figure out why browser_editor_autocomplete_js.js is failing. Though I have noted that even hopping into the test editor window and manually hitting ctrl+space does nothing, whereas it does in Scratchpad, so maybe the test editor just isn't being setup properly? And what should be done about the win.tern tests? Just toggling them to !win.tern is obviously enough to get them to pass, but then they seem a bit pointless. Manually checked still working: Scratchpad autocompletion using ctrl+space Style Editor autocompletion with auto-popup + enter Note to self: vim tests needs Pos = CodeMirror.Pos setting, and probably import adjustments, to pass.

I've also tested cold folding in both the Style Editor and Scratchpad, and it works in both places. Things of note: Pre-upgrade: The in-tree codemirror/README claims that "In cm_comment_test.js [...] fallbackToBlock and fallbackToLine tests" must be commented out. This is not currently the case. Why don't they fail then? Because this line https://hg.mozilla.org/mozilla-central/file/0cefb584fd1a/browser/devtools/sourceeditor/test/codemirror.html#l65 means they never run, nor do any of the other tests before the 2nd driver.js import. Removing that import, they now do run, and need to be commented out. In addition, running all tests by removing the 2nd driver import reveals that doc_docKeepsMode also currently fails (since it relies on the markdown mode existing) and must be commented out. cm_mode_javascript_test.js never runs because it isn't in test/browser.ini CSS and XML modes tests are currently in the "editor doesn't use these modes" list, even though it does. Post-upgrade: core_collapsedRangeCoordsChar, core_wrappingAndResizing, core_moveVstuck all fail, I don't know why, and need to be commented out.

(In reply to Ian Moody [:Kwan] from comment #5) > I assume we'll want to update to 5.0 rather than 4.13, since 5 seems to have > the changes you want for bug 1088804. I wouldn't want to do the work to upgrade to 4.13 if we are going to very quickly switch to 5, so let's check about the relative stability of 5.0. Marijn, how do you feel about Dev Tools upgrading to the mobile edition, in terms of stability and partformance? I notice it's listed at 'experimental' in the release notes.

> I cannot for the life of me figure out why browser_editor_autocomplete_js.js > is failing. Though I have noted that even hopping into the test editor > window and manually hitting ctrl+space does nothing, whereas it does in > Scratchpad, so maybe the test editor just isn't being setup properly? > > And what should be done about the win.tern tests? Just toggling them to > !win.tern is obviously enough to get them to pass, but then they seem a bit > pointless. Can you attach a patch with your current work? I will see if there is anything sticking out. Feel free to re-enable any tests that you mention in Comment 6, but please don't comment out any failing ones yet.

copies the new files over

Perform the Mozilla-specific patching, as mentioned in the README. Includes the previous missing disabling of the fallback tests, and disabling the docKeepsMode test.

> Marijn, how do you feel about Dev Tools upgrading to the mobile edition, in terms of stability and partformance? I notice it's listed at 'experimental' in the release notes. The old textarea-based input implementation should be stable, so as long as you are on a desktop browser and don't explicitly enable the contenteditable-based one, you are unlikely to run into any problems. However, since I get the impression screen reader support is important for Mozilla, you might want to consider turning on the contenteditable implementation, to test it and help me get it to a point where we can consider it stable. That's a call you have to make.

Alright, let's proceed by upgrading to 5.0 without turning on contentEditable in this bug, and then do further testing to decide whether we should enable that in Bug 1088804

Comment on attachment 8568896 [details] [diff] [review] Part 2 - Apply Mozilla customisations Review of attachment 8568896 [details] [diff] [review]: ----------------------------------------------------------------- > Perform the Mozilla-specific patching, as mentioned in the README. Includes the previous missing disabling of the fallback tests, and disabling the docKeepsMode test. Can you please update the README with any relevant info?

Try push with the patch queue: https://treeherder.mozilla.org/#/jobs?repo=try&revision=87f3d993d310

Please, mark this bug as blocking for bug 1176964

Gabriel, all of these patches are going to need to be rebased on top of Bug 912121 (assuming that sticks on fx-team), sorry :(. I can still do the reviews in the meantime. I think you could find/replace on the patch files to do something like: browser/devtools/ -> devtools/client/ toolkit/devtools/server -> devtools/server toolkit/devtools -> devtools/shared browser/themes/shared/devtools -> devtools/client/themes And that should get you most of the way. Also see https://github.com/jryans/devtools-migrate#updating-your-patches

I made a simple script that can migrate the file paths on a patch file: https://gist.github.com/bgrins/8be6ad65a2fd41602f18

From what I understand we are going to land 1206425 before this. Is that right Gabriel? I'm going to clear reviews for now since these need rebasing anyway, and the patches will change quite a bit after the reorg. We may also want to bump to 5.7 (up to you if you want to do that now or in another bug)

Comment on attachment 8665887 [details] [diff] [review] Part 12: Stylesheet Editor should always batch up updates [1.0] Review of attachment 8665887 [details] [diff] [review]: ----------------------------------------------------------------- Tom, just a heads up that we are planning to remove the 'immediate' functionality inside updateStyleSheet on the frontend. We couldn't find anywhere this was intentionally being called, and the event object was being passed in as the first parameter so the truthy check on line 482 was incorrectly being hit. Seem alright with you?

Gabe, a note about landing this: let's fold all the upgrade stuff into one patch. Believe that's Parts 1, 2, 4, 5, 6, 7, and 13. Then the various fixes needed for the upgrade could fold into a second patch - 3, 8, 9, 10, 11, and 12.

Comment on attachment 8665878 [details] [diff] [review] Part 8: Editor config option should be updated [1.0] Review of attachment 8665878 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/sourceeditor/editor.js @@ +1023,5 @@ > this.config.autocomplete = v; > this.setupAutoCompletion(); > } else { > cm.setOption(o, v); > + this.config[o] = v; I can't remember why we needed to do this. Do you? It does make sense that we'd keep the config state up to date though (as we do with config.autocomplete above)

ni? for Comment 63

Comment on attachment 8665887 [details] [diff] [review] Part 12: Stylesheet Editor should always batch up updates [1.0] Review of attachment 8665887 [details] [diff] [review]: ----------------------------------------------------------------- Looks good; and fwiw won't cause any problems with the authored series.

Comment on attachment 8665879 [details] [diff] [review] Part 9: Editor should emit a cursorActivity on setCursor [1.0] Review of attachment 8665879 [details] [diff] [review]: ----------------------------------------------------------------- Ship it!

(In reply to Brian Grinstead [:bgrins] from comment #63) > Comment on attachment 8665878 [details] [diff] [review] > Part 8: Editor config option should be updated [1.0] > > Review of attachment 8665878 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: devtools/client/sourceeditor/editor.js > @@ +1023,5 @@ > > this.config.autocomplete = v; > > this.setupAutoCompletion(); > > } else { > > cm.setOption(o, v); > > + this.config[o] = v; > > I can't remember why we needed to do this. Do you? It does make sense that > we'd keep the config state up to date though (as we do with > config.autocomplete above) I believe this was because the autocomplete test was failing since the config is not actually saved.

https://hg.mozilla.org/integration/fx-team/rev/0390e740bfc55c2e99cfed3654c1f888e5da95e0 Bug 1132557 - Part 1: Upgrade to CodeMirror 5.7.0 r=bgrins https://hg.mozilla.org/integration/fx-team/rev/2bfe5435c1d1e2784af3b225d629a210023daecd Bug 1132557 - Part 2: Apply fixes for CodeMirror 5.7.0 upgrade r=bgrins

https://hg.mozilla.org/mozilla-central/rev/0390e740bfc5 https://hg.mozilla.org/mozilla-central/rev/2bfe5435c1d1