604411 - Localization API for Mozmill tests

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Adrian Kalla [:adriank]]

This bug is about the implementation of the L10nAPI.

Comment 1

[Adrian Kalla [:adriank]]

Attachment: L10nAPI v1

Comment 2

[Adrian Kalla [:adriank]]

Attachment: L10nAPI v2

updated to match the current DOMWalker patch from bug

Also changed all functions to static.

Comment 3

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

Comment on attachment 488766 [details] [diff] [review]
L10nAPI v2

>+++ b/shared-modules/localization.js
>+ * The Initial Developer of the Original Code is Mozilla Foundation.

nit: This has to be "the Mozilla Foundation" in all cases.

>+ * It packs a submitted node and its access key into a double array
>+ *
>+ * @param {node} node Node containing the access key
>+ * @returns lower-cased access key and its node in a nested array
>+ * @type {array of array}
>+ */
>+function callbackAccessKeys(node) {
>+  return [[node.accessKey.toLowerCase(), node]];
>+}

I'm not that familiar with access keys. Does it mean we do not have to differentiate between upper and lower case? Given the semantic shouldn't the access key be the same letter as the character in the label:

"Tes_t_" => t
"_N_ew" => N

Also why do we have to return a 2d array. If it is just for adding the result to collectedResults, you should use .push inside the DOMWalker.

>+ * @param {MozmillController} cont

Please try to use controller as much as possible.

>+    if (node.id)
>+      var id = node.id;
>+    else
>+      var id ="(id is undefined)"

This can simply be written as 'var id = node.id || "(id is undefined)";'

>+    if (node.label)
>+      var label = node.label;
>+    else
>+      var label = "(label is undefined)";

Same here.

>+      var innerIds = [];
>+      var innerRects = [];

Save the duplicated declaration of the variables by declaring them before the if statement with the default values. Same question why this has to be an array applies here.

