1308902 - Add localization API for handling entities and properties to Marionette

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

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

We should move the two l10n related methods from Firefox Puppeteer into Marionette to allow handling of localized builds for MarionetteTestCase based tests.

https://dxr.mozilla.org/mozilla-central/source/testing/puppeteer/firefox/firefox_puppeteer/api/l10n.py

I wonder if both methods should even pollute the `self.marionette` instance more or if we get them added by copying the above file and make them available via eg. `self.marionette.l10n.*`.

David, what would you like more? If we want to do the former we might have to adjust the method names especially for `get_property()`.

Comment 1

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

Another option would be to move the whole file and make it not available by default. But tests which have to go through the l10n path need to do the following:

> from marionette_driver.l10n import L10n
> l10n = L10n(Marionette)
> dtd = l10n.get_entity('MENU_FILE_QUIT')
> prop = l10n.get_property('MENU_FILE_QUIT')
> [..]

Comment 2

[David Burns :automatedtester]

These shouldnt hang off the marionette object at all.

Since this is for DTD, wouldnt a name closer to that be much better or is it something we only care about in L10N mode?

Comment 3

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

(In reply to David Burns :automatedtester from comment #2)
> Since this is for DTD, wouldnt a name closer to that be much better or is it
> something we only care about in L10N mode?

DTD entities and string bundles you will only need to retrieve localized content. The former is used directly in XUL files, whereby the latter has to be used in JS code (see StringBundle). We could rename `get_entity` to `get_dtd_entity` but given that there is only one type of entity used in DTD files I don't see a naming conflict. 

Here two examples:

DTD file:
https://dxr.mozilla.org/mozilla-central/source/browser/branding/aurora/locales/en-US/brand.dtd

Properties file:
https://dxr.mozilla.org/mozilla-central/source/browser/branding/unofficial/locales/en-US/brand.properties

David, so comment 1 would be the way to go then?

Comment 4

[David Burns :automatedtester]

yes, comment 1 is the way to go! sorry for not being clear.

Comment 5

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

Thanks David. But before I will get started, how does the integration story with geckodriver looks like? I ask because we also want to have this feature for FoxPuppet which uses Selenium. Andreas, do you mind enlighten me? Thanks.

Comment 6

[Andreas Tolfsen ❲:ato❳]

I agree this should not be exposed directly on the Marionette class.  We don’t want it to become a God object.  The suggested approach above seems fine to me.

(In reply to Henrik Skupin (:whimboo) from comment #5)
> Thanks David. But before I will get started, how does the integration story
> with geckodriver looks like? I ask because we also want to have this feature
> for FoxPuppet which uses Selenium. Andreas, do you mind enlighten me? Thanks.

geckodriver speaks directly to the Marionette server over TCP, so any l10n features you code into the Marionette Python client will not be available to WebDriver clients for as long as they are not part of the protocol.

https://dxr.mozilla.org/mozilla-central/source/testing/puppeteer/firefox/firefox_puppeteer/api/l10n.py seems like a probable candidate to turn into a Marionette server, but it requires careful planning of the API.  We can expose these as ‘extension commands’ in the geckodriver HTTPD:

    https://w3c.github.io/webdriver/webdriver-spec.html#protocol-extensions

Comment 7

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

(In reply to Andreas Tolfsen ‹:ato› from comment #6)
> geckodriver speaks directly to the Marionette server over TCP, so any l10n
> features you code into the Marionette Python client will not be available to
> WebDriver clients for as long as they are not part of the protocol.
> 
> https://dxr.mozilla.org/mozilla-central/source/testing/puppeteer/firefox/
> firefox_puppeteer/api/l10n.py seems like a probable candidate to turn into a
> Marionette server, but it requires careful planning of the API.  We can
> expose these as ‘extension commands’ in the geckodriver HTTPD:
> 
>     https://w3c.github.io/webdriver/webdriver-spec.html#protocol-extensions

So you are saying it's easier for now to first get that feature into the client and let it bake for a while before requesting an addition to the webdriver spec as extension? I can say that we are using those two methods for l10n for years now, also with Mozmill formerly. So all what's included there definitely works.

I would base my work on your judgement in how to best proceed. As mentioned above we need l10n related features over in foxpuppet (https://github.com/mozilla/FoxPuppet), so adding it as extension might be essential.

Comment 8

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) from comment #7)
> So you are saying it's easier for now to first get that feature into the
> client and let it bake for a while before requesting an addition to the
> webdriver spec as extension?

It wouldn’t be added to the spec, but to the Marionette protocol.  As an example of what I mean, I recently moved the on-runtime addon installation code from the Marionette client to the server: https://github.com/mozilla/gecko-dev/blob/master/testing/marionette/addon.js

> I would base my work on your judgement in how to best proceed. As mentioned
> above we need l10n related features over in foxpuppet
> (https://github.com/mozilla/FoxPuppet), so adding it as extension might be
> essential.

I don’t have a strong preference.

Comment 9

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

I talked to Andreas on IRC and here is what has to be done:

* Adding the l10n.js module to Marionette server
* Optionally adding existing tests as xpcshell tests
* Add commands to GeckoDriver.prototype.commands
** Marionette:l10n:get_entity
** Marionette:l10n:get_property
* Update Firefox UI tests to use l10n modules from Marionette and not Puppeteer
* Check if update tests rely on the l10n.py module - if not get it removed.

Comment 10

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) from comment #9)
> * Add commands to GeckoDriver.prototype.commands
> ** Marionette:l10n:get_entity
> ** Marionette:l10n:get_property

Make these GetEntity and GetProperty.

> * Update Firefox UI tests to use l10n modules from Marionette and not
> Puppeteer

Ensure that they retain the old code if they are used for upgrade tests, as the Marionette client needs to retain backwards compatibility.

Comment 11

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

I have a working solution locally, but would like to wait until bug 1313312 has been landed. 

(In reply to Andreas Tolfsen ‹:ato› from comment #10)
> > * Update Firefox UI tests to use l10n modules from Marionette and not
> > Puppeteer
> 
> Ensure that they retain the old code if they are used for upgrade tests, as
> the Marionette client needs to retain backwards compatibility.

I will put the fallback code completely into Puppeteer to not pollute Marionette. Means that I will check if the marionette_driver.l10n module is available. If yes it will be used, and otherwise the current code will be executed.

Comment 12

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

Attachment: Bug 1308902 - Add l10n module and commands to Marionette.

Review commit: https://reviewboard.mozilla.org/r/91390/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/91390/

Comment 13

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

Attachment: Bug 1308902 - Update Puppeteer to use new L10n module of Marionette.

Review commit: https://reviewboard.mozilla.org/r/91392/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/91392/

Comment 14

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

Try build has passing Marionette and fx-ui functional jobs. Locally I tested the changes with a latest Nightly build and Puppeteer falls back to its own implementation, which can be removed when support for Firefox 45ESR ends.

I will go ahead and ask for review by keeping in mind that puppeteer code would need some smaller rebases.

Comment 15

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8808588 [details]
Bug 1308902 - Add l10n module and commands to Marionette.

https://reviewboard.mozilla.org/r/91390/#review91274

::: testing/marionette/client/marionette_driver/l10n.py:6
(Diff revision 1)
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +# You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +
> +class L10n(object):

Please provide documentation.

::: testing/marionette/client/marionette_driver/l10n.py:11
(Diff revision 1)
> +class L10n(object):
> +
> +    def __init__(self, marionette):
> +        self._marionette = marionette
> +
> +    def localize_entity(self, urls, id):

`id` is a [built-in function](https://docs.python.org/2/library/functions.html#id) in Python.  We should avoid overriding it.

::: testing/marionette/client/marionette_driver/l10n.py:16
(Diff revision 1)
> +        :param dtd_urls: A list of dtd files to search.
> +        :param entity_id: The id to retrieve the value from.

Does not match API.

::: testing/marionette/client/marionette_driver/l10n.py:21
(Diff revision 1)
> +        :param dtd_urls: A list of dtd files to search.
> +        :param entity_id: The id to retrieve the value from.
> +
> +        :returns: The localized string for the requested entity.
> +
> +        :raises MarionetteException: When entity id is not found in dtd_urls.

The server returns a `no such element` error.

::: testing/marionette/client/marionette_driver/l10n.py:24
(Diff revision 1)
> +        :returns: The localized string for the requested entity.
> +
> +        :raises MarionetteException: When entity id is not found in dtd_urls.
> +        """
> +        body = {"urls": urls, "id": id}
> +        return self._marionette._send_message("Marionette::l10n::localizeEntity",

Drop `Marionette::`.

::: testing/marionette/client/marionette_driver/l10n.py:33
(Diff revision 1)
> +        :param property_urls: A list of property files to search.
> +        :param property_id: The id to retrieve the value from.

Does not match API.

::: testing/marionette/client/marionette_driver/l10n.py:38
(Diff revision 1)
> +        :raises MarionetteException: When property id is not found in
> +            property_urls.

The server returns a `no such element` error.

::: testing/marionette/client/marionette_driver/l10n.py:42
(Diff revision 1)
> +
> +        :raises MarionetteException: When property id is not found in
> +            property_urls.
> +        """
> +        body = {"urls": urls, "id": id}
> +        return self._marionette._send_message("Marionette::l10n::localizeProperty",

Drop `Marionette::`.

::: testing/marionette/driver.js:2736
(Diff revision 1)
>  
> +/**
> + * Retrieve the localized value of an entity from one or more DTD files.
> + *
> + * Example:
> + *     getLocalizedEntity(["chrome://global/locale/about.dtd"], "about.version")

Does not match API.

::: testing/marionette/driver.js:2739
(Diff revision 1)
> + *
> + * Example:
> + *     getLocalizedEntity(["chrome://global/locale/about.dtd"], "about.version")
> + *
> + * @param {Array.string} urls
> + *        Array of DTD URLs.

Four space indent

::: testing/marionette/driver.js:2741
(Diff revision 1)
> + *     getLocalizedEntity(["chrome://global/locale/about.dtd"], "about.version")
> + *
> + * @param {Array.string} urls
> + *        Array of DTD URLs.
> + * @param {string} id
> + *        The ID of the entity to retrieve the value from.

Four space indent

::: testing/marionette/driver.js:2743
(Diff revision 1)
> + * @param {Array.string} urls
> + *        Array of DTD URLs.
> + * @param {string} id
> + *        The ID of the entity to retrieve the value from.
> + *
> + * @returns {string}

s/returns/return/

::: testing/marionette/driver.js:2745
(Diff revision 1)
> + * @param {string} id
> + *        The ID of the entity to retrieve the value from.
> + *
> + * @returns {string}
> + */
> +GeckoDriver.prototype.localizeEntity = function (cmd, resp) {

No space after `function`

::: testing/marionette/driver.js:2760
(Diff revision 1)
> +
> +/**
> + * Retrieve the localized value of a property from one or more properties files.
> + *
> + * Example:
> + *     getLocalizedProperty(["chrome://global/locale/findbar.properties"], "FastFind")

Does not match API.

::: testing/marionette/driver.js:2767
(Diff revision 1)
> + * @param {Array.string} urls
> + *        Array of properties URLs.
> + * @param {string} id
> + *        The ID of the property to retrieve the value from.
> + *
> + * @returns {string}

s/returns/return/

::: testing/marionette/driver.js:2769
(Diff revision 1)
> + * @param {string} id
> + *        The ID of the property to retrieve the value from.
> + *
> + * @returns {string}
> + */
> +GeckoDriver.prototype.localizeProperty = function (cmd, resp) {

No space after `function`

::: testing/marionette/driver.js:2770
(Diff revision 1)
> + *        The ID of the property to retrieve the value from.
> + *
> + * @returns {string}
> + */
> +GeckoDriver.prototype.localizeProperty = function (cmd, resp) {
> +  if (!Array.isArray(cmd.parameters.urls)) {

Can we assign `urls` and `id` to make the lines shorter?

    let {urls, id} = cmd.parameters;

::: testing/marionette/driver.js:2865
(Diff revision 1)
> +  "Marionette::l10n::localizeEntity": GeckoDriver.prototype.localizeEntity,
> +  "Marionette::l10n::localizeProperty": GeckoDriver.prototype.localizeProperty,

Drop `Marionette` prefix as this is (for stupid reasons) added implicitly.

Also use a single colon to denote namespaces, which seems to be the norm elsewhere in Gecko.

::: testing/marionette/l10n.js:7
(Diff revision 1)
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;

`Cr` isn’t used.

::: testing/marionette/l10n.js:15
(Diff revision 1)
> +
> +Cu.import("chrome://marionette/content/error.js");
> +
> +this.EXPORTED_SYMBOLS = ["l10n"];
> +
> +this.l10n = {};

Please provide a short explanation for the module.

::: testing/marionette/l10n.js:21
(Diff revision 1)
> +
> +/**
> + * Retrieve the localized value of an entity from one or more DTD files.
> + *
> + * Example:
> + *     getLocalizedEntity(["chrome://global/locale/about.dtd"], "about.version")

Example doesn’t match API.

::: testing/marionette/l10n.js:23
(Diff revision 1)
> + * Retrieve the localized value of an entity from one or more DTD files.
> + *
> + * Example:
> + *     getLocalizedEntity(["chrome://global/locale/about.dtd"], "about.version")
> + *
> + * @param {Array.string} urls

`Array.<string>`

::: testing/marionette/l10n.js:24
(Diff revision 1)
> + *
> + * Example:
> + *     getLocalizedEntity(["chrome://global/locale/about.dtd"], "about.version")
> + *
> + * @param {Array.string} urls
> + *        Array of DTD URLs.

Four space indent.

::: testing/marionette/l10n.js:26
(Diff revision 1)
> + *     getLocalizedEntity(["chrome://global/locale/about.dtd"], "about.version")
> + *
> + * @param {Array.string} urls
> + *        Array of DTD URLs.
> + * @param {string} id
> + *        The ID of the entity to retrieve the value from.

Four space indent.

::: testing/marionette/l10n.js:28
(Diff revision 1)
> + * @param {Array.string} urls
> + *        Array of DTD URLs.
> + * @param {string} id
> + *        The ID of the entity to retrieve the value from.
> + *
> + * @returns {string}

s/returns/return/

::: testing/marionette/l10n.js:35
(Diff revision 1)
> +  let entityLocations = "";
> +  for (let ii = 0; ii < urls.length; ii++) {
> +    entityLocations += `<!ENTITY % dtd_${ii} SYSTEM "${urls[ii]}">%dtd_${ii};`;
> +  }

Replace `entityLocations` with an array that is pushed onto and then joined together with `.join("")`.  This is much faster than string concatentation.

::: testing/marionette/l10n.js:36
(Diff revision 1)
> +  for (let ii = 0; ii < urls.length; ii++) {
> +    entityLocations += `<!ENTITY % dtd_${ii} SYSTEM "${urls[ii]}">%dtd_${ii};`;
> +  }

s/ii/i/ or use `Array.prototype.forEach` which provides the index as the second argument IIRC.

::: testing/marionette/l10n.js:41
(Diff revision 1)
> +  for (let ii = 0; ii < urls.length; ii++) {
> +    entityLocations += `<!ENTITY % dtd_${ii} SYSTEM "${urls[ii]}">%dtd_${ii};`;
> +  }
> +
> +  // Use the DOM parser to resolve the entity and extract its real value
> +  let parser = Cc["@mozilla.org/xmlextras/domparser;1"].createInstance(Ci.nsIDOMParser);

Make this lazily loaded through [`XPCOMUtils.defineLazyServiceGetter`](https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/XPCOMUtils.jsm#defineLazyServiceGetter).

::: testing/marionette/l10n.js:44
(Diff revision 1)
> +
> +  // Use the DOM parser to resolve the entity and extract its real value
> +  let parser = Cc["@mozilla.org/xmlextras/domparser;1"].createInstance(Ci.nsIDOMParser);
> +  let header = `<?xml version="1.0"?><!DOCTYPE elem [${entityLocations}]>`;
> +  let elem = `<elem id="elementID">&${id};</elem>`;
> +  let doc = parser.parseFromString(header + elem, 'text/xml');

Double quotes.

::: testing/marionette/l10n.js:52
(Diff revision 1)
> +  if (element === null) {
> +    throw new NoSuchElementError(`Entity with id='${id}' hasn't been found`);
> +  }
> +
> +  return element.textContent;
> +}

Missing `;`.

::: testing/marionette/l10n.js:58
(Diff revision 1)
> +
> +/**
> + * Retrieve the localized value of a property from one or more properties files.
> + *
> + * Example:
> + *     getLocalizedProperty(["chrome://global/locale/findbar.properties"], "FastFind")

This example doesn’t match the API.

::: testing/marionette/l10n.js:60
(Diff revision 1)
> + * Retrieve the localized value of a property from one or more properties files.
> + *
> + * Example:
> + *     getLocalizedProperty(["chrome://global/locale/findbar.properties"], "FastFind")
> + *
> + * @param {Array.string} urls

`Array.<string>`

::: testing/marionette/l10n.js:61
(Diff revision 1)
> + *
> + * Example:
> + *     getLocalizedProperty(["chrome://global/locale/findbar.properties"], "FastFind")
> + *
> + * @param {Array.string} urls
> + *        Array of properties URLs.

Four space indent.

::: testing/marionette/l10n.js:63
(Diff revision 1)
> + *     getLocalizedProperty(["chrome://global/locale/findbar.properties"], "FastFind")
> + *
> + * @param {Array.string} urls
> + *        Array of properties URLs.
> + * @param {string} id
> + *        The ID of the property to retrieve the value from.

Four space indent.

::: testing/marionette/l10n.js:65
(Diff revision 1)
> + * @param {Array.string} urls
> + *        Array of properties URLs.
> + * @param {string} id
> + *        The ID of the property to retrieve the value from.
> + *
> + * @returns {string}

s/returns/return/

::: testing/marionette/l10n.js:70
(Diff revision 1)
> + * @returns {string}
> + */
> +l10n.localizeProperty = function(urls, id) {
> +  let property = null;
> +
> +  urls.some(url => {

I find `Array.prototype.some` a bit misleading in this case.  Can it not just iterate either with `for…of`  or `Array.prototype.forEach`?

::: testing/marionette/l10n.js:75
(Diff revision 1)
> +    }
> +    catch (ex) { }

Bring `catch` up to the line of `}`.

s/ex/e

No space between `{ }`.

::: testing/marionette/l10n.js:84
(Diff revision 1)
> +  if (property === null) {
> +    throw new NoSuchElementError(`Property with id='${id}' hasn't been found`);
> +  }
> +
> +  return property;
> +}

Missing `;`.

Comment 16

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8808589 [details]
Bug 1308902 - Update Puppeteer to use new L10n module of Marionette.

https://reviewboard.mozilla.org/r/91392/#review91290

::: testing/puppeteer/firefox/firefox_puppeteer/api/l10n.py:5
(Diff revision 1)
> +# -----------------
> +# DEPRECATED module
> +# -----------------
> +# Replace its use in tests when Firefox 45 ESR support ends with marionette_driver.l10n.L10n
> +

Can you include this note in the ReadTheDocs for Puppeteer?

Comment 17

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

Comment on attachment 8808588 [details]
Bug 1308902 - Add l10n module and commands to Marionette.

https://reviewboard.mozilla.org/r/91390/#review91274

Thanks for the review! There two issues left I would like to get further feedback from you. Once done I will upload a new version of the patch.

> Drop `Marionette::`.

So that was actually the proposal you gave before. Means we do not really want to integrate the name of the harness here. I can remove it for sure.

> No space after `function`

Here we actually should add a space given that without a space it means there is a function call for a function with the name function. But given that this file doesn't use a space for no other method, I will make this change. We should consider to update this overall.

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#JavaScript

> Please provide a short explanation for the module.

I tried to make it similar to the addons.js module. But I can add one for sure, which will be a simply copy from the Python file.

> s/ii/i/ or use `Array.prototype.forEach` which provides the index as the second argument IIRC.

I simply copied our code from fx-ui-tests which came from Mozmill before. So yes, this is indeed a better method nowaday.

> Make this lazily loaded through [`XPCOMUtils.defineLazyServiceGetter`](https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/XPCOMUtils.jsm#defineLazyServiceGetter).

Why that? We need this immediately, so I don't see why it has to be lazily loaded.

> I find `Array.prototype.some` a bit misleading in this case.  Can it not just iterate either with `for…of`  or `Array.prototype.forEach`?

`forEach()` won't work here because there is no way to break out of the loop earlier. But `for..of` is definitely the better way. I should not have only copied the code from the old codebase.

Comment 18

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

Comment on attachment 8808589 [details]
Bug 1308902 - Update Puppeteer to use new L10n module of Marionette.

https://reviewboard.mozilla.org/r/91392/#review91290

> Can you include this note in the ReadTheDocs for Puppeteer?

Oh, that is a great idea! I will do.

Comment 19

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

Comment on attachment 8808588 [details]
Bug 1308902 - Add l10n module and commands to Marionette.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91390/diff/1-2/

Comment 20

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

Comment on attachment 8808589 [details]
Bug 1308902 - Update Puppeteer to use new L10n module of Marionette.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91392/diff/1-2/

Comment 21

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

Andreas, I already pushed a new version of the patch. This time I accounted in that L10n will not be the only option for us in the future. L20n might be coming soon. So naming the module localization.py for the client makes sense. Please let me know if you already want that for the server too and if yes how the function names in driver.js should look like. If the current code is fine for now we could do the server part later when L20n is active for some of our UI in Firefox. For now I feel we should have set the correct command, and this is done now.

Comment 22

[Andreas Tolfsen ❲:ato❳]

What is l20n and how does it related to l10n?

Comment 23

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

(In reply to Andreas Tolfsen ‹:ato› from comment #22)
> What is l20n and how does it related to l10n?

It's the new way of localizing Gecko based applications. There was a great talk in London during the last all hands. Please have a look at this blog post: https://blog.mozilla.org/l10n/2016/06/29/l20n-in-firefox-a-summary-for-localizers/. So it might come soon, and that's why I accounted for.

Comment 24

[Andreas Tolfsen ❲:ato❳]

If the APIs of l10n and l20n are different it makes sense to expose them as distinct services and give them unique names.  I don’t think you need to make any changes to your current approach.

Comment 25

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8808589 [details]
Bug 1308902 - Update Puppeteer to use new L10n module of Marionette.

https://reviewboard.mozilla.org/r/91392/#review91278

::: testing/puppeteer/firefox/firefox_puppeteer/api/l10n.py:38
(Diff revision 1)
>          :param dtd_urls: A list of dtd files to search.
>          :param entity_id: The id to retrieve the value from.
>  
>          :returns: The localized string for the requested entity.
>  
>          :raises MarionetteException: When entity id is not found in dtd_urls.

Server returns a `no such element` error.

::: testing/puppeteer/firefox/firefox_puppeteer/api/l10n.py:84
(Diff revision 1)
>          :raises MarionetteException: When property id is not found in
>              property_urls.

Server returns a `no such element` error.

Comment 26

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8808588 [details]
Bug 1308902 - Add l10n module and commands to Marionette.

https://reviewboard.mozilla.org/r/91390/#review91274

> So that was actually the proposal you gave before. Means we do not really want to integrate the name of the harness here. I can remove it for sure.

I forgot that we don’t prefix commands with `Marionette:`.  We only do that for internal IPC messages, see e.g. https://github.com/mozilla/gecko-dev/blob/master/testing/marionette/frame.js#L213.

Thanks for fixing.

> Here we actually should add a space given that without a space it means there is a function call for a function with the name function. But given that this file doesn't use a space for no other method, I will make this change. We should consider to update this overall.
> 
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#JavaScript

But it doesn’t conform to the style used elsewhere under testing/marionette, so the change shouldn’t be made in this patch.

I think it’s a good point how `foo = function(arg) { … }` can be mistaken for a function call though.  I hadn’t considered it, and we should probably change it.

> I simply copied our code from fx-ui-tests which came from Mozmill before. So yes, this is indeed a better method nowaday.

FWIW I’m quite happy to continue using `i`-style for loops, it’s just that `ii` is unfamiliar.

> Why that? We need this immediately, so I don't see why it has to be lazily loaded.

It’s constructed on every invocation of `l10n.localizeEntity`.  If you move it to the top of the module and use `defineLazyServiceGetter` it will only be constructed once and them memoised.

Comment 27

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

Comment on attachment 8808589 [details]
Bug 1308902 - Update Puppeteer to use new L10n module of Marionette.

https://reviewboard.mozilla.org/r/91392/#review91278

> Server returns a `no such element` error.

Hm, strange. I'm sure that I updated that! Thanks.

Comment 28

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

Comment on attachment 8808588 [details]
Bug 1308902 - Add l10n module and commands to Marionette.

https://reviewboard.mozilla.org/r/91390/#review91274

> But it doesn’t conform to the style used elsewhere under testing/marionette, so the change shouldn’t be made in this patch.
> 
> I think it’s a good point how `foo = function(arg) { … }` can be mistaken for a function call though.  I hadn’t considered it, and we should probably change it.

Agreed. And I filed bug 1316975 for that.

> It’s constructed on every invocation of `l10n.localizeEntity`.  If you move it to the top of the module and use `defineLazyServiceGetter` it will only be constructed once and them memoised.

Ah, by moving this into the global scope this proposal makes indeed sence now.

Comment 29

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

Comment on attachment 8808588 [details]
Bug 1308902 - Add l10n module and commands to Marionette.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91390/diff/2-3/

Comment 30

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

Comment on attachment 8808589 [details]
Bug 1308902 - Update Puppeteer to use new L10n module of Marionette.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91392/diff/2-3/

Comment 31

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

Comment on attachment 8808588 [details]
Bug 1308902 - Add l10n module and commands to Marionette.

https://reviewboard.mozilla.org/r/91388/#review92484

::: testing/puppeteer/firefox/firefox_puppeteer/api/l10n.py:17
(Diff revision 3)
>  
> -from marionette_driver.errors import MarionetteException
> +from marionette_driver.errors import (
> +    NoSuchElementException,
> +    UnknownCommandException,
> +)
> +from marionette_driver.l10n import L10n as L10nMarionette

Ups, just noticed that I missed to update the module name here.

Comment 32

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

Comment on attachment 8808589 [details]
Bug 1308902 - Update Puppeteer to use new L10n module of Marionette.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91392/diff/3-4/

Comment 33

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

Just noticed that when we push this we can not release a new version of puppeteer before a marionette_driver package has been released. I talked with Maja and we decided to move out the second part of this patch to a new bug, which will cover all changes for new APIs in Marionette.

Andreas, once this has been landed which code I would have to update to make this available to geckodriver so the l10n feature can be used from Selenium?

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/89e259f133ce
Add l10n module and commands to Marionette. r=ato

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/89e259f133ce

Comment 36

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) from comment #33)
> Andreas, once this has been landed which code I would have to update to make
> this available to geckodriver so the l10n feature can be used from Selenium?

The changes to testing/marionette/*.{js,mn}.

Comment 37

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

(In reply to Andreas Tolfsen ‹:ato› from comment #36)
> The changes to testing/marionette/*.{js,mn}.

Well, maybe my question was not that ideal. Those files I already have updated with this patch. So would it mean Selenium works out of the box and can use the localization feature? I assume I would have to do some updates there too.

Comment 38

[Andreas Tolfsen ❲:ato❳]

Yes, you need to patch geckodriver by adding endpoints under /session/{session id}/moz/l10n/* and the necessary geckodriver-to-Marionette plumbing by implementing the ToMarionette trait.  In Selenium you need to add an API to the relevant client bindings so they can cal these endpoints.