722352 - Modify Google Search String Attributes

Status: RESOLVED FIXED

(Firefox :: Search, defect, P1)

Description

[Kev Needham [:kev]]

As part of the renewal with Google, search query attributes for the Mozilla-branded versions of Firefox require modification to the "client" attributes, and the addition of a "channel" attribute. Modifications to the "rls" attribute are not required. The changes need to be made to each release channel we offer for desktop Firefox as below. Please note that these changes should NOT be used for Firefox Mobile.

Client Attribute Changes:

The client attributes must now be release-channel specific. The following client codes should be used by release, where Google is the default search provider (note that nightly should still use "unofficial" in the rls component, and the other three channels should use "official" in the rls component, as they are Mozilla-distributed builds. Firefox ESR should use the same client values as the Release channel:

Release: firefox-a
Beta:    firefox-beta
Aurora:  firefox-aurora
Nightly: firefox-nightly


Channel Attribute Additions:

Location Bar: All queries originating from the Location Bar should include the attribute of "channel=fflb" in addition to the existing attributes

Context/Right-click Search: All queries originating from the Context Menu/Right Click search for Google should include the attribute "channel=rcs" in addition to the existing attributes

About:home: All queries originating from the locally-hosted about:home should have an attribute of "channel=np" in addition to the existing attributes

Please let me know what additional information is required to develop product/config changes to accommodate these changes.

Comment 1

[Johnathan Nightingale [:johnath]]

Adding Gavin and Dolske to the cc list. I'm hoping that one of them can write a patch for the other to review, assuming there are no concerns with the request?

Comment 2

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

(In reply to Kev [:kev] Needham from comment #0)
> (note that nightly should still use "unofficial" in the rls
> component, and the other three channels should use "official" in the rls
> component, as they are Mozilla-distributed builds.

Just a point of clarification: "official" vs. "unofficial" in the rls parameter has been controlled since pre-Firefox 4 by the official branding flag - so the only builds we've shipped with "official" are beta/release. I take this to mean that we now want to change Aurora from "unofficial" to "official". (Also the use of the term "Mozilla-distributed" to distinguish Nightly from Aurora/Beta/Release is a little confusing - they're all distributed by Mozilla, right?).

> Release: firefox-a
> Beta:    firefox-beta
> Aurora:  firefox-aurora
> Nightly: firefox-nightly

Should whether Google is the default engine for the build continue to be a factor in the parameter value, or should we now send these values unconditionally? I.e. should the Google search plugin in Bing builds send these parameters, or something else?

Comment 3

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

This will require some search service changes, it's not just a matter of tweaking the default plugin.

Comment 4

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

(In reply to Gavin Sharp (use [e-mail] for email) from comment #2)
> Should whether Google is the default engine for the build continue to be a
> factor in the parameter value, or should we now send these values
> unconditionally? I.e. should the Google search plugin in Bing builds send
> these parameters, or something else?

Nevermind, I see this was answered in your email. We should only send "client=firefox-a" for builds where Google is the default. That leaves in question what we should send when it isn't though - we currently send "client=firefox", so I guess we can continue to do that?

Comment 5

[Kev Needham [:kev]]

(In reply to Gavin Sharp (use [e-mail] for email) from comment #2)
> (In reply to Kev [:kev] Needham from comment #0)
> > (note that nightly should still use "unofficial" in the rls
> > component, and the other three channels should use "official" in the rls
> > component, as they are Mozilla-distributed builds.
> 
> Just a point of clarification: "official" vs. "unofficial" in the rls
> parameter has been controlled since pre-Firefox 4 by the official branding
> flag - so the only builds we've shipped with "official" are beta/release. I
> take this to mean that we now want to change Aurora from "unofficial" to
> "official". (Also the use of the term "Mozilla-distributed" to distinguish
> Nightly from Aurora/Beta/Release is a little confusing - they're all
> distributed by Mozilla, right?).

Sorry, I wasn't clear there. Aurora, Beta, and Release are branded in a way to attribute them as Mozilla builds. I don't believe it's necessary to have Aurora use the official branding flag, but will double-check with Google.

> > Release: firefox-a
> > Beta:    firefox-beta
> > Aurora:  firefox-aurora
> > Nightly: firefox-nightly
> 
> Should whether Google is the default engine for the build continue to be a
> factor in the parameter value, or should we now send these values
> unconditionally? I.e. should the Google search plugin in Bing builds send
> these parameters, or something else?

The old rules apply. If Google is not the default build, we should send "firefox". The client codes above should be sent if Google is the default search engine, and the Moz Param function may need some tweaking as a result (or we'd have to use separate search plugins per release channel). I will also clarify this with Google, but my interpretation is that we need to make the determination still.

Comment 6

[Kev Needham [:kev]]

Yes, that. :D

(In reply to Gavin Sharp (use [e-mail] for email) from comment #4)
> Nevermind, I see this was answered in your email. We should only send
> "client=firefox-a" for builds where Google is the default. That leaves in
> question what we should send when it isn't though - we currently send
> "client=firefox", so I guess we can continue to do that?

Comment 7

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

For changes to the "client" parameter, I think we can simplify some of the logic by preprocessing the plugin at build time, and have the MozParam depend on the value of MOZ_UPDATE_CHANNEL.

Comment 8

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

I filed bug 724116 to track the "channel" parameter changes in the Firefox 12 time frame.

We can use this bug to track changes to the "client" param.

Comment 9

[Matthew N. [:MattN]]

Attachment: v.1 Use preprocessor and MOZ_UPDATE_CHANNEL

I noticed that the bg locale has its own Google search plugin which I'm guessing is a mistake.

I'm not an expert in our build system so I may be doing something wrong.

There is potential for this makefile to break L10N/build scripts since they may be making the assumption that the source dir. search plugins are the same as the dist. ones and therefore use the preprocessed version instead of the non-preprocessed and vice versa.

Comment 10

[Matthew N. [:MattN]]

(In reply to Matthew N. [:MattN] from comment #9)
> Created attachment 594399 [details] [diff] [review]
> v.1 Use preprocessor and MOZ_UPDATE_CHANNEL

Note that this patch doesn't change the "rls" attribute as its unclear to me what needs to be done.

Comment 11

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

(In reply to Matthew N. [:MattN] from comment #9)
> I noticed that the bg locale has its own Google search plugin which I'm
> guessing is a mistake.

Yes, a few locales incorrectly have their own copies of google.xml (http://mxr.mozilla.org/l10n-mozilla-aurora/find?text=&string=google.xml). The packaging code gives precedence to the en-US version when there are duplicates, though, so this isn't much of a problem in practice.

Comment 12

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

I don't think anything in the build system will be negatively affected (it sucks a little that our checked-in search plugin won't be a valid plugin in source form, but I don't think anything depends on that).

I am a little bit worried that localizers will need to avoid introducing preprocessor-unfriendly changes in their search plugins, but that should be quite unlikely to occur (valid XML and preprocessor instructions don't really overlap at all).

Comment 13

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9e27547c22b

Try run: https://tbpl.mozilla.org/?tree=Try&rev=6db593b1780e

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f9e27547c22b

Comment 15

[Axel Hecht [:Pike]]

We'll need follow up bugs for l10n now, right?

http://mxr.mozilla.org/l10n-mozilla-aurora/find?string=google-&tree=l10n-mozilla-aurora&hint= finds the non-en-US google plugins we have in l10n.

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 594399 [details] [diff] [review]
v.1 Use preprocessor and MOZ_UPDATE_CHANNEL

[Approval Request Comment]
Regression caused by (bug #): N/A
User impact if declined: N/A
Testing completed (on m-c, etc.): m-c since 2012-02-08
Risk to taking this patch (and alternatives if risky): Potential for L10N automation problems since google.xml is now pre-processed. 
String changes made by this patch: None

Comment 17

[Alex Keybl [:akeybl]]

Comment on attachment 594399 [details] [diff] [review]
v.1 Use preprocessor and MOZ_UPDATE_CHANNEL

[Triage Comments]
Approved for Aurora 12.

Comment 18

[Axel Hecht [:Pike]]

Comment on attachment 594399 [details] [diff] [review]
v.1 Use preprocessor and MOZ_UPDATE_CHANNEL

Review of attachment 594399 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/locales/Makefile.in
@@ +172,5 @@
> +	$(NSINSTALL) -D $(DESTDIR)$mozappdir/searchplugins
> +	for i in $^; do \
> +	  SEARCH_PLUGIN_BASE=`basename $$SEARCH_PLUGIN`;\
> +	  $(PYTHON) $(topsrcdir)/config/Preprocessor.py $(DEFINES) $(ACDEFINES) \
> +	    $$SEARCH_PLUGIN > $(DESTDIR)$mozappdir/searchplugins/$$SEARCH_PLUGIN_BASE; \

The change from $(mozappdir) to $mozappdir looks dangerous here. I'm not sure if anything is actually using the instal target these days, so I don't know if we'd had seen it if it was actually broken.

Comment 19

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/14b4ca2bc6bc

Comment 20

[Marco Bonardo [:mak]]

I suppose this is the right bug for this changeset? I don't see the landing annotated anywhere.
https://hg.mozilla.org/mozilla-central/rev/aa70aeacde1f