Closed Bug 908927 Opened 9 years ago Closed 9 years ago

Use templates instead of string concatenation for search URIs

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: nhirata, Assigned: benfrancis)

Details

(Whiteboard: [apps watch list])

Attachments

(2 files)

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
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
Flags: needinfo?(bfrancis)
Flags: needinfo?(krudnitski)
Flags: needinfo?(blassey.bugs)
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
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.
Flags: needinfo?(bfrancis)
Summary: [browser] search engines add ?q= and not all search engines use ?q= → Use templates instead of string concatenation for search URIs
Flags: needinfo?(krudnitski)
Whiteboard: [apps watch list]
what info do you need from me?
Flags: needinfo?(blassey.bugs) → needinfo?(nhirata.bugzilla)
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.
Flags: needinfo?(nhirata.bugzilla)
Assignee: nobody → bfrancis
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
Attachment #802575 - Flags: review?(dale)
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 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.
Attachment #802575 - Flags: review?(dale)
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+
Attachment #802575 - Flags: review?(dale) → review+
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)
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
https://github.com/mozilla-b2g/gaia/commit/abff7bf12d5033201405d12d3e8d8bcd6f1dfdff
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
A Pivotal Tracker story has been created for this Bug: http://www.pivotaltracker.com/story/show/57231894
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Attachment mime type: text/plain → text/x-github-pull-request
You need to log in before you can comment on or make changes to this bug.