Closed Bug 971054 Opened 10 years ago Closed 10 years ago

Create translation module

Categories

(Firefox :: Translations, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 32

People

(Reporter: Felipe, Assigned: Felipe)

References

Details

(Whiteboard: p=5 s=it-32c-31a-30b.2 [qa!])

Attachments

(6 files, 10 obsolete files)

2.78 KB, patch
florian
: review+
Details | Diff | Splinter Review
5.24 KB, patch
florian
: review+
Details | Diff | Splinter Review
7.04 KB, patch
florian
: review+
Details | Diff | Splinter Review
7.60 KB, patch
florian
: review+
Details | Diff | Splinter Review
3.53 KB, patch
florian
: review+
Details | Diff | Splinter Review
8.87 KB, patch
florian
: review+
Details | Diff | Splinter Review
This module takes as input a string formatted by the document walker module (bug 971043) and then sends it, along with extra data (source/target language, api keys, etc) to the translation service.

It should return a promise that will resolve once translation is finished, or handle any provider/network error and reject the promise.
Blocks: 971060
Whiteboard: p=0
Assignee: nobody → felipc
Status: NEW → ASSIGNED
Attached patch Translation modules (obsolete) — Splinter Review
Delegate the feedback as you wish.

This is the main code for the Google translation engine, plus a test LoremIpsum engine. Feedback both in the actual code and the way it is structured is welcome!

I'm still gonna improve a lot of the error handling, unifying all of them with a common structure. I'm planning on using a "Response" class similar to what rest.js does, which will have common error/success codes for everything related to the translation core.

Each engine can have a different API because it's not worth making a common one, as they all have different needs. The public-facing API to use should be DocumentTranslator.translate module.

It should also be easy for add-ons or us to include support for more engines, by adding them to the TranslationUtils list and adding a function _do{Name}Translation in DocumentTranslator
Attachment #8381135 - Flags: feedback?(gavin.sharp)
Comment on attachment 8381135 [details] [diff] [review]
Translation modules

Review of attachment 8381135 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/modules/translation/engines/LoremIpsumTranslator.jsm
@@ +47,5 @@
> +    return CommonUtils.laterTickResolvingPromise();
> +  },
> +
> +  supportsLanguagePair: function(sourceLanguage, targetLanguage) {
> +    return targetLanguage == "lipsum";

Am I correct in assuming that sourceLanguage and targetLanguage in language pairs are going to be BCP 47 language tags? If so, this should use 'x-lipsum' to ensure a valid language tag is used for testing. (Same goes for GoogleTranslator.jsm.)
That is correct, thanks for the pointer. The LoremIpsumTranslator will never be exposed outside of testing, but it's good to have the correct value for it.
Comment on attachment 8381135 [details] [diff] [review]
Translation modules

Review of attachment 8381135 [details] [diff] [review]:
-----------------------------------------------------------------

Removing feedback flag for now as the feedback I was mostly interested at (the structure, where to put files etc.) was already given through other bugs
Attachment #8381135 - Flags: feedback?(gavin.sharp)
Whiteboard: p=0 → p=5 s=it-31c-30a-29b.1
Assigning to Bogdan Maris as QA Contact for Translation.
QA Contact: bogdan.maris
Whiteboard: p=5 s=it-31c-30a-29b.1 → p=5 s=it-31c-30a-29b.1 [qa+]
Attached patch Google Translator module (obsolete) — Splinter Review
Attachment #8381135 - Attachment is obsolete: true
Attachment #8399387 - Flags: review?(florian)
Comment on attachment 8399387 [details] [diff] [review]
Google Translator module

Review of attachment 8399387 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/translation/engines/GoogleTranslator.jsm
@@ +8,5 @@
> +
> +this.EXPORTED_SYMBOLS = [ "GoogleTranslator" ];
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://services-common/rest.js");

rest.js doesn't seem used outside of /services; are we sure it's OK to use it in browser/?
I'm more familiar with Http.jsm.

@@ +30,5 @@
> +}
> +
> +
> +this.GoogleTranslator = {
> +  translate: function(parts, sourceLanguage, targetLanguage) {

Does this need a comment documenting the expected values for the parameters, and what's returned?

@@ +52,5 @@
> +    }
> +
> +    for (let part of parts) {
> +      params.push("q=" + encodeURIComponent(part));
> +    }

Would this be an improvement?
  params = params.concat(parts.map(p => "q=" + encodeURIComponent(p)));

@@ +55,5 @@
> +      params.push("q=" + encodeURIComponent(part));
> +    }
> +
> +    let request = new RESTRequest(apiEntryPoint);
> +    request.setHeader("X-HTTP-Method-Override", "GET");

Would a comment saying that we use POST instead of GET to be able to send more than 2k chars be useful here?

@@ +57,5 @@
> +
> +    let request = new RESTRequest(apiEntryPoint);
> +    request.setHeader("X-HTTP-Method-Override", "GET");
> +    request.setHeader("Content-Type", "application/x-www-form-urlencoded");
> +    request.setHeader("Referer", "https://translationrequest.mozilla.org")

Missing ;

@@ +68,5 @@
> +      } else if (this.response.success == false) {
> +        deferred.reject(ErrorResult({
> +          code: this.response.status,
> +          message: this.response.statusText
> +        }, "http-error"))

Missing ;

@@ +72,5 @@
> +        }, "http-error"))
> +      } else {
> +        let response;
> +        try {
> +          response = JSON.parse(this.response.body);

I think the code flow would be slightly easier to read if you moved the if (error) reject else resolve here (inside the try block). Then the "let" can also be moved inside the try block, and the return; in the catch block is no longer needed. This may just be a matter of personal preference though.

@@ +84,5 @@
> +        } else {
> +          deferred.resolve(response.data);
> +        }
> +      }
> +

I don't think this empty line is intentional.

@@ +88,5 @@
> +
> +    });
> +
> +    return deferred.promise;
> +  },

Trailing ,

::: browser/components/translation/moz.build
@@ +3,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  JS_MODULES_PATH = 'modules/translation'
>  
> +DIRS += ['engines']

