1510296 - Update Firefox to use new Google Codes

Status: VERIFIED FIXED

(Firefox :: Search, defect, P1)

Description

[Mike Kaply [:mkaply]]

We've got some new codes from Google we'll be using in product.

Comment 1

[Mike Kaply [:mkaply]]

Attachment: Bug 1510296 - Update Google search for new codes.

Comment 2

[Mike Kaply [:mkaply]]

Upon further research, we can't switch the code from google in absearch for various technical reasons.

I'm proposing that we switch to the new engine names dynamically (since we have to do it for ESR anyway)

mconnor, need your feedback. I've updated the patch in phabricator with this approach.

Comment 3

[Mike Connor [:mconnor]]

Discussed elsewhere, this will have to do.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/91ff42a9952c
Update Google search for new codes. r=daleharvey

Comment 5

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/91ff42a9952c

Comment 6

[Andrei Ciure[:aciure]]

Backed out 1 changesets (Bug 1510296) for blocking Bug 1516028

Comment 7

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/0124a534484d
Backed out 1 changesets for blocking bug 1516028 a=backout

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2b4dd7891375
Update Google search for new codes. r=daleharvey

Comment 9

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out changeset 2b4dd7891375 (Bug 1510296) for build bustages.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=2b4dd78913757e27c9175105197ac45844b41b90&selectedJob=219178069

Backout link: https://hg.mozilla.org/integration/autoland/rev/f2227be9432b24ec3a9c2eb7ee76c597412d8683

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=219178069&repo=autoland&lineNumber=43575

[task 2018-12-29T05:24:10.412Z] 05:24:10     INFO -  package> Duplicates 5262 bytes:
[task 2018-12-29T05:24:10.412Z] 05:24:10     INFO -  package>   browser/chrome/icons/default/default64.png
[task 2018-12-29T05:24:10.412Z] 05:24:10     INFO -  package>   browser/chrome/browser/content/branding/icon64.png
[task 2018-12-29T05:24:10.412Z] 05:24:10     INFO -  package>
[task 2018-12-29T05:24:10.412Z] 05:24:10     INFO -  package> Duplicates 6185 bytes:
[task 2018-12-29T05:24:10.412Z] 05:24:10     INFO -  package>   browser/chrome/devtools/modules/devtools/client/themes/toolbars.css
[task 2018-12-29T05:24:10.412Z] 05:24:10     INFO -  package>   browser/chrome/devtools/skin/toolbars.css
[task 2018-12-29T05:24:10.412Z] 05:24:10     INFO -  package>
[task 2018-12-29T05:24:10.413Z] 05:24:10     INFO -  package> Duplicates 7156 bytes:
[task 2018-12-29T05:24:10.413Z] 05:24:10     INFO -  package>   browser/chrome/browser/content/browser/places/bookmarkProperties.xul
[task 2018-12-29T05:24:10.413Z] 05:24:10     INFO -  package>   browser/chrome/browser/content/browser/places/bookmarkProperties2.xul
[task 2018-12-29T05:24:10.413Z] 05:24:10     INFO -  package>
[task 2018-12-29T05:24:10.413Z] 05:24:10     INFO -  package> Duplicates 12923 bytes:
[task 2018-12-29T05:24:10.413Z] 05:24:10     INFO -  package>   browser/chrome/icons/default/default128.png
[task 2018-12-29T05:24:10.413Z] 05:24:10     INFO -  package>   browser/chrome/browser/content/branding/icon128.png
[task 2018-12-29T05:24:10.413Z] 05:24:10     INFO -  package>
[task 2018-12-29T05:24:10.413Z] 05:24:10     INFO -  package> Duplicates 19892 bytes:
[task 2018-12-29T05:24:10.413Z] 05:24:10     INFO -  package>   browser/features/[e-mail]/chrome/content/autofillEditForms.js
[task 2018-12-29T05:24:10.413Z] 05:24:10     INFO -  package>   browser/chrome/browser/res/payments/formautofill/autofillEditForms.js
[task 2018-12-29T05:24:10.413Z] 05:24:10     INFO -  package>
[task 2018-12-29T05:24:10.413Z] 05:24:10     INFO -  package> WARNING: Found 26 duplicated files taking 77226 bytes (uncompressed)
[task 2018-12-29T05:24:10.414Z] 05:24:10     INFO -  package> ERROR: The following duplicated files are not allowed:
[task 2018-12-29T05:24:10.414Z] 05:24:10     INFO -  package> browser/chrome/browser/searchplugins/images/google.ico
[task 2018-12-29T05:24:10.414Z] 05:24:10     INFO -  package> removed-files
[task 2018-12-29T05:24:10.414Z] 05:24:10     INFO -  package> /builds/worker/workspace/build/src/toolkit/mozapps/installer/packager.mk:26: recipe for target 'stage-package' failed
[task 2018-12-29T05:24:10.414Z] 05:24:10    ERROR -  package> make[5]: *** [stage-package] Error 1
[task 2018-12-29T05:24:10.414Z] 05:24:10     INFO -  package> make[5]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/browser/installer'
[task 2018-12-29T05:24:10.414Z] 05:24:10     INFO -  package> /builds/worker/workspace/build/src/toolkit/mozapps/installer/packager.mk:95: recipe for target 'make-package' failed
[task 2018-12-29T05:24:10.414Z] 05:24:10    ERROR -  package> make[4]: *** [make-package] Error 2
[task 2018-12-29T05:24:10.414Z] 05:24:10     INFO -  package> /builds/worker/workspace/build/src/config/rules.mk:424: recipe for target 'default' failed
[task 2018-12-29T05:24:10.414Z] 05:24:10    ERROR -  package> make[3]: *** [default] Error 2
[task 2018-12-29T05:24:10.414Z] 05:24:10     INFO -  package> /builds/worker/workspace/build/src/browser/build.mk:6: recipe for target 'package' failed
[task 2018-12-29T05:24:10.415Z] 05:24:10    ERROR -  package> make[2]: *** [package] Error 2
[task 2018-12-29T05:24:10.415Z] 05:24:10     INFO -  /builds/worker/workspace/build/src/build/moz-automation.mk:84: recipe for target 'automation/package' failed
[task 2018-12-29T05:24:10.415Z] 05:24:10    ERROR -  make[1]: *** [automation/package] Error 2

Comment 10

[Mike Kaply [:mkaply]]

So the problem here is that the google.ico ended up 0 bytes.

I thought the problems with phabricator and binary files had been fixed?

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f6790e7386ac
Update Google search for new codes. r=daleharvey

Comment 12

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/258f373334d6
Backed out changeset f6790e7386ac for causing Android build bustages CLOSED TREE

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/605c950e275b
Update Google search for new codes. r=daleharvey

Comment 14

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/605c950e275b

Comment 15

[Pascal Chevrel:pascalc]

Mike, is that a patch that will need to be uplifted to beta or backported to ESR?

Comment 16

[Mike Kaply [:mkaply]]

Comment on attachment 9027939 [details]
Bug 1510296 - Update Google search for new codes.

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: New Google Codes

User impact if declined: No user impact

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: Yes

If yes, steps to reproduce: Search Google in Firefox, verify new codes

List of other uplifts needed: None

Risk to taking this patch: Medium

Why is the change risky/not risky? (and alternatives if risky): I'm marking risky because it's revenue related, but this has been verified on nightly and we will be watching the data closely to make sure there are no changes to our search volume.

String changes made/needed:

Comment 17

[Mike Kaply [:mkaply]]

Comment on attachment 9027939 [details]
Bug 1510296 - Update Google search for new codes.

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: Needed for better search tracking on ESR

User impact if declined: No user impact

Fix Landed on Version: 66

Risk to taking this patch: Medium

Why is the change risky/not risky? (and alternatives if risky): Marking medium because it's revenue related, but we are tracking this very closely via dashboards.

String or UUID changes made by this patch:

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9027939 [details]
Bug 1510296 - Update Google search for new codes.

[Triage Comment]
Updates our Google search codes. Approved for 65.0b8 and 60.5.0esr.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fea77d171e0f

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Needs a rebased patch for ESR60.

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Mike, while we may have relanded the right fix and this time it worked well, should we add automated tests that ensure we don't accidentally revert our default search engine on Fennec? I believe we need an automated test that would catch bug 1516028.

