638967 - Refactoring Round -- Milestone 1

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

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

Parent bug for refactoring tasks to be completed before we close out Milestone 1

Comment 1

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

Attachment: All identifier-related refactor fixes

Sorry this took so long. I had to touch most every file, and managed to introduce a syntax error that took me forever to track down.

All tests (both UI and back-end) run against this fine now.

Here's what fixes you'll find here:

bug 638969 Change naming of widget class tree per A-Team meeting
bug 638970 Change acronym capitalization
bug 638971 Change to aParam standard 
bug 638976 Remove XML class tree 
bug 640051 Align naming of multiword modules 
bug 640060 Lowercase module objects 

Also, I removed any trailing spaces and put line ends at the ends of the couple of files that didn't have them.

Finally, one slightly more substantial change: I removed the browser.get() function in favor of just directly injecting the Browser class in initTestModule(). So tests now say browser = new Browser() in their setupModule, not browser = Browser.get().

Now that we've lowercased module names, the browser module was going to conflict with the most obvious test variable name "browser". I changed it so tests don't have to reference the module. Anyway, the get() function was kind of useless, so I don't feel like we're cutting anything important.

Generated docs at:

http://people.mozilla.com/~gmealer/docs/sprint3-rename

Github branch at:

https://github.com/geoelectric/mozmill-api-refactor/tree/sprint3-renames

Comment 2

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

Comment on attachment 518586 [details] [diff] [review]
All identifier-related refactor fixes

>diff --git a/modules/dom_utils.js b/modules/dom_utils.js
>deleted file mode 100644

Haven't you used 'git mv' here to move the file? 

>+var tabBar = require("tabbar");
>+var navBar = require("navbar");
>+var widgets = require("widgets");
>+var inheritance = require("../external/inheritance");

nit: We should have those in the right alphabetical order

>+var Browser = inheritance.Class.extend(widgets.Region, {

nit: A quick jsdoc entry would be nice so we can get this out of the global scope.

>+  openURL: function Browser_openUrl(aText, aTimeout) {

Browser_openURL

>+++ b/modules/ui/tabbar.js
>+var inheritance = require("../external/inheritance");
>+var widgets = require("widgets");
>+var dom = require("../dom");

nit: Same for ordering here.

>+++ b/modules/ui/widgets.js
>+var inheritance = require("../external/inheritance");
>+var dom = require("../dom");

>+    if (!aLocator) {
>       throw new Error("Missing locator");
[..]
>+        throw new Error("Invalid locator type: " + aLocatorType);

Something we should fix (not necessarily in m1) to use special Error classes for those situations.

>+var Widget = inheritance.Class.extend(Element,
>+/** @lends widgets.Widget */

It has to lend the prototype of Widget so we can get rid of all the @memberOf lines.

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

Defaults for all click methods on the controller is the center of the element. That's kinda important.

>+var Region = inheritance.Class.extend(Widget,
>+/** @lends widgets.Region */

Same here for prototype and @memberOf.

Please update those parts before check-in the code. Otherwise it looks fine.

Comment 3

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

(In reply to comment #2)

> >diff --git a/modules/dom_utils.js b/modules/dom_utils.js
> >deleted file mode 100644
> 
> Haven't you used 'git mv' here to move the file? 

I did. That's how it came out in the git diff.

> 
> >+var tabBar = require("tabbar");
> >+var navBar = require("navbar");
> >+var widgets = require("widgets");
> >+var inheritance = require("../external/inheritance");
> 
> nit: We should have those in the right alphabetical order

OK. I'll touch these up.

> >+var Browser = inheritance.Class.extend(widgets.Region, {
> 
> nit: A quick jsdoc entry would be nice so we can get this out of the global
> scope.

Not in scope for this review (it's an improvement of existing code) but noted. That would be a good thing for you to file on one of the refactoring round bugs.

However, nothing should be showing up for you in any scope on this, since it's not jsdoc'ed at all yet. It'd only show up if you ran docs with -a, which I don't want us to do.

> 
> >+  openURL: function Browser_openUrl(aText, aTimeout) {
> 
> Browser_openURL

Good catch, I'll fix.

> 
> >+++ b/modules/ui/tabbar.js
> >+var inheritance = require("../external/inheritance");
> >+var widgets = require("widgets");
> >+var dom = require("../dom");
> 
> nit: Same for ordering here.

OK.

> 
> >+++ b/modules/ui/widgets.js
> >+var inheritance = require("../external/inheritance");
> >+var dom = require("../dom");
> 
> >+    if (!aLocator) {
> >       throw new Error("Missing locator");
> [..]
> >+        throw new Error("Invalid locator type: " + aLocatorType);
> 
> Something we should fix (not necessarily in m1) to use special Error classes
> for those situations.

Agreed, also something good for you to file on m2 refactor if you get a chance. It'd be better to have short bugs there rather than lose suggestions in comments.

> 
> >+var Widget = inheritance.Class.extend(Element,
> >+/** @lends widgets.Widget */
> 
> It has to lend the prototype of Widget so we can get rid of all the @memberOf
> lines.

Out of scope for this review; I let you know which bugs I fixed above. That one will be fixed with the changes for Bug 638973

> 
> >+   * @param {Number} [aLeft=0] Relative horizontal coordinate inside Element.
> >+   * @param {Number} [aTop=0] Relative vertical coordinate inside Element.
> 
> Defaults for all click methods on the controller is the center of the element.
> That's kinda important.

OK. Mozmill team -really- needs to keep up their docs, as that's not what's currently documented. I'll change our docs to say center before I check in. The code itself is simply a passthrough to the controller versions, so does actually default to whatever those do.

> 
> Same here for prototype and @memberOf.

Will be fixed on Bug 638973.

I'll post the commit once I've changed and merged.

Comment 4

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

(In reply to comment #3)
> > >+var Browser = inheritance.Class.extend(widgets.Region, {
> > 
> > nit: A quick jsdoc entry would be nice so we can get this out of the global
> > scope.
> 
> Not in scope for this review (it's an improvement of existing code) but noted.
> That would be a good thing for you to file on one of the refactoring round
> bugs.
> 
> However, nothing should be showing up for you in any scope on this, since it's
> not jsdoc'ed at all yet. It'd only show up if you ran docs with -a, which I
> don't want us to do.

Oh, I see what happened here. The Browser in global scope isn't from this file, it's from your original POC where you had pseudo-JSDoc comments in there. If you look at the file it's defined in, you'll see it's the "ui/whimboo" subdir version.

Now that we've pretty much replaced every file you did in the original, I propose either deleting the whimboo subdirs entirely, or at least moving them out of the modules/firefox directories into their own top-level so they won't get mixed up with our stuff.

Comment 5

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

Commit of the patch: https://github.com/geoelectric/mozmill-api-refactor/commit/1e198aa9e54747cfb9516a199b36d0c81fc61a51

Commit where I had to re-add dom.js and stack.js because it turns out the diff/patch method doesn't properly merge renamed files unless you explicitly re-add them: https://github.com/geoelectric/mozmill-api-refactor/commit/31026ba9a1ac632a2a16e09f4e679ea94dbdf57e

Comment 6

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

All sub-bugs done, so resolving.