660784 - Add the Orion source code editor to the browser

Status: VERIFIED FIXED

(DevTools :: General, defect)

Description

[Mihai Sucan [:msucan]]

We need to add a source code editor to the browser so we can use it in the developer tools we are working on.

Comment 1

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Use Ace

Comment 2

[Mihai Sucan [:msucan]]

Attachment: first WIP (orion)

This is the first Orion integration work-in-progress patch.

This code works and can be used. Will provide an example use of Orion in bug 636727.

Details:

- Added orion to browser/base/content/orion.
- Added a LICENSE which contains a copy of EDL and a README.
- Added a Makefile.dryice.js script which needs to be run in the orion.client/bundles folder from the original Orion codebase. This script uses Dryice to make a build of the Orion editor component, as we need it for our purposes.
- The script builds orion.js (the original source code) and orion.min.js which is minified.
- In browser/base/jar.mn one can choose to use orion.js or orion.min.js. For development purposes, we can use the unpacked script. It's really convenient to use this approach.
- Syntax highlighting for JS, CSS and Java is available.
- One can start Orion in readonly mode.


Concerns:

- I am not sure if the license attribution is correct. I am not even sure if the Orion editor component is EDL, but it seems so. :) The Orion client codebase has no license file, just the EDL header in the source files.

- The README I added is rather poor. Suggestions for changes are very much welcome.

- When orion.css is loaded it always causes a syntax error to be shown in the Error Console, on the first line. This is weird, needs further investigation. Otherwise Orion runs and renders fine...

- No line-wrapping features in Orion. We need this. If assigned by Kevin, I can work on this improvement for Orion.

- No context menu support. We need context menu support for Scratchpad. I added this feature to Orion. Will submit my first patch to the Orion devs. We'll see how upstream contribs work.

This is the only change I made to Orion, actually. Otherwise it runs fine.

- Styling is a bit weird. I think we don't want odd and even line numbers to render differently. :)

- To keep Orion changes to a minimum source-editor.jsm adds a XUL iframe to the target document, then orion.js is loaded into the iframe contentWindow.  The iframe document is an HTML document.

- Nesting of iframes seems to be no problem, actually. It's ugly from a tech POV, but it works fine.

- If we don't like the technical ugliness we need to:

* fix support for XHTML documents in Orion. It currently does createElement("FOO") (note the upper case tag name) which breaks things. XHTML documents, even if they have the default XHTML namespace, they require tag names to be lower case.

This is really easy to fix in Orion.

* fix support for XUL documents. This depends on the work in the previous point, but with additional logic: we need to use createElementNS(XHTML_NS, "foo") whenever possible. This is needed because the default namespace is not the XHTML one in XUL documents. Beyond this, we need to ensure that objects like document.body are not used (XUL documents have no body element), or that we provide technically feasible alternatives.

This work is more involved, but it's really doable.

- We may just leave it as it is, if it doesn't bother us.

- The xul:iframe I add Orion to needs chrome privileges. This is ugly, but with content privileges Orion cannot call document.open() and document.write() in the nested iframe it creates. Security violation. Surprisingly, this works in a web page, uh oh. I don't know why there's this difference.

Any thoughts? I wanted to use type=content on the xul:iframe...

- We need a better story for keyboard shortcuts. See how things turned out in bug 636727.

- We need a fallback story here. Developer users of SourceEditor shouldn't bother with anything. They should just use the SourceEditor and that's all. We should have, in SourceEditor, a fallback mechanism to use a simple textbox only, but expose the same API to the users of the component. We should also include a devtools pref to allow people to toggle off the fancy editor. Thoughts?


Things TODO:

- Add more API, if we need more. I added almost all of the API needed for Scratchpad.

- Add destroy() to SourceEditor, to make sure we won't leak objects.

- Add insertText() to SourceEditor.

Currently the Orion API has a public setText() method which allows us to replace the whole editor content. There is a private _doContent() method that allows the *insertion* of text, but it's not public, so I cannot use it.

Scratchpad needs insertText() for the Display option - to insert the resulting text. At the moment I use setText() but this is suboptimal (see how undo/redo works - it behaves like all the text changed).

- We probably need an additional stylesheet, for bright text on dark background, as an option. Still, we should keep dark text on bright background (like now) as the default.

- Properly comment the SourceEditor component source code.

- Add mochitests for the new component.

Anything else?


As always, looking forward to your feedback!

Comment 3

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

