1079717 - Add about:accounts in-content page class

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P1)

Description

[Daniel Gherasim]

As requested in bug 1005811 let's add a class for handling the about:accounts in-content page.

This is blocking the TPS tests as we need it there (so P1).
2 methods of opening it comes from the preferences in-content page, so we might need some TO DO items here if the prefs doesn't get landed before.

Comment 1

[Daniel Gherasim]

I already started to work on this.

Comment 2

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v1.0

So what I had to do here was just to overwrite the getElements method and to extend the open.
I extended the open method because it had all the listeners for opening the new tab and setting the contentWindow. 
I had to wait for iFrame to load though, for that I added a listener on window and I check that the target location is the one that is default in preferences, that was the safer way I could though since we can't attache listeners to the iFrame, without hitting a race-condition.

Comment 3

[Andrei Eftimie]

Comment on attachment 8520563 [details] [diff] [review]
patch v1.0

Review of attachment 8520563 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox/lib/tests/testAboutAccountsPage.js
@@ +7,5 @@
> +// Include required modules
> +var browser = require("../ui/browser");
> +var aboutAccounts = require("../ui/about-accounts-page");
> +
> +  function setupModule(aModule) {

nit: indentation

@@ +29,5 @@
> +  var signIn = aboutAccounts.getElement({type: "signIn"});
> +  signIn.waitThenClick();
> +
> +  var signUp = aboutAccounts.getElement({type: "signUp"});
> +  signUp.waitThenClick();

Should we assert something here, to know that the page has actually loaded?

::: firefox/lib/ui/about-accounts-page.js
@@ +23,5 @@
> +
> +  this._browserWindow = aBrowserWindow;
> +}
> +
> +AboutAccountsPage.prototype = new baseInContentPage.BaseInContentPage(true);

Please use Object.create for extending a class.
See unknown-content-type-dialog.js for an example.

@@ +39,5 @@
> + * @params {string} [aSpec.method="menu"|"signIn"|"signUp"|"callback"]
> + *         Method to use when opening the AboutAccountsPage
> + */
> +AboutAccountsPage.prototype.open = function AboutAccountsPage_open(aSpec={}) {
> +  var controller = aSpec.controller || this.browserWindow.controller;

We should expect a browserWindow object, not a controller.
Use the controller as part of the browserWindow instance.

@@ +66,5 @@
> +        aSpec.callback();
> +      default:
> +        assert.fail("Unknown method - " + method);
> +    }
> +  }

nit: leave an empty line here

