Closed Bug 1518514 Opened 1 year ago Closed 1 year ago

Search Engine reporting not working on Fennec

Categories

(Firefox for Android :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 66
Tracking Status
firefox-esr60 --- unaffected
firefox64 --- wontfix
firefox65 --- fixed
firefox66 --- fixed

People

(Reporter: mkaply, Assigned: mkaply)

References

Details

(Keywords: regression)

Attachments

(1 file)

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.

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.

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
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66
Blocks: 1461432
Keywords: regression

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+

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)
Depends on: 1522151
You need to log in before you can comment on or make changes to this bug.