702331 - Update Orion from upstream

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Mihai Sucan [:msucan]]

We need an Orion update for:

- upstream has a number of fixes that make our current patches on top of it, obsolete. Mainly the drag and drop support.

- more bug fixes that have landed since bug697407: async init is one of the important ones.

- potentially we'll include more features from upstream (depends on the complexity of the task).

Comment 1

[Mihai Sucan [:msucan]]

Attachment: proposed patch

Proposed patch. Changes:

1. latest upstream Orion with a good number of changes.
2. had to change quite some code in source-editor-orion.jsm. Orion initialization is now async and they changed the event listener API and some more stuff.
3. had to change scratchpad tests - they started to fail because Orion init is now async and they don't wait for the right events.
4. switched to our own mozilla.css - forked their orion.css. They have vendor-specific prefixes which show as errors in the Error Console. We are now free to do whatever we want. I continue to bundle orion.css as a measure of comparison - so we don't forget to update our CSS as needed.

Orion-specific changes:
1. async init.
2. event listener API.
3. drag-and-drop support from upstream.
4. they have included all the fixes we had monkey-patched inside our last Orion update.
5. fast horizontal scrolling with very long line.
6. typing in very long lines is now faster as well.
7. fixes for css syntax highlighting.
8. proper API for changing readOnly mode and other Orion settings.
9. support for changing the viewport CSS class names, so we can now have a readOnly-specific styling - see bug 680376.
10. Orion now uses requirejs but it includes minimal require() and define() shims we use in our code, so no problem here.
11. support for middle-click paste on Linux.
12. support for expand tabs to spaces. We had our own code which did this, now I was able to clean up some parts of source-editor-orion.jsm.
13. support for editor focus/blur events and hasFocus().

Where appropriate I have updated SourceEditor to use the new APIs. Please let me know if you find any problems.

Thank you!

Comment 2

[Mihai Sucan [:msucan]]

The patch here fixes these following bugs: 680376, 680465, 687861, 687865, 695035, 681360.

In related news, the try server results are red:

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

Will look into fixing the failures and I'll update the patch when I get green result.

Comment 3

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

Comment on attachment 579155 [details] [diff] [review]
proposed patch

thanks, Mihai. Resubmit review request when you figure out the try errors.

Comment 4

[Mihai Sucan [:msucan]]

Attachment: updated patch - test fixes

Got all the tests fixed. There was one Scratchpad test I forgot, in browser/components/sessionstore, and two other failures related to Macs and their menu items having the checked=false attribute by default.

Green results:
https://tbpl.mozilla.org/?tree=Try&rev=0afc1dce5033

Builds and logs:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-0afc1dce5033

Comment 5

[Mihai Sucan [:msucan]]

Attachment: updated patch - textarea fixes

Updated the patch to fix some test failures with the textarea fallback.

We should file a follow up to remove that. When would that be an appropriate time?

Comment 6

[Mihai Sucan [:msucan]]

Attachment: fixes for bug 695035 - middle-click paste on Linux

Updated the patch to fix a problem in Orion related to bug 695035. The upstream fix worked, but it seems there was a problem when running with chrome privileges (as it runs in our Source Editor). The fix was trivial - two lines.

With this occasion I added a test for bug 695035. Please note that I added head.js and a waitForSelection() function that's very similar to waitForClipboard(). This is going to be used by the test for bug 695032 as well.

Looking forward for your review. Thank you!

Comment 7

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

all tests pass.

Comment 8

[Mihai Sucan [:msucan]]

Attachment: fix for right-click

The joy of working on other Source Editor bugs - I get to find regressions and bugs. :)

This update has about 5 lines of changes to fix a problem with right-clicking in the editor. See bug 709476 and see https://bugs.eclipse.org/bugs/show_bug.cgi?id=366312

Comment 9

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

it's significantly more than 5 lines, but OK.

Comment 10

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

Comment on attachment 580653 [details] [diff] [review]
fix for right-click

-}
\ No newline at end of file
+}

:)

openScratchpad() is a nice addition to scratchpad's head.js file. I hope there's one for the Style Editor! (continues)

in mozilla.css:

+.view {
+ background: #fff;
 }
 
+.readonly > .view {
+ background: #f6f6f6;
+}

Is this file an import? The indentation's kinda funny.

