Last Comment Bug 722352 - Modify Google Search String Attributes
: Modify Google Search String Attributes
Status: RESOLVED FIXED
[qa+]
:
Product: Firefox
Classification: Client Software
Component: Search (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: Firefox 13
Assigned To: Matthew N. [:MattN] (PM me if requests are blocking you)
:
: Florian Quèze [:florian] [:flo]
Mentors:
Depends on: 724116 731078 731080
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-30 09:20 PST by Kev Needham [:kev]
Modified: 2013-12-27 14:22 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
v.1 Use preprocessor and MOZ_UPDATE_CHANNEL (2.97 KB, patch)
2012-02-03 23:23 PST, Matthew N. [:MattN] (PM me if requests are blocking you)
gavin.sharp: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Kev Needham [:kev] 2012-01-30 09:20:58 PST
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] 2012-01-30 10:44:16 PST
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: gavin@gavinsharp.com] 2012-01-30 11:38:52 PST
(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: gavin@gavinsharp.com] 2012-01-30 11:40:12 PST
This will require some search service changes, it's not just a matter of tweaking the default plugin.
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-30 12:00:27 PST
(In reply to Gavin Sharp (use gavin@gavinsharp.com 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] 2012-01-30 12:03:27 PST
(In reply to Gavin Sharp (use gavin@gavinsharp.com 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] 2012-01-30 12:04:06 PST
Yes, that. :D

(In reply to Gavin Sharp (use gavin@gavinsharp.com 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: gavin@gavinsharp.com] 2012-02-03 14:46:52 PST
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: gavin@gavinsharp.com] 2012-02-03 14:53:41 PST
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] (PM me if requests are blocking you) 2012-02-03 23:23:31 PST
Created attachment 594399 [details] [diff] [review]
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] (PM me if requests are blocking you) 2012-02-03 23:56:05 PST
(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: gavin@gavinsharp.com] 2012-02-06 14:31:02 PST
(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: gavin@gavinsharp.com] 2012-02-06 17:23:04 PST
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] (PM me if requests are blocking you) 2012-02-08 00:01:50 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9e27547c22b

Try run: https://tbpl.mozilla.org/?tree=Try&rev=6db593b1780e
Comment 14 Ed Morley [:emorley] 2012-02-08 08:59:11 PST
https://hg.mozilla.org/mozilla-central/rev/f9e27547c22b
Comment 15 Axel Hecht [:Pike] 2012-02-15 07:48:00 PST
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] (PM me if requests are blocking you) 2012-02-15 21:31:03 PST
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] 2012-02-15 21:50:44 PST
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] 2012-02-19 23:58:35 PST
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] (PM me if requests are blocking you) 2012-02-23 18:59:12 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/14b4ca2bc6bc
Comment 20 Marco Bonardo [::mak] 2012-02-24 02:47:13 PST
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

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