Closed Bug 952098 Opened 11 years ago Closed 11 years ago

Add places as a rocketbar provider

Categories

(Firefox OS Graveyard :: Gaia::Search, defect)

Product:
Firefox OS Graveyard
Component:
Gaia::Search
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
1.3 C2/1.4 S2(17jan)

People

(Reporter: daleharvey, Assigned: daleharvey)

References

Details

(Whiteboard: [ucid:System86, 1.4:P2, ft:systems-fe][systemsfe])

User Story

User Story:
As a user, I want Rocketbar to search and display results based on partial URLs or webpage titles that I enter for websites that I have visited in the past to make it much quicker to browse to sites I've previously visited.

Acceptance Criteria:
1. I can enter a partial string and relevant results from my history are displayed as is currently done in the Browser.
2. Selecting one of the history results loads that page.

Assumptions: 
1. UX spec to specify number of entries. 
2. Frecency algorithm is the same that is currently used in the Browser Awesome Bar. 
3. String matching with historical web pages is the same as in Browser Awesome Bar.

Attachments

(1 file, 3 obsolete files)

https://github.com/mozilla-b2g/gaia/pull/15437/files
52 bytes, text/plain
kgrandon
: review+
Details
No description provided.
Blocks: rocketbar-search-mvp
Assignee: nobody → dale
Attached file Add places as a rocketbar provider (obsolete) —
Attachment #8350069 - Flags: review?(kgrandon)
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
Attachment #8350069 - Flags: review?(kgrandon)
Attached patch resolve_952098_conflicts.patch (obsolete) — — Splinter Review
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.
Attachment #8354873 - Flags: feedback?(dale)
User Story: (updated)
Whiteboard: [ucid:System86, 1.4:P2, ft:systems-fe]
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
Attachment #8354873 - Flags: feedback?(dale) → feedback+
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?
Flags: needinfo?(fdjabri)
No longer blocks: rocketbar-search-mvp
Component: Gaia → Gaia::Search
Whiteboard: [ucid:System86, 1.4:P2, ft:systems-fe] → [ucid:System86, 1.4:P2, ft:systems-fe][systemsfe]
Target Milestone: --- → 1.3 C2/1.4 S2(17jan)
Blocks: 959353
Attachment #8350069 - Attachment is obsolete: true
Attachment #8354873 - Attachment is obsolete: true
Attachment #8361086 - Flags: review?(kgrandon)
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] [review] 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!
Attachment #8361086 - Flags: review?(kgrandon) → review+
Addressed the nits and added a new test for the duplication of results
https://github.com/mozilla-b2g/gaia/commit/ff30172aa6f374c4c4cc3450dd3bb0dcff6d30d5
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Backed out for now due to test failure. https://github.com/mozilla-b2g/gaia/commit/f80e8067d076a1ab4b1225d14c215a4ade2eeb43
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/98d8d856549b
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Shouldnt have been resolved, should I have added some flag? the commit got backed out
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Attachment #8361086 - Attachment is obsolete: true
Attachment #8361489 - Flags: review?(kgrandon)
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.
Attachment #8361489 - Flags: review?(kgrandon) → review+
https://github.com/mozilla-b2g/gaia/commit/6b5c4493e9dbda2fd85bb0e2dca9c2d3f3880f66 praying it sticks this time
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Blocks: 948303
No longer blocks: 959353
Flags: needinfo?(fdjabri)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: