This mochitest is failing, because spellchecking is disabled for Fennec. I guess the best solution is to adjust the testcase in such a way that it detects if spellchecking is enabled, and when it isn't, that it doesn't carry out the tests at all.

This fixes it. I've tested and it seems to work.

I get a "todo | [[Simpletest.report()]] No checks actually run" message, though. It might be that Serge considers this bad practice, considering bug 496443. Serge, how should I avoid that message?

Comment on attachment 512831 [details] [diff] [review] patch this patch looks fine. Does it work with e10s (browser.tabs.remote=true)? After looking at the test file, it seems that we are basically doing nothing for this test if spellcheck is not on. Can we turn on spell check inside of fennec? Would we want to remove this file in general from a mobile build inside the makefile? I just want to make sure we solve the problem the right way, and if editing the test case itself (which I think is a good way to do it) is the approach we want, then this patch is good. I would add something in the log file to indicate we are skipping this test because spellcheck is not turned on if we do skip it.

(In reply to comment #3) > this patch looks fine. Does it work with e10s (browser.tabs.remote=true)? Yes, this is on Windows, though. (I'm afraid I'm still stuck on the Tegra board) > After looking at the test file, it seems that we are basically doing nothing > for this test if spellcheck is not on. Yes, that's what I mentioned in comment 2. > Can we turn on spell check inside of fennec? Would we want to remove this file > in general from a mobile build inside the makefile? Afaik, spellchecking can only be turned on for Fennec, when doing that in about:config. My guess is that at one point, this is getting turned on again in Fennec. At that point, with this patch, the test would automatically work again. With removing this file in the Makefile, you would have to remember to readd this file. > I just want to make sure we solve the problem the right way, and if editing the > test case itself (which I think is a good way to do it) is the approach we > want, then this patch is good. I would add something in the log file to > indicate we are skipping this test because spellcheck is not turned on if we do > skip it. Yeah, I was just asking Serge in comment 2 about it. I'll see what he writes about it and write a better fix if he has a good suggestion.

Same as previous patch, except with is() replaced by todo() and ok() ;-)

Comment on attachment 512860 [details] [diff] [review] (Av2) Use todo() when spellCheck is turned off cool! this addresses my concern from the previous patch.

Thanks, Serge!

While here...

Sorry, there are probably some other tests that need the same treatment. I'll try to change those too, tomorrow: /editor/libeditor/html/tests/test_bug484181.html (View Hg log or Hg annotations) /editor/libeditor/html/tests/test_bug366682.html (View Hg log or Hg annotations) /editor/libeditor/html/tests/test_bug432225.html (View Hg log or Hg annotations) /editor/libeditor/text/tests/test_bug596333.html (View Hg log or Hg annotations)

Comment on attachment 512950 [details] [diff] [review] (Bv1) s/CR+LF/LF/g, Remove ending whitespaces hmm, this won't apply with the other patch. It is just a line ending cleanup for the existing file. I don't see anything wrong with the patch, just that it will require a new patch for the isspelling todo patch earlier. Can we make this patch as a second in a series or a superset?

