Last Comment Bug 752841 - [New Tab Page] make the number of rows and columns adjustable
: [New Tab Page] make the number of rows and columns adjustable
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal with 9 votes (vote)
: Firefox 17
Assigned To: Bellindira Castillo [:bellindira]
:
Mentors:
: 854087 (view as bug list)
Depends on:
Blocks: 455553
  Show dependency treegraph
 
Reported: 2012-05-08 02:32 PDT by Sean Newman
Modified: 2013-03-23 06:15 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (7.25 KB, patch)
2012-05-24 17:23 PDT, Bellindira Castillo [:bellindira]
ttaubert: review-
Details | Diff | Splinter Review
Patch v2 (9.49 KB, patch)
2012-05-31 17:16 PDT, Bellindira Castillo [:bellindira]
no flags Details | Diff | Splinter Review
Patch v3. Applied fixes. (10.82 KB, patch)
2012-07-16 23:18 PDT, Bellindira Castillo [:bellindira]
ttaubert: feedback+
Details | Diff | Splinter Review
Patch and test case (13.27 KB, patch)
2012-08-07 11:10 PDT, Bellindira Castillo [:bellindira]
ttaubert: review+
Details | Diff | Splinter Review
Patch and test case (13.00 KB, patch)
2012-08-16 22:02 PDT, Bellindira Castillo [:bellindira]
ttaubert: review+
Details | Diff | Splinter Review

Description Sean Newman 2012-05-08 02:32:02 PDT
There should be prefs, so users could specify the number of columns and rows of tabs shown on the about:newtab page.
Comment 1 Bellindira Castillo [:bellindira] 2012-05-24 17:23:48 PDT
Created attachment 627041 [details] [diff] [review]
Patch v1
Comment 2 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-05-29 06:19:10 PDT
Comment on attachment 627041 [details] [diff] [review]
Patch v1

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

::: browser/base/content/newtab/grid.js
@@ +50,5 @@
>  
>    /**
>     * All cells contained in the grid.
>     */
>    get cells() {

I think Grid.cells should stay a lazy getter but we need to introduce a _cells property so that it doesn't need to overwrite itself on first access.

@@ +135,5 @@
>    /**
> +   * Creates the newtab grid according to PREF_NEWTAB_ROWS and
> +   * PREF_NEWTAB_COLUMNS.
> +   */
> +  _createGrid: function Grid_createGrid() {

Let's name this _renderGrid(). While you're at it please rename _draw() to _render(). This function should be called at the beginning of _render() and should only proceed if there aren't any rows/cells yet or the number of rows/cells differs from the actual preference values.

@@ +220,5 @@
> +   * Adds a preference observer
> +   */
> +  _addObserver: function Grid_addObserver() {
> +    Services.prefs.addObserver(PREF_NEWTAB_ROWS, this, true);
> +    Services.prefs.addObserver(PREF_NEWTAB_COLUMNS, this, true);

Every opened newtab page would add itself as an observer. We should better implement the observer in NewTabUtils.jsm. If a pref changes this will just call:

> AllPages.update();
Comment 3 Bellindira Castillo [:bellindira] 2012-05-31 17:16:10 PDT
Created attachment 628988 [details] [diff] [review]
Patch v2

Implemented the commented fixes.
Comment 4 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-06-23 09:04:01 PDT
Comment on attachment 628988 [details] [diff] [review]
Patch v2

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

Thanks, Bellindira! This looks better but we should centralize all the code accessing and watching the prefs in the JSM. We can also simplify the rendering part a little like I described below.

::: browser/base/content/newtab/grid.js
@@ +45,5 @@
> +
> +  /**
> +   * Updates the grid cells and caches its value
> +   */
> +  _updateCells: function Grid_updateCells() {

We shouldn't add another method for this. Just do it in the getter like this:

> get cells() {
>   if (this._cells)
>     return this._cells;
>   
>   /* ... create the cells ... */
>   return this._cells = cells;
> }

@@ +130,5 @@
> +    //creates the structure of one row
> +    for (let j = 0; j < this.gridColumns; j++) {
> +      row.appendChild(column.cloneNode(true));
> +    }
> +    //creates the grid

Please write comments with a space after '//' and with proper capitalization and punctuation.

> // Creates the grid.

@@ +133,5 @@
> +    }
> +    //creates the grid
> +    for (let i = 0; i < this.gridRows; i++) {
> +      this._node.appendChild(row.cloneNode(true));
> +    }

We need to set |this._cells = null| here, so that the cells are collected again when accessing |this.cells| the next time.

@@ +165,4 @@
>     */
> +  _render: function Grid_render() {
> +    let rowNumber = Services.prefs.getIntPref(PREF_NEWTAB_ROWS);
> +    let colNumber = Services.prefs.getIntPref(PREF_NEWTAB_COLUMNS);

These values should be provided by the GridPrefs object. They should be lazily read and updated in the JSM.

@@ +173,5 @@
> +      this._gridRows = rowNumber;
> +      this._gridColumns = colNumber;
> +      this._renderGrid();
> +      this._updateCells();
> +    }

Instead of all this just call this._renderGrid() here. The page is only updated/refreshed when about:newtab instances are synced.

@@ +179,1 @@
>      let cells = this.cells;

Please make this and the following code an extra method called "_renderSites" and call it from "_render".
Comment 5 Bellindira Castillo [:bellindira] 2012-07-16 23:18:10 PDT
Created attachment 642870 [details] [diff] [review]
Patch v3. Applied fixes.
Comment 6 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-07-24 03:24:26 PDT
Comment on attachment 642870 [details] [diff] [review]
Patch v3. Applied fixes.

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

Thank you, Bellindira! This looks pretty good but we still need to clean this up a little.

Also, we need a test for this feature. Can you please add a test that modifies the rows/columns prefs and check if new and existing pages are updated accordingly? We should add tests for invalid values (like 0/-1) as well. Thanks!

::: browser/base/content/newtab/grid.js
@@ +17,5 @@
>    /**
> +   * Number of rows on the grid
> +   */
> +  _gridRows: null,
> +  get gridRows() this._gridRows,

We don't need this anymore...

@@ +23,5 @@
> +  /**
> +   * Number of columns on the grid.
> +   */
> +  _gridColumns: null,
> +  get gridColumns() this._gridColumns,

... and this, too.

@@ +33,5 @@
>  
>    /**
>     * All cells contained in the grid.
>     */
> +  _cells : null,

Nit: please remove the space before ":".

@@ +38,3 @@
>    get cells() {
> +    if (this._cells)
> +      return this._cells;

I think we should turn this into:

> get cells() this._cells,

And remove the other code that queries the DOM for .newtab-cell elements. "_renderGrid" should keep track of all cells it creates and then assign them to this._cells.

@@ +113,5 @@
> +   * and "browser.newtabpage.columns"
> +   */
> +  _renderGrid: function Grid_renderGrid() {
> +    let row = document.createElementNS(HTML_NAMESPACE, "div");
> +    let column = document.createElementNS(HTML_NAMESPACE, "div");

This variable should be named "cell".

@@ +117,5 @@
> +    let column = document.createElementNS(HTML_NAMESPACE, "div");
> +    row.classList.add("newtab-row");
> +    column.classList.add("newtab-cell");
> +    this._gridRows = gGridPrefs.gridRows;
> +    this._gridColumns = gGridPrefs.gridColumns;

Those two should just be local variables, no need to save them as properties.

@@ +131,5 @@
> +    for (let i = 0; i < this.gridRows; i++) {
> +      this._node.appendChild(row.cloneNode(true));
> +    }
> +    //Re-initialize the cells.
> +    this._cells = null;

As noted above, this should be:

> // (Re-)initialize all cells.
> cellElements = this._node.querySelectorAll(".newtab-cell");
> this._cells = [new Cell(this, cell) for (cell of cellElements)];

@@ +180,5 @@
> +    if (this._cells === null ||
> +        this.gridRows != gGridPrefs.gridRows ||
> +        this.gridColumns != gGridPrefs.gridColumns) {
> +      this._renderGrid();
> +    }

We should add another method that tells whether we need to render the grid. This should do the following:

1) Determine the number of rows currently in the grid by getting this._node.querySelectorAll(".newtab-row").length. If that's not equal to gGridPrefs.gridRows, return false.

2) Determine the number of cells currently in the grid by getting this._node.querySelectorAll(".newtab-cell").length. If that's not equal to (gGridPrefs.gridRows * gGridPrefs.gridColumns), return false.

3) return true.

The code should then roughly look like:

> _render: function Grid_render() {
>   if (this._gridNeedsRendering()) {
>     this._renderGrid();
>   }
>   this._renderSites();
> }

::: browser/base/content/newtab/newTab.js
@@ +19,5 @@
>    links: gLinks,
>    allPages: gAllPages,
>    pinnedLinks: gPinnedLinks,
> +  blockedLinks: gBlockedLinks,
> +  gridPrefs : gGridPrefs

Nit: please remove the space before ":".

::: browser/modules/NewTabUtils.jsm
@@ +18,5 @@
>  
>  // The preference that tells whether this feature is enabled.
>  const PREF_NEWTAB_ENABLED = "browser.newtabpage.enabled";
>  
> +// The preference that tells the number of rows of newtab grid.

Nit: of *the* newtab grid.

@@ +21,5 @@
>  
> +// The preference that tells the number of rows of newtab grid.
> +const PREF_NEWTAB_ROWS = "browser.newtabpage.rows";
> +
> +// The preference that tells the number of columns of newtab grid.

Nit: of *the* newtab grid.

@@ +196,5 @@
> +   */
> +  _gridRows: null,
> +  get gridRows() {
> +    if (this._gridRows === null)
> +      this._gridRows = Services.prefs.getIntPref(PREF_NEWTAB_ROWS);

We need to make sure the given value is >= 1.

> if (!this._gridRows)
>   this._gridRows = Math.max(1, Services.prefs.getIntPref(PREF_NEWTAB_ROWS));

@@ +207,5 @@
> +   */
> +  _gridColumns: null,
> +  get gridColumns() {
> +    if (this._gridColumns === null)
> +      this._gridColumns = Services.prefs.getIntPref(PREF_NEWTAB_COLUMNS);

Same here.

> if (!this._gridColumns)
>   this._gridColumns = Math.max(1, Services.prefs.getIntPref(PREF_NEWTAB_COLUMNS));

@@ +226,5 @@
> +   * Implements the nsIObserver interface to get notified when the preference
> +   * value changes.
> +   */
> +  observe: function GridPrefs_observe(aSubject, aTopic, aData) {
> +    if (aTopic == "nsPref:changed") {

We don't need to check the topic, it's always "nsPref:changed" in our case.

@@ +229,5 @@
> +  observe: function GridPrefs_observe(aSubject, aTopic, aData) {
> +    if (aTopic == "nsPref:changed") {
> +      switch (aData) {
> +        case PREF_NEWTAB_ROWS:
> +          this._gridRows = Services.prefs.getIntPref(PREF_NEWTAB_ROWS);

Using a switch statement for those two cases seems a bit overkill and we also need to enforce minimum values here again. It's easier to do:

> if (aData == PREF_NEWTAB_ROWS) {
>   this._gridRows = null;
> } else {
>   this._gridColumns = null;
> }
Comment 7 Bellindira Castillo [:bellindira] 2012-08-07 11:10:07 PDT
Created attachment 649710 [details] [diff] [review]
Patch and test case

Here is a new patch which fixes the nits. Also, I added the test case required.
Comment 8 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-08-09 02:57:14 PDT
Comment on attachment 649710 [details] [diff] [review]
Patch and test case

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

Thank you, Bellindira. That looks great. r=me with the small fixes mentioned below.

::: browser/base/content/newtab/grid.js
@@ +84,5 @@
>    },
>  
>    /**
> +   * Creates the newtab grid according to preferences "browser.newtabpage.rows"
> +   * and "browser.newtabpage.columns"

Nit: just "Creates the newtab grid." So we don't have to update the comment when the prefs change or the like.

@@ +108,5 @@
> +    // (Re-)initialize all cells.
> +    this._cells = [];
> +    let cellElements = this.node.querySelectorAll(".newtab-cell");
> +    for (let k = 0; k < cellElements.length; k++)
> +      this._cells.push(new Cell(this, cellElements[k]));

Better:

> // (Re-)initialize all cells.
> let cellElements = this.node.querySelectorAll(".newtab-cell");
> this._cells = [new Cell(this, cell) for (cell of cellElements)];

@@ +154,5 @@
> +   * Renders the grid.
> +   */
> +  _render: function Grid_render() {
> +    if (this._shouldRenderGrid())
> +      this._renderGrid();

Nit: please add braces here.

@@ +164,5 @@
> +    let rowsLength = this._node.querySelectorAll(".newtab-row").length;
> +    let cellsLength = this._node.querySelectorAll(".newtab-cell").length;
> +
> +    if (rowsLength == gGridPrefs.gridRows &&
> +        cellsLength == (gGridPrefs.gridRows * gGridPrefs.gridColumns))

You can just do:

return (rowsLength != gGridPrefs.gridRows ||
        cellsLength != (gGridPrefs.gridRows * gGridPrefs.gridColumns));

::: browser/modules/NewTabUtils.jsm
@@ +198,5 @@
> +   */
> +  _gridRows: null,
> +  get gridRows() {
> +    if (!this._gridRows)
> +      this._gridRows = Math.max(1, Services.prefs.getIntPref(PREF_NEWTAB_ROWS));

Nit: please add braces here.

@@ +209,5 @@
> +   */
> +  _gridColumns: null,
> +  get gridColumns() {
> +    if (!this._gridColumns)
> +      this._gridColumns = Math.max(1, Services.prefs.getIntPref(PREF_NEWTAB_COLUMNS));

Nit: please add braces here.

@@ +220,5 @@
> +   * Initializes object. Adds a preference observer
> +   */
> +  init: function GridPrefs_init() {
> +    Services.prefs.addObserver(PREF_NEWTAB_ROWS, this, true);
> +    Services.prefs.addObserver(PREF_NEWTAB_COLUMNS, this, true);

Please pass "false" as the third argument. We don't actually want the observer to be a weak reference.

@@ +231,5 @@
> +  observe: function GridPrefs_observe(aSubject, aTopic, aData) {
> +    if (aData == PREF_NEWTAB_ROWS)
> +      this._gridRows = null;
> +    else
> +      this._gridColumns = null;

Nit: please add braces here.

@@ +237,5 @@
> +    AllPages.update();
> +  },
> +
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver,
> +                                         Ci.nsISupportsWeakReference])

We don't need that QI definition. You can just remove it.
Comment 9 Bellindira Castillo [:bellindira] 2012-08-16 22:02:55 PDT
Created attachment 652672 [details] [diff] [review]
Patch and test case

Fixed nits.
Comment 10 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-08-17 12:56:04 PDT
Comment on attachment 652672 [details] [diff] [review]
Patch and test case

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

Thank you Bellindira! I'll land it right away.
Comment 11 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-08-17 13:03:57 PDT
https://hg.mozilla.org/integration/fx-team/rev/257a59b45897
Comment 12 Geoff Lankow (:darktrojan) 2012-08-18 02:49:38 PDT
Doesn't the preloaded new tab page need to be recreated when these prefs change?
Comment 13 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-08-18 09:32:52 PDT
(In reply to Geoff Lankow (:darktrojan) from comment #12)
> Doesn't the preloaded new tab page need to be recreated when these prefs
> change?

Nope, magic! All active pages are updated by the JSM when one of the prefs changes. This includes the preloaded/hidden one as well.
Comment 14 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-08-20 09:11:56 PDT
https://hg.mozilla.org/mozilla-central/rev/257a59b45897
Comment 15 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2013-03-23 06:15:35 PDT
*** Bug 854087 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.