While working on bug 697981 I notices that my test that works locally as a single test fails in the test suite when run with many tests. Looking for my debug "writing content preferences" in http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mozilla@jorgk.com-e5af6b2c0ea8/try-linux64/try_ubuntu64_vm_test-mochitest-other-bm118-tests1-linux64-build849.txt.gz I can see that there are 11 cases in the following tests: 1) test_async_UpdateCurrentDictionary.html ***** Writing content preferences for |testing-XX| 2) test_bug1200533.html ***** Writing content preferences for |en-US| 3) test_bug338427.html ***** Writing content preferences for |en-US| 4) test_bug678842.html ***** Writing content preferences for |en-GB| ***** Writing content preferences for |en-US| 5) test_bug717433.html ***** Writing content preferences for |en-GB| ***** Writing content preferences for |en-US| 6) test_add_remove_dictionaries.xul ***** Writing content preferences for |base-utf| ***** Writing content preferences for |maputf| ***** Writing content preferences for |base-utf| ***** Writing content preferences for |en-US| We should clean this up, since new tests will fail in weird and wonderful ways depending on what previous tests have left behind.

There could be some side effects from bug 1204147. We need to count again after that bug landed.

I ran the test again with the patch for 1204147 included: https://treeherder.mozilla.org/#/jobs?repo=try&revision=122f1e4fdbc6 That greatly reduced the list of content preferences written: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mozilla@jorgk.com-122f1e4fdbc6/try-linux64/try_ubuntu64_vm_test-mochitest-other-bm53-tests1-linux64-build597.txt.gz In total we now have: 1) editor/composer/test/test_async_UpdateCurrentDictionary.html ***** Writing content preferences for |testing-XX| Analysis: line 56: SetCurrentDictionary("testing-XX"); 2) editor/composer/test/test_bug338427.html ***** Writing content preferences for |en-US| Analysis: line 37: spellchecker.spellChecker.SetCurrentDictionary(lang); 3) editor/composer/test/test_bug678842.html ***** Writing content preferences for |en-GB| Analysis: line 68: spellchecker.SetCurrentDictionary('en-US'); 4) editor/composer/test/test_bug717433.html ***** Writing content preferences for |en-US| Analysis: line 73 (or 76): spellchecker.SetCurrentDictionary("en-US"); 5) extensions/spellcheck/tests/chrome/test_add_remove_dictionaries.xul ***** Writing content preferences for |base-utf| ***** Writing content preferences for |maputf| Analysis: line 75: setCurrentDictionary(editor, "base-utf"); line 83: setCurrentDictionary(editor, "maputf"); The differences to comment #0 are caused by the fix in bug 1204147. In other words: The fix in bug 1204147 killed five cases where content preferences were written erroneously. Fantastic, so all the content preferences written here are expected. Now, the problem remains: How do we fix these five tests to clean up after themselves? Are there a few lines of script I can use, to remove the content preferences when the test finishes? I've looked at "removeGlobal" but couldn't make much sense of it: http://mxr.mozilla.org/mozilla-central/ident?i=removeGlobal Help!

I'm not really familiar with APIs dealing with content preferences, sorry. So I would as well just need to go through the existing tests and other code to see how to do that.

Who can I ask? I found toolkit/components/contentprefs/tests/unit_cps2/test_remove.js, and being not much of a JS programmer, can't make too much sense of that. Looks like the interface is also asynchronous, which makes harder still. Let's try Ehsan. He's always good for a quick tip. Executive summary and question ;-) In a test that sets content preferences, how can I remove them once the test is done, so the next test along doesn't fall over (because it doesn't expect those content preferences being set)? This case has happened, hence this bug to clean up the mess.

test_bug717433.html has this: is(currentDictonary, expected, expected + " expected"); content.removeEventListener('load', loadListener, false); // Remove the fake en-GB dictionary again, since it's otherwise picked up by later tests. hunspell.removeDirectory(en_GB); // Reset the preference, so the last value we set doesn't collide with the next test. Services.prefs.setCharPref("spellchecker.dictionary", ""); SimpleTest.finish(); Is something wrong with that? SpecialPowers.pushPrefEnv is a cleaner way to do this though: https://dxr.mozilla.org/mozilla-central/search?tree=mozilla-central&q=pushprefenv

Yes, sadly. If you look at comment #2 you can see that this test sets the content preference to "en-US". Removing the "en-GB" dictionary and clearing the preference will *not* remove the content preference. If you remove a dictionary which is referenced by a content preference, then there is some clean-up happening and the content preference is removed. Looking again at comment #2, all the tests which set content preference "en-US" need to be fixed.

What's your feeling on this? It makes no sense to allow empty content preferences to be written. I debugged it, the content preference really gets written to the database with an empty value. With this, we'd kill two birds with one stone. Make calling SetCurrentDictionary with an empty string do something useful and also open a door to cleaning up the tests.

Since the SetCurrentDictionary() also sets the preference, we can lose more code.

Here's the complete fix. Note that test_add_remove_dictionaries.xul doesn't need fixing since it removes the dictionaries for which content preferences were written, so that does the clean-up. Try run to follow once the try server opens again (currently closed: bug 1204832)

Previous patch wasn't quite right due to cut/paste error and being tricked by an if-statement without braces. Now we're green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4698e1e0817 Sadly, just when I thought I'm finished, another one popped up: Bug 1193293.

Comment on attachment 8661349 [details] [diff] [review] Proposed solution (v4) - Implements option 1) in comment #19 Review of attachment 8661349 [details] [diff] [review]: ----------------------------------------------------------------- I don't think we should change our C++ code here. Use pushPrefEnv.

