Add support for Firefox Metro to Mozmill

RESOLVED FIXED

Status

Testing Graveyard
Mozmill
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: Andrei Eftimie, Assigned: whimboo)

Tracking

({meta})

Details

(Whiteboard: [ateamtrack: p=mozmill q=2013q2 m=3][mozmill-2.0+][metro])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
Tracking bug for Mozmill issues under metro.
Blocks: 845079
No longer blocks: 731280
Keywords: meta
Hardware: x86 → All
Summary: [meta] MozMill problems under metro → Add support for Firefox Metro to Mozmill
Whiteboard: [metro]
Depends on: 879386
Created attachment 758890 [details] [diff] [review]
Quit Firefox Metro v1 (checked-in)

With this patch Mozmill is able to close Firefox Metro.
Attachment #758890 - Flags: review?(ctalbert)

Comment 2

5 years ago
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. :-)
Attachment #758890 - Flags: review?(ctalbert) → review+
Comment on attachment 758890 [details] [diff] [review]
Quit Firefox Metro v1 (checked-in)

Landed on master:
https://github.com/mozilla/mozmill/commit/f5cf2c42663c21f1b8057abbd147373f3a77e957
Attachment #758890 - Attachment description: Quite Firefox Metro v1 → Quite Firefox Metro v1 (checked-in)
Attachment #758890 - Attachment description: Quite Firefox Metro v1 (checked-in) → Quit Firefox Metro v1 (checked-in)
Status: NEW → ASSIGNED
Created attachment 759787 [details] [diff] [review]
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.
Attachment #759787 - Flags: review?(ctalbert)
Attachment #759787 - Flags: feedback?(andrei.eftimie)
Attachment #759787 - Flags: feedback?(andreea.matei)
Created attachment 759804 [details]
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.
(Reporter)

Comment 6

5 years ago
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.",
(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.
It is working now with the latest change.
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.
Attachment #759787 - Flags: review?(ctalbert)
Attachment #759787 - Flags: feedback?(andrei.eftimie)
Attachment #759787 - Flags: feedback?(andreea.matei)
Created attachment 759935 [details] [diff] [review]
Tabs and window handling v1.1

Updated patch which now includes the test but which we cannot run by default as of now.
Attachment #759787 - Attachment is obsolete: true
Attachment #759804 - Attachment is obsolete: true
Attachment #759935 - Flags: feedback?(andrei.eftimie)
Attachment #759935 - Flags: feedback?(andreea.matei)
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.
Attachment #759935 - Flags: feedback?(andreea.matei) → feedback+
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.
Attachment #759935 - Flags: review?(ctalbert)
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.
Depends on: 880839

Comment 14

5 years ago
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').
Attachment #759935 - Flags: review?(ctalbert) → review+
(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.
(Reporter)

Updated

5 years ago
Depends on: 882055
No longer depends on: 882055
Created attachment 762522 [details] [diff] [review]
Tabs and window handling v2 (checked-in)

Updated patch for recently released mozrunner 5.18. Carrying over r+.
Attachment #759935 - Attachment is obsolete: true
Attachment #759935 - Flags: feedback?(andrei.eftimie)
Attachment #762522 - Flags: review+
Depends on: 880426
Comment on attachment 762522 [details] [diff] [review]
Tabs and window handling v2 (checked-in)

Landed on master:
https://github.com/mozilla/mozmill/commit/cf49285b37d793c01783a7a159a1fad88a662e9e
Attachment #762522 - Attachment description: Tabs and window handling v2 → Tabs and window handling v2 (checked-in)
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.
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
No longer depends on: 880426
Resolution: --- → FIXED
Whiteboard: [metro] → [ateamtrack: p=mozmill q=2013q2 m=3][mozmill-2.0+][metro]
OS: Windows 8 Metro → Windows 8.1
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.