@@ +91,5 @@
> + * @returns {Object[]} Array of elements which have been found
> + */
> +AboutAccountsPage.prototype.getElements = function (aSpec) {
> +  var spec = aSpec || { };
> +  var root = spec.parent ? spec.parent.getNode() : this.browserWindow.controller.window.document;

nit: split this on 2 lines please

Comment 4

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: pa

(In reply to Andrei Eftimie from comment #3)
> @@ +29,5 @@
> > +  var signIn = aboutAccounts.getElement({type: "signIn"});
> > +  signIn.waitThenClick();
> > +
> > +  var signUp = aboutAccounts.getElement({type: "signUp"});
> > +  signUp.waitThenClick();
> 
> Should we assert something here, to know that the page has actually loaded?
waitThanClick makes the assertion that the node exists and they do only if the prior action executed correctly. This is hard to follow indeed so I added some comments

> > + * @params {string} [aSpec.method="menu"|"signIn"|"signUp"|"callback"]
> > + *         Method to use when opening the AboutAccountsPage
> > + */
> > +AboutAccountsPage.prototype.open = function AboutAccountsPage_open(aSpec={}) {
> > +  var controller = aSpec.controller || this.browserWindow.controller;
> 
> We should expect a browserWindow object, not a controller.
> Use the controller as part of the browserWindow instance.
Then we should add an extra check to see that the browserWindow has been provided or not, even if we care only about controller.

Comment 5

[Andrei Eftimie]

Comment on attachment 8523857 [details] [diff] [review]
pa

Review of attachment 8523857 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox/lib/ui/about-accounts-page.js
@@ +33,5 @@
> + * @params {object} [aSpec={}]
> + *         Information about opening the page
> + * @params {function} [aSpec.callback]
> + *         Function that triggers the opening
> + * @params {MozMillController} [aSpec.controller]

Same here, expect a browserWindow object

@@ +39,5 @@
> + * @params {string} [aSpec.method="menu"|"signIn"|"signUp"|"callback"]
> + *         Method to use when opening the AboutAccountsPage
> + */
> +AboutAccountsPage.prototype.open = function AboutAccountsPage_open(aSpec={}) {
> +  var controller = aSpec.controller || this.browserWindow.controller;

As said previously save a reference to browserWindow, not controller

@@ +51,5 @@
> +    if (location.contains(signUpURI)) {
> +      pageLoaded = true;
> +    }
> +  }
> +  controller.window.addEventListener("DOMContentLoaded", contentLoaded);

This should always be `this.browserWindow.controller`

@@ +57,5 @@
> +  // Define the callback that opens the in-content page
> +  var callback = () => {
> +    switch (method) {
> +      case "menu":
> +        controller.mainMenu.click("#sync-setup");

And here use the local `browserWindow` instance

@@ +83,5 @@
> + * Retrieve list of UI elements based on the given specification
> + *
> + * @param {Object} aSpec
> + *        Information of the UI elements which should be retrieved
> + * @param {Object} [aSpec.parent=this._controller.window.document]

this.browserWindow.controller.window.document

Comment 6

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v1.1

We can't open the about:accounts page from another place than browserWindow(a modal dialog for example), so it has no sense to keep the controller parameter in the opening method.

Comment 7

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8525956 [details] [diff] [review]
patch v1.1

Review of attachment 8525956 [details] [diff] [review]:
-----------------------------------------------------------------

With these changes you have my r+.

::: firefox/lib/tests/testAboutAccountsPage.js
@@ +34,5 @@
> +  // "Sign Up" link should be displayed when we clicked on "Sign In" link
> +  var signUp = aboutAccounts.getElement({type: "signUp"});
> +  signUp.waitThenClick();
> +
> +  signIn.waitThenClick();

Why we have this twice?

::: firefox/lib/ui/about-accounts-page.js
@@ +84,5 @@
> + * @param {Object} aSpec
> + *        Information of the UI elements which should be retrieved
> + * @param {Object} [aSpec.parent=this._controller.window.document]
> + *        Parent of the element to find
> + * @param {String} aSpec.type

Please let the types be in lowercase. We had a bug for refactoring all these.

Comment 8

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Andreea Matei [:AndreeaMatei] from comment #7)
> With these changes you have my r+.
> 
> ::: firefox/lib/tests/testAboutAccountsPage.js
> @@ +34,5 @@
> > +  // "Sign Up" link should be displayed when we clicked on "Sign In" link
> > +  var signUp = aboutAccounts.getElement({type: "signUp"});
> > +  signUp.waitThenClick();
> > +
> > +  signIn.waitThenClick();
> 
> Why we have this twice?
They are not the same elements/actions.

Comment 9

[Andreea Matei [:AndreeaMatei]]

Oh right, then it's fine. Update just the other thing and add review for Henrik. Thanks.

Comment 10

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v2.0

Thanks

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

Comment on attachment 8526705 [details] [diff] [review]
patch v2.0

Review of attachment 8526705 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox/lib/tests/testAboutAccountsPage.js
@@ +5,5 @@
> +"use strict";
> +
> +// Include required modules
> +var browser = require("../ui/browser");
> +var aboutAccounts = require("../ui/about-accounts-page");

nit: ordering.

::: firefox/lib/ui/about-accounts-page.js
@@ +4,5 @@
> +
> +"use strict";
> +
> +var baseInContentPage = require("base-in-content-page");
> +var prefs = require("../prefs");

Please separate ui and backend modules.

@@ +6,5 @@
> +
> +var baseInContentPage = require("base-in-content-page");
> +var prefs = require("../prefs");
> +
> +const FXACCOUNTS_REMOTE_SIGNUP_URI = "identity.fxaccounts.remote.signup.uri";

This is a constant for a pref, so please prefix it with PREF_ as we usually do.

@@ +11,5 @@
> +const TIMEOUT_REMOTE = 30000;
> +
> +/**
> + * 'About Accounts' in content page class
> + * @abstract

This is not an abstract class.

@@ +34,5 @@
> + *         Information about opening the page
> + * @params {function} [aSpec.callback]
> + *         Function that triggers the opening
> + * @params {string} [aSpec.method="menu"|"signIn"|"signUp"|"callback"]
> + *         Method to use when opening the AboutAccountsPage

Again, possible values for parameters are part of the description. I pointed that out a lot in other reviews, but this is still happening. Please ensure to read review comments even for bugs you are not working on to learn about those things.

@@ +46,5 @@
> +  function contentLoaded(e) {
> +    var location = e.target.defaultView.location.toString();
> +    if (location.contains(signUpURI)) {
> +      pageLoaded = true;
> +    }

So Mozmill itself does not work here? Please clarify that. I don't see any comment here why this is necessary at all, and cannot be covered via waitForPageLoad().

@@ +49,5 @@
> +      pageLoaded = true;
> +    }
> +  }
> +  this.browserWindow.controller.window
> +                    .addEventListener("DOMContentLoaded", contentLoaded);

So the page is in-content, why do you attach the listener to the chrome window?

@@ +59,5 @@
> +        this.browserWindow.controller.mainMenu.click("#sync-setup");
> +        break;
> +      case "callback":
> +        assert.equal(typeof aSpec.callback, "function",
> +                     "Callback function has been defined");

I think I mentioned this too a hundred of times... there is no need for function given that callback implicitly is a function.

@@ +82,5 @@
> + * Retrieve list of UI elements based on the given specification
> + *
> + * @param {object} aSpec
> + *        Information of the UI elements which should be retrieved
> + * @param {object} [aSpec.parent=this._controller.window.document]

This does not match. Maybe simply 'window.document'?

@@ +92,5 @@
> + */
> +AboutAccountsPage.prototype.getElements = function (aSpec) {
> +  var spec = aSpec || { };
> +  var root = spec.parent ? spec.parent.getNode()
> +                         : this.browserWindow.controller.window.document;

Keep in mind that this is chrome while the page is in content.

@@ +98,5 @@
> +  switch (spec.type) {
> +    case "getStartedButton":
> +      return [findElement.ID(root, "buttonGetStarted")];
> +    case "inputEmail":
> +      root = this.getElement({type: "remoteFrame"}).getNode();

Please cache this inside the constructor. There is no need to call this over and over again. You may also wanna have a look at the discovery pane class, how frames are handled there.

Comment 12

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Henrik Skupin (:whimboo) from comment #11)
> Please cache this inside the constructor. There is no need to call this over
> and over again. You may also wanna have a look at the discovery pane class,
> how frames are handled there.

I cannot cache this inside the constructor as the page might not be opened, I'll rather make an getter and cache it on the first call.
Regarding the discovery pane, what should I add? Keywords for elements, a waitForFrame method?

Comment 13

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Henrik Skupin (:whimboo) from comment #11)
> 
> > +  this.browserWindow.controller.window
> > +                    .addEventListener("DOMContentLoaded", contentLoaded);
> 
> So the page is in-content, why do you attach the listener to the chrome
> window?
in-content window is not opened at the time we add the listener, and if attach it latter the event might be already dispatched, so here it will bubble to the chrome

> @@ +92,5 @@
> > + */
> > +AboutAccountsPage.prototype.getElements = function (aSpec) {
> > +  var spec = aSpec || { };
> > +  var root = spec.parent ? spec.parent.getNode()
> > +                         : this.browserWindow.controller.window.document;
> 
> Keep in mind that this is chrome while the page is in content.
We have a contentWindow getter from the inherited class, I'll use that.

> > +      root = this.getElement({type: "remoteFrame"}).getNode();
> 
> Please cache this inside the constructor. There is no need to call this over
> and over again. You may also wanna have a look at the discovery pane class,
> how frames are handled there.
I cannot cache this inside the constructor as the page might not be opened, I'll rather make an getter and cache it on the first call.
Regarding the discovery pane, what should I add? Keywords for elements, a waitForFrame method?

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

(In reply to Cosmin Malutan from comment #12)
> I cannot cache this inside the constructor as the page might not be opened,
> I'll rather make an getter and cache it on the first call.

Sounds fine to me. 

> Regarding the discovery pane, what should I add? Keywords for elements, a
> waitForFrame method?

The handling of the frame, which should be identical to the about:accounts page, or?

(In reply to Cosmin Malutan from comment #13)
> > > +  this.browserWindow.controller.window
> > > +                    .addEventListener("DOMContentLoaded", contentLoaded);
> > 
> > So the page is in-content, why do you attach the listener to the chrome
> > window?
> in-content window is not opened at the time we add the listener, and if
> attach it latter the event might be already dispatched, so here it will
> bubble to the chrome

You would get lesser questions about such code if you would add comments to your code, which explain what you just did now. Try to do that by default for complex and unusual code.

Comment 15

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Henrik Skupin (:whimboo) from comment #14)
> > Regarding the discovery pane, what should I add? Keywords for elements, a
> > waitForFrame method?
> 
> The handling of the frame, which should be identical to the about:accounts
> page, or?
It's not, for discovery pane we have a <browser> element not an iFrame, it's little different, for the discovery pane we have events for the cases the pane changes, which we don't have for the iFrame in about:accounts 

> (In reply to Cosmin Malutan from comment #13)
> > > > +  this.browserWindow.controller.window
> > > > +                    .addEventListener("DOMContentLoaded", contentLoaded);
> > > 
> > > So the page is in-content, why do you attach the listener to the chrome
> > > window?
> > in-content window is not opened at the time we add the listener, and if
> > attach it latter the event might be already dispatched, so here it will
> > bubble to the chrome
> 
> You would get lesser questions about such code if you would add comments to
> your code, which explain what you just did now. Try to do that by default
> for complex and unusual code.
I'll add a comment.

Are we fine? Can I update the patch?

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

(In reply to Cosmin Malutan from comment #15)
> > The handling of the frame, which should be identical to the about:accounts
> > page, or?
> It's not, for discovery pane we have a <browser> element not an iFrame, it's
> little different, for the discovery pane we have events for the cases the
> pane changes, which we don't have for the iFrame in about:accounts 

Not exactly sure what you refer to with the pane for the discovery pane. When we talk about the latter it is really only the included remote content, but not the categories where viewChanged events are getting sent. That is outside of its scope. about:accounts would also need a browser given that remote web content is located there. So for loading other pages the waitForPageLoad should be used, and frames should be supported by mozmill automatically.

Comment 17

[Cosmin Malutan, [:cosmin-malutan]]

All right, so I think I understand what you want, to make a subclass for the remote window, and a getter method on the main class.
Right?

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

about:accounts is all about remote content, and which correlates exactly to the discovery pane. So it's class you already have. No need to create a new one, but its about using the correct windows for events and pageloads.

Comment 19

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v3.0

I fixed the nits, the only other thing that I had was regarding the frame loading. For this as Henrik suggested the I wait similar as we do for Discovery pane. Once loaded the frame changes only the href location, this doesn't triggers any other events so it was pointless to make a different method for that.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

Comment on attachment 8530132 [details] [diff] [review]
patch v3.0

Review of attachment 8530132 [details] [diff] [review]:
-----------------------------------------------------------------

That's much better! Thanks a lot for this change. With the remaining nit fixed you have my r+.

::: firefox/lib/ui/about-accounts-page.js
@@ +65,5 @@
> +  baseInContentPage.BaseInContentPage.prototype.open.call(this, callback);
> +
> +  // Wait for remote frame to load too.
> +  this.browserWindow.controller.waitForPageLoad(this.remoteFrameWindow.document,
> +                                                TIMEOUT_REMOTE);

Please remove the TIMEOUT_REMOTE variable and its usage. This value is already the default for waitForPageLoad.

Comment 21

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v4.0

Thanks for review Henrik, passing this to Andrei/Andreea.

Comment 22

[Andreea Matei [:AndreeaMatei]]

Landed on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/00e3016cc168 (default)

With the merge tonight we'll have this on aurora too, and since it's blocking in-content pref bug and that the search lib changes that we need in beta, we might want to check and have this down to beta as well.

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

(In reply to Andreea Matei [:AndreeaMatei] from comment #22)
> With the merge tonight we'll have this on aurora too, and since it's
> blocking in-content pref bug and that the search lib changes that we need in
> beta, we might want to check and have this down to beta as well.

I assume you mean for the next beta in terms of Firefox 35. Below there were no in-content preferences enabled by default.

Comment 24

[Cosmin Malutan, [:cosmin-malutan]]

This is for about:accounts, and it's enabled by default, I just downloaded the latest Beta, hit the "Sign in to Sync" button and it opened about:accounts page, then I ran the tests. All wen't well.
From my opinion we can have it back-ported down to beta.

Comment 25

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v4.0 beta

There is an NS_ERROR_FAILURE on OSX, so I had to update the patch to retrieve the 'signIn' element again  when I reuse it after a page change.

Comment 26

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8532526 [details] [diff] [review]
patch v4.0 beta

Review of attachment 8532526 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox/lib/tests/testAboutAccountsPage.js
@@ +34,5 @@
> +  // "Sign Up" link should be displayed when we clicked on "Sign In" link
> +  var signUp = aboutAccounts.getElement({type: "signUp"});
> +  signUp.waitThenClick();
> +
> +  var signIn = aboutAccounts.getElement({type: "signIn"});

Please remove var here and add this small fix to default and aurora as well. Thanks.

Comment 27

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v4.1 beta

Beta patch.

Comment 28

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: follow-up patch default

Follow up patch for default.

Comment 29

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8533050 [details] [diff] [review]
follow-up patch default

Review of attachment 8533050 [details] [diff] [review]:
-----------------------------------------------------------------

http://hg.mozilla.org/qa/mozmill-tests/rev/b6a0f73cd8a4 (default)

Comment 30

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8533049 [details] [diff] [review]
patch v4.1 beta

Review of attachment 8533049 [details] [diff] [review]:
-----------------------------------------------------------------

http://hg.mozilla.org/qa/mozmill-tests/rev/2df32e4ba402 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/844c356b1125 (beta)
Thanks!