(In reply to comment #2)
> Created attachment 536274 [details] [diff] [review] [review]
> first WIP (orion)

Mihai, can you explain why you're using Orion and not Ace,
a project officially supported by Mozilla?

Comment 4

[Kevin Dangoor]

(In reply to comment #3)
> Mihai, can you explain why you're using Orion and not Ace,
> a project officially supported by Mozilla?

https://wiki.mozilla.org/DevTools/Features/CodeEditor#Evaluating_the_Options

Comment 5

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

updating the summary to reflect reality. This isn't an exercise in deciding which editor to use anymore.

Comment 6

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

(In reply to comment #2)
> Created attachment 536274 [details] [diff] [review] [review]
> first WIP (orion)
> 
> This is the first Orion integration work-in-progress patch.
> 
> This code works and can be used. Will provide an example use of Orion in bug
> 636727.
> 
> Details:
> 
> - Added orion to browser/base/content/orion.
> - Added a LICENSE which contains a copy of EDL and a README.
> - Added a Makefile.dryice.js script which needs to be run in the
> orion.client/bundles folder from the original Orion codebase. This script
> uses Dryice to make a build of the Orion editor component, as we need it for
> our purposes.

Really? This is a pretty specialized build system. I realize this is a component, but it means future attempts to update the code will rely on it. Where does dryice live? How does it run? Etc.

> - The script builds orion.js (the original source code) and orion.min.js
> which is minified.

We don't generally include minified code in the browser.

> - In browser/base/jar.mn one can choose to use orion.js or orion.min.js. For
> development purposes, we can use the unpacked script. It's really convenient
> to use this approach.

And it gets problematic when trying to debug a minified version.

> - Syntax highlighting for JS, CSS and Java is available.

I don't think we need Java highlighting.

> - One can start Orion in readonly mode.
> 
> 
> Concerns:
> 
> - I am not sure if the license attribution is correct. I am not even sure if
> the Orion editor component is EDL, but it seems so. :) The Orion client
> codebase has no license file, just the EDL header in the source files.

That's definitely something we need to be sure about before we can proceed. Can we get some clarification here?

> - The README I added is rather poor. Suggestions for changes are very much
> welcome.

I will take a look!

> - When orion.css is loaded it always causes a syntax error to be shown in
> the Error Console, on the first line. This is weird, needs further
> investigation. Otherwise Orion runs and renders fine...
> 
> - No line-wrapping features in Orion. We need this. If assigned by Kevin, I
> can work on this improvement for Orion.

It should definitely be an option, yes.

> - No context menu support. We need context menu support for Scratchpad. I
> added this feature to Orion. Will submit my first patch to the Orion devs.
> We'll see how upstream contribs work.

A good test!

> This is the only change I made to Orion, actually. Otherwise it runs fine.
> 
> - Styling is a bit weird. I think we don't want odd and even line numbers to
> render differently. :)

mm. no. Not by default anyway.

> - To keep Orion changes to a minimum source-editor.jsm adds a XUL iframe to
> the target document, then orion.js is loaded into the iframe contentWindow. 
> The iframe document is an HTML document.

as we discussed. Good.

> - Nesting of iframes seems to be no problem, actually. It's ugly from a tech
> POV, but it works fine.
> 
> - If we don't like the technical ugliness we need to:
> 
> * fix support for XHTML documents in Orion. It currently does
> createElement("FOO") (note the upper case tag name) which breaks things.
> XHTML documents, even if they have the default XHTML namespace, they require
> tag names to be lower case.
> 
> This is really easy to fix in Orion.

Yes, doesn't sound like a big deal for us, but I have a feeling it may be necessary for some of the other consumers of Orion. We might not be able to upstream those changes. It would be good to hear if that's a problem.

> * fix support for XUL documents. This depends on the work in the previous
> point, but with additional logic: we need to use createElementNS(XHTML_NS,
> "foo") whenever possible. This is needed because the default namespace is
> not the XHTML one in XUL documents. Beyond this, we need to ensure that
> objects like document.body are not used (XUL documents have no body
> element), or that we provide technically feasible alternatives.
> 
> This work is more involved, but it's really doable.

Again, doing these types of changes will make our port an orphan and require ongoing maintenance. It makes incorporating upstream changes very difficult.

> - We may just leave it as it is, if it doesn't bother us.

We'll work with that to start and see how far we can go.

> - The xul:iframe I add Orion to needs chrome privileges. This is ugly, but
> with content privileges Orion cannot call document.open() and
> document.write() in the nested iframe it creates. Security violation.
> Surprisingly, this works in a web page, uh oh. I don't know why there's this
> difference.

It uses document.write()? Ew.

> Any thoughts? I wanted to use type=content on the xul:iframe...

I do have some thoughts, but I'll keep them to myself for the time-being.

> - We need a better story for keyboard shortcuts. See how things turned out
> in bug 636727.

Noted. Already read your concerns there.

> - We need a fallback story here. Developer users of SourceEditor shouldn't
> bother with anything. They should just use the SourceEditor and that's all.
> We should have, in SourceEditor, a fallback mechanism to use a simple
> textbox only, but expose the same API to the users of the component. We
> should also include a devtools pref to allow people to toggle off the fancy
> editor. Thoughts?

That's ... tricky. I guess it depends on how much of an API we need to build to mask out Orion in the SourceEditor. If it's relatively simple, it's feasible. I could see handling graceful fallback being a pretty complicated thing to do though.

> Things TODO:
> 
> - Add more API, if we need more. I added almost all of the API needed for
> Scratchpad.

That's a good start anyway. It's needs are relatively simple.

> - Add destroy() to SourceEditor, to make sure we won't leak objects.
> 
> - Add insertText() to SourceEditor.
> 
> Currently the Orion API has a public setText() method which allows us to
> replace the whole editor content. There is a private _doContent() method
> that allows the *insertion* of text, but it's not public, so I cannot use it.
> 
> Scratchpad needs insertText() for the Display option - to insert the
> resulting text. At the moment I use setText() but this is suboptimal (see
> how undo/redo works - it behaves like all the text changed).

Sounds like we'll need another feature added.

> - We probably need an additional stylesheet, for bright text on dark
> background, as an option. Still, we should keep dark text on bright
> background (like now) as the default.

Ideally yes, but I wouldn't block on that. Could be done in a followup. Supporting additional style formats would also be nice, but definitely sounds like a follow-up feature.

> - Properly comment the SourceEditor component source code.
> 
> - Add mochitests for the new component.
> 
> Anything else?

That's a pretty solid list of TODOs already. I'll see if I can come up with some more after reading the code.

> As always, looking forward to your feedback!

I'm looking forward to giving you some!

Comment 7

[Mihai Sucan [:msucan]]

(In reply to comment #6)
> (In reply to comment #2)
> > Created attachment 536274 [details] [diff] [review] [review] [review]
> > first WIP (orion)
> > 
> > This is the first Orion integration work-in-progress patch.
> > 
> > This code works and can be used. Will provide an example use of Orion in bug
> > 636727.
> > 
> > Details:
> > 
> > - Added orion to browser/base/content/orion.
> > - Added a LICENSE which contains a copy of EDL and a README.
> > - Added a Makefile.dryice.js script which needs to be run in the
> > orion.client/bundles folder from the original Orion codebase. This script
> > uses Dryice to make a build of the Orion editor component, as we need it for
> > our purposes.
> 
> Really? This is a pretty specialized build system. I realize this is a
> component, but it means future attempts to update the code will rely on it.
> Where does dryice live? How does it run? Etc.

Dryice is a Mozilla project. It's available as a nodejs script. It is available here:

https://github.com/mozilla/dryice

We need to add this info to the README. Good questions.

I used dryice because it's a convenient automated way to package Orion into a single file (with optional minification). It's not needed per-se, but I think we should put it here, with info inside README, such that other devs who want to update Orion know how we got the orion.js file. We can't really say "hey, copy/paste these files, from these folders, then join them, ..." and so on.

Is this fine for you? Or do you think we should do things differently?


> > - The script builds orion.js (the original source code) and orion.min.js
> > which is minified.
> 
> We don't generally include minified code in the browser.

True. Do you think I should drop orion.min.js and we only use the big orion.js file?

> > - Syntax highlighting for JS, CSS and Java is available.
> 
> I don't think we need Java highlighting.

True, but it comes in the same file. Can't split them apart without forking the Orion editor project. ;)

> > Concerns:
> > 
> > - I am not sure if the license attribution is correct. I am not even sure if
> > the Orion editor component is EDL, but it seems so. :) The Orion client
> > codebase has no license file, just the EDL header in the source files.
> 
> That's definitely something we need to be sure about before we can proceed.
> Can we get some clarification here?

I believe Kevin can help us here. Kevin, do you have some thoughts here?


> > - When orion.css is loaded it always causes a syntax error to be shown in
> > the Error Console, on the first line. This is weird, needs further
> > investigation. Otherwise Orion runs and renders fine...
> > 
> > - No line-wrapping features in Orion. We need this. If assigned by Kevin, I
> > can work on this improvement for Orion.
> 
> It should definitely be an option, yes.

Yeah, but as I understand it's not a "deal breaker". Line wrapping will surely come soon to Orion, even if we don't provide upstream patches.


> > - No context menu support. We need context menu support for Scratchpad. I
> > added this feature to Orion. Will submit my first patch to the Orion devs.
> > We'll see how upstream contribs work.
> 
> A good test!

Yep, and I am glad to see they are responsive!


> > - Nesting of iframes seems to be no problem, actually. It's ugly from a tech
> > POV, but it works fine.
> > 
> > - If we don't like the technical ugliness we need to:
> > 
> > * fix support for XHTML documents in Orion. It currently does
> > createElement("FOO") (note the upper case tag name) which breaks things.
> > XHTML documents, even if they have the default XHTML namespace, they require
> > tag names to be lower case.
> > 
> > This is really easy to fix in Orion.
> 
> Yes, doesn't sound like a big deal for us, but I have a feeling it may be
> necessary for some of the other consumers of Orion. We might not be able to
> upstream those changes. It would be good to hear if that's a problem.
> 
> > * fix support for XUL documents. This depends on the work in the previous
> > point, but with additional logic: we need to use createElementNS(XHTML_NS,
> > "foo") whenever possible. This is needed because the default namespace is
> > not the XHTML one in XUL documents. Beyond this, we need to ensure that
> > objects like document.body are not used (XUL documents have no body
> > element), or that we provide technically feasible alternatives.
> > 
> > This work is more involved, but it's really doable.
> 
> Again, doing these types of changes will make our port an orphan and require
> ongoing maintenance. It makes incorporating upstream changes very difficult.

The point is that we shall do these changes *only* if they are accepted upstream. Otherwise, we are going to basically maintain a fork of the Orion editor - which is what we are going to avoid as much as possible.

Based on discussions with the Orion team, they are open for such changes. So, we just have to do them. Do you want me to embark on this "adventure" or do you believe the current iframe solution is good enough? Before working on it, I thought the iframe solution might cause problems for us, but I am glad and surprised to see it works really well. Now I don't think we should bother unless it's really needed.

Other consumers of Orion within the Mozilla codebase will have to rely on source-editor.jsm - the glue code. They can be XUL extensions or anything else. I expect they won't have problems.

Other consumers of Orion outside Mozilla will of course benefit from our upstream contributions, if we make them. It will suddenly become easier to integrate Orion into a wider range of environments.


> > - We may just leave it as it is, if it doesn't bother us.
> 
> We'll work with that to start and see how far we can go.

Yep. To make sure, you also agree we go forward with the xul:iframe workaround, and we see how far we can go with it?

> > - The xul:iframe I add Orion to needs chrome privileges. This is ugly, but
> > with content privileges Orion cannot call document.open() and
> > document.write() in the nested iframe it creates. Security violation.
> > Surprisingly, this works in a web page, uh oh. I don't know why there's this
> > difference.
> 
> It uses document.write()? Ew.

Yes. :)


> > - We need a fallback story here. Developer users of SourceEditor shouldn't
> > bother with anything. They should just use the SourceEditor and that's all.
> > We should have, in SourceEditor, a fallback mechanism to use a simple
> > textbox only, but expose the same API to the users of the component. We
> > should also include a devtools pref to allow people to toggle off the fancy
> > editor. Thoughts?
> 
> That's ... tricky. I guess it depends on how much of an API we need to build
> to mask out Orion in the SourceEditor. If it's relatively simple, it's
> feasible. I could see handling graceful fallback being a pretty complicated
> thing to do though.

The initial idea was that we should be able to pref off the source editor, so users get the simple textbox, if we decide it's not ready for primetime. We can't do this trivially with such involved changes in Scratchpad and other devtools. We'll need to make a SourceEditor-look-a-like with the same API that uses a textbox, fakes stuff, and does noops for things like asking for JS syntax highlighting and so on.

Thoughts?


> > - We probably need an additional stylesheet, for bright text on dark
> > background, as an option. Still, we should keep dark text on bright
> > background (like now) as the default.
> 
> Ideally yes, but I wouldn't block on that. Could be done in a followup.
> Supporting additional style formats would also be nice, but definitely
> sounds like a follow-up feature.

True, but it's easy to add. The Orion project already has the alternative stylesheet. Not sure how we should expose it to users? I could add a pref... Or should we not bother at all about this?

Anyhow, the SourceEditor API already allows XUL extensions to provide alternate styles - they only need to give us a chrome URL to the CSS file they want.


> > As always, looking forward to your feedback!
> 
> I'm looking forward to giving you some!

Thank you!

Comment 8

[Shawn Wilsher :sdwilsh]

(In reply to comment #7)
> I used dryice because it's a convenient automated way to package Orion into a
> single file (with optional minification). It's not needed per-se, but I think
> we should put it here, with info inside README, such that other devs who want
> to update Orion know how we got the orion.js file. We can't really say "hey,
> copy/paste these files, from these folders, then join them, ..." and so on.
Yes, please make sure upgrading instructions are very clear.  We do this for SQLite:
https://mxr.mozilla.org/mozilla-central/source/db/sqlite3/README.MOZILLA

> > We don't generally include minified code in the browser.
> 
> True. Do you think I should drop orion.min.js and we only use the big orion.js
> file?
I'm not sure if it matters if it's minified if we are just taking source drops.

> The point is that we shall do these changes *only* if they are accepted
> upstream. Otherwise, we are going to basically maintain a fork of the Orion
> editor - which is what we are going to avoid as much as possible.
I can say that having a patched local copy makes updates hard, and thus makes them less likely to happen.  The less you have patched in our tree, the better.

Comment 9

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

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #2)
> > > Created attachment 536274 [details] [diff] [review] [review] [review] [review]
> > > first WIP (orion)
> > > 
> > > This is the first Orion integration work-in-progress patch.
> > > 
> > > This code works and can be used. Will provide an example use of Orion in bug
> > > 636727.
> > > 
> > > Details:
> > > 
> > > - Added orion to browser/base/content/orion.
> > > - Added a LICENSE which contains a copy of EDL and a README.
> > > - Added a Makefile.dryice.js script which needs to be run in the
> > > orion.client/bundles folder from the original Orion codebase. This script
> > > uses Dryice to make a build of the Orion editor component, as we need it for
> > > our purposes.
> > 
> > Really? This is a pretty specialized build system. I realize this is a
> > component, but it means future attempts to update the code will rely on it.
> > Where does dryice live? How does it run? Etc.
> 
> Dryice is a Mozilla project. It's available as a nodejs script. It is
> available here:
> 
> https://github.com/mozilla/dryice
> 
> We need to add this info to the README. Good questions.

And, as Shawn suggested, we could even add a local README.

> I used dryice because it's a convenient automated way to package Orion into
> a single file (with optional minification). It's not needed per-se, but I
> think we should put it here, with info inside README, such that other devs
> who want to update Orion know how we got the orion.js file. We can't really
> say "hey, copy/paste these files, from these folders, then join them, ..."
> and so on.
> 
> Is this fine for you? Or do you think we should do things differently?

No, dryice is ok, I think. GCLI will be using it as well.

> > We don't generally include minified code in the browser.
> 
> True. Do you think I should drop orion.min.js and we only use the big
> orion.js file?

I think I'd prefer it. Unless the minified version gives us significant performance improvements, I think I'd prefer a full version.

> > > - Syntax highlighting for JS, CSS and Java is available.
> > 
> > I don't think we need Java highlighting.
> 
> True, but it comes in the same file. Can't split them apart without forking
> the Orion editor project. ;)

yeah, that's fine. You could ask to upstream a patch that modularizes these files but I wouldn't block on it.

> > > Concerns:
> > > 
> > > - I am not sure if the license attribution is correct. I am not even sure if
> > > the Orion editor component is EDL, but it seems so. :) The Orion client
> > > codebase has no license file, just the EDL header in the source files.
> > 
> > That's definitely something we need to be sure about before we can proceed.
> > Can we get some clarification here?
> 
> I believe Kevin can help us here. Kevin, do you have some thoughts here?

