671458 - lib/driver.js:getBrowserWindow() and openBrowserWindow() need generic and local versions

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

lib/driver.js currently has two functions, getBrowserWindow() and openBrowserWindow(). They return existing and new browser windows respectively.

Currently, getBrowserWindow(), by default, returns BrowserWindow, a local specified class for Firefox windows.

openBrowserWindow, in contrast, returns a default of ChromeWindowWrapper, a generic class for any Chrome window.

Ultimately, we want the A-Team to own these as part of Mozmill 2.0. For that to happen, both need to return ChromeWindowWrappers in their versions. But or our use, we'll want versions that return BrowserWindows by default. 

So, we need the original functions genericized, and wrappers around them that feed them the right default class for our use (they're factories, after Bug 668221 is checked in).

There are a couple of ways to do this:

1) We find somewhere else in our API to add getBrowserWindow and openBrowserWindow. I'd argue the most "correct" way would be as a class function on BrowserWindow, or at least helpers in the browser module. But then that has to be imported as well, which is kind of sucky.

2) We replace the driver functions in the global setup function:

driver.getBrowserWindowOrig = driver.getBrowserWindow
driver.getBrowserWindow = function (class, open) { 
  this.getBrowserWindowOrig(class || browser.BrowserWindow, open) 
}

I'm throwing it out as a possibility, but it's not one I'd probably choose. If any internal Mozmill code uses these functions, I think they'll get our classes by default if we do this.

3) In setup, we create new functions on driver:

driver.getFirefoxWindow = function (class, open) { 
  this.getBrowserWindow(class || browser.BrowserWindow, open) 
}

This the route I'd probably choose, and likely with that naming. 

We might even choose to rename the BrowserWindow class to FirefoxWindow instead (and browser.js to firefox.js, etc) which I like better for a number of reasons, not the least of which is that SeaMonkey doesn't get covered by our "browser" classes. 

We might even want DesktopFirefox naming to differentiate from the eventual maps for Mobile Firefox.

A bit of the tangent on naming, but not irrelevant; however we choose to name, the local versions of the functions should be named the same.

Comment 1

[u415141]

A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/15817987

Comment 2

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

As discussed on the Mozmill-Dev mailing list both functions in the driver module should return a basic DOMWindow. We would need our local wrappers which return the appropriate window wrapper.

http://groups.google.com/group/mozmill-dev/browse_thread/thread/66ed32ade47a3329

Comment 3

[u415141]

A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/16760143

Comment 4

[u415141]

Henrik Skupin changed story state to started in Pivotal Tracker

Comment 5

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

Attachment: Patch v1

Comment 6

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 551708 [details] [diff] [review]
Patch v1

r+, let's get it merged in and the driver module to Clint. I thought we were going to just remove driver.getBrowserWindow() and driver.openBrowserWindow() but I'm OK with this approach too.

I'm not 100% sure on having these functions be top-level of the windows module; at the very least, we'll probably want to restore the class-factory aspects so they can return subclasses. But let's worry about that later, it doesn't need to block checkin.

Comment 7

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

(In reply to Geo Mealer [:geo] from comment #6)
> r+, let's get it merged in and the driver module to Clint. I thought we were
> going to just remove driver.getBrowserWindow() and
> driver.openBrowserWindow() but I'm OK with this approach too.

No. As given by Clint those methods should remain even if they are application specific.

> I'm not 100% sure on having these functions be top-level of the windows
> module;

You can't put those methods into a window wrapper or one of its subclasses. What would you have in mind?

> at the very least, we'll probably want to restore the class-factory
> aspects so they can return subclasses. But let's worry about that later, it
> doesn't need to block checkin.

Why would we need that? I don't think that you will ever wanna have a basic WindowWrapper returned. The methods are specific for the browser window and should always return an instance of BrowserWindow. But hey, we could move those methods from windows.js into browser.js, it's a better location there.

Comment 8

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Doh, I'm a bad reviewer! I saw you moved them into windows, and assumed they were returning chrome wrappers. My comments were based on that.

Yes, let's move them into browser.js as top-level functions and keep them the way they are. No re-review necessary if you're just changing files and removing the extra requires. If you break something, I'll quickly fix it in the repo, but I think it'll be fine.

Re: Clint, no prob. His email reply was a little ambiguous as to which way he agreed, but I'm happy to do it either way.

Comment 9

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

Landed as two patches because I missed to remove the unnecessary module inclusion:

https://github.com/geoelectric/mozmill-api-refactor/commit/a1ee795f37e57d5da924262dfefe47ea987227d7
https://github.com/geoelectric/mozmill-api-refactor/commit/ff8465389691eabf30729e776d6fe824338bbd68

Comment 10

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

And one more for good luck: https://github.com/geoelectric/mozmill-api-refactor/commit/569504b6d6e32bfb013f11fb89725cb942ff47c5

Comment 11

[u415141]

Henrik Skupin changed story state to delivered in Pivotal Tracker

Comment 12

[u415141]

Henrik Skupin changed story state to accepted in Pivotal Tracker

Comment 13

[u415141]

Henrik Skupin changed story state to accepted in Pivotal Tracker