879371 - Add support for Firefox Metro to Mozmill

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Andrei Eftimie]

Tracking bug for Mozmill issues under metro.

Comment 1

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

Attachment: Quit Firefox Metro v1 (checked-in)

With this patch Mozmill is able to close Firefox Metro.

Comment 2

[cmtalbert]

Comment on attachment 758890 [details] [diff] [review]
Quit Firefox Metro v1 (checked-in)

Review of attachment 758890 [details] [diff] [review]:
-----------------------------------------------------------------

Yep, this looks like what we should have done all along. :-)

Comment 3

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

Comment on attachment 758890 [details] [diff] [review]
Quit Firefox Metro v1 (checked-in)

Landed on master:
https://github.com/mozilla/mozmill/commit/f5cf2c42663c21f1b8057abbd147373f3a77e957

Comment 4

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

Attachment: Tabs and window handling v1

Next step and most likely the last one for a full metro support. This patch implements:

* Bump to Mozrunner 5.16 (still needs a 5.17 release)
* An update for the application id map to support Firefox Metro
* Removal of SeaMonkey support and never used preferences additions
* A new getBrowserObject() method to retrieve the underlying global browser object
* Updates for window event listeners (no need to do it via the browser object anymore!!)

One thing I'm not sure about yet is if we want to keep getBrowserObject in utils.js or move it into windows.js. I will have to think about it.

Andreea and Andrei, can you please test this patch with Firefox Metro and Firefox Desktop? Does it work for you? I would also like to get some feedback how Firefox tests run on OS X with those changes.

Comment 5

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

Attachment: example test for metro which works with tabs

Just a test for Firefox Metro to check if tabs are working and pages are loaded correctly.

Comment 6

[Andrei Eftimie]

Working fine on OSX:
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbe5fc5eb

And on Metro.

The example test is failing tough with:  "message": "'activeTabIndex' not supported.",

Comment 7

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

(In reply to Andrei Eftimie from comment #6)
> The example test is failing tough with:  "message": "'activeTabIndex' not
> supported.",

Yes, that's my fault. 'FirefoxMetro' should become 'MetroFirefox' in the appropriate methods. Just an error in typing. I will see what to do with the getBrowerObject method, which should also benefit from this switch statement and not rely on a check for gBrowser vs. Browser.

Comment 8

[Andreea Matei [:AndreeaMatei]]

It is working now with the latest change.

Comment 9

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

Comment on attachment 759787 [details] [diff] [review]
Tabs and window handling v1

Review of attachment 759787 [details] [diff] [review]:
-----------------------------------------------------------------

An updated patch will follow shortly.

Comment 10

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

Attachment: Tabs and window handling v1.1

Updated patch which now includes the test but which we cannot run by default as of now.

Comment 11

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 759935 [details] [diff] [review]
Tabs and window handling v1.1

Review of attachment 759935 [details] [diff] [review]:
-----------------------------------------------------------------

Works with 2.0 and in Metro mode.

Comment 12

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

Comment on attachment 759935 [details] [diff] [review]
Tabs and window handling v1.1

Review of attachment 759935 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Andreea. I'm going to ask for review from Clint now.

Comment 13

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

With my changes in bug 880839 the command line option for metro will change from firefoxmetro to metrofirefox, so it's in sync with the real application name.

Comment 14

[cmtalbert]

Comment on attachment 759935 [details] [diff] [review]
Tabs and window handling v1.1

Review of attachment 759935 [details] [diff] [review]:
-----------------------------------------------------------------

just one nit. I think this is going to be ok, but let's test it really, really well.

r+

::: mozmill/mozmill/extension/resource/modules/windows.js
@@ +178,5 @@
>      }
>  
>      // We need to add/remove the unload/pagehide event listeners to preserve caching.
> +    aWindow.addEventListener("beforeunload", beforeUnloadHandler, true);
> +    aWindow.addEventListener("pagehide", pageHideHandler, true);

I'm a little leery of these, but they are probably ok. We are setting these event listeners on fundamentally different objects with this change, and that could cause some different behavior. It might be that the window event is a superset of the browser event, since I believe we're talking about chrome windows here so in that case it might be OK.

We want to keep an eye on this change and verify that all our tests pass with this change.

@@ +214,4 @@
>        map.updatePageLoadStatus(id, false);
>      }
>  
> +    aWindow.emoveEventListener("beforeunload", beforeUnloadHandler, true);

this is a pretty important typo ^ aWindow.removeEventListner... (missing 'r').

Comment 15

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

(In reply to Clint Talbert ( :ctalbert ) from comment #14)
> >      // We need to add/remove the unload/pagehide event listeners to preserve caching.
> > +    aWindow.addEventListener("beforeunload", beforeUnloadHandler, true);
> > +    aWindow.addEventListener("pagehide", pageHideHandler, true);
> 
> I'm a little leery of these, but they are probably ok. We are setting these
> event listeners on fundamentally different objects with this change, and
> that could cause some different behavior. It might be that the window event
> is a superset of the browser event, since I believe we're talking about
> chrome windows here so in that case it might be OK.
> 
> We want to keep an eye on this change and verify that all our tests pass
> with this change.

This has been tested very well by Andreea, Andrei, and myself for mutt tests and our existing mozmill tests for Firefox. There was not a single failure. The thing is that the events are most likely are bubbling up to the root node which is the chrome window itself. And that makes sense. So with that we should be really safe and also support any kind of window now which has similar events and not only browser windows.

> > +    aWindow.emoveEventListener("beforeunload", beforeUnloadHandler, true);
> 
> this is a pretty important typo ^ aWindow.removeEventListner... (missing
> 'r').

Oh my, thanks for catching that! I will fix it.

I will wait with landing this patch until we got mozrunner 5.18 and mozprofile 0.11 released. Both are necessary to correctly run Metro tests.

Comment 16

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

Attachment: Tabs and window handling v2 (checked-in)

Updated patch for recently released mozrunner 5.18. Carrying over r+.

Comment 17

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

Comment on attachment 762522 [details] [diff] [review]
Tabs and window handling v2 (checked-in)

Landed on master:
https://github.com/mozilla/mozmill/commit/cf49285b37d793c01783a7a159a1fad88a662e9e

Comment 18

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

I will leave this bug open until the remaining touch events support patch has been landed. Everything else seems to work fine for now. If you discover something else which doesn't work for Metro please let me know.

Comment 19

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

Touch events are not a real blocker here and we have decided to land them for rc5. Everything else has been updated and should work now in Mozmill. If we detect still not working code, we will file follow-up bugs. Closing this bug in favor of getting Mozmill 2.0rc4 released.