Closed
Bug 977814
Opened 7 years ago
Closed 7 years ago
Add a pref to control total number of top site tiles
Categories
(Firefox for Metro Graveyard :: Firefox Start, defect, P1)
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)
16.50 KB,
patch
|
sfoster
:
review+
|
Details | Diff | Splinter Review |
15.50 KB,
patch
|
jimm
:
review+
|
Details | Diff | Splinter Review |
The "we love our tiles!" bug.
Updated•7 years ago
|
Blocks: metrobacklog
![]() |
Assignee | |
Comment 1•7 years ago
|
||
I like having 16 vs. 8.
Attachment #8383263 -
Flags: review?(sfoster)
![]() |
Assignee | |
Comment 2•7 years ago
|
||
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)
![]() |
Assignee | |
Comment 3•7 years ago
|
||
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 4•7 years ago
|
||
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+
![]() |
Assignee | |
Updated•7 years ago
|
Whiteboard: p=2
Updated•7 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P1
QA Contact: kamiljoz
Whiteboard: p=2 → p=2 s=it-30c-29a-28b.3 r=ff30
![]() |
Assignee | |
Comment 5•7 years ago
|
||
Attachment #8383327 -
Attachment is obsolete: true
![]() |
Assignee | |
Updated•7 years ago
|
Attachment #8384753 -
Attachment is patch: true
![]() |
Assignee | |
Comment 6•7 years ago
|
||
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 7•7 years ago
|
||
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+
![]() |
Assignee | |
Comment 8•7 years ago
|
||
This seems to have broken a couple bookmarks test. Will dig into it.
![]() |
Assignee | |
Comment 9•7 years ago
|
||
ahh - this._startView._limit is accessed in BookmarksHelper.
![]() |
Assignee | |
Comment 10•7 years ago
|
||
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+
![]() |
Assignee | |
Updated•7 years ago
|
Attachment #8384753 -
Attachment is obsolete: false
![]() |
Assignee | |
Updated•7 years ago
|
Attachment #8386446 -
Attachment description: patch w/test updates v.5 → test updates
![]() |
Assignee | |
Comment 11•7 years ago
|
||
try push: https://tbpl.mozilla.org/?tree=Try&showall=0&rev=8d743a9f7282
Comment 12•7 years ago
|
||
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 :)
![]() |
Assignee | |
Comment 13•7 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/be56a571e0a5
https://hg.mozilla.org/mozilla-central/rev/be56a571e0a5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Comment 15•7 years ago
|
||
For testing and verification. Reopen if any defects found.
Flags: needinfo?(kamiljoz)
Comment 16•7 years ago
|
||
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.
Description
•