1009358 - [User Story] Rocketbar: Ability to choose search provider used on Enter press

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Settings, defect, P3)

Comment 1

[Peter Dolanjski [:pdol]]

It may make sense to pull the settings for this out of the existing Browser into Settings so that the user choice applies to both the current Browser and Rocketbar. (assuming that is possible)

Comment 2

[Kevin Grandon :kgrandon]

Un-assigning from myself until homescreen work is finished.

Comment 3

[Jacqueline Savory [:jsavory] UX]

Attachment: Homepage_Settings_May28.pdf

Adding homescreen settings page which includes the search engine selector.

Comment 4

[Kevin Grandon :kgrandon]

Ben - is it realistic that we will have this done in 2.0? I think going with a configured provider (instead of user-selected) is slightly less work for 2.0 and something we were originally planning on doing. I'm just wondering if we should move this into 2.1, but keep bug 1009353 in 2.0.

Comment 5

[Ben Francis [:benfrancis]]

Hi Kevin, we discussed this on Vidyo yesterday. Bug 1009353 is the first priority and this one is higher risk but would be nice to have for 2.0 as well. They're both in the backlog, let's see what we get done.

Comment 6

[Ben Francis [:benfrancis]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/20069

Submitting this WIP patch for some feedback from Taipei in the hope that Evelyn or Arthur are still around to give some feedback before they go home.

I still need to write tests for this patch but wanted to check I'm going in the right direction so we can get it landed by feature landing.

Comment 7

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8434875 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20069

Overall it looks good to me. However, I still have concerns on storing the provider list in the settings DB as the list is not user preference.

Comment 8

[Ben Francis [:benfrancis]]

Thanks for the feedback Arthur, I have updated the patch based on your comments.

With regards to using DataStore instead of Settings API, we had a long discussion about this yesterday and I suggested the same thing (maintaining a DataStore of search providers), but in the end I was persuaded to use the settings API instead. The reasons are:
1) We populate the setting at build time based on customisations (see bug 1009353) which we can't do with DataStore AFAIK
2) DataStore has no mechanism to get a cursor over a list of entries and is intended to be used in conjunction with IndexedDB. Creating an IndexedDB database and a DataStore and keeping them synchronised just for this one thing seems like overkill. daleharvey argued against it.

Comment 9

[Ben Francis [:benfrancis]]

I have filed bug 1020920 to allow DataStores to be populated at build time, like we can with settings, so that in future we don't have to use the Settings API like this.

Comment 10

[Ben Francis [:benfrancis]]

Comment on attachment 8434875 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20069

I've added tests, now ready for review. Hopefully we can get this landed tomorrow (Friday) in time for Feature Landing :)

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

EJ, would you be able to review this so Ben could reach FL here?

Comment 12

[Ben Francis [:benfrancis]]

Comment on attachment 8434875 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20069

Thanks for the review Vivien! I have addressed your comments.

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8434875 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20069

r+ with nits fixed and a followup bug to create a VerticalHomescreen.searchEngines object that contains itself.

It will make the code more isolated and easier to read.

Comment 14

[Dale Harvey (:daleharvey)]

Ben just to check, this currently doesnt implement the icon settings right? thats something I am gonna need in https://bugzilla.mozilla.org/show_bug.cgi?id=1022053

Comment 15

[Dale Harvey (:daleharvey)]

Also dont worry about implementing the UI for the icons in the search app at least, I can do that in https://bugzilla.mozilla.org/show_bug.cgi?id=1022053, but will be blocked by the icon setting being populated

Comment 16

[Ben Francis [:benfrancis]]

Note: Waiting for green Travis to land, there's what looks like a perma fail on vertical homescreen making the tree red :(

Comment 17

[Ben Francis [:benfrancis]]

Dale, no this doesn't implement the settings entry point in the search app (in the form of an icon). This is intentional because that version of the spec is only supposed to be implemented if we support multiple search suggestion providers (bug 1021779).

We are storing the search.suggestionUrlTemplate setting, but it is not yet being used because we need a simple opensearch suggestions implementation in order to build the front end for this, and I didn't quite have time to do that before the end of the week. (We basically need a small opensearch library to sit alongside the EverythingMe one and use OpenSearch suggestions instead of the EverythingMe API if EverythingMe isn't set as the current search engine).

Currently suggestions are always provided by EverythingMe so it would be misleading to display the icon of another search engine next to those suggestions.

Comment 18

[Ben Francis [:benfrancis]]

Oh and also yes, the setting in the Settings app currently only changes the search.urlTemplate setting, it doesn't set search.iconUrl or search.suggestionsUrlTemplate which is maybe what you were asking?

Comment 19

[Ben Francis [:benfrancis]]

https://github.com/benfrancis/gaia/commit/bafa8ed1b3257de4b41c2fb0cbdc3d5d021b823d

Comment 20

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Thanks for the review, Vivien :)

Comment 21

[Kevin Grandon :kgrandon]

Mass modify - set status-b2g-v2.0 fixed for fixed bugs under vertical homescreen dependency tree.

Comment 22

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

https://moztrap.mozilla.org/manage/case/13895/
https://moztrap.mozilla.org/manage/case/13892/

Verified:
Gaia      8df02268fcd7e80c5fab8c1ec099772e37f8659d
Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/731a5e8831e6
BuildID   20140627000201
Version   32.0a2
ro.build.version.incremental=109
ro.build.date=Mon Jun 16 16:51:29 CST 2014
B1TC00011220
Flame