Last Comment Bug 511017 - Replace existing Google search plugin with one that supports dynamic updates
: Replace existing Google search plugin with one that supports dynamic updates
Status: RESOLVED WONTFIX
: productization
Product: Firefox
Classification: Client Software
Component: Search (show other bugs)
: Trunk
: All All
-- normal with 1 vote (vote)
: ---
Assigned To: Ryan Flint [:rflint] (ping via IRC for reviews)
:
: Florian Quèze [:florian] [:flo] (PTO until February 27)
Mentors:
: 460623 (view as bug list)
Depends on: 514377 556512
Blocks:
  Show dependency treegraph
 
Reported: 2009-08-17 16:44 PDT by Kev Needham [:kev]
Modified: 2013-03-05 22:28 PST (History)
24 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
Updated Google Plugin incorporating dynamic updates (3.79 KB, patch)
2009-08-17 16:44 PDT, Kev Needham [:kev]
no flags Details | Diff | Splinter Review
Updated plugin (2.99 KB, patch)
2010-01-29 18:30 PST, Ryan Flint [:rflint] (ping via IRC for reviews)
mbeltzner: review-
Details | Diff | Splinter Review

Description User image Kev Needham [:kev] 2009-08-17 16:44:17 PDT
Created attachment 394942 [details] [diff] [review]
Updated Google Plugin incorporating dynamic updates

