634111 - Tracking Bug: Shared Modules Refactor -- Sprint 2

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

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

Project tracking bug for Sprint 2 of the Shared Modules refactor, week ending
Feb 25th, 2011. Individual tasks will be added as dependencies

Comment 1

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

Geo, when do you plan to create the sprint2 branch in your repository?

Comment 2

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

Henrik, we decided that we're each creating our own. I'll create mine, you create yours. Call it sprint2-whimboo or something.

Comment 3

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

iow, "soon," but it shouldn't matter to you.

Comment 4

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

Henrik, if you have time please get started looking at the sprint2-docs branch:

https://github.com/geoelectric/mozmill-api-refactor/commit/7221e7b6ad82c5906b4ab53d852c8616fed7d2e3

Element and HtmlXulElement are now documented here. 

This will also get you started on writing your own field/method docs. You're welcome to experiment with different tag styles and whatnot, but this was what it took to make the docs look "right" to me.

Tomorrow I'll figure out the web space issue and upload generated docs somewhere you can see them. Until then, just pull the sprint2-docs branch and generate them locally.

I'll create a diff later and get it uploaded, but as I said in the sprint 1 bug, we need to get faster so I don't want to hang on a lot of formality. I just need our eyes on each other's code, not strict r+/r- process.

Comment 5

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

Once the generated docs are up I will give it a closer look.

Comment 6

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

Attachment: Sprint 2 Docs diff

Diff for sprint2-docs

Comment 7

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

Attachment: Generated docs for sprint2-docs

Generated docs for attachment 516123 [details] [diff] [review], outline and default

Comment 8

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

Henrik, note this has the verbose tagging. I'll try to get it to work in the style you've landed upon in refactor.

Comment 9

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

Comment on attachment 516123 [details] [diff] [review]
Sprint 2 Docs diff

>+   *     <dd>node</dd>   <dt>A node, as in Mozmill's Elem()</dt>

We should put the dt elements onto their own lines. A way like that will cause troubles with alignments and needs more work if a name changes. Also it doesn't align to the way how @param have to be added. Those entries also have the description in the next line(s).

>   initialize: function Element_initialize(locatorType, locator, owner) {

So you aren't using the @lends tag with the prototype right now which requires you to add the @fieldOf and @name line to all class methods and properties. When are you planning to add this?

>+  /**
>+   * The Mozmill controller object associated with this Element.
>+   *
>+   * @name controller
>+   * @type controller
>+   * @fieldOf widgets.Element#
>+   */
>   get controller() {
>     return this._controller;
>   },

I wonder if we should simply remove this getter to not expose this property to tests. It's really private and should not be used outside of the class. 

>+   * The DOM window object associated with this Element.
>+   *
>+   * @name window
>+   * @type window
>+   * @fieldOf widgets.Element#

Can we add a @see tag to the DOMWindow idl file?

>+   * The DOM node object associated with this Element.
>+   *
>+   * @name node
>+   * @type node
>+   * @fieldOf widgets.Element#

Same here: https://developer.mozilla.org/en/DOM/Node

>   get node() {
>     return this.elem.getNode();
>   }

Whereby we have to make sure to test elem first that it is not null.

>+   * @param {node|String} locator 
>+   *   The actual locator. If locatorType is "node," a node object is expected. 
>+   *   For all other types, a String is expected.
>+   * @param {document|Element} owner
>+   *   The owner (parent) of this Element. The top of an Element map is owned by 
>+   *   a document. Other members of the map are owned by their parent Elements.

Is there a way to directly link to Node and Element in the type declaration? If not shall we do that inline? IMO it will improve the usability of the docs a lot. Also 'owner vs. parent', an owner can only be the document itself. The parent of an element is really a parent. 

>@@ -223,6 +293,23 @@ var XmlTree = Inheritance.Class.extend(Element,

What is an XMLTree? I can't find any description except the @param and general doc declaration. Also didn't we wanted to remove the XML classes for the time beeing?

>+   * @methodOf widgets.HtmlXulElement#

XUL and HTML are acronyms and should be used that way.

>+   * @param {Number} [left=0] Relative horizontal coordinate inside Element.
>+   * @param {Number} [top=0] Relative vertical coordinate inside Element.

We should be consistent where to put the description. As for all the other docs it should be contained in its own line.

>+   * @returns {Boolean} true if succeeded, false otherwise.
>+   */
>   click: function HtmlXulElement_click(left, top) {
>-    this.controller.click(this.elem, left, top);
>+    return this.controller.click(this.elem, left, top);

controller.click doesn't return a Boolean:
https://github.com/mozautomation/mozmill/blob/hotfix-1.5.2/mozmill/mozmill/extension/resource/modules/controller.js#L489

>   doubleClick: function HtmlXulElement_doubleClick(left, top) {
>-    this.controller.click(this.elem, left, top);
>+    return this.controller.click(this.elem, left, top);

Same here.

>+   * @param {String} keycode Either a literal like 'b' or an enum like 'VK_ESCAPE'.

We should come up with the solution for keycode vs. aKeyCode quickly.

>+   * @param {Object} [modifiers={}] Indicates modifier keys. true means pressed.
>+   * @param {Boolean} [modifiers.ctrlKey=false] The Ctrl key.
>+   * @param {Boolean} [modifiers.altKey=false] The Alt/Option key.
>+   * @param {Boolean} [modifiers.shiftKey=false] The Shift key.
>+   * @param {Boolean} [modifiers.metaKey=false] The Meta/Cmd key.
>+   * @param {Boolean} [modifiers.accelKey=false] Ctrl key on Windows/Linux, 
>+                                                 Cmd key on Mac.

So you are mixing up definition lists and this new way. I like it, but we should be consistent.

>   mouseDown: function HtmlXulElement_mouseDown(button, left, top) {
>   mouseUp: function HtmlXulElement_mouseUp(button, left, top) {
>   rightClick: function HtmlXulElement_rightClick(left, top) {

We absolutely have to add controller.mouseEvent. If you wanna do that in a follow-up please file a bug. For info see:
https://github.com/mozautomation/mozmill/blob/hotfix-1.5.2/mozmill/mozmill/extension/resource/modules/controller.js#L409

Comment 10

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

Comment on attachment 516124 [details]
Generated docs for sprint2-docs

Instead of zip files we should upload examples somewhere. Otherwise I kinda like the documentation even we have to improve the template for our code. Looks good.

Comment 11

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

(In reply to comment #9)

You make some good points below, and I've answered one by one. However, you're doing a lot of reviewing existing code here. We can fix that stuff in refactoring.

Just for the sake of sanity, let's stick to reviewing things that have changed in a given patch, and maybe starting a backlog to prioritize other desired changes so we don't lose them

I have some ideas about how best to do this for Milestone 2, and I'll talk to you about them offline.

> We should put the dt elements onto their own lines. A way like that will cause
> troubles with alignments and needs more work if a name changes. Also it doesn't
> align to the way how @param have to be added. Those entries also have the
> description in the next line(s).

Well, you make a good point re: realigning if a name changes or a longer one is added.

I was intentionally not aligning with @param--things that are different should look different, IMO, for max clarity.

I tend to think formatting of this sort of thing should be case-by-case. Per our convo on IRC, let's hold off reviews on comment formatting and the like for now, unless it's truly just unreadable, and come back to these sorts of issues later once we've made more progress.

> So you aren't using the @lends tag with the prototype right now which requires
> you to add the @fieldOf and @name line to all class methods and properties.
> When are you planning to add this?

Well, I don't know that's why I had to add the @memberOf/name stuff, but I'm going to give it a shot with your methodology. I want to do that during refactoring though since it touches the whole file, including already-existing stuff, not just these docs in the patch.

> >   get controller() {
> 
> I wonder if we should simply remove this getter to not expose this property to
> tests. It's really private and should not be used outside of the class. 

Especially with it going away. :/ I left it exposed in case we needed "friend" functions that would use it, but I think you make a good point. 

I'll remove the getter during refactoring. It's out of scope for this review because it's already existing code.

> >+   * The DOM window object associated with this Element.
> 
> Can we add a @see tag to the DOMWindow idl file?

Sure.

> >+   * The DOM node object associated with this Element.
> 
> Same here: https://developer.mozilla.org/en/DOM/Node

Yep.

> 
> >   get node() {
> >     return this.elem.getNode();
> >   }
> 
> Whereby we have to make sure to test elem first that it is not null.

elem can't be null. The elem getter makes sure of that. That's why you always go through your properties, and never straight to _fields, even internally, unless you absolutely have to.

> Is there a way to directly link to Node and Element in the type declaration? 

Not that I know of. BTW, we should settle on capitalization for Node, Document, Number, String, etc. I went lowercase on node and document because MDC generally does. Other non-Mozilla docs go initial-upper, since it's a class.

> If not shall we do that inline? IMO it will improve the usability of the docs
> a lot. Also 'owner vs. parent', an owner can only be the document itself. The
> parent of an element is really a parent.

You're welcome to play with ways to do it inline, but I want to see what it looks like in the code before I say yes/no. If you're asking whether I think we should put @see Node and @see Element all over the place, though, not sure it's worth the extra clutter. Let's talk more later.

Re: owner vs. parent, I stayed away from parent because the Javascript Node uses parent to specifically mean "the thing right above me." In our case, a node's owner can be any ancestor because selectors can easily skip levels.

The other reason is that it really is an owner in the has-a object-oriented sense. That element owns other elements (not parents other nodes). We use the third input to be the document for the top-level Element as a slightly hacky convenience, but note that internally it's not assigned as the owner. But owner_or_document was too long of a @param name. ;)


> >@@ -223,6 +293,23 @@ var XmlTree = Inheritance.Class.extend(Element,
> 
> What is an XMLTree? I can't find any description except the @param and general
> doc declaration. Also didn't we wanted to remove the XML classes for the time
> beeing?

Yep, we're removing it in refactoring. My original thought was that XMLTree was the equivalent of HTMLRegion, i.e. something that owned a tree of XML elements.

> >+   * @methodOf widgets.HtmlXulElement#
> 
> XUL and HTML are acronyms and should be used that way.

Well, I was. In a lot of naming schemes (including my preferred) you only capitalize the first letter of acronyms. But I forgot Mozilla doesn't generally do that. 

I'll fix in refactoring, though, since it was already existing code.

> 
> >+   * @param {Number} [left=0] Relative horizontal coordinate inside Element.
> >+   * @param {Number} [top=0] Relative vertical coordinate inside Element.
> 
> We should be consistent where to put the description. As for all the other docs
> it should be contained in its own line.

Disagree; I think it's roughly like our standard on if. -If- within a given jsdoc comment any one description is on its own line, they all should be. But in another jsdoc comment, it can be different.

It's mostly a matter of length. If the descriptions are very short (like the keyPress modifiers, for example) it's much less readable to have them that spread out.

> 
> >+   * @returns {Boolean} true if succeeded, false otherwise.
> >+   */
> >   click: function HtmlXulElement_click(left, top) {
> >-    this.controller.click(this.elem, left, top);
> >+    return this.controller.click(this.elem, left, top);
> 
> controller.click doesn't return a Boolean:
> https://github.com/mozautomation/mozmill/blob/hotfix-1.5.2/mozmill/mozmill/extension/resource/modules/controller.js#L489

Yes it does:

https://developer.mozilla.org/en/Mozmill/Mozmill_Controller_Object#click.28.29

OTOH, the code for 1.5.2's Click() has a bug in it because they forgot to return true like they do for every other mouse event in there. We should file that. :)

IMO, always write to the documented interface, not the actual implementation, unless it'll break code.

> >   doubleClick: function HtmlXulElement_doubleClick(left, top) {
> >-    this.controller.click(this.elem, left, top);
> >+    return this.controller.click(this.elem, left, top);
> 
> Same here.

Same answer:

https://developer.mozilla.org/en/Mozmill/Mozmill_Controller_Object#doubleClick.28.29

But even the actual code returns a true here, so make sure you double-check this stuff before calling it out please. :)

> >+   * @param {String} keycode Either a literal like 'b' or an enum like 'VK_ESCAPE'.
> 
> We should come up with the solution for keycode vs. aKeyCode quickly.

I looked at mxr, and decided that--although I think it's a stupid, stupid standard to have in an explicit-this language--it is our standard. I'll change my code during refactoring.

> >+   * @param {Boolean} [modifiers.accelKey=false] Ctrl key on Windows/Linux, 
> >+                                                 Cmd key on Mac.
> 
> So you are mixing up definition lists and this new way. I like it, but we
> should be consistent.

No, I'm not mixing them up (though you kinda did in your other checkin). @params are used for separate identifiers/fields. Definition lists are when any given identifier can take one of several values (enums), like locatorType on the constructor.

If I had a @value tag or something, I'd use that instead of a definition list; they're a formatting workaround.

> 
> >   mouseDown: function HtmlXulElement_mouseDown(button, left, top) {
> >   mouseUp: function HtmlXulElement_mouseUp(button, left, top) {
> >   rightClick: function HtmlXulElement_rightClick(left, top) {
> 
> We absolutely have to add controller.mouseEvent. If you wanna do that in a
> follow-up please file a bug. For info see:
> https://github.com/mozautomation/mozmill/blob/hotfix-1.5.2/mozmill/mozmill/extension/resource/modules/controller.js#L409

Yeah, they need to document it though. Again, I was warned--and agreed--to only write to documented stuff. But in this case, I know we need it for modifier keys (that is the use case, right?).

Apparently, we have lots of bugs to file for refactoring. Basically, though, the process will be you file bugs for things you want and I'll file bugs for things I want. Or ask me, and I might do a favor. :)

I'll get the refactoring bug filed and will send to you. We can start filing issues from this list and others.

Comment 12

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

BTW, since none of your comments dealt specifically with this patch, I'm going to go ahead and merge so we can move forward.

Comment 13

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

Oops, correcting myself. I will go ahead and add the @sees to the IDL, but I'm going to merge after that. I'll post the github commit.

Comment 14

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

Committed here: https://github.com/geoelectric/mozmill-api-refactor/commit/7621d01ad988fe72648a7ef7b023593ffafa8403