The default bug view has changed. See this FAQ.

Modify Google Search String Attributes

RESOLVED FIXED in Firefox 12

Status

()

Firefox
Search
P1
normal
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: kev, Assigned: MattN)

Tracking

Trunk
Firefox 13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox12+ fixed)

Details

(Whiteboard: [qa+])

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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.
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?
(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?
This will require some search service changes, it's not just a matter of tweaking the default plugin.
Assignee: nobody → gavin.sharp
(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?
(Reporter)

Comment 5

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

Comment 6

5 years ago
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?
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.
Assignee: gavin.sharp → mnoorenberghe+bmo
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.
Depends on: 724116
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.
Attachment #594399 - Flags: review?(gavin.sharp)
(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.
(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.
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).
Attachment #594399 - Flags: review?(gavin.sharp) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9e27547c22b

Try run: https://tbpl.mozilla.org/?tree=Try&rev=6db593b1780e
Status: NEW → ASSIGNED
Target Milestone: Firefox 11 → Firefox 13
https://hg.mozilla.org/mozilla-central/rev/f9e27547c22b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
tracking-firefox12: --- → ?

Updated

5 years ago
tracking-firefox12: ? → +

Comment 15

5 years ago
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 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
Attachment #594399 - Flags: approval-mozilla-aurora?
Comment on attachment 594399 [details] [diff] [review]
v.1 Use preprocessor and MOZ_UPDATE_CHANNEL

[Triage Comments]
Approved for Aurora 12.
Attachment #594399 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 18

5 years ago
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.
https://hg.mozilla.org/releases/mozilla-aurora/rev/14b4ca2bc6bc
status-firefox12: --- → fixed
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
Depends on: 731078
Depends on: 731080
Whiteboard: [qa+]
See Also: → bug 869398
You need to log in before you can comment on or make changes to this bug.