511017 - Replace existing Google search plugin with one that supports dynamic updates

Status: RESOLVED WONTFIX

(Firefox :: Search, defect)

Description

[Kev Needham [:kev]]

Attachment: 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

[Mike Beltzner [:beltzner, not reading bugmail]]

Connor, can you review the en-US?

Comment 2

[Mike Connor [:mconnor]]

Comment on attachment 394942 [details] [diff] [review]
Updated Google Plugin incorporating dynamic updates

This is good to go.

Comment 4

[Jordan Gutterman]

>-<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

[Ryan Flint [:rflint] (ping via IRC for reviews)]

(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

[Axel Hecht [:Pike]]

CCing l10n community to get more global testing coverage.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

(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

[Ryan Flint [:rflint] (ping via IRC for reviews)]

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

[effrat]

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

[mack]

Hi all, I'll be taking over from Jon on this bug going forward and added myself to the cc list.

Mack

Comment 11

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Updated plugin

This works around bug 514376.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

What's the chance of this getting back-ported, once reviewed?

Comment 13

[Axel Hecht [:Pike]]

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

[Mike Beltzner [:beltzner, not reading bugmail]]

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

[Kev Needham [:kev]]

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

[Mike Beltzner [:beltzner, not reading bugmail]]

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

[effrat]

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

[:Gavin Sharp [email: [e-mail]]]

(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

[:Gavin Sharp [email: [e-mail]]]

(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

[:Gavin Sharp [email: [e-mail]]]

I filed bug 557890 for the URL changes - they're orthogonal to his bug.

Comment 21

[mack]

Sounds good, I'll comment on the other bug.

Comment 22

[Dietrich Ayala (:dietrich)]

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.