1117745 - Add about:newtab in-content page class

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Teodor Druta]

Add a new 'about:newtab' base in content page class.

Comment 1

[Teodor Druta]

Attachment: aboutnewtab.patch

This patch includes the new about:newtab in content page class, a new lib test and some minor additions/improvements to the base-in-content-page lib.

Comment 2

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8548209 [details] [diff] [review]
aboutnewtab.patch

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

Please request from Henrik next. Thanks.

::: firefox/lib/tests/testAboutNewtabPage.js
@@ +16,5 @@
> +function teardownModule(aModule) {
> +  aModule.browserWindow.tabs.closeAllTabs();
> +}
> +
> +function testAboutNewTab() {

Please add jsdoc to this.

@@ +17,5 @@
> +  aModule.browserWindow.tabs.closeAllTabs();
> +}
> +
> +function testAboutNewTab() {
> +  aboutNewTab.open({method: "menu"});

I see 'menu' is the default option, no need to mention it.

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

This are separate, one is ui.

@@ +31,5 @@
> + *         Callback to open the "What is this page?" panel
> + *
> + * @returns {object} "What is this page?" panel
> + */
> +AboutNewtabPage.prototype.openWhatIsThisPage = function AboutNewtabPage_openWhatIsThisPage (aCallback) {

Could we use some acronym for this, like ANTP_openWhatIsThisPage?

@@ +50,5 @@
> +  this._browserWindow.controller.window.addEventListener("DOMAttrModified", onDOMTree);
> +  this._browserWindow.controller.window.addEventListener("popupshowing", onPopupShow);
> +  try {
> +    callback();
> +    assert.waitFor(() => (flagPopupShow && flagDomTree), "'What is this page?' popup is open");

New line for the message, goes over 80chars.

@@ +86,5 @@
> +        break;
> +      case "callback":
> +        assert.equal(typeof aSpec.callback, "function",
> +                     "Callback has been defined");
> +        aSpec.callback();

Please sort them alphabetically.

@@ +122,5 @@
> +    case "grid":
> +      return [findElement.ID(root, "newtab-grid")];
> +    case "introPanel":
> +      return [findElement.ID(root, "newtab-intro-panel")];
> +    case "whatIsThisPage":

Please move this at the bottom to keep them sorted.

Comment 3

[Teodor Druta]

Attachment: aboutnewtabv2.patch

Fixed all reported nits.

Comment 4

[Teodor Druta]

Attachment: aboutnewtabv2.patch

Just a small change regarding the "browser.newtab.preload" property, setting it to false.
https://bugzilla.mozilla.org/show_bug.cgi?id=1120906#c4

Comment 5

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

Comment on attachment 8548921 [details] [diff] [review]
aboutnewtabv2.patch

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

::: firefox/lib/tests/testAboutNewtabPage.js
@@ +24,5 @@
> + * Opens a new tab using the default method, menu entry
> + */
> +function testAboutNewTab() {
> +  aboutNewTab.open();
> +  assert.ok(aboutNewTab.isOpen, "Tab with the in-content page has been opened");

I don't see any checks for defined elements here. This is a requirement as for all the other ui modules.

::: firefox/lib/ui/about-newtab-page.js
@@ +22,5 @@
> +function AboutNewtabPage(aBrowserWindow) {
> +  assert.ok(aBrowserWindow, "Browser window has been specified");
> +
> +  this._browserWindow = aBrowserWindow;
> +  this._contentWindow = null;

Both are set by the BaseInContentPage constructor which you are not calling here.

@@ +26,5 @@
> +  this._contentWindow = null;
> +
> +  // TODO: Bug 1120906
> +  // Set the "browser.newtab.preload" property to false
> +  prefs.setPref(PREF_NEWTAB_PRELOAD, false);

This needs a better comment. Right now it just duplicates the code. I don't see why it has to be done.

@@ +40,5 @@
> + *         Callback to open the "What is this page?" panel
> + *
> + * @return {object} "What is this page?" panel
> + */
> +AboutNewtabPage.prototype.openWhatIsThisPage = function ANTP_openWhatIsThisPage (aCallback) {

nit: no blank after a function name and the brackets

@@ +44,5 @@
> +AboutNewtabPage.prototype.openWhatIsThisPage = function ANTP_openWhatIsThisPage (aCallback) {
> +  // Click on "What is this page?" element
> +  var self = this;
> +  function openIntro() {
> +    var link = self.getElement({type: "whatIsThisPage"});

Use a fat arrow method here so you do not need self.

@@ +51,5 @@
> +
> +  var callback = aCallback || openIntro;
> +
> +  var flagPopupShow = false;
> +  var flagDomTree = false;

I would call that attrModified and popupShown.

@@ +96,5 @@
> +        this.browserWindow.controller.mainMenu.click("#menu_newNavigatorTab");
> +        break;
> +      case "shortcut":
> +        var cmdKey = this._browserWindow.getEntity("tabCmd.accesskey");
> +        this._browserWindow.controller.keypress(null, cmdKey, {accelKey: true});

Use this.browserWindow in both cases.

@@ +105,5 @@
> +  }
> +
> +  baseInContentPage.BaseInContentPage
> +                   .prototype.open.call(this, callback,
> +                                        this.browserWindow.controller.window.BROWSER_NEW_TAB_URL);

The open() method only accepts a callback but no URL as second parameter.

@@ +135,5 @@
> +      return [findElement.ID(root, "newtab-intro-panel")];
> +    case "learnMore":
> +      return [findElement.Selector(root, "#newtab-intro-panel > p:first-of-type a")];
> +    case "privacyNotice":
> +      return [findElement.Selector(root, "#newtab-intro-panel > p:nth-of-type(2) a")];

Both need an introPanel prefix

@@ +138,5 @@
> +    case "privacyNotice":
> +      return [findElement.Selector(root, "#newtab-intro-panel > p:nth-of-type(2) a")];
> +    case "sponsored":
> +      var newTabGrid = this.getElement({type: "grid"});
> +      nodeCollector.root = newTabGrid.getNode();

Why two lines and not `nodeCollector.root = this.getElement({type: "grid"}).getNode()`

@@ +143,5 @@
> +      nodeCollector.queryNodes(".newtab-sponsored")
> +                   .filterByCSSProperty("display", "block");
> +      return [nodeCollector.elements];
> +    case "sponsoredExplained":
> +      nodeCollector.queryNodes(".sponsored-explain");

You don't need the grid as root here?

@@ +147,5 @@
> +      nodeCollector.queryNodes(".sponsored-explain");
> +      return [nodeCollector.elements];
> +    case "sponsoredLearnMore":
> +      var sponsoredExplained = this.getElement({type: "sponsoredExplained"});
> +      nodeCollector.root = sponsoredExplained.getNode();

Same as above.

::: firefox/lib/ui/base-in-content-page.js
@@ +81,5 @@
>     *
>     * @returns {string} URL of the page
>     */
>    get url() {
> +    return this.contentWindow ? this.contentWindow.location.href : false;

false is not a URL. If there are cases we do not have a contentWindow it should be null.

Comment 6

[Teodor Druta]

Attachment: aboutnewtabv3.patch

(In reply to Henrik Skupin (:whimboo) from comment #5)
> 
> ::: firefox/lib/tests/testAboutNewtabPage.js
> @@ +24,5 @@
> > + * Opens a new tab using the default method, menu entry
> > + */
> > +function testAboutNewTab() {
> > +  aboutNewTab.open();
> > +  assert.ok(aboutNewTab.isOpen, "Tab with the in-content page has been opened");
> 
> I don't see any checks for defined elements here. This is a requirement as
> for all the other ui modules.
> 

Added checks for main page elements.

> ::: firefox/lib/ui/about-newtab-page.js
> @@ +22,5 @@
> > +function AboutNewtabPage(aBrowserWindow) {
> > +  assert.ok(aBrowserWindow, "Browser window has been specified");
> > +
> > +  this._browserWindow = aBrowserWindow;
> > +  this._contentWindow = null;
> 
> Both are set by the BaseInContentPage constructor which you are not calling
> here.
> 

Removed these lines, added a call to the BaseInContentPage constructor.

> @@ +26,5 @@
> > +  this._contentWindow = null;
> > +
> > +  // TODO: Bug 1120906
> > +  // Set the "browser.newtab.preload" property to false
> > +  prefs.setPref(PREF_NEWTAB_PRELOAD, false);
> 
> This needs a better comment. Right now it just duplicates the code. I don't
> see why it has to be done.
> 

Updated to a more suggestive comment.

> @@ +40,5 @@
> > + *         Callback to open the "What is this page?" panel
> > + *
> > + * @return {object} "What is this page?" panel
> > + */
> > +AboutNewtabPage.prototype.openWhatIsThisPage = function ANTP_openWhatIsThisPage (aCallback) {
> 
> nit: no blank after a function name and the brackets
> 

Fixed here and in the other two class methods.

> @@ +44,5 @@
> > +AboutNewtabPage.prototype.openWhatIsThisPage = function ANTP_openWhatIsThisPage (aCallback) {
> > +  // Click on "What is this page?" element
> > +  var self = this;
> > +  function openIntro() {
> > +    var link = self.getElement({type: "whatIsThisPage"});
> 
> Use a fat arrow method here so you do not need self.
> 

Updated to a fat arrow.

> @@ +51,5 @@
> > +
> > +  var callback = aCallback || openIntro;
> > +
> > +  var flagPopupShow = false;
> > +  var flagDomTree = false;
> 
> I would call that attrModified and popupShown.
> 

Modified.

> @@ +96,5 @@
> > +        this.browserWindow.controller.mainMenu.click("#menu_newNavigatorTab");
> > +        break;
> > +      case "shortcut":
> > +        var cmdKey = this._browserWindow.getEntity("tabCmd.accesskey");
> > +        this._browserWindow.controller.keypress(null, cmdKey, {accelKey: true});
> 
> Use this.browserWindow in both cases.
> 

Fixed everywhere it applied.

> @@ +105,5 @@
> > +  }
> > +
> > +  baseInContentPage.BaseInContentPage
> > +                   .prototype.open.call(this, callback,
> > +                                        this.browserWindow.controller.window.BROWSER_NEW_TAB_URL);
> 
> The open() method only accepts a callback but no URL as second parameter.
> 

Removed the second parameter.

> @@ +135,5 @@
> > +      return [findElement.ID(root, "newtab-intro-panel")];
> > +    case "learnMore":
> > +      return [findElement.Selector(root, "#newtab-intro-panel > p:first-of-type a")];
> > +    case "privacyNotice":
> > +      return [findElement.Selector(root, "#newtab-intro-panel > p:nth-of-type(2) a")];
> 
> Both need an introPanel prefix
> 

Added a prefix.

> @@ +138,5 @@
> > +    case "privacyNotice":
> > +      return [findElement.Selector(root, "#newtab-intro-panel > p:nth-of-type(2) a")];
> > +    case "sponsored":
> > +      var newTabGrid = this.getElement({type: "grid"});
> > +      nodeCollector.root = newTabGrid.getNode();
> 
> Why two lines and not `nodeCollector.root = this.getElement({type:
> "grid"}).getNode()`
> 

Modified everywhere it applied.

> @@ +143,5 @@
> > +      nodeCollector.queryNodes(".newtab-sponsored")
> > +                   .filterByCSSProperty("display", "block");
> > +      return [nodeCollector.elements];
> > +    case "sponsoredExplained":
> > +      nodeCollector.queryNodes(".sponsored-explain");
> 
> You don't need the grid as root here?
> 

Yes, fixed that.

> @@ +147,5 @@
> > +      nodeCollector.queryNodes(".sponsored-explain");
> > +      return [nodeCollector.elements];
> > +    case "sponsoredLearnMore":
> > +      var sponsoredExplained = this.getElement({type: "sponsoredExplained"});
> > +      nodeCollector.root = sponsoredExplained.getNode();
> 
> Same as above.
> 

Here the root is the sponsoredExplained element declared above.

> ::: firefox/lib/ui/base-in-content-page.js
> @@ +81,5 @@
> >     *
> >     * @returns {string} URL of the page
> >     */
> >    get url() {
> > +    return this.contentWindow ? this.contentWindow.location.href : false;
> 
> false is not a URL. If there are cases we do not have a contentWindow it
> should be null.

Fixed.

Comment 7

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8549680 [details] [diff] [review]
aboutnewtabv3.patch

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

I see all the changes implemented.

Comment 8

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

Comment on attachment 8549680 [details] [diff] [review]
aboutnewtabv3.patch

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

::: firefox/lib/tests/testAboutNewtabPage.js
@@ +10,5 @@
> +
> +const TEST_DATA = [
> +  {name: "grid", localName: "div"},
> +  {name: "introPanel", localName: "panel"},
> +  {name: "whatIsThisPage", localName: "div"}

Those are not all the elements. Don't we have to test the others?

::: firefox/lib/ui/about-newtab-page.js
@@ +24,5 @@
> +
> +  // TODO: Bug 1120906
> +  // waitForPageLoad fails after opening "about:newtab" when
> +  // "browser.newtab.preload" property is set to the "true" value
> +  prefs.setPref(PREF_NEWTAB_PRELOAD, false);

We actually never reset this preference. I think a better place would be in open(), so we can reset the preference right after the tab got opened and the page loaded.

@@ +127,5 @@
> +    case "grid":
> +      return [findElement.ID(root, "newtab-grid")];
> +    case "introPanel":
> +      return [findElement.ID(root, "newtab-intro-panel")];
> +    case "introPanelLearnMore":

To be in sync with the other libs this needs to be `introPanel_learnMore`. Same for the other elements in that panel.

Comment 9

[Teodor Druta]

Attachment: aboutnewtabv4.patch

(In reply to Henrik Skupin (:whimboo) from comment #8)
> Comment on attachment 8549680 [details] [diff] [review]
> aboutnewtabv3.patch
> 
> Review of attachment 8549680 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: firefox/lib/tests/testAboutNewtabPage.js
> @@ +10,5 @@
> > +
> > +const TEST_DATA = [
> > +  {name: "grid", localName: "div"},
> > +  {name: "introPanel", localName: "panel"},
> > +  {name: "whatIsThisPage", localName: "div"}
> 
> Those are not all the elements. Don't we have to test the others?
> 

Removed the sponsored* elements from the getElements, there is no Sponsored tile in the latest nightly, I'll add any sponsored related elements in Bug 1110187, that is if we will still do that.

> ::: firefox/lib/ui/about-newtab-page.js
> @@ +24,5 @@
> > +
> > +  // TODO: Bug 1120906
> > +  // waitForPageLoad fails after opening "about:newtab" when
> > +  // "browser.newtab.preload" property is set to the "true" value
> > +  prefs.setPref(PREF_NEWTAB_PRELOAD, false);
> 
> We actually never reset this preference. I think a better place would be in
> open(), so we can reset the preference right after the tab got opened and
> the page loaded.
> 

Moved this to AboutNewtabPage pen() Method, the preference is cleared now in bicp open.

> @@ +127,5 @@
> > +    case "grid":
> > +      return [findElement.ID(root, "newtab-grid")];
> > +    case "introPanel":
> > +      return [findElement.ID(root, "newtab-intro-panel")];
> > +    case "introPanelLearnMore":
> 
> To be in sync with the other libs this needs to be `introPanel_learnMore`.
> Same for the other elements in that panel.

Fixed.

Comment 10

[Mihaela Velimiroviciu (:mihaelav)]

Comment on attachment 8552516 [details] [diff] [review]
aboutnewtabv4.patch

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

Looks fine to me

Comment 11

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8552516 [details] [diff] [review]
aboutnewtabv4.patch

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

Just a comment and you can ask from Henrik. Thanks!

::: firefox/lib/ui/base-in-content-page.js
@@ +185,5 @@
>  
> +    // TODO: Bug 1120906
> +    // waitForPageLoad fails after opening "about:newtab" when
> +    // "browser.newtab.preload" property is set to the "true" value
> +    prefs.clearUserPref(PREF_NEWTAB_PRELOAD);

I would have another comment here as this is for clearing it and mention where we set this pref.

Comment 12

[Teodor Druta]

Attachment: aboutnewtabv4.1.patch

(In reply to Andreea Matei [:AndreeaMatei] from comment #11)
> Comment on attachment 8552516 [details] [diff] [review]
> aboutnewtabv4.patch
> 
> Review of attachment 8552516 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just a comment and you can ask from Henrik. Thanks!
> 
> ::: firefox/lib/ui/base-in-content-page.js
> @@ +185,5 @@
> >  
> > +    // TODO: Bug 1120906
> > +    // waitForPageLoad fails after opening "about:newtab" when
> > +    // "browser.newtab.preload" property is set to the "true" value
> > +    prefs.clearUserPref(PREF_NEWTAB_PRELOAD);
> 
> I would have another comment here as this is for clearing it and mention
> where we set this pref.

I think that we should just leave a TODO reference to the bug, that way we will know that, this needs to be removed, once it is fixed in mozmill.

Comment 13

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

Comment on attachment 8553150 [details] [diff] [review]
aboutnewtabv4.1.patch

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

Only one remaining items left, so please fix it and re-request review. Thanks.

::: firefox/lib/ui/base-in-content-page.js
@@ +183,5 @@
>        this.browserWindow.controller.waitForPageLoad();
>      }
>  
> +    // TODO: Bug 1120906
> +    prefs.clearUserPref(PREF_NEWTAB_PRELOAD);

Why is this not done in the about new tab class? You should not add those cross-behaviors.

Comment 14

[Teodor Druta]

Attachment: aboutnewtabv5.patch

(In reply to Henrik Skupin (:whimboo) from comment #13)
> ::: firefox/lib/ui/base-in-content-page.js
> @@ +183,5 @@
> >        this.browserWindow.controller.waitForPageLoad();
> >      }
> >  
> > +    // TODO: Bug 1120906
> > +    prefs.clearUserPref(PREF_NEWTAB_PRELOAD);
> 
> Why is this not done in the about new tab class? You should not add those
> cross-behaviors.

Yes, you're right, the preference can be cleared at the end of the open() method of the AboutNewtab class right after the call to the open() method of the BICP class.
Fixed.

Comment 15

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

Comment on attachment 8555719 [details] [diff] [review]
aboutnewtabv5.patch

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

::: firefox/lib/ui/about-newtab-page.js
@@ +103,5 @@
> +
> +  baseInContentPage.BaseInContentPage.prototype.open.call(this, callback);
> +
> +  // TODO: Bug 1120906
> +  prefs.clearUserPref(PREF_NEWTAB_PRELOAD);

This will miss to reset the pref in case of an exception earlier in this method. You want a try/finally around line 104.

Comment 16

[Teodor Druta]

Attachment: aboutnewtabv6.patch

Wraped the BICP open() call in a try {} -> the preferences is cleared in finally {}.

Comment 17

[Mihaela Velimiroviciu (:mihaelav)]

Comment on attachment 8559137 [details] [diff] [review]
aboutnewtabv6.patch

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

Just a few nits, otherwise it looks good.

::: firefox/lib/tests/testAboutNewtabPage.js
@@ +26,5 @@
> +}
> +
> +/**
> + * Bug 1117745
> + * Add 'about:newtab' in-content page class

This comment is not necessary

@@ +28,5 @@
> +/**
> + * Bug 1117745
> + * Add 'about:newtab' in-content page class
> + *
> + * Opens a new tab using the default method, menu entry

"Verify the about:newtab page content"

::: firefox/lib/ui/about-newtab-page.js
@@ +114,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=window.document]

Please move this optional parameter after aSpec.type

::: firefox/lib/ui/base-in-content-page.js
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> +var prefs = require("../../../lib/prefs");

You don't use this

Comment 18

[Teodor Druta]

Attachment: aboutnewtabv6.1.patch

(In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #17)
> ::: firefox/lib/ui/about-newtab-page.js
> @@ +114,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=window.document]
> 
> Please move this optional parameter after aSpec.type
> 

Actually I think that we group the parameters in the case of aSpec object alphabetically and in groups by type, so in this case we have "aSpec" which is an object followed by "aSpec.parent" which is also an object and is alphabetically ordered the second, the third parameter will be "aSpec.type" which is a string.

All the rest nits have been fixed.

Comment 19

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

Comment on attachment 8559206 [details] [diff] [review]
aboutnewtabv6.1.patch

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

Lets get it in! Thanks Teodor!

Comment 20

[Andreea Matei [:AndreeaMatei]]

https://hg.mozilla.org/qa/mozmill-tests/rev/73636abdb8c2 (default)

Comment 21

[Teodor Druta]

I think that we should leave this landed only on default, because it's a new lib.