Do we expect to have enough different engines to deserve a subdir for them?
Attachment #8399387 - Flags: review?(florian) → feedback+
Whiteboard: p=5 s=it-31c-30a-29b.1 [qa+] → p=5 s=it-31c-30a-29b.2 [qa+]
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Whiteboard: p=5 s=it-31c-30a-29b.2 [qa+] → p=5 s=it-31c-30a-29b.3 [qa+]
Whiteboard: p=5 s=it-31c-30a-29b.3 [qa+] → p=5 s=it-32c-31a-30b.1 [qa+]
Attached patch TranslationDocument (obsolete) — Splinter Review
This implements a TranslationDocument class which will be the main representation, during the translation process, of the document being translated and all the data related it. I've added plenty of comments to try to explain what it does and why it does things.
Attachment #8399387 - Attachment is obsolete: true
Attachment #8414338 - Flags: review?(florian)
Attached patch BingTranslator (obsolete) — Splinter Review
This implements the module that process the TranslationDocument to be used with Bing's API. It's missing some the handling of what to do when the translation process finishes, but that will be added in a separate patch.
Attachment #8414340 - Flags: review?(florian)
Attached patch BingRequest (obsolete) — Splinter Review
A request to Bing's translation service. Feedback because I need to add error handling and some more things
Attachment #8414341 - Flags: feedback?(florian)
Attached patch BingTokenManager (obsolete) — Splinter Review
The token manager, minus the keys
Attachment #8414344 - Flags: review?(florian)
Comment on attachment 8414338 [details] [diff] [review]
TranslationDocument