I agree that changing C++ code in a bug that says "clean up tests" doesn't seem right. However, I think that SetCurrentDictionary() had the additional task "glued" onto it to write stuff to an mozStorage (SQLite) database. You can call SetCurrentDictionary() with an empty string, and it will deselect any dictionary, however, the additional data sticks like glue, you need to do something completely different to get rid of it. That doesn't seem right from a design aspect. Why should I use one interface to do something and have to know about the internals and do two things to undo the action. If you want, please consider this as fixing two bugs: 1) Fix SetCurrentDictionary() to do the right thing when called with an empty string, since it makes no sense to store an empty content preference as it currently does. Preferably clear the stored data in this case. 2) Fix these tests. Look at it some other way: If tomorrow we decide to MozStorage but something else, then we'd have to revisit all the tests. Would you kindly reconsider your decision?

I give you another orthogonality argument: None of the dictionary tests mention content preferences. Instead they do this: 1) Focus an element. 2) Set the dictionary. 3) Reload the page. 4) Check that the setting persisted. They do NOT do: 1) Focus an element. 2) Set the dictionary. 3) Check that the content preference was correctly written. I think a test that went: SetCurrentDictionary("foobar"); should just have a symmetric clean-up action: SetCurrentDictionary(""); To me, this is the cleanest, simplest, safest (and yes, also fastest) solution. Please reconsider.

Alternatively we could create a function ClearCurrentDictionary() which un-selects the dictionary and clears the content preferences.

I think it would be better to have a new method ClearCurrentDictionary which just wipes out the content pref. That's better than having a magic parameter value to SetCurrentDictionary. Tests should use pushPrefEnv to save and restore the spellchecker.dictionary pref.

Please ignore the previous comments. I got confused. Here is a new version of my answer to your comment #15: In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #15) > I think it would be better to have a new method ClearCurrentDictionary which > just wipes out the content pref. That's better than having a magic parameter > value to SetCurrentDictionary. Fair enough. Although this function would only be used in the tests. Do you really want to allow a call of SetCurrentDictionary(""); to store an empty value in the content preferences (mozStorage/SQLite)? To my eye, that doesn't make sense. And no user action can ever cause this. The user can only select a dictionary, there is no UI to select "no dictionary". SetCurrentDictionary() also already clears content preferences, if the chosen dictionary coincides with what the site requires: https://dxr.mozilla.org/mozilla-central/source/editor/composer/nsEditorSpellCheck.cpp#620 (Line numbers will change today due to a landing, look for "Clearing content preferences"). So in fact, there would be little magic to having "" clear the storage ;-) > Tests should use pushPrefEnv to save and restore the spellchecker.dictionary pref. OK, you want me to use SpecialPowers.pushPrefEnv({'clear': [['spellchecker.dictionary']]}, SimpleTest.finish); instead of Services.prefs.setCharPref("spellchecker.dictionary", ""); That won't be necessary: Either I can convince you that SetCurrentDictionary("") will clear the content preference *and* the preference, or we go with ClearCurrentDictionary() to do both actions. Sorry about this lengthy discussion about a trivial issue that will never go in front of the user. Implementing ClearCurrentDictionary() would have taken less time that this discussion. But to me, we should determine what SetCurrentDictionary("") should do. Currently it writes an empty content preference and that to me makes no sense. So I'd like to restate the options: 1) Make SetCurrentDictionary("") clear preference and content preference (my initial proposal). 2) Implement ClearCurrentDictionary() and have it clear both. Leave SetCurrentDictionary("") as is writing an empty content preference. 3) Implement ClearCurrentDictionary() and have it clear both and change SetCurrentDictionary() to reject empty input. 4) Implement ClearCurrentDictionary() and have it clear both and change SetCurrentDictionary() to call ClearCurrentDictionary() when given empty input (in which case we could just do option 1)). I'll gladly implement 1) 3) or 4). I don't like option 2) since I think we should fix SetCurrentDictionary() to do something meaningful when receiving empty input. What do you think?

