Search Engine reporting not working on Fennec

RESOLVED FIXED in Firefox 65

Status

()

defect
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: mkaply, Assigned: mkaply)

Tracking

({regression})

unspecified
Firefox 66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox64 wontfix, firefox65 fixed, firefox66 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 months ago

Fennec isn't reporting the default search engine.

It's also showing search related errors on the console (though everything is working)

I think this was caused by:

https://bugzilla.mozilla.org/show_bug.cgi?id=1461432

When we moved away from browsersearch.json, list.json was no longer in the locale JAR file, so we're trying to read it from the wrong location.

We can't read a resource file in SearchEngineManager.java, so we need to create a custom content URL so we can read it.

(Assignee)

Comment 2

3 months ago

I was right. In bug 1461432, we did the switch but didn't realize the locale implications.

Because the browser itself uses the search service in JS, this didn't have any side effects except to break reporting. (Everything worked correct in the browser).

This patch switches list.json to be accessible via a chrome JAR URL and properly parses it.

Comment 3

3 months ago
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/b1522f31ad7d
Correctly read and parse list.json in search engine manager. r=nalexander

Comment 4

3 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66
(Assignee)

Updated

3 months ago
Blocks: 1461432
Keywords: regression
(Assignee)

Comment 5

3 months ago

Comment on attachment 9035173 [details]
Bug 1518514 - Correctly read and parse list.json in search engine manager.

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1461432

User impact if declined: No user impact, reporting only.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: This will be verified in reporting.

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): The function wasn't working at all, so this just makes it work.

String changes made/needed:

Attachment #9035173 - Flags: approval-mozilla-beta?

I'd feel a lot better about taking this if it had some sort of test coverage.

Comment on attachment 9035173 [details]
Bug 1518514 - Correctly read and parse list.json in search engine manager.

[Triage Comment]
I'm going to approve this for 65.0b12 because it fixes a function which is currently broken. NI mkaply to verify that 65.0b12 is properly reporting this information after it ships on Friday.

Flags: needinfo?(mozilla)
Attachment #9035173 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Comment 9

3 months ago

I'm going to approve this for 65.0b12 because it fixes a function which is currently broken. NI mkaply to verify that 65.0b12 is properly reporting this information after it ships on Friday.

Sadly it's not. I have one more fix that will have to wait for 66.

Flags: needinfo?(mozilla)
(Assignee)

Updated

3 months ago
Depends on: 1522151
You need to log in before you can comment on or make changes to this bug.