I can restrict the number of results to four using a hack on GridProvider, but this wouldn't create the scrollable UI the spec asks for. Spoke to Kevin and it looks like we need to stop using <gaia-grid> here and create something custom instead.

Here's a work in progress patch which simply limits the grid to 4 items. This doesn't achieve the horizontal scrolling behaviour required in the spec, but I think that's going to require some serious refactoring of GaiaGrid which we might want to consider doing in a follow-up. I think implementing the scrolling UI will require either a) adding a horizontal scrolling mode to <gaia-grid> or b) creating a <gaia-icon> component to reuse in both <gaia-grid> and a new IconProvider, in order to reuse all the code in the various GridItems. Limiting the grid to 4 items also doesn't meet the spec with regards to landscape mode showing more icons, but that's already limited to 4 wide so at least this isn't a regression.

OK, I think this is ready for review. There are some trade-offs in this patch. The apps results area has a fixed height to reduce flicker but means there's quite a gap before the next results section when icons only have one line labels. Also, the apps results area is hidden when there are no results, but this happens per grid provider which means that if the first provider returns no results it will get hidden but shown again if subsequent providers return results. Because all the providers return asynchronously there's no easy way around this without waiting to show results until all providers have returned. The horizontal scrolling feature will happen in follow-up bug 1127369, but this patch at least reduces the results down to one line so that we can start to see the new results formatting taking shape.

Comment on attachment 8556612 [details] Patch Few nits I would like to see addressed, this looks good and agree with the approach to limit the gaia-grid to 4 and then work on horizontal scrolling

Comment on attachment 8556612 [details] Patch Thanks Dale, have addressed review comments.

Comment on attachment 8556612 [details] Patch Yeh putting any of this logic in grid_provider which is a base class for each of the providers is messy and possibly even wrong (if grid.add were to be synchronous), I cant think of what the problem adding it to search.js would be but I can help out with it. search.js handles deduplication and is the singleton that all the providers run through so its the right place to handle that logic That aside the rest looks good, thanks

OK, not sure if this is any better.

Ben, how about collect: function(provider, results) { if (provider.remote) { this.loadingElement.classList.remove('loading'); } if (provider.dedupes) { results = this.dedupe.reduce(results, provider.dedupeStrategy); } if (results.length + shownResults > MAX_RESULTS) { var spaces = MAX_RESULTS - shownResults; if (spaces < 1) { return; // abort too? } results.splice(spaces, (results.length - spaces)); } shownResults += results.length; provider.render(results); }, Remembering to clear shownResults in `clear`, It keeps the dedupe logic together with the count logic and doesnt depend on any implementation details of the grid so we can not render to gaia-grid and it doesnt need to change.

OK, I've updated the patch. I think we may have some broken tests though, waiting for TreeHerder.

Treeherder is green! Dale, I think this is ready for review again.

Comment on attachment 8558132 [details] Patch Awesome, this is working well thanks

Comment on attachment 8558132 [details] Patch If this sticks we should uplift to 2.2 as part of the search refactoring committed feature.

v2.2: https://github.com/mozilla-b2g/gaia/commit/935f9c99ef55c1f57e54a4bbfaabd2efc89feb94

This issue is verified fixed on Flame 3.0 If the user conducts a search in the search bar they will only see 4 apps total of any combination from combined local apps, bookmarks and Marketplace results. Below the apps is a search list based on what search provider is currently being used. Environmental Variables: Device: Flame 3.0 (319mb)(Kitkat)(Full Flash) Build ID: 20150210010523 Gaia: 0cf517083f7eb5fc269e1236edba50534f65e3cd Gecko: 2cb22c058add Gonk: e7c90613521145db090dd24147afd5ceb5703190 Version: 38.0a1 (3.0) Firmware Version: v18D-1 User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0

adding verifyme keyword to be verified on 2.2

This issue has been verified successfully on latest Flame2.2 STR: 1. Launch Serch bar. 2. Input some keywords. **You can see the same phenomenon with comment 12 Verify video:" verify_1117967.mp4". Flame 2.2 build: Build ID 20150210002516 Gaia Revision b30c8e4303595a0fcb5b640d673cf8503b954701 Gaia Date 2015-02-10 04:09:47 Gecko Revision https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/3e9fa4e70a1b Gecko Version 37.0a2 Device Name flame Firmware(Release) 4.4.2 Firmware(Incremental) eng.cltbld.20150210.041059 Firmware Date Tue Feb 10 04:11:10 EST 2015 Bootloader L1TC000118D0

(In reply to SandKing from comment #17) > **You can see the same phenomenon with comment 12 Same with comment 15 not comment 12