(In reply to Jorg K (GMT+2) from comment #19) > Sorry about this lengthy discussion about a trivial issue that will never go > in front of the user. Implementing ClearCurrentDictionary() would have taken > less time that this discussion. Code clarity is important whether or not it affects what goes in front of the user. > But to me, we should determine what SetCurrentDictionary("") should do. > Currently it writes an empty content preference and that to me makes no > sense. So I'd like to restate the options: > 1) Make SetCurrentDictionary("") clear preference and content preference (my > initial proposal). > 2) Implement ClearCurrentDictionary() and have it clear both. Leave > SetCurrentDictionary("") as is > writing an empty content preference. > 3) Implement ClearCurrentDictionary() and have it clear both and change > SetCurrentDictionary() to reject > empty input. > 4) Implement ClearCurrentDictionary() and have it clear both and change > SetCurrentDictionary() to call > ClearCurrentDictionary() when given empty input (in which case we could > just do option 1)). > > I'll gladly implement 1) 3) or 4). I don't like option 2) since I think we > should fix SetCurrentDictionary() to do something meaningful when receiving > empty input. Preferably no-one should call SetCurrentDictionary with an invalid dictionary name. So I suggest we add ClearCurrentDictionary, make SetCurrentDictionary assert when called with an empty string or other invalid dictionary name, and fix callers to stop them from doing that.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20) > > 3) Implement ClearCurrentDictionary() and have it clear both and change > > SetCurrentDictionary() to reject empty input. > Preferably no-one should call SetCurrentDictionary with an invalid > dictionary name. So I suggest we add ClearCurrentDictionary, make > SetCurrentDictionary assert when called with an empty string or other > invalid dictionary name, and fix callers to stop them from doing that. Looks like you opted for option 3). I was in the process of implementing this and found this: https://dxr.mozilla.org/mozilla-central/source/editor/txtsvc/nsISpellChecker.h?offset=100#113 /** * Tells the spellchecker to use a specific dictionary. * @param aDictionary a string that is in the list returned * by GetDictionaryList() or an empty string. If aDictionary is * empty string, spellchecker will be disabled. */ NS_IMETHOD SetCurrentDictionary(const nsAString &aDictionary) = 0; So it seems like passing in an empty string is desired behaviour. So we have to options: 1) Leave the old behaviour and fix it, so content preferences are removed. (My initial proposal, see attachment 8661349 [details] [diff] [review]). 2) Go ahead and change the established semantics of SetCurrentDictionary() and add ClearCurrentDictionary(). As far as I can see SetCurrentDictionary() is not called with an empty string in the code base, but changing the behaviour might upset some add-ons. Looking at nsISpellChecker.h I've noticed that I forgot to remove CheckCurrentDictionary() (which got removed as part of bug 1193293). Once you decide which option you prefer, I'll present another patch. Since you mentioned "code clarity", I also suggest to use "TryDictionary()" internally instead of "SetCurrentDictionary()", which then removes the necessity of the RAII object.

Good catch. Let's do option 1.

Thanks. As I said: I'll submit another patch with some more clean-up (remove CheckCurrentDictionary() from nsISpellChecker.h, switch over to using TryDictiornay() internally and remove the RAII trickery). Coming up, you'll see it.

Changed my mind. Doing the clean-up I mentioned, I realised that the fix in bug 1193293 wasn't right. So I had that backed out. Any further clean-up will be done in there. So we go with this "as is": 1) Fix SetCurrentDictionary() to do the right thing when given an empty string. 2) Fix the tests.

Dear Sheriff, Please land bug 1193293 and bug 1204540 together. Please apply the patch from bug 1193293 first.