Replace existing Google search plugin with one that supports dynamic updates

RESOLVED WONTFIX

Status

()

Firefox
Search
RESOLVED WONTFIX
8 years ago
4 years ago

People

(Reporter: kev, Assigned: rflint)

Tracking

({productization})

Trunk
productization
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 -)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

8 years ago
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).
Connor, can you review the en-US?
Comment on attachment 394942 [details] [diff] [review]
Updated Google Plugin incorporating dynamic updates

This is good to go.
Attachment #394942 - Flags: review+
Duplicate of this bug: 460623

Comment 4

8 years ago
>-<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?
(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

8 years ago
CCing l10n community to get more global testing coverage.
(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 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}"/>
Attachment #394942 - Flags: review+

Updated

8 years ago
Keywords: productization
Depends on: 514377

Comment 9

8 years ago
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

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

Mack
Created attachment 424369 [details] [diff] [review]
Updated plugin

This works around bug 514376.
Assignee: nobody → rflint
Attachment #394942 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #424369 - Flags: review?(gavin.sharp)
blocking2.0: --- → ?
What's the chance of this getting back-ported, once reviewed?

Comment 13

7 years ago
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?
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?
(Reporter)

Comment 15

7 years ago
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.
(Reporter)

Updated

7 years ago
Depends on: 556512
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
Attachment #424369 - Flags: review?(gavin.sharp) → review-

Comment 17

7 years ago
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.
(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.
(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.
I filed bug 557890 for the URL changes - they're orthogonal to his bug.

Comment 21

7 years ago
Sounds good, I'll comment on the other bug.
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.
blocking2.0: ? → -
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.