Last Comment Bug 718816 - Orion upstream update (with debugger ruler support code)
: Orion upstream update (with debugger ruler support code)
Status: RESOLVED FIXED
[sourceeditor][orion][fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: Firefox 13
Assigned To: Mihai Sucan [:msucan]
:
Mentors:
Depends on:
Blocks: 701347 707987 717631
  Show dependency treegraph
 
Reported: 2012-01-17 13:43 PST by Mihai Sucan [:msucan]
Modified: 2012-02-22 09:38 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed patch (82.22 KB, patch)
2012-01-26 04:26 PST, Mihai Sucan [:msucan]
no flags Details | Diff | Review
updated patch (129.44 KB, patch)
2012-01-27 12:52 PST, Mihai Sucan [:msucan]
no flags Details | Diff | Review
more fixes (111.35 KB, patch)
2012-02-09 07:53 PST, Mihai Sucan [:msucan]
rcampbell: review+
Details | Diff | Review
rebase (112.35 KB, patch)
2012-02-17 13:53 PST, Mihai Sucan [:msucan]
no flags Details | Diff | Review
[in-fx-team] improved test fix (114.13 KB, patch)
2012-02-21 10:07 PST, Mihai Sucan [:msucan]
no flags Details | Diff | Review

Description Mihai Sucan [:msucan] 2012-01-17 13:43:19 PST
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] 2012-01-26 04:26:04 PST
Created attachment 591753 [details] [diff] [review]
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] 2012-01-27 12:52:00 PST
Created attachment 592225 [details] [diff] [review]
updated patch

Updated patch with more fixes from upstream. This also includes the fix from Alex.
Comment 3 Mihai Sucan [:msucan] 2012-02-09 07:53:38 PST
Created attachment 595743 [details] [diff] [review]
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) 2012-02-14 07:50:40 PST
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] 2012-02-15 13:26:32 PST
(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] 2012-02-17 13:53:33 PST
Created attachment 598360 [details] [diff] [review]
rebase
Comment 7 Rob Campbell [:rc] (:robcee) 2012-02-17 14:18:44 PST
(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] 2012-02-18 02:56:04 PST
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] 2012-02-18 04:06:55 PST
Comment on attachment 598360 [details] [diff] [review]
rebase

Landed:
https://hg.mozilla.org/integration/fx-team/rev/a9d68fc52703
Comment 10 Mihai Sucan [:msucan] 2012-02-18 08:42:04 PST
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] 2012-02-21 10:07:44 PST
Created attachment 599238 [details] [diff] [review]
[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] 2012-02-21 10:37:41 PST
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) 2012-02-22 09:38:43 PST
https://hg.mozilla.org/mozilla-central/rev/f6a20bf85118

Note You need to log in before you can comment on or make changes to this bug.