Mihai Sucan [:msucan] Assignee
	Description • 13 years ago

It's time for another Orion update.

	Mihai Sucan [:msucan] Assignee
	Comment 1 • 13 years ago

Work in progress patch - not ready for review, yet.

Changes:

- updated Orion from upstream.
- started to use our new require.jsm. Thanks to Joe for his work!
- reworked the dryice Makefile so dependencies are automatically fetched into the target script.
- Orion is now localizable and more accessible (upstream work on accessibility).
  - They have removed the inner iframe which means we no longer have nested frames.
  - Initialization is now back to sync, not async...

There was only one obvious regression that I had to fix - read-only mode was broken. Landed the fix upstream.

Before I go for review and landing we need to establish the scope of this update - which improvements and fixes we want.

	Mihai Sucan [:msucan] Assignee
	Comment 2 • 12 years ago

Changed the Makefile to generate a list of known Orion modules such that SourceEditor.destroy() can unload them, as suggested in bug 759348 comment 4.

I also removed the exception for when modules are redefined.

	Mihai Sucan [:msucan] Assignee
	Comment 3 • 12 years ago

Updated to work around any require.jsm changes. All tests pass again.

	Thaddee Tyl [:espadrine]
	Comment 4 • 12 years ago

This patch documents the new procedure needed to upgrade Orion,
and solves a couple of obvious bugs related to highlighting
the current line in the source editor.

The fixed bugs are the following:

- Creating a newline from an empty line made both lines highlighted as the
  current line.
- Removing the first character of a line, and then the newline before it, did
  not make the current line highlighted as the current line.

msucan: as you wanted, I have made a github repository to track my progress.
It is available at <https://github.com/espadrine/moz-patches>.

	Mihai Sucan [:msucan] Assignee
	Comment 5 • 12 years ago

Comment on attachment 635640 [details] [diff] [review]
wip 4 - current line highlighting fix + documentation

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

Thanks for your update Thaddee! This looks good!

I'm glad to see you've updated Orion. Your wip4 includes new fixes for bug 725586 and bug 760980.

I expect you can take an even newer commit of Orion - should be safe. They are stabilizing for the release of Orion 0.5.

Also thanks for the github repo - that's useful for tracking changes.

::: browser/devtools/sourceeditor/orion/README
@@ +13,3 @@
>  
>    + patch for Eclipse Bug 370584 - [Firefox] Edit menu items in context menus
>      http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=137d5a8e9bbc0fa204caae74ebd25a7d9d4729bd

We need to check if these patches are in Orion upstream (afaik they are). If yes, we can remove this list of patches.

::: browser/devtools/sourceeditor/orion/UPGRADE
@@ +9,1 @@
>  org.eclipse.orion.client/bundles/

Thanks for the UPGRADE changes.

One mention: users don't even need to copy the script anymore. They can run it from the Firefox codebase. That's why I added the two env vars, so the script can live anywhere.

We could make FIREFOX_HOME to not be required - we can make it a default to assume the script is run from the Firefox codebase.

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +191,4 @@
>    _styler: null,
>    _annotationStyler: null,
>    _annotationModel: null,
> +  _annotationType: null,

Please make sure you set this new property to null in destroy() to avoid any memleaks.

