908927 - Use templates instead of string concatenation for search URIs

Status: RESOLVED FIXED

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

Description

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

commit 5910944bc3cd4a263299431a3832c5582325f393 
Build 8/23/2013

1. modify the <b2g>/build/applications-data.js to include http://search.yahoo.co.jp/search?p=
2. use the search engine, by tapping on the url and typing a word
3. hit enter

Expected: the search should be done on the word 
Actual: a prefix of "?q=" is added to the query

Comment 1

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

Attachment: applications-data.js

Comment 2

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

https://mxr.mozilla.org/gaia/source/apps/browser/js/browser.js#713

https://mxr.mozilla.org/gaia/source/apps/browser/js/browser.js#1011

Comment 3

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

Just to note for some of the shipping countries : 

Google poland uses : #q= 
ex : http://www.google.pl/#q=asdfasdf

Yahoo Brazil uses : ?p=
http://br.search.yahoo.com/search?p=asdfasdfas

Comment 4

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

To clarify the actual result : 
http://search.yahoo.co.jp/search?p=?q=blah would show if I searched for blah... and the end result of what is searched is ?q=blah instead of blah

Comment 5

[Ben Francis [:benfrancis]]

The solution we have right now is very simplistic and another problem with this I discovered today is that the search strings we need to add for Google can not be added as part of build time customisation because they need to go after the ? - this makes it a high priority.

My plan is to use the template syntax from OpenSearch (http://www.opensearch.org/Specifications/OpenSearch/1.1#OpenSearch_URL_template_syntax) to describe a pattern for a search URI rather than simplistic concatenation of strings.

I was thinking of making this change in bug 833149 as we'll also need that syntax for the suggestions URI, but we can track the change to the main search URI here if you like.

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

what info do you need from me?

Comment 7

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

Hi Brad, I am calling this to attention (see comment 5) to place on a higher priority in pivotal, as this could potentially be a problem in other countries.  I wasn't sure if this would be considered a blocker so I haven't set the flag.

Comment 8

[Ben Francis [:benfrancis]]

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

Comment 9

[Dale Harvey (:daleharvey)]

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

This patch looks good, however failing unit tests right now, clearing r? till they are passing

Comment 10

[Ben Francis [:benfrancis]]

Yay, more intermittent oranges! And unrelated to this patch.

The tests are passing for me but I will try to figure out how to reproduce the failures...

Comment 11

[Ben Francis [:benfrancis]]

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

These test failures are definitely unrelated to this patch as they already occur intermittently in master. I'm working on fixing them in bug 915076 but it would be good to get an r+ or r- on this bug separately. We can wait until the tree is green to land it if you prefer.

Comment 12

[Dale Harvey (:daleharvey)]

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

We need to come up with a better mechanism for upgrading data than wiping the current data (which my have been modified), for now since we havent previously released customisation features its ok, also the upgrade code is very flaky looking, https://github.com/mozilla-b2g/gaia/pull/12090/files#L2R55 happening async without feedback is almost certain to screw up tests at some point, it may be the problem with the current tests.

But we definitely want to be using templates, and this works so r+

Comment 13

[Dale Harvey (:daleharvey)]

and yeh we cant land this until the tests are fixed or disabled, if they are intermittent then at least a green build is needed (I just retriggered)

Comment 14

[Dale Harvey (:daleharvey)]

This is r+'d so if you keep a green tree its good to land

I really should have mentioned that we should be adding tests, I think doing a simple successful search would suffice, but theres a follow up to this so I think its cool to land this and to the tests in the other bug

Comment 15

[Ben Francis [:benfrancis]]

https://github.com/mozilla-b2g/gaia/commit/abff7bf12d5033201405d12d3e8d8bcd6f1dfdff

Comment 16

[ffos-browser]

A Pivotal Tracker story has been created for this Bug: http://www.pivotaltracker.com/story/show/57231894