Closed Bug 1510296 Opened Last year Closed 11 months ago

Update Firefox to use new Google Codes

Categories

(Firefox :: Search, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 66
Tracking Status
firefox-esr60 - wontfix
firefox64 --- wontfix
firefox65 + verified
firefox66 + verified

People

(Reporter: mkaply, Assigned: mkaply)

References

Details

Attachments

(1 file, 2 obsolete files)

We've got some new codes from Google we'll be using in product.
Status: NEW → ASSIGNED
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.
Flags: needinfo?(mconnor)
Discussed elsewhere, this will have to do.
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/91ff42a9952c
Update Google search for new codes. r=daleharvey
https://hg.mozilla.org/mozilla-central/rev/91ff42a9952c
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
No longer blocks: 1516028
Depends on: 1516028
Backed out 1 changesets (Bug 1510296) for blocking Bug 1516028
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 65 → ---
Backout by aciure@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/0124a534484d
Backed out 1 changesets for blocking bug 1516028 a=backout
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/2b4dd7891375
Update Google search for new codes. r=daleharvey
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/formautofill@mozilla.org/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
Flags: needinfo?(mozilla)
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?
Flags: needinfo?(mozilla)
Flags: needinfo?(mconnor)
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/f6790e7386ac
Update Google search for new codes. r=daleharvey
Backout by aciure@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/258f373334d6
Backed out changeset f6790e7386ac for causing Android build bustages CLOSED TREE
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/605c950e275b
Update Google search for new codes. r=daleharvey
https://hg.mozilla.org/mozilla-central/rev/605c950e275b
Status: REOPENED → RESOLVED
Closed: Last year11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66
Mike, is that a patch that will need to be uplifted to beta or backported to ESR?
Flags: needinfo?(mozilla)
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:
Flags: needinfo?(mozilla)
Attachment #9027939 - Flags: approval-mozilla-beta?
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:
Attachment #9027939 - Flags: approval-mozilla-esr60?
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.
Attachment #9027939 - Flags: approval-mozilla-esr60?
Attachment #9027939 - Flags: approval-mozilla-esr60+
Attachment #9027939 - Flags: approval-mozilla-beta?
Attachment #9027939 - Flags: approval-mozilla-beta+
Needs a rebased patch for ESR60.
Flags: needinfo?(mozilla)
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.
> 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.
Flags: needinfo?(mozilla)
Thanks Mike. Do you want a new bug on file for that? or should we reuse this one to track that?
Mike, could you elaborate a bit how we should verify this issue? I'm not familiar with this part of search.
Flags: needinfo?(mozilla)
> 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
Flags: needinfo?(mozilla)
Still needs a rebased patch for ESR60.
Flags: needinfo?(mozilla)
Sorry for the delay on this. Build issues with ESR60. Will have patch today.
Flags: needinfo?(mozilla)
Duplicate of this bug: 1510656
Attached patch Patch for ESR (obsolete) — Splinter Review

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 on attachment 9027939 [details]
Bug 1510296 - Update Google search for new codes.

ESR uplift

Flags: needinfo?(ryanvm)
Attachment #9027939 - Flags: approval-mozilla-esr60+ → approval-mozilla-esr60?
Flags: needinfo?(ryanvm)
Attachment #9027939 - Flags: approval-mozilla-esr60?
Attachment #9034780 - Flags: approval-mozilla-esr60+

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"

Flags: needinfo?(gwimberly)
Attached patch Fix test that is failing on ESR (obsolete) — Splinter Review

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.

Attachment #9035155 - Flags: review?(mconnor)
Attachment #9035155 - Flags: review?(mconnor) → review+

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.

Flags: needinfo?(gwimberly) → needinfo?(adrian.florinescu)

(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

Status: RESOLVED → VERIFIED
Depends on: 1522019
Attachment #9034780 - Attachment is obsolete: true
Attachment #9034780 - Flags: approval-mozilla-esr60+ → approval-mozilla-esr60-
Attachment #9035155 - Attachment is obsolete: true

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

Flags: qe-verify+
See Also: → 1522686
Regressions: 1532761
You need to log in before you can comment on or make changes to this bug.