As per bug 652990, we need to namespace the global 'documentLoaded' that we attach to each document.

This patch unfortunately seems to cause some tests to fail, and I can't figure out why because it's just replacing a variable with a new variable name. When running: mozmill -t mozmill-tests/mozmill-tests/tests/functional/testTabbedBrowsing I get 3 new test failures: TEST-UNEXPECTED-FAIL | /Users/harth/repos/mozmill-tests/tests/functional/testTabbedBrowsing/testBackgroundTabScrolling.js | testScrollBackgroundTabIntoView TEST-UNEXPECTED-FAIL | /Users/harth/repos/mozmill-tests/tests/functional/testTabbedBrowsing/testOpenInBackground.js | testOpenInBackgroundTab TEST-UNEXPECTED-FAIL | /Users/harth/repos/mozmill-tests/tests/functional/testTabbedBrowsing/testOpenInForeground.js | testOpenInForegroundTab INFO Passed: 13 INFO Failed: 3 INFO Skipped: 0 I get this error when running mozmill-tests/tests/functional/testTabbedBrowsing/testBackgroundTabScrolling.js: ERROR | Test Failure: {"exception": {"stack": "waitFor([object Proxy],\"Window content has been loaded.\")@resource://mozmill/modules/utils.js:449\n@:0\n", "message": "Window content has been loaded.", "fileName": "resource://mozmill/modules/utils.js", "name": "Error", "lineNumber": 449}}

A change like this will break our Mozmill tests for the new release. Reason is the modal dialog and non-modal window handler which are using this window property. Not sure if we can get-rid of those instances, but right now this upcoming change has massive impact to our test-runs. Especially that modal dialogs cannot be closed anymore and the whole test-run will be aborted. We should figure out if we could implement a less-invasive way.