+.token_space {
+ /* images/white_space.png */
+ background-image: url("data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAkAAAAJCAIAAABv85FHAAAABnRSTlMA/wAAAACkwsAdAAAAIUlEQVR4nGP4z8CAC+GUIEXuABhgkTuABEiRw2cmae4EAH05X7xDolNRAAAAAElFTkSuQmCC");
+ background-repeat: no-repeat;
+   background-positio

what are these for?

Is this file also incorporated from the orion build with a few Mozilla-ish modifications?

+ function FoldingAnnotation (projectionModel, type, start, end, expandedHTML, expandedStyle, collapsedHTML, collapsedStyle) {

that's new!

Also, the Orion guys don't really worry about long lines, do they? :)\

looks like they've really cleaned up a lot of their event mechanism.

        /*
        * Bug in Firefox.  For some reason, the caret does not show after the
        * view is refreshed.  The fix is to toggle the contentEditable state and
        * force the clientDiv to loose and receive focus if the it is focused.
        */
        if (isFirefox) {

typos in that line. I should really be filing bugs in Eclipse's bugzilla and filing patches upstream to fix their documentation. Not the first one of these I've seen today. :)

and copy and pasted further down in _handleBodyMouseUp:

several "if (isFirefox)" tests in here...

+   _compare: function (s1, s2) {

for when a simple == just isn't enough.

+     /*
+     * Feature in WebKit.  In WebKit, window load will not wait for the style sheets
+     * to be loaded unless there is script element after the style sheet link elements.
+     */
+     html.push("<script>");
+     html.push("var waitForStyleSheets = true;");
+     html.push("</script>");

that's an interesting feature. 

in _createView:

+     }
+     /*
+     * Bug in Firefox.  Firefox does not send window load event if document.write
+     * is done inside of the frame load event handler.
+     */
+     if (isFirefox && !this._sync) {
+       setTimeout(write, 0);
+     } else {
+       write();
+     }
+     if (this._sync) {
+       this._createContent();
+     }

yeesh

Not real happy about that blob and the previous one (the webkit "feature").

+     var frameDocument = this._frameDocument;
+     /*
+     * Bug in Safari.  Safari sends the window load event before the
+     * style sheets are loaded. The fix is to defer creation of the
+     * contents until the document readyState changes to complete.
+     */
+     var self = this;
+     if (!this._sync && frameDocument.readyState !== "complete") {
+       setTimeout(function() {
+         self._createContent();
+       }, 10);
+       return;
+     }

and this one.

It feels like they're doing a great deal of hackery to get around browser page load quirks.

some of this copy paste code would make me feel better if we had decent unittests for it.

Unsurprisingly, they have to go through the same hoops as we do to calculate scrollbar widths. We should really get an API call for that.

skipping scanner and grammar code.

Lots of changes. Mostly for the better. Some of the tricky stuff they're doing makes me nervous. And a lot of the drag and drop, context menu stuff and clipboard handling has been completely revamped or written from scratch. You are right to want to test this.

R+

Things to test:
Context menus,
Drag and drop
Entering/Exiting Source Editors (with and without focus)
Copy and Pastes
Undo/Redo
Everything else.

Comment 11

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

https://hg.mozilla.org/integration/fx-team/rev/847e7bead2db

Comment 12

[Mihai Sucan [:msucan]]

(In reply to Rob Campbell [:rc] (robcee) from comment #10)
> Comment on attachment 580653 [details] [diff] [review]
> fix for right-click
> 
> -}
> \ No newline at end of file
> +}
> 
> :)

Added automatically by the editor. :)


> in mozilla.css:
> 
> +.view {
> + background: #fff;
>  }
>  
> +.readonly > .view {
> + background: #f6f6f6;
> +}
> 
> Is this file an import? The indentation's kinda funny.

Yep, will look into fixing styling for mozilla.css in some subsequent file update. Maybe bug 709006 will fix it. ;)


> +.token_space {
> + /* images/white_space.png */
> + background-image:
> url("data:image/png;base64,
> iVBORw0KGgoAAAANSUhEUgAAAAkAAAAJCAIAAABv85FHAAAABnRSTlMA/
> wAAAACkwsAdAAAAIUlEQVR4nGP4z8CAC+GUIEXuABhgkTuABEiRw2cmae4EAH05X7xDolNRAAAAAE
> lFTkSuQmCC");
> + background-repeat: no-repeat;
> +   background-positio
> 
> what are these for?
> 
> Is this file also incorporated from the orion build with a few Mozilla-ish
> modifications?

Yes, this is orion.css copy/pasted into mozilla.css, with changes we need.


> + function FoldingAnnotation (projectionModel, type, start, end,
> expandedHTML, expandedStyle, collapsedHTML, collapsedStyle) {
> 
> that's new!

Yep!

> Also, the Orion guys don't really worry about long lines, do they? :)\

Yeah, they really have a different coding style.

> looks like they've really cleaned up a lot of their event mechanism.

Indeed.

>         /*
>         * Bug in Firefox.  For some reason, the caret does not show after the
>         * view is refreshed.  The fix is to toggle the contentEditable state
> and
>         * force the clientDiv to loose and receive focus if the it is
> focused.
>         */
>         if (isFirefox) {
> 
> typos in that line. I should really be filing bugs in Eclipse's bugzilla and
> filing patches upstream to fix their documentation. Not the first one of
> these I've seen today. :)

Yeah, they do have typos at time in the code, but these are going to be fixed. ;)


> It feels like they're doing a great deal of hackery to get around browser
> page load quirks.

True...

> some of this copy paste code would make me feel better if we had decent
> unittests for it.

The copy/paste code makes me want clipboardData support implemented in Firefox ASAP. Writing tests and fixing every single corner case is overly complex without some clipboard API.


> Unsurprisingly, they have to go through the same hoops as we do to calculate
> scrollbar widths. We should really get an API call for that.

True.

> Lots of changes. Mostly for the better. Some of the tricky stuff they're
> doing makes me nervous. And a lot of the drag and drop, context menu stuff
> and clipboard handling has been completely revamped or written from scratch.
> You are right to want to test this.

Yep, there's a lot of stuff changed.


> R+
> 
> Things to test:
> Context menus,
> Drag and drop
> Entering/Exiting Source Editors (with and without focus)
> Copy and Pastes
> Undo/Redo
> Everything else.

We need to fix bug 668320...


Thank you for the review and for landing the patch!

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/847e7bead2db