Closed Bug 977814 Opened 6 years ago Closed 6 years ago

Add a pref to control total number of top site tiles

Categories

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

x86_64
Windows 8.1
defect

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 30

People

(Reporter: jimm, Assigned: jimm)

References

Details

(Whiteboard: p=2 s=it-30c-29a-28b.3 r=ff30)

Attachments

(2 files, 3 obsolete files)

The "we love our tiles!" bug.
Attached patch patch (obsolete) — Splinter Review
I like having 16 vs. 8.
Attachment #8383263 - Flags: review?(sfoster)
Attached patch patch v.2 (obsolete) — Splinter Review
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.
Attachment #8383263 - Attachment is obsolete: true
Attachment #8383263 - Flags: review?(sfoster)
Attachment #8383325 - Flags: review?(sfoster)
Attached patch patch v.3 (obsolete) — Splinter Review
sorry for the spam - a couple updates, some cleanup in top site view code, and I standardized pref naming.
Attachment #8383325 - Attachment is obsolete: true
Attachment #8383325 - Flags: review?(sfoster)
Attachment #8383327 - Flags: review?(sfoster)
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.
Attachment #8383327 - Flags: review?(sfoster) → review+
Whiteboard: p=2
Status: NEW → ASSIGNED
Priority: -- → P1
QA Contact: kamiljoz
Whiteboard: p=2 → p=2 s=it-30c-29a-28b.3 r=ff30
Attached patch patch v.4Splinter Review
Attachment #8383327 - Attachment is obsolete: true
Attachment #8384753 - Attachment is patch: true
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.
Attachment #8384753 - Flags: review?(sfoster)
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
Attachment #8384753 - Flags: review?(sfoster) → review+
This seems to have broken a couple bookmarks test. Will dig into it.
ahh - this._startView._limit is accessed in BookmarksHelper.
Attached patch test updatesSplinter Review
This just replaces all _limit references in the helpers and the tests with the new maxTiles prop.
Attachment #8384753 - Attachment is obsolete: true
Attachment #8386446 - Flags: review+
Attachment #8384753 - Attachment is obsolete: false
Attachment #8386446 - Attachment description: patch w/test updates v.5 → test updates
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 :)
https://hg.mozilla.org/mozilla-central/rev/be56a571e0a5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
For testing and verification.  Reopen if any defects found.
Flags: needinfo?(kamiljoz)
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
Status: RESOLVED → VERIFIED
Flags: needinfo?(kamiljoz)
You need to log in before you can comment on or make changes to this bug.