There should be prefs, so users could specify the number of columns and rows of tabs shown on the about:newtab page.

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();

Implemented the commented fixes.

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 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; > }

Here is a new patch which fixes the nits. Also, I added the test case required.

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.

Fixed nits.

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.

Doesn't the preloaded new tab page need to be recreated when these prefs change?

(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.