718816 - Orion upstream update (with debugger ruler support code)

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

[Mihai Sucan [:msucan]]

We need to update Orion from upstream. This time we will include the upsream debugger ruler support code needed to fix bug 707987.

Please note that this patch will include only the orion.js file changes and minimal (or no) changes for the Source Editor's usage of Orion. The actual Source Editor changes to add the debugger ruler API will happen in bug 707987.

Comment 1

[Mihai Sucan [:msucan]]

Attachment: proposed patch

Proposed patch. Please note that the diff churn is quite greater than the *actual* number of changes. If you check with a visual diff tool you'll see there have been only minor changes. I had to reorder the orion.js code (see the change in Makefile.dryice.js), plus there was a change in upstream newlines for global.js. There's also a change in the Orion async init code, but I haven't noticed any breakage, so hopefully it's fine.

The code reorder is the only required change for bug 707987.

Comment 2

[Mihai Sucan [:msucan]]

Attachment: updated patch

Updated patch with more fixes from upstream. This also includes the fix from Alex.

Comment 3

[Mihai Sucan [:msucan]]

Attachment: more fixes

Rebased patch and included more fixes:

- fix for bug 717631 - selection now behaves correctly, please test to confirm. Works fine on my system.

- context menu-related fix. When you select some text the cut/copy/paste/delete menu items will be updated correctly in the context menu.

- undo/redo bug fixes: deletions that start at offset 0 were broken. UndoStack had problems managing such kinds of actions. Also fixed a case where the TextModel incorrectly generated ModelChanged events for empty changes (no added/removed chars).

- fix for the Source Editor: the undo (ctrl-z) and redo (ctrl-shift-z) keyboard shortcuts incorrectly generated two calls to the same function (ouch).

Please note that the fix for bug 717631 caused a minor visual change: padding is no longer applied within the line div, it is applied to the whole view, which means the selection is also moved 4px to the right. This is a minor inconvenience compared to the bug we are fixing.

The Orion team has submitted a fix to allow paddings on line divs, but that code change might cause regressions, so I'm postponing that change for a future Orion update.

Looking forward to your review. Thank you!

Comment 4

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

Comment on attachment 595743 [details] [diff] [review]
more fixes

rejiggered define().

in orion.js