@@ +820,2 @@
>  
> +    if (this._model) {

Is this check needed? Afaik we don't run this method before Orion is initialized.

@@ +826,5 @@
> +
> +      let remove = this._currentLineAnnotation;
> +      if (remove) {
> +        remove.start = oldSelection.start;
> +        remove.end = oldSelection.end;

This trick works, but not in all cases. :(

Try to type:

function foo() {
a
}

... then press backspace as many times as needed to delete what you typed. Try this several times. For me, most of the time, I still get double, triple lines highlighted. This is starting to look like a bug in Orion. I will investigate and make a bug report upstream if needed.

	Mihai Sucan [:msucan] Assignee
	Comment 6 • 12 years ago

Thaddee: I looked into Orion's codebase to see how they do the line highlighting. I found editor.js has a method that does line highlighting. See /bundles/org.eclipse.orion.client.editor/web/orion/editor/editor.js. I suggest playing with that.

	Thaddee Tyl [:espadrine]
	Comment 7 • 12 years ago

I took care of msucan's comments.

This patch implements the CSS changes for all platforms (which I have tested on Linux and Mac).

I consider this patch good to go. It uses today's latest Orion.
However, I marked this as a WIP because of remaining Orion glitches (our code is fine). Should we wait for Orion's stable release?

(The glitches seem to go away pretty soon. In your example, typing `function foo() {\n` will keep that line highlighted until we type the corresponding `}\n`, which nearly feels like a feature!)

	Mihai Sucan [:msucan] Assignee
	Comment 8 • 12 years ago

Comment on attachment 639862 [details] [diff] [review]
wip 5 - current line highlighting fix + documentation

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

Thanks for the update!

1. In comment 5 I asked for an update related to the patches list in README. Did you check if those patches have landed in Orion upstream? If yes, please remove them from the list.

2. Line highlighting is still broken. We cannot land with a known regression like this one. I don't see this bug when I use the Orion demo from upstream. [1] This makes me believe it's a bug in our implementation.

More comments below...

[1] http://orion.eclipse.org/examples/textview/demo.html

::: browser/devtools/sourceeditor/orion/README
@@ +8,5 @@
>  
>  To upgrade Orion to a newer version see the UPGRADE file.
>  
> +Orion version: git clone from 2012-05-28
> +               commit hash cafca7acd3433d6c106ea052ec3a17fe60644993

please update the date to say the date of the commit.

::: browser/devtools/sourceeditor/orion/UPGRADE
@@ +21,2 @@
>  
>  6. Make a new build of Firefox.

Wrong number in the list.

	Thaddee Tyl [:espadrine]
	Comment 9 • 12 years ago

Indeed, all patches in the README seem to have landed.

As for line highlighting, I will revisit my code and compare it again to the 0.5 release's.

I tested the I20120622-1139 build, which has obvious failings in the way the current line is highlighted. I will try HEAD as soon as I can.

	Thaddee Tyl [:espadrine]
	Comment 10 • 12 years ago

This patch fixes the bugs previously discussed.

I cannot find any bug this time.

	Mihai Sucan [:msucan] Assignee
	Comment 11 • 12 years ago

Comment on attachment 641664 [details] [diff] [review]
Orion update — sourceeditor tests pass, no obvious bug

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

Thanks for your fixes! Indeed, I can no longer reproduce any problem with the line highlighter.

Giving r+ with the comments below addressed. We will land this patch after the Aurora merge.

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +11,5 @@
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource:///modules/source-editor-ui.jsm");
> +Cu.import("resource:///modules/devtools/Require.jsm");
> +Cu.import("resource:///modules/devtools/Console.jsm");

Please remove this import. I imported the console object only for testing and debugging purposes.

@@ +289,5 @@
> +    domain.syntax = "amd";
> +
> +    window.require = domain.require.bind(domain);
> +    window.define = define;
> +    window.console = console;

Please remove this - debugging purposes only.

@@ +302,5 @@
> +    window.document.head.appendChild(script);
> +    script.addEventListener("load", onScriptLoad, false);
> +  },
> +
> +  _initializeOrion: function SE__initializeOrion()

This new method needs a jsdoc comment. Please make sure you follow the same style as for other methods.

@@ +329,4 @@
>        tabSize: this._tabSize,
>        expandTab: this._expandTab,
>        readonly: config.readOnly,
> +      tabMode: "tabMode" in config ? config.tabMode : !config.readOnly,

This patch changes SourceEditor configuration options. Please see the init() comment which points to SourceEditor.DEFAULTS. The DEFAULTS object in source-editor.jsm needs to be updated to reflect the changes.

The new tabMode tells weather the Tab key is consumed by the editor or not. If not, then Tab key is allowed to do the browser-default action: focus traversal.

@@ +814,5 @@
>     * @private
>     * @param object aEvent
>     *        The Selection event object.
>     */
> +  _highlightCurrentLine: function SE__highlightCurrentLine(aEvent) {

We put the brace on a new line. Please follow the coding style of the file.

@@ +839,4 @@
>      }
>  
> +    // Only add annotation for the current line if the selection is empty.
> +    let add = this._currentLineAnnotation = null;

You do |let add = null| but |add| goes unused. Do you need the variable?

@@ +844,5 @@
> +      this._currentLineAnnotation =
> +          this._annotationType.createAnnotation(type, start, end);
> +    }
> +    this._annotationModel.removeAnnotations(type);
> +    this._annotationModel.addAnnotation(this._currentLineAnnotation);

this._currentLineAnnotation can be null. Please call addAnnotation only if needed.

@@ +2069,5 @@
>      this._lastFind = null;
>    },
>  };
> +
> +function unregisterModules(aModules) {

Please document the function. This is something I did not do in my first WIP patches and I'd like us to do, before we land.

@@ +2077,5 @@
> +    }
> +  });
> +}
> +
> +function orion_strings_proxy() {

Ditto.

@@ +2085,5 @@
> +    }
> +  });
> +}
> +
> +function orion_i18nUtil()

