Pull in driver module into Mozmill 2

RESOLVED FIXED

Status

Testing Graveyard
Mozmill
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: whimboo, Assigned: whimboo)

Tracking

Details

(Whiteboard: [mozmill-2.0+])

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
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.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Attachment #552059 - Flags: review?(jhammel)

Comment 2

6 years ago
(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.

Updated

6 years ago
Attachment #552059 - Flags: review?(jhammel) → review+
(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.
(Assignee)

Comment 4

6 years ago
(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

6 years ago
(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.
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

6 years ago
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)
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.
(Assignee)

Comment 9

6 years ago
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

6 years ago
Yeah, I like :)
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

6 years ago
could be e.g. windowByTypeFilter(), since it does return a filter, seems semantic </suggestion:random>
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.
(Assignee)

Comment 14

6 years ago
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.
(Assignee)

Updated

6 years ago
Depends on: 679404
(Assignee)

Comment 15

6 years ago
Created attachment 554030 [details] [diff] [review]
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.
Attachment #552059 - Attachment is obsolete: true
Attachment #554030 - Flags: review?(jhammel)

Comment 16

6 years ago
Comment on attachment 554030 [details] [diff] [review]
Patch v2

I haven't tested, but looks good to me
Attachment #554030 - Flags: review?(jhammel) → review+
(Assignee)

Comment 17

6 years ago
Landed as:
https://github.com/mozautomation/mozmill/commit/91fc55848b2df4fce4be473513b0b04742b3002d
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 18

6 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

6 years ago
Depends on: 689513
(Assignee)

Comment 19

6 years ago
Created attachment 563110 [details] [diff] [review]
Fix for broken getTopmostWindow

Adapted patch from our API refactoring repository.
Attachment #563110 - Flags: review?(jhammel)

Comment 20

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

wfm
Attachment #563110 - Flags: review?(jhammel) → review+
(Assignee)

Comment 21

6 years ago
Landed as:
https://github.com/mozautomation/mozmill/commit/93366763dba9e2ba18c468c56d9fc7c7daae7818
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED

Comment 22

6 years ago
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
Blocks: 698790
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 23

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

Comment 24

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

Comment 25

6 years ago
Created attachment 571129 [details] [diff] [review]
fix for broken services not defined
Attachment #571129 - Flags: review?(hskupin)
(Assignee)

Comment 26

6 years ago
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.
Attachment #571129 - Flags: review?(hskupin) → review+

Comment 27

6 years ago
pushed to hotfix-2.0: https://github.com/mozautomation/mozmill/commit/7b9971ea42b58b5231295087f5a0288e8bc2fb4c
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.