Move the Source Editor to toolkit

RESOLVED WONTFIX

Status

P4
enhancement
RESOLVED WONTFIX
7 years ago
2 months ago

People

(Reporter: msucan, Assigned: db48x)

Tracking

Trunk
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sourceeditor])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

7 years ago
Some people have expressed interest in reusing the Source Editor in different applications, outside of Firefox. They asked that the Source Editor moves to toolkit.

I believe we should consider this possibility once the Source Editor is mature enough.

(please add comments on requirements for this to happen, and blockers we need to fix. thank you!)
(Reporter)

Updated

7 years ago
Blocks: 722319
As it currently looks like, Firebug 1.10 will use/require SourceEditor, and without this bug getting fixed or a fallback implemented on Firebug's side, this will make it impossible to use Firebug with SeaMonkey.

Cf. <http://code.google.com/p/fbug/issues/detail?id=5219>
(Reporter)

Updated

7 years ago
Assignee: nobody → mihai.sucan
Moving to Source Editor component.

Filter on CHELICERAE.
Component: Developer Tools → Developer Tools: Source Editor
I see the bugs this depends on have been marked fixed.  What prevents us from doing a wholesale move from browser/devtools/sourceeditor to toolkit/devtools/sourceeditor ?

I note that a lot of the references go directly to "resource:///modules/source-editor.jsm", which is wrong. They should point to resource://gre/modules/source-editor.jsm after the move.  (Technically, they should point to resource://app/... now.)

I can probably start crafting a patch this weekend.
Created attachment 738711 [details] [diff] [review]
patch, minimally tested

Or maybe I can do it sooner... :)
Attachment #738711 - Flags: review?(mihai.sucan)
(Reporter)

Comment 5

5 years ago
Comment on attachment 738711 [details] [diff] [review]
patch, minimally tested

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

This looks good. Nothing obvious holds us away from moving the source editor to toolkit. Your patch is very much welcome. Thank you!

Before we can do the move all of the source editor tests need to become mochitest-chrome tests. This means you can't open new tabs or windows. This can be easily changed: mostly mechanical conversion of the existing js tests into xul tests. Can you please do this? Once you have converted one test file please ping me on IRC so I can check how it is before you do the rest of the tests. Thanks!

::: browser/devtools/debugger/debugger-controller.js
@@ +20,5 @@
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/devtools/dbg-server.jsm");
>  Cu.import("resource://gre/modules/devtools/dbg-client.jsm");
>  Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js");
> +Cu.import("resource://gre/modules/source-editor.jsm");

Please move source-editor.jsm into resource://gre/modules/devtools.

::: browser/devtools/sourceeditor/source-editor.jsm
@@ +19,5 @@
>  try {
>    if (component == "ui") {
>      throw new Error("The ui editor component is not available.");
>    }
> +  Cu.import("resource://gre/modules/source-editor-" + component + ".jsm", obj);

Ditto for the rest of source editor jsm files.
Attachment #738711 - Flags: review?(mihai.sucan) → feedback+
Comment on attachment 738711 [details] [diff] [review]
patch, minimally tested

This patch wasn't even close to complete:  I forgot to move locale & skin files.
Attachment #738711 - Attachment is obsolete: true
Created attachment 748464 [details] [diff] [review]
patch with locales & themes moved

Rewriting of tests is forthcoming.
(Assignee)

Comment 8

5 years ago
Created attachment 749087 [details] [diff] [review]
partially converted tests, wip

Got held up on a problem in Orion; it tries to clear the content of its inner iframe (using document.open) only to find that this is dissallowed. I believe that the outer iframe should be type=content so that the entire editor is content instead of half-chrome-half-content.
Assignee: mihai.sucan → db48x
Daniel, do you want to regenerate that patch using diff --git, to show repo moves? :-)  The patch is rather large without that...
(Assignee)

Comment 10

5 years ago
Created attachment 749547 [details] [diff] [review]
wip, updated
Attachment #748464 - Attachment is obsolete: true
Attachment #749087 - Attachment is obsolete: true
Created attachment 750475 [details] [diff] [review]
wip, updated (patches folded into one)

Well, tests are failing early, due to exceptions as Daniel reports in comment 8.  Mihai, you can probably identify the problems and direct us on the fixes fairly easily.  Suggestions are welcome.
Attachment #749547 - Attachment is obsolete: true
Attachment #750475 - Flags: feedback?(mihai.sucan)
(Reporter)

Comment 12

5 years ago
Alex and Daniel: thank you for your patch. I believe you saw that the devtools team is planning to move all tools into their own folder and that means we'll move the source editor again. Should we do this move here and now?

The split between toolkit/ and browser/ for devtools is becoming a problem for us and we would like to solve the problem for all tools at once.
(In reply to Mihai Sucan [:msucan] from comment #12)
> Alex and Daniel: thank you for your patch. I believe you saw that the
> devtools team is planning to move all tools into their own folder and that
> means we'll move the source editor again. Should we do this move here and
> now?

No, that makes little sense.  Also, on irc, there was some discussion of replacing Orion with codemirror (but no decision).  It sounds like we're jumping the gun just a little bit.
(Reporter)

Comment 14

5 years ago
Comment on attachment 750475 [details] [diff] [review]
wip, updated (patches folded into one)

Canceling feedback request then. Thanks!
Attachment #750475 - Flags: feedback?(mihai.sucan)
I haven't seen any activity in this bug or heard any interest about moving the source editor into toolkit. I assume this is safe to mark as WONTFIX. Please reopen if these assumptions are wrong.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WONTFIX

Updated

2 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.