Ditto.

@@ +2103,5 @@
> +  };
> +}
> +
> +define("i18n!orion/textview/nls/messages", [], orion_strings_proxy);
> +define("orion/textview/i18nUtil", [], orion_i18nUtil);

We should mention that we override these modules with our own implementations, so we can have the editor strings localized.

	Thaddee Tyl [:espadrine]
	Comment 12 • 12 years ago

Mihai, I have gone through all the points you highlighted.
Most of them were pretty obvious.

On the other hand, I'm not 100% sure I did what you expected about
SourceEditor.DEFAULTS. I simply added a field in source-editor.jsm,
for tabMode. Tell me if that is fine!

	Mihai Sucan [:msucan] Assignee
	Comment 13 • 12 years ago

Comment on attachment 642349 [details] [diff] [review]
Orion update — sourceeditor tests pass, no obvious bug

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

Thank you Thaddee! r+, you don't need to ask for re-review.

We are adding a new orion.properties strings file - I am not sure if we need (or not) a review from a browser peer (or from the l10n team). Please ping Rob about this. Otherwise, the patch is good to go.

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +836,5 @@
> +
> +    // Hack for empty lines.
> +    // We set the old reference to what it was meant to hold,
> +    // then we assign the reference to a brand new annotation.
> +    if (oldStart === oldEnd && this._currentLineAnnotation) {

nit (don't need to update for this, just going to mention it for other patches!): we don't use strict equality checks, generally, unless we really need them.

@@ +2079,5 @@
> +/**
> + * Remove all require.js modules located in an object.
> + * Relevant: window.knownOrionModules, which can be passed to this function.
> + * @param object aModules
> + *        map from "define" module names to the corresponding modules.

nit: we start with an upper case.

::: browser/devtools/sourceeditor/source-editor.jsm
@@ +122,5 @@
>  
>    /**
> +   * Tells whether the tab key is consumed by the editor or not.
> +   * If not, then the tab key is allowed to do the default browser action,
> +   * focus traversal.

Please mention that the default is !readOnly.

(which means: true when readOnly is false, false when readOnly is true)

	Thaddee Tyl [:espadrine]
	Comment 14 • 12 years ago

I modified the patch according to your nitpicks…

	Mihai Sucan [:msucan] Assignee
	Comment 15 • 12 years ago

Pushed the patch to the try server:

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

Once we get green results, we can land.

	Mihai Sucan [:msucan] Assignee
	Comment 16 • 12 years ago

Comment on attachment 642630 [details] [diff] [review]
Orion update

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

We can't land this, for now. Found a regression while working on bug 669011.

When the editor is set to readonly mode, the editor background color does not change. This is because in this Orion update some of the elements changed their class names. See the diff of orion/orion.css. My wip patch was not complete, and it seems we've missed making the full set of changes.

Please look at the orion.css changes and make sure it's all working - double check that the changes in orion/orion.css are correctly reflected in themes/*/orion.css. Thank you!

	Mihai Sucan [:msucan] Assignee
	Comment 17 • 12 years ago

Thaddee: the try push from comment 15 shows some oranges that seem to be new.

The errors:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | no breakpoint was added to the editor - Got 1, expected 0
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | one breakpoint was removed from the editor - Got 0, expected 1
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | no breakpoint in the debugger - Got 2, expected 0
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | getBreakpoint(scriptLocations[0], 5) returns no breakpoint


Can you please look into the try push logs and try to fix the failures?

From a quick look, we might need to update the code that adds/removes the breakpoints. Similar to what you did for the line highlighting - there are some annotations API changes which probably break things for breakpoints as well (eh).

Thank you! Please let me know if I can help you with anything.

	Rob Campbell [:rc] (:robcee)
	Comment 18 • 12 years ago

filter on CHOCOLATE.

	Victor Porof [:vporof][:vp]
	Comment 19 • 12 years ago

Any hints on when this may be in the radar again?
No rush, I'm just asking to help me prioritize some assigned bugs that depend on 725399 and 725392 that depend on this.

	Thaddee Tyl [:espadrine]
	Comment 20 • 12 years ago

Victor: it certainly is on my radar. The issues that remain take time to work on, but I'm on it.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 21 • 12 years ago

I am seeing the following error if inserting the SourceEditor (Orion) into a <browser> element with type="content"

Error: SecurityError: The operation is insecure.
Source File: chrome://browser/content/orion.js
Line: 7105

It's because Orion requires chrome privileges since it uses document.write() into an iframe, and it also executes XHR from chrome://

However, Firebug needs Orion within <browser> with type="content". 

Will it work after this bug is fixed?

Honza

	Mihai Sucan [:msucan] Assignee
	Comment 22 • 12 years ago

This patch alone, from this bug, will not make things work with type=content - and we are very close to finish.

Honza, after this bug lands we will look, in a follow up bug, if we can switch to type=content. I'm going to open the bug, as soon as possible.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 23 • 12 years ago

(In reply to Mihai Sucan [:msucan] from comment #22)
> This patch alone, from this bug, will not make things work with type=content
> - and we are very close to finish.
> 
> Honza, after this bug lands we will look, in a follow up bug, if we can
> switch to type=content. I'm going to open the bug, as soon as possible.
Thanks Mihai! Note that this blocks Firebug to use Orion in the Script panel.

Honza

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 24 • 12 years ago

Any chance to get the patch landed soon?

Honza

	Rob Campbell [:rc] (:robcee)
	Comment 25 • 12 years ago

assigning to Mihai.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 26 • 12 years ago

I tested the attached patch "Orion update" and I verified that it solves the:
Bug 787086 - Lower Orion iframe privileges  

Is there any estimate/goal when it'll land?

Honza

	Mihai Sucan [:msucan] Assignee
	Comment 27 • 12 years ago

(In reply to Jan Honza Odvarko from comment #26)
> I tested the attached patch "Orion update" and I verified that it solves the:
> Bug 787086 - Lower Orion iframe privileges  

Great!

> Is there any estimate/goal when it'll land?

For the moment no, but we will do our best to land this patch as soon as possible.

	jodvarko
	Comment 28 • 12 years ago

I can't apply the patch to the current http://hg.mozilla.org/integration/fx-team repo. Any tips? Or the patch is out of date?

Honza

C:\src\mozilla.org\fx-team>hg import orion-update.patch
applying orion-update.patch
patching file browser/devtools/sourceeditor/source-editor-orion.jsm
Hunk #2 FAILED at 145
1 out of 14 hunks FAILED -- saving rejects to file browser/devtools/sourceeditor
/source-editor-orion.jsm.rej
patching file browser/locales/jar.mn
Hunk #1 FAILED at 34
1 out of 1 hunks FAILED -- saving rejects to file browser/locales/jar.mn.rej
patching file browser/themes/gnomestripe/devtools/orion.css
Hunk #1 FAILED at 1
Hunk #3 succeeded at 150 with fuzz 2 (offset -6 lines).
1 out of 3 hunks FAILED -- saving rejects to file browser/themes/gnomestripe/dev
tools/orion.css.rej
patching file browser/themes/pinstripe/devtools/orion.css
Hunk #1 FAILED at 1
Hunk #3 succeeded at 150 with fuzz 2 (offset -6 lines).
1 out of 3 hunks FAILED -- saving rejects to file browser/themes/pinstripe/devto
ols/orion.css.rej
patching file browser/themes/winstripe/devtools/orion.css
Hunk #1 FAILED at 1
Hunk #3 succeeded at 150 with fuzz 2 (offset -6 lines).
1 out of 3 hunks FAILED -- saving rejects to file browser/themes/winstripe/devto
ols/orion.css.rej
abort: patch failed to apply

	Rob Campbell [:rc] (:robcee)
	Comment 29 • 12 years ago

yeah, it's quite badly out of date, Honza. We're planning to update this soon.

	Mihai Sucan [:msucan] Assignee
	Comment 30 • 12 years ago

Rebased patch.

	Anton Kovalyov (:anton)
	Comment 31 • 11 years ago

Closing this as WONTFIX since we're removing Orion. Sorrrryyy.