Comment on attachment 512950 [details] [diff] [review] (Bv1) s/CR+LF/LF/g, Remove ending whitespaces (In reply to comment #10) > Can we make this patch as a second in a series or a superset? My intent is to land this patch (just) before an updated Av2 patch.

Comment on attachment 512950 [details] [diff] [review] (Bv1) s/CR+LF/LF/g, Remove ending whitespaces ok great.

This fixes the instances mentioned in comment 9.

Comment on attachment 513080 [details] [diff] [review] patchv2 r- because this makes the test pass if the pref gets disabled for Firefox, which we don't want.

Why not?

Comment on attachment 512950 [details] [diff] [review] (Bv1) s/CR+LF/LF/g, Remove ending whitespaces The landing of this patch made me *really* sad. These types of patches effectively ruin the blame history, are useless, and should not land without going through the proper review requirements. I'll r- to express my discontent here, but I guess it's too late to fix this now. :(

(In reply to comment #15) > Why not? Because we want to catch this type of breakage in Firefox. ie., if somebody makes that pref value false, we want the unit tests to fail.

Comment on attachment 512860 [details] [diff] [review] (Av2) Use todo() when spellCheck is turned off r- because of the mentioned reason.

(In reply to comment #17) > (In reply to comment #15) > > Why not? > > Because we want to catch this type of breakage in Firefox. ie., if somebody > makes that pref value false, we want the unit tests to fail. Why would you want the unit tests to fail in that case?

Ehsan, In Fennec these tests fail and have the pref off. All automation is run with a clean fresh profile including all the hard requirements. If we decide to run in the wild, the tests should be smart enough not to throw a lot of errors just because some settings were tweaked. Is there something else I am overlooking?

(In reply to comment #14) > r- because this makes the test pass if the pref gets disabled for Firefox, > which we don't want. Why don't you add a Firefox specific test to check the pref value, if that's explicitly what you want to check? Forcing Core tests to depend on Firefox likings isn't good at all (for all other apps), I think.

(In reply to comment #19) > (In reply to comment #17) > > (In reply to comment #15) > > > Why not? > > > > Because we want to catch this type of breakage in Firefox. ie., if somebody > > makes that pref value false, we want the unit tests to fail. > > Why would you want the unit tests to fail in that case? Because we want to catch the case where someone changes the default value for that pref in Firefox. We want the tests to fail in that case.

(In reply to comment #20) > Ehsan, > > In Fennec these tests fail and have the pref off. All automation is run with a > clean fresh profile including all the hard requirements. If we decide to run > in the wild, the tests should be smart enough not to throw a lot of errors just > because some settings were tweaked. > > Is there something else I am overlooking? Yes, I understand. The right solution is to detect whether we're in Fennec and expect the default value of the pref to be false if the test is run in Fennec.

(In reply to comment #21) > (In reply to comment #14) > > r- because this makes the test pass if the pref gets disabled for Firefox, > > which we don't want. > > Why don't you add a Firefox specific test to check the pref value, if that's > explicitly what you want to check? The owner of this bug can do that. If we do have such a Firefox specific test, then my point no longer applies, but that's not really needed (see comment 23). > Forcing Core tests to depend on Firefox likings isn't good at all (for all > other apps), I think. Nobody is talking about Firefox specific things. We want every Gecko based app to support spell checking unless there is a good reason to the contrary. Fennec has one I assume, so it should be exempted explicitly. Not the other way around.

I chatted with Ehsan on irc about this. The solution we came up is that in the "if (!spellcheckOn)" check, we add another check that checks if Fennec is running, and if Fennec is running, you get the todo() result, otherwise you get a fail result (for any other Gecko based application).

I know you guys already decided this, but I think there should be a separate test in browser (i.e. Firefox specific) that makes sure the default pref is whatever we want it to be for Firefox.

there has been random talk of letting end users run mochitests on their systems and if they have a pref misconfigured it will be hard to tell what is going on. This scenario is probably a bigger issue, I am just glad we are making some progress to get more tests green (or not red) on Fennec.

(In reply to comment #26) > I know you guys already decided this, but I think there should be a separate > test in browser (i.e. Firefox specific) that makes sure the default pref is > whatever we want it to be for Firefox. Firefox is not the exception here. Fennec is. So on the second thought, we shouldn't do *anything* specific to Firefox here. See comment 25 about what needs to happen here.

(In reply to comment #27) > there has been random talk of letting end users run mochitests on their systems > and if they have a pref misconfigured it will be hard to tell what is going on. FWIW, this is not the goal of this bug at all, and if some day we get there, we should discuss that part in its own bug. Hint: getting there requires *lots* of effort, as our tests make very specific assumptions which are only going to be true in the test suite environment.

(In reply to comment #28) > (In reply to comment #26) > > I know you guys already decided this, but I think there should be a separate > > test in browser (i.e. Firefox specific) that makes sure the default pref is > > whatever we want it to be for Firefox. > > Firefox is not the exception here. Fennec is. So on the second thought, we > shouldn't do *anything* specific to Firefox here. See comment 25 about what > needs to happen here. my reasoning is that the default for a given pref is an app level preference and should not be enforced in platform tests

(In reply to comment #30) > (In reply to comment #28) > > (In reply to comment #26) > > > I know you guys already decided this, but I think there should be a separate > > > test in browser (i.e. Firefox specific) that makes sure the default pref is > > > whatever we want it to be for Firefox. > > > > Firefox is not the exception here. Fennec is. So on the second thought, we > > shouldn't do *anything* specific to Firefox here. See comment 25 about what > > needs to happen here. > > my reasoning is that the default for a given pref is an app level preference > and should not be enforced in platform tests No, we have platform level prefs which apps can _override_. Spellchecking is enabled by default in core: <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/base/nsEditor.cpp#376>. Fennec disables it: <http://mxr.mozilla.org/mobile-browser/source/app/mobile.js#192>. So, any application which is not Fennec will get spellchecking by default, so I don't want to special case anything for Firefox.

(In reply to comment #31) > No, we have platform level prefs which apps can _override_. Spellchecking is > enabled by default in core: > <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/base/nsEditor.cpp#376>. Why is Firefox setting the "layout.spellcheckDefault" then? http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#631 Afaict, it shouldn't be doing that, should it? Since it is already set on a platform level? I'm trying to find out where the decision was made that this should be turned on at the platform level. I found bug 302050, comment 29 where the developers where leaning on turning it off at the platform level. Could anybody give me a link of a page that describes this should have been turned on at the platform level?

(In reply to comment #32) > (In reply to comment #31) > > No, we have platform level prefs which apps can _override_. Spellchecking is > > enabled by default in core: > > <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/base/nsEditor.cpp#376>. > > Why is Firefox setting the "layout.spellcheckDefault" then? > http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#631 > Afaict, it shouldn't be doing that, should it? Since it is already set on a > platform level? It's just setting it to the default value. Why does it matter? > I'm trying to find out where the decision was made that this should be turned > on at the platform level. I found bug 302050, comment 29 where the developers > where leaning on turning it off at the platform level. > Could anybody give me a link of a page that describes this should have been > turned on at the platform level? There is no such page. That's just how the code works right now. Still, I don't see why this is important. What's wrong with just testing this correctly for both Fennec and Firefox?

Ok, never mind, Firefox setting the "layout.spellcheckDefault" pref is probably because it's in the advanced prefs pane: http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/advanced.xul#73 And I guess the decision to turn this on on a platform level has been made at bug 339127, comment 17: " * Editor will enable spellchecking if the user has forced it on, or if the element says it should be on and the user has not forced it off (either locally or globally through layout.spellcheckDefault). Note that checking the element (to get the UA default) required some irritating changes to nsIContent et. al "

I still think that the way to go here would be comment 25...

This is as done as suggested in comment 25. This doesn't work actually, because in Fennec ipc is enabled and this gives an error when querying for nsIXULAppInfo. So what needs to be done in order for this to get working is, you would need a new api that uses specialpowers.js to message to chrome content to query the appInfo.id. That's quite a hassle, I'll try and see if I can get it working. Anyway, the querying for prefs in these mochitests need to be adjusted too, using the specialpowers.js api.

>This doesn't work actually, because in Fennec ipc is enabled and this gives an >error when querying for nsIXULAppInfo. Huh, I wonder if this is something we should fix. Filed bug 640700.

I don't know if this is desirable for specialpowers. Anyway, I would need this to follow the way of comment 25.

Comment on attachment 519904 [details] [diff] [review] add getAppID for specialpowers This looks fine, just two nits: >diff --git a/testing/mochitest/specialpowers/components/SpecialPowersObserver.js b/testing/mochitest/specialpowers/components/SpecialPowersObserver.js >--- a/testing/mochitest/specialpowers/components/SpecialPowersObserver.js >+++ b/testing/mochitest/specialpowers/components/SpecialPowersObserver.js >@@ -80,16 +80,17 @@ SpecialPowersObserver.prototype = { >+ case "SPappinfo": >+ var appinfo = Components.classes["@mozilla.org/xre/app-info;1"] >+ .getService(Components.interfaces.nsIXULAppInfo); >+ return(appinfo.ID); >+ break; You don't need the parentheses in the return line (I know we have them in other lines, they're not necessary there either). >diff --git a/testing/mochitest/specialpowers/content/specialpowers.js b/testing/mochitest/specialpowers/content/specialpowers.js >--- a/testing/mochitest/specialpowers/content/specialpowers.js >+++ b/testing/mochitest/specialpowers/content/specialpowers.js >@@ -115,16 +115,19 @@ var SpecialPowers = { > listener, > false); > }, > isBackButtonEnabled: function(window) { > return !this._getTopChromeWindow(window).document > .getElementById("Browser:Back") > .hasAttribute("disabled") > }, >+ getAppID: function() { >+ return sendSyncMessage('SPappinfo', {})[0]; >+ }, > } Can you add a simple sanity check test for this in http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/test_SpecialPowersExtension.html?force=1 ? Something like isnot("", SpecialPowers.getAppID()), just to make sure that the value gets set. Also, for bonus points you could make this a getter instead, so you could just say "SpecialPowers.AppID", something like: http://hg.mozilla.org/users/tmielczarek_mozilla.com/mq/file/1a5c5c5ecf2f/de-privilege#l171 But that's just an option.

But um, looking at your test usage, testing the App ID seems like the wrong thing to do. I fully disagree with Ehsan here, you shouldn't have to resort to checking the App ID inside of tests. If Ehsan wants a sanity check test that asserts that spell checking is enabled for Firefox, he should write a Mochitest in browser/ that simply checks the pref value and fails if it's not set, not force you to put awful hacks like this into tests.

(In reply to comment #26) > I know you guys already decided this, but I think there should be a separate > test in browser (i.e. Firefox specific) that makes sure the default pref is > whatever we want it to be for Firefox. Yes, this. Why did nobody listen to this?

(In reply to comment #41) > If Ehsan wants a sanity check test that asserts that spell checking is enabled > for Firefox, he should write a Mochitest in browser/ that simply checks the > pref value and fails if it's not set, not force you to put awful hacks like > this into tests. If I understand correctly, Ehsan wants a sanity check test that asserts that spell checking is not disabled for toolkit applications other than Firefox. I guess I could do that check in browser/base/content/test/ instead. But I would still need the SpecialPowers.getAppID() code for that.

OK, let's reach a middle ground here. Let's get a test in editor/libeditor/base/tests which asserts that the pref value is true, and just that. Then exclude that test from Fennec in Makefile.in (don't use the app ID), so that any Gecko application except for Fennec would run it, and then modify the rest of the spellchecking tests to check layout.spellcheckDefault, and just bail out if it's false. Also, there are a lot of spellchecker tests in http://mxr.mozilla.org/mozilla-central/source/layout/reftests/editor/ too. Although I'm not sure if you care about reftests at this point.

Ehsan, at the root of it, the pref is an app preference and should only be tested by app specific tests. The platform should not be testing for the pref.

(In reply to comment #45) > Ehsan, at the root of it, the pref is an app preference and should only be > tested by app specific tests. The platform should not be testing for the pref. Please see comment 31. The default platform behavior is to spell check editable fields, and I want our tests to fail if somebody breaks that. I also want to do that as cleanly as possible. I'm open to suggestions if you think there's a better way to ensure that than what I suggested in comment 44.

I guess this with the move to use SpecialPowers, these tests need to move to chrome tests, probably.

(In reply to comment #47) > I guess this with the move to use SpecialPowers, these tests need to move to > chrome tests, probably. I agree.

I haven't tested this with, because my build is outdated (still had the packed.js thing in there, I already tested with the packed.js thing, it worked fine then, so I'm confident this is working also, but I'll have to retest). I guess the first suggestion in comment 44 still has to be done, right?

Ok, this adds the layout.spellcheckDefault pref test. This works fine in Firefox mochitest and mochitest-chrome, I haven't tested on Fennec yet (they should not run at all, there).

Comment on attachment 556843 [details] [diff] [review] move test_bug36682 to mochitest chrome , added layout.spellcheckDefault pref test >+SimpleTest.waitForExplicitFinish(); >+addLoadEvent(runTest); >+ >+function runTest() >+{ >+ is(SpecialPowers.getIntPref("layout.spellcheckDefault"), 1, "Check if the layout.spellcheckDefault pref is turned on"); >+ SimpleTest.finish(); >+} No need to wait for the load event, merely the |is| call should be enough. Looks great, r=me with this nit addressed. Thanks!

With nit of comment 51 addressed.

Missing commit message / author, so removing checkin-needed. I'd add them myself, but given the 9 obsolete patches & 50+ comments here, not sure mine would accurately summarise the current approach / reasoning for it, without me having to read all 50 comments. Also might be worth fixing your hgrc so the author gets added automatically. Only takes 10 seconds to change, but saves everyone else lots of time in the long run. Thanks :-)

(In reply to Ed Morley [:edmorley] from comment #53) > Missing commit message / author, so removing checkin-needed. I'm the author, you can add my name, thanks. > I'd add them myself, but given the 9 obsolete patches & 50+ comments here, > not sure mine would accurately summarise the current approach / reasoning > for it, without me having to read all 50 comments. Ok, brief summary: test_bug366682.html is failing on Fennec, because spellchecking is disabled there. There were various solutions to the problem: - Disable the test from inside the Makefile.in for Fennec - Fix the test in such a way that it isn't run for agents that don't have the spellchecking pref set. - Move the test to chrome tests and add a check that checks that the spellhecking pref is turned on and disable that test for Fennec. After much debate, we opted for the 3rd solution. However, this is not the optimal solution. It was the middleground that was reached. I'll file a follow-up bug for the optimal solution, as soon as this bug is fixed (although, I have to admit, I don't know what the optimal solution is myself). > Also might be worth fixing your hgrc so the author gets added automatically. > Only takes 10 seconds to change, but saves everyone else lots of time in the > long run. How do I fix my hgrc? > Thanks :-) Thank you for checking in!

> How do I fix my hgrc? [ui] username = First Middle Last <email> (or whatever string you want to show up as author). Also: [defaults] qnew = -U so that all mq patches automatically add the author bit. For the checkin comment, I recommend either putting it directly in the patch (using hg qref) or explicitly putting the text you want as the checkin comment in the bug.

More precisely, hg qnew -e will let you edit the commit message.

Or hg qref -e, yes.

Ok, I guess this is what you want, I found this patch inside: C:\mozilla-build\fennec_trunk\.hg\patches And I believe this is the format that is wanted.

In my queue with a few other bits that are being sent to try first and then onto inbound :-)

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=87ba0b8cbf99 https://hg.mozilla.org/integration/mozilla-inbound/rev/b063f02ccd43

Thanks for the patience, everyone.