677482 - Pull in driver module into Mozmill 2

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

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

Once bug 671458 has been fixed, any dependency to our own window module will have been removed. Given that we can go forward in porting this module to Mozmill. Setting mozmill-2.0+ as given by reply from Clint.

Comment 1

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

Attachment: Patch v1

Patch with the driver module port and additional tests for the major functions in this new module.

Comment 2

[Jeff Hammel]

(In reply to Henrik Skupin (:whimboo) from comment #1)
> Created attachment 552059 [details] [diff] [review]
> Patch v1
> 
> Patch with the driver module port and additional tests for the major
> functions in this new module.

+var sleep = utils.sleep;

Why expose a convenience accessor for this?  You only use it a few
places in driver.js . 

+var waitFor = utils.waitFor;

Same here.

+//
+// INTERNAL WINDOW ENUMERATIONS
+//
+

Since there is only one method here, you probably don't want/need this
delimeter. Is there any reason that this method should be private?

+function _getWindows(aEnumerator, aFilterCallback, aFilterValue,
aStrict) {

I'm not sure if aFilterValue is a good way of setting this up.  It
forces the filter function to have the signature `filterMethod(window,
aFilterValue)`

Isn't it better to use closures here and just have the filter method
just take the window?  E.g.


function _getWindows(aEnumerator, aFilterCallback, aStrict) {

...

if (!aFilterCallback || aFilterCallback(window)) { ... }

...

function filterWindowByTitle(aTitle) {
  return function(aWindow) { return (aWindow.document.title === aTitle); }
  // XXX this doesn't have to be an inner function; just illustratin'
}
// use like _getWindows(aEnumerator, filterWindowByTitle('foo'), aStrict)

+    throw new errors.UnexpectedError(message);

Where is errors defined/injected?  Also, are we always asserting that
one *should* find windows with this method?  

+function getWindowsByAge(aFilterCallback, aFilterValue, aStrict) {
...
+function getWindowsByZOrder(aFilterCallback, aFilterValue, aStrict) {

likewise for the function signature

+var driver = exports;
...
+// Export of functions
+driver.getBrowserWindow = getBrowserWindow;
+driver.openBrowserWindow = openBrowserWindow;
...

So currently we use EXPORTED_SYMBOLS and Components.utils.import(...)
for this across mozmill.  We should probably stick to one convention;
maybe not with this bug, but if we commit this we should file another
bug to cleanup the current method and replace it with require.

+driver.sleep = sleep;
+driver.waitFor = waitFor;

Again, no need to export convenience methods, unless i am missing something

-    Cu: Components.utils,
Not sure why this is gone.

+++ b/mutt/mutt/tests/js/test_driver.js

Please add to jstests.ini.


Overall, this looks good, r=me.  I would greatly appreciate it if you
could make all of the filter functions just take the window and then
resolving the specific filter with closures rather than locking us
into the "filters always take the window and some random argument" as
an architecture, or we could discuss this in more detail.

Comment 3

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

(In reply to Jeff Hammel [:jhammel] from comment #2)
> (In reply to Henrik Skupin (:whimboo) from comment #1)
> > Created attachment 552059 [details] [diff] [review]
> > Patch v1
> > 
> > Patch with the driver module port and additional tests for the major
> > functions in this new module.
> 
> +var sleep = utils.sleep;
> 
> Why expose a convenience accessor for this?  You only use it a few
> places in driver.js . 
> 
> +var waitFor = utils.waitFor;
> 
> Same here.

Same reason you exposed them as controller.sleep() and controller.waitFor(). utils isn't supposed to be accessed directly by users, but these important functions are buried there for whatever reason.

Essentially, anything that was in controller and wasn't element-specific landed in driver. That was per original expectations that that's what driver primarily was.

That's also why we exported them, as you called out later. Tests use driver.waitFor() and driver.sleep().

> 
> +//
> +// INTERNAL WINDOW ENUMERATIONS
> +//
> +
> 
> Since there is only one method here, you probably don't want/need this
> delimeter. Is there any reason that this method should be private?
> 
> +function _getWindows(aEnumerator, aFilterCallback, aFilterValue,
> aStrict) {

It's a little rough to expose to tests, so it's private. It was designed as a helper function within that module only.

Re: delimiter, we had two functions there when I put that in, but my coding style runs towards dividing files into sections that way anyway for comprehension. I tend to do it before I even write the functions, so it's not sensitive to the count; it establishes where I'll put them later.

We could remove the delimiter (and any others) if you want to fit your project's coding style.

> 
> I'm not sure if aFilterValue is a good way of setting this up.  It
> forces the filter function to have the signature `filterMethod(window,
> aFilterValue)`
> 
> Isn't it better to use closures here and just have the filter method
> just take the window?  E.g.
> 
> 
> function _getWindows(aEnumerator, aFilterCallback, aStrict) {
> 
> ...
> 
> if (!aFilterCallback || aFilterCallback(window)) { ... }
> 
> ...
> 
> function filterWindowByTitle(aTitle) {
>   return function(aWindow) { return (aWindow.document.title === aTitle); }
>   // XXX this doesn't have to be an inner function; just illustratin'
> }
> // use like _getWindows(aEnumerator, filterWindowByTitle('foo'), aStrict)
> 

To be fair, this also forces the closure to comply with certain things. But I do like it in that it keeps the two values from getting echoed across a lot of signatures. 

What we'll lose is the detailed error message. There won't be a simple way for this function to know what the closure is checking on. Anything I can think of is more complicated than what we're doing now.

I'd still be OK with that tradeoff, and I'd be willing to rearchitect it this way if Henrik agrees.

> +    throw new errors.UnexpectedError(message);
> 
> Where is errors defined/injected?  Also, are we always asserting that
> one *should* find windows with this method?  

Good call on errors. Guessing we didn't find that in testing because we never hit that code path.

Assuming you're still in _getWindows(), errors are thrown if aStrict === true. That's noted by the header comment, and the code itself is inside an if() that defines that behavior.

> +// Export of functions
> +driver.getBrowserWindow = getBrowserWindow;
> +driver.openBrowserWindow = openBrowserWindow;
> ...
> 
> So currently we use EXPORTED_SYMBOLS and Components.utils.import(...)
> for this across mozmill.  We should probably stick to one convention;
> maybe not with this bug, but if we commit this we should file another
> bug to cleanup the current method and replace it with require.

Sure, makes sense.

> -    Cu: Components.utils,
> Not sure why this is gone.

+    Cu: Components.utils,

It's added two lines later. I think Henrik alphabetized them, so moved it under Cr.

> Overall, this looks good, r=me.  I would greatly appreciate it if you
> could make all of the filter functions just take the window and then
> resolving the specific filter with closures rather than locking us
> into the "filters always take the window and some random argument" as
> an architecture, or we could discuss this in more detail.

Let me get Henrik's feedback on it. It's not rare to have filters match a particular spec that comes down to boolean Filter(actual, expected) (super-common in any language w/o closures, in fact), but I take your points and largely agree.

Comment 4

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

(In reply to Geo Mealer [:geo] from comment #3)
> Essentially, anything that was in controller and wasn't element-specific
> landed in driver. That was per original expectations that that's what driver
> primarily was.

And I didn't wanted to duplicate the code from utils. That's why I have simply exposed those functions again on the driver. Once controller is abandoned we can move over the code completely. 

> > if (!aFilterCallback || aFilterCallback(window)) { ... }
> > 
> > ...
> > 
> > function filterWindowByTitle(aTitle) {
> >   return function(aWindow) { return (aWindow.document.title === aTitle); }
> >   // XXX this doesn't have to be an inner function; just illustratin'
> > }
> > // use like _getWindows(aEnumerator, filterWindowByTitle('foo'), aStrict)

So how does the closure in filterWindowByTitle knows about aWindow? This is a public method and can be used anywhere, also by tests themselves. So we can't define the filter function inside _getWindows().

At least that's how I have had it before the driver has been refactored by bug 668221:

https://github.com/geoelectric/mozmill-api-refactor/blob/cac8ff9afa33970fa735efe9fc1dc1ed9d1bf58a/lib/api/driver.js#L149

> > +    throw new errors.UnexpectedError(message);
> > 
> > Where is errors defined/injected?  Also, are we always asserting that
> > one *should* find windows with this method?  
> 
> Good call on errors. Guessing we didn't find that in testing because we
> never hit that code path.

Correct. Question is also if you want to have such an error class in Mozmill or to which class we should fallback.

I will wait with an update of the patch until the remaining questions have been solved.

Comment 5

[Jeff Hammel]

(In reply to Henrik Skupin (:whimboo) from comment #4)
> (In reply to Geo Mealer [:geo] from comment #3)
> > Essentially, anything that was in controller and wasn't element-specific
> > landed in driver. That was per original expectations that that's what driver
> > primarily was.
> 
> And I didn't wanted to duplicate the code from utils. That's why I have
> simply exposed those functions again on the driver. Once controller is
> abandoned we can move over the code completely. 

Fair enough; sounds like a plan.

> > > if (!aFilterCallback || aFilterCallback(window)) { ... }
> > > 
> > > ...
> > > 
> > > function filterWindowByTitle(aTitle) {
> > >   return function(aWindow) { return (aWindow.document.title === aTitle); }
> > >   // XXX this doesn't have to be an inner function; just illustratin'
> > > }
> > > // use like _getWindows(aEnumerator, filterWindowByTitle('foo'), aStrict)
> 
> So how does the closure in filterWindowByTitle knows about aWindow? This is
> a public method and can be used anywhere, also by tests themselves. So we
> can't define the filter function inside _getWindows().

The filter function is not in _getWindows.  The basic architecture is that you have a function, e.g. filterWindowByTitle, that takes whatever arguments, e.g. 'foo', that returns another function in its scope that takes a single argument, aWindow, that returns true or false.  So similar to case you linked to, just the filters have to change, and the function signatures no longer have to take an the filter argument.

> > > +    throw new errors.UnexpectedError(message);
> > > 
> > > Where is errors defined/injected?  Also, are we always asserting that
> > > one *should* find windows with this method?  
> > 
> > Good call on errors. Guessing we didn't find that in testing because we
> > never hit that code path.
> 
> Correct. Question is also if you want to have such an error class in Mozmill
> or to which class we should fallback.

I don't know, really.  At this point I could go either way.  The mozmill js side needs significant cleanup, so its hard for me to have a strong preference at this point (though I'd probably have something more specific, e.g. WindowsNotFoundError).  So whatever makes your lives easier.

Comment 6

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

Henrik:

function createNameFilterCallback(name) {
  return function (window) { return window.name === name); }
}

// later...

list = getWindows_(someEnumerator, createNameFilterCallback("my target name"));

I have a good grasp on how to do this. We can share the patch if need be.

I don't actually find this to be simpler, mind you--the way we set it up is much closer to how something would handle it in C-like languages--but someone raised on closures might. 

When in Rome...

Comment 7

[Jeff Hammel]

Indeed; fwiw i wouldn't do it with "closures" in python either (because python closures are awkward) and would pass the *args, **kwargs around; but unlike JS, you can do function(anArgument, *args, **kwargs)

Comment 8

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

JS has some analogous concepts; you can pass the args as an array. It's just that dealing with multiple arbitrary arguments would not have been an improvement here.

Closures will work, gives us elegant signature, but they're a little more opaque as to what's going on. However, that's because they could do most anything, not just a simple compare.

Classic actual, expected filters gives us clumsy but comprehensible signatures and is IMO a little bit clearer as to what's going on and less complicated to write. However, they lock you into actual vs. expected compares.

I'm 50/50. If it were my code, I'd go classic because I find it simpler to explain and document. Because it's your code, I'm willing to go for what you want with Henrik's approval.

Comment 9

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

So lets see how the calling convention will look like for test creators.

Now:
----
browser.handleWindow(driver.filterWindowByType, "navigator:view-source", callback);

After:
------
browser.handleWindow(driver.filterWindowByType("navigator:view-source"), callback);

I have to admit that I like it because it makes the code cleaner. There is no obvious big change for test creators. Also I wouldn't mind that much for the creation of custom filter types. The only thing I'm concerned about and second Geo is the documentation side. Not sure how I should as best document that returned closure correctly yet.

Jeff, please tell me your opinion so that I can start updating the patch, what I will have to do in any way because of the non-existent error class. Thanks.

Comment 10

[Jeff Hammel]

Yeah, I like :)

Comment 11

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

I wonder about the naming of those closure-generating functions. filterWindowByType() isn't wholly accurate anymore; it's more of a createWindowByTypeFilter().

OTOH, that might be confusing. Not sure what's more confusing: being accurate to the function purpose but exposing the closure-based machanism outwards, or being inaccurate to the function purpose but abstracting the closure part completely.

Maybe if that param was aFilter instead of aCallback, going with createFooFilter() would make more sense? That makes things fully semantic.

Comment 12

[Jeff Hammel]

could be e.g. windowByTypeFilter(), since it does return a filter, seems semantic </suggestion:random>

Comment 13

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

Sure, that works. I'm in the camp that tends to name functions as imperative verb phrases always, but I'm also just fine with naming it after a noun phrase result.

Comment 14

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

I got some issues while writing this patch with one of our tests in the API refactoring repository. Looks like a focus issue, but I have to figure that out first before I can upload a new version of the patch.

Comment 15

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

Attachment: Patch v2

Updated patch according to discussion on this thread. Also I have fixed the broken test_assertions.js test. I haven't included the errors module yet, because that's a pain when working with the old module system and the new securable module one. We really have to transition over to a common system first.

Comment 16

[Jeff Hammel]

Comment on attachment 554030 [details] [diff] [review]
Patch v2

I haven't tested, but looks good to me

Comment 17

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

Landed as:
https://github.com/mozautomation/mozmill/commit/91fc55848b2df4fce4be473513b0b04742b3002d

Comment 18

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

We have to reopen that bug because of a problem I have seen today. We kinda missed to test this patch on Linux. We fail in openBrowserWindow when trying to call hWindow.OpenBrowserWindow() with an error that this method is undefined. I'm already working on a solution in our API refactoring repository. Sorry for that.

Comment 19

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

Attachment: Fix for broken getTopmostWindow

Adapted patch from our API refactoring repository.

Comment 20

[Jeff Hammel]

Comment on attachment 563110 [details] [diff] [review]
Fix for broken getTopmostWindow

wfm

Comment 21

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

Landed as:
https://github.com/mozautomation/mozmill/commit/93366763dba9e2ba18c468c56d9fc7c7daae7818

Comment 22

[Jeff Hammel]

So this is currently broken, wrt https://github.com/mozautomation/mozmill/blob/master/mozmill/mozmill/extension/resource/modules/driver.js#L296 `services` being undefined.  See bug 698790 for an instance of failure

Comment 23

[Jeff Hammel]

looks like this can be fixed using https://github.com/mozautomation/mozmill/blob/master/mozmill/mozmill/extension/resource/modules/driver.js#L47

Comment 24

[Jeff Hammel]

pushed to master: https://github.com/mozautomation/mozmill/commit/c380f62a307bda136954cb2d09c0f10fb8921b14

Comment 25

[Jeff Hammel]

Attachment: fix for broken services not defined

Comment 26

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

Comment on attachment 571129 [details] [diff] [review]
fix for broken services not defined

Silly mistake, or I have used an old patch for the check-in. I should really only work on branches from now on.

Comment 27

[Jeff Hammel]

pushed to hotfix-2.0: https://github.com/mozautomation/mozmill/commit/7b9971ea42b58b5231295087f5a0288e8bc2fb4c