The existing Google search plugin for all locales uses a google.com query URL, which relies on a geo-IP lookup to redirect the user to the appropriate search results. Per the work done to support OpenSearch updates (see https://wiki.mozilla.org/User:Mconnor/SearchUpdates for more info), Google has provided a new search plugin which will support dynamic updates.

Instead of using the Google geo IP-based redirects for every uery, the initial search query will (on query) set the query URL to be used for that user. Subsequent searches would go directly to that URL, which would be refreshed periodically, reducing response time/lag that users currently incur when forced through the google.com redirect for each query.

The updated plugin should be the default Google plugin for all locales (I believe a couple locales use localized Google hostnames, and those should be removed and replaced with the default (on successful testing/acceptance).
Comment 1 User image Mike Beltzner [:beltzner, not reading bugmail] 2009-08-31 08:31:51 PDT
Connor, can you review the en-US?
Comment 2 User image Mike Connor [:mconnor] 2009-09-01 15:21:01 PDT
Comment on attachment 394942 [details] [diff] [review]
Updated Google Plugin incorporating dynamic updates

This is good to go.
Comment 3 User image Ryan Flint [:rflint] (ping via IRC for reviews) 2009-09-01 21:23:55 PDT
*** Bug 460623 has been marked as a duplicate of this bug. ***
Comment 4 User image Jordan Gutterman 2009-09-01 21:55:22 PDT
>-<Image width="16" height="16">data:image/png;base64,blahblahblahblah
>+<Image height="16" width="16" type="image/x-icon">http://www.google.com/favicon.ico</Image>

Does this require downloading a new favicon over http on every startup to show in the search box or is it cached somewhere?
Comment 5 User image Ryan Flint [:rflint] (ping via IRC for reviews) 2009-09-01 22:17:55 PDT
(In reply to comment #4)
> Does this require downloading a new favicon over http on every startup to show
> in the search box or is it cached somewhere?

It's cached in search.json once downloaded.
Comment 6 User image Axel Hecht [:Pike] 2009-09-02 11:35:57 PDT
CCing l10n community to get more global testing coverage.
Comment 7 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2009-09-02 21:38:09 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > Does this require downloading a new favicon over http on every startup to show
> > in the search box or is it cached somewhere?
> 
> It's cached in search.json once downloaded.

I'm not sure that requiring a download of the favicon on first startup and after each cache invalidate is a great idea. We can add an IconUpdateURL instead if we want to have the favicon update, right?
Comment 8 User image Ryan Flint [:rflint] (ping via IRC for reviews) 2009-09-03 01:11:31 PDT
Comment on attachment 394942 [details] [diff] [review]
Updated Google Plugin incorporating dynamic updates

(In reply to comment #7)
> I'm not sure that requiring a download of the favicon on first startup and
> after each cache invalidate is a great idea. We can add an IconUpdateURL
> instead if we want to have the favicon update, right?

Well, the update Google gives us when we hit /searchdomaincheck has a remote favicon - so we'll download a new one along with the new XML on first use and during every update anyway. Given that, there's not really any advantage to shipping the plugin with a remote favicon, and when I tested it, I only got the default icon (bug 514376). So there's clearly a downside to not using the data URI :)

>diff --git a/browser/locales/en-US/searchplugins/google.xml b/browser/locales/en-US/searchplugins/google.xml
>+<moz:SearchForm>http://www.google.com/firefox?client=firefox-a&amp;rls=org.mozilla:{moz:locale}:official</moz:SearchForm>

We don't run ParamSubstitution() on SearchForm (bug 514377) so {moz:locale} just gets passed through raw. It also seems like rls here probably wants to match the substitutions used in the search URL version, anyway:
>+  <Param name="rls" value="{moz:distributionID}:{moz:locale}:{moz:official}"/>
Comment 9 User image effrat 2009-12-17 13:23:25 PST
Trystan and I (from Google) also did some ad hoc testing, and didn't come across any issues.  For example, we substituted the new Google search plugin xml in for the old one in one of the nightly builds, and then used a proxy to simulate coming from a few different countries, and found the default domain was set as expected after install.
Comment 10 User image mack 2009-12-17 13:34:25 PST
Hi all, I'll be taking over from Jon on this bug going forward and added myself to the cc list.

Mack
Comment 11 User image Ryan Flint [:rflint] (ping via IRC for reviews) 2010-01-29 18:30:59 PST
Created attachment 424369 [details] [diff] [review]
Updated plugin

This works around bug 514376.
Comment 12 User image Mike Beltzner [:beltzner, not reading bugmail] 2010-03-26 23:47:18 PDT
What's the chance of this getting back-ported, once reviewed?
Comment 13 User image Axel Hecht [:Pike] 2010-03-27 03:08:39 PDT
Another question, would the dynamic updates be impacted if we shipped the search plugins inside the en-US.jar (etc) like we do for fennec?
Comment 14 User image Mike Beltzner [:beltzner, not reading bugmail] 2010-03-31 22:44:35 PDT
Gavin: this change to the default search plugin looks pretty straightforward now that bug 514377 is fixed; can you review?

Kev: see any issues here?
Comment 15 User image Kev Needham [:kev] 2010-04-01 06:33:29 PDT
Unfortunately, yes. There's a code change we need to make to ensure that updates don't accidentally overwrite params. I got feedback on a proposed methodology, and will file a bug outlining it and will make this one dependent on it.
Comment 16 User image Mike Beltzner [:beltzner, not reading bugmail] 2010-04-01 07:37:55 PDT
Comment on attachment 424369 [details] [diff] [review]
Updated plugin

>-<Url type="application/x-suggestions+json" method="GET" template="http://suggestqueries.google.com/complete/search?output=firefox&amp;client=firefox&amp;hl={moz:locale}&amp;q={searchTerms}"/>
>+<Url rel="self" type="application/opensearchdescription+xml" template="https://www.google.com/searchdomaincheck?format=opensearch&amp;sourceid=firefox"/>

^ shaver asks, and rightly, I think, why we don't actually point this at a service that Mozilla offers to ensure that we have a bit more direct control over what is the default search plugin. Google can continue to file bugs in our Bugzilla to have changes made, but we could roll out the changes with proper review.

>+<Url rel="suggestions" type="application/x-suggestions+json" method="GET" template="http://clients1.google.com/complete/search?client=firefox&amp;q={searchTerms}"/>

^ is this change intentional? the addition of the link rel tag is fine, but we're changing the destination server here ..

>-<SearchForm>http://www.google.com/firefox</SearchForm>
>+<SearchForm>http://www.google.com/firefox?client=firefox-a&amp;rls=org.mozilla:{moz:locale}:official</SearchForm>

^ I don't think we can do this in the non-branded tree
Comment 17 User image effrat 2010-04-01 11:46:33 PDT
Per the destination server question: suggestqueries.google.com is now deprecated, so the change to clients1.google.com is in fact intentional.  In terms of the other changes to the suggest request URL, the output param is deprecated in favor of just the client param, and we dropped the hl (language) since it's already inferred server-side based on the user's language preference in her Google cookie and the accept-language header from the client.
Comment 18 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2010-04-01 13:17:10 PDT
(In reply to comment #16)
> >-<SearchForm>http://www.google.com/firefox</SearchForm>
> >+<SearchForm>http://www.google.com/firefox?client=firefox-a&amp;rls=org.mozilla:{moz:locale}:official</SearchForm>
> 
> ^ I don't think we can do this in the non-branded tree

We can, we should just do it the same way we currently do rls:

<Param name="rls" value="{moz:distributionID}:{moz:locale}:{moz:official}"/>

{moz:distributionID} is effectively hardcoded to "org.mozilla" (it can change if someone configures their build with --with-distribution-id, but I don't think anyone does that). {moz:official} is "official" in official-branding builds, and "unofficial" otherwise.

The "client" param is also handled seperately:

<MozParam name="client" condition="defaultEngine" trueValue="firefox-a" falseValue="firefox"/>

This means that we send client=firefox-a if Google is the build's default engine, and client=firefox otherwise.
Comment 19 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2010-04-01 13:18:44 PDT
(In reply to comment #18)
> We can, we should just do it the same way we currently do rls:

Oh, except that <SearchForms> don't currently support having <Params>. I guess we should probably just fix that, shouldn't be very hard.
Comment 20 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2010-06-10 11:12:30 PDT
I filed bug 557890 for the URL changes - they're orthogonal to his bug.
Comment 21 User image mack 2010-06-10 11:21:01 PDT
Sounds good, I'll comment on the other bug.
Comment 22 User image Dietrich Ayala (:dietrich) 2010-08-04 05:53:03 PDT
This is not a blocker - doesn't prevent major functionality, not a regression, etc.

The changes seem low-risk though, so we'd take a patch if one is ready in time.

Note You need to log in before you can comment on or make changes to this bug.