Closed
Bug 879371
Opened 11 years ago
Closed 11 years ago
Add support for Firefox Metro to Mozmill
Categories
(Testing Graveyard :: Mozmill, defect)
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)
1.59 KB,
patch
|
cmtalbert
:
review+
|
Details | Diff | Splinter Review |
14.61 KB,
patch
|
whimboo
:
review+
|
Details | Diff | Splinter Review |
Tracking bug for Mozmill issues under metro.
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Updated•11 years ago
|
Keywords: meta
Hardware: x86 → All
Summary: [meta] MozMill problems under metro → Add support for Firefox Metro to Mozmill
Whiteboard: [metro]
Assignee | ||
Comment 1•11 years ago
|
||
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+
Assignee | ||
Comment 3•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #758890 -
Attachment description: Quite Firefox Metro v1 (checked-in) → Quit Firefox Metro v1 (checked-in)
Assignee | ||
Updated•11 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•11 years ago
|
||
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)
Assignee | ||
Comment 5•11 years ago
|
||
Just a test for Firefox Metro to check if tabs are working and pages are loaded correctly.
Reporter | ||
Comment 6•11 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.",
Assignee | ||
Comment 7•11 years ago
|
||
(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•11 years ago
|
||
It is working now with the latest change.
Assignee | ||
Comment 9•11 years ago
|
||
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)
Assignee | ||
Comment 10•11 years ago
|
||
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 11•11 years ago
|
||
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+
Assignee | ||
Comment 12•11 years ago
|
||
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)
Assignee | ||
Comment 13•11 years ago
|
||
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•11 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+
Assignee | ||
Comment 15•11 years ago
|
||
(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.
Assignee | ||
Comment 16•11 years ago
|
||
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+
Assignee | ||
Comment 17•11 years ago
|
||
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)
Assignee | ||
Comment 18•11 years ago
|
||
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.
Assignee | ||
Comment 19•11 years ago
|
||
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.
Assignee | ||
Updated•11 years ago
|
Whiteboard: [metro] → [ateamtrack: p=mozmill q=2013q2 m=3][mozmill-2.0+][metro]
Updated•10 years ago
|
OS: Windows 8 Metro → Windows 8.1
Updated•8 years ago
|
Product: Testing → Testing Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•