I believe it's OK, but I'd like confirmation.

> > > - When orion.css is loaded it always causes a syntax error to be shown in
> > > the Error Console, on the first line. This is weird, needs further
> > > investigation. Otherwise Orion runs and renders fine...
> > > 
> > > - No line-wrapping features in Orion. We need this. If assigned by Kevin, I
> > > can work on this improvement for Orion.
> > 
> > It should definitely be an option, yes.
> 
> Yeah, but as I understand it's not a "deal breaker". Line wrapping will
> surely come soon to Orion, even if we don't provide upstream patches.

Yeah, not a deal-breaker.

[snip]

> > Again, doing these types of changes will make our port an orphan and require
> > ongoing maintenance. It makes incorporating upstream changes very difficult.
> 
> The point is that we shall do these changes *only* if they are accepted
> upstream. Otherwise, we are going to basically maintain a fork of the Orion
> editor - which is what we are going to avoid as much as possible.

Yeah, that would be a problem.

> Based on discussions with the Orion team, they are open for such changes.
> So, we just have to do them. Do you want me to embark on this "adventure" or
> do you believe the current iframe solution is good enough? Before working on
> it, I thought the iframe solution might cause problems for us, but I am glad
> and surprised to see it works really well. Now I don't think we should
> bother unless it's really needed.
> 
> Other consumers of Orion within the Mozilla codebase will have to rely on
> source-editor.jsm - the glue code. They can be XUL extensions or anything
> else. I expect they won't have problems.
> 
> Other consumers of Orion outside Mozilla will of course benefit from our
> upstream contributions, if we make them. It will suddenly become easier to
> integrate Orion into a wider range of environments.

Indeed. Local changes should be avoided as much as possible. In fact, if we need to make local changes, I'd have a harder time giving this a pass.

> > > - We may just leave it as it is, if it doesn't bother us.
> > 
> > We'll work with that to start and see how far we can go.
> 
> Yep. To make sure, you also agree we go forward with the xul:iframe
> workaround, and we see how far we can go with it?

Sure, we can as an experiment.

> > > - We need a fallback story here. Developer users of SourceEditor shouldn't
> > > bother with anything. They should just use the SourceEditor and that's all.
> > > We should have, in SourceEditor, a fallback mechanism to use a simple
> > > textbox only, but expose the same API to the users of the component. We
> > > should also include a devtools pref to allow people to toggle off the fancy
> > > editor. Thoughts?
> > 
> > That's ... tricky. I guess it depends on how much of an API we need to build
> > to mask out Orion in the SourceEditor. If it's relatively simple, it's
> > feasible. I could see handling graceful fallback being a pretty complicated
> > thing to do though.
> 
> The initial idea was that we should be able to pref off the source editor,
> so users get the simple textbox, if we decide it's not ready for primetime.
> We can't do this trivially with such involved changes in Scratchpad and
> other devtools. We'll need to make a SourceEditor-look-a-like with the same
> API that uses a textbox, fakes stuff, and does noops for things like asking
> for JS syntax highlighting and so on.
> 
> Thoughts?

That sounds like a lot of work.

Would #IFDEFs in makefiles be an easier route to provide a backout option? It's not ideal, but I'm not really liking the sound of creating all those codepaths just for the sake of being able to turn off this editor.

> > > - We probably need an additional stylesheet, for bright text on dark
> > > background, as an option. Still, we should keep dark text on bright
> > > background (like now) as the default.
> > 
> > Ideally yes, but I wouldn't block on that. Could be done in a followup.
> > Supporting additional style formats would also be nice, but definitely
> > sounds like a follow-up feature.
> 
> True, but it's easy to add. The Orion project already has the alternative
> stylesheet. Not sure how we should expose it to users? I could add a pref...
> Or should we not bother at all about this?

I'd leave it for now. Let's focus on the basics.

> Anyhow, the SourceEditor API already allows XUL extensions to provide
> alternate styles - they only need to give us a chrome URL to the CSS file
> they want.

Cool!

In addition to the above, now we have to deal with the possibility of including dojo in our sourcecode. This is going to add some serious complexity to this import and potentially be an issue for import. We've already punted on attempts to import jQuery into Firefox code before and went so far as to create a local, stripped down version for Panorama.

While I understand Dojo is a fairly modular toolkit, it's still an extra chunk of code we'll have to vet.

I ran out of time this week so I can't really manage a full review of the attached code, I'm hopeful we can address or overcome the above issues.

I'd love to get a build together with this included so we can pass it around between our i18n and a11y experts to play with.

Comment 10

[Mihai Sucan [:msucan]]

Attachment: second WIP

This is the latest WIP patch. This includes work I did about a week ago, when I switched to other higher priority work.