+		/** @ignore */
+		_binarySearch: function (array, offset) {

I can't @ignore! :)

Annotation bits moved around.

 		_handleBlur: function (e) {
 			if (!e) { e = window.event; }
 			if (this._ignoreBlur) { return; }
@@ -4515,16 +4555,24 @@ define(['orion/textview/textModel', 'ori
 			if (!e) { e = window.event; }
 			this._dropTarget = false;
 			this._dragOffset = -1;
 			if (this.isListening("DragEnd")) {

...
+				if (e.dataTransfer.dropEffect === "none" && !e.dataTransfer.mozUserCancelled) {
+					this._fixCaret();
+				}

wonder if we should file a bug to investigate this further? Could be an editor bug. Have you tried reproducing this?

...

* Feature in Firefox. When a key is held down the browser sends 

I like that these are features.

+		_isDocumentReady: function() {
+			var frameDocument = this._frameDocument;
+			if (!frameDocument) { return false; }
+			if (frameDocument.readyState === "complete") {
+				return true;
+			} else if (frameDocument.readyState === "interactive" && isFirefox) {
+				/*
+				* Bug in Firefox. Firefox does not change the document ready state to complete 
+				* all the time. The fix is to wait for the ready state to be "interactive" and check that 
+				* all css rules are initialized.
+				*/

wonder if that's something that we should investigate deeper as well? Do you know if there's a bug on file for this and is it reproducible?

+		_isLinkURL: function(string) {
+			return string.toLowerCase().lastIndexOf(".css") === string.length - 4;
+		},

I am not sure what the purpose of this is, but it doesn't look like it's testing if that's actually a "Link URL". Stylesheet? I guess this is just to exclude certain stylesheets from being processed, but it doesn't really seem to be validating it other than checking the file suffix.

Nothing jumps out at me as unreasonable here.

How's our testsuite coming along?

Comment 5

[Mihai Sucan [:msucan]]

(In reply to Rob Campbell [:rc] (robcee) from comment #4)
> @@ -4515,16 +4555,24 @@ define(['orion/textview/textModel', 'ori
>  			if (!e) { e = window.event; }
>  			this._dropTarget = false;
>  			this._dragOffset = -1;
>  			if (this.isListening("DragEnd")) {
> 
> ...
> +				if (e.dataTransfer.dropEffect === "none" &&
> !e.dataTransfer.mozUserCancelled) {
> +					this._fixCaret();
> +				}
> 
> wonder if we should file a bug to investigate this further? Could be an
> editor bug. Have you tried reproducing this?

We play a lot with the draggable and contenteditable booleans of the elements inside Orion, to work around worse bugs in Firefox, hence we have these problems. We need the fix for bug 614974 then we can remove the major workaround we have for it. I expect caret will render properly after that.

I didn't see any bug filed for this. Plus, this is something on the fringe of "don't do this at all! it's obvious you get unexpected/broken/weird behavior!" (this = the workaround for bug 614974). Do you think we should file the bug about such breakage after a workaround, nonetheless?


> * Feature in Firefox. When a key is held down the browser sends 
> 
> I like that these are features.
> 
> +		_isDocumentReady: function() {
> +			var frameDocument = this._frameDocument;
> +			if (!frameDocument) { return false; }
> +			if (frameDocument.readyState === "complete") {
> +				return true;
> +			} else if (frameDocument.readyState === "interactive" && isFirefox) {
> +				/*
> +				* Bug in Firefox. Firefox does not change the document ready state to
> complete 
> +				* all the time. The fix is to wait for the ready state to be
> "interactive" and check that 
> +				* all css rules are initialized.
> +				*/
> 
> wonder if that's something that we should investigate deeper as well? Do you
> know if there's a bug on file for this and is it reproducible?

I didn't see if they filed any bug report about this specific issue. Shall I email them to ask nicely for that?


> How's our testsuite coming along?

As you have noticed, I didn't take the bug, yet. Shall I do that?


Thanks for your review+!

Comment 6

[Mihai Sucan [:msucan]]

Attachment: rebase

Comment 7

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

(In reply to Mihai Sucan [:msucan] from comment #5)
> (In reply to Rob Campbell [:rc] (robcee) from comment #4)
[...]
> > How's our testsuite coming along?
> 
> As you have noticed, I didn't take the bug, yet. Shall I do that?

Yes please!

> Thanks for your review+!

thank you. This is landable?

Comment 8

[Mihai Sucan [:msucan]]

This is landable, but I prefer to land it together with the entire patch queue. I keep the patch in my queue to notice any potential breakage or regressions.

Comment 9

[Mihai Sucan [:msucan]]

Comment on attachment 598360 [details] [diff] [review]
rebase

Landed:
https://hg.mozilla.org/integration/fx-team/rev/a9d68fc52703

Comment 10

[Mihai Sucan [:msucan]]

Comment on attachment 598360 [details] [diff] [review]
rebase

backedout due to test failure:
https://hg.mozilla.org/integration/fx-team/rev/9bbb35269c58

WinXP machines have:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/sourceeditor/test/browser_bug725388_mouse_events.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/sourceeditor/test/browser_bug725388_mouse_events.js | Found a after previous test timed out

See: https://tbpl.mozilla.org/?tree=Fx-Team&rev=00bce5149395

Comment 11

[Mihai Sucan [:msucan]]

Attachment: [in-fx-team] improved test fix

Improved the test fix.

Try server shows no failures after a good number of runs:

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

Going to land the patch queue.

Comment 12

[Mihai Sucan [:msucan]]

Comment on attachment 599238 [details] [diff] [review]
[in-fx-team] improved test fix

Landed:
https://hg.mozilla.org/integration/fx-team/rev/f6a20bf85118

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/f6a20bf85118