Comment 22

[Mike Kaply [:mkaply]]

> Hi Mike, while we may have relanded the right fix and this time it worked well, should we add automated tests that ensure we don't accidentally revert our default search engine on Fennec? I believe we need an automated test that would catch bug 1516028.

Yes. I'm looking into that.

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Thanks Mike. Do you want a new bug on file for that? or should we reuse this one to track that?

Comment 24

[Adrian Florinescu [:aflorinescu]]

Mike, could you elaborate a bit how we should verify this issue? I'm not familiar with this part of search.

Comment 25

[Mike Kaply [:mkaply]]

> Mike, could you elaborate a bit how we should verify this issue? I'm not familiar with this part of search.

Sure. Doing a search on Google from the address bar in the US used to show firefox-b-1-ab and outside the US, firefox-b-1

After this change, the codes will be firefox-b-1-d and firefox-b-d (and the -ab part is gone).

On Fennec, they've changed to firefox-b-1-m and firefox-b-m

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Still needs a rebased patch for ESR60.

Comment 27

[Mike Kaply [:mkaply]]

Sorry for the delay on this. Build issues with ESR60. Will have patch today.

Comment 29

[Mike Kaply [:mkaply]]

Attachment: Patch for ESR

Here's the ESR patch.

I've verified the tests pass locally and ESR code is being used in the product.

I don't completely understand why I didn't need to check for:

AppConstants.MOZ_APP_VERSION_DISPLAY.endsWith("esr"))

and use a different version of the codes in the tests.

Comment 30

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 9027939 [details]
Bug 1510296 - Update Google search for new codes.

ESR uplift

Comment 31

[Adrian Florinescu [:aflorinescu]]

Verified the code changes to " firefox-b-d" for non-US on:

Ubuntu 16.04 , Windows 10 , OSX 10.14.2
65.0b9 20190107180200
66.0a1 20190108101613

A short breakdown of scenarios ran during verification:

• new code visible in address bar search result: "search?client=firefox-b-d"
• code verification in telemetry (about:telemetry#keyed-histograms-tab - filter by by search)
  SEARCH_COUNTS

    google.in-content:sap:firefox-b-d  
    google.in-content:sap-follow-on:firefox-b-d   
    google-b-d.urlbar 
    google-b-d.searchbar    
    google-b-d.newtab 
  
  

The above covers scenarios of address bar google search, new page google search, search bar google search, multiple searches from the same inpage google search. Also covered the scenario in which google would not be default search engine and the search engine shortcut would be used for search panel.

Grover, could you assist covering the above scenarios for the code changes for US: "firefox-b-1-d"

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/acf00cfc7227

Comment 33

[Mike Kaply [:mkaply]]

Attachment: Fix test that is failing on ESR

Extra patch needed for ESR (and I'll get it on trunk before ESR 68)

The test reads directly from the list.json, so it needs to know about the ESR switch.

Comment 34

[u554753]

Verified on 65.0b9
Build ID 20190107180200
With Windows 10 x64, Ubuntu 18.10 x64, and Mac OS X 10.13

On 66, I'm unable to navigate with a search on about:newtab as it moves the cursor to the urlbar, I don't know if this is intended behavior or not.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/d02b5cc2d51b

Comment 36

[Adrian Florinescu [:aflorinescu]]

(In reply to Grover Wimberly IV [:Grover-QA] Use NeedInfo from comment #34)

On 66, I'm unable to navigate with a search on about:newtab as it moves the
cursor to the urlbar, I don't know if this is intended behavior or not.

I've noticed the above as well, but since that seems to be a feature: bug 1508388, I think we are good.
Based on comment 31 and comment 34 marking this issue as verified.
Leaving the NI on for ESR verification

Comment 37

[Julien Cristau [:jcristau]]

Backed out per https://bugzilla.mozilla.org/show_bug.cgi?id=1522019#c4

https://hg.mozilla.org/releases/mozilla-esr60/rev/dc7991c83de9

Comment 38

[Adrian Florinescu [:aflorinescu]]

Removing the qe+ flag as well since according to comment 37 this fix won't make it to ESR 60.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

ESR work moved to bug 1522686.