Changes since the first patch:

- EDL license is correct, according to Kevin.
- Added more info to README.
- Added an UPGRADE file that explains how one can update Orion to newer versions.
- Fixed the bug which caused orion.css to show an error when it loaded in Scratchpad. (fixed upstream, in Orion)
- Updated to a newer Orion codebase.
- Fixed the styling of the line numbers gutter.
- Added more APIs we need.
  - insertText() API was actually available as setText() in Orion.
  - Added destroy().
- Added configuration options: readonly mode, tab size, theme (stylesheet, just point to a CSS), enable/disable line numbers, syntax highlighting type, placeholder text, undo limit, and so on.

Still in TODO:

- Properly comment the SourceEditor component source code.
- Add mochitests for the new component.
- Look into a better solution for keyboard shortcuts.
- Decide on the fallback problem.
- Address feedback comments.

Comment 11

[Mihai Sucan [:msucan]]

Attachment: proposed patch

Comment 12

[Mihai Sucan [:msucan]]

I forgot to comment about the new patch:

- updated Orion to the latest upstream code.
- included an upstream fix for keyboard shortcuts. We can now use our XUL keybindings.
- added support for expand tabs to spaces, and configurable tab size.
- added more APIs.
- commented the source code.
- added a mochitest for the new component which tests some of the editing features and events.
- initialization and destroy() fixes.

Please note that Orion is no longer minified (this is a change I forgot to mention when I posted the second WIP patch), as was requested in one of the comments above.

Comment 13

[Mihai Sucan [:msucan]]

Attachment: updated patch

Updated the patch.

Changes:

- updated Orion to the latest upstream github clone.
- added support for HTML syntax highlighting, from Orion's codebase.
  - this is thanks to an upstream fix which removes the dojo dependency of the relevant code.
- added debug output - a few dump() calls to help me track down the issues with new lines. It seems to be a bug I cannot reproduce, but Alex and Rob can, see bug 636727 comment 14.
- added support for indentation/unindentation of multiple lines at once with tab/shift-tab (selected lines).
- added support for automatic indentation based on the previous line indentation level.
- improved the default keyboard bindings for Mac users, to better match the Firefox native shortcuts. This code is going upstream to Orion.
- hopefully fixed the "child is undefined" error reported by Robert in bug 636727 comment 15. This is an upstream fix.
- added API for grouping changes so Undo/Redo work on the entire set of changes at once. This really exposes functionality we need at times from Orion.
- added API to read the current caret line number, and to jump to a different line number.
- updated the tests accordingly.

Looking forward to the review. Thank you!


(please don't mind the dump() calls, they are only temporary)

Comment 14

[Mihai Sucan [:msucan]]

Attachment: patch update 5

Updated patch. Minor changes:

- removed the debug output (the dump() calls).
- fixed the tests to pass on all systems. Previous patch had some minor failures. This new patch has already gone through the try server. All tests passed.
  - for this I had to add API: getLineCount() and getLineDelimiter().
- updated to the latest Orion codebase. This includes all the upstream fixes we submitted.
  - this fixes Mac key bindings for good and the Firefox on Ubuntu line height issues.

Looking forward to your reviews. Thank you!

Gavin: please note that Ehsan wants to review some of the Orion internals, but in the mean time, we'd very much appreciate if we could have some progress on these patches - that is, your review of the integration code. Thanks!

Comment 15

[(no longer active)]

Comment on attachment 544476 [details] [diff] [review]
patch update 5

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

In general, I don't see how we're going to make things work with the current TextView implementation.  TextView is not a small piece of code, but it's the most problematic one.  On the other hand, a lot of the stuff that TextView implements is already implemented in Gecko, so maybe reimplementing it to just use the built-in stuff is not a very crazy idea?

Below you can find a more detailed set of issues.  I hope that I have not forgotten anything.  :-)

::: browser/base/content/orion/orion.js
@@ +1401,5 @@
> +			node.detachEvent("on" + type, handler);
> +		}
> +	}
> +	var isIE = document.selection && window.ActiveXObject && /MSIE/.test(navigator.userAgent) ? document.documentMode : undefined;
> +	var isFirefox = parseFloat(navigator.userAgent.split("Firefox/")[1] || navigator.userAgent.split("Minefield/")[1]) || undefined;

This is the sort of thing which is embarrassing for us to have in our tree.  Let's look for "Gecko" instead.

@@ +1420,5 @@
> +	 * 
> +	 * @class A Selection represents a range of selected text in the view.
> +	 * @name orion.textview.Selection
> +	 */
> +	var Selection = (function() {

I think this library should be changed to include the built-in selection APIs (window.getSelection() and friends).

@@ +3063,5 @@
> +						e.preventDefault();
> +				}
> +			}
> +		},
> +		_handleMouseMove: function (e) {

We shouldn't need to hittest here, if we would let the browser do what it's supposed to do...

@@ +3151,5 @@
> +				*/
> +				if (isW3CEvents) { this._setGrab(null); }
> +			}
> +		},
> +		_handleMouseWheel: function (e) {

Handling scrolling is a pain to get right, and I don't think we should attempt to do that here.

@@ +3310,5 @@
> +			var rect = this._frame.getBoundingClientRect();
> +			var body = this._parentDocument.body;
> +			return {left: touch.clientX - rect.left - body.scrollLeft, top: touch.clientY - rect.top - body.scrollTop};
> +		},
> +		_handleTouchStart: function (e) {

Somebody familiar with touch events should look at this code...

@@ +3366,5 @@
> +			}
> +			//Cannot prevent to show maginifier
> +//			e.preventDefault();
> +		},
> +		_handleTouchMove: function (e) {

Ditto.

@@ +3496,5 @@
> +				}
> +			}
> +			return false;
> +		},
> +		_doBackspace: function (args) {

This is bad.  I bet that it doesn't handle Unicode surrogates correctly.  We should also test to see whether it handles ligatures correctly or not.  This is an example of something which should not be duplicated in Orion.

@@ +3527,5 @@
> +				return this._setClipboardText(text, e);
> +			}
> +			return true;
> +		},
> +		_doCursorNext: function (args) {

This code doesn't handle bidi caret navigation.  Again, this is something that Orion should rely on the browser to do.

@@ +3546,5 @@
> +			if (!args.select) { selection.collapse(); }
> +			this._setSelection(selection, true);
> +			return true;
> +		},
> +		_doCursorPrevious: function (args) {

Ditto.

@@ +3574,5 @@
> +				return this._setClipboardText(text, e);
> +			}
> +			return true;
> +		},
> +		_doDelete: function (args) {

This doesn't handle a bunch of things correctly, such as deleting clusters and not characters...

@@ +3646,5 @@
> +			if (!args.select) { selection.collapse(); }
> +			this._setSelection(selection, true);
> +			return true;
> +		},
> +		_doLineDown: function (args) {

These methods (linedown/up and pagedown/up) have a different behavior to the built-in stuff for these, and also have bugs such as not scrolling properly.

@@ +3922,5 @@
> +			}
> +			this._setSelection(selection, true);
> +			return true;
> +		},
> +		_commitIME: function () {

This needs to be looked at by somebody who is familiar with IME (Masayuki perhaps).  I'm not sure how well this would work with the built-in IME support.

@@ +3972,5 @@
> +					addTextFunc(text.substring(start, end));
> +					addDelimiterFunc();
> +				}
> +		},
> +		_createActions: function () {

Lots of the actions here are implemented natively.  I'd really rather Orion to use the native actions than handle everything on its own...

@@ +4449,5 @@
> +							if (text) { self._doContent(text); }
> +						}, 0);
> +						return null;
> +					} else {
> +						/* no event and no clipboard permission, paste can't be performed */

I'm not sure if this would be a problem in practice.

@@ +4544,5 @@
> +				return this._getOffset_W3C(offset, unit, direction);
> +			}
> +			return isIE ? this._getOffset_IE(offset, unit, direction) : this._getOffset_W3C(offset, unit, direction);
> +		},
> +		_getOffset_W3C: function (offset, unit, direction) {

This code again duplicates what the browser already does. :(

@@ +4758,5 @@
> +				}
> +				return newRects;
> +			}
> +			var logicalXDPI = isIE ? window.screen.logicalXDPI : 1;
> +			var deviceXDPI = isIE ? window.screen.deviceXDPI : 1;

This seems to be IE specific, which could make this code misbehave with different zooming levels and device/logical DPIs.

@@ +4818,5 @@
> +								range.move("character", start);
> +								range.moveEnd("character", end - start);
> +							}
> +							rect = range.getClientRects()[0];
> +							//TODO test for character trailing (wrong for bidi)

This should probably be looked at?

@@ +4838,5 @@
> +							var rangeChild = lineChild.firstChild;
> +							while (rangeChild) {
> +								rect = rangeChild.getBoundingClientRect();
> +								if (rect.left <= x && x < rect.right) {
> +									//TODO test for character trailing (wrong for bidi)

Ditto.

@@ +5456,5 @@
> +						setTimeout(cleanup, 0);
> +						return false;
> +					}
> +				}
> +				/* no event and no permission, copy can not be done */

Again I'm not sure if this would be a problem in practice.

@@ +5751,5 @@
> +				selection.extend(end);
> +			} 
> +			this._setSelection(selection, true, true);
> +		},
> +		_showCaret: function () {

Showing a caret is a lot more involved than what happens here.  For example, we support a bidi caret for users with bidi keyboard layouts, and this will leave them out in the cold.

@@ +6202,5 @@
> +/**
> + * Uses a grammar to provide some very rough syntax highlighting for HTML.
> + * @class orion.syntax.HtmlGrammar
> + */
> +orion.editor.HtmlGrammar = (function() {

I barely skimmed over the stuff after here.  They're all functions that Gecko doesn't implement, so I wasn't too sensitive on them.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 544476 [details] [diff] [review]
patch update 5

Looks like ehsan's concerns need to be addressed.

Did the licensing issue get sorted out? [e-mail] should sign off on this, if they haven't already.

Comment 17

[(no longer active)]

Comment on attachment 544476 [details] [diff] [review]
patch update 5

r=me for landing it preffed off by default in Scratchpad.  We'll fix a bunch of bugs post landing, and we'll decide whether we want this enabled in Firefox 8 before the migration date.

Comment 18

[Kevin Dangoor]

Adding Gerv to confirm the licensing...

Gerv: We're adding Orion's editor component to Firefox. All of Orion's client-side code is licensed under the EDL:

http://www.eclipse.org/org/documents/edl-v10.php

which is BSD, so there should be no problem, but it's always good to have a double check before landing a chunk of code from another project.

Comment 19

[Gervase Markham [:gerv]]

Thanks for double-checking. Yes, the EDL is fine - note that the E_P_L might not be, please don't get the two confused. Once the code is checked in, please file a bug to get the EDL added to about:licence.

Gerv

Comment 20

[Mihai Sucan [:msucan]]

Attachment: patch update 6 - add textbox fallback and pref

Ehsan: thank you for your review comments and for the r+!

Rob: Added a new JSM that provides the same SourceEditor API implemented on top of a xul:textbox.

Made some API and code fixes. I did not update Orion in avoid regressions. I will look into doing so once I review their upstream changes.

Please let me know if the fallback approach is acceptable. I load the -textarea.jsm from the main JSM when needed.

Looking forward to your review comments. Thank you!

Comment 21

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

Comment on attachment 549908 [details] [diff] [review]
patch update 6 - add textbox fallback and pref

+// Tells if you want to enable the source code editor or not. The source code
+// editor implements syntax highlighting and other programmer-specific editor
+// features.

maybe replace with:

// This preference enables the Orion source code editor in Scratchpad. It
// provides programmer-specific editor features such as syntax
// highlighting, indenting and bracket recognition. It may not be
// appropriate for all locales (esp. RTL) or a11y situations.

+pref("devtools.codeeditor.enabled", true);

Wondering if this is a good name and type for this pref. If we had, e.g.,

pref("devtools.source.editor", "orion");

or "textbox" for the options, it could allow us to reuse this pref for
new editors.

 EXTRA_JS_MODULES = \
 	content/openLocationLastURL.jsm \
 	content/NetworkPrioritizer.jsm \
 	content/domplate.jsm \
+	content/source-editor.jsm \
+	content/source-editor-textarea.jsm \
 	$(NULL)

I thought you were going to move this into browser/devtools?

stopping here, since you're doing a rebase. :)

Comment 22

[Mihai Sucan [:msucan]]

Attachment: patch update 7 - another rebase, move to devtools, pref change

Rebased the patch for the Scratchpad move to browser/devtools.

Also moved the new SourceEditor component into its own browser/devtools/SourceEditor folder, as discussed.

Changed away from the devtools.codeeditor.enabled pref to devtools.editor.component, as agreed on IRC.

Thanks for your comments. Looking forward for more! :)

Comment 23

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

Comment on attachment 550494 [details] [diff] [review]
patch update 7 - another rebase, move to devtools, pref change

--- /dev/null
+++ b/browser/devtools/SourceEditor/orion/README
@@ -0,0 +1,8 @@
+This is the Orion editor packaged for Mozilla.
+
+The Orion editor web site: http://www.eclipse.org/orion

You should probably add a LICENSE section that includes some text to the effect of:

"The following files are licensed according to the contents in the LICENSE file.
  orion.js
  orion.css"


+++ b/browser/devtools/SourceEditor/orion/UPGRADE
@@ -0,0 +1,20 @@
+Upgrade notes:
+
+1. Get the Orion client source code from:
+http://www.eclipse.org/orion
+
+2. Install Dryice from:
+https://github.com/mozilla/dryice

This feels like it could use some more explanation. Maybe it's enough.

--- /dev/null
+++ b/browser/devtools/SourceEditor/orion/orion.css
@@ -0,0 +1,115 @@
+.view {
+	background-color: white;
+}
+

Surprised there's no license header on this, but I guess they're still new.

orion.js...

+ */
+orion.textview.KeyBinding = (function() {
+	var isMac = window.navigator.platform.indexOf("Mac") !== -1;
+	/** @private */
+	function KeyBinding (keyCode, mod1, mod2, mod3, mod4) {
+		if (typeof(keyCode) === "string") {
+			this.keyCode = keyCode.toUpperCase().charCodeAt(0);
+		} else {
+			this.keyCode = keyCode;

yuck! hard tabs.

I don't suppose you could translate those when building in dryice, could you?

probably not worth it. skipping onto the source-editor parts...

+++ b/browser/devtools/SourceEditor/source-editor-orion.jsm
@@ -0,0 +1,839 @@

+ * The SourceEditor object constructor. The SourceEditor component allows you to
+ * provide users with an editor tailored to the specific needs of editing source
+ * code, mainly geared towards web developers.

Not sure we need the "mainly geared towards web developers" part. If you must, reword it to, "aimed primarily at web developers". "geared towards" is colloquial and might confuse.

+SourceEditor.MODES = {
+  JAVASCRIPT: "js",
+  CSS: "css",
+  TEXT: "text",
+  HTML: "html",
+  XML: "xml",
+};
+
+/**
+ * Predefined themes for syntax highlighting.
+ */
+SourceEditor.THEMES = {
+  TEXTMATE: "chrome://browser/content/orion.css",
+};
+
+/**
+ * Source editor configuration defaults.
+ */
+SourceEditor.DEFAULTS = {
+  MODE: SourceEditor.MODES.TEXT,
+  THEME: SourceEditor.THEMES.TEXTMATE,
+  UNDO_LIMIT: 200,
+  TAB_SIZE: 4, // overriden by pref
+  EXPAND_TAB: true, // overriden by pref
+};

These are duplicated in both source editor files. Could lead to problems if we update one but forget to do the other.

Might want to break these into a source-editor-constants.jsm or similar? I dunno, might not be worth it.

+    let self = this;
+    this._iframe.addEventListener("load", function SE_init_onLoad() {
+      self._iframe.removeEventListener("load", SE_init_onLoad, true);
+
+      Services.scriptloader.loadSubScript(ORION_SCRIPT,
+        self._iframe.contentWindow.wrappedJSObject, "utf8");
+
+      self._onLoad(aCallback);
+    }, true);

you could use bind() here to get rid of the closure but it's not required.

+    let window = this._iframe.contentWindow.wrappedJSObject;

is the wrappedJSObject needed?

+  /**
+   * The "Unindent lines" editor action implementation.
+   * @private
+   */
+  _doUnindentLines: function SE__doUnindentLines()

When is this used? Maybe add some commentary to what calls this command. (shift-tab keybinding).

+  /**
+   * The editor Enter action implementation, which adds simple automatic
+   * indentation based on the previous line.
+   * @private
+   */
+  _doEnter: function SE__doEnter()

Same thing. "on the previous line when the user presses the Enter key".

Probably obvious, but it's nice to know where these things are being used.

+  /**
+   * Add an event listener to the editor. You can use one of the known events.
+   *
+   * @see SourceEditor.EVENTS
+   *
+   * @param string aEventType
+   *        The event type you want to listen for.
+   * @param function aCallback
+   *        The function you want executed when the event is triggered.
+   * @param object [aContext]
+   *        Optional scope for the callback execution.
+   * @param mixed [aData]
+   *        Optional data to pass to the callback when the event is triggered.
+   */
+  addEventListener:
+  function SE_addEventListener(aEventType, aCallback, aContext, aData)

when are these used? Only in testing? At integration time?

... skips ahead.

--- /dev/null
+++ b/browser/devtools/SourceEditor/source-editor-textarea.jsm

+ * The SourceEditor object constructor. The SourceEditor component allows you to
+ * provide users with an editor tailored to the specific needs of editing source
+ * code, mainly geared towards web developers.

same as above comment.

...

strong code throughout. Very cool.

+/**
+ * Various utility methods used by the SourceEditor.
+ */
+var Utils = {

seems a bit silly to have a container object for just one method, but ... OK.

in +++ b/browser/devtools/SourceEditor/source-editor.jsm

cute.

that is a helluva test file.

This is fantastic stuff, Mihai. I'm really excited to start dropping SourceEditors into everything. :)

r+ with the nits addressed. Only thing I'm really interested in changing for this is the edits to the text files and the comment changes. I wouldn't worry about creating a separate module for the constants or modifying dryice to convert tabs (though some may complain).

Comment 24

[Mihai Sucan [:msucan]]

(In reply to Rob Campbell [:rc] (robcee) from comment #23)
> Comment on attachment 550494 [details] [diff] [review] [diff] [details] [review]
> patch update 7 - another rebase, move to devtools, pref change
> 
> --- /dev/null
> +++ b/browser/devtools/SourceEditor/orion/README
> @@ -0,0 +1,8 @@
> +This is the Orion editor packaged for Mozilla.
> +
> +The Orion editor web site: http://www.eclipse.org/orion
> 
> You should probably add a LICENSE section that includes some text to the
> effect of:
> 
> "The following files are licensed according to the contents in the LICENSE
> file.
>   orion.js
>   orion.css"

Thanks. Will add this note.


> +++ b/browser/devtools/SourceEditor/orion/UPGRADE
> @@ -0,0 +1,20 @@
> +Upgrade notes:
> +
> +1. Get the Orion client source code from:
> +http://www.eclipse.org/orion
> +
> +2. Install Dryice from:
> +https://github.com/mozilla/dryice
> 
> This feels like it could use some more explanation. Maybe it's enough.

I thought it's sufficient. There's obviously a learning curve to getting the source code of the Orion client, or a learning curve to install Dryice and nodejs, but that's really part of the deal. Beyond the purpose of the UPGRADE info file.

Nonetheless, if you have specific suggestions about adding some more details, I can do it. Please let me know!


> --- /dev/null
> +++ b/browser/devtools/SourceEditor/orion/orion.css
> @@ -0,0 +1,115 @@
> +.view {
> +	background-color: white;
> +}
> +
> 
> Surprised there's no license header on this, but I guess they're still new.

Yeah, that's what I get from the Orion upstream code.

> orion.js...
> 
> + */
> +orion.textview.KeyBinding = (function() {
> +	var isMac = window.navigator.platform.indexOf("Mac") !== -1;
> +	/** @private */
> +	function KeyBinding (keyCode, mod1, mod2, mod3, mod4) {
> +		if (typeof(keyCode) === "string") {
> +			this.keyCode = keyCode.toUpperCase().charCodeAt(0);
> +		} else {
> +			this.keyCode = keyCode;
> 
> yuck! hard tabs.
> 
> I don't suppose you could translate those when building in dryice, could you?
> probably not worth it. skipping onto the source-editor parts...

Yeah, their source code does not follow strict guidelines for indentation and trailing whitespace. There's not much point in "beautifying" the code since it's not meant to be edited/modified. It is only rebuilt when needed, from the original source code.


> +++ b/browser/devtools/SourceEditor/source-editor-orion.jsm
> @@ -0,0 +1,839 @@
> 
> + * The SourceEditor object constructor. The SourceEditor component allows
> you to
> + * provide users with an editor tailored to the specific needs of editing
> source
> + * code, mainly geared towards web developers.
> 
> Not sure we need the "mainly geared towards web developers" part. If you
> must, reword it to, "aimed primarily at web developers". "geared towards" is
> colloquial and might confuse.

Hehe. :) Thanks for pointing out that "geared towards" is colloquial.


> +SourceEditor.MODES = {
> +  JAVASCRIPT: "js",
> +  CSS: "css",
> +  TEXT: "text",
> +  HTML: "html",
> +  XML: "xml",
> +};
> +
> +/**
> + * Predefined themes for syntax highlighting.
> + */
> +SourceEditor.THEMES = {
> +  TEXTMATE: "chrome://browser/content/orion.css",
> +};
> +
> +/**
> + * Source editor configuration defaults.
> + */
> +SourceEditor.DEFAULTS = {
> +  MODE: SourceEditor.MODES.TEXT,
> +  THEME: SourceEditor.THEMES.TEXTMATE,
> +  UNDO_LIMIT: 200,
> +  TAB_SIZE: 4, // overriden by pref
> +  EXPAND_TAB: true, // overriden by pref
> +};
> 
> These are duplicated in both source editor files. Could lead to problems if
> we update one but forget to do the other.
> 
> Might want to break these into a source-editor-constants.jsm or similar? I
> dunno, might not be worth it.

I thought of this as well. I wanted to put them in source-editor.jsm.

Will update the patch, and I will put the constants in source-editor.jsm.


> +    let window = this._iframe.contentWindow.wrappedJSObject;
> 
> is the wrappedJSObject needed?

Yes, otherwise you can only access native methods and properties.

> +  /**
> +   * Add an event listener to the editor. You can use one of the known
> events.
> +   *
> +   * @see SourceEditor.EVENTS
> +   *
> +   * @param string aEventType
> +   *        The event type you want to listen for.
> +   * @param function aCallback
> +   *        The function you want executed when the event is triggered.
> +   * @param object [aContext]
> +   *        Optional scope for the callback execution.
> +   * @param mixed [aData]
> +   *        Optional data to pass to the callback when the event is
> triggered.
> +   */
> +  addEventListener:
> +  function SE_addEventListener(aEventType, aCallback, aContext, aData)
> 
> when are these used? Only in testing? At integration time?

The three events supported (contextmenu, selection and textchanged) are used both in testing and integration. Scratchpad uses the contextmenu event.

Cedric asked for the TextChanged event for the StyleEditor he's working on. Selection also makes sense in different integration cases.

> strong code throughout. Very cool.

Thank you!


> +/**
> + * Various utility methods used by the SourceEditor.
> + */
> +var Utils = {
> 
> seems a bit silly to have a container object for just one method, but ... OK.

Yeah, I said there might be more methods in the future.


> in +++ b/browser/devtools/SourceEditor/source-editor.jsm
> 
> cute.
> 
> that is a helluva test file.
> 
> This is fantastic stuff, Mihai. I'm really excited to start dropping
> SourceEditors into everything. :)

Thank you! I am also excited about having Orion throughout the devtools.


> r+ with the nits addressed. Only thing I'm really interested in changing for
> this is the edits to the text files and the comment changes. I wouldn't
> worry about creating a separate module for the constants or modifying dryice
> to convert tabs (though some may complain).

Thanks for the r+!

Will update the patch to address your comments.

Comment 25

[Mihai Sucan [:msucan]]

Attachment: patch update 8 - address comments, es5 strict mode

Addressed the comments Robert had for the patch. Also enabled es5 strict mode, since the code already followed strict guidelines - needed only a couple of trivial changes.

Looking forward to your review. We would like to land this in time for Firefox 8. Thank you!

Comment 26

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 551010 [details] [diff] [review]
patch update 8 - address comments, es5 strict mode

>diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js

>+// Tells which component you want to use for source editing in developer tools.

Why does this need to be a pref? Do we plan on supporting the two different kinds of editors indefinitely? It seems like this patch would be a lot simpler if you only had one component (Orion).

>diff --git a/browser/devtools/Makefile.in b/browser/devtools/Makefile.in

> DIRS = \
>   webconsole \
>   scratchpad \
>+  SourceEditor \

Is there a reason to diverge from the standard lowercase directory names?

>diff --git a/browser/devtools/SourceEditor/Makefile.in b/browser/devtools/SourceEditor/Makefile.in

>+ifdef ENABLE_TESTS
>+ifneq (mobile,$(MOZ_BUILD_APP))
>+	DIRS += test
>+endif
>+endif

Why can't these tests runs on mobile? Is this a conscious decision, or just copying/pasting from another Makefile?

>diff --git a/browser/devtools/SourceEditor/source-editor-orion.jsm b/browser/devtools/SourceEditor/source-editor-orion.jsm

>+ * The editor used here is Eclipse Orion from http://www.eclipse.org/orion. The
>+ * implementation provided here can be replaced to the specific needs of any
>+ * other source editor.

I don't understand this comment.

>+function SourceEditor() {

>+  if (tabsize < 1) {
>+    // Tab size is invalid, clear back to the default value.
>+    Services.prefs.clearUserPref(SourceEditor.PREFS.TAB_SIZE);
>+    tabsize = Services.prefs.getIntPref(SourceEditor.PREFS.TAB_SIZE);
>+  }

You shouldn't bother with this kind of pref validation, if someone sets their tabsize pref to something bogus they deserve whatever happens :)

>+  _doTab: function SE__doTab()

>+      let lines = [];
...
>+      lines.push("");

nit: this is unnecessary (just initialize "lines" to [""]).

>+  _doEnter: function SE__doEnter()
>+  {
>+    let selection = this.getSelection();
>+    if (selection.start == selection.end) {

nit: you can return early here rather than indenting the majority of the function.

>+  focus: function SE_focus()
>+  {
>+    this._iframe.focus();
>+    this._view.focus();
>+  },

This looks odd. _view.focus() seems to call focus() on an element, so is there any point in focusing the iframe?

Comment 27

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

> >diff --git a/browser/devtools/SourceEditor/Makefile.in b/browser/devtools/SourceEditor/Makefile.in
> 
> >+ifdef ENABLE_TESTS
> >+ifneq (mobile,$(MOZ_BUILD_APP))
> >+	DIRS += test
> >+endif
> >+endif
> 
> Why can't these tests runs on mobile? Is this a conscious decision, or just
> copying/pasting from another Makefile?

This is in browser/, so we should never be getting here for mobile.

Comment 28

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

(In reply to Gavin Sharp from comment #26)
> Comment on attachment 551010 [details] [diff] [review] [diff] [details] [review]
> patch update 8 - address comments, es5 strict mode
> 
> >diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js
> 
> >+// Tells which component you want to use for source editing in developer tools.
> 
> Why does this need to be a pref? Do we plan on supporting the two different
> kinds of editors indefinitely? It seems like this patch would be a lot
> simpler if you only had one component (Orion).

Yes it would be, but we need to be able to pref it off for locales or a11y users that don't want it. I suggested creating a separate text component for fallback.

We might also someday want a different component here as well. It's future-proofy!

> >diff --git a/browser/devtools/Makefile.in b/browser/devtools/Makefile.in
> 
> > DIRS = \
> >   webconsole \
> >   scratchpad \
> >+  SourceEditor \
> 
> Is there a reason to diverge from the standard lowercase directory names?

Other downstream patches have that format. Wasn't sure we wanted hyphens.

> >+function SourceEditor() {
> 
> >+  if (tabsize < 1) {
> >+    // Tab size is invalid, clear back to the default value.
> >+    Services.prefs.clearUserPref(SourceEditor.PREFS.TAB_SIZE);
> >+    tabsize = Services.prefs.getIntPref(SourceEditor.PREFS.TAB_SIZE);
> >+  }
> 
> You shouldn't bother with this kind of pref validation, if someone sets
> their tabsize pref to something bogus they deserve whatever happens :)

really?

thanks for reviewing!

Comment 29

[Mihai Sucan [:msucan]]

(In reply to Gavin Sharp from comment #26)
> Comment on attachment 551010 [details] [diff] [review] [diff] [details] [review]
> patch update 8 - address comments, es5 strict mode
> 
> >diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js
> 
> >+// Tells which component you want to use for source editing in developer tools.
> 
> Why does this need to be a pref? Do we plan on supporting the two different
> kinds of editors indefinitely? It seems like this patch would be a lot
> simpler if you only had one component (Orion).

True, it would be simpler. Maintaining the fallback mechanism will have its cost. Eventually we will have to remove the textarea fallback.

For now we have added this as it was decided during a talk with Ehsan who has reviewed Orion. He pointed out that a11y and bidi/RTL support is not ideal in all cases. Therefore, we currently need to be able to pref Orion off.

Ideally, we'll have to improve Orion (and upstream will also do this), such that a11y and bidi/RTL support will be good enough so we can remove the fallback mechanism.

Also, as Robert pointed out, we want to be able to change the underlying editor anytime we decide, without having to change all the code that uses the SourceEditor.

> >diff --git a/browser/devtools/SourceEditor/Makefile.in b/browser/devtools/SourceEditor/Makefile.in
> 
> >+ifdef ENABLE_TESTS
> >+ifneq (mobile,$(MOZ_BUILD_APP))
> >+	DIRS += test
> >+endif
> >+endif
> 
> Why can't these tests runs on mobile? Is this a conscious decision, or just
> copying/pasting from another Makefile?

True, this came from some other devtools/ Makefile. Will fix.


> >diff --git a/browser/devtools/SourceEditor/source-editor-orion.jsm b/browser/devtools/SourceEditor/source-editor-orion.jsm
> 
> >+ * The editor used here is Eclipse Orion from http://www.eclipse.org/orion. The
> >+ * implementation provided here can be replaced to the specific needs of any
> >+ * other source editor.
> 
> I don't understand this comment.

Ooops, engrish. Will fix!

> >+function SourceEditor() {
> 
> >+  if (tabsize < 1) {
> >+    // Tab size is invalid, clear back to the default value.
> >+    Services.prefs.clearUserPref(SourceEditor.PREFS.TAB_SIZE);
> >+    tabsize = Services.prefs.getIntPref(SourceEditor.PREFS.TAB_SIZE);
> >+  }
> 
> You shouldn't bother with this kind of pref validation, if someone sets
> their tabsize pref to something bogus they deserve whatever happens :)

This was requested when I did the code for bug 660560. I can remove these checks.

> >+  focus: function SE_focus()
> >+  {
> >+    this._iframe.focus();
> >+    this._view.focus();
> >+  },
> 
> This looks odd. _view.focus() seems to call focus() on an element, so is
> there any point in focusing the iframe?

Good point. Will remove the _frame.focus() call.

Thanks for your review comments!

Comment 30

[Mihai Sucan [:msucan]]

Attachment: patch update 9 - address gavin's comments

Updated the patch to address Gavin's comments.

Thank you!

Comment 31

[:Gavin Sharp [email: [e-mail]]]

(In reply to Mihai Sucan [:msucan] from comment #29)
> > You shouldn't bother with this kind of pref validation, if someone sets
> > their tabsize pref to something bogus they deserve whatever happens :)
> 
> This was requested when I did the code for bug 660560. I can remove these
> checks.

I disagree with Dietrich here, but it's not a big deal either way.

Comment 32

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 551798 [details] [diff] [review]
patch update 9 - address gavin's comments

>diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js

>+pref("devtools.editor.component", "orion");

If the idea is for this to be a locale-dependent pref, shouldn't it be a localized pref?

>diff --git a/browser/devtools/SourceEditor/source-editor-orion.jsm b/browser/devtools/SourceEditor/source-editor-orion.jsm

>+Cu.import("resource:///modules/Services.jsm");

This should be resource://gre/modules/Services.jsm (this comment also applies to the other modules).

>diff --git a/browser/devtools/SourceEditor/source-editor-textarea.jsm b/browser/devtools/SourceEditor/source-editor-textarea.jsm

>+  init: function SE_init(aElement, aConfig, aCallback)

>+    this._lineDelimiter = win.navigator.platform.indexOf("Win") > -1 ?
>+                          "\r\n" : "\n";

I thought perhaps there was some way to get this from the platform, but I couldn't find it offhand.

>+  hasFocus: function SE_hasFocus()

>+    return this._textbox.ownerDocument.activeElement ===
>+           this._textbox.inputField;

this.hasAttribute("focused")?

>+   * Note: this implementation makes no difference between any of the available
>+   * modes.

>+  setMode: function SE_setMode(aMode)
>+  {
>+    this._mode = aMode; // nothing to do here

Seems like this should avoid setting _mode...

>+  getMode: function SE_getMode()
>+  {
>+    return this._mode;

...and this should always return the same (default?) value.

>+function EditActionListener(aSourceEditor) {

Did ehsan review this part?

r=me with those addressed.

Comment 33

[Mihai Sucan [:msucan]]

(In reply to Gavin Sharp from comment #32)
> Comment on attachment 551798 [details] [diff] [review]
> patch update 9 - address gavin's comments
> 
> >diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js
> 
> >+pref("devtools.editor.component", "orion");
> 
> If the idea is for this to be a locale-dependent pref, shouldn't it be a
> localized pref?

Will update the patch to use a localized pref.


> >diff --git a/browser/devtools/SourceEditor/source-editor-orion.jsm b/browser/devtools/SourceEditor/source-editor-orion.jsm
> 
> >+Cu.import("resource:///modules/Services.jsm");
> 
> This should be resource://gre/modules/Services.jsm (this comment also
> applies to the other modules).

Will fix.


> >diff --git a/browser/devtools/SourceEditor/source-editor-textarea.jsm b/browser/devtools/SourceEditor/source-editor-textarea.jsm
> 
> >+  init: function SE_init(aElement, aConfig, aCallback)
> 
> >+    this._lineDelimiter = win.navigator.platform.indexOf("Win") > -1 ?
> >+                          "\r\n" : "\n";
> 
> I thought perhaps there was some way to get this from the platform, but I
> couldn't find it offhand.

I couldn't find anything either. I looked into the source of the native editor:

http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/

... and I didn't find the solution there either.


> >+  hasFocus: function SE_hasFocus()
> 
> >+    return this._textbox.ownerDocument.activeElement ===
> >+           this._textbox.inputField;
> 
> this.hasAttribute("focused")?

This semi-works. The attribute is updated async and I would have to rely on events. I can't do editor.focus() then check editor.hasFocus(). I haven't added support for the focus/blur events because Orion has nothing like this, yet. Orion will soon implement these events, and then we can do this.


> >+   * Note: this implementation makes no difference between any of the available
> >+   * modes.
> 
> >+  setMode: function SE_setMode(aMode)
> >+  {
> >+    this._mode = aMode; // nothing to do here
> 
> Seems like this should avoid setting _mode...
> 
> >+  getMode: function SE_getMode()
> >+  {
> >+    return this._mode;
> 
> ...and this should always return the same (default?) value.

Will fix.


> >+function EditActionListener(aSourceEditor) {
> 
> Did ehsan review this part?

He reviewed Orion. Does he need to review the rest of the code as well?


> r=me with those addressed.


Thank you very much!

Comment 34

[Mihai Sucan [:msucan]]

Gavin: also, wrt. the localized pref. Is it acceptable if I use browser.properties? Or should I use a different file? We do not have devtools-specific properties.

pref("devtools.editor.component", "chrome://browser/locale/browser.properties");

Thanks!

Comment 35

[Mihai Sucan [:msucan]]

Attachment: patch update 10 - address gavin's review

Addressed Gavin's review comment 32.

Ehsan: can you please do a quick review on source-editor-textarea.jsm? Gavin has suggested that I should ask you to review the new file (he did so in comment 32).

We would like to land the patch this week. Thank you!

Comment 36

[Mihai Sucan [:msucan]]

Attachment: patch update 11 - turn off orion by default

Updated to turn Orion off by default, as asked by Ehsan.

Comment 37

[(no longer active)]

Comment on attachment 552400 [details] [diff] [review]
patch update 11 - turn off orion by default

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

r- for the localized pref, and also the EditActionListener comments...

::: browser/app/profile/firefox.js
@@ +1049,5 @@
> +//   "orion" - this is the Orion source code editor from the Eclipse project. It
> +//   provides programmer-specific editor features such as syntax highlighting,
> +//   indenting and bracket recognition. It may not be appropriate for all
> +//   locales (esp. RTL) or a11y situations.
> +pref("devtools.editor.component", "chrome://browser/locale/browser.properties");

So, firstly, having a localized pref doesn't help us turn Orion off for disabled users.  And I don't really think that our decision of whether or not to turn it on would depend on specific locales.  On top of that, this will put the decision of whether or not to use Orion on the localizers, and they don't have enough information to make the right call.

Considering the above, this should not be a localizable pref.

::: browser/devtools/Makefile.in
@@ +47,5 @@
>  
>  DIRS = \
>    webconsole \
>    scratchpad \
> +  SourceEditor \

This is just a nit, but we don't usually use upper case letter in out directory names...

::: browser/devtools/SourceEditor/orion/LICENSE
@@ +1,1 @@
> +Eclipse Distribution License - v 1.0

This should go into about:license too.

::: browser/devtools/SourceEditor/orion/README
@@ +7,5 @@
> +# Upgrade
> +
> +To upgrade Orion to a newer version see the UPGRADE file.
> +
> +Orion version: git clone from 2011-07-07 (after the 0.2 release).

Please include the git commit hash for the version that you've pulled for future reference.

::: browser/devtools/SourceEditor/source-editor-orion.jsm
@@ +45,5 @@
> +
> +const ORION_SCRIPT = "chrome://browser/content/orion.js";
> +const ORION_IFRAME = "data:text/html;charset=utf8,<!DOCTYPE html>" +
> +  "<html style='height:100%'>" +
> +  "<body style='height:100%;margin:0;overflow:hidden'>" +

Please add dir="ltr" to this.  We don't want the source editor to change its direction in RTL mode.

::: browser/devtools/SourceEditor/source-editor-textarea.jsm
@@ +114,5 @@
> +    let win = doc.defaultView;
> +
> +    this._textbox = doc.createElementNS(XUL_NS, "textbox");
> +    this._textbox.flex = 1;
> +    this._textbox.setAttribute("multiline", true);

Similarly, please set the dir attribute to "ltr" here.

@@ +771,5 @@
> +    } else {
> +      event = {
> +        start: 0,
> +        removedCharCount: 0,
> +        addedCharCount: aNode.textContent.length,

You're assuming that the inserted node is a textnode.  That assumption is false I think.  For plaintext editors, it could also be a BR node.  So you need to check the nodeType here.

@@ +774,5 @@
> +        removedCharCount: 0,
> +        addedCharCount: aNode.textContent.length,
> +        removedLineCount: 0,
> +        addedLineCount: aNode.textContent.length > 0 ?
> +                        Utils.countLines(aNode.textContent) - 1 : 0,

This is going to be really inefficient, if the textarea size is really large.  Why do you need to count the number of lines exactly?

@@ +797,5 @@
> +      removedCharCount: 0,
> +      addedCharCount: aString.length,
> +      removedLineCount: 0,
> +      addedLineCount: aString.length > 0 ?
> +                      Utils.countLines(aString) - 1 : 0,

Same concerns as the above.

@@ +811,5 @@
> +    let event = {
> +      start: aOffset,
> +      removedCharCount: aLength,
> +      addedCharCount: 0,
> +      removedLineCount: aLength > 0 ? Utils.countLines(str) - 1 : 0,

Ditto.

@@ +833,5 @@
> +    let event = {
> +      start: selection.start,
> +      removedCharCount: str.length,
> +      addedCharCount: 0,
> +      removedLineCount: str.length > 0 ? Utils.countLines(str) - 1 : 0,

Ditto.

@@ +861,5 @@
> +    let i = 0, n = aString.length, c0, c1;
> +    let lines = aString.length > 0 ? 1 : 0;
> +    while(i < n) {
> +      c1 = aString.charAt(i++);
> +      if (c1 == "\r" || (c0 != "\r" && c1 == "\n")) {

Wait a minute.  Are we storing carriage returns in our textarea?  That's very very bad!  If we are, we shouldn't do it.

Comment 38

[Mihai Sucan [:msucan]]

Attachment: patch update 12 - address Ehsan's review comments

Updated the patch to address Ehsan's review comments. Changes:

- removed added/removedLineCount properties from TextChanged events, to not cause performance issues.
- removed get/setCaretLine() API.
- changed back to a normal char preference for devtools.editor.component (no longer a localized pref).
- added EDL to about:license.
- added the dir="ltr" attributes as asked.
- renamed SourceEditor folder to sourceeditor.
- updated orion/README to include the git commit hash.

Thank you!

Comment 39

[(no longer active)]

Comment on attachment 552510 [details] [diff] [review]
patch update 12 - address Ehsan's review comments

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

This looks great, thanks!

::: browser/devtools/sourceeditor/source-editor-textarea.jsm
@@ +707,5 @@
> +  WillInsertNode: function() { },
> +
> +  DidInsertNode: function EAL_DidInsertNode(aNode)
> +  {
> +    if (aNode.nodeType != Ci.nsIDOMNode.TEXT_NODE) {

Nit:
if (aNode.nodeType != aNode.TEXT_NODE) {

Comment 40

[Mihai Sucan [:msucan]]

Attachment: [checked-in] patch update 13

Updated the patch to address the review nit.

Thank you Gavin, Ehsan and Robert for your reviews!

Comment 41

[Mihai Sucan [:msucan]]

Comment on attachment 552625 [details] [diff] [review]
[checked-in] patch update 13

Landed:

http://hg.mozilla.org/integration/fx-team/rev/9975c8487a09

Comment 42

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

Comment on attachment 552625 [details] [diff] [review]
[checked-in] patch update 13

http://hg.mozilla.org/mozilla-central/rev/9975c8487a09

Comment 43

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

[09:24:04.061] "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:8.0a1) Gecko/20110814 Firefox/8.0a1"