Closed Bug 1292592 Opened 8 years ago Closed 8 years ago

sourceeditor uses xul

Categories

(DevTools :: Framework, enhancement, P1)

Product:
DevTools
Component:
Framework
Type:
enhancement

Tracking

(firefox52 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 52
Iteration:
52.1 - Oct 3
Tracking Flags:
Tracking Status
firefox52 --- verified

People

(Reporter: tromey, Assigned: gasolin)

References

Details

(Whiteboard: [devtools-html])

Attachments

(5 files)

Bug 1292592 - load sourceeditor in inspector without iframe;
58 bytes, text/x-review-board-request
Details
Sample Code: Use a resource:// document in editor
2.26 KB, text/plain
Details
0001-Bug-1292592-remove-xul-dependency-in-sourceeditor.patch
31.71 KB, patch
Details | Diff | Splinter Review
Bug 1292592 - isolate xul dependency in sourceeditor and theme-switching;
58 bytes, text/x-review-board-request
Details
Bug 1292592 - Convert sourceeditor to use key shortcuts module;
58 bytes, text/x-review-board-request
ochameau
: review+
Details 
The sourceeditor component uses some xul; and this component is loaded
by the inspector.

In particular editor.js references a bunch of chrome URLs, which will
need to be changed:

https://dxr.mozilla.org/mozilla-central/source/devtools/client/sourceeditor/editor.js#57

There's also this:

https://dxr.mozilla.org/mozilla-central/source/devtools/client/sourceeditor/editor.js#262
Flags: qe-verify?
Whiteboard: [devtools-html] → [devtools-html] [triage]
Flags: qe-verify? → qe-verify+
Priority: -- → P2
QA Contact: cristian.comorasu
Whiteboard: [devtools-html] [triage] → [reserve-html]
James, I saw Bug 1295318 mentioned to reuse CodeMirror.

Does new debugger already converted the sourceeditor from XUL? Anything suggestion that may help porting and reuse CodeMirror code?
Flags: needinfo?(jlong)
(In reply to Fred Lin [:gasolin] from comment #1)
> James, I saw Bug 1295318 mentioned to reuse CodeMirror.
> 
> Does new debugger already converted the sourceeditor from XUL? Anything
> suggestion that may help porting and reuse CodeMirror code?

No, it just uses a plain CodeMirror instance when developing locally in a tab. When the debugger is running inside Firefox, it will use sourceeditor/editor.js. The only thing I did was make sure that we could load CodeMirror without an iframe. The new debugger still runs in privileged context technically so chrome:// URLs still work.

We've talked about refactoring sourceeditor/editor.js so that will probably happen soon.
Flags: needinfo?(jlong)
Issues to de-xul sourceeditor

1. It create an xul element
https://dxr.mozilla.org/mozilla-central/source/devtools/client/sourceeditor/editor.js#265

simply switch to `el.ownerDocument.createElement("iframe");` would work


2. ci.inIDOMUtils

after search, new debugger seems have the replacement for that
https://github.com/devtools-html/debugger.html/blob/master/public/js/lib/devtools-sham/sham/inDOMUtils.js

3. Services.prefs
new debugger seems have the replacement for that as well
https://github.com/devtools-html/debugger.html/blob/master/public/js/lib/devtools-sham/sham/services/prefs.js


4. theme-switching.js include XUL
https://dxr.mozilla.org/mozilla-central/source/devtools/client/sourceeditor/editor.js#57


James, #2,#3 could reuse part of debugger.html works, and I found you might already have some plan in bug 1295213 and bug 1294749 , do you have some suggestion how we reuse inDOMUtils.js/pref.js from new debugger?
Flags: needinfo?(jlong)
@Brian: any plans how to solve #4? (see comment #3)

Honza
Flags: needinfo?(bgrinstead)
(In reply to Fred Lin [:gasolin] from comment #3)

> 2. ci.inIDOMUtils
> 
> after search, new debugger seems have the replacement for that
> https://github.com/devtools-html/debugger.html/blob/master/public/js/lib/
> devtools-sham/sham/inDOMUtils.js

This is being handled by one of the dependencies of bug 1263287.

> 3. Services.prefs
> new debugger seems have the replacement for that as well
> https://github.com/devtools-html/debugger.html/blob/master/public/js/lib/
> devtools-sham/sham/services/prefs.js

This is already dealt with in m-c.
See devtools/client/shared/shim/Services.js.
This Services shim is loaded by the content loader, see bug 1291049.
Thanks Tom for clarify it. 

* Bug 1289425 - replace uses of getCSSPropertyNames already dealt with `2` and is wait for landing.
* `3` is invalid (Services.prefs is not xul)

Therefore in this issue we only need to deal with are 1 & 4
Assignee: nobody → gasolin
Depends on: 1289425
Flags: needinfo?(jlong)
Status: NEW → ASSIGNED
Comment on attachment 8786193 [details]
Bug 1292592 - load sourceeditor in inspector without iframe;

I've addressed 1 & 4 and passed tests. 
But `theme-switching.js` seems lost the ability to `require` if I commented out the `Cu.import(...Loader.jsm)` part, so this PR is not true de-xul patch yet.

It might because of `theme-switching.js` is injected in iframe? Or do I need to add its dependencies on editor's CM_SCRIPTS list directly?
Attachment #8786193 - Flags: feedback?(ttromey)
Comment on attachment 8786193 [details]
Bug 1292592 - load sourceeditor in inspector without iframe;

https://reviewboard.mozilla.org/r/75154/#review73246

I think this is a reasonable first step, but as noted below, the chrome:// URLs have to be replaced somehow.  I am not sure how best to do this.  Maybe in the content case we can somehow avoid the iframe?  (Not sure if this makes sense.)  Or have webpack make a separate bundle for CodeMirror?

Another question is whether we want to keep using theme-switching.js in content.  If so then it seems like it will need some reworking; plus css-reload.js will also need some work.

I'm probably not the best person to ask some of these things.  I suggest :bgrins or :jryans.  Sorry about that!

::: devtools/client/shared/theme-switching.js:9
(Diff revision 1)
>  
>  /* eslint-env browser */
>  "use strict";
>  (function () {
> +  const { utils: Cu } = Components;
> +  const { require } = Cu.import("resource://devtools/shared/Loader.jsm", {});

I think the reason this is needed is that theme-switching.js is generally loaded from XUL; except in this one spot in the source editor where it seems to be injected into the iframe.

I don't know much about this I'm afraid.  As far as de-chrome-ification goes I think at a minimum we need to find a way to inject or load all the editor code without using chrome:// URLs -- so this patch would have to do that somehow.

I don't really know what the options are.

::: devtools/client/shared/theme-switching.js:12
(Diff revision 1)
>  (function () {
> +  const { utils: Cu } = Components;
> +  const { require } = Cu.import("resource://devtools/shared/Loader.jsm", {});
> +  const Services = require("Services");
> +  const { gDevTools } = require("devtools/client/framework/devtools");
> +  const StylesheetUtils = require("sdk/stylesheet/utils");

This require seems problematic.

::: devtools/client/shared/theme-switching.js:120
(Diff revision 1)
> -    let hiddenDOMWindow = Cc["@mozilla.org/appshell/appShellService;1"]
> -                 .getService(Ci.nsIAppShellService)
> -                 .hiddenDOMWindow;
> -
>      // TODO: extensions might want to customize scrollbar styles too.
> -    if (!hiddenDOMWindow.matchMedia("(-moz-overlay-scrollbars)").matches) {
> +    if (!Services.appShell.hiddenDOMWindow.matchMedia("(-moz-overlay-scrollbars)").matches) {

This is going to need some support in the Services shim, if we want to keep using this.  (But I'm not sure about this, will comment elsewhere.)

::: devtools/client/sourceeditor/css-autocompleter.js:10
(Diff revision 1)
>  "use strict";
>  
>  /* eslint-disable complexity */
>  
>  /* eslint-disable mozilla/reject-some-requires */
> -const { Cc, Ci } = require("chrome");
> +const { Ci, Cc } = require("chrome");

Seems like an unnecessary edit.

::: devtools/client/sourceeditor/editor.js:24
(Diff revision 1)
>  
>  const ENABLE_CODE_FOLDING = "devtools.editor.enableCodeFolding";
>  const KEYMAP = "devtools.editor.keymap";
>  const AUTO_CLOSE = "devtools.editor.autoclosebrackets";
>  const AUTOCOMPLETE = "devtools.editor.autocomplete";
> -const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> +const L10N_BUNDLE = "chrome://devtools/locale/sourceeditor.properties";

I think this is probably not needed, due to Julian's changes in one of the l10n bugs.
Attachment #8786193 - Flags: feedback?(ttromey)
Comment on attachment 8786193 [details]
Bug 1292592 - load sourceeditor in inspector without iframe;

https://reviewboard.mozilla.org/r/75154/#review73246

> This require seems problematic.

`sdk/stylesheet/utils` depends on `nsIDOMWindowUtils.loadSheetUsingURIString` API to synchronously loads a style sheet from `uri`

> This is going to need some support in the Services shim, if we want to keep using this.  (But I'm not sure about this, will comment elsewhere.)

Ooh I saw `Services.appShell.hiddenDOMWindow` is used elsewhere like https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/rules/rules.js#95
so it's fine to use it. After checking `client/shared/shim/service.js` it does not contain that shim yet
In summary of comment 9, we still need suggestions to

1. find a way to inject or load all the editor code without using chrome:// URLs
2. find way to deal with xul in shared/theme-switching.js (require, appshell.hiddenDOMWindow, sdk/stylesheet/utils)


Since :bgrin is ni'd, I'll also ni :jryans for suggestion
Flags: needinfo?(jryans)
See Also: → 1299419
create bug 1299419 since I found rule.js also use appshell.hiddenDOMWindow and APIs from `sdk/`
(In reply to Fred Lin [:gasolin] from comment #12)
> In summary of comment 9, we still need suggestions to
> 
> 1. find a way to inject or load all the editor code without using chrome://
> URLs

I guess we could make an editor.html file that refers to relative paths for the script / style files instead of using a data URL.  It'll still be a chrome URL to load the HTML file but we could maybe find a way to reference it another way.

> 2. find way to deal with xul in shared/theme-switching.js (require,
> appshell.hiddenDOMWindow, sdk/stylesheet/utils)

I converted most usages of StylesheetUtils to createProcessingInstruction but from what I remember we needed to use it for loading agent stylesheets which was needed for custom scrollbars?  In any case, we could run all of the hiddenDOMWindow / StylesheetUtils code only when we have chrome priv available and not try it otherwise.  Maybe easiest thing to do is to try / catch that whole block of code?
Flags: needinfo?(bgrinstead)
Comment on attachment 8786193 [details]
Bug 1292592 - load sourceeditor in inspector without iframe;

https://reviewboard.mozilla.org/r/75154/#review73754

::: devtools/client/shared/theme-switching.js:19
(Diff revision 2)
> +  const { OS } = Services.appinfo;
>    const SCROLLBARS_URL = "chrome://devtools/skin/floating-scrollbars-dark-theme.css";
>    let documentElement = document.documentElement;
>  
>    let os;
> -  let platform = navigator.platform;
> +  if (OS === "WINNT") {

navigator.platform is safe in content, so we don't need to switch away from it
(In reply to Brian Grinstead [:bgrins] from comment #14)
> (In reply to Fred Lin [:gasolin] from comment #12)
> > In summary of comment 9, we still need suggestions to
> > 
> > 1. find a way to inject or load all the editor code without using chrome://
> > URLs
> 
> I guess we could make an editor.html file that refers to relative paths for
> the script / style files instead of using a data URL.  It'll still be a
> chrome URL to load the HTML file but we could maybe find a way to reference
> it another way.

It seems like you can try something like:

1. Make a new editor.html file like :bgrins mentions
2. Add it to the DevToolsModules list in that directory's moz.build
  * This exposes the file as a resource://devtools/... URL where the URL matches the file path in tree
  * resource:// URLs aren't trusted like chrome:// URLs, so it should be similar to what would happen when loading as a website
3. Use the new resource:// URL as the iframe src

For the various scripts and styles, do something like:

1. Create moz.build files for their directories and list them as DevToolsModules
2. Change all the chrome:// URLs to resource:// URLs that match their paths
  * Since they are now available under the resource:// scheme, they can also be relative URLs in the new editor.html like :bgrins said

This will run everything without the trusted access of a chrome:// URL, so there's probably bits to work around, like the theme switching.

With relative URLs, it should work out fairly well as a website too.  The main change in a website is the editor.html URL becomes https:// instead of resource://, and we can probably use a loader API later on to get the right URL.
Flags: needinfo?(jryans)
Once the scripts and styles are no longer needed as chrome:// URLs, you should also remove them from devtools/client/jar.mn (otherwise we would package two copies of each file when we only care about one).
Thanks Brain & Ryan's suggestions!

While converting source to resource:// , I found child iframe is only accessible when I use chrome:// to load editor.html, with resource:// seems not work.
(In reply to Fred Lin [:gasolin] from comment #18)
> Thanks Brain & Ryan's suggestions!
> 
> While converting source to resource:// , I found child iframe is only
> accessible when I use chrome:// to load editor.html, with resource:// seems
> not work.

I noticed that too when doing some work on Bug 1291049.  Is it not possible to build HTML files as resource://?  I found examples with JS and CSS but haven't found one with HTML.
Flags: needinfo?(jryans)
(In reply to Brian Grinstead [:bgrins] from comment #19)
> (In reply to Fred Lin [:gasolin] from comment #18)
> > Thanks Brain & Ryan's suggestions!
> > 
> > While converting source to resource:// , I found child iframe is only
> > accessible when I use chrome:// to load editor.html, with resource:// seems
> > not work.
> 
> I noticed that too when doing some work on Bug 1291049.  Is it not possible
> to build HTML files as resource://?  I found examples with JS and CSS but
> haven't found one with HTML.

Can you be more specific about the problems encountered?  Perhaps attaching your WIP and asking for feedback would be good.

I am attaching some sample code that uses a resource:// document in the editor.  HTML, CSS, and JS all appear to work over resource://.
Flags: needinfo?(jryans)
Thanks for the sample. There's no problem to reference resource:// inside the iframe, the issue occurred when editor.js want to access `win.CodeMirror` from the iframe. editor.js will pass configs into win.CodeMirror to customize the editor.

https://dxr.mozilla.org/mozilla-central/source/devtools/client/sourceeditor/editor.js#304

When the editor.html load with chrome://, the variable is accessible. When editor.html load with resource://, its not.
Flags: needinfo?(jryans)
Comment on attachment 8786193 [details]
Bug 1292592 - load sourceeditor in inspector without iframe;

The patch use resource:// to load css/js in editor.html, (but use chrome:// to load editor.html itself, the reason is as above comment)
haven't fix tests yet which depends on `chrome://` path. Will remove redundant resources from jar.mn once test fixed.
Attachment #8786193 - Flags: feedback?(jryans)
Attachment #8786193 - Flags: feedback?(jryans)
Thanks for posting the WIP, that's very helpful!  There's no problem with loading a simple document via resource://.  The issue we're hitting here is that if we change only the CM frame to resource://, then it becomes cross-origin from its parent frame (like the Debugger's frame) which is currently loaded over chrome://.  This causes the platform to turn on security wrappers that generally try to stop you from sharing objects across two cross-origin frames.  (These are the same wrappers you see most commonly trying to prevent browser UI from crossing into general web content windows.)

One piece of context to keep in mind is that, at least for the Debugger panel, this would likely be a short lived problem.  We want to convert the Debugger's frame to be resource:// as well soon, and that would make the problem go away.  However, we would still run into this issue for other tools still using chrome:// frames, like the Style Editor.

Additionally, we may hit a similar problem to this one if we move forward with using a XUL document (loaded from chrome://) as the root of the toolbox inside Firefox, since it will be cross-origin from the toolbox loaded inside it.  (Whether this is actually an issue will depend on whether we're trying to call functions / pass object across frames for that particular case.)

With this context in mind, here are some possible paths forward:

A. Use Gecko helpers to work around security
--------------------------------------------

We can add some utility code that uses special Gecko helpers like `Cu.waiveXrays` and `Cu.exportFunction` to work around the security barrier.  I have started down this path locally, and it is possible to do, but it adds a lot of noise to the editor.js module, since you need to drop extra calls in every place that crosses frames, and there are quite a few in this module.

It's not just a one way issue either: in my experience so far, you have to convert objects going into the frame and also convert objects coming back via a callback or similar.

I suppose we could also ask platform if there's some way to say "just stop it with that security thing", but I am fairly sure they would say "either make the frames same-origin or deal with the consequences of cross-origin".

B. Make the frame's scheme match the parent frame
-------------------------------------------------

Instead of forcing the CM frame to always use resource://, we could instead make it use the scheme of the parent frame.  This ensures they are always same-origin.

I am not sure if there's a clean way to use a relative URL for the document itself (thankfully there is at least for the scripts inside it) that would work in all cases.  Maybe we can delegate this problem to the loader (not `require`, just `resolve` or similar) to turn an ID for the document into the proper URL.  At the very least, we could check if the parent frame is chrome://, resource://, or https:// and hard code the right answer for each case.

C. Remove the iframe
--------------------

Another option is to remove the iframe and mount the CM editor onto some element in the caller's own frame.  (In fact, this is how debugger.html uses CM today in their own repo.)

This of course removes the security issues and makes it even more clear that it's okay to directly access CM objects, which we've already proposed doing more of in bug 1295213.

It's possible this would introduce some styling issues if CM CSS is not sufficiently scoped.  If that does happen though, it should be fairly easy to tweak the CSS to accommodate this.

D. Convert to message passing approach
--------------------------------------

If we want to make things work with cross-origin frames without Gecko tricks (option A), we could convert the editor.js module to use message passing to talk to the CM frame, such as using `postMessage` or `MessageChannel`.  This would mean all editor.js APIs that interact with CM become async / promise based.  I have no idea how much work that implies, probably fairly significant?

Summary
-------

IMO, option C seems like a good way to go.  I am not really sure why we have a separate CM frame to begin with, but we were pretty frame happy back when editor.js was created, so probably just for some feeling of isolation.

:bgrins, any thoughts?
Flags: needinfo?(jryans) → needinfo?(bgrinstead)
See Also: → 1295213
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #28)
> IMO, option C seems like a good way to go.  I am not really sure why we have
> a separate CM frame to begin with, but we were pretty frame happy back when
> editor.js was created, so probably just for some feeling of isolation.
> 
> :bgrins, any thoughts?

IIRC we can't remove the frame when it's contained within a XUL document - because of course CodeMirror doesn't expect to be loaded inside XUL.  Maybe it's possible to keep it as a chrome:// iframe when loaded in XUL and append it in a DOM element when not. This is a lot like Bug 1295213, with some tradeoffs being listed in https://bugzilla.mozilla.org/show_bug.cgi?id=1295213#c2. Keeping the ni? so I can think more about it later.
Comment on attachment 8786193 [details]
Bug 1292592 - load sourceeditor in inspector without iframe;

https://reviewboard.mozilla.org/r/75154/#review74552

::: devtools/client/shared/theme-switching.js:8
(Diff revision 7)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  /* eslint-env browser */
>  "use strict";
>  (function () {
> +  const { utils: Cu } = Components;

In content, there is no `Cu`, so it seems like this will fail early, even though we don't need this stuff for all code paths here.  Can we do something like test for `Cu` and skip the blocks that don't work without it?

::: devtools/client/sourceeditor/editor.js:405
(Diff revision 7)
>  
>        win.CodeMirror.defineExtension("l10n", (name) => {
>          return L10N.getStr(name);
>        });
>  
>        cm.getInputField().controllers.insertControllerAt(0, controller(this));

This stuff about `controllers` will likely need to be reimplmented for content (depends on the approach we choose from the ones I just commented about).  I believe the `controllers` property only exists on elements when loaded via chrome:// URLs.  So, we'll need to redo the features connected to this using regular web APIs.
(In reply to Brian Grinstead [:bgrins] from comment #29)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #28)
> > IMO, option C seems like a good way to go.  I am not really sure why we have
> > a separate CM frame to begin with, but we were pretty frame happy back when
> > editor.js was created, so probably just for some feeling of isolation.
> > 
> > :bgrins, any thoughts?
> 
> IIRC we can't remove the frame when it's contained within a XUL document -
> because of course CodeMirror doesn't expect to be loaded inside XUL.  Maybe
> it's possible to keep it as a chrome:// iframe when loaded in XUL and append
> it in a DOM element when not. This is a lot like Bug 1295213, with some
> tradeoffs being listed in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1295213#c2. Keeping the ni? so
> I can think more about it later.

Ah, that's a good point about XUL docs.  Well, option B (match the parent's frame) also seems appealing as a way to preserve current behavior while still getting ready for the web.  It just means you can't be _sure_ the editor component is web-ready by itself, since it will follow what its parent frame does.
I'll try Option C first and see if any style issues there, thanks for advices!

For controllers.insertControllerAt replacement, it seems totally depends on XUL  
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Places/View_Controller
Do you have any suggestion to de-XUL it?
Flags: needinfo?(jryans)
(In reply to Fred Lin [:gasolin] from comment #32)
> For controllers.insertControllerAt replacement, it seems totally depends on
> XUL  
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Places/View_Controller
> Do you have any suggestion to de-XUL it?

I think you should be able to listen for key events that map to each of the actions and trigger them from there.  editor.js should be able to instantiate our new KeyShortcuts module (devtools/client/shared/key-shortcuts) and pass the CodeMirror frame's content window to it.
Flags: needinfo?(jryans)
For what it's worth, bug 1295213 will conflict with this patch, but not so badly. This patch doesn't change sourceeditor/editor.js very much. Resolving those conflicts will be easy. That patch introduces a way to embed CodeMirror without an iframe, which is what we need in debugger.html.
Thanks for notice! 

I've applied bug 1295213 with current WIP, put dependent css/js into markup.xhtml and apply appendToLocalElement to inspector (modify a bit to let appendToLocalElement return a promise). It works well without iframe.

I will check if we can dynamically load external JavaScript/CSS files without xul, then we can replace  `appendTo` usage for all components.
https://dxr.mozilla.org/mozilla-central/search?q=editor.appendTo&redirect=false
Depends on: 1295213
Here's the WIP based on Bug 1295213.

Inject css/js (via `el.ownerDocument.getElementsByTagName("head")[0].appendChild(script);`) seems not work in XUL though.
> I will check if we can dynamically load external JavaScript/CSS files without xul, then we can replace  `appendTo` usage for all components.

I don't want to try to load CodeMirror in XUL frames.  We can switch to appendToLocalElement if/when it makes sense per tool. The markup view is a good candidate, and it should help with the progress in bug 1291049.
(In reply to Brian Grinstead [:bgrins] from comment #37)
> > I will check if we can dynamically load external JavaScript/CSS files without xul, then we can replace  `appendTo` usage for all components.
> 
> I don't want to try to load CodeMirror in XUL frames.  We can switch to
> appendToLocalElement if/when it makes sense per tool. The markup view is a
> good candidate, and it should help with the progress in bug 1291049.

To be clear, in the XUL case it's OK if communication between frame and module is privileged, since those usages will never need to happen outside of chrome.  So as long as we have one path that's content safe (which could be appendToLocalElement), we don't need to solve this problem.  So in reference to Comment 28, I think option C is the way to try, but only for usages that we want to be content-safe.
Flags: needinfo?(bgrinstead)
Comment on attachment 8789309 [details] [diff] [review]
0001-Bug-1292592-remove-xul-dependency-in-sourceeditor.patch

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

::: devtools/client/sourceeditor/editor.js
@@ +279,4 @@
>    _setup: function(el) {
>      let win = el.ownerDocument.defaultView;
>  
> +    // let scriptsToInject = CM_SCRIPTS.concat(this.config.externalScripts);

If we go this route we'll still need to inject all scripts for local element case, but not for the frame (if we move away from the data uri frame).  And we'll need something like Option B I guess to determine the protocol on which to load the scripts (and also, what should the paths to the scripts be in the appendToLocalElement case for content-windows)?
If long term we want to be moving to appendToLocalElement, then we could stick with the current chrome:// data URI iframe for XUL/priv code and move away from frames entirely for content code.  Then we could do something different for resources that get injected in the content case.

One of these approaches might satisfy this (sorry for the naming, just continuing on from earlier proposal to prevent confusion)

Option E:
1) modify the chrome script paths to HTTP/HTTPS/resource paths based on protocol the host is loaded in - although I don't know what the base URI would be in that case (maybe passed in as an option).
2) Go away from the subscriptLoader but only when running with appendToLocalElement - you can use the appendChild into the host document thing in this case since we know it's an HTML doc.

Option F:
1) See if we can load the modules directly through the loader instead of relying them being defined on the window (like https://github.com/devtools-html/debugger.html/blob/bb617bab4a469b7754712029642d3fba5ce4db6a/public/js/utils/source-editor.js#L1-L6).  Not sure if this will work with both our loader and webpack, but it might end up being the simplest solution if it did.
2) Require that the page includes the <link> tag for CSS using a relative path

Ryan, any opinions?
Flags: needinfo?(jryans)
Since our main goal is to resolve inspector's sourceeditor XUL dependency, I'd like take the following approach:


1. change sourceeditor/editor.js referred js/css from `chrome://` to `resource://`, make it work in XUL and HTML

2. keep editor.js `appendTo` method unchanged, so it won't affect loading CodeMirror in existing XUL panels

3. use `appendToLocalElement` to load inspector's html panels (and any new html panels). Currently I'll put the referenced js/css in markup.html, we can load the modules directly through the loader once we support that.

4. add extra param `isHtml` in _setup(), to exclude XUL APIs such as `scriptsToInject`, `controllers.insertControllerAt` and provide the alternative solution for html pages.

With this approach it does not influence the existing XUL panels but still support us move to the new html panels.


Does that make sense?
Flags: needinfo?(bgrinstead)
(In reply to Fred Lin [:gasolin] from comment #41)
> Since our main goal is to resolve inspector's sourceeditor XUL dependency,
> I'd like take the following approach:
> 
> 
> 1. change sourceeditor/editor.js referred js/css from `chrome://` to
> `resource://`, make it work in XUL and HTML

chrome:// should work fine in both XUL and HTML as long as the code is privileged.  I think the solution in 3 will remove the need to do this - the current target is to load the inspector in a normal browser tab that won't have permission to resource:// anyway so AFAICT you don't need to do this part.

> 2. keep editor.js `appendTo` method unchanged, so it won't affect loading
> CodeMirror in existing XUL panels
> 
> 3. use `appendToLocalElement` to load inspector's html panels (and any new
> html panels). Currently I'll put the referenced js/css in markup.html, we
> can load the modules directly through the loader once we support that.

OK, let's try that in a patch first and make sure it's all working.  I think before landing that we'll need to also figure out a solution for bundling all of the codemirror scripts into a single script so we don't need to keep that big list in sync in 2 places.  I think the relative require paths inside the CM folder are not correct right now, but we could get that fixed (cc Gabe, who's been managing CM updates).

> 4. add extra param `isHtml` in _setup(), to exclude XUL APIs such as
> `scriptsToInject`, `controllers.insertControllerAt` and provide the
> alternative solution for html pages.

I don't think we need this if we convert all content-priv usages to appendToLocalElement as in 3.

> With this approach it does not influence the existing XUL panels but still
> support us move to the new html panels.
> 
> 
> Does that make sense?

Yes, thanks
Flags: needinfo?(bgrinstead)
The overall direction of this bug seems reasonable.

(In reply to Brian Grinstead [:bgrins] from comment #44)
> (In reply to Fred Lin [:gasolin] from comment #41)
> > Since our main goal is to resolve inspector's sourceeditor XUL dependency,
> > I'd like take the following approach:
> > 
> > 
> > 1. change sourceeditor/editor.js referred js/css from `chrome://` to
> > `resource://`, make it work in XUL and HTML
> 
> chrome:// should work fine in both XUL and HTML as long as the code is
> privileged.  I think the solution in 3 will remove the need to do this - the
> current target is to load the inspector in a normal browser tab that won't
> have permission to resource:// anyway so AFAICT you don't need to do this
> part.

Not clear to me if this step 1 here means only JS and CSS via resource://, or also using a resource:// document as well for XUL consumers...  It seems like a resource:// document would trigger the cross-origin issues we found earlier in bug, so probably that's not what you want.

I agree with :bgrins that it seems reasonable to focus on step 3 and forcing content page (low privilege) consumers down the appendToLocalElement path.

(In reply to Brian Grinstead [:bgrins] from comment #44)
> > 4. add extra param `isHtml` in _setup(), to exclude XUL APIs such as
> > `scriptsToInject`, `controllers.insertControllerAt` and provide the
> > alternative solution for html pages.
> 
> I don't think we need this if we convert all content-priv usages to
> appendToLocalElement as in 3.

Somewhere there's work to do to replace `controllers.insertControllerAt` with key shortcut bindings for content page consumers.  So, I assume that's at least part of what is in this step?  If so, it sounds like it would still be needed so that select all, copy, etc. continue to work?
Flags: needinfo?(jryans)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) (on PTO Sept. 10 - 25) from comment #45)
> (In reply to Brian Grinstead [:bgrins] from comment #44)
> > > 4. add extra param `isHtml` in _setup(), to exclude XUL APIs such as
> > > `scriptsToInject`, `controllers.insertControllerAt` and provide the
> > > alternative solution for html pages.
> > 
> > I don't think we need this if we convert all content-priv usages to
> > appendToLocalElement as in 3.
> 
> Somewhere there's work to do to replace `controllers.insertControllerAt`
> with key shortcut bindings for content page consumers.  So, I assume that's
> at least part of what is in this step?  If so, it sounds like it would still
> be needed so that select all, copy, etc. continue to work?

Oh, right.  I'd be fine with null-checking controllers there and not running the code at least for a first step.  I think those basic editing items would work in a content environment even without the code.  I don't know about being able to add new context menu items (like the debugger does) but again, I think that could be follow-up material
Depends on: 1301790
patch updated to reflect the proposal in comment 41 (2,3,4), will convert controllers command to keyshort cut (comment 33) then send the feedback request.
WIP, add shortcuts for codemirror editor, The patch now can respond the shortcut events but not work as expected. I guess current codemirror shortcut does not work in new debugger as well.
Currently in editors.js there are two system to catch shortcuts

1. CodeMirror with extraKeys
https://github.com/mozilla/gecko-dev/blob/master/devtools/client/sourceeditor/editor.js#L150

2. Commands for XUL Controller
https://github.com/mozilla/gecko-dev/blob/master/devtools/client/sourceeditor/editor.js#L1349

At the end they both execute commands in CodeMirror
https://github.com/mozilla/gecko-dev/blob/master/devtools/client/sourceeditor/editor.js#L1397
https://codemirror.net/doc/manual.html#commands

Since we want de-XUL the editor, I think we can get rid of XUL controller/commands and make all of shortcut passed through CodeMirror config.extrakeys.

:bgrin do you know who is more familiar with CodeMirror part and can confirm if its a right way to go?
Flags: needinfo?(bgrinstead)
(In reply to Fred Lin [:gasolin] from comment #52)
> Currently in editors.js there are two system to catch shortcuts
> 
> 1. CodeMirror with extraKeys
> https://github.com/mozilla/gecko-dev/blob/master/devtools/client/
> sourceeditor/editor.js#L150
> 
> 2. Commands for XUL Controller
> https://github.com/mozilla/gecko-dev/blob/master/devtools/client/
> sourceeditor/editor.js#L1349
> 
> At the end they both execute commands in CodeMirror
> https://github.com/mozilla/gecko-dev/blob/master/devtools/client/
> sourceeditor/editor.js#L1397
> https://codemirror.net/doc/manual.html#commands
> 
> Since we want de-XUL the editor, I think we can get rid of XUL
> controller/commands and make all of shortcut passed through CodeMirror
> config.extrakeys.
> 
> :bgrin do you know who is more familiar with CodeMirror part and can confirm
> if its a right way to go?

That sounds generally right to me.  Forwarding the ni? to Gabe to confirm
Flags: needinfo?(bgrinstead) → needinfo?(gl)
(In reply to Brian Grinstead [:bgrins] from comment #53)
> (In reply to Fred Lin [:gasolin] from comment #52)
> > Currently in editors.js there are two system to catch shortcuts
> > 
> > 1. CodeMirror with extraKeys
> > https://github.com/mozilla/gecko-dev/blob/master/devtools/client/
> > sourceeditor/editor.js#L150
> > 
> > 2. Commands for XUL Controller
> > https://github.com/mozilla/gecko-dev/blob/master/devtools/client/
> > sourceeditor/editor.js#L1349
> > 
> > At the end they both execute commands in CodeMirror
> > https://github.com/mozilla/gecko-dev/blob/master/devtools/client/
> > sourceeditor/editor.js#L1397
> > https://codemirror.net/doc/manual.html#commands
> > 
> > Since we want de-XUL the editor, I think we can get rid of XUL
> > controller/commands and make all of shortcut passed through CodeMirror
> > config.extrakeys.
> > 
> > :bgrin do you know who is more familiar with CodeMirror part and can confirm
> > if its a right way to go?
> 
> That sounds generally right to me.  Forwarding the ni? to Gabe to confirm

Yes, I think this sounds right. We use this.config.extraKeys to define additional shortcuts and keep the shortcut keys in sourceeditor.properties, and we use xul controllers in Editor.js#471. So, we want to remove this XUL controller and pass through config.extraKeys.
Flags: needinfo?(gl)
I re-organize the patch so it will be easier for review.
 
I found the Keys handling part is broken for sourceeditor attached with appendToLocalElement method(aka, the current debugger), I'd like to create another bug to fix the Keys handling part, then switch inspector's editor to appendToLocalElement will cause less impact.
Blocks: devtools-html-phase2
No longer blocks: devtools-html-2
Iteration: --- → 52.1 - Oct 3
Priority: P2 → P1
Whiteboard: [reserve-html] → [devtools-html]
Depends on: 1304262
Comment on attachment 8786193 [details]
Bug 1292592 - load sourceeditor in inspector without iframe;

The PR contain 3 patches
1. load sourceeditor in inspector without iframe
2. isolate xul dependency in sourceeditor and theme-switching
3. convert sourceeditor to use key shortcuts module
Attachment #8786193 - Flags: review?(bgrinstead)
Comment on attachment 8786193 [details]
Bug 1292592 - load sourceeditor in inspector without iframe;

https://reviewboard.mozilla.org/r/75154/#review79560
Attachment #8786193 - Flags: review?(bgrinstead) → review+
Comment on attachment 8792839 [details]
Bug 1292592 - isolate xul dependency in sourceeditor and theme-switching;

https://reviewboard.mozilla.org/r/79728/#review79562
Attachment #8792839 - Flags: review+
Comment on attachment 8794087 [details]
Bug 1292592 - Convert sourceeditor to use key shortcuts module;

https://reviewboard.mozilla.org/r/80700/#review79564

Will take a closer look at this next week, but in the mean time just want to note that devtools/client/sourceeditor/test/browser_editor_find_again.js is failing in the try push
Attachment #8794087 - Flags: review?(bgrinstead)
Comment on attachment 8794087 [details]
Bug 1292592 - Convert sourceeditor to use key shortcuts module;

Flagging Alex for review on the key-shortcuts part
Attachment #8794087 - Flags: review?(bgrinstead) → review?(poirot.alex)
Update and re-run the treeherder, it shows devtools/client/sourceeditor/test/browser_editor_find_again.js only fail in mac OS 10.10. 

I can reproduce it on mac OS 10.11.6(EL Capitan) , will find out what's going on.
Comment on attachment 8794087 [details]
Bug 1292592 - Convert sourceeditor to use key shortcuts module;

https://reviewboard.mozilla.org/r/80700/#review79678

::: devtools/client/locales/en-US/sourceeditor.properties:118
(Diff revision 4)
> -# the typed search
> -find.commandkey=F
> -
> -# LOCALIZATION NOTE  (findAgain.commandkey): This is the key to use in
> -# conjunction with accel (Command on Mac or Ctrl on other platforms) to find
> -# again the typed search
> +# Do not localize "CmdOrCtrl", "F", or change the format of the string. These are
> +# key identifiers, not messages displayed to the user.
> +find.key=CmdOrCtrl+F
> +
> +# LOCALIZATION NOTE (selectAll.key):
> +# Key shortcut used to select  the whole content of the editor

The comment is wrong, you copy pasted selectAll.key and its comment.

::: devtools/client/locales/en-US/sourceeditor.properties:121
(Diff revision 4)
> -# LOCALIZATION NOTE  (findAgain.commandkey): This is the key to use in
> -# conjunction with accel (Command on Mac or Ctrl on other platforms) to find
> -# again the typed search
> -findAgain.commandkey=G
> +
> +# LOCALIZATION NOTE (selectAll.key):
> +# Key shortcut used to select  the whole content of the editor
> +# Do not localize "Shift", "CmdOrCtrl", "F", or change the format of the string. These are
> +# key identifiers, not messages displayed to the user.
> +replaceAll.key=Shift+CmdOrCtrl+F

I'm not on Mac, but looking at existing code, it looks like replaceAll is Alt+Cmd+F on Mac and woule require a per OS l10n key. Or tweak _initShortcuts to prepend Shift+ or Alt+ depending of the OS.

::: devtools/client/sourceeditor/editor.js:1262
(Diff revision 4)
> +        cm.execCommand("find");
> +
> +        if (isSearchInput || isReplaceAll) {
> +          node.select();
> +          isReplaceAll = false;
> +        }

I may be more readable to put that in a function with a isReplaceAll parameter to prevent fall through.
Same for findPrev/findNext.

::: devtools/client/sourceeditor/editor.js:1314
(Diff revision 4)
> +    // Prevent default for this action
> +    event.stopPropagation();
> +    event.preventDefault();
> +  },
> +
> +    /**

Indentation is wrong here.

::: devtools/client/sourceeditor/test/browser_editor_find_again.js:12
(Diff revision 4)
>  
>  const {LocalizationHelper} = require("devtools/shared/l10n");
>  const L10N = new LocalizationHelper("devtools/locale/sourceeditor.properties");
>  
> -const FIND_KEY = L10N.getStr("find.commandkey");
> -const FINDAGAIN_KEY = L10N.getStr("findAgain.commandkey");
> +const FIND_KEY = "F";
> +const FINDNEXT_KEY = "G";

Better use synthesizeKeyShortcut:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/test/shared-head.js#192-213
and keep pulling the key shortcut from localization file:

  const FIND_KEY = L10N.getStr("find.commandkey");
  synthesizeKeyShortcut(FIND_KEY, edWin);
Attachment #8794087 - Flags: review?(poirot.alex)
Attachment #8794087 - Flags: review?(poirot.alex)
Comment on attachment 8794087 [details]
Bug 1292592 - Convert sourceeditor to use key shortcuts module;

https://reviewboard.mozilla.org/r/80700/#review80598

I'm puzzled, it looks like we could just remove all this key shortcut code.
We should get the confirmation from someone more involved into our codemirror usage...

::: devtools/client/sourceeditor/editor.js:1058
(Diff revision 5)
> +    }
> +
> +    if (!isReplaceAll) {
> +      cm.execCommand("find");
> +    } else {
> +      cm.execCommand("replaceAll");

Actually, there wasn't any "replace all" shortcut before, just a "replace" one. Here you are slightly changing the behavior.
When pressing Shift+Ctrl+F the search dialog from code mirror (you can test this in the style editor) will alternate between Replace and ReplaceAll. Whereas the existing behavior is going to ask you at the end if you want to replace all or just one.

Also, I don't know much about code mirror, but it looks like we don't need to call these execCommand(find) nor execCommand(replaceAll).

I'm discovering codemirror code and it looks like all the key shortcuts are builtin:
http://searchfox.org/mozilla-central/source/devtools/client/sourceeditor/codemirror/lib/codemirror.js#5800-5831
Could it be that most of this code is just useless?

I don't see any difference in the style editor if I remove them all!
See comment 76. I supposed you are involved into our usage of codemirror, but do not hesitate to ni? someone who is.
Flags: needinfo?(bgrinstead)
In my experiment some shortcut make no difference (redo, undo, selecAll) so I would just removed them in next update. But others make some test failed, maybe there're some evil details there.
(In reply to Alexandre Poirot [:ochameau] from comment #77)
> See comment 76. I supposed you are involved into our usage of codemirror,
> but do not hesitate to ni? someone who is.

Forwarding the question to Gabe
Flags: needinfo?(bgrinstead) → needinfo?(gl)
Testing a clean codemirror seems to indicate that a lot of these shortcuts already works. I would need to test further more extensively on all OSes to confirm this is entirely true. I would propose removing the duplicate shortcuts if what :ochameau's experiment yields to be true.
Flags: needinfo?(gl)
Comment on attachment 8794087 [details]
Bug 1292592 - Convert sourceeditor to use key shortcuts module;

updated sourceeditor tests to include framework/tests/shared-head.js

Will set review once all try run green
Attachment #8794087 - Flags: review?(poirot.alex)
See Also: → 1306143
Comment on attachment 8792839 [details]
Bug 1292592 - isolate xul dependency in sourceeditor and theme-switching;

https://reviewboard.mozilla.org/r/79728/#review81136

::: devtools/client/sourceeditor/editor.js:316
(Diff revision 5)
> -      if (url.startsWith("chrome://")) {
> +        if (url.startsWith("chrome://")) {
> -        Services.scriptloader.loadSubScript(url, win, "utf8");
> +          Services.scriptloader.loadSubScript(url, win, "utf8");
> -      }
> +        }
> -    });
> +      });
> +    } catch (e) {
> +      console.warn("inject script is only supported in XUL");

This is not true. This feature allows add-ons (HTML or XUL) to inject custom scripts into the editor. I use it within my add-on which is 100% HTML.

When I originally added this bit of code, the 
if (url.startsWith("chrome://") was to forbid injecting scripts from remote urls (http/https) which can potentially be dangerous. "chrome://" just ensures the script you inject comes from the browser, or an add-on, with webextensions and Jetpack, "moz-extension" and "resource" should be allowed as well.

A better solution would be replacing the condition with:

if (!url.startsWith("http"))

or with:

let isSafeURLRegex = new RegExp("^(resource|moz-extension|chrome):\/\/");
if (url.search(isSafeURLRegex) > -1)
ntim, for comment 85 since this bug is focus on deXUL but not to change the existing behaviour, do you mind to file that in a separate bug?
Flags: needinfo?(ntim.bugs)
Comment on attachment 8792839 [details]
Bug 1292592 - isolate xul dependency in sourceeditor and theme-switching;

https://reviewboard.mozilla.org/r/79728/#review81320

::: devtools/client/sourceeditor/editor.js:309
(Diff revision 6)
>    _setup: function (el, doc) {
>      doc = doc || el.ownerDocument;
>      let win = el.ownerDocument.defaultView;
>  
> +    try {
> -    let scriptsToInject = CM_SCRIPTS.concat(this.config.externalScripts);
> +      let scriptsToInject = CM_SCRIPTS.concat(this.config.externalScripts);

In any way, this behaviour is wrong, if this block of code fails (although I don't see any reason why it would), the CodeMirror script won't be injected. (Because we concat CM_SCIPTS with this.config.externalScripts.

So I think we should remove the try { } catch(e) {} here anyway, since there's no reason why this would break.

::: devtools/client/sourceeditor/editor.js:316
(Diff revision 6)
> -      if (url.startsWith("chrome://")) {
> +        if (url.startsWith("chrome://")) {
> -        Services.scriptloader.loadSubScript(url, win, "utf8");
> +          Services.scriptloader.loadSubScript(url, win, "utf8");
> -      }
> +        }
> -    });
> +      });
> +    } catch (e) {
> +      console.warn("inject script is only supported in XUL");

This statement isn't true as I explained before.
I'm fine with not changing the behaviour, but that includes removing the try {} catch (e) {} which is incorrect behaviour as well (as it would prevent the CM_SCRIPTS from being injected if that fails, although I wouldn't see how injecting scripts would fail).
Flags: needinfo?(ntim.bugs)
Comment on attachment 8794087 [details]
Bug 1292592 - Convert sourceeditor to use key shortcuts module;

https://reviewboard.mozilla.org/r/80700/#review81334

Oh. Sorry if I wasn't clear enough, but it looks like we do not need any key shortcut code in editor.js.
Everything seems to be already implemented into codemirror itself.
So you should be able to remove every code related to key short code from editor.js (KeyShortcuts, _initShortcuts and the l10n keys from the .properties file)

But you can keep the modification you made to the tests even if you will have to modify them again as you won't have the l10n keys anymore.

::: devtools/client/framework/test/shared-head.js:212
(Diff revision 8)
>      metaKey: shortcut.meta,
>      shiftKey: shortcut.shift
> -  }, target);
> +  };
> +  if (shortcut.keyCode) {
> +    aKeyEvent.keyCode = shortcut.keyCode;
> +  }

Why do not have to do that?
Attachment #8794087 - Flags: review?(poirot.alex)
As comment 78, remove shortcut handling like findNextOrPrev/findOrReplace failed some existing tests. 
We need at least handle replaceAll/find/findNext/findPrev shotcuts.


The New PR update find_again test with `synthesizeKeyShortcut` provided by shared-header.js
I have to modify synthesizeKeyShortcut a bit to not pass `undefined/null` keycode property
https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/EventUtils.js#759

Pass null keycode in synthesizeKeyShortcut returns wrong input.selectionStart result
Flags: needinfo?(poirot.alex)
Comment on attachment 8786193 [details]
Bug 1292592 - load sourceeditor in inspector without iframe;

https://reviewboard.mozilla.org/r/75152/#review81370

::: devtools/client/sourceeditor/test/browser_editor_find_again.js:13
(Diff revision 21)
> -const FIND_KEY = L10N.getStr("find.commandkey");
> -const FINDAGAIN_KEY = L10N.getStr("findAgain.commandkey");
> -
>  const { OS } = Services.appinfo;
>  
> -// On linux, getting immediately the selection's range here fails, returning
> +const FIND_KEY = L10N.getStr("find.key");

It looks like you stripped the existing comment:
"// On linux, getting immediately the selection's range here fails, returning" is missing.

::: devtools/client/sourceeditor/test/browser_editor_find_again.js:62
(Diff revision 21)
>  
>    // Ensure the input has the focus before send the key – necessary on Linux,
>    // it seems that during the tests can be lost
>    input.focus();
>  
> -  EventUtils.synthesizeKey(FINDAGAIN_KEY, { accelKey: true, shiftKey }, edWin);
> +  if (shiftKey) {

You should renamed `shiftKey` variable to findPrevious or something alike.
Comment on attachment 8794087 [details]
Bug 1292592 - Convert sourceeditor to use key shortcuts module;

https://reviewboard.mozilla.org/r/80700/#review81372

Sorry, yes you were right, there is some very subtle details that justify these reimplementations.
I tried to find why and suggest you to add comment about that.

::: devtools/client/framework/test/shared-head.js:212
(Diff revision 8)
>      metaKey: shortcut.meta,
>      shiftKey: shortcut.shift
> -  }, target);
> +  };
> +  if (shortcut.keyCode) {
> +    aKeyEvent.keyCode = shortcut.keyCode;
> +  }

We prefix variable with `a` for function arguments only. Also, in devtools we try to avoid doing that now. We may only do that if existing code does it.
Please name this `keyEvent` or just `event`.

::: devtools/client/sourceeditor/editor.js:1040
(Diff revision 8)
>      cm.setSelection({ line: start.line + 1, ch: start.ch },
>        { line: end.line + 1, ch: end.ch });
>    },
>  
>    /**
> +   * Find the typed search or replace the content of the editor

We should tell why we reimplement an existing codemirror shortcut. You can see why by looking at why the tests fail when not doing this.
Here, we overload CmdOrCtrl+F in order to select the search input. Just that. And as we prevent the event we also have to call execCommand(find).

::: devtools/client/sourceeditor/editor.js:1057
(Diff revision 8)
> +      return;
> +    }
> +
> +    if (isSearchInput || isReplaceAll) {
> +      node.select();
> +    }

So, here, please comment that this call to node.select is the precise reason why we reimplement search key shortcut.

::: devtools/client/sourceeditor/editor.js:1059
(Diff revision 8)
> +
> +    if (isSearchInput || isReplaceAll) {
> +      node.select();
> +    }
> +
> +    cm.execCommand("find");

And that here, we call find command as we prevent the propagation of the event and cancel codemirror key handling.

::: devtools/client/sourceeditor/editor.js:1082
(Diff revision 8)
> +        posFrom: null,
> +        posTo: null,
> +        overlay: null,
> +        query
> +      };
> +    }

Same here for replace, that cm.state.search allows to automatically start searching for the next occurance.
Otherwise, without this patch, we first have to Hit enter after typing a text to search before this key shortcut actually starts working.
i.e. with this hack, we can immediately search for next occurance after typing a word to search.

::: devtools/client/sourceeditor/editor.js:1088
(Diff revision 8)
> +
> +    if (isFindPrev) {
> +      cm.execCommand("findPrev");
> +    } else {
> +      cm.execCommand("findNext");
> +    }

Also the same story here, we have to call the related commands as we cancel codemirror key handling.

::: devtools/client/sourceeditor/editor.js:1319
(Diff revision 8)
> +        this.findNextOrPrev(node, false);
> +        break;
> +      case "delete.key":
> +        if (this.somethingSelected()) {
> +          cm.execCommand("delCharAfter");
> +        }

Do you know which test fail before of this one?
It would be great to also justify why we reimplement this.
Flags: needinfo?(poirot.alex)
Comment on attachment 8794087 [details]
Bug 1292592 - Convert sourceeditor to use key shortcuts module;

https://reviewboard.mozilla.org/r/80700/#review81372

> Do you know which test fail before of this one?
> It would be great to also justify why we reimplement this.

`delete` doesn't fail any test, I'll just remove it
for comment 91 I think it might be more precise with message like "inject script is only supported in chrome privilege"?
Flags: needinfo?(ntim.bugs)
removed try block since we could provide the really alternative in followup de-chome patch
Flags: needinfo?(ntim.bugs)
Comment on attachment 8794087 [details]
Bug 1292592 - Convert sourceeditor to use key shortcuts module;

https://reviewboard.mozilla.org/r/80700/#review81644

Looks good. Thanks a lot for cleaning up this code!
Attachment #8794087 - Flags: review?(poirot.alex) → review+
thanks all for review & feedback!
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7d6959ca82f5
load sourceeditor in inspector without iframe;r=bgrins
https://hg.mozilla.org/integration/autoland/rev/5bf1cf5d2c58
isolate xul dependency in sourceeditor and theme-switching;r=bgrins
https://hg.mozilla.org/integration/autoland/rev/e0ef5898308b
Convert sourceeditor to use key shortcuts module;r=ochameau
Keywords: checkin-needed
Talos has detected a Firefox performance regression from push e0ef5898308b07ceb17f5bbf2192a5d496c7b6e7. As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

  1%  damp summary windows7-32 pgo     256.92 -> 259.76

You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=3741


Hi Fred, the initial alert we noticed is https://treeherder.mozilla.org/perf.html#/alerts?id=3642, which is a merged push, so I did some retriggers and found the root caused push might be one of following patches:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5962dad680d46f59733ecd4d085ec9a0af389d86&tochange=e0ef5898308b07ceb17f5bbf2192a5d496c7b6e7
(only happened in PGO build)

Here is the compare result with the previous push:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=autoland&originalRevision=5962dad680d46f59733ecd4d085ec9a0af389d86&newProject=autoland&newRevision=e0ef5898308b07ceb17f5bbf2192a5d496c7b6e7&originalSignature=3bbad54b048b2d9e6bdcf1b771c21ea9d9eac4d5&newSignature=3bbad54b048b2d9e6bdcf1b771c21ea9d9eac4d5&framework=1

Is the regression expected? Do you have any ideas about this? Thank you!
Flags: needinfo?(gasolin)
Blocks: 1302124
I think the main difference is we introduce keyshortcut in new manner. We just introduce keyshortcut in netmonitor as well at bug 1268444, please monitor the performance and see if they have similar performance impact
Flags: needinfo?(gasolin) → needinfo?(ashiue)
(In reply to Fred Lin [:gasolin] from comment #112)
> I think the main difference is we introduce keyshortcut in new manner. We
> just introduce keyshortcut in netmonitor as well at bug 1268444, please
> monitor the performance and see if they have similar performance impact

OK, I will keep an eye on that, thank you!
Flags: needinfo?(ashiue)
Hi!
Can you please give me some details on how I should manually verify this bug?
Thank you!
Sorry I just come back from the conference. Here's the steps to test this bug:

1. Open developer > Inspector from the menu in start page
2. select any element on the page with inspector tool, the related source code will be highlighted
3. right click to open the context menu, choose `edit HTML`
4. the editor will be embedded inside of source code view

Then compare v52/nightly and previous version to check:

1. the visibility are the same
2. the editor's shortcuts are the same (ctrl+], ctrl+[ for indent, ctrl+z for redo, ctrl+f, ctrl+alt+f/ctrl+g for find next/previous/replace)
Flags: needinfo?(gasolin)
Thank you for clarifying!

I used Fx 51.0a1, build ID: 20160805030444, and Fx 52.0a1, build ID: 20161113030203 for the comparison.
I checked the visibility, shortcuts and some additional exploratory testing. No issues were found. I verified on Windows 10 x64, Mac OS X 10.11 and Ubuntu 14.04 LTS. I am marking this issue as verified.  

Cheers!
Status: RESOLVED → VERIFIED
status-firefox52: fixed → verified
Flags: qe-verify+
Depends on: 1397366
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: