Closed Bug 632451 Opened 10 years ago Closed 9 years ago

Refactor controller API so it is more intuitive

Categories

(Testing Graveyard :: Mozmill, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ahal, Assigned: ahal)

References

Details

(Whiteboard: [mozmill-2.0+])

Attachments

(2 files, 7 obsolete files)

The controller API is currently really difficult to use.  All methods are thrown into this object which makes it somewhat confusing.  Some of the naming conventions are unintuitive as well.

Currently the API is used like: 
controller.click(element);

We want to move to a:
element.click();

model instead.  The controller should still be used for browser specific methods and assertions, but any method that interacts with an element can be a property of that object.
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
Whiteboard: [mozmill-2.0?]
It may be worth looking at the WebDriver API. I understand the existing Mozmill API was partly based on Selenium 1, and the WebDriver API is part of Selenium 2.
Also our current work for the shared module refactoring should be a source:
https://wiki.mozilla.org/QA/Mozmill_Test_Automation/Shared_Modules_Refactor
(In reply to comment #1)
> It may be worth looking at the WebDriver API. I understand the existing Mozmill
> API was partly based on Selenium 1, and the WebDriver API is part of Selenium
> 2.

Yep. This is actually where I was planning to look. We want to make the two API's as similar as possible.  I'll also take a look at the Share Modules stuff and try to get the best of both worlds.
Attached patch WIP Patch - Refactor API (obsolete) — Splinter Review
This is what I have so far. Elements are no longer wrapped by the elementslib object (in fact there is no longer an elementslib object) but are instead wrapped by a 'mozelement' object (not sure if that's the best name for it).

Anyway, you can do stuff like:
> var el = elementslib.ID(document, "foo");
> el.click();

Note that this doesn't change anything in controller.js so it is so far completely backwards compatible (as far as I have tested which is admittedly not much).

This is also a very massive patch, so there's a few things I want to discuss with the team before checking it in.  I'm not sure if using an element hierarchy is worth it or not.

Finally, the only thing left to do (assuming that what I have done so far is ok) is to decide on the naming of the methods (i.e do some webdriver research).  Oh and also test the lookup expressions to make sure that still works.
Attachment #512378 - Flags: feedback?
Whiteboard: [mozmill-2.0?] → [mozmill-2.0+]
Blocks: 567148
So one of the things that has always bugged me when getting elements was the requirement to type the document each time.

It seems that 99% of the time we use either 'controller.window.document' or 'controller.tabs.activeTab' (which is actually just controller.window.gBrowser.selectedBrowser.contentDocument).

I would like to give users the option to leave the document parameter empty, and the elementslib API will automatically search against those two common use cases. We would first search against the current tab, and then broaden that to the window document if nothing was found. You could still pass in a specific document if you needed to for finer control.

I would even consider searching through all open windows using nsIWindowMediator (in a most recent to least recent order). The only problem with this is that it might introduce 'weird' errors for novice test writers as they might be selecting elements in different windows by accident (although we currently have this problem with tabs anyway). I'm not entirely sold on this idea myself, but I think it would be nice for the user to have some better form of managing their windows.
I'm fairly certain that using an element hierarchy is worth it, but do consider we're working on implementing the same over here (see comment 2), so you may not want to duplicate the effort.

If you end up willing to use our code--a definite possibility, per conversations with Clint, then our minimum requirements for what we're working on would be:

a single element class that basically can do:

element.Elem(node)
element.callMethod("someMethod", arbitrary, list, of, parameters, expected, by, node)

We're currently handling the rest of the document handling and stuff in our classes. Please see https://github.com/geoelectric/mozmill-api-refactor/blob/master/modules/ui/widgets.js for more details.

Unless you do a hierarchy, I really wouldn't suggest trying to do a lot of specific methods past click and type. Putting them all in the same element class (i.e. trying to give it both list/composite control and standard control semantics) will be confusing.

Maybe we should talk about this in person/on conf-call to get some quick conclusions, then dump them back into the bug.
Oh, and re: callMethod, as long as I can get myElem.node so I can call these things myself, I don't even need that. Frankly, I wasn't planning on using any of your event dispatch methods (click, type, etc.) directly unless they just happened to be there and convenient.
Attached patch WIP Patch2 - Add lazy loading (obsolete) — Splinter Review
So as you can see, this patch is getting pretty huge. I'd really like to land it so I can do more incremental development on it. There's a few bugs open requiring work on the controller that I don't want to start until this has landed.

I can break this up into a handful of smaller patches (maybe one per file) if that makes it easier to review.

There's also a few things left to discuss, like nomenclature, do we want to remove the controller API, how to implement the static map and more.  Most of this can wait for incremental development though.
Attachment #512378 - Attachment is obsolete: true
Attachment #518446 - Flags: feedback?
Attachment #512378 - Flags: feedback?
Attached patch Patch 1.0 - elementslib refactor (obsolete) — Splinter Review
I decided to put up a patch per file to make this easier to review.

The main things to take away from this patch are:
1. There is no longer such thing as an elementslib object. We still have elementslib.Name etc.. but they are just simple functions.

2. Each locator function in elementslib simply returns the actual element, no wrappers. Wrapping the element is now taken care of in mozelement.js.

3. Document is now an optional parameter. If no document is supplied, elementslib will first search in the current tab, and then the current window if nothing was found.

4. The code to actually find the elements hasn't changed at all. The order of the parameters (elementslib.ID(doc, locator_string);) also hasn't changed to preserve backwards compatibility.. though ideally they would be the other way around.
Attachment #518446 - Attachment is obsolete: true
Attachment #518446 - Flags: feedback?
Attached patch Patch 1.0 - mozelement.js (obsolete) — Splinter Review
This file contains the element wrappers around the DOM elements. In many respects it replaces the elementslib object. The user no longer calls into elementslib, but instead calls into this file which in turn creates the wrapper.

Things to take away:
1. MozMillElement is the base class. Other classes (e.g MozMillCheckBox) inherit from this class.

2. Most controller methods are now inside MozMillElement or one of subclasses. controller.js hasn't been touched, so that still works the same.

3. There are two ways to instantiate a MozMillElement:
   a) the normal way (use getElementBy.ID etc). In this case the createInstance method will automatically determine the type of the element and return the correct subclass.
   b) Explicitly instantiating (e.g var radio = new MozMillRadio(...)). In this case the wrapped DOM element is lazy loaded. This will be needed for getting elements via a static map.
Attached patch Patch 1.0 - frame.js (obsolete) — Splinter Review
The only changes to frame.js are what is injected into the global namespace. The only thing of note here is that we load mozelement.js into the 'elementslib' namespace to maintain backwards compatibility (i.e so var foo = new elementslib.ID() still works).

Also, in the final version of this we probably won't inject the actual MozMillElement classes into the global namespace itself, rather we'll use the static map.
Finally the test case I've been using.

Note that you can get elements using:
A) getElementBy.ID(doc, id);
B) new elementslib.ID(doc, id);
C) new MozMillElement(locatorType, locator, {'document':doc});
A and B are literally exactly the same (B is just for compatibility).
C will be needed by the static map.
In all cases, the document is optional.
Attachment #518559 - Flags: feedback?
Attachment #518558 - Flags: feedback?
Attachment #518551 - Flags: feedback?
(In reply to comment #9)
> 3. Document is now an optional parameter. If no document is supplied,
> elementslib will first search in the current tab, and then the current window
> if nothing was found.

This can cause problems when you operate on live websites and which would require us to always use the document again. What about if you want to get an element from chrome but already have found it inside the active tab? How does it handle tabs in the background? I assume you only check the active tab?

(In reply to comment #10)
> 1. MozMillElement is the base class. Other classes (e.g MozMillCheckBox)
> inherit from this class.

Haven't we decided on Widget in the last meeting? Not sure if I'm uptodate with that information.

Also should we really do the magic in createInstance or let the user explicitly select which type of element is wanted? I like the idea but this method could end-up in a huge switch case.

Also we should add custom errors for cases elements cannot be found.

For the inheritance stuff we should have a single method which handles all the prototype foo actions.

In general it looks great. Looking forward to even more refactoring.
(In reply to comment #13)
> This can cause problems when you operate on live websites and which would
> require us to always use the document again. What about if you want to get an
> element from chrome but already have found it inside the active tab? How does
> it handle tabs in the background? I assume you only check the active tab?

Yeah, this probably needs some further discussion. Currently it will first check the active tab, and then check the entire window which encompasses background tabs (we have this problem now when using controller.window.document). I'm not sure if this is a good idea or not, maybe it should only check the active tab by default.

At any rate, you still have the ability to pass in a document the same as before, this is more of a convenience thing than anything.

> Haven't we decided on Widget in the last meeting? Not sure if I'm uptodate with
> that information.

AFAIK Geo was just calling his refactor Widgets to differentiate between my refactor. MozMillElement is just the first name I came up with though, it doesn't have to be anything permanent.
 
> Also should we really do the magic in createInstance or let the user explicitly
> select which type of element is wanted? I like the idea but this method could
> end-up in a huge switch case.

Yeah, it could become a huge switch case, but we *need* this. The user might not know the type of an element they want to get. Instantiating explicitly also requires a lot of arcane knowledge on the user's part. They'd have to know exactly which elements we've sub-classed, which ones we haven't, what the sub-classes are called etc.. It would make the API even less intuitive than it is already which is the opposite of what this bug is trying to accomplish.

> Also we should add custom errors for cases elements cannot be found.

Yeah, I can do this. I want to get this checked in before I do too much more work on it though.

> For the inheritance stuff we should have a single method which handles all the
> prototype foo actions.

I'm not sure what you mean by this, or how I would do it.

> In general it looks great. Looking forward to even more refactoring.

Thanks for your feedback!
(In reply to comment #12)
> Created attachment 518561 [details] [diff] [review]
> Patch 1.0 - test_controllerRefactor.js
> 
> Finally the test case I've been using.
> 
> Note that you can get elements using:
> A) getElementBy.ID(doc, id);
> B) new elementslib.ID(doc, id);
> C) new MozMillElement(locatorType, locator, {'document':doc});
> A and B are literally exactly the same (B is just for compatibility).
> C will be needed by the static map.
> In all cases, the document is optional.

Note that C, if it's going to work with the map, needs to take both doc and another Element (either/or). Alternatively, instead of Element it could take a raw DOM node, but that'll be messy.

The map is a tree; whatever the root node of the tree is takes the document as its "owner", but subnodes take their parent node as an owner.

This aligns with how nodeCollector finds nodes (give me the node(s) matching this css selector under this document or node), and is necessary if you don't want to have to supply a full path to the node starting from the document. The idea is to be able to supply partial information that also takes the parent's info into account.

Note that I didn't -call- this parent in my classes because it can skip generations compared to the DOM. IOW, the DOM may be:

browser
|
hbox "foo"
|
vbox "bar"
|
panel "mypanel"
|
hbox "baz"
|
button "mybutton"

...but the map would probably only have:

browser
|
panel "mypanel"
|
button "mybutton"

...so they aren't true parents in the DOM sense--hence my staying away from that terminology in my code, though I could see valid debate on that.
(In reply to comment #15)
> Note that C, if it's going to work with the map, needs to take both doc and
> another Element (either/or). Alternatively, instead of Element it could take a
> raw DOM node, but that'll be messy.
> 
> The map is a tree; whatever the root node of the tree is takes the document as
> its "owner", but subnodes take their parent node as an owner.

Yeah, I saw this logic in your refactor, but does this need to be in the base element class?  This is only needed by things that will be in the static map, so would it make sense to pass in the 'owner' parameter to a 'static map' sub-class of the base Element?

e.g
BaseElem(locatorType, locator)
..StaticElem(locatorType, locator, owner, ...)
....UrlBar(...)
....etc..
(In reply to comment #14)
> > Haven't we decided on Widget in the last meeting? Not sure if I'm uptodate with
> > that information.
> 
> AFAIK Geo was just calling his refactor Widgets to differentiate between my
> refactor. MozMillElement is just the first name I came up with though, it
> doesn't have to be anything permanent.

Ok. So for a smooth transition we should simply come up with the same names later. 

> > Also should we really do the magic in createInstance or let the user explicitly
> > select which type of element is wanted? I like the idea but this method could
> > end-up in a huge switch case.
> 
> Yeah, it could become a huge switch case, but we *need* this. The user might
> not know the type of an element they want to get. Instantiating explicitly also
> requires a lot of arcane knowledge on the user's part. They'd have to know
> exactly which elements we've sub-classed, which ones we haven't, what the
> sub-classes are called etc.. It would make the API even less intuitive than it
> is already which is the opposite of what this bug is trying to accomplish.

So one question, how do you plan to handle XBL bindings (anonymous nodes)? I would assume the user has to create a separate map for it.

> > For the inheritance stuff we should have a single method which handles all the
> > prototype foo actions.
> 
> I'm not sure what you mean by this, or how I would do it.

You have a lot of line like that:

MozMillCheckBox.prototype = new MozMillElement();
MozMillCheckBox.prototype.parent = MozMillElement.prototype;
MozMillCheckBox.prototype.constructor = MozMillCheckBox;

It can be packed into an inheritance helper method:

function inherit(SubClass, BaseClass) {
  SubClass.prototype = new BaseClass();
  SubClass.prototype.parent = BaseClass.prototype;
  SubClass.prototype.constructor = SubClass;
}


So you simply have to call:

inherit(MozMillCheckBox, MozMillElement);
inherit(MozMillTextBox, MozMillElement);
[...]

Not sure if it can even be smarter.
(In reply to comment #15)
> Note that C, if it's going to work with the map, needs to take both doc and
> another Element (either/or). Alternatively, instead of Element it could take a
> raw DOM node, but that'll be messy.

You even don't need the document at all. If a DOM node is used, it always knows about its document. So instead of using explicitly a document opening it up for any node will make it a lot more flexible.
It'll make the instantiations really ugly, though. You'll have to use myElement.node all over the place. It was far more elegant to pass in the Element and have it quietly supply the node.

Also, the top element (usually the browser window, but could be another dialog, etc, since they have their own document) still has to be located. 

I suppose you could put in a requirement that it's always instantiated without the nodeCollector. Instead, I chose to keep that option, but in that case the nodeCollector either needs a node above it or the document. The document was the easiest thing to pass in.

At any rate, I'm sure many alternative architectures will work, but I was pretty happy with how clean mine came out for doing the map, and it seems flexible. I'd want to weigh it against others before we made drastic changes.
Oh, I remember the other problem that made me specifically not use node. If you're going to pass in the node, it has to be present and located at instantiation time. If you pass in the element, the node can be found lazily, and doesn't have to be present until something that needs it is accessed.

That's what let me change from your POC, which had to explicitly lazy-instantiate the Elements themselves, to my architecture, which instantiated the Elements normally but lazy-requested the nodes. 

The original POC was--no offense meant--really, really code-heavy to create a map in, so I'd be very resistant to going back to that architecture.
So from the mozmill-core perspective, AFAIK we don't want to be including any static maps (someone correct me if I'm wrong). Mozmill is supposed to be a generic tool to test any XUL applications, not just Firefox.  
  
That being said, we want to make it easy for you guys to extend the element class to make a static map.  Geo, would the proposal in comment 16 be suitable?  You could implement a "StaticElem" subclass that can take in any additional parameters you want, and that way you can setup the owner/document however you want. Actually the third parameter in the MozMillElement constructor is an object exactly for this reason.
I think I'm proposing that you consider communicating using static maps a best practice thing for any XUL application, and support it from mozmill-core. It is considered best-practice in most similar frameworks I know of. I thought Clint and I had discussed that before, so perhaps we should discuss it again.

There's absolutely nothing specific to Firefox about doing a static map, only about creating a browser object in particular to put at the top of it. We're happy to build and maintain the browser object from our side; it's simply a customized Region, per our class hierarchy. I would expect Thunderbird to have a different customized top-level object, as I would Komodo or any other XUL application.

But yes, if you continue to be skeptical of that approach, inheritance might work. Re: 16, though, I'm not sure how you'd get along not taking at -least- a document (or something that gives you that), as Elem currently does.

My only concern with using inheritance is not having enough "public" methods to override to make lazy-finding the nodes work, which is critical. Your other constructor methods assume the node already exists, which'll causes problems as explained in Comment 20. I don't want to have to gut your private implementation to do this, and deal with the maintenance issues that'd arrive from that.

But as I stated wayyy back up in Comment 6, if I have some basics I can wrap what you're doing just like I'm wrapping elementlib now, and then you don't have to worry about any of this. The only change would be the Widget classes talking straight to the Elem instead of handing it to a controller object. 

So if you don't support this, I'll likely continue to wrap you and write to our classes rather than yours. That may defeat your purpose in doing all this work though, so we should consider that option carefully.

However we go, I would want to work closely with you to make sure our path isn't blocked. Since some of this stuff might be hashed out more quickly and clearly in an in-person conversation, please don't hesitate to hit me up either ad-hoc or scheduled.
I was just under the impression that we wouldn't be implementing the static map, but I could very well be wrong. If it makes sense for us to do it, then I am not opposed in the least.

Re the other constructors needing the element to be present, this is not true, that element parameter is optional.  The third parameter is an object which can have either a "element" attribute, a "document" attribute, both or neither.  Lazy loading already works as is in the above patch.

I agree that we should talk though, my schedule is generally pretty empty :p
(In reply to comment #23)

> Re the other constructors needing the element to be present, this is not true,
> that element parameter is optional.  The third parameter is an object which can
> have either a "element" attribute, a "document" attribute, both or neither. 
> Lazy loading already works as is in the above patch.

OK, then I missed something. Let me reread your patch, then we'll figure out time to talk.

Geo
(In reply to comment #5)

> I would even consider searching through all open windows using
> nsIWindowMediator (in a most recent to least recent order). The only problem
> with this is that it might introduce 'weird' errors for novice test writers as
> they might be selecting elements in different windows by accident (although we
> currently have this problem with tabs anyway). I'm not entirely sold on this
> idea myself, but I think it would be nice for the user to have some better form
> of managing their windows.

Yeah, I wouldn't go that far.  That's a bit too automagic and implicit.  Seems to me that current tab then document is pretty sensible.
Not surprisingly, the patch against frame.js has bitrotted.  We should probably break our files up more, unless there are semantic or performance reasons not to, to avoid bitrot
Maybe this would be a good patch to inject controller into the module in frame.loadFile.  Is there any reason not to do this?  It avoids having to have a setupModule for the baseline case where it is `module.controller = mozmill.getBrowserController();`
for attachment 518561 [details] [diff] [review]:

+var testFoo = function(){

please come up with a better name.  testMozelement?

Also, please either avoid or document uses of sleep.  If sleeps aren't necessary, don't use them.  If they are necessary, please add why
(In reply to comment #27)
> Maybe this would be a good patch to inject controller into the module in
> frame.loadFile.  Is there any reason not to do this?  It avoids having to have
> a setupModule for the baseline case where it is `module.controller =
> mozmill.getBrowserController();`

controller will become driver later and will not be bound to a specific window. This driver instance will be augmented into the module scope.
(In reply to comment #29) 
> controller will become driver later and will not be bound to a specific window.
> This driver instance will be augmented into the module scope.

For 2.0 or for what time-frame?  Is there a bug on this?
That was from the meeting you guys had last week. Not sure if there is a bug for that yet. That's Andrews part.
-Selector.prototype.getNode = function (index) {
-  var nodes = this.nodeSearch(this._view.document, this.getNodeForDocument, this.selector);
+  this.getNodeForDocument = function (s) {
+    return this.document.querySelectorAll(s);
+  };
+  var nodes = nodeSearch(this._view.document, this.getNodeForDocument, this.selector);
   return nodes ? nodes[index || 0] : null;

index is undefined in this implementation;  if its unused, we should just go with zero, or add another argument to allow it to be non-zero.  Honestly, I'm not sure what use-cases we're supporting here
I'd be tempted to make elementslib more "classy" -- it feels like a bunch of functions that kinda need each other to work, kinda need some state to work, but aren't actually tied together.  But maybe that's not for this refactor
+var createInstance = function (locatorType, locator, elem) {
+  switch(elem.localName.toLowerCase()) {
+    case 'select':
+    case 'menulist':
+      return new MozMillDropList(locatorType, locator, {"element":elem});
+    case 'input':
+      var type = elem.getAttribute('type');
+      if (type === 'checkbox') {
+        return new MozMillCheckBox(locatorType, locator, {"element":elem});
+      } else if (type === 'radio') {
+        return new MozMillRadio(locatorType, locator, {"element":elem});
+      }
+      break;
+    case 'checkbox':
+      return new MozMillCheckBox(locatorType, locator, {"element":elem});
+    case 'radio':
+      return new MozMillRadio(locatorType, locator, {"element":elem});
+    default:
+  }
+  return new MozMillElement(locatorType, locator, {"element":elem});
+};

Why have default when you don't return there?  I'd move the default case up to the case statement if you're going to have a default key anyway.  Ultimately, its probably more scalable and easier to read to have either a function or a function on e.g. MozMillRadio that tells if it matches given a string: MozMillRadio.prototype.match = function(string) { return (string=='radio' || (string == 'input' && ...)}; This way, the actual, logic of the function, `return new Foo(locatorType, locator, {"element": elem})` doesn't have to be repeated.
Similarly, MozMillElement.prototype.__defineGetter__("element", function() ...
Other than those nits, f+ on all of these.  mozelements looks like a big improvement.  I'm not sure if I'm really qualified to review;  i'd give it an r+, but would be more comfortable having someone who knew the problem space better look it over. We should not only improve the code in this case, but make sure its something that is desired and will be utilized downstream
Unbitrotted frame.js

Jeff, thanks for all the feedback, I'll address your comments in a bit
Attachment #518559 - Attachment is obsolete: true
Attachment #518559 - Flags: feedback?
(In reply to comment #32)
> -Selector.prototype.getNode = function (index) {
> -  var nodes = this.nodeSearch(this._view.document, this.getNodeForDocument,
> this.selector);
> +  this.getNodeForDocument = function (s) {
> +    return this.document.querySelectorAll(s);
> +  };
> +  var nodes = nodeSearch(this._view.document, this.getNodeForDocument,
> this.selector);
>    return nodes ? nodes[index || 0] : null;
> 
> index is undefined in this implementation;  if its unused, we should just go
> with zero, or add another argument to allow it to be non-zero.  Honestly, I'm
> not sure what use-cases we're supporting here

document.querySelectorAll() returns an array of all the elements that match the CSS selector. We were using the getNode() wrapper to get the node at an index in that array e.g. elementslib.Selector(".thumbnail").getNode(3). I guess we'll scrap that functionality?
(In reply to comment #38)

> 
> document.querySelectorAll() returns an array of all the elements that match the
> CSS selector. We were using the getNode() wrapper to get the node at an index
> in that array e.g. elementslib.Selector(".thumbnail").getNode(3). I guess we'll
> scrap that functionality?

no, why would we scrap it?  This refactor should be functionally equivalent to the elmentslib library before the refactor.
Comment on attachment 518551 [details] [diff] [review]
Patch 1.0 - elementslib refactor

>diff --git a/mozmill/mozmill/extension/resource/modules/elementslib.js b/mozmill/mozmill/extension/resource/modules/elementslib.js
>index 91d4b16..5a05ef2 100644
>--- a/mozmill/mozmill/extension/resource/modules/elementslib.js
>+++ b/mozmill/mozmill/extension/resource/modules/elementslib.js
> 
>-var Selector = function(_document, selector) {
>-  if (_document == undefined || selector == undefined) {
>+/**
>+ * Selector()
>+ *
>+ * Finds an element by selector string
>+ */
>+function Selector(_document, selector) {
>+  if (selector == undefined) {
>     throw new Error('Selector constructor did not recieve enough arguments.');
>   }
>-  this._view = _document.defaultView;
>+  this._view = _document == undefined ? _document : _document.defaultView;
I think this is wrong.  If _document is undefined then this._view = _docment?  I think you mean != or !==?

There's not enough here to see how lookup works.  Is lookup using the nodesearch pattern?  Or is it still doing its own thing?

I think these functions are fine building blocks, I don't see any need to make these classes as there is no shared state between them that I see in this first review. Perhaps I misunderstood jhammel's comment.
Attachment #518551 - Flags: feedback? → feedback+
Comment on attachment 518558 [details] [diff] [review]
Patch 1.0 - mozelement.js

>diff --git a/mozmill/mozmill/extension/resource/modules/mozelement.js b/mozmill/mozmill/extension/resource/modules/mozelement.js
>new file mode 100644
>index 0000000..7539cb8
>--- /dev/null
>+++ b/mozmill/mozmill/extension/resource/modules/mozelement.js
>@@ -0,0 +1,630 @@
>+// ***** BEGIN LICENSE BLOCK *****
>+// Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+//
>+// The contents of this file are subject to the Mozilla Public License Version
>+// 1.1 (the "License"); you may not use this file except in compliance with
>+// the License. You may obtain a copy of the License at
>+// http://www.mozilla.org/MPL/
>+//
>+// Software distributed under the License is distributed on an "AS IS" basis,
>+// WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+// for the specific language governing rights and limitations under the
>+// License.
>+//
>+// The Original Code is Mozilla Corporation Code.
>+//
>+// The Initial Developer of the Original Code is
>+// Adam Christian.
>+// Portions created by the Initial Developer are Copyright (C) 2008
>+// the Initial Developer. All Rights Reserved.
>+//
>+// Contributor(s):
>+//  Adam Christian <adam.christian@gmail.com>
>+//  Mikeal Rogers <mikeal.rogers@gmail.com>
>+//  Henrik Skupin <hskupin@mozilla.com>
>+//
Add yourself if you want.
>+// Alternatively, the contents of this file may be used under the terms of
>+// either the GNU General Public License Version 2 or later (the "GPL"), or
>+// the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+// in which case the provisions of the GPL or the LGPL are applicable instead
>+// of those above. If you wish to allow use of your version of this file only
>+// under the terms of either the GPL or the LGPL, and not to allow others to
>+// use your version of this file under the terms of the MPL, indicate your
>+// decision by deleting the provisions above and replace them with the notice
>+// and other provisions required by the GPL or the LGPL. If you do not delete
>+// the provisions above, a recipient may use your version of this file under
>+// the terms of any one of the MPL, the GPL or the LGPL.
>+//
>+// ***** END LICENSE BLOCK *****
>+
Please change to the proper license header comment style: http://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-c

>+var EXPORTED_SYMBOLS = ["Elem", "Selector", "ID", "Link", "XPath", "Name", "Lookup", 
>+                        "MozMillElement", "MozMillCheckBox", "MozMillRadio", "MozMillDropList",
>+                       ];
>+
>+/**
>+ * createInstance()
>+ *
>+ * Returns an new instance of a MozMillElement
>+ * The type of the element is automatically determined
>+ */
So this is for the explicit case where we aren't working with a map, right?  And the test
doesn't want to have to care about what it is instantiating.
>+var Selector = function(_document, selector) {
>+  return createInstance("Selector", selector, elementslib.Selector(_document, selector));
>+};
>+
>+var ID = function(_document, nodeID) {
>+  return createInstance("ID", nodeID, elementslib.ID(_document, nodeID));
>+};
>+
>+var Link = function(_document, linkName) {
>+  return createInstance("Link", linkName, elementslib.Link(_document, linkName));
>+};
>+
>+var XPath = function(_document, expr) {
>+  return createInstance("XPath", expr, elementslib.XPath(_document, expr));
>+};
>+
>+var Name = function(_document, nName) {
>+  return createInstance("Name", nName, elementslib.Name(_document, nName));
>+};
>+
>+var Lookup = function(_document, expression) {
>+  return createInstance("Lookup", expression, elementslib.Lookup(_document, expression));
>+};
How do you keep these from name colliding with the functions in elementslib?  maybe namespace the ones in elementslib?
I guess it's not an issue until a test imports both these files, or if that happens does the commonJS stuff handle it and you
just end up with a MozMillElement.Selector and an ElementsLib.Selector.

Also, isn't it Mozmill (versus MozMill?)  I just noticed that when I had to type it, sorry.

>+
>+
>+/**
>+ * MozMillElement
>+ * The base class for all mozmill elements
>+ */
>+MozMillElement.prototype.__defineGetter__("element", function() {
>+  if (this._element == undefined) {
>+    switch(this._locatorType) {
>+      case "Elem":
>+        this._element = this.locator;
>+        break;
>+      case "Selector":
>+        this._element = elementslib.Selector(this._document, this._locator);
>+        break;
>+      case "ID":
>+        this._element = elementslib.ID(this._document, this._locator);
>+        break;
>+      case "Link":
>+        this._element = elementslib.Link(this._document, this._locator);
>+        break;
>+      case "XPath":
>+        this._element = elementslib.XPath(this._document, this._locator);
>+        break;
>+      case "Name":
>+        this._element = elementslib.Name(this._document, this._locator);
>+        break;
>+      case "Lookup":
>+        this._element = elementslib.Lookup(this._document, this._locator);
>+        break;
>+      default:
>+        throw new Error("Unknown locator type: " + this._locatorType);
Is it only for backwards compatibility that these methods aren't on this class?  I believe it is.  But wanted to ask anyway.
So this is the lazy loading.  You might want to add a comment about that.  I missed the distinction between element and _element.  It'd be good for people reading this code to realize you're making that distinction.

So that we can support the hierarchical map view of things, I think we need some of those parent-child functions on this base class.  From the discussions we've had I believe we agreed that essentially everything (inluding the top of the region map) is a mozmillelement, and then the specific controls inherit from that to become specialized controls.

The role I see for Mozmill here is that it should provide the ability for downstream folks to contruct such maps.  Then in some future version, I want to work on a tool that automatically reads XUL/XBL and constructs those maps for people, so that constructing the map isn't a time consuming function.  But that is future.  Right now, we just need the proper methods/attributes in the base class.

Patch looks good on the balance, a little more to do w.r.t. the mapping, looking forward to the review.
Attachment #518558 - Flags: feedback? → feedback+
Comment on attachment 519461 [details] [diff] [review]
Patch 1.0 - frame.js (unbitrotted)

>diff --git a/mozmill/mozmill/extension/resource/modules/frame.js b/mozmill/mozmill/extension/resource/modules/frame.js
>index 65b33fd..9357c52 100644
>--- a/mozmill/mozmill/extension/resource/modules/frame.js
>+++ b/mozmill/mozmill/extension/resource/modules/frame.js
>@@ -67,16 +67,16 @@ arrayRemove = function(array, from, to) {
>   return array.push.apply(array, rest);
> };
> 
>-mozmill = undefined; elementslib = undefined;
>+mozmill = undefined; mozelement = undefined;
> 
> var loadTestResources = function () {
>   if (mozmill == undefined) {
>     mozmill = {};
>     Components.utils.import("resource://mozmill/modules/mozmill.js", mozmill);
>   }
>-  if (elementslib == undefined) {
>-    elementslib = {};
>-    Components.utils.import("resource://mozmill/modules/elementslib.js", elementslib);
>+  if (mozelement == undefined) {
>+    mozelement = {};
>+    Components.utils.import("resource://mozmill/modules/mozelement.js", mozelement);
>   }
> }
> 
>@@ -90,7 +90,14 @@ var loadFile = function(path, collector) {
>   module.collector = collector
>   loadTestResources();
>   module.mozmill = mozmill;
>-  module.elementslib = elementslib;
>+  module.elementslib = mozelement;
>+  module.getElementBy = mozelement;
Hehe, very sneaky.  Nice backwards compat.

>+  // I don't think injecting each type of element into the global scope manually is a good idea.
>+  // This is more for demonstration purposes until we figure out how to implement the static map.
>+  module.MozMillElement = mozelement.MozMillElement;
>+  module.MozMillCheckBox = mozelement.MozMillCheckBox;
>+  module.MozMillRadio = mozelement.MozMillRadio;
>+  module.MozMillDropList = mozelement.MozMillDroplist;
Holy global namespace batman.  Yeah, we don't want to do this.

Geo, how are you planning to get your static map into the tests?  I had imagined it would be just a shared module that you would include.  The map would take care of ensuring these happy functions are available for direct instantiation.  In fact, I would have thought that the test would never even have to know what they were instantiating per se:
/* Some theoretical test code, imports FirefoxMap */
FirefoxMap.ToolbarRegion.BookmarksButton.click();
FirefoxMap.ToolbarRegion.BookmarksMenu.Menu["Some Default Bookmark"].waitForElement();
assert.equal(FirefoxMap.ToolbarRegion.BookmarksMenu.Menu["Some Default Bookmark"].text, "Some Default Bookmark Title");

I believe that's what we're aiming for in the static map case.  This way we don't have crazy name pollution on the global scope.  We also free test authors from having to know hardly anything about how the tests work under the covers.  They reference these things by following the map.  We'd probably also want to (one day, post 2.0) change our inspector so that if a map is available, it returns you the map string rather than an ID, Lookup, XPath etc locator.
(In reply to comment #40)
> I think this is wrong.  If _document is undefined then this._view = _docment? 
> I think you mean != or !==?
Oops, good catch.

> There's not enough here to see how lookup works.  Is lookup using the
> nodesearch pattern?  Or is it still doing its own thing?
It's still doing its own thing, same as before.

Thanks for the initial review!
(In reply to comment #41)
> >+ * createInstance()
> So this is for the explicit case where we aren't working with a map, right? 
Yes, if by explicit you mean implicit :p

> How do you keep these from name colliding with the functions in elementslib? 
> maybe namespace the ones in elementslib?
> I guess it's not an issue until a test imports both these files, or if that
> happens does the commonJS stuff handle it and you
> just end up with a MozMillElement.Selector and an ElementsLib.Selector.
The way I envisioned it was that elementslib.js is kind of like a 'private helper method' to mozelement.js. It also only becomes a problem if both files get imported into the same namespace. But you're right, since the user doesn't see these methods it wouldn't hurt to put them into a different scope or change the names.

> Also, isn't it Mozmill (versus MozMill?)  I just noticed that when I had to
> type it, sorry.
Doesn't matter to me. I'm open to change any naming to whatever works best.

> 
> >+
> >+
> >+/**
> >+ * MozMillElement
> >+ * The base class for all mozmill elements
> >+ */
> >+MozMillElement.prototype.__defineGetter__("element", function() {
> >+  if (this._element == undefined) {
> >+    switch(this._locatorType) {
> >+      case "Elem":
> >+        this._element = this.locator;
> >+        break;
> >+      case "Selector":
> >+        this._element = elementslib.Selector(this._document, this._locator);
> >+        break;
> >+      case "ID":
> >+        this._element = elementslib.ID(this._document, this._locator);
> >+        break;
> >+      case "Link":
> >+        this._element = elementslib.Link(this._document, this._locator);
> >+        break;
> >+      case "XPath":
> >+        this._element = elementslib.XPath(this._document, this._locator);
> >+        break;
> >+      case "Name":
> >+        this._element = elementslib.Name(this._document, this._locator);
> >+        break;
> >+      case "Lookup":
> >+        this._element = elementslib.Lookup(this._document, this._locator);
> >+        break;
> >+      default:
> >+        throw new Error("Unknown locator type: " + this._locatorType);
> Is it only for backwards compatibility that these methods aren't on this 
> class?
If you mean the methods in this file above, then no. Those are there so we can implicitly return an element. If they were on this class, then we'd have to instantiate an element object, search for the DOM element, and then implicitly instantiate a new element object.

> So this is the lazy loading.  You might want to add a comment about that.  I
> missed the distinction between element and _element.  It'd be good for people
> reading this code to realize you're making that distinction.
Noted.
 
> So that we can support the hierarchical map view of things, I think we need
> some of those parent-child functions on this base class.
Not sure which ones you're talking about, you mean passing in the owner? The only problem with that is that knowing who your 'owner' is purely something needed by the static map, not when we instantiate implicitly.
> Also, isn't it Mozmill (versus MozMill?)  I just noticed that when I had to
> type it, sorry.

Sadly (IMHO), we use MozMill mostly (exclusively?) for naming classes/functions.  I'm against this, but its a huge and risky cleanup just for a name change.
> Geo, how are you planning to get your static map into the tests?  I had
> imagined it would be just a shared module that you would include.  The map
> would take care of ensuring these happy functions are available for direct
> instantiation.  In fact, I would have thought that the test would never even
> have to know what they were instantiating per se:

Yeah, shared-mod w/ the map in it. The only reason I can see that the test themselves would have to import the classes themselves is to do an instanceOf, which would only be needed in the "I have no idea what this is but return me a subclass anyway" case that I don't think we'll make much use of.
Already mentioned to Ahal in person, but IMO we should go ahead and land these before they rot more. Since the intention is there to support us, I don't see much more purpose in sitting on the code. We should just treat incompatibilities/problems as bugs.

Ahal and I will have to come up with a "static map" testcase or three that ensure our use cases are supported, and determine the right flow for future development of the subclasses. He and I will meet in person this aft. to hammer some of that out.
(In reply to comment #47)
> Already mentioned to Ahal in person, but IMO we should go ahead and land these
> before they rot more. Since the intention is there to support us, I don't see
> much more purpose in sitting on the code. We should just treat
> incompatibilities/problems as bugs.
> 
> Ahal and I will have to come up with a "static map" testcase or three that
> ensure our use cases are supported, and determine the right flow for future
> development of the subclasses. He and I will meet in person this aft. to hammer
> some of that out.

I agree that we should land it sooner rather than later.  I don't see there is any value in sitting on this code any longer. 

w.r.t. putting the "parent/child" settings into the code so taht we can properly construct the static maps - I think we can and should do that by using optional parameters on the constructors of the base class.  Then you can say things like:
* Implicit construction: 
MozmillElement(somenode)

* Static map style construction:
MozmillElement(somenode, owner)

In the implicit case, we simply default "owner" (and any child nodes we also have created) to null.  If the owner is specified then we use that.
I put up some early documentation for the new Element object hierarchy: https://developer.mozilla.org/en/Mozmill/Mozmill_Element_Object

This will need to be updated as we add new subclasses and new methods to existing subclasses. I also still need to document the 'findElement' method, as well as how to extend the hierarchy.
This is an incremental patch to mozelement over patch 1.0.

See https://github.com/ahal/mozmill/commit/0f62f60fe2c78400d566c456f2d3bb6f233d9550

for changes to the rest of the files. The only differences however are:
elementslib.js: fixed the bugs in the Selector method
frame.js: took out the subclasses from the global scope
Attachment #518551 - Attachment is obsolete: true
Attachment #518558 - Attachment is obsolete: true
Attachment #518561 - Attachment is obsolete: true
Attachment #519461 - Attachment is obsolete: true
Attachment #521698 - Flags: review?(ctalbert)
Comment on attachment 521698 [details] [diff] [review]
Patch 1.1 - mozelement.js

I like the new isType stuff.  Excellent.
Attachment #521698 - Flags: review?(ctalbert) → review+
Sorry for double spam, also checked the git diff.  Looks good.  Please land.
master: https://github.com/mozautomation/mozmill/commit/313332ba86d310d773197c549dfa31551484c8f6

I'm keeping this bug open for further development. Anything relating to the framework of how things work can go in this bug.

We should file a new bug for stuff related to creating new subclasses and adding methods to existing subclasses.
This patch should make it very easy for shared-modules to extend the mozmill's element hierarchy.  I put up detailed docs here: https://developer.mozilla.org/en/Mozmill/Mozmill_Element_Object/Extending_Element_Hierarchy

Look at the 'Putting it All Together' section for an example of how the shared module will work with this patch.
Attachment #528470 - Flags: review?(ctalbert)
Comment on attachment 528470 [details] [diff] [review]
Patch 2.0 - Use subclasses list so shared modules can push their subclasses onto it

Looks good.  The docs are great. Thanks
Attachment #528470 - Flags: review?(ctalbert) → review+
I'm going to close this bug.  If any issues with the framework of the hierarchy
arise this can be re-opened or a new bug filed.  Issues related to
creating/modifying subclasses should go in a separate bug.

master:
https://github.com/mozautomation/mozmill/commit/0585c4804ff76530cd3c00fe66f0799b8f019ff0
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.