(In reply to comment #2) > A change like this will break our Mozmill tests for the new release. Reason > is the modal dialog and non-modal window handler which are using this window > property. > > Not sure if we can get-rid of those instances, but right now this upcoming > change has massive impact to our test-runs. Especially that modal dialogs > cannot be closed anymore and the whole test-run will be aborted. > > We should figure out if we could implement a less-invasive way. Oh wow, it's used in the shared libs? There should be some method on the controller that wraps that, like a controller.isLoaded(), would that work?

I don't have time to check this anymore before my vacation, so one of you will have to check it. If it's doable we definitely need an interim beta we could use to update the tests.

(In reply to comment #2) > A change like this will break our Mozmill tests for the new release. Reason > is the modal dialog and non-modal window handler which are using this window > property. > > Not sure if we can get-rid of those instances, but right now this upcoming > change has massive impact to our test-runs. Especially that modal dialogs > cannot be closed anymore and the whole test-run will be aborted. > > We should figure out if we could implement a less-invasive way. There is no less invasive way. The shared libraries simply should not be depending on the private data of the implementation. This is one of those cases where an API was needed to be created, and no one raised or realized it (myself included, as I've reviewed all the code in question). Instead, code was written that depends on implementation specific, private behavior. Shared libraries should _absolutely_never_ depend on internal implementation-specific behaviors. If they do need access to the data contained therein, an API should be created for that so we don't end up in this situation again. We all need to think about this going forward as we finish 2.0 and as we update the shared libraries. In this particular case, we'll make the missing API, ensure it works in the shared libraries and keep moving forward. Please please please uplift bugs like this instead of working around them. It will cause us all a lot less trouble in the future. And yes, obviously, we'd need another beta once this change is in and a patch is made to the libraries. We'll have to land both at once to ensure we maintain working compatibility.

Alright, this exposes a isLoaded() on the controller that checks if the window is loaded, also namespaces all those documentLoadeds for the AMO review. I have a patch for mozmill-tests coming up.

This gets mozmill-tests working with the mozmill patch.

Landed the mozmill side: master: https://github.com/mozautomation/mozmill/commit/d77446174992024764d92dd7eaed49dbb9d43648 hotfix-1.5: https://github.com/mozautomation/mozmill/commit/583342c357769e7644835cff88de42db10c5cd57 leaving bug open to track landing the mozmill-tests fixes. Mozmill QA NOTE: This change will BREAK TESTS until the remaining patch is reviewed and checked in. I can't remember when Henrik is coming back - does someone else from QA want to take this review and land it?

Frankly, I'm really concerned about this check-in here. In the current state it breaks all the tests which get executed by Mozmill Crowd or users who are running the latest Mozmill release. For situations like those you should really wait until we have updated our tests, so a smooth transition is possible. Why there was a rush to get this in so quickly? Did I miss something?

This was done to address AMO review comments: https://bugzilla.mozilla.org/show_bug.cgi?id=652990

Comment on attachment 537612 [details] [diff] [review] export controller.isLoaded() and replace 'documentLoaded' with 'mozmillDocumentLoaded' >diff --git a/mozmill/docs/_build/doctrees/environment.pickle b/mozmill/docs/_build/doctrees/environment.pickle >index e86b375..06e69b1 100644 >Binary files a/mozmill/docs/_build/doctrees/environment.pickle and b/mozmill/docs/_build/doctrees/environment.pickle differ >diff --git a/mozmill/docs/_build/doctrees/index.doctree b/mozmill/docs/_build/doctrees/index.doctree >index 15c7914..e0487cb 100644 >Binary files a/mozmill/docs/_build/doctrees/index.doctree and b/mozmill/docs/_build/doctrees/index.doctree differ What has changed for those entries? Why are those in this patch? >+MozMillController.prototype.isLoaded = function() { >+ return this.window.mozmillDocumentLoaded; >+}; That's not enough for an API. It does not obey the new feature of handling documentLoaded events for iframes or embedded browsers in content-space. It definitely needs a window parameter, but can fallback to this.window if none has been specified.

(In reply to comment #11) > Comment on attachment 537612 [details] [diff] [review] [review] > export controller.isLoaded() and replace 'documentLoaded' with > 'mozmillDocumentLoaded' > > >diff --git a/mozmill/docs/_build/doctrees/environment.pickle b/mozmill/docs/_build/doctrees/environment.pickle > >index e86b375..06e69b1 100644 > >Binary files a/mozmill/docs/_build/doctrees/environment.pickle and b/mozmill/docs/_build/doctrees/environment.pickle differ > >diff --git a/mozmill/docs/_build/doctrees/index.doctree b/mozmill/docs/_build/doctrees/index.doctree > >index 15c7914..e0487cb 100644 > >Binary files a/mozmill/docs/_build/doctrees/index.doctree and b/mozmill/docs/_build/doctrees/index.doctree differ > > What has changed for those entries? Why are those in this patch? They're not in the raw patch as far as I can tell, where are you seeing these? > > >+MozMillController.prototype.isLoaded = function() { > >+ return this.window.mozmillDocumentLoaded; > >+}; > > That's not enough for an API. It does not obey the new feature of handling > documentLoaded events for iframes or embedded browsers in content-space. It > definitely needs a window parameter, but can fallback to this.window if none > has been specified. You can make a controller for the iframe.

Comment on attachment 537615 [details] [diff] [review] replace documentLoaded with controller.isLoaded() in mozmill-tests >+++ b/lib/modal-dialog.js Mon Jun 06 13:09:56 2011 -0700 >@@ -116,9 +116,11 @@ > observe : function mdObserver_observe(aSubject, aTopic, aData) { > // Once the window has been found and loaded we can execute the callback > var window = this.findWindow(); >- if (window && ("documentLoaded" in window)) { >+ var controller = new mozmill.controller.MozMillController(window); >+ >+ if (window && controller.isLoaded()) { > try { >- this._callback(new mozmill.controller.MozMillController(window)); >+ this._callback(controller); That will not work if there is a delay in opening the modal dialog. With your change we will throw an exception and do not start another timer. As talked in the meeting I will come up with an alternative approach, which will hopefully work in all cases.

Can we place get this backed out or have a beta4 without that patch included? This change touched a very fragile area of code and we really have to make sure it works perfectly. It needs more time for me to come up with a working patch and ensure everything works.

Comment on attachment 537612 [details] [diff] [review] export controller.isLoaded() and replace 'documentLoaded' with 'mozmillDocumentLoaded' After additional tests I still stay at my position that this patch is not ready to land as it is. We really have to back it out and fix it correctly. Here some more comments in my additional review: >@@ -309,8 +309,8 @@ var MozMillController = function (window) { > > utils.waitFor(function() { > return window != null && >- ("documentLoaded" in window) && >- window.documentLoaded; >+ ("mozmillDocumentLoaded" in window) && >+ window.mozmillDocumentLoaded; In the constructor you will have to use the isLoaded() method, which you have added to this class. >+MozMillController.prototype.isLoaded = function() { >+ return this.window.mozmillDocumentLoaded; >+}; That's broken because "mozmillDocumentLoaded" will not always be present. See the checks from above how to correctly implement that. > MozMillController.prototype.waitFor = function(callback, message, timeout, > interval, thisObject) { > utils.waitFor(callback, message, timeout, interval, thisObject); >@@ -1285,7 +1289,7 @@ function browserAdditions (controller) { > > // Wait until the content in the tab has been loaded > this.waitFor(function() { >- return ("documentLoaded" in owner && owner.documentLoaded); >+ return ("mozmillDocumentLoaded" in owner && owner.mozmillDocumentLoaded); Same as above. Why don't you use the isLoaded() method?

I see why you didn't do that. We have different owners here. In the constructor it's the ChromeWindow, while in the waitForPageLoad method we operate on the content window. Please see my comment 11 again. I'm sure we have to add a parameter to isLoaded().

With this follow-up all my concerns should be addressed. I have also updated the patch for the shared modules over on bug 658231 and everything seems to work for me with a Nightly build. Geo can you please double-check with a test-run? Instead of backing out the original patch we could land a follow-up and build beta4 with it included. That's probably the easiest solution.

Comment on attachment 541047 [details] [diff] [review] Proposed follow-up Haven't done the test run (per irc, send me a tarball of the patched mozmill) but code looks ok to me. f+

Prebuilt 1.5.4b3 with the patch included can be found here: http://people.mozilla.com/~hskupin/downloads/mozmill-1.5.4b3.tar.gz I have also landed our patches for the shared modules, so we simply need this final patch landed.

(In reply to comment #17) > Created attachment 541047 [details] [diff] [review] [review] Couldn't apply the patch to a fresh checkout of mozauto/hotfix-1.5, could you push to a remote branch?

Not sure why you have problems. It works pretty well for me. But here my newly created remote branch: https://github.com/whimboo/mozmill/tree/documentLoaded

Comment on attachment 541047 [details] [diff] [review] Proposed follow-up Thanks for the upgrade. I'm not sure why the patch didn't apply, but I've hand patched it and it works with the new mozmill-tests. I'll check it in.

hotfix-1.5: https://github.com/mozautomation/mozmill/commit/f1c852b67fbe0b834455d479ff2ba14d9b885e49 master: https://github.com/mozautomation/mozmill/commit/eb13aa91b8d7e5f157f10253408a05821bc50b04

So the fix to master doesn't in fact work and is at least partially responsible for the mutt test failure in https://bugzilla.mozilla.org/show_bug.cgi?id=644249#c10 Note that in mozmill 1.5, there is an owner: https://github.com/mozautomation/mozmill/blob/hotfix-1.5/mozmill/mozmill/extension/resource/modules/controller.js#L1269 But not in mozmill 2.0: https://github.com/mozautomation/mozmill/blob/master/mozmill/mozmill/extension/resource/modules/controller.js#L897 Does anyone know the reason for the change? I'll hit blame and try to do some detective work. Notably, this breaks the mutt tests which pretty much stops development. A week ago, all mutt tests passed. Now there are two failures, this being (at least) one of them. If you're developing 2.0, please run the tests before checking in. We'll have automation someday (which is a safety not, not a first line of defense), but running `mutt` before pushing is pretty important.

This patch just changes owner to null as owner is not defined. It fixes the failing mutt test. I have no idea if it is sufficient, but having owner undefined certainly breaks things.

Comment on attachment 542637 [details] [diff] [review] use null instead of owner That's wrong. If we want to have an intermediate fix it should be isLoaded(tab)

master: https://github.com/mozautomation/mozmill/commit/b4c2a8a6eb743a0203a8cf66c50a97fce79f2341

Comment on attachment 542664 [details] [diff] [review] use tab for some reason fixed with the prev checkin, thanks!

Yeah, all mutt tests currently pass \o/ Let's keep it that way. won't push