685865 - Don't special case tabs for mozmillDocumentLoaded

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

One big mistake we made in the past was to special case the handling of the mozmillDocumentLoaded state for tabs. It absolutely makes no sense and I'm not sure how this would even work with e10s. We should stay in the scope of content and don't bridge over to chrome. Also in all other cases the window itself gets this property set.

I run into this problem while working on the WindowWrapper class for in-content windows like normal tabs, or frames on bug 639870. Problem here is our MozMillController constructor which calls isLoaded(). This method only checks the internal window assigned to the controller and doesn't know anything about tabs and how to get the appropriate browser element (content -> chrome).

Given that when a page is reloaded only the document gets replaced, but not the window, all properties set on this window will remain. So we are absolutely fine and stopping our behavior and special casing the tab window load state.

A patch is upcoming. I hope that we can fold this into 2.0 and even 1.5.4. Especially the latter version is kinda important for our API rewrite.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Patch v1 (mozmill 1.5.x) [checked-in]

Comment 2

[cmtalbert]

Comment on attachment 559456 [details] [diff] [review]
Patch v1 (mozmill 1.5.x) [checked-in]

Ok, It takes some judicious mxr'ing to get a handle on what is going on here.  But essentially the old problem was that we'd sometimes set the mozmillDocumentLoaded attribute on the tabbrowser object rather than on the window of the document in question.  We always want that set on the window of the document in question so we can check for it on the window element when we do our waitFor's.  So, good catch.
Some stuff to help with understanding:
document.defaultView is:
http://www.whatwg.org/specs/web-apps/current-work/multipage/browsers.html#dom-document-defaultview

And the old way we were using getBrowserForDocument:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#317
which doesn't give us what we want.

r+

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Landed on hotfix-1.5:
https://github.com/mozautomation/mozmill/commit/e252680d73a62ebb87538049853ebd2eddf98bfe

Working now on a Mozmill 2.0 fix.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Follow-up (hotfix-1.5)

While working on the patch for Mozmill 2.0 one of the tests we have under Mutt revealed a failure if the document is 'null'. So we need an extra check here.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Patch v1 (master)

Patch for Mozmill 2.0. Due to the new bug 686030 I have had to disable a test in the test_waitForPageLoad.js module. Further I have switched the last two tests so we do not run into bug 672884. Otherwise it's identical to

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Clint, any chance you can review this code ASAP? Especially the hotfix patch for 1.5.5. Thanks.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Follow-up for hotfix-1.5 landed as:
https://github.com/mozautomation/mozmill/commit/dccafce1a032e8062136e845054e72d89a636035

Master branch patch landed as:
https://github.com/mozautomation/mozmill/commit/8dd1bda043dd035c3864076f20e554496aaded9b