Comment on attachment 8350069 [details] Add places as a rocketbar provider Clearing review flag for now. I've added some comments to the pull request here: https://github.com/mozilla-b2g/gaia/pull/14855

Hi Dale - The interfaces have been changed/tweaked a bit. Didn't want you to suffer after you came back from break, so here is a simple patch that you can apply to resolve conflicts after merging master to your 'places-with-rocket' branch. Hope it helps.

Comment on attachment 8354873 [details] [diff] [review] resolve_952098_conflicts.patch Cheers for that, will apply and get the tests done ready for getting this merged

In bug 946778 the Places database records visits for both app windows and browser windows and doesn't currently differentiate between the two. That includes app:// URLs for packaged apps and http:// URLs for installed hosted apps as well as http:// URLs for all other web pages. We could try to not store history for windows that have a mozapp attribute so that we only record browser history for browser windows, but it would be nice to treat them consistently. Storing frecency of URLs for installed apps is just as valuable as the URLs you browse to with the URL bar, and could be the key to indexing and inter-ranking Rocketbar results from mixed sources (contacts, music, video etc.) in the future. There are also cases like clicking external hyperlinks in apps where an external web page might currently be loaded inside an "app window" of a different origin (e.g. clicking a link in a Facebook post) so it would be nice to log those too. However, this raises the question of how app history results should be displayed in the search app and what should happen when you click on them. * Do we want to hide app:// URLs from the user? * When you click a URL corresponding to an app (which isn't always easy to determine), should it open in an app window or in a browser window? What if the user has previously navigated to the same URL both inside an app and a browser window or loaded an external URL in an app window? * If we display results for bookmarked URLs and installed apps, should they show the app/bookmark title and app/bookmark icon, or the page title and favicon? Do we need to then de-dupe between history and app results (as we currently search apps separately)? Further down the line, when we migrate to the new bookmarks content model and sheets navigation I think we can treat all URLs more consistently. Sheets could be grouped by origin with a group for each bookmarked/installed app origin and a catch-all "browser" group for everything else. But in the meantime while app windows and browser windows have more distinct behaviour we need an interim behaviour. One suggestion of a simple interim solution might be: * Open all app:// URLs in app windows (try to determine the manifest URL corresponding to the app:// URL and open a new or existing app window at that URL). * Open all http:// URLs from history results in browser windows (even for installed hosted app history). * Display the page title and favicon for all history results, regardless of whether they correspond to an installed app. * Consider displaying bookmarks as icons alongside apps with their bookmark title and icon, as distinct from the corresponding history result. (Bookmarks and apps already have a lot in common and are displayed alongside each other on the homescreen.) How does that sound?

This currently ignores any app:// urls, and opens all results in the browser, favicons are currently missing, filing a follow up, I didnt want this patch to get too large.

Comment on attachment 8361086 [details] https://github.com/mozilla-b2g/gaia/pull/15414 Looks good. Please fix the issue where if I visit a URL twice it shows up twice in the results. Also add a test for that. Fixing the nits are up to you. Thanks for the quick/awesome work on this!

Addressed the nits and added a new test for the duplication of results

Backed out for now due to test failure. https://github.com/mozilla-b2g/gaia/commit/f80e8067d076a1ab4b1225d14c215a4ade2eeb43

Shouldnt have been resolved, should I have added some flag? the commit got backed out

This has changed enough to warrant a new review, there were 3 bugs 1. We get spurious app-open messages that caused visible results to be cleared, these no longer need to render the search app so removed it (we should still investigate where they come from) seperately 2. Searching in succession quickly wiped the screen, now we wait for the app to show / go home then do a new search 3. We only synced when we became visible, if the time between becoming visible and doing the search was less than the time it took to sync, the results wouldnt show, we now notify on write to sync

and here is 20 passing runs on travis :) https://travis-ci.org/mozilla-b2g/gaia/builds/17108372

Comment on attachment 8361489 [details] https://github.com/mozilla-b2g/gaia/pull/15437/files Nice work! Left a few comments on github.

https://github.com/mozilla-b2g/gaia/commit/6b5c4493e9dbda2fd85bb0e2dca9c2d3f3880f66 praying it sticks this time