Closed Bug 1413298 Opened 2 years ago Closed 2 years ago

International searches don't work through engines added with WebExtensions

Categories

(Firefox :: Search, defect, major)

57 Branch
defect
Not set
major

Tracking

()

RESOLVED FIXED
Firefox 58
Tracking Status
firefox57 + fixed
firefox58 --- fixed

People

(Reporter: mkaply, Assigned: mkaply)

References

Details

Attachments

(1 file)

If you add an engine via WebExtension manifest.json and search for Japanese characters, we translate it to HTML before executing the search.

[Tracking Requested - why for this release]: Breaks international characters in searches added with WebExtensions.

I realize this is last minute, but I hope it's a simple fix and will have something today if it is fixable.
Sorry, I didn't totally understand the problem when I opened it. Come to find out, search engines default to windows-1252. Since we don't set an encoding, we get Windows 1252.

So when you do a search on:

日本最大級のポータルサイト

the search happens with:

日本最大級のポータルサイト
Comment on attachment 8923953 [details]
Bug 1413298 - Allow search encoding to be specifed by WebExtension.

https://reviewboard.mozilla.org/r/195138/#review200194

Looks reasonable, but I'm not going to r+ without test coverage.

::: browser/components/extensions/ext-chrome-settings-overrides.js:187
(Diff revision 2)
>          template: searchProvider.search_url,
>          iconURL: searchProvider.favicon_url,
>          alias: searchProvider.keyword,
>          extensionID: extension.id,
>          suggestURL: searchProvider.suggest_url,
> +        queryCharset: searchProvider.encoding ? searchProvider.encoding : "UTF-8",

In JS a ? a : b  can be simplified to a || b

::: toolkit/components/search/nsSearchService.js:1816
(Diff revision 2)
>      this._urls.push(new EngineURL(URLTYPE_SEARCH_HTML, method, aParams.template));
>      if (aParams.suggestURL) {
>        this._urls.push(new EngineURL(URLTYPE_SUGGEST_JSON, "GET", aParams.suggestURL));
>      }
> +    if (aParams.queryCharset) {
> +      this._queryCharset = aParams.queryCharset;

I wonder if this would be a better place to set the default encoding. Not sure if reusing the OS_PARAM_INPUT_ENCODING_DEF constant is a good idea.
Comment on attachment 8923953 [details]
Bug 1413298 - Allow search encoding to be specifed by WebExtension.

https://reviewboard.mozilla.org/r/195138/#review200174

::: browser/components/extensions/schemas/chrome_settings_overrides.json:103
(Diff revision 1)
>                    "is_default": {
>                      "type": "boolean",
>                      "optional": true,
>                      "description": "Sets the default engine to a built-in engine only."
> +                  },
> +                  "encoding": {

I'd be happier if this had some kind of actual validation.  So long as a broken value here has no real ill effect on searchservice, lets do a followup bug.
Attachment #8923953 - Flags: review?(mixedpuppy)
I've added a test.

Just to be more clear as to relman why this is important - we only give engines one change to install when then extension is installed and if the charset is bad, it will always be bad (there's no way for the addon developer to fix).

So we need to get it right the first time.

This is a very small patch that only adds handling of something that wasn't handled before.
Comment on attachment 8923953 [details]
Bug 1413298 - Allow search encoding to be specifed by WebExtension.

https://reviewboard.mozilla.org/r/195138/#review200244

I think this is good enough to solve the immediate problem, so r=me. Comment 5 looks like Shane wants to have another look and/or you to file a follow-up.
From our IRC discussion, I think you also want to file a follow-up to cleanup the mess of default encodings in the search service (3 different default encodings depending on how the engine is loaded).
Attachment #8923953 - Flags: review?(florian) → review+
Comment on attachment 8923953 [details]
Bug 1413298 - Allow search encoding to be specifed by WebExtension.

https://reviewboard.mozilla.org/r/195138/#review200260

::: browser/components/extensions/ext-chrome-settings-overrides.js:187
(Diff revision 4)
>          template: searchProvider.search_url,
>          iconURL: searchProvider.favicon_url,
>          alias: searchProvider.keyword,
>          extensionID: extension.id,
>          suggestURL: searchProvider.suggest_url,
> +        queryCharset: searchProvider.encoding || "UTF-8",

The test is only testing the default value, UTF-8, and not any engine setting encoding.  If the default value is enough to fix things for now, I'd rather not introduce encoding into the schema.
Attachment #8923953 - Flags: review?(mixedpuppy)
Comment on attachment 8923953 [details]
Bug 1413298 - Allow search encoding to be specifed by WebExtension.

https://reviewboard.mozilla.org/r/195138/#review200262

via irc the schema change will be dropped and we'll just send the searchservice UTF-8 for the queryEncoding.  r+ from me for that.
Attachment #8923953 - Flags: review?(mixedpuppy) → review+
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/2b8095d47140
Allow search encoding to be specifed by WebExtension. r=florian,mixedpuppy
Comment on attachment 8923953 [details]
Bug 1413298 - Allow search encoding to be specifed by WebExtension.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1397975 - Search in WebExtensions
[User impact if declined]: Unable to do i18n searches from any WebExtensions search engine
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: Non
[Is the change risky?]: No
[Why is the change risky/not risky?]: Isolated to adding the queryCharset in this one case.
[String changes made/needed]: No.

I realize this is last minute. Unfortunately it wasn't found until yesterday. Without this change, any WebExtension that adds a search engine in Firefox Quantum will always be broke because we only provide a mechanism to add engines at install (deliberate decision).
Attachment #8923953 - Flags: approval-mozilla-beta?
Relanded with fix. I'm adding the commit hooks so it doesn't happen again.
Flags: needinfo?(mozilla)
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/7f84d2fa5625
Allow search encoding to be specifed by WebExtension. r=florian,mixedpuppy
https://hg.mozilla.org/mozilla-central/rev/7f84d2fa5625
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Comment on attachment 8923953 [details]
Bug 1413298 - Allow search encoding to be specifed by WebExtension.

Sounds like a must fix, Beta57+
Attachment #8923953 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee: nobody → mozilla
(In reply to Mike Kaply [:mkaply] from comment #14)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: not yet
> [Needs manual test from QE? If yes, steps to reproduce]: No

Setting qe-verify- based on Mike's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
Duplicate of this bug: 336208
You need to log in before you can comment on or make changes to this bug.