977814 - Add a pref to control total number of top site tiles

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: Firefox Start, defect, P1)

Description

[Jim Mathies [:jimm]]

The "we love our tiles!" bug.

Comment 1

[Jim Mathies [:jimm]]

Attachment: patch

I like having 16 vs. 8.

Comment 2

[Jim Mathies [:jimm]]

Attachment: patch v.2

I split the prefs out per compartment. Also removed an old fennec pref we had that we weren't using. Made sure a value of 0 for any grid hides that grid.

We had a discussion about this on irc, four different users, four different preferences for layout. So providing control over grid compartment display seemed like a good idea.

Comment 3

[Jim Mathies [:jimm]]

Attachment: patch v.3

sorry for the spam - a couple updates, some cleanup in top site view code, and I standardized pref naming.

Comment 4

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8383327 [details] [diff] [review]
patch v.3

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

Looks great, thanks for doing this - everytime I saw those hard-coded values I winced a little. It might be a nice follow-up to have the start views watch their respective prefs and re-populate when they change. If I know something should be in my history but don't see it on the start page, I might go to about:config, bump up the value and expect to see it. It doesn't hurt much to reload, but nor would it be hard to fix.

Comment 5

[Jim Mathies [:jimm]]

Attachment: patch v.4

Comment 6

[Jim Mathies [:jimm]]

Comment on attachment 8384753 [details] [diff] [review]
patch v.4

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

I moved as much of this logic as I could down into View.

Comment 7

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8384753 [details] [diff] [review]
patch v.4

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

Looks great, tests pass. Just the one question about the 'show' property. I would expect show to be a method name, showing to be the settable property.

::: browser/metro/modules/View.jsm
@@ +46,5 @@
> +  get maxTiles() {
> +    return this._maxTiles;
> +  },
> +
> +  set show(aFlag) {

maybe 'showing'? show sounds like an imperative verb, not a settable state

Comment 8

[Jim Mathies [:jimm]]

This seems to have broken a couple bookmarks test. Will dig into it.

Comment 9

[Jim Mathies [:jimm]]

ahh - this._startView._limit is accessed in BookmarksHelper.

Comment 10

[Jim Mathies [:jimm]]

Attachment: test updates

This just replaces all _limit references in the helpers and the tests with the new maxTiles prop.

Comment 11

[Jim Mathies [:jimm]]

try push:
https://tbpl.mozilla.org/?tree=Try&showall=0&rev=8d743a9f7282

Comment 12

[Sam Foster [:sfoster] (he/him)]

Odd, I'm sure I ran the tests. Maybe I just ran some selectively and not the whole suite. Glad you were paying attention at least :)

Comment 13

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/fx-team/rev/be56a571e0a5

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/be56a571e0a5

Comment 15

[Marco Mucci [:MarcoM]]

For testing and verification.  Reopen if any defects found.

Comment 16

[Kamil Jozwiak [:kjozwiak]]

Went through the verification process using the following Nightly build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-03-10-03-02-01-mozilla-central/

Used the following preferences again the test cases listed below:
- browser.display.startUI.topsites.maxresults
- browser.display.startUI.bookmarks.maxresults
- browser.display.startUI.history.maxresults

- Ensured that setting the pref's to 0 will disable and hide the category under the about:start
- Disabled all three categories and ensured everything was still working correctly
- Ensured that once a category is disabled, bookmarks, history and top sites categories are not filled in with current visited sites
- Bookmarked several websites while the "Bookmark" category was disabled, ensured that the website appeared once the category was re-enabled
- Ensured that bookmarked websites still have the correct button state even though the category has been disabled
- Ensured that categories appeared in the correct order once others are disabled (example, Top sites disabled, the first one should be Bookmarks)
- Ensured that setting the preferences to a negative number didn't cause any issues (stability, unexpected crashes etc..)
- Ensured that the correct tiles are still appearing under the auto complete search when categories are disabled
- Ensured that the "Reset" button correctly added the correct default values for all three preferences
- Ensured that switching between fxdesktop and fxmetro didn't reset the preferences back to default values or some other obscure values
- Ensured that the number of tiles appearing under each category matches the corresponding pref

Found a view issues relating to changing the above pref's, created the following tickets:
- Bug #981655
- Bug #981666