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?

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.

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

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

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

(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.

I'll take a look into this.

Chris, any news on this? Thanks

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.

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 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...

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.

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 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.

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.

(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?

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.

Fixed indent.

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.

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 on attachment 8568053 [details] [diff] [review] bug-1121417.patch Review of attachment 8568053 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!

checkin-patch

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

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.

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.

Asking Gavin since Florian is unavailable.

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

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.

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 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.

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.

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 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 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+

(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.

Needs rebasing for beta uplift.

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 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.

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).

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?

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

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.

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

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.

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.

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.

Fixed missing newline at end of file.

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 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 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+

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.

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

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

Rebasad on mozilla-beta tip.

Indenting