As stated in bug 812435, we should: > read-out the preferences in setupModule to figure out the currently set languages for > content. When we add a new language we could select a random one from the list which is > different from the list retrieved in setupModule().

Please do not forget to set the status of the bug to assigned when you want to work on a bug. Thanks.

I've refactored the test to pick a random language other then the locale, and use that. I have hardcoded the language list, which I don't particularly like, but I have not found a way of accessing all available languages from Mozmill. Does anyone have an idea how to do this?

Comment on attachment 702826 [details] [diff] [review] patch v1 Review of attachment 702826 [details] [diff] [review]: ----------------------------------------------------------------- Given by the state of the test this is feedback not review. Just two things to note... ::: tests/functional/testPreferences/testPreferredLanguage.js @@ +20,5 @@ > + "os","pa","pi","pl","ps","pt","qu","rm","rn","ro","ru","rw","sa","sc","sd", > + "se","sg","si","sk","sl","sm","sn","so","son","sq","sr","ss","st","su","sv", > + "sw","ta","te","tg","th","ti","tig","tk","tl","tlh","tn","to","tr","ts","tt", > + "tw","ty","ug","uk","ur","uz","ve","vi","vo","wa","wen","wo","xh","yi","yo", > + "za","zh","zu"]; There is no need to hard-code all the locales. Instead retrieve all the values from the drop down in the content preference window. @@ +37,5 @@ > /** > * Choose your preferred language for display > */ > var testSetLanguages = function () { > + // Call preferences dialog and set a primary language, other than the browserlocale The first part of the comment we don't need. I think it's clear when you check the next line of code. @@ +77,5 @@ > */ > var langHandler = function(controller) { > + > + // Get a random primary language > + newLanguage = pickRandomLanguage(); There is no need for an additional method. It's called only once.

Fixed some nits and also: > There is no need to hard-code all the locales. Instead retrieve all the > values from the drop down in the content preference window. Thanks for the idea, I've refactored the code in taking a random language directly from the dropdown menu. > There is no need for an additional method. It's called only once. I am actually recalling this function if the randomly picked item is the same as the browserLocale (the test would fail in this case), so keeping this as a separate helper function makes sense. I've thought about removing the browserLocale from the initial list (which would work with a static list). But as we're using the dropdown now, I am not sure it is even feasible anymore.

Comment on attachment 703328 [details] [diff] [review] patch v2 Review of attachment 703328 [details] [diff] [review]: ----------------------------------------------------------------- ::: tests/functional/testPreferences/testPreferredLanguage.js @@ +12,5 @@ > > var setupModule = function(module) { > module.controller = mozmill.getBrowserController(); > module.browserLocale = utils.appInfo.locale; > + module.newLanguage = null; I would call it `contentLocale`. @@ +22,5 @@ > > /** > * Choose your preferred language for display > */ > +var testSetLanguages = function() { Would you mind to change the name to `testChangeContentLanguage`? @@ +31,4 @@ > > + // Verify the primary language is correctly set > + expect.ok(acceptedLanguage.indexOf(newLanguage) === 0, > + "The primary language set is correctly set"); I would like to see it as a regex via `expect.match()`. Also because it would give us way more information in the log string. Further I would say: '.. has been correctly updated'. @@ +35,5 @@ > } > > /** > * Open preferences dialog to switch the primary language > + * @param {MozMillController} controller MozMillController of the window to operate on nit: Please change it to `aController` when you are on it already. @@ +44,2 @@ > > + // Call language dialog and set the primary language nit: I would add 'a new' @@ +54,5 @@ > } > > /** > * Callback handler for languages dialog > + * @param {MozMillController} controller MozMillController of the window to operate on nit: Same as above. @@ +62,5 @@ > var langDropDown = new elementslib.ID(controller.window.document, "availableLanguages"); > controller.waitForElement(langDropDown); > > + // Pick a random language > + var languageList = langDropDown.getNode().childNodes[0].childNodes; I think that's a bit flaky and can cause problems. You might want to use the nodeCollector here with "#availableLanguagesPopup menuitem" as the selector string. @@ +64,5 @@ > > + // Pick a random language > + var languageList = langDropDown.getNode().childNodes[0].childNodes; > + newLanguage = pickRandomLanguage(languageList); > + var language = utils.getProperty("chrome://global/locale/languageNames.properties", newLanguage); Please rename language to languageName to make it clear what it contains. @@ +67,5 @@ > + newLanguage = pickRandomLanguage(languageList); > + var language = utils.getProperty("chrome://global/locale/languageNames.properties", newLanguage); > + > + // Select the language > + for (key in language) { Keep the for loop syntax as it was or make use of forEach. But please don't use that construct because it can cause side-effects. @@ +99,5 @@ > +/** > + * Returns a random language selected from "languageList" that is different from > + * the browserLocale > + * @param {NodeList} languageList Firefox pref list with all available languages > + * @return {string} Language ID string Please use the 'a' prefix for parameters and '@returns'. The languageList should look like: {menuitem[]} aLanguages List of menuitems each with a valid language id. @@ +103,5 @@ > + * @return {string} Language ID string > + */ > +var pickRandomLanguage = function(languageList) { > + var randomPosition = Math.floor(Math.random() * languageList.length); > + var language = languageList[randomPosition].getAttribute("id").split("-")[0]; Why do you split the locale id? 'en' is different from 'en-US' or 'en-GB'. @@ +104,5 @@ > + */ > +var pickRandomLanguage = function(languageList) { > + var randomPosition = Math.floor(Math.random() * languageList.length); > + var language = languageList[randomPosition].getAttribute("id").split("-")[0]; > + return (language !== browserLocale.split("-")[0]) ? language : pickRandomLanguage(languageList); Please split that up into two lines and add a blank line before the return statement. Better would be if we would remove the menuentry for the current browser locale from the list before we do any operation on it. That way we wouldn't have to call this method multiple times recursively.

Implemented all name changes / nits, plus: > @@ +31,4 @@ > > > > + // Verify the primary language is correctly set > > + expect.ok(acceptedLanguage.indexOf(newLanguage) === 0, > > + "The primary language set is correctly set"); > I would like to see it as a regex via `expect.match()`. Also because it > would give us way more information in the log string. Further I would say: > '.. has been correctly updated'. Nice ideea, with this we do see on each iteration which language has been selected. > @@ +62,5 @@ > > var langDropDown = new elementslib.ID(controller.window.document, "availableLanguages"); > > controller.waitForElement(langDropDown); > > > > + // Pick a random language > > + var languageList = langDropDown.getNode().childNodes[0].childNodes; > I think that's a bit flaky and can cause problems. You might want to use the > nodeCollector here with "#availableLanguagesPopup menuitem" as the selector > string. Done > @@ +67,5 @@ > > + newLanguage = pickRandomLanguage(languageList); > + var language = utils.getProperty("chrome://global/locale/languageNames.properties", > newLanguage); > > + > > + // Select the language > > + for (key in language) { > Keep the for loop syntax as it was or make use of forEach. But please don't > use that construct because it can cause side-effects. I've changed that to use a regular for loop. Would you please share what kind of side-effects we could see here? I can't think of any other then going up the prototype-chain, which we could test with item.hasOwnProperty(property) - and I agree this would be overkill in this situation. From experience I would be very weary of using this construct in Trident (or however IE's engine is called nowadays), but Gecko has never posed me any problems. > @@ +103,5 @@ > > + * @return {string} Language ID string > > + */ > > +var pickRandomLanguage = function(languageList) { > > + var randomPosition = Math.floor(Math.random() * languageList.length); > > + var language = languageList[randomPosition].getAttribute("id").split("-")[0]; > > Why do you split the locale id? 'en' is different from 'en-US' or 'en-GB'. We are using this file to get the name of the language: "chrome://global/locale/languageNames.properties" And this file does not have the locales, thus the only result we can get back is the language, so in effect even if our random pick would select en-US, we are still only able to click on "English". ATM we only operate on the core languages, ignoring the locales. To solve this we would need to get the actual name of the locale, which I have not been able to find. I've made some small adjustments to test only against the core languages. (So to not have duplicates for the locales). I've also found one other interesting thing: "English" is always available besides the browserLocale, so I had to filter it out as well. > @@ +104,5 @@ > > + */ > > +var pickRandomLanguage = function(languageList) { > > + var randomPosition = Math.floor(Math.random() * languageList.length); > > + var language = languageList[randomPosition].getAttribute("id").split("-")[0]; > + return (language !== browserLocale.split("-")[0]) ? language : pickRandomLanguage(languageList> ); > Please split that up into two lines and add a blank line before the return > statement. Better would be if we would remove the menuentry for the current > browser locale from the list before we do any > operation on it. That way we > wouldn't have to call this method multiple times recursively. I've removed this completely. Also a passing testrun: http://mozmill-crowd.blargon7.com/#/functional/report/5dfc8c8f9af0c35c0952dab5300601b5

Comment on attachment 703952 [details] [diff] [review] patch v3 Review of attachment 703952 [details] [diff] [review]: ----------------------------------------------------------------- I like the progress and the dynamic in this test. Looks like we will have it shortly. ::: tests/functional/testPreferences/testPreferredLanguage.js @@ +3,5 @@ > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > // Include the required modules > var { assert, expect } = require("../../../lib/assertions"); > +var domUtils = require("../../../lib/dom-utils"); Please import the nodeCollector directly by putting the class name into brackets as we do for all the other imports. @@ +23,5 @@ > > /** > * Choose your preferred language for display > */ > +var testChangeContentLanguage = function() { nit: missing blank between function and opening bracket. @@ +32,4 @@ > > + // Verify the primary language is correctly set > + expect.match(acceptedLanguage, new RegExp("^" + contentLocale), > + "The primary language has been correctly updated") There shouldn't be the need to explicitly create a RegExp object here. Simply pass in the concatenated string and we should be fine. @@ +57,5 @@ > > /** > * Callback handler for languages dialog > * > + * @param {MozMillController} aController MozMillController of the window to operate on nit: In the description just call it 'Controller' @@ +62,2 @@ > */ > +var langHandler = function(aController) { nit: missing blank after function and opening bracket. @@ +67,3 @@ > > + // Get the language list > + var nodeCollector = new domUtils.nodeCollector(aController.window.document); You should be able to already pass in the langDropDown element as the root. That way the node collector will be faster. @@ +72,5 @@ > + var languageNameList = languageList.elements > + // Parse the list into a normal array > + .map(function(self) { > + return self.getNode().getAttribute("id"); > + }) The comment doesn't map to the code you are executing here. Also the wrapping is weird. @@ +73,5 @@ > + // Parse the list into a normal array > + .map(function(self) { > + return self.getNode().getAttribute("id"); > + }) > + // Filter out the browserLocale, english and all locales from the available language pool Not sure what is done here? Can you please explain it? @@ +80,5 @@ > + }); > + > + // Pick a random language > + var randomPosition = Math.floor(Math.random() * languageNameList.length); > + contentLocale = languageNameList[randomPosition]; Do we need the extra randomPosition variable? I think we could save space by giving shorter but still understandable variable names. Also don't forget to declare variables with var. @@ +105,4 @@ > > while (upButton.getNode().getAttribute("disabled") != "true") { > + aController.click(upButton); > + aController.sleep(0); Do we still need this sleep?

Fixed issues and: > @@ +32,4 @@ > > > > + // Verify the primary language is correctly set > > + expect.match(acceptedLanguage, new RegExp("^" + contentLocale), > > + "The primary language has been correctly updated") > > There shouldn't be the need to explicitly create a RegExp object here. Simply > pass in the concatenated string and we should be fine. It actually fails to produce a correct regular expression if I leave it as a string. For example this passes (and it should fail): acceptedLanguage: 'no, en-US, en' contentLocale: 'en' I've reordered the code around a bit to this: > // Parse the list into a normal array > var languageNameFullList = languageList.elements.map(function (self) { > return self.getNode().getAttribute("id"); > }); > > // Filter out the browserLocale, english and all locales from the available language pool > var languageNameList = languageNameFullList.filter(function (self) { > return self !== browserLocale && > self !== 'en' && > self.indexOf('-') === -1; > }); We initially get an array of Mozmill Wrapped objects, we parse this down to a simple array of strings. (eg ["en", "de", "ro" ...] ) We then filter our the values we don't want to pick: 1. the current browserLocale 2. english (as it is always available) 3. all locales (eg. "en-US") -- as the menu in the UI only contains languages, no locales > @@ +80,5 @@ > > + }); > > + > > + // Pick a random language > > + var randomPosition = Math.floor(Math.random() * languageNameList.length); > > + contentLocale = languageNameList[randomPosition]; > > Do we need the extra randomPosition variable? I think we could save space by > giving shorter but still understandable variable names. Also don't forget to > declare variables with var. 1. I've removed the randomPosition variable. 2. We need contentLocale outside of the scope of this function. And as this function does not return anything anywhere (it is invoked in: md) > @@ +105,4 @@ > > > > while (upButton.getNode().getAttribute("disabled") != "true") { > > + aController.click(upButton); > > + aController.sleep(0); > > Do we still need this sleep? I've removed it and it seems to perform just fine without it. Tested on OS X and Win7.

Comment on attachment 706351 [details] [diff] [review] patch v4 Review of attachment 706351 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Andrei Eftimie from comment #8) > > > + expect.match(acceptedLanguage, new RegExp("^" + contentLocale), > > > + "The primary language has been correctly updated") > > > > There shouldn't be the need to explicitly create a RegExp object here. Simply > > pass in the concatenated string and we should be fine. > > It actually fails to produce a correct regular expression if I leave it as a > string. > > For example this passes (and it should fail): > acceptedLanguage: 'no, en-US, en' > contentLocale: 'en' How do you call this? I kinda would like to see the code. > We then filter our the values we don't want to pick: > 1. the current browserLocale > 2. english (as it is always available) Why is english always available? I can't remember that I have seen that e.g. in a German build. > 3. all locales (eg. "en-US") -- as the menu in the UI only contains > languages, > no locales I can clearly see a lot of different locales listed in the menu for English only. As long as there are still outstanding issues please do not upload a new patch and request for review. This only adds extra work. Thanks.

(In reply to Henrik Skupin (:whimboo) from comment #9) > Comment on attachment 706351 [details] [diff] [review] > patch v4 > > Review of attachment 706351 [details] [diff] [review]: > ----------------------------------------------------------------- > > (In reply to Andrei Eftimie from comment #8) > > > > + expect.match(acceptedLanguage, new RegExp("^" + contentLocale), > > > > + "The primary language has been correctly updated") > > > > > > There shouldn't be the need to explicitly create a RegExp object here. Simply > > > pass in the concatenated string and we should be fine. > > > > It actually fails to produce a correct regular expression if I leave it as a > > string. > > > > For example this passes (and it should fail): > > acceptedLanguage: 'no, en-US, en' > > contentLocale: 'en' > > How do you call this? I kinda would like to see the code. I've created a testcase for this, uploaded a patch for you to see it in action. When creating a string it appears the Regular Expression is not correctly created. "^en" => "/(?:)/" new RegExp("^en") => "/^en/" It appears expect.match suffers from some bug and the RegExp is first stringified then recreated. That could be the case... in assertions.js: // XXX Bug 634948 // Regex objects are transformed to strings when evaluated in a sandbox // For now lets re-create the regex from its string representation We might need to fiddle with the expect.match code to make this work. Have not really looked into that to deeply. > > We then filter our the values we don't want to pick: > > 1. the current browserLocale > > 2. english (as it is always available) > > Why is english always available? I can't remember that I have seen that e.g. > in a German build. This is indeed strange. Manually I also only see the default language. However when invoked from Mozmill, en and en-US are always available. I've tested with an Italian, French and Arabic build. To test this, simply open Firefox from Mozmill: > mozmill -b /Application/FirefoxNightly.ar.app The go to Preferences > Content > Languages > Choose I have no ideea why or how these are made available... > > 3. all locales (eg. "en-US") -- as the menu in the UI only contains > > languages, > > no locales > > I can clearly see a lot of different locales listed in the menu for English > only. > > As long as there are still outstanding issues please do not upload a new > patch and request for review. This only adds extra work. Thanks. I've wrongly writtten here that its the UI. It is as I've written in comment 6: We get a localized language name from chrome://global/locale/languageNames.properties and this file only includes the language, not the locale. (So we can only search for "en" and get "English" back, there is no "en-us" there.

Comment on attachment 706377 [details] [diff] [review] Testcase for regular expressions Review of attachment 706377 [details] [diff] [review]: ----------------------------------------------------------------- ::: test_regularExpression.js @@ +10,5 @@ > + */ > +var testRegExp = function () { > + > + // This test should fail > + expect.match("de, en, en-US", "^en", Sorry my fault. I should have seen this earlier. Regular expressions have to be specified in the form `/expression/`. You cannot use a string for it. Once changed it should work as expected.

Thanks Henrik. The regular expression works correctly as a string with: "/^" + myString + "/" Updated the code.

Comment on attachment 708070 [details] [diff] [review] patch v5 Review of attachment 708070 [details] [diff] [review]: ----------------------------------------------------------------- ::: tests/functional/testPreferences/testPreferredLanguage.js @@ +77,5 @@ > + // Filter out the browserLocale, english and all locales from the available language pool > + var languageNameList = languageNameFullList.filter(function (self) { > + return self !== browserLocale && > + self !== 'en' && > + self.indexOf('-') === -1; Given that this hasn't been cleared yet, I'm still against removing 'en' and any other value with a '-' in the id. We should probably only remove those items which are part of the content locale pref and nothing else. @@ +81,5 @@ > + self.indexOf('-') === -1; > + }); > + > + // Pick a random language > + contentLocale = languageNameList[Math.floor(Math.random() * languageNameList.length)]; missing 'var' @@ +88,5 @@ > + var languageName = utils.getProperty("chrome://global/locale/languageNames.properties", > + contentLocale); > + > + // Select the language in the dropdown > + for (var i = 0, l = languageName.length; i < l; i++) { drop 'l' from here please. @@ +106,3 @@ > > while (upButton.getNode().getAttribute("disabled") != "true") { > + aController.click(upButton); Forgot about this before. Can we convert this while loop into a waitFor? Otherwise it might run forever if we fail to move the item up.

(In reply to Henrik Skupin (:whimboo) from comment #13) > Comment on attachment 708070 [details] [diff] [review] > patch v5 > > Review of attachment 708070 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: tests/functional/testPreferences/testPreferredLanguage.js > @@ +77,5 @@ > > + // Filter out the browserLocale, english and all locales from the available language pool > > + var languageNameList = languageNameFullList.filter(function (self) { > > + return self !== browserLocale && > > + self !== 'en' && > > + self.indexOf('-') === -1; > > Given that this hasn't been cleared yet, I'm still against removing 'en' and > any other value with a '-' in the id. We should probably only remove those > items which are part of the content locale pref and nothing else. If we leave "en" in, our test will fail when our random generator picks the English language. ( statistically 1 in 191 runs). English is always available on localized builds. As for the removal of locales, I've already explained this in comment 6 and comment 10: We are typing in a localized name of the language we are selecting, for this we are using chrome://global/locale/languageNames.properties which gives us the localized name of each language. Note that this file does not contain any locale, only the main language. In effect we are unable to select any locale from the dropdown, that's why we filter them out. > @@ +81,5 @@ > > + self.indexOf('-') === -1; > > + }); > > + > > + // Pick a random language > > + contentLocale = languageNameList[Math.floor(Math.random() * languageNameList.length)]; > > missing 'var' This is not a local variable, it needs to be shared with the rest of the module, because are not able to return it from the function itself. It is declared in setup via: module.contentLocale = null; > @@ +88,5 @@ > > + var languageName = utils.getProperty("chrome://global/locale/languageNames.properties", > > + contentLocale); > > + > > + // Select the language in the dropdown > > + for (var i = 0, l = languageName.length; i < l; i++) { > > drop 'l' from here please. Done > @@ +106,3 @@ > > > > while (upButton.getNode().getAttribute("disabled") != "true") { > > + aController.click(upButton); > > Forgot about this before. Can we convert this while loop into a waitFor? > Otherwise it might run forever if we fail to move the item up. Done

Comment on attachment 709675 [details] [diff] [review] patch v6 Review of attachment 709675 [details] [diff] [review]: ----------------------------------------------------------------- Taking the feedback request while Henrik is on PTO. I'd like to see if we could only exclude the default languages, and not all regional languages. ::: tests/functional/testPreferences/testPreferredLanguage.js @@ +76,5 @@ > + > + // Filter out the browserLocale, english and all locales from the available language pool > + var languageNameList = languageNameFullList.filter(function (self) { > + return self !== browserLocale && > + self !== 'en' && It appears that en and en-us are always present for me (tried French and German locales). @@ +77,5 @@ > + // Filter out the browserLocale, english and all locales from the available language pool > + var languageNameList = languageNameFullList.filter(function (self) { > + return self !== browserLocale && > + self !== 'en' && > + self.indexOf('-') === -1; I don't like that we're excluding all languages with a dash. Is this only so we can get the language string for selecting by keypress? If so, I suspect there are other ways we can select our target language. @@ +84,5 @@ > + // Pick a random language > + contentLocale = languageNameList[Math.floor(Math.random() * languageNameList.length)]; > + > + // Get the name of the selected language > + var languageName = utils.getProperty("chrome://global/locale/languageNames.properties", A quick search on MXR and I found the region names too chrome://global/locale/regionNames.properties

I have updated the test so we can use all locales now. We are not checking the localised language through the pref anymore, we are taking them directly from the dropdown. > ::: tests/functional/testPreferences/testPreferredLanguage.js > @@ +76,5 @@ > > + > > + // Filter out the browserLocale, english and all locales from the available language pool > > + var languageNameList = languageNameFullList.filter(function (self) { > > + return self !== browserLocale && > > + self !== 'en' && > > It appears that en and en-us are always present for me (tried French and German locales). That what I have also seen. Also on some locales en-gb is also present (Romanian one for example). To be on the safe-side I have excluded all en locales

(In reply to Andrei Eftimie from comment #16) > > It appears that en and en-us are always present for me (tried French and German locales). > > That what I have also seen. Also on some locales en-gb is also present > (Romanian one for example). > > To be on the safe-side I have excluded all en locales Could we retrieve the already present languages and only exclude those, rather than excluding all en languages?

(In reply to Dave Hunt (:davehunt) from comment #17) > (In reply to Andrei Eftimie from comment #16) > > > It appears that en and en-us are always present for me (tried French and German locales). > > > > That what I have also seen. Also on some locales en-gb is also present > > (Romanian one for example). > > > > To be on the safe-side I have excluded all en locales > > Could we retrieve the already present languages and only exclude those, > rather than excluding all en languages? This might really work! Thanks, will update.

We now only eliminate the actual installed locales! We do this by reading the pref beforehand and eliminating them from our list based on that.

Added the new way of not having globar variables in setupMpdule with the aModule argument.

Comment on attachment 717116 [details] [diff] [review] patch v8.1 Review of attachment 717116 [details] [diff] [review]: ----------------------------------------------------------------- ::: tests/functional/testPreferences/testPreferredLanguage.js @@ +74,4 @@ > > + // Filter out installed locales > + var filtertedLanguageList = languageList.nodes.filter(function (self) { > + return initLang.indexOf(self.getAttribute("id")) === -1 I dumped the length of languageList.nodes.length and then length of filtertedLanguageList and for initLanguages en and en-US, only one gets removed. For es build with initLanguages: es-ES, es, en-US, en, two languages are removed instead of 4.

I have fixed the issue. (uppercase problem in the PREF_ACCEPT_LANG string)

Comment on attachment 718382 [details] [diff] [review] patch v8.2 Review of attachment 718382 [details] [diff] [review]: ----------------------------------------------------------------- Tests well now, it removes the initial languages. I'd like to see Henrik or Dave's thoughts on this as well, please ask review.

Comment on attachment 718382 [details] [diff] [review] patch v8.2 Review of attachment 718382 [details] [diff] [review]: ----------------------------------------------------------------- That looks pretty good! With the mentioned issues addressed and another review from Andreea we seem to be ready for landing. ::: tests/functional/testPreferences/testPreferredLanguage.js @@ +16,5 @@ > + aModule.contentLocale = null; > + > + var intlProperties = prefs.preferences.getPref(PREF_ACCEPT_LANG, "", false); > + var initLangString = utils.getProperty(intlProperties, PREF_ACCEPT_LANG); > + aModule.initLang = initLangString.replace(/ /g,"").toLowerCase().split(","); When do we have spaces which have to be removed? @@ +70,4 @@ > > + // Get the language list > + var collector = new nodeCollector(langDropDown.getNode()); > + var languageList = collector.queryNodes("#availableLanguagesPopup menuitem"); '#availableLanguagesPopup' shouldn't be necessary to specify here. There are no other menuitems under a different child node. @@ +74,4 @@ > > + // Filter out installed locales > + var filtertedLanguageList = languageList.nodes.filter(function (self) { > + return initLang.indexOf(self.getAttribute("id")) === -1 nit: missing trailing semicolon. @@ +84,5 @@ > + contentLocale = filtertedLanguageList[randomPosition].getAttribute("id"); > + > + // Select the language in the dropdown > + aController.click(langDropDown); > + for (var i = 0; i < languageName.length; i++) { Would you mind changing it to a forEach? @@ +103,2 @@ > > + assert.waitFor(function () { nit: kill the empty line.

Addressed Henriks requests. > That looks pretty good! With the mentioned issues addressed and another review > from Andreea we seem to be ready for landing. > > ::: tests/functional/testPreferences/testPreferredLanguage.js > @@ +16,5 @@ > > + aModule.contentLocale = null; > > + > > + var intlProperties = prefs.preferences.getPref(PREF_ACCEPT_LANG, "", false); > > + var initLangString = utils.getProperty(intlProperties, PREF_ACCEPT_LANG); > > + aModule.initLang = initLangString.replace(/ /g,"").toLowerCase().split(","); > > When do we have spaces which have to be removed? The string we get has spaces between the items eg: "ro-ro, ro, en-us, en-gb, en" (Romanian build) or "en-US, en" (US build)

(In reply to Andrei Eftimie from comment #25) > The string we get has spaces between the items eg: > "ro-ro, ro, en-us, en-gb, en" (Romanian build) or > "en-US, en" (US build) If that's so, can't we just split on ', '? Or use a regular expression such as /\s*,\s*/ See https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/String/split#Example:_Removing_spaces_from_a_string

Thanks for the idea Henrik! Have not done a split based on a regular expression until now. Whitespace trim and split done in 1 step now.

(In reply to Andrei Eftimie from comment #27) > > Thanks for the idea Henrik! Yeah, nice one Henrik! ;)

Comment on attachment 719952 [details] [diff] [review] v8.4 Review of attachment 719952 [details] [diff] [review]: ----------------------------------------------------------------- ::: tests/functional/testPreferences/testPreferredLanguage.js @@ +74,3 @@ > > + // Filter out installed locales > + var filtertedLanguageList = languageList.nodes.filter(function (self) { We should have a better parameter name here, used 'a' prefix as well. @@ +84,5 @@ > + contentLocale = filtertedLanguageList[randomPosition].getAttribute("id"); > + > + // Select the language in the dropdown > + aController.click(langDropDown); > + languageName.split("").forEach(function (aChar) { I don't think it's ok to use forEach when referring to a string and turn it into an array. We can do this check similar as in awesome bar tests, by typing the word and waiting for the selected option to match our string. @@ +87,5 @@ > + aController.click(langDropDown); > + languageName.split("").forEach(function (aChar) { > + aController.keypress(langDropDown, aChar, {}); > + aController.sleep(100); > + }) Missing semicolon. Python fan? :)

Thanks for the tip Andreea! Used the controller.type() method.

Comment on attachment 723374 [details] [diff] [review] patch v8.5 Review of attachment 723374 [details] [diff] [review]: ----------------------------------------------------------------- ::: tests/functional/testPreferences/testPreferredLanguage.js @@ +84,5 @@ > + contentLocale = filtertedLanguageList[randomPosition].getAttribute("id"); > + > + // Select the language in the dropdown > + aController.click(langDropDown); > + aController.type(langDropDown, languageName); This tests fine on my computer, but I'm not sure how this will react with fast machines since the method doesn't have a wait call and we have multiple similar items (see "English/"). This is why we used the waitFor in the awesomeBar tests, to make sure the ending result matches the expected one. In this case, the selected item in drop down should match languageName. Could we do the same here? If it selects another item than languageName, there's no way to see it as it is.

We are now checking that the correct value has been selected in the dropDown.

Comment on attachment 724347 [details] [diff] [review] patch v8.6 Review of attachment 724347 [details] [diff] [review]: ----------------------------------------------------------------- One more small change and we're good to land. ::: tests/functional/testPreferences/testPreferredLanguage.js @@ +86,5 @@ > + aController.click(langDropDown); > + aController.type(langDropDown, languageName); > + aController.keypress(langDropDown, "VK_ENTER", {}); > + > + assert.waitFor(function (){ Please add a space here between ) and opening {.

Fixed last nit

Comment on attachment 724863 [details] [diff] [review] patch v8.7 Review of attachment 724863 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, landed as: http://hg.mozilla.org/qa/mozmill-tests/rev/74768c0512fa (default)

We have a failure on Ubuntu 12.04 with a french locale. "TimeoutError: Espagnol/Argentine [es-ar] has been correctly selected - got 'false'" http://mozmill-ci.blargon7.com/#/functional/report/2a6536e9db9f5f44ed48c585107205c8 Investigating to see what went wrong here (other tests, same locale: passed, might be something related to the selected Language))

It could be a race given the fact that we enter the language in one go: aController.type(langDropDown, languageName) I would suggest we still do a loop over the characters and wait for 100ms each. This might help.

It seems the problem is deeper than what we assumed. On a resource constrained machine it fails (about 1 in 2 runs) with - controller.type - controller.type + waitFor the highlighted entry (its not a race condition) - loop over each character + sleep(100) ms From watching the test unfold it appears that when it fails, it starts a new selection halfway (or even multiple ones). eg. for "Somali [so]" if selects "Samoan [sm]" then "Somali [so]" finally "Albanian [sq]". In this example it seems the time between _m_ and _a_ is to big, and it registers as a new word so "al" selects albanian. I can reproduce this issue if I type very slowly. Investigating more to see if I can find some fix or workaround.

Can't we do it like a user would? Go through all dropdown list and find the language needed. In our case a use array.some comparing until we find the languageName.

Just wondering if we can use the controller.select() method instead.

Passes on the resource constrained machine. Ran tests on OS X and WIN7 on all branches (including ESR17). No failures. Instead of typing the name of the language, we simply click on the selected element.

I backed this patch out from default due to a lot of test failures: http://hg.mozilla.org/qa/mozmill-tests/rev/c78357ed548a

Comment on attachment 725369 [details] [diff] [review] followUp patch v1 Review of attachment 725369 [details] [diff] [review]: ----------------------------------------------------------------- How will this work? Is the element you click on and which is hidden, be scrolled into view now? We haven't had this behavior in the past. If that's not the case this code will not work.

Comment on attachment 725369 [details] [diff] [review] followUp patch v1 Review of attachment 725369 [details] [diff] [review]: ----------------------------------------------------------------- Giving the backout this is no longer needed. Please provide a new patch and it would be great if you can test it on the machine where it failed.

Revised patch. Original patch + followup. We click on the relevant language in the dropdown instead of typing its name. (In reply to Henrik Skupin (:whimboo) [away 03/15 - 03/22] from comment #43) > How will this work? Is the element you click on and which is hidden, be > scrolled into view now? We haven't had this behavior in the past. If that's > not the case this code will not work. Mozmill calls the scrollIntoView() method before issuing a click. This would work for most elements since around Firefox 3.X. We had a problem where support for scrollIntoView() for a particular type of element has only recently landed. The MenuList item works very good this way. **** I have also tried to use controller.select() - it might be more stable I thought - but for this particular case we have a pass rate of 44%. 6 out of 10 attempts would fail to select the Language at all. I have not investigated _why_ this is the case.

(In reply to Andrei Eftimie from comment #45) > I have also tried to use controller.select() - it might be more stable I > thought - but for this particular case we have a pass rate of 44%. 6 out of > 10 attempts would fail to select the Language at all. I have not > investigated _why_ this is the case. That should not happen when we select via the option field. If that's really the case file a bug against mozmill if it also happens with 2.0rc.

Filed bug 852116 for the controller.select() problem. - Using controller.click() is shielding us from this, as it calls scrollIntoView() internally.

Comment on attachment 726036 [details] [diff] [review] Patch v9 Review of attachment 726036 [details] [diff] [review]: ----------------------------------------------------------------- Tested with normal and heavy loaded system and it passes. Lets wait for a round of reports before backporting. http://hg.mozilla.org/qa/mozmill-tests/rev/b294bb86106f (default)

No errors in daily runs. Applies cleanly to all branches. Also passes on all branches (OS X). Should be safe to transplant.

Also reports (thanks Mario!): Aurora: http://mozmill-crowd.blargon7.com/#/functional/report/db924715258381629c26ddbad676182f Beta: http://mozmill-crowd.blargon7.com/#/functional/report/db924715258381629c26ddbad67624cc Release: http://mozmill-crowd.blargon7.com/#/functional/report/db924715258381629c26ddbad676323c ESR17: http://mozmill-crowd.blargon7.com/#/functional/report/db924715258381629c26ddbad6764c12

Lots of failures on Windows today. Andreea please back this out. Culprit is a regression in Firefox. (Yesterday's build works perfect, today build fails every time). Will find the responsible changeset, and update the patch on Monday to cope with Firefox's changes.

Backed out unfortunately: http://hg.mozilla.org/qa/mozmill-tests/rev/2999fdf669b4

Responsible pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=0e9badd3cf39

Please provide some details of the failure.

> var acceptedLanguage = prefs.preferences.getPref(PREF_ACCEPT_LANG, ""); Instead of returning the pref string (eg: "de, en-US, en") on Windows it returns "chrome://global/locale/intl.properties". Linux and OSX builds work fine, as well as Windows builds prior to 22nd March.

(In reply to Andrei Eftimie from comment #53) > Responsible pushlog: > http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=0e9badd3cf39 Please use tinderbox builds to nail down the specific changeset. On the other hand I think we have to get the value via a complex value.

I can replicate this on Nightly, Aurora, and Beta on Mac... Components.utils.import("resource://gre/modules/Services.jsm"); branch = Services.prefs.getDefaultBranch(""); branch.getCharPref("intl.accept_languages"); returns: "chrome://global/locale/intl.properties"

(In reply to Henrik Skupin (:whimboo) [away 03/15 - 03/22] from comment #56) > (In reply to Andrei Eftimie from comment #53) > > Responsible pushlog: > > http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=0e9badd3cf39 > > Please use tinderbox builds to nail down the specific changeset. I got that pushlog using tinderbox builds. Need to build Firefox on windows to get more specific (have not yet build FF under windows, I am assuming its more tricky then on *nix systems)

(In reply to Dave Hunt (:davehunt) from comment #57) > Components.utils.import("resource://gre/modules/Services.jsm"); > branch = Services.prefs.getDefaultBranch(""); > branch.getCharPref("intl.accept_languages"); > > returns: "chrome://global/locale/intl.properties" Please test with getComplexValue(), which should return the correct values.

For what it's worth, branch.getComplexValue("intl.accept_languages", Components.interfaces.nsIPrefLocalizedString); works for me.

The actual problem was that we failed to click on the "OK" button in the "Choose Language" window, because the Language Dropdown would remain open in Windows even after we selected the value, obscuring the "OK" button. Language would be selected, added to the list, moved up if needed but the Dropdown would still remain open. Fix: after clicking on the Dropdown option, make another click on the Dropdown main element. Tested on OSX, Windows (7 & XP), Ubuntu. Testruns on OSX: http://mozmill-crowd.blargon7.com/#/functional/report/677f05f4f4af7748bfaf0197ee054a14 WIN7: http://mozmill-crowd.blargon7.com/#/functional/report/677f05f4f4af7748bfaf0197ee04ecd0 ========== In regards to "chrome://global/locale/intl.properties" We get the path to the properties file if there is no user pref set for the specific value we are looking for. Ultimately there was no need to use getComplexValue()

(In reply to Andrei Eftimie from comment #61) > Fix: after clicking on the Dropdown option, make another click on the > Dropdown main element. I'm still not a friend of that. This feels awkward. Why do we have to do another click? That should not be necessary. If it's because the element is not visible by default, we really should consider the select() method. And as mentioned on the other filed bug I can't believe that it doesn't work. Please show me the code you have used to implement the selection for this particular element. > In regards to "chrome://global/locale/intl.properties" > We get the path to the properties file if there is no user pref set for the > specific value we are looking for. > Ultimately there was no need to use getComplexValue() Sorry but I don't understand. For which specific value? We are retrieving a preference at the beginning of the test which never has an user pref. I'm a bit confused.

Updated bug 852116 with relevant info on why controller.select() fails and a pull request on github to fixing the issue in mozmill. For the second part let me rephrase that: If there is no userPref set (we have only the default value) we can't get it from the pref branch. At the begging of the test we do this: > var intlProperties = prefs.preferences.getPref(PREF_ACCEPT_LANG, "", false); > var initLangString = utils.getProperty(intlProperties, PREF_ACCEPT_LANG); dump(intlProperties) // chrome://global/locale/intl.properties dump(initLangString) // en-US, en **I can't say I fully understand how prefs are stored into branches, and how the mechanisms of retrieveing them work; my explanations might not be the most accurate as they are based more on observations.

If you substitute this with getComplexValue() aren't we saving the need to use utils.getProperty?

Updated patch to use getComplexValue()

Comment on attachment 730587 [details] [diff] [review] Patch v9.2 Review of attachment 730587 [details] [diff] [review]: ----------------------------------------------------------------- Clearing review request until we decide which method to be used for the dropdown part.

We decided bug 852116 won't block this one (since that might not land in mozmill 1.5) so we're going ahead with the previous implementation. Added a comment to remind us to update the code to controller.select() once that has been fixed.

Comment on attachment 734622 [details] [diff] [review] Patch v9.3 Review of attachment 734622 [details] [diff] [review]: ----------------------------------------------------------------- I would also like to see reports on all platforms before landing this. Thanks. ::: tests/functional/testPreferences/testPreferredLanguage.js @@ +87,5 @@ > + // XXX: Once bug 852116 lands we should change this code to use > + // controller.select(); > + aController.click(langDropDown); > + assert.waitFor(function () { > + return langDropDown.getNode().open === true; Can't we just return langDropDown.getNode().open here?

> > + assert.waitFor(function () { > > + return langDropDown.getNode().open === true; > > Can't we just return langDropDown.getNode().open here? Yep, removed the superfluous strict comparison. Also testruns: OSX http://mozmill-crowd.blargon7.com/#/functional/report/cb9e33213cf3d2f37d7b0f8dcca4d8d2 Win http://mozmill-crowd.blargon7.com/#/functional/report/8ec48e7ab0431a61b624e36d3100eec7 Linux http://mozmill-crowd.blargon7.com/#/functional/report/cb9e33213cf3d2f37d7b0f8dcca4054c

Comment on attachment 736699 [details] [diff] [review] Patch v9.4 Review of attachment 736699 [details] [diff] [review]: ----------------------------------------------------------------- Great, landed: http://hg.mozilla.org/qa/mozmill-tests/rev/c9cc4d91ce8d (default)

No failures from yesterday. Applies cleanly on all branches, can be transplanted.

I would like to see reports please. Thanks.

Additional reports across branches: ESR17: http://mozmill-crowd.blargon7.com/#/functional/report/8ec48e7ab0431a61b624e36d31d6c602 Release: http://mozmill-crowd.blargon7.com/#/functional/report/8ec48e7ab0431a61b624e36d31d6d2b8 Beta: http://mozmill-crowd.blargon7.com/#/functional/report/8ec48e7ab0431a61b624e36d31eb5d26 Aurora: http://mozmill-crowd.blargon7.com/#/functional/report/8ec48e7ab0431a61b624e36d31ec4cb4

As requested also on win and linux: Windows ======= Aurora: http://mozmill-crowd.blargon7.com/#/functional/report/8ec48e7ab0431a61b624e36d31ef357c Beta: http://mozmill-crowd.blargon7.com/#/functional/report/8ec48e7ab0431a61b624e36d31f07d80 Release: http://mozmill-crowd.blargon7.com/#/functional/report/8ec48e7ab0431a61b624e36d31f23cab ESR17: http://mozmill-crowd.blargon7.com/#/functional/report/8ec48e7ab0431a61b624e36d31f3a82c Linux ===== Aurora: http://mozmill-crowd.blargon7.com/#/functional/report/8ec48e7ab0431a61b624e36d31f02ca6 Beta: http://mozmill-crowd.blargon7.com/#/functional/report/8ec48e7ab0431a61b624e36d31f27ce5 Release: http://mozmill-crowd.blargon7.com/#/functional/report/8ec48e7ab0431a61b624e36d31ff2710 ESR17: http://mozmill-crowd.blargon7.com/#/functional/report/c8094a822ef568b588c5eddaca0338e9

Please make sure to have the latest build when testing. Transplanted: http://hg.mozilla.org/qa/mozmill-tests/rev/0a1ece7373f7 (aurora) http://hg.mozilla.org/qa/mozmill-tests/rev/dde8bfa1f7d3 (beta) http://hg.mozilla.org/qa/mozmill-tests/rev/039fa671319c (release) http://hg.mozilla.org/qa/mozmill-tests/rev/f6d7a972cb54 (esr17)