>+function callbackDimensions(node) {
>+  if (node.boxObject) {
>+    return checkDimensions(node);

Why another function while all that code can be part of this callback function? I would also prefer a better name.

>+function callbackDimensionsResults(cont, boxes) {
>+  jumlib.assertEquals(boxes.length, 0, 'cropped boxes '+boxes);

Please check the whitespaces around operands in all files.

>+function checkDimensions(child) {
>+  // check width
>+  if (childBox.height && childBox.screenX < parentBox.screenX) {
>+    badRects.push([childBox.x, childBox.y, parentBox.x - childBox.x,
>+                   childBox.height]);
>+    jumlib.assert(false, 'to the left');
>+  }
>+  if (childBox.height && childBox.screenX + childBox.width >
>+      parentBox.screenX + parentBox.width) {
>+    badRects.push([parentBox.x + parentBox.width, childBox.y,
>+                   childBox.x + childBox.width - parentBox.x - parentBox.width,
>+                   childBox.height]);
>+    jumlib.assert(false, 'to the right');
>+  }

This doesn't work for RTL, right? Also when you assert please include the element as for checking the access keys.

>+function createScreenshot(cont, boxes) {

This function doesn't belong into this module. We need a screenshot.js module.

>+  var doc = cont.window.document;

Same convention here with controller.

>+  for (var i=0, ii=boxes.length; i<ii; ++i) {

why ii? Also check whitespaces.

>+    if (rect[0] + rect[2] > maxWidth) maxWidth = rect[0] + rect[2];
>+    if (rect[1] + rect[3] > maxHeight) maxHeight = rect[1] + rect[3];

Can we draw outside of the window?

>+  var canvas = doc.createElementNS("http://www.w3.org/1999/xhtml", "canvas");
>+  var width = doc.documentElement.boxObject.width;
>+  var height = doc.documentElement.boxObject.height;
>+  canvas.width = maxWidth;
>+  canvas.height = maxHeight;
>+  var ctx = canvas.getContext("2d");



>+  for (var i=0, ii=boxes.length; i<ii; ++i) {

again.

>+    jumlib.assert(false, [maxWidth, maxHeight]);
>+    jumlib.assert(false, rect);

Why do we assert here?

>+  var filename = utils.appInfo.name + "-" + utils.appInfo.locale + "." +
>+                 utils.appInfo.version + "." + utils.appInfo.buildID + "." +
>+                 utils.appInfo.os + ".screenshot.png";
>+  
>+  // This will have to change once Mozmill will have screenshots-support
>+  if (mozmill.isWindows) {
>+    _saveCanvas(canvas, "C:\\"+filename);
>+  } else {
>+    _saveCanvas(canvas, "/tmp/"+filename);
>+  }

This code shouldn't be part of this function. Pass in the full path as parameter.

>+function filterAccessKeys(node) {

We should be consistent in naming the callback functions. I would propose to remove the callback prefix from the other ones.

>+  // Menus will need a separate filter set
>+  if (!node.disabled && !node.collapsed && !node.hidden &&
>+      node.localName != "menu" && node.localName != "menubar" &&
>+      node.localName != "menupopup" && node.localName != "popupset") {

Use an array for the element type you want to exclude. With indexOf only one single comparison would have to be made.

>+    if (node.parentNode && (node.parentNode.localName == "prefwindow" &&
>+                            node.parentNode.currentPane.id != node.id) ||
>+        ((node.parentNode.localName == "tabpanels" ||
>+          node.parentNode.localName == "deck") &&
>+          node.parentNode.selectedPanel.id != node.id)) {
>+      // we don't want to test not visible elements
>+      return domUtils.DOMWalker.FILTER_REJECT;

This code is very specific to the preference window. It needs some improvements so we can use it globally. Please file a bug for future work on this part.

>+function filterCroppedNodes(node) {
>+  switch (node.boxObject) {

An if/else clause should be easier in this case.

>+      if (!node.disabled && !node.collapsed && !node.hidden) {
>+        if (node.parentNode && (node.parentNode.localName == "prefwindow" &&
>+                                node.parentNode.currentPane.id != node.id) ||
>+            ((node.parentNode.localName == "tabpanels" ||
>+              node.parentNode.localName == "deck") &&
>+             node.parentNode.selectedPanel.id != node.id)) {
>+          // we don't want to test not visible elements
>+          return domUtils.DOMWalker.FILTER_REJECT;
>+        } else {
>+          return domUtils.DOMWalker.FILTER_ACCEPT;

Same here for pref window specific code.

>+function _saveCanvas(canvas, filePath) {

Not part of this module.

>+  var Cc = Components.classes;
>+  var Ci = Components.interfaces;

Those are already declared by Mozmill.
  
>+  // prepare to save the canvas data
>+  var persist = Cc["@mozilla.org/embedding/browser/nsWebBrowserPersist;1"]
>+                          .createInstance(Ci.nsIWebBrowserPersist);

Please use another name as persist to reduce confusion with persisted. Also check the indentation.

>\ No newline at end of file

nit: please add a blank line.

Comment 4

[Adrian Kalla [:adriank]]

Attachment: L10nAPI v3

(In reply to comment #3)
> Comment on attachment 488766 [details] [diff] [review]
> L10nAPI v2
> 
> >+++ b/shared-modules/localization.js
> >+ * The Initial Developer of the Original Code is Mozilla Foundation.
> 
> nit: This has to be "the Mozilla Foundation" in all cases.

right, done

> >+ * It packs a submitted node and its access key into a double array
> >+ *
> >+ * @param {node} node Node containing the access key
> >+ * @returns lower-cased access key and its node in a nested array
> >+ * @type {array of array}
> >+ */
> >+function callbackAccessKeys(node) {
> >+  return [[node.accessKey.toLowerCase(), node]];
> >+}
> 
> I'm not that familiar with access keys. Does it mean we do not have to
> differentiate between upper and lower case? Given the semantic shouldn't the
> access key be the same letter as the character in the label:
> 
> "Tes_t_" => t
> "_N_ew" => N

That's a difficult topic. Access keys are case-sensitive, but they also aren't.
From your examples:
"Tes_t_" => t
but
"_T_est" => T
and
"_N_ew" => N
but
"_N_ew" => n (AND NOT: "New(n)" !=> n)

So at the end it does matter where the access key will be "displayed", but it does not make any differences when used, as an access key "T" collides with an another access key "t" in the same scope.

> Also why do we have to return a 2d array. If it is just for adding the result
> to collectedResults, you should use .push inside the DOMWalker.

callbackAccessKeys is not the only function adding back to collectedResults - that's why it is a 2d array here.

> >+ * @param {MozmillController} cont
> 
> Please try to use controller as much as possible.

done

> >+    if (node.id)
> >+      var id = node.id;
> >+    else
> >+      var id ="(id is undefined)"
> 
> This can simply be written as 'var id = node.id || "(id is undefined)";'
> 
> >+    if (node.label)
> >+      var label = node.label;
> >+    else
> >+      var label = "(label is undefined)";
> 
> Same here.

done
 
> >+      var innerIds = [];
> >+      var innerRects = [];
> 
> Save the duplicated declaration of the variables by declaring them before the
> if statement with the default values.

done

> Same question why this has to be an array
> applies here.

it's much more convenient (at least to me) to have it as an array to use it at the end of this function

> >+function callbackDimensions(node) {
> >+  if (node.boxObject) {
> >+    return checkDimensions(node);
> 
> Why another function while all that code can be part of this callback function?
> I would also prefer a better name.

moved that to the checkDimensions function itself
 
> >+function callbackDimensionsResults(cont, boxes) {
> >+  jumlib.assertEquals(boxes.length, 0, 'cropped boxes '+boxes);
> 
> Please check the whitespaces around operands in all files.

checked (in this case: that line was removed as it was unneeded - we don't have to report the same error more than once)

> >+function checkDimensions(child) {
> >+  // check width
> >+  if (childBox.height && childBox.screenX < parentBox.screenX) {
> >+    badRects.push([childBox.x, childBox.y, parentBox.x - childBox.x,
> >+                   childBox.height]);
> >+    jumlib.assert(false, 'to the left');
> >+  }
> >+  if (childBox.height && childBox.screenX + childBox.width >
> >+      parentBox.screenX + parentBox.width) {
> >+    badRects.push([parentBox.x + parentBox.width, childBox.y,
> >+                   childBox.x + childBox.width - parentBox.x - parentBox.width,
> >+                   childBox.height]);
> >+    jumlib.assert(false, 'to the right');
> >+  }
> 
> This doesn't work for RTL, right?

It should work, but I'll probably have to make sure that it does...

> Also when you assert please include the
> element as for checking the access keys.

done

> >+function createScreenshot(cont, boxes) {
> 
> This function doesn't belong into this module. We need a screenshot.js module.

moved there

> >+  var doc = cont.window.document;
> 
> Same convention here with controller.

done

> >+  for (var i=0, ii=boxes.length; i<ii; ++i) {
> 
> why ii? Also check whitespaces.

changed to j and added whitespaces

> >+    if (rect[0] + rect[2] > maxWidth) maxWidth = rect[0] + rect[2];
> >+    if (rect[1] + rect[3] > maxHeight) maxHeight = rect[1] + rect[3];
> 
> Can we draw outside of the window?

yes

> >+  for (var i=0, ii=boxes.length; i<ii; ++i) {
> 
> again.

done

> >+    jumlib.assert(false, [maxWidth, maxHeight]);
> >+    jumlib.assert(false, rect);
> 
> Why do we assert here?

no idea... removed

> >+  var filename = utils.appInfo.name + "-" + utils.appInfo.locale + "." +
> >+                 utils.appInfo.version + "." + utils.appInfo.buildID + "." +
> >+                 utils.appInfo.os + ".screenshot.png";
> >+  
> >+  // This will have to change once Mozmill will have screenshots-support
> >+  if (mozmill.isWindows) {
> >+    _saveCanvas(canvas, "C:\\"+filename);
> >+  } else {
> >+    _saveCanvas(canvas, "/tmp/"+filename);
> >+  }
> 
> This code shouldn't be part of this function. Pass in the full path as
> parameter.

done

> >+function filterAccessKeys(node) {
> 
> We should be consistent in naming the callback functions. I would propose to
> remove the callback prefix from the other ones.

ok, callbacks removed. Function names changed to (hopefully) better ones.

> >+  // Menus will need a separate filter set
> >+  if (!node.disabled && !node.collapsed && !node.hidden &&
> >+      node.localName != "menu" && node.localName != "menubar" &&
> >+      node.localName != "menupopup" && node.localName != "popupset") {
> 
> Use an array for the element type you want to exclude. With indexOf only one
> single comparison would have to be made.

done

> >+    if (node.parentNode && (node.parentNode.localName == "prefwindow" &&
> >+                            node.parentNode.currentPane.id != node.id) ||
> >+        ((node.parentNode.localName == "tabpanels" ||
> >+          node.parentNode.localName == "deck") &&
> >+          node.parentNode.selectedPanel.id != node.id)) {
> >+      // we don't want to test not visible elements
> >+      return domUtils.DOMWalker.FILTER_REJECT;
> 
> This code is very specific to the preference window. It needs some improvements
> so we can use it globally. Please file a bug for future work on this part.

This function is usuable globally but may need further additions for other special cases like in the preferences window (like the one above).
A bug will be needed only, if such a special case will get identified.

> >+function filterCroppedNodes(node) {
> >+  switch (node.boxObject) {
> 
> An if/else clause should be easier in this case.

changed

> >+      if (!node.disabled && !node.collapsed && !node.hidden) {
> >+        if (node.parentNode && (node.parentNode.localName == "prefwindow" &&
> >+                                node.parentNode.currentPane.id != node.id) ||
> >+            ((node.parentNode.localName == "tabpanels" ||
> >+              node.parentNode.localName == "deck") &&
> >+             node.parentNode.selectedPanel.id != node.id)) {
> >+          // we don't want to test not visible elements
> >+          return domUtils.DOMWalker.FILTER_REJECT;
> >+        } else {
> >+          return domUtils.DOMWalker.FILTER_ACCEPT;
> 
> Same here for pref window specific code.

like the other case

> >+function _saveCanvas(canvas, filePath) {
> 
> Not part of this module.

moved to screenshot.js

> >+  var Cc = Components.classes;
> >+  var Ci = Components.interfaces;
> 
> Those are already declared by Mozmill.

ok, removed

> >+  // prepare to save the canvas data
> >+  var persist = Cc["@mozilla.org/embedding/browser/nsWebBrowserPersist;1"]
> >+                          .createInstance(Ci.nsIWebBrowserPersist);
> 
> Please use another name as persist to reduce confusion with persisted. Also
> check the indentation.

changed to wbPersist

> >\ No newline at end of file
> 
> nit: please add a blank line.

done

Comment 5

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

(In reply to comment #4)
> but
> "_N_ew" => n (AND NOT: "New(n)" !=> n)
> 
> So at the end it does matter where the access key will be "displayed", but it
> does not make any differences when used, as an access key "T" collides with an
> another access key "t" in the same scope.

And the UI part is nothing we have to check, right? The appropriate letter of the entry will automatically be chosen depending on the given access key (upper or lower case)? I assume it's the first index found in the string.

> > Also why do we have to return a 2d array. If it is just for adding the result
> > to collectedResults, you should use .push inside the DOMWalker.
> 
> callbackAccessKeys is not the only function adding back to collectedResults -
> that's why it is a 2d array here.

That doesn't answer my question.

> collectedResults = collectedResults.concat(nodeTestResults);

In a case like collectedResults you should use .push instead of .concat. I don't see a reason for carrying around a 2D array. 

> > >+function checkDimensions(child) {
[..]
> > This doesn't work for RTL, right?
> 
> It should work, but I'll probably have to make sure that it does...

Please do so. If it's not the case file a follow-up so it can be after all the current code has been landed.

> > This function doesn't belong into this module. We need a screenshot.js module.
> 
> moved there

As already said in the DOMWalker review request, during our last 1-1 we agreed on that this move shouldn't happen right now but will be worked on later. Please revert those changes. We absolutely need a cleaner way to pass around the path/filename for the screenshot. The current solution shouldn't land in our tree.

> > >+    if (rect[0] + rect[2] > maxWidth) maxWidth = rect[0] + rect[2];
> > >+    if (rect[1] + rect[3] > maxHeight) maxHeight = rect[1] + rect[3];
> > 
> > Can we draw outside of the window?
> 
> yes

Give me a hint, why or when we need that. That area would still be invisible in the screenshot because we can't include window decorations like title and borders.

> > >+    if (node.parentNode && (node.parentNode.localName == "prefwindow" &&
> > >+                            node.parentNode.currentPane.id != node.id) ||
> > >+        ((node.parentNode.localName == "tabpanels" ||
> > >+          node.parentNode.localName == "deck") &&
> > >+          node.parentNode.selectedPanel.id != node.id)) {
> > >+      // we don't want to test not visible elements
> > >+      return domUtils.DOMWalker.FILTER_REJECT;
> > 
> > This code is very specific to the preference window. It needs some improvements
> > so we can use it globally. Please file a bug for future work on this part.
> 
> This function is usuable globally but may need further additions for other
> special cases like in the preferences window (like the one above).
> A bug will be needed only, if such a special case will get identified.

So can you please format that above code in a way which makes it clear what's a global check and which are specific to the pref pane? That will help us a lot for future work.

Comment 6

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

Comment on attachment 491814 [details] [diff] [review]
L10nAPI v3

r-, as given by the the last comment.

Comment 7

[Adrian Kalla [:adriank]]

(In reply to comment #5)
> (In reply to comment #4)
> > but
> > "_N_ew" => n (AND NOT: "New(n)" !=> n)
> > 
> > So at the end it does matter where the access key will be "displayed", but it
> > does not make any differences when used, as an access key "T" collides with an
> > another access key "t" in the same scope.
> 
> And the UI part is nothing we have to check, right? The appropriate letter of
> the entry will automatically be chosen depending on the given access key (upper
> or lower case)? I assume it's the first index found in the string.

yes - there is nothing we have to care about on the Mozmill side (beside always testing lower-cased access keys). And your assumption is right, with the small addition like in your "Test" case: first it looks for the first index of the letter with the same case and tif not found, for the first index with the other case.

> > > Also why do we have to return a 2d array. If it is just for adding the result
> > > to collectedResults, you should use .push inside the DOMWalker.
> > 
> > callbackAccessKeys is not the only function adding back to collectedResults -
> > that's why it is a 2d array here.
> 
> That doesn't answer my question.

see below

> > collectedResults = collectedResults.concat(nodeTestResults);
> 
> In a case like collectedResults you should use .push instead of .concat. I
> don't see a reason for carrying around a 2D array. 

if it would be just for this case - yes, it does not make sense. But checkDimensions() will always return me a 2d array, so I don't see any sane way than doing this this way.

> > > >+function checkDimensions(child) {
> [..]
> > > This doesn't work for RTL, right?
> > 
> > It should work, but I'll probably have to make sure that it does...
> 
> Please do so. If it's not the case file a follow-up so it can be after all the
> current code has been landed.

ok

> > > This function doesn't belong into this module. We need a screenshot.js module.
> > 
> > moved there
> 
> As already said in the DOMWalker review request, during our last 1-1 we agreed
> on that this move shouldn't happen right now but will be worked on later.
> Please revert those changes. We absolutely need a cleaner way to pass around
> the path/filename for the screenshot. The current solution shouldn't land in
> our tree.

ok, my missunderstanding... 

> > > >+    if (rect[0] + rect[2] > maxWidth) maxWidth = rect[0] + rect[2];
> > > >+    if (rect[1] + rect[3] > maxHeight) maxHeight = rect[1] + rect[3];
> > > 
> > > Can we draw outside of the window?
> > 
> > yes
> 
> Give me a hint, why or when we need that. That area would still be invisible in
> the screenshot because we can't include window decorations like title and
> borders.

that's correct. But we can highlight the area that would be occupied by the cropped elements if they would be not cropped. That makes it much easier to see and find out if there is just 1px missing or 100px.

> > > >+    if (node.parentNode && (node.parentNode.localName == "prefwindow" &&
> > > >+                            node.parentNode.currentPane.id != node.id) ||
> > > >+        ((node.parentNode.localName == "tabpanels" ||
> > > >+          node.parentNode.localName == "deck") &&
> > > >+          node.parentNode.selectedPanel.id != node.id)) {
> > > >+      // we don't want to test not visible elements
> > > >+      return domUtils.DOMWalker.FILTER_REJECT;
> > > 
> > > This code is very specific to the preference window. It needs some improvements
> > > so we can use it globally. Please file a bug for future work on this part.
> > 
> > This function is usuable globally but may need further additions for other
> > special cases like in the preferences window (like the one above).
> > A bug will be needed only, if such a special case will get identified.
> 
> So can you please format that above code in a way which makes it clear what's a
> global check and which are specific to the pref pane? That will help us a lot
> for future work.

added comments which do explain that

Comment 8

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

(In reply to comment #7)
> if it would be just for this case - yes, it does not make sense. But
> checkDimensions() will always return me a 2d array, so I don't see any sane way
> than doing this this way.

You are pushing coordinate values to an array, which then gets pushed to another array. The values in the first array have different meanings. The first two values are coordinates, while the other two are dimensions. You should better stick those into a rect like object with clear names for the individual entries.    

> that's correct. But we can highlight the area that would be occupied by the
> cropped elements if they would be not cropped. That makes it much easier to see
> and find out if there is just 1px missing or 100px.

That means we will have a white/grey area at this side of the window and with the box overlayed?

Comment 9

[Adrian Kalla [:adriank]]

(In reply to comment #8)
> You are pushing coordinate values to an array, which then gets pushed to
> another array. The values in the first array have different meanings. The first
> two values are coordinates, while the other two are dimensions. You should
> better stick those into a rect like object with clear names for the individual
> entries.    

ok, will try that

> That means we will have a white/grey area at this side of the window and with
> the box overlayed?

yes

Comment 10

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

(In reply to comment #9)
> > two values are coordinates, while the other two are dimensions. You should
> > better stick those into a rect like object with clear names for the individual
> > entries.    
> 
> ok, will try that

If it's to complicated for you today we can push this also off to a follow-up bug.

> > That means we will have a white/grey area at this side of the window and with
> > the box overlayed?
> 
> yes

Sounds good. Taken! Thanks for the explanation.

Comment 11

[Adrian Kalla [:adriank]]

Attachment: L10nAPI v4

(In reply to comment #10)
> (In reply to comment #9)
> > > two values are coordinates, while the other two are dimensions. You should
> > > better stick those into a rect like object with clear names for the individual
> > > entries.    
> > 
> > ok, will try that
> 
> If it's to complicated for you today we can push this also off to a follow-up
> bug.

ok, I'll open a follow-up bug for that - this way it should go faster

Comment 12

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

Comment on attachment 491875 [details] [diff] [review]
L10nAPI v4

Looks good so far. Only two things left.

>+/**
>+ * Saves a given Canvas object to a file with the given path
>+ * 
>+ * @param {canvas} canvas
>+ */

The comment should explain that the path is given on the command line. If that's not the case that we fallback to tmp.

>+  var path = persisted.screenshotPath;

This should cause a JS strict error. You should first check for '"variable" in persisted'.

>+  // fallback if no path given
>+  if (!path) {
>+    path = utils.appInfo.name + "-" + utils.appInfo.locale + "." +
>+                   utils.appInfo.version + "." + utils.appInfo.buildID + "." +
>+                   utils.appInfo.os + ".png";

The indentation is a bit off here. Use the same column.

>+    if (mozmill.isWindows) {
>+      path = "C:\\" + path;
>+    } else {
>+      path = "/tmp/" + path;
>+    }

Use "tmpD" to get the temporary folder. We don't want to pollute the root folder on Windows. See the following example:
http://mxr.mozilla.org/mozilla-central/source/netwerk/test/unit/test_bug263127.js#47

Comment 13

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

Move of Mozmill Test related project bugs to newly created components. You can
filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.

Comment 14

[Adrian Kalla [:adriank]]

Attachment: L10nAPI v5

(In reply to comment #12)
> Comment on attachment 491875 [details] [diff] [review]
> L10nAPI v4
> >+/**
> >+ * Saves a given Canvas object to a file with the given path
> >+ * 
> >+ * @param {canvas} canvas
> >+ */
> 
> The comment should explain that the path is given on the command line. If
> that's not the case that we fallback to tmp.
> 
> >+  var path = persisted.screenshotPath;
> 
> This should cause a JS strict error. You should first check for '"variable" in
> persisted'.

done

> >+  // fallback if no path given
> >+  if (!path) {
> >+    path = utils.appInfo.name + "-" + utils.appInfo.locale + "." +
> >+                   utils.appInfo.version + "." + utils.appInfo.buildID + "." +
> >+                   utils.appInfo.os + ".png";
> 
> The indentation is a bit off here. Use the same column.
> 
> >+    if (mozmill.isWindows) {
> >+      path = "C:\\" + path;
> >+    } else {
> >+      path = "/tmp/" + path;
> >+    }
> 
> Use "tmpD" to get the temporary folder. We don't want to pollute the root
> folder on Windows. See the following example:
> http://mxr.mozilla.org/mozilla-central/source/netwerk/test/unit/test_bug263127.js#47

done

Comment 15

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

Comment on attachment 492856 [details] [diff] [review]
L10nAPI v5

>+    var file = Cc["@mozilla.org/file/directory_service;1"]
>+               .getService(Ci.nsIProperties)
>+               .get("TmpD", Ci.nsIFile);

To be consistent with out other tests and modules, the dot should be at the end of the file and not at the beginning.

>+    var fileName = utils.appInfo.name + "-" + utils.appInfo.locale + "." +
>+                   utils.appInfo.version + "." + utils.appInfo.buildID + "." +
>+                   utils.appInfo.os + ".png";

Could you just let the utils.appInfo.xxx parts flow into a new line?

r=me with those changes.

Comment 16

[Adrian Kalla [:adriank]]

Attachment: L10nAPI v6 [checked-in]

changes based on review comments from Comment 15

Comment 17

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

Comment on attachment 493363 [details] [diff] [review]
L10nAPI v6 [checked-in]

Landed on default as:
http://hg.mozilla.org/qa/mozmill-tests/rev/d4a2dcda93ac

Lets check what has to be done for the backports.

Comment 18

[Adrian Kalla [:adriank]]

Attachment: L10nAPI for 1.9.2 [checked-in]

localization.js and screenshot.js backport to 1.9.2 - fix for Bug 614990 included

Comment 19

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

Comment on attachment 494073 [details] [diff] [review]
L10nAPI for 1.9.2 [checked-in]

Landed on 1.9.2:
http://hg.mozilla.org/qa/mozmill-tests/rev/c644b0ce2829

Comment 20

[Adrian Kalla [:adriank]]

since Bug 621013 did land now, we can close this bug (there won't be a 1.9.1 backport of this shared-module).