Review of attachment 8414338 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/translation/TranslationDocument.jsm
@@ +25,5 @@
> + *
> + * @param document  The document to be translated
> + */
> +this.TranslationDocument = function(document) {
> +  this.itensMap = new Map();

You meant ite_m_sMap, right?

@@ +28,5 @@
> +this.TranslationDocument = function(document) {
> +  this.itensMap = new Map();
> +  this.simpleRoots = [];
> +  this.nonSimpleRoots = [];
> +  this._init(document);

Is there a reason for this _init method to be separate from the constructor? It seems to only be called from here.

@@ +29,5 @@
> +  this.itensMap = new Map();
> +  this.simpleRoots = [];
> +  this.nonSimpleRoots = [];
> +  this._init(document);
> +}

Missing ;

@@ +40,5 @@
> +   * @param document  The document to be translated
> +   */
> +  _init: function(document) {
> +    let window = document.defaultView;
> +    let wutils = window.QueryInterface(Ci.nsIInterfaceRequestor)

winUtils may be more readable.

@@ +48,5 @@
> +    // a translation node is a node from the document which
> +    // contains useful content for translation, and therefore
> +    // must be included in the translation process.
> +    let nodeList = wutils.getTranslationNodes(document.body);
> +    

Trailing whitespace.

@@ +66,5 @@
> +    }
> +
> +    // At first all roots are stored in the roots list, and only after
> +    // the process has finished we're able to determine which roots are
> +    // simple, and which ones are not.

Do we really need to store them in 2 different lists?

@@ +70,5 @@
> +    // simple, and which ones are not.
> +
> +    // A simple root is defined by a root with no children items, which
> +    // basically represents an element from a page with only text content
> +    // inside.

I think when reading it would have helped me to find this comment near the first time the notion of simple/nonSimple root appeared (in the TranslationDocument constructor).

@@ +77,5 @@
> +    // simple root as plain-text in the translation process and with that
> +    // we are able to reduce their data payload sent to the translation service.
> +
> +    for (let root of roots) {
> +      if (root.children == 0) {

I think you forgot a .length here.

@@ +78,5 @@
> +    // we are able to reduce their data payload sent to the translation service.
> +
> +    for (let root of roots) {
> +      if (root.children == 0) {
> +        root.isSimpleRoot = true;

What about making isSimpleRoot a getter in the prototype of TranslationItem?

@@ +130,5 @@
> +      return [item.nodeRef.firstChild.nodeValue, 0];
> +    }
> +
> +    let id = startId;
> +    let localName = item.nodeRef.localName.toLowerCase();

According to my reading of MDN, .localName is already lower case since Gecko 2.0.

@@ +155,5 @@
> +        // we can simply replace it by a placeholder node.
> +        // We can't simply eliminate this node from our string representation
> +        // because that could change the HTML structure (e.g., it would
> +        // probably merge two separate text nodes).
> +        str += '<br/>';

Are we generating XML output? (If not you can skip the '/')
Are HTML comments handled with a <br/> node too?

@@ +162,5 @@
> +
> +    str += '</' + localName + '>';
> +    return [str, id];
> +  }
> +}

Missing ;

@@ +182,5 @@
> + * exist in the original string.
> + */
> +function trim(str) {
> +  let len = str.length;
> +  if (len < 2) return str;

Line break before 'return'.

@@ +184,5 @@
> +function trim(str) {
> +  let len = str.length;
> +  if (len < 2) return str;
> +  let startingSpace = str.charAt(0) == " " ? " " : "";
> +  let endingSpace = str.charAt(len - 1) == " " ? " " : "";

What about str.startsWith/endsWith?

@@ +185,5 @@
> +  let len = str.length;
> +  if (len < 2) return str;
> +  let startingSpace = str.charAt(0) == " " ? " " : "";
> +  let endingSpace = str.charAt(len - 1) == " " ? " " : "";
> +  return startingSpace + str.trim() + endingSpace;

The regular space character isn't the only character removed by String.trim. Do you care about adding one leading/trailing space if other whitespace was removed by trim?
Attachment #8414338 - Flags: review?(florian) → review-
Comment on attachment 8414340 [details] [diff] [review]
BingTranslator

Review of attachment 8414340 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/translation/BingTranslator.jsm
@@ +22,5 @@
> +// request.
> +const MAX_REQUEST_CHUNKS = 1900; // Documentation says 2000 but let's
> +                                 // play safe.
> +
> +// Self-imposed limit of 20 requests

Please expand this comment. I initially thought you were limiting the total of simultaneous requests to the server, and queuing additional requests. After reading more of the code and our IRC discussion, it seems this is limiting the amount of text data we will translate per document.

@@ +83,5 @@
> +        if (request.finished) {
> +          // When we finished generating chunks for the simpleRoots list,
> +          // we move on to generate requests for the nonSimpleRoots. After
> +          // both lists are complete, or we've reached our maximum request
> +          // limit, we are done.

Doesn't this mean that for pages containing more than the maximum amount of data, we will only translate the simple roots, which aren't necessarily near the beginning of the document, and could cause us to have some translated sentences in apparently random places of a mostly non-translated document?

@@ +121,5 @@
> +      text = escapeXML(text);
> +      let newCurSize = currentDataSize + text.length;
> +      let newChunks = currentChunks + 1;
> +
> +      if (newCurSize > MAX_REQUEST_DATA ||

Can generateTextForItem return a text longer than MAX_REQUEST_DATA and cause the function to return empty lists?

@@ +128,5 @@
> +        // If we've reached the API limits, let's stop accumulating data
> +        // for this request and return. We return information useful for
> +        // the caller to pass back on the next call, so that the function
> +        // can keep working from where it stopped.
> +        return { 

Trailing whitespace.

@@ +147,5 @@
> +      lastIndex: 0
> +    };
> +  }
> +
> +}

Missing ;, and the empty line before doesn't seem useful.

@@ +158,5 @@
> +             .replace(/&/g, "&amp;")
> +             .replace(/"/g, "&quot;")
> +             .replace(/'/g, "&apos;")
> +             .replace(/</g, "&lt;")
> +             .replace(/>/g, "&gt;");

Would something like this be more efficient?

aStr.replace(/[&"'><]/g, function(char) {
  return {
    '&': '&amp;',
    '>': '&gt;',
    '<': '&lt;',
    '"': '&quot;',
    "'": '&gt;'
  }[char];
});

::: browser/components/translation/moz.build
@@ +4,5 @@
>  
>  JS_MODULES_PATH = 'modules/translation'
>  
>  EXTRA_JS_MODULES = [
> +	'BingTranslator.jsm',

The rest of the list is indented with spaces, you used a tab.
Attachment #8414340 - Flags: review?(florian) → review-
Comment on attachment 8414341 [details] [diff] [review]
BingRequest

Review of attachment 8414341 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/translation/BingTranslator.jsm
@@ +160,5 @@
> + * @param sourceLanguage    The source language of the document.
> + * @param targetLanguage    The target language for the translation.
> + *
> + */
> +function BingRequest(translationData, sourceLanguage, targetLanguage) {

I don't see how the this.pendingRequests array of the BingTranslation object is used, but I'm wondering if this really needs to be an object, or if it would rather want to be just a function returning a promise.

@@ +174,5 @@
> +   */
> +  _fireRequest: function() {
> +    return Task.spawn(function *(){
> +      let token = yield BingTokenManager.getToken();
> +      let auth = "Bearer " + (token);

Why the () here?

@@ +190,5 @@
> +          </Options>\
> +          <Texts>';
> +
> +      for (let [, text] of this.translationData) {
> +        requestString += getEntryString(text);

I don't think this helper function makes the code more readable and it seems used only once.

If your goal with it was to separate the XML cruft from the logic of the code, maybe you would want to put the helper in the BingRequest prototype, and also have a getRequestPrefix and a getRequestSuffix helpers.

@@ +197,5 @@
> +      requestString += '</Texts>\
> +          <To>' + this.targetLanguage + '</To>\
> +        </TranslateArrayRequest>';
> +
> +      let utf8 = CommonUtils.encodeUTF8(requestString);

This is using nsIScriptableUnicodeConverter. A recent post on dev-planning suggests it should be avoided if possible.
Attachment #8414341 - Flags: feedback?(florian)
Comment on attachment 8414344 [details] [diff] [review]
BingTokenManager

Review of attachment 8414344 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/translation/BingTranslator.jsm
@@ +242,5 @@
> +   * @returns {Promise}  A promise that resolves with the token
> +   *                     string once it is obtained.
> +   */
> +  _getNewToken: function() {
> +    let request = new RESTRequest("https://datamarket.accesscontrol.windows.net/v2/OAuth2-13");

Http.jsm? Not sure if we should really switch to it, but just thought I would mention it as a gentle reminder.

@@ +249,5 @@
> +      "grant_type=client_credentials",
> +      "scope=" + encodeURIComponent("http://api.microsofttranslator.com"),
> +      "client_id=",
> +      "client_secret="
> +     ];

Nit: I would expect the ']' character here to be aligned with the 'l' of 'let'.

@@ +258,5 @@
> +        deferred.reject(err);
> +      }
> +
> +      try {
> +        let json = JSON.parse(this.response.body);

Should we use XHR's JSON parsing instead?
Attachment #8414344 - Flags: review?(florian) → feedback+
Replying to comments first, will post patches afterwards


(In reply to Florian Quèze [:florian] [:flo] from comment #12)
> Comment on attachment 8414338 [details] [diff] [review]
> TranslationDocument
> 
> Review of attachment 8414338 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/translation/TranslationDocument.jsm
> @@ +25,5 @@
> > + *
> > + * @param document  The document to be translated
> > + */
> > +this.TranslationDocument = function(document) {
> > +  this.itensMap = new Map();
> 
> You meant ite_m_sMap, right?

yeah

> 
> @@ +28,5 @@
> > +this.TranslationDocument = function(document) {
> > +  this.itensMap = new Map();
> > +  this.simpleRoots = [];
> > +  this.nonSimpleRoots = [];
> > +  this._init(document);
> 
> Is there a reason for this _init method to be separate from the constructor?
> It seems to only be called from here.

I usually keep the simple code in constructors (initializing variables, registering event listeners), and split off the complicated parts to _init.

> @@ +66,5 @@
> > +    }
> > +
> > +    // At first all roots are stored in the roots list, and only after
> > +    // the process has finished we're able to determine which roots are
> > +    // simple, and which ones are not.
> 
> Do we really need to store them in 2 different lists?

No, I removed this and i'm only storing it in a single list, which simplified a lot of stuff.  The dual lists were intended for future use but when I need it I can re-add it, possibly with a better structure.

> 
> @@ +70,5 @@
> > +    // simple, and which ones are not.
> > +
> > +    // A simple root is defined by a root with no children items, which
> > +    // basically represents an element from a page with only text content
> > +    // inside.
> 
> I think when reading it would have helped me to find this comment near the
> first time the notion of simple/nonSimple root appeared (in the
> TranslationDocument constructor).

Now that there are no prior references to simpleRoot, I kept it here

> 
> @@ +77,5 @@
> > +    // simple root as plain-text in the translation process and with that
> > +    // we are able to reduce their data payload sent to the translation service.
> > +
> > +    for (let root of roots) {
> > +      if (root.children == 0) {
> 
> I think you forgot a .length here.

Yeah, and there's also another check needed. root.children.length will only verify the number of other elements, but in the case of root with two adjacent text nodes (and a non-translation item in between), it still can't be considered a simple root.

E.g.  <div> AAA <br/> BBB </div>:   root.children.length = 0, but it can't be a simple root. I've fixed this in this patch.


> 
> @@ +78,5 @@
> > +    // we are able to reduce their data payload sent to the translation service.
> > +
> > +    for (let root of roots) {
> > +      if (root.children == 0) {
> > +        root.isSimpleRoot = true;
> 
> What about making isSimpleRoot a getter in the prototype of TranslationItem?

done.

> 
> @@ +130,5 @@
> > +      return [item.nodeRef.firstChild.nodeValue, 0];
> > +    }
> > +
> > +    let id = startId;
> > +    let localName = item.nodeRef.localName.toLowerCase();
> 
> According to my reading of MDN, .localName is already lower case since Gecko
> 2.0.

ok.


> @@ +184,5 @@
> > +function trim(str) {
> > +  let len = str.length;
> > +  if (len < 2) return str;
> > +  let startingSpace = str.charAt(0) == " " ? " " : "";
> > +  let endingSpace = str.charAt(len - 1) == " " ? " " : "";
> 
> What about str.startsWith/endsWith?

I got rid of this smart trim function, and I'm just using plain String.prototype.trim(). The server returns all strings trimmed, so trying to preserve the trailing and leading space wouldn't work here.
This is still necessary though, so this was moved to the moment when we're replacing the text with its translation (bug 973288)
(In reply to Florian Quèze [:florian] [:flo] from comment #13)
> @@ +83,5 @@
> > +        if (request.finished) {
> > +          // When we finished generating chunks for the simpleRoots list,
> > +          // we move on to generate requests for the nonSimpleRoots. After
> > +          // both lists are complete, or we've reached our maximum request
> > +          // limit, we are done.
> 
> Doesn't this mean that for pages containing more than the maximum amount of
> data, we will only translate the simple roots, which aren't necessarily near
> the beginning of the document, and could cause us to have some translated
> sentences in apparently random places of a mostly non-translated document?

With the removal of the two lists, this has been fixed. Things will get translated in the order of the document.

> 
> @@ +121,5 @@
> > +      text = escapeXML(text);
> > +      let newCurSize = currentDataSize + text.length;
> > +      let newChunks = currentChunks + 1;
> > +
> > +      if (newCurSize > MAX_REQUEST_DATA ||
> 
> Can generateTextForItem return a text longer than MAX_REQUEST_DATA and cause
> the function to return empty lists?

Yeah, I will fix this in a follow up as it will be a bit complicated and I don't want to increase the scope here.

> @@ +158,5 @@
> > +             .replace(/&/g, "&amp;")
> > +             .replace(/"/g, "&quot;")
> > +             .replace(/'/g, "&apos;")
> > +             .replace(/</g, "&lt;")
> > +             .replace(/>/g, "&gt;");
> 
> Would something like this be more efficient?
> 
> aStr.replace(/[&"'><]/g, function(char) {
>   return {
>     '&': '&amp;',
>     '>': '&gt;',
>     '<': '&lt;',
>     '"': '&quot;',
>     "'": '&gt;'
>   }[char];
> });

I tested various options and I'm now using the fastest one, which is String.replace("&", "&amp;", "g");
http://jsperf.com/xml-str-replace
(In reply to Florian Quèze [:florian] [:flo] from comment #15)
> ::: browser/components/translation/BingTranslator.jsm
> @@ +242,5 @@
> > +   * @returns {Promise}  A promise that resolves with the token
> > +   *                     string once it is obtained.
> > +   */
> > +  _getNewToken: function() {
> > +    let request = new RESTRequest("https://datamarket.accesscontrol.windows.net/v2/OAuth2-13");
> 
> Http.jsm? Not sure if we should really switch to it, but just thought I
> would mention it as a gentle reminder.

It does seem better but it's missing two things from its API to be able to use it here. I'd like to switch to it though, so I'll add those things and switch in a follow-up. (I'll file a bug with details)

> 
> @@ +249,5 @@
> > +      "grant_type=client_credentials",
> > +      "scope=" + encodeURIComponent("http://api.microsofttranslator.com"),
> > +      "client_id=",
> > +      "client_secret="
> > +     ];
> 
> Nit: I would expect the ']' character here to be aligned with the 'l' of
> 'let'.
> 
> @@ +258,5 @@
> > +        deferred.reject(err);
> > +      }
> > +
> > +      try {
> > +        let json = JSON.parse(this.response.body);
> 
> Should we use XHR's JSON parsing instead?

I will do this when we move to Http.jsm and we add something to its API to set overrideMimeType
(In reply to Florian Quèze [:florian] [:flo] from comment #14)
> > +function BingRequest(translationData, sourceLanguage, targetLanguage) {
> 
> I don't see how the this.pendingRequests array of the BingTranslation object
> is used, but I'm wondering if this really needs to be an object, or if it
> would rather want to be just a function returning a promise.

this.pendingRequests was not useful as an array, I converted it to an integer now which is used to track when all requests have finished.

BingRequest being a class is not useful now but will be in the future, as I'm planning of adding a queue of parallel requests and have a max of N requests running at the same time. So these requests will be in a pending queue and then we call .fireRequest() when a slot becomes available.



> 
> @@ +190,5 @@
> > +          </Options>\
> > +          <Texts>';
> > +
> > +      for (let [, text] of this.translationData) {
> > +        requestString += getEntryString(text);
> 
> I don't think this helper function makes the code more readable and it seems
> used only once.

Ok yeah, I got rid of it and merged the code here.

> @@ +197,5 @@
> > +      requestString += '</Texts>\
> > +          <To>' + this.targetLanguage + '</To>\
> > +        </TranslateArrayRequest>';
> > +
> > +      let utf8 = CommonUtils.encodeUTF8(requestString);
> 
> This is using nsIScriptableUnicodeConverter. A recent post on dev-planning
> suggests it should be avoided if possible.

Hopefully with the use of Http.jsm we won't need to explicitly do this, so I left it as is for now. If it doesn't, I'll change it afterwards.
(In reply to :Felipe Gomes from comment #17)

> > @@ +158,5 @@
> > > +             .replace(/&/g, "&amp;")
> > > +             .replace(/"/g, "&quot;")
> > > +             .replace(/'/g, "&apos;")
> > > +             .replace(/</g, "&lt;")
> > > +             .replace(/>/g, "&gt;");
> > 
> > Would something like this be more efficient?
> > 
> > aStr.replace(/[&"'><]/g, function(char) {
> >   return {
> >     '&': '&amp;',
> >     '>': '&gt;',
> >     '<': '&lt;',
> >     '"': '&quot;',
> >     "'": '&gt;'
> >   }[char];
> > });
> 
> I tested various options and I'm now using the fastest one, which is
> String.replace("&", "&amp;", "g");
> http://jsperf.com/xml-str-replace

The fastest one (on Firefox 29) depends on the size of the input string, see http://jsperf.com/xml-str-replace/2

Was
    a += a + a + a + a + a + a + a + a;
    a += a + a + a + a + a + a + a + a;
    a += a;
done to match specifically the expected size of the chunks we will be processing here?

On Chrome your version using strings instead of regexps is much faster.
Attached patch Part 1 - Translation Document (obsolete) — Splinter Review
Part 1, translation document.  Basically the same thing as before, simplified now with only the .roots array instead of both.

The _createItemForNode now takes an id as parameter instead of generating one by itself. And all roots are named <div>s and non-roots <b>s, instead of using the real node name (because this guarantees it will always be valid HTML)
Attachment #8414338 - Attachment is obsolete: true
Attachment #8420795 - Flags: review?(florian)
Attached patch Part 2 - BingTranslator (obsolete) — Splinter Review
Replaced my waitForTick impl with an existing one, CommonUtils.laterTickResolvingPromise().
Attachment #8414340 - Attachment is obsolete: true
Attachment #8420798 - Flags: review?(florian)
Attached patch Part 3 - BingRequest (obsolete) — Splinter Review
Attachment #8420800 - Flags: review?(florian)
Component: General → Translation
Fixed a small prob where a call to getToken should first check if there's a pendingRequest and return its promise instead of generating another unnecessary request
Attachment #8414341 - Attachment is obsolete: true
Attachment #8414344 - Attachment is obsolete: true
Attachment #8420801 - Flags: review?(florian)
Trivial, just move what's currently in content.js to its own JSM because Part 6 adds more code to it and I don't want this to be loaded for users who have the feature disabled
Attachment #8420802 - Flags: review?(florian)
This makes the clicks on Translation/Show Original/Show Translation buttons actually work, by sending these commands through the message manager and making content start the translation process
Attachment #8420804 - Flags: review?(florian)
(In reply to Florian Quèze [:florian] [:flo] from comment #20)
> (In reply to :Felipe Gomes from comment #17)
> 
> > > @@ +158,5 @@
> > > > +             .replace(/&/g, "&amp;")
> > > > +             .replace(/"/g, "&quot;")
> > > > +             .replace(/'/g, "&apos;")
> > > > +             .replace(/</g, "&lt;")
> > > > +             .replace(/>/g, "&gt;");
> > > 
> > > Would something like this be more efficient?
> > > 
> > > aStr.replace(/[&"'><]/g, function(char) {
> > >   return {
> > >     '&': '&amp;',
> > >     '>': '&gt;',
> > >     '<': '&lt;',
> > >     '"': '&quot;',
> > >     "'": '&gt;'
> > >   }[char];
> > > });
> > 
> > I tested various options and I'm now using the fastest one, which is
> > String.replace("&", "&amp;", "g");
> > http://jsperf.com/xml-str-replace
> 
> The fastest one (on Firefox 29) depends on the size of the input string, see
> http://jsperf.com/xml-str-replace/2
> 
> Was
>     a += a + a + a + a + a + a + a + a;
>     a += a + a + a + a + a + a + a + a;
>     a += a;
> done to match specifically the expected size of the chunks we will be
> processing here?
> 
> On Chrome your version using strings instead of regexps is much faster.

The 3rd version is much faster on Chrome because it ignores the 3rd param (it's non standard), so it does a one-time-only replace.

Smaller payloads are more likely to exist, I saw other versions winning with only huge strings which wouldn't be translated anyway.
Whiteboard: p=5 s=it-32c-31a-30b.1 [qa+] → p=5 s=it-32c-31a-30b.2 [qa+]
Comment on attachment 8420795 [details] [diff] [review]
Part 1 - Translation Document

Review of attachment 8420795 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/translation/TranslationDocument.jsm
@@ +40,5 @@
> +   */
> +  _init: function(document) {
> +    let window = document.defaultView;
> +    let winUtils = window.QueryInterface(Ci.nsIInterfaceRequestor)
> +                       .getInterface(Ci.nsIDOMWindowUtils);

Nit: fix alignment.

@@ +46,5 @@
> +    // Get all the translation nodes in the document's body:
> +    // a translation node is a node from the document which
> +    // contains useful content for translation, and therefore
> +    // must be included in the translation process.
> +    let nodeList = winUtils.getTranslationNodes(document.body);

Is <head><title>blah blah</title></head> something we also want to translate?

@@ +60,5 @@
> +      // Nodes which are translation roots do not have parents.
> +      let item = this._createItemForNode(node, i,
> +                                         isRoot ? null : node.parentNode);
> +      if (isRoot) {
> +        item.isRoot = true;

Would it make sense to have isRoot be initialized by the _createItemForNode function?

I guess more generally I'm confused by the separation between the code inlined in this loop and the content of the _createItemForNode function. Would it make sense for the function to also do the |isRoot ? null : node.parentNode| thing and the this.roots.push?

@@ +77,5 @@
> +    // This distinction is useful for optimization purposes: we treat a
> +    // simple root as plain-text in the translation process and with that
> +    // we are able to reduce their data payload sent to the translation service.
> +
> +    for (let root of roots) {

Isn't roots always an empty array? Unless I missed something, you mixed |roots| and |this.roots|.

@@ +126,5 @@
> +   */
> +  generateTextForItem: function(item) {
> +    if (item.isSimpleRoot) {
> +      item.original = [item.nodeRef.firstChild.nodeValue];
> +      return item.nodeRef.firstChild.nodeValue;

Save item.nodeRef.firstChild.nodeValue in a temporary variable to avoid the duplication?

No .trim() for the simple roots?

@@ +138,5 @@
> +    for (let child of item.nodeRef.childNodes) {
> +      if (child.nodeType == TEXT_NODE) {
> +        let x = child.nodeValue.trim();
> +        str += x;
> +        item.original.push(x);

are you intentionally saving the trimmed value in the "original" array?

@@ +182,5 @@
> +  get isRoot() this._isRoot,
> +  set isRoot(val) this._isRoot = val,
> +
> +  get isSimpleRoot() this._isSimpleRoot,
> +  set isSimpleRoot(val) this._isSimpleRoot = val,

Why do you have these no-op accessors for isSimpleRoot and isRoot? It seems you could as well just do:
  isRoot: false,
  isSimpleRoot: false,
in the prototype.
Attachment #8420795 - Flags: review?(florian) → review-
Comment on attachment 8420798 [details] [diff] [review]
Part 2 - BingTranslator

Review of attachment 8420798 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/translation/BingTranslator.jsm
@@ +77,5 @@
> +        let bingRequest = new BingRequest(request.data,
> +                                          this.sourceLanguage,
> +                                          this.targetLanguage);
> +        this._pendingRequests++;
> +        bingRequest.fireRequest().then(this._chunkCompleted.bind(this));

Are we sure that bingRequest.fireRequest() will always resolve the promise and never try to call the error callback instead? I'm just wondering if there's a risk that this._pendingRequests stays > 0 forever.

"Part 3 - BingRequest" has a function named _fireRequest but no fireRequest.

@@ +126,5 @@
> +    if (this._parseChunkResult(request.response.body)) {
> +      this._partialSuccess = true;
> +    }
> +
> +    maybeFinishedTranslation();

The code flow in this function can be simplified:

if (!request.error && request.response.success &&
    this._parseChunkResult(request.response.body)) {
  this._partialSuccess = true;
}

// inline here maybeFinishedTranslation
Attachment #8420798 - Flags: review?(florian) → review-
Comment on attachment 8420800 [details] [diff] [review]
Part 3 - BingRequest

Review of attachment 8420800 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/translation/BingTranslator.jsm
@@ +207,5 @@
> +      request.setHeader("Content-type", "text/xml");
> +      request.setHeader("Authorization", auth);
> +
> +      let requestString = '\
> +        <TranslateArrayRequest>\

Should we quote each line separately, and with a + operator on each line, to avoid sending the indent whitespace over the network?
(In reply to Florian Quèze [:florian] [:flo] from comment #28)

> @@ +46,5 @@
> > +    // Get all the translation nodes in the document's body:
> > +    // a translation node is a node from the document which
> > +    // contains useful content for translation, and therefore
> > +    // must be included in the translation process.
> > +    let nodeList = winUtils.getTranslationNodes(document.body);
> 
> Is <head><title>blah blah</title></head> something we also want to translate?

Yeah, it's a future improvement. When you get to bug 976556 and bug 976554 you'll notice that I moved the responsibility for the parsing/swapping code from the TranslationDocument to the TranslationItem objects. This is because I intend to make TranslationItem a base interface and have more types of items to be translated later, like the document title, some attributes like placeholder="", images' alt text, etc.

> 
> @@ +60,5 @@
> > +      // Nodes which are translation roots do not have parents.
> > +      let item = this._createItemForNode(node, i,
> > +                                         isRoot ? null : node.parentNode);
> > +      if (isRoot) {
> > +        item.isRoot = true;
> 
> Would it make sense to have isRoot be initialized by the _createItemForNode
> function?
> 
> I guess more generally I'm confused by the separation between the code
> inlined in this loop and the content of the _createItemForNode function.
> Would it make sense for the function to also do the |isRoot ? null :
> node.parentNode| thing and the this.roots.push?

Yeah, done

> 
> @@ +77,5 @@
> > +    // This distinction is useful for optimization purposes: we treat a
> > +    // simple root as plain-text in the translation process and with that
> > +    // we are able to reduce their data payload sent to the translation service.
> > +
> > +    for (let root of roots) {
> 
> Isn't roots always an empty array? Unless I missed something, you mixed
> |roots| and |this.roots|.

I meant this.roots

> 
> @@ +126,5 @@
> > +   */
> > +  generateTextForItem: function(item) {
> > +    if (item.isSimpleRoot) {
> > +      item.original = [item.nodeRef.firstChild.nodeValue];
> > +      return item.nodeRef.firstChild.nodeValue;
> 
> Save item.nodeRef.firstChild.nodeValue in a temporary variable to avoid the
> duplication?
> 
> No .trim() for the simple roots?
> 
> @@ +138,5 @@
> > +    for (let child of item.nodeRef.childNodes) {
> > +      if (child.nodeType == TEXT_NODE) {
> > +        let x = child.nodeValue.trim();
> > +        str += x;
> > +        item.original.push(x);
> 
> are you intentionally saving the trimmed value in the "original" array?

The .trim() for simple roots was missing, they are both intentionally stored trimmed

> 
> @@ +182,5 @@
> > +  get isRoot() this._isRoot,
> > +  set isRoot(val) this._isRoot = val,
> > +
> > +  get isSimpleRoot() this._isSimpleRoot,
> > +  set isSimpleRoot(val) this._isSimpleRoot = val,
> 
> Why do you have these no-op accessors for isSimpleRoot and isRoot? It seems
> you could as well just do:
>   isRoot: false,
>   isSimpleRoot: false,
> in the prototype.

It's what I had before.. I thought that's what you meant by "What about making isSimpleRoot a getter in the prototype of TranslationItem?" ;)  But I reverted it to the simpler version
Attachment #8420795 - Attachment is obsolete: true
Attachment #8421455 - Flags: review?(florian)
(In reply to Florian Quèze [:florian] [:flo] from comment #29)
> Comment on attachment 8420798 [details] [diff] [review]
> Part 2 - BingTranslator
> 
> Review of attachment 8420798 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/translation/BingTranslator.jsm
> @@ +77,5 @@
> > +        let bingRequest = new BingRequest(request.data,
> > +                                          this.sourceLanguage,
> > +                                          this.targetLanguage);
> > +        this._pendingRequests++;
> > +        bingRequest.fireRequest().then(this._chunkCompleted.bind(this));
> 
> Are we sure that bingRequest.fireRequest() will always resolve the promise
> and never try to call the error callback instead? I'm just wondering if
> there's a risk that this._pendingRequests stays > 0 forever.

Yeah, there's nothing in the code that rejects the promise, so the success one will always be called.

> 
> @@ +126,5 @@
> > +    if (this._parseChunkResult(request.response.body)) {
> > +      this._partialSuccess = true;
> > +    }
> > +
> > +    maybeFinishedTranslation();
> 
> The code flow in this function can be simplified:
> 
> if (!request.error && request.response.success &&
>     this._parseChunkResult(request.response.body)) {
>   this._partialSuccess = true;
> }
> 
> // inline here maybeFinishedTranslation

done
Attachment #8420798 - Attachment is obsolete: true
Attachment #8421458 - Flags: review?(florian)
(In reply to Florian Quèze [:florian] [:flo] from comment #30)
> > +      let requestString = '\
> > +        <TranslateArrayRequest>\
> 
> Should we quote each line separately, and with a + operator on each line, to
> avoid sending the indent whitespace over the network?

Sure

> 
> "Part 3 - BingRequest" has a function named _fireRequest but no fireRequest.

fixed, fireRequest is the right name
Attachment #8421459 - Flags: review?(florian)
(In reply to :Felipe Gomes from comment #31)
> Created attachment 8421455 [details] [diff] [review]
> Part 1 - Translation Document

> > @@ +182,5 @@
> > > +  get isRoot() this._isRoot,
> > > +  set isRoot(val) this._isRoot = val,
> > > +
> > > +  get isSimpleRoot() this._isSimpleRoot,
> > > +  set isSimpleRoot(val) this._isSimpleRoot = val,
> > 
> > Why do you have these no-op accessors for isSimpleRoot and isRoot? It seems
> > you could as well just do:
> >   isRoot: false,
> >   isSimpleRoot: false,
> > in the prototype.
> 
> It's what I had before.. I thought that's what you meant by "What about
> making isSimpleRoot a getter in the prototype of TranslationItem?" ;)  But I
> reverted it to the simpler version

I meant |get isSimpleRoot() this.children == 0,| instead of setting the isSimpleRoot value from a loop.

In the new patch you would need:
get isSimpleRoot() this.children.length == 0 && this.nodeRef.childElementCount == 0,
Comment on attachment 8421455 [details] [diff] [review]
Part 1 - Translation Document

Review of attachment 8421455 [details] [diff] [review]:
-----------------------------------------------------------------

I mostly reviewed the interdiff between this patch and the previous version, and it looks good. r=me with comment 34 addressed or acknowledged.
Attachment #8421455 - Flags: review?(florian) → review+
Comment on attachment 8421458 [details] [diff] [review]
Part 2 - BingTranslator

Review of attachment 8421458 [details] [diff] [review]:
-----------------------------------------------------------------

Don't forget to file the follow up mentioned in comment 17 :-).

::: browser/components/translation/BingTranslator.jsm
@@ +44,5 @@
> +  this.sourceLanguage = sourceLanguage;
> +  this.targetLanguage = targetLanguage;
> +  this._pendingRequests = 0;
> +  this._partialSuccess = false;
> +}

Nit: missing ;

@@ +174,5 @@
> +      finished: true,
> +      lastIndex: 0
> +    };
> +  }
> +}

Missing ;
Attachment #8421458 - Flags: review?(florian) → review+
Comment on attachment 8421459 [details] [diff] [review]
Part 3 - BingRequest

Review of attachment 8421459 [details] [diff] [review]:
-----------------------------------------------------------------

Don't forget the follow-ups related to using Http.jsm and not doing XPCOM unicode conversions.
Attachment #8421459 - Flags: review?(florian) → review+
Comment on attachment 8420800 [details] [diff] [review]
Part 3 - BingRequest

attachment 8421459 [details] [diff] [review] is an updated version of this.
Attachment #8420800 - Attachment is obsolete: true
Attachment #8420800 - Flags: review?(florian)
Comment on attachment 8420801 [details] [diff] [review]
Part 4 - BingTokenManager

Review of attachment 8420801 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/translation/BingTranslator.jsm
@@ +306,5 @@
> +    });
> +
> +    return deferred.promise;
> +  }
> +}

nit: missing ;
Attachment #8420801 - Flags: review?(florian) → review+
Comment on attachment 8420802 [details] [diff] [review]
Part 5 - TranslationContentHandler

Review of attachment 8420802 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/content.js
@@ +443,5 @@
>    },
>  };
>  PageStyleHandler.init();
>  
> +// Keep a reference to the translation content handler to avoid it it being GC'ed.

Isn't webProgress.addProgressListener keeping a strong reference?

@@ +445,5 @@
>  PageStyleHandler.init();
>  
> +// Keep a reference to the translation content handler to avoid it it being GC'ed.
> +let trHandler = null;
> +if (Services.prefs.getBoolPref("browser.translation.detectLanguage")) {

Now that this pref is exposed in the UI (bug 976574), I think we will want to add a pref observer to remove the language detection progress listener when the user turns off the feature.

This may be a reason to keep a reference to the TranslationContentHandler instance.

Addressing this can be a separate bug.

::: browser/components/translation/TranslationContentHandler.jsm
@@ +13,5 @@
> +function TranslationContentHandler() {
> +}
> +
> +TranslationContentHandler.prototype = {
> +  init: function(global, docShell) {

I think this method should be inlined in the constructor. I wrote this code with an init method initially because we had a single object and no constructor.
Attachment #8420802 - Flags: review?(florian) → review-
Comment on attachment 8420802 [details] [diff] [review]
Part 5 - TranslationContentHandler

Review of attachment 8420802 [details] [diff] [review]:
-----------------------------------------------------------------

Changing to r+ as the only comment left is something to address in a follow-up.

::: browser/base/content/content.js
@@ +443,5 @@
>    },
>  };
>  PageStyleHandler.init();
>  
> +// Keep a reference to the translation content handler to avoid it it being GC'ed.

No, it isn't. Ignore that comment. :-)

::: browser/components/translation/TranslationContentHandler.jsm
@@ +13,5 @@
> +function TranslationContentHandler() {
> +}
> +
> +TranslationContentHandler.prototype = {
> +  init: function(global, docShell) {

I've now seen that you are doing this in part 6.
Attachment #8420802 - Flags: review- → review+
Comment on attachment 8420804 [details] [diff] [review]
Part 6 - Hook up translation from the infobar

Review of attachment 8420804 [details] [diff] [review]:
-----------------------------------------------------------------

A few vague concerns I have with all these messages being passed asynchronously:
- can the code be in a confused state if the user clicks "show original" before we are done displaying the translation?
- what happens if the page is navigated away while we are in the process of replacing some of its DOM node? (I saw in bug 976554 that we are replacing node content by batch of 100 nodes and then letting the event loop spin)

I haven't spotted any specific problem related to these concerns, but I'm not confident things won't break, so I thought I would just share them so that you can check.

::: browser/components/translation/TranslationContentHandler.jsm
@@ +63,5 @@
> +      {
> +        Cu.import("resource:///modules/translation/TranslationDocument.jsm");
> +        Cu.import("resource:///modules/translation/BingTranslator.jsm");
> +
> +        this.translationDocument = new TranslationDocument(content);

You told me on IRC "the owner of the translationDocument object should be the content, not the global from the framescript". (just wanted to have this somewhere in the bug)

@@ +64,5 @@
> +        Cu.import("resource:///modules/translation/TranslationDocument.jsm");
> +        Cu.import("resource:///modules/translation/BingTranslator.jsm");
> +
> +        this.translationDocument = new TranslationDocument(content);
> +        let bingTranslation = new BingTranslation(this.translationDocument,

I was hoping we wouldn't have anything related to a specific provider hardcoded in the code that otherwise could be generic, but I guess at this point it's the pragmatic thing to do.

@@ +81,5 @@
> +      }
> +
> +      case "Translation:ShowOriginal":
> +        if (this.translationDocument) {
> +          this.translationDocument.swapText("original");

I'm not really thrilled about this API. The "original" and "translation" strings seem like an internal detail of TranslationDocument.jsm, and it's easy to typo while typing them. What about |showTranslation(true)| and |showTranslation(false)| (to show the original), or just |showTranslation()| and |showOriginal()| ?
Attachment #8420804 - Flags: review?(florian) → feedback+
NB: part 5 will now be minorly bitrotted by the patch on bug 1006379, which just hit fx-team. Sorry! :-(
Comment on attachment 8420804 [details] [diff] [review]
Part 6 - Hook up translation from the infobar

Review of attachment 8420804 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/translation/Translation.jsm
@@ +149,5 @@
> +  receiveMessage: function(msg) {
> +    switch (msg.name) {
> +      case "Translation:Finished":
> +        if (msg.data.success) {
> +          this.state = this.STATE_TRANSLATED;

Setting .state to TRANSLATED is not enough to update the UI (the URL bar icon needs to be changed to the colored version, and .originalShown should be set to false).

The current test calls showTranslatedContent when a translation succeeds.
Attachment #8420804 - Flags: review-
Comments addressed
Attachment #8420804 - Attachment is obsolete: true
Attachment #8424530 - Flags: review?(florian)
(In reply to Florian Quèze [:florian] [:flo] from comment #42)
> Comment on attachment 8420804 [details] [diff] [review]
> Part 6 - Hook up translation from the infobar
> 
> Review of attachment 8420804 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A few vague concerns I have with all these messages being passed
> asynchronously:
> - can the code be in a confused state if the user clicks "show original"
> before we are done displaying the translation?

Yeah it could, it wouldn't be bad but I covered this in the patch for Bug 1012519.

> - what happens if the page is navigated away while we are in the process of
> replacing some of its DOM node? (I saw in bug 976554 that we are replacing
> node content by batch of 100 nodes and then letting the event loop spin)

no harm

> 
> ::: browser/components/translation/TranslationContentHandler.jsm
> @@ +63,5 @@
> > +      {
> > +        Cu.import("resource:///modules/translation/TranslationDocument.jsm");
> > +        Cu.import("resource:///modules/translation/BingTranslator.jsm");
> > +
> > +        this.translationDocument = new TranslationDocument(content);
> 
> You told me on IRC "the owner of the translationDocument object should be
> the content, not the global from the framescript". (just wanted to have this
> somewhere in the bug)

this is done now, and it's also used in bug 1012519.
Depends on: 1012522
Depends on: 1012530
Depends on: 1012532
Depends on: 1012533
Comment on attachment 8424530 [details] [diff] [review]
Part 6 - Hook up translation from the infobar

Review of attachment 8424530 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/translation/TranslationContentHandler.jsm
@@ +84,5 @@
> +        break;
> +      }
> +
> +      case "Translation:ShowOriginal":
> +        if (this.global.content.translationDocument) {

Do you expect this.global.content.translationDocument to sometimes be null when receiving the ShowOriginal or ShowTranslation messages?

Unless I missed something, if it's null that means we are trying to show the original or the translation before the "Translate" button was ever clicked... which sounds like a code error that we would be happy to see in the error console.
Attachment #8424530 - Flags: review?(florian) → review+
I`ve tested the translation module doing exploratory testing using the try build on Windows 7 64bit, Mac OS X 10.9.3 and Ubuntu 13.10 32bit, found a few new issues, removing qa+ for now and leave this bug Resolved Fixed until all the dependences are fixed.
Whiteboard: p=5 s=it-32c-31a-30b.2 [qa+] → p=5 s=it-32c-31a-30b.2 [qa!]
Depends on: 1014598
Marking this bug as VERIFIED FIXED since all dependences are tracked, also patches or fixes will go into those bugs.
Status: RESOLVED → VERIFIED
(In reply to :Felipe Gomes from comment #48)

> I'll work in the test suite on a separate bug

Was this bug filed? It may be nice to have it included in the next iteration.
Flags: needinfo?(felipc)
Depends on: 1022729
(In reply to Florian Quèze [:florian] [:flo] from comment #53)
> (In reply to :Felipe Gomes from comment #48)
> 
> > I'll work in the test suite on a separate bug
> 
> Was this bug filed? It may be nice to have it included in the next iteration.

bug 1022729 and bug 1022725
Depends on: 1022725
Flags: needinfo?(felipc)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: