Closed Bug 879371 Opened 11 years ago Closed 11 years ago

Add support for Firefox Metro to Mozmill

Categories

(Testing Graveyard :: Mozmill, defect)

All
Windows 8.1
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: andrei, Assigned: whimboo)

References

Details

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

Attachments

(2 files, 3 obsolete files)

Tracking bug for Mozmill issues under metro.
Blocks: 845079
No longer blocks: metro-testing
Keywords: meta
Hardware: x86 → All
Summary: [meta] MozMill problems under metro → Add support for Firefox Metro to Mozmill
Whiteboard: [metro]
Depends on: 879386
With this patch Mozmill is able to close Firefox Metro.
Attachment #758890 - Flags: review?(ctalbert)
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+
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
Attached patch Tabs and window handling v1 (obsolete) — Splinter Review
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)
Just a test for Firefox Metro to check if tabs are working and pages are loaded correctly.
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)
Attached patch Tabs and window handling v1.1 (obsolete) — Splinter Review
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 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.
Depends on: 882055
No longer depends on: 882055
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
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
Closed: 11 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.

Attachment

General

Created:
Updated:
Size: