1121417 - Search plugins with diacritics in their name cannot be hidden (only removed)

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[Michal Stanke (Mozilla.cz) [:mstanke][:MikkCZ]]

Happens in Czech Firefox 35 with plugins Heuréka and Slunečnice (default ones), or with any plugin I add myself (e.g. on Seznam.cz website).

How to reproduce it:
1. Go to the search options.
2. Uncheck a search plugin with diacritics in its name.
3. Now the plugin shouldn't be shown in the search tooltip.
4. But it is even after restarting Firefox. The options stays unchecked.

I tried to remove the diacritics from the name to be without diacritics, and after that the hiding works OK.

Btw. I also wonder, why its necessary to run Firefox once without the plugin .xml file, just changing the file contents makes no change in Firefox even after restart - maybe another standalone bug?

Comment 1

[:Gavin Sharp [email: [e-mail]]]

browser.search.hiddenOneOffs is read/set using getCharPref/setCharPref, which likely loses data here.

We should just use Preferences.jsm for this, which uses getComplexValue(..., nsISupportsString) automatically.

Comment 3

[Florian Quèze [:florian]]

[Tracking Requested - why for this release]: hiding search providers doesn't work reliably on localized builds.

Comment 4

[Mike Hoye [:mhoye]]

If I can get a DXR link up in here I bet this'd be a good mentored or first bug.

Comment 5

[Dave Townsend [:mossop]]

Here is where the preference is defined for the preference UIs, they probably both need to be switched to unichar types:

http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/search.xul#9
http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/search.xul#29

We also get the pref in the main UI here and there we need to use Preferences.jsm:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/urlbarBindings.xml#1144

Comment 6

[Florian Quèze [:florian]]

(In reply to Mike Hoye [:mhoye] from comment #4)
> If I can get a DXR link up in here I bet this'd be a good mentored or first
> bug.

It would certainly be a good mentored bug. My only concern is it might bitrot with the patch I'm currently mentoring in bug 1106926.

Comment 7

[Chris]

I'll take a look into this.

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Chris, any news on this? Thanks

Comment 9

[Chris]

Hey Sylv, I've been waiting on bug 1106926 to resolve (It looks like it just has) since I'll need to switch the code added for it from using the browser.search.hiddenOneOffs pref to using Preferences.jsm.

I've already done what should be most of the work for this bug and should have a patch posted sometime later this week or this weekend when time permits.

Comment 10

[Chris]

Attachment: bug-1121417.patch

I'm almost finished, but I'm having some issues with getting the test case to work properly.  I've varified that my changes work manually but on the test case, when I check the value of shownOneOffs in browser_hiddenOneOffs_diacritics.js it doesn't contain the test engine that I added regardless of whether or not I add it to the hiddenOneOffs pref.

I also added another test case to browser_hiddenOneOffs_cleanup.js to make sure that the observer is handling diacritics correctly (Using Preferences.jsm instead of Service.prefs).

Comment 11

[Florian Quèze [:florian]]

Comment on attachment 8561143 [details] [diff] [review]
bug-1121417.patch

Review of attachment 8561143 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch and the tests!

::: browser/components/nsBrowserGlue.js
@@ +424,3 @@
>            let hiddenEngines = hiddenPref ? hiddenPref.split(",") : [];
>            hiddenEngines = hiddenEngines.filter(x => x !== engineName);
>            Services.prefs.setCharPref("browser.search.hiddenOneOffs",

This should use Preferences.jsm too.

::: browser/components/search/test/browser_hiddenOneOffs_cleanup.js
@@ +71,5 @@
> +  const diacritic_engine = "Foo \u2661";
> +  let ns = {};
> +  Cu.import("resource://gre/modules/Preferences.jsm", ns);
> +
> +  Services.prefs.setCharPref("browser.search.hiddenOneOffs", diacritic_engine);

This needs to use Preferences.jsm

@@ +79,5 @@
> +    ns.Preferences.get("browser.search.hiddenOneOffs").split(",");
> +  is(hiddenOneOffs.includes(diacritic_engine), false,
> +     "Observer cleans up added hidden engines that include a diacritic.");
> +
> +  Services.prefs.setCharPref("browser.search.hiddenOneOffs", diacritic_engine);

This too.

::: browser/components/search/test/browser_hiddenOneOffs_diacritics.js
@@ +38,5 @@
> +
> +add_task(function* test_hidden() {
> +  let ns = {};
> +  Cu.import("resource://gre/modules/Preferences.jsm", ns);
> +  Services.prefs.setCharPref("browser.search.hiddenOneOffs", diacritic_engine);

You are importing Preferences.jsm here, but not using it; that's probably the reason why your test doesn't pass.

@@ +45,5 @@
> +  info("Opening search panel");
> +  searchbar.focus();
> +  yield promise;
> +
> +  let shownOneOffs = Array.from(getOneOffs(), x => x.getAttribute("label"));

getOneOffs() already returns an array, so you don't need to use Array.from, Array.map would be enough.

@@ +46,5 @@
> +  searchbar.focus();
> +  yield promise;
> +
> +  let shownOneOffs = Array.from(getOneOffs(), x => x.getAttribute("label"));
> +  is(shownOneOffs.includes(diacritic_engine), false,

comparing the value with |false| isn't really useful, so you could just do ok(!..., 

This could probably be simplified even more with Array.some:
ok(!getOneOffs().some(x => x.getAttribute("label") == diacritic_engine),
   "shown oneOffs...

Comment 12

[Sylvestre Ledru [:Sylvestre]]

We are late in the 36 cycle. I don't think we should rush to fix it. We can however accept a patch in 37.

Comment 13

[Chris]

Attachment: bug-1121417.patch

I corrected the errors that were present in the test case, swapped out Services.prefs for Preferences.jsm, and added a new test to ensure that engines with Diacritics are shown after being removed from hiddenOneOffs.

I applied this patch on top of the patch for Bug 1132028 so that will need to land before this patch does.

Comment 14

[Florian Quèze [:florian]]

Comment on attachment 8564785 [details] [diff] [review]
bug-1121417.patch

Review of attachment 8564785 [details] [diff] [review]:
-----------------------------------------------------------------

Did you mean to request review on this updated patch?

::: browser/components/nsBrowserGlue.js
@@ +427,1 @@
>                                       hiddenEngines.join(","));

nit: Please fix the indent on this line.

::: browser/components/search/test/browser_hiddenOneOffs_diacritics.js
@@ +11,5 @@
> +
> +let ns = {};
> +Cu.import("resource://gre/modules/Preferences.jsm", ns);
> +
> +function promiseNewEngine(basename) {

I wonder if it would be possible to use the promiseNewEngine function from head.js (http://mxr.mozilla.org/mozilla-central/source/browser/components/search/test/head.js#139) instead of duplicating it.

Comment 15

[Chris]

I agree that using the promiseNewEngine in head.js would be the preferred option, but the cleanup functions that it registered weren't working properly after adding two engines (nsSearchService complained about an engine not being in store).  The only real difference between the two functions is that my version isn't doing anything with the current engine.  Instead I took care of that in the init function since that seems to work better and nsSearchService doesn't complain.

Comment 16

[Florian Quèze [:florian]]

(In reply to Chris from comment #15)

> The only real difference between the two
> functions is that my version isn't doing anything with the current engine. 

Should we add a parameter to disable that behavior?

Comment 17

[Chris]

Attachment: bug-1121417.patch

Made the fixes suggested, I added an optional parameter to promiseNewEngine in head.js and removed the function from the diacritic test case.

Push to try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=23e245510855

There's an extra try job at https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a2b5f4fe299 that I still can't seem to cancel.

I thought that it might be related to me giving the system wrong credentials when I cancelled it last time but I gave it the same username/password that I have set up for pushing to try server and it still won't allow me to cancel.  So I'll have to look into what Gavin was saying for that.

Comment 18

[Chris]

Attachment: bug-1121417.patch

Fixed indent.

Comment 19

[Florian Quèze [:florian]]

Comment on attachment 8567551 [details] [diff] [review]
bug-1121417.patch

Review of attachment 8567551 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for removing the promiseNewEngine code duplication! :-)

::: browser/components/nsBrowserGlue.js
@@ +437,1 @@
>                                       hiddenEngines.join(","));

Please fix the indent of this line. Already requested in comment 14.

::: browser/components/search/test/head.js
@@ +142,5 @@
>      Services.search.init({
>        onInitComplete: function() {
>          let url = getRootDirectory(gTestPath) + basename;
> +        if (swapCurrent) {
> +          var current = Services.search.currentEngine;

While this (using 'var') works, I think there's a risk that it would confuse someone in the future, so I would prefer if you instead either just added "let current;" before the if, or just kept doing "let current = Services.search.currentEngine;" unconditionally (this code is only running in tests; not in the product itself, so a tiny amount of extra unneeded code execution isn't really a problem).

@@ +167,5 @@
>          });
>        }
>      });
>    });
> +}
\ No newline at end of file

Please keep a line break at the end of the file.

Comment 20

[Chris]

Attachment: bug-1121417.patch

I apologize about the indent.  I changed promiseNewEngine to just instantiate current and set it to current engine regardless of whether or not it's used.

Comment 21

[Florian Quèze [:florian]]

Comment on attachment 8568053 [details] [diff] [review]
bug-1121417.patch

Review of attachment 8568053 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 22

[Chris]

Attachment: bug-1121417.patch

checkin-patch

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/fx-team/rev/a7923665bfa0

Comment 24

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/3554805703a7 since e10s bc1 was unhappy about it, https://treeherder.mozilla.org/logviewer.html#?job_id=2081232&repo=fx-team

Comment 25

[Chris]

I've narrowed the problem down to the tests in browser_diacritic_engine.js.  It looks like something in those tests is causing issues when run with e10s.  I'll have to take another look and see why it's not cleaning up properly when run with e10s.

Comment 26

[Chris]

The issue is being caused by this snippet of code.

>  let promise = promiseEvent(searchPopup, "popupshown");
>  searchbar.focus();
>  yield promise;

Keyboard navigation tests seem to be the only tests affected by this.  I'm not entirely sure about the best way to change this to get keyboard navigation tests and diacritic one offs test to play nice together.  The only thing I can think of is to remove the diacritic tests from the e10s test suite.

Comment 27

[Chris]

Asking Gavin since Florian is unavailable.

Comment 28

[Chris]

Attachment: bug-1121417.patch

Uploading an updated patch since the old one had bitrotted.  There aren't any changes in here to fix the e10s testing issue.

Comment 29

[Chris]

I think I found out what the devious problem was.  I've pushed the updated patch to try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f46fd900d2d

It looks like opening the search popup by changing the textbox's value caused an issue with keyboard navigation tests, for some reason it interfered with that test's ability to alter the search history when run under e10s.

Comment 30

[Chris]

Attachment: bug-1121417.patch

The e10s try run looks good, this patch should good to go.  I had to make some pretty substantial changes to the way to test case worked in order to pass e10s.  Gavin would you mind taking a look at the test case since Florian is away?

Comment 31

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8570521 [details] [diff] [review]
bug-1121417.patch

>diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml

>+        let ns = {};
>+        Cu.import("resource://gre/modules/Preferences.jsm", ns);
>+        let pref =
>+          ns.Preferences.get("browser.search.hiddenOneOffs");

You should be able to re-write this a bit more clearly as:

let Preferences = Cu.import("resource://gre/modules/Preferences.jsm", {}).Preferences;
let pref = Preferences.get("browser.search.hiddenOneOffs");

(and then use this pattern more consistently throughout the patch)

>diff --git a/browser/components/search/test/browser_hiddenOneOffs_diacritics.js b/browser/components/search/test/browser_hiddenOneOffs_diacritics.js

>+add_task(function* init() {
>+  const cachedPref = Services.prefs.getCharPref("browser.search.hiddenOneOffs");

Shouldn't you avoid using getCharPref here? Though actually, I don't think you need to store the old value - you can just clearUserPref at the end of the test?

>diff --git a/browser/components/search/test/head.js b/browser/components/search/test/head.js

>+function promiseNewEngine(basename, swapCurrent = true) {

Boolean parameters are a little bit evil. To make the callers clearer, you could make this an optional "options" parameter, defined as an object that optionally has the "setAsCurrent" property. E.g.:

function promiseNewEngine(basename, options = {}) {
  let setAsCurrent = options.setAsCurrent || false;
  // ...
}

>diff --git a/browser/components/search/test/testEngine_diacritics.xml b/browser/components/search/test/testEngine_diacritics.xml

>+  <ShortName>Foo &#9825;</ShortName>
>+  <Description>Diacritics Test Engine</Description>

I might adjust this just to explain what's unique about this engine in more detail: "Engine whose ShortName contains non-BMP Unicode characters"

It looks like there's no coverage for the preferences window getting/setting of this pref, but that's probably not worth worrying about too much.

Good job chasing down the e10s issue!

r=me with those comments addressed.

Comment 32

[Chris]

Attachment: bug-1121417.patch

Thanks Gavin.

> function promiseNewEngine(basename, options = {}) {
>  let setAsCurrent = options.setAsCurrent || false;
>  // ...
> }

I changed the optional parameter as suggested but I decided to use

let setAsCurrent = 
  options.setAsCurrent == undefined ? true : options.setAsCurrent;

I want to default setAsCurrent to true to match the most likely use case without having to alter other parts of the code.

I made the other changes that you suggested.

Comment 33

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/0bc001f799df

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0bc001f799df

Comment 35

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Setting as qe-verify- since this seems to be well covered by automated tests, so I don't think additional manual testing is needed. Feel free to set it back if you think otherwise.

Comment 36

[Florian Quèze [:florian]]

Comment on attachment 8570834 [details] [diff] [review]
bug-1121417.patch

Approval Request Comment
[Feature/regressing bug #]: bug 1088660 (new search bar)
[User impact if declined]: impossible to hide the one-off buttons of some default engines in some localized builds (eg. cz).
[Describe test coverage new/current, TreeHerder]: tests included in the patch.
[Risks and why]: low risk; most of the patch is test additions.
[String/UUID change made/needed]: none.

Comment 37

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8570834 [details] [diff] [review]
bug-1121417.patch

Given the test coverage in this patch and that it is QE-, let's take this in Beta 3. Note that this is a fix for an issue introduced in 34. Beta+ Aurora+

Comment 38

[Florian Quèze [:florian]]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #37)

> this is a fix for an issue introduced in 34.

While the problem in the code was indeed introduced in 34, we only started localizing the new search UI in 35, so this problem was unlikely to impact significant numbers of users in 34.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for beta uplift.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/06d70b82938e

Comment 41

[Chris]

Attachment: bug-1121417_r.patch

I rebased this on top of the changeset with the tag FIREFOX_AURORA_38_BASE using the command hg update -r FIREFOX_AURORA_38_BASE.  This is the first time that I have rebased a patch to match the beta channel of firefox so please feel free to correct me if I've made a mistake.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8570834 [details] [diff] [review]
bug-1121417.patch

You need to rebase on top of Gecko 37, not 38. Also, please don't obsolete the original patch as in this case, the branch patch is something entirely different.

Comment 43

[Chris]

Attachment: bug-1121417_r.patch

I based this patch off of FIREFOX_AURORA_37_BASE as requested.  I ran the tests on my box for safety and they all passed.  I'll push to try if necassary as soon as I know that this is the right patch.

This patch does include stuff from the patch for Bug 1106926 which is necassary since the this patch changes code that was made in that patch (I've also included the test cases for Bug 1106926 to make sure we don't lose coverage).

Comment 44

[Ryan VanderMeulen [:RyanVM]]

Gavin, I'm not sure if this needs re-review or not given the inclusion of changes from another bug. Likewise on approval. What are your thoughts?

Comment 45

[:Gavin Sharp [email: [e-mail]]]

If this is built on bug 1106926, we should just also uplift that to 37. Does the patch in that bug apply cleanly?

Comment 46

[Chris]

I included the code changes for bug 1106926 in bug-1121417_r.patch since the patch for this bug was written on top of the patch for bug 1106926.

I could split bug-1121417_r.patch up into two patches, one that makes changes to http://mxr.mozilla.org/mozilla-central/source/browser/base/content/urlbarBindings.xml and one that makes changes to http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js and other code files affected by bug 1106926.  Just let me know which you would prefer.

Comment 47

[Chris]

It does look like the patch for bug 1106926 will apply cleanly to 37.

Comment 48

[Ryan VanderMeulen [:RyanVM]]

It'd probably be better for blame and consistency if we just uplift bug 1106926 separately in that case. How risky would that be? If you don't see that as a big deal, go ahead and nominate it.

Comment 49

[Chris]

Attachment: bug-1121417_r37.patch

I went ahead and made a patch for 37 that should apply cleanly as long as the patch for bug 1106926 is uplifted first, from what I tested the patch for bug 1106926 should just apply clean to 37 without any issues (Tested using hg import).

I added the promiseNewEngine function to http://mxr.mozilla.org/mozilla-central/source/browser/components/search/test/head.js since the tests in http://mxr.mozilla.org/mozilla-central/source/browser/components/search/test/browser_hiddenOneOffs_diacritics.js rely on it and it didn't exist when 37 was cut.

Comment 50

[Lawrence Mandel [:lmandel] (use needinfo)]

I don't see a big concern in uplifting bug 1106926 to Beta. Let's get a nom on that bug and another nom on the branch patch in this bug. If the branch patch in this bug is close enough to the original, I think we can carry forward the r+. If not, let's get another review on the patch before landing it on Beta.

Comment 51

[Chris]

Attachment: bug-1121417_r37.patch

Fixed missing newline at end of file.

Comment 52

[Chris]

The patch that I uploaded earlier today is pretty much identical to the original patch except for the changes mentioned in my previous post.  We may want to go ahead and do a review for that function but everything else should be fine.

Comment 53

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8574212 [details] [diff] [review]
bug-1121417_r37.patch

This will need to be adjusted to include a proper commit message (include the a=lmandel).

Comment 54

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8574212 [details] [diff] [review]
bug-1121417_r37.patch

I have already approved bug 1106926 for uplift. Approving this patch as well. Please note comment 53 about the required commit message. Beta+

Comment 55

[Chris]

Attachment: bug-1121417_r37.patch

The patch for bug 1106926 will need to be uplifted to 37 first otherwise this patch will fail to apply cleanly.  The patch for bug 1106926 was approved to be uplifted in bug 1106926 comment 43.  Once it lands this patch should apply cleanly.

Comment 56

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/513cff18abaa

Comment 57

[Ryan VanderMeulen [:RyanVM]]

I had to back this out for hitting mochitest-bc failures. Please rebase your patch on top of mozilla-beta tip.
https://hg.mozilla.org/releases/mozilla-beta/rev/89b593b91e5e

https://treeherder.mozilla.org/logviewer.html#?job_id=293807&repo=mozilla-beta

Comment 58

[Lawrence Mandel [:lmandel] (use needinfo)]

ni Chris to fix this up for potential relanding for Beta 5 by Thu, Mar 12.

Comment 59

[Chris]

Attachment: bug-1121417_r37.patch

Rebasad on mozilla-beta tip.

Comment 60

[Chris]

Attachment: bug-1121417_r37.patch

Indenting

Comment 61

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/90a33a9f129e