826251 - Make testPreferences/testPreferredLanguage.js language independent

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P2)

Description

[Andrei Eftimie]

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

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 2

[Andrei Eftimie]

Attachment: patch v1

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 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 4

[Andrei Eftimie]

Attachment: patch v2

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 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 6

[Andrei Eftimie]

Attachment: patch v3

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 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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?

Comment 8

[Andrei Eftimie]

Attachment: patch v4

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 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 10

[Andrei Eftimie]

Attachment: Testcase for regular expressions

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

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 12

[Andrei Eftimie]

Attachment: patch v5

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

Updated the code.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 14

[Andrei Eftimie]

Attachment: patch v6

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

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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

Comment 16

[Andrei Eftimie]

Attachment: patch v7

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

Comment 17

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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

Comment 18

[Andrei Eftimie]

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

Comment 19

[Andrei Eftimie]

Attachment: patch v8

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.

Comment 20

[Andrei Eftimie]

Attachment: patch v8.1

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

Comment 21

[Andreea Matei [:AndreeaMatei]]

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.

Comment 22

[Andrei Eftimie]

Attachment: patch v8.2

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

Comment 23

[Andreea Matei [:AndreeaMatei]]

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 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 25

[Andrei Eftimie]

Attachment: patch v8.3

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)

Comment 26

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(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

Comment 27

[Andrei Eftimie]

Attachment: v8.4

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.

Comment 28

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Andrei Eftimie from comment #27)
> 
> Thanks for the idea Henrik!

Yeah, nice one Henrik! ;)

Comment 29

[Andreea Matei [:AndreeaMatei]]

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? :)

Comment 30

[Andrei Eftimie]

Attachment: patch v8.5

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

Comment 31

[Andreea Matei [:AndreeaMatei]]

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.

Comment 32

[Andrei Eftimie]

Attachment: patch v8.6

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

Comment 33

[Andreea Matei [:AndreeaMatei]]

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

Comment 34

[Andrei Eftimie]

Attachment: patch v8.7

Fixed last nit

Comment 35

[Andreea Matei [:AndreeaMatei]]

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)

Comment 36

[Andrei Eftimie]

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

Comment 37

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 38

[Andrei Eftimie]

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.

Comment 39

[Andreea Matei [:AndreeaMatei]]

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.

Comment 40

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 41

[Andrei Eftimie]

Attachment: followUp patch v1

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.

Comment 42

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 43

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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 44

[Andreea Matei [:AndreeaMatei]]

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.

Comment 45

[Andrei Eftimie]

Attachment: Patch v9

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.

Comment 46

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 47

[Andrei Eftimie]

Filed bug 852116 for the controller.select() problem.

- Using controller.click() is shielding us from this, as it calls scrollIntoView() internally.

Comment 48

[Andreea Matei [:AndreeaMatei]]

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)

Comment 49

[Andrei Eftimie]

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

Should be safe to transplant.

Comment 50

[Andrei Eftimie]

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

Comment 51

[Andrei Eftimie]

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.

Comment 52

[Andreea Matei [:AndreeaMatei]]

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

Comment 53

[Andrei Eftimie]

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

Comment 54

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Please provide some details of the failure.

Comment 55

[Andrei Eftimie]

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

Comment 56

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 57

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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"

Comment 58

[Andrei Eftimie]

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

Comment 59

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 60

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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

Comment 61

[Andrei Eftimie]

Attachment: Patch v9.1

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

Comment 62

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 63

[Andrei Eftimie]

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.

Comment 64

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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

Comment 65

[Andrei Eftimie]

Attachment: Patch v9.2

Updated patch to use getComplexValue()

Comment 66

[Andreea Matei [:AndreeaMatei]]

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.

Comment 67

[Andrei Eftimie]

Attachment: Patch v9.3

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 68

[Andreea Matei [:AndreeaMatei]]

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?

Comment 69

[Andrei Eftimie]

Attachment: Patch v9.4

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

[Andreea Matei [:AndreeaMatei]]

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)

Comment 71

[Andrei Eftimie]

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

Comment 72

[Andreea Matei [:AndreeaMatei]]

I would like to see reports please. Thanks.

Comment 73

[Andrei Eftimie]

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

Comment 74

[Andrei Eftimie]

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

Comment 75

[Andreea Matei [:AndreeaMatei]]

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)