978158 - Set up client FHR reporting for Translation project

Status: VERIFIED FIXED

(Firefox :: Translations, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

Bug 973292, bug 973293 and bug 973294 are about collecting data from the Translation feature (to be released first as an experimental run targeting users from certain markets).

Each of those bugs detail one data point that we want to collect, and I'm opening this bug to set up the base structure around the FHR api to make the bugs themselves straightforward to implement.

Comment 1

[John Jensen]

Hi there, adding Saptarshi and Christina of the Metrics team. As data analysts, we may be able to help identify the types and formatting of metrics in the FHR payload to maximize the value we get out of them.

Comment 2

[Steven MacLeod [:smacleod]]

Attachment: Patch 1 - Move xpcshell tests to their own directory

This moves xpcshell tests to their own subdirectory to follow the normal conventions around xpcshell tests.

Comment 3

[Steven MacLeod [:smacleod]]

Attachment: Patch 1 - Move xpcshell tests to their own directory

Attached wrong patch, this one moves the tests.

Comment 4

[Steven MacLeod [:smacleod]]

Attachment: Patch 2 - Setup FHR provider for Translation project.

This patch introduces an FHR provider for the translation project and a measurement with some counts (which are inspired from the dependent bugs).

Felipe, as we discussed I've included some methods on the provider to be called when translation opportunities arise, or translation occurs. I have yet to write the wrapper methods to retrieve the provider from the FHR service and call the record method, but that will be trivial to write. I was thinking this should live in Translation.jsm, what do you think?

Saptarshi, could you please weigh in on the format these measurements are taking?

Richard, it would be helpful if you could look over the FHR stuff here and make sure I'm doing things properly. We decided to go with explicit methods for recording data rather than observers or a collection method since there aren't plans for Translation to emit the notifications.

Comment 5

[Steven MacLeod [:smacleod]]

Comment on attachment 8414197 [details] [diff] [review]
Patch 1 - Move xpcshell tests to their own directory

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

(Please view the attachment body for file moves)

Comment 6

["Saptarshi Guha[:joy]"]

> 
> Saptarshi, could you please weigh in on the format these measurements are
> taking?

Hi Steven,

Will do. Give me a day  for the Australis buzz to subside a bit and will respond here.

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 8414202 [details] [diff] [review]
Patch 2 - Setup FHR provider for Translation project.

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

This looks fantastic. I assume there's a separate patch for actually calling the provider from the translator?

::: browser/components/translation/moz.build
@@ +11,2 @@
>      'LanguageDetector.jsm',
>      'Translation.jsm'

Maybe rename healthreport.jsm to be a little more indicative, and also to match the naming convention here? E.g., "TranslationHealthReportProvider.jsm"?

Comment 8

[Steven MacLeod [:smacleod]]

Thanks for the quick feedback!

(In reply to Richard Newman [:rnewman] from comment #7)
> Comment on attachment 8414202 [details] [diff] [review]
> Patch 2 - Setup FHR provider for Translation project.
> 
> Review of attachment 8414202 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks fantastic. I assume there's a separate patch for actually calling
> the provider from the translator?

Yes, I'm waiting on Felipe's feedback before finalizing that part.

 
> ::: browser/components/translation/moz.build
> @@ +11,2 @@
> >      'LanguageDetector.jsm',
> >      'Translation.jsm'
> 
> Maybe rename healthreport.jsm to be a little more indicative, and also to
> match the naming convention here? E.g.,
> "TranslationHealthReportProvider.jsm"?

I was following the way sync set things up here "services/sync/modules/healthreport.jsm" since the import will be under translation anyway "resource:///modules/translation/healthreport.jsm". Having translation in the name seems a little redundant to me. If you still don't like "healthreport.jsm", would "HealthReportProvider.jsm" be preferable?

Comment 9

[Richard Newman [:rnewman]]

(In reply to Steven MacLeod [:smacleod] from comment #8)
> If you still
> don't like "healthreport.jsm", would "HealthReportProvider.jsm" be
> preferable?

It's your part of the tree; if you're happy with mixed case and short name, then rock on! Was just making sure you'd considered this.

Comment 10

["Saptarshi Guha[:joy]"]

I see three things being measured 

1) translationOpportunityCount, 
2) pageTranslatedCount (<= translationOpportunityCount)
3) character count

It might be helpful to answer "what is a translation opportunity" , both here and in the file dataformat.rst
- is translationOpportunity defined as "pages visited written in non-default languages given the profile's locale" 
- are the counts (1) and (2) of unique pages? or if i visit the same page twice, it will be counted twice?

Comment 11

[Steven MacLeod [:smacleod]]

(In reply to "Saptarshi Guha[:joy]" from comment #10)
> I see three things being measured 
> 
> 1) translationOpportunityCount, 
> 2) pageTranslatedCount (<= translationOpportunityCount)
> 3) character count
> 
> It might be helpful to answer "what is a translation opportunity" , both
> here and in the file dataformat.rst

Definitely. The way I understood it a "translation opportunity" was when we detected the pages language to be different than the users locale, and we offered to translate. Looking again at Bug 973293 though, it appears the measurements need to be much more complex to meet the "acceptance criteria".

According to Bug 973293 comment 2 we need to be able to break down these opportunities by language as well. Looking at the criteria for report generation in Bug 973293, how would you suggest structuring this data?

> - is translationOpportunity defined as "pages visited written in non-default
> languages given the profile's locale"

> - are the counts (1) and (2) of unique pages? or if i visit the same page
> twice, it will be counted twice?

I was assuming it would be counted twice. Everytime we offer to translate that is counted, and everytime we translate a page that would be counted.

Overall, it seems like these measurements are grossly inadequate for the acceptance criteria in Bug 973293 and Bug 973294. Maybe it would be more useful to start discussing the specific details of the measurements in Bug 973293?

Comment 12

["Saptarshi Guha[:joy]"]

(In reply to Steven MacLeod [:smacleod] from comment #11)
> (In reply to "Saptarshi Guha[:joy]" from comment #10)

> > It might be helpful to answer "what is a translation opportunity" , both
> > here and in the file dataformat.rst
> 
> Definitely. The way I understood it a "translation opportunity" was when we
> detected the pages language to be different than the users locale, and we
> offered to translate. 

yes, that appears to match Comment 1 in Bug 973293 
 
> 
> According to Bug 973293 comment 2 we need to be able to break down these
> opportunities by language as well. Looking at the criteria for report
> generation in Bug 973293, how would you suggest structuring this data?
> 

From Comment 2,
1. "I can view the gross number of times that Beta users of Firefox on every build visit pages written in languages other than the user’s default for all users in the test and control groups"

- The "test" and "control" groups is related to telemetry experiments. I dont know how this will be recorded in FHR.  bsmedberg might have more info.
- "visit pages in other languages" is measured by translationOpportunityCount

2. " breakdown of this number that lets me know how frequent each non-default language was detected (e.g., users say 10 pages written in French, 9 pages written in Spanish and 8 pages written in Sanskrit)"
- that would mean having a dictionary like(example)


    "org.mozilla.translation.translation": {
      "_v": 1,
      "translationOpportunityCount": 17, //17 == 10+8+9,
      "pageTranslatedCount": 6,
      "charactersTranslatedCount": "1126"
      "translationOpportunityCountByLanguage" : {
             "fr": 10,
             "sanskrit" : 8, //what locale is this?
             "es" : 9
       },
       "pageTranslatedCountByLanguage" :{
              "fr": 9
       }
}

As for privacy, best that bsmedberg comment.

The locale information (third criteria) is already present in FHR.

Yes, better take the discussion there.

Comment 13

[Benjamin Smedberg]

I just wanted to clarify something here: this data is only supposed to be collected if telemetry is enabled: we're collecting it using the FHR data collection mechanism because that allows us to track user behavior over time, but this is still considered a measurement under the telemetry privacy policy, not the FHR privacy policy.

Comment 14

[Steven MacLeod [:smacleod]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #13)
> I just wanted to clarify something here: this data is only supposed to be
> collected if telemetry is enabled: we're collecting it using the FHR data
> collection mechanism because that allows us to track user behavior over
> time, but this is still considered a measurement under the telemetry privacy
> policy, not the FHR privacy policy.

This is news to me, did I missing a bug or something where this was decided?

Is there an example of where this has been done before? Should I just be checking
the telemetry pref before I record a measurement, or is there something built
into FHR for measurements like this?

Comment 15

[Benjamin Smedberg]

This was decided at a meeting a month ago. I posted to dev-privacy about the plan and made sure the Firefox privacy notices are correct.

There is no precedent for this. You should check the toolkit.telemetry.enabled pref before including this data in the submitted FHR payload. It is not necessary to check the pref before recording the data in the local databases.

Comment 16

[Chad Weiner [:chadw]]

Steven--are you clear on the expectations at this point or is there anything I can do to make it clearer?
Thanks!



(In reply to Steven MacLeod [:smacleod] from comment #11)
> (In reply to "Saptarshi Guha[:joy]" from comment #10)
> > I see three things being measured 
> > 
> > 1) translationOpportunityCount, 
> > 2) pageTranslatedCount (<= translationOpportunityCount)
> > 3) character count
> > 
> > It might be helpful to answer "what is a translation opportunity" , both
> > here and in the file dataformat.rst
> 
> Definitely. The way I understood it a "translation opportunity" was when we
> detected the pages language to be different than the users locale, and we
> offered to translate. Looking again at Bug 973293 though, it appears the
> measurements need to be much more complex to meet the "acceptance criteria".
> 
> According to Bug 973293 comment 2 we need to be able to break down these
> opportunities by language as well. Looking at the criteria for report
> generation in Bug 973293, how would you suggest structuring this data?
> 
> > - is translationOpportunity defined as "pages visited written in non-default
> > languages given the profile's locale"
> 
> > - are the counts (1) and (2) of unique pages? or if i visit the same page
> > twice, it will be counted twice?
> 
> I was assuming it would be counted twice. Everytime we offer to translate
> that is counted, and everytime we translate a page that would be counted.
> 
> Overall, it seems like these measurements are grossly inadequate for the
> acceptance criteria in Bug 973293 and Bug 973294. Maybe it would be more
> useful to start discussing the specific details of the measurements in Bug
> 973293?

Comment 17

[Florian Quèze [:florian]]

Mass move of translation bugs to the new Translation component.

Comment 18

[Steven MacLeod [:smacleod]]

(In reply to Chad Weiner from comment #16)
> Steven--are you clear on the expectations at this point or is there anything
> I can do to make it clearer?
> Thanks!

Things are clear now, thanks Chad.

Comment 19

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8414202 [details] [diff] [review]
Patch 2 - Setup FHR provider for Translation project.

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

(waiting for newer patch)

Comment 20

[Steven MacLeod [:smacleod]]

Attachment: Patch 2 - Setup FHR provider for Translation project.

This new patch:
- Will no longer transmit the measurements when "toolkit.telemetry.enabled" is false.
- Additionally records translation opportunities and translations broken down by language.
- Provides helper methods in Translation.jsm for recording opportunities and translations.

Felipe: To me, an obvious place to call |TranslationHealthReport.recordTranslationOpportunity()| would be in |Translation.languageDetected()| (but I did not include it in this patch). We don't seem to have the number of characters we're translating but when we do calling |TranslationHealthReport.recordTranslation()| should just work.

Richard, could you please give the FHR stuff another look?

Benjamin, I wanted to get your feedback on the way I'm handling not sending the data when telemetry is disabled, could you take a look please?

Comment 21

[Benjamin Smedberg]

Comment on attachment 8427412 [details] [diff] [review]
Patch 2 - Setup FHR provider for Translation project.

The shouldIncludeField check is good, that prevents us from sending the data if the user turns telemetry off. But I think we should also add a check in the record* functions so that we don't even record the information if telemetry is disabled.

Some notes below; I didn't do a complete review; that should be done by rnewman.

>From: Steven MacLeod <[e-mail]>
>Subject: Bug 978158 - Setup FHR provider for Translation project.

I encourage you to add a much more detailed paragraph-length commit message here.

>diff --git a/browser/components/translation/HealthReport.jsm b/browser/components/translation/HealthReport.jsm

>+// TODO: RENAME THIS FILE HealthReport.jsm and export an infterface
>+// to yet-to-be-written helper methods which will call into the
>+// provider.

Is this TODO still valid? Why is this in a separate file from Translation.jsm? ISTM they could be combined, which would be fewer JS compartments and you might be able to short-circuit the datareportingservice.getProvider(...) bits.

>+TranslationMeasurement1.prototype = Object.freeze({


I think you should include a detailed comment here explaining that this is a telemetry measurement in the FHR payload.


>+  shouldIncludeField: function (field) {
>+    if (!Services.prefs.getBoolPref("toolkit.telemetry.enabled")) {
>+      // This measurment should only be included when telemetry is

nit spelling

>diff --git a/browser/components/translation/Translation.jsm b/browser/components/translation/Translation.jsm

> this.Translation = {
>   supportedSourceLanguages: ["en", "zh", "ja", "es", "de", "fr", "ru", "ar", "ko", "pt"],
>   supportedTargetLanguages: ["en", "pl", "tr", "vi"],

I'm sure this comment belongs in another bug, but do we want this hardcoded? This seems like the kind of thing that belongs in a hidden pref.

Comment 22

[Florian Quèze [:florian]]

Comment on attachment 8427412 [details] [diff] [review]
Patch 2 - Setup FHR provider for Translation project.

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

A few drive-by comments:

::: browser/components/translation/Translation.jsm
@@ +189,5 @@
> +   * Record a translation opportunity in the health report.
> +   * @param language
> +   *        The language of the page.
> +   */
> +   recordTranslationOpportunity: function (language) {

nit: the indent isn't correct on this line.

@@ +199,5 @@
> +        reporter.onInit().then(function record() {
> +          try {
> +            reporter.getProvider("org.mozilla.translation").recordTranslationOpportunity(language);
> +          } catch (ex) {
> +            Cu.reportError(ex);

Is there a reason for using Cu.reportError here and console.error a few lines later?

@@ +223,5 @@
> +
> +      if (reporter) {
> +        reporter.onInit().then(function record() {
> +          try {
> +            reporter.getProvider("org.mozilla.translation").recordTranslation(language, numCharacters);

comment 21 suggests you may avoid the .getProvider here, but if you do need it, could you use a helper to reduce the duplication between recordTranslationOpportunity and recordTranslation?

Comment 23

[Steven MacLeod [:smacleod]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #21)
> Comment on attachment 8427412 [details] [diff] [review]
> Patch 2 - Setup FHR provider for Translation project.
> 
> The shouldIncludeField check is good, that prevents us from sending the data
> if the user turns telemetry off. But I think we should also add a check in
> the record* functions so that we don't even record the information if
> telemetry is disabled.

That was my initial plan but I only filtered at sending the payload because
of Comment 15. Do you want me to go ahead and add the pre record filtering?


> I encourage you to add a much more detailed paragraph-length commit message
> here.

Will do.


> >+// TODO: RENAME THIS FILE HealthReport.jsm and export an infterface
> >+// to yet-to-be-written helper methods which will call into the
> >+// provider.
> 
> Is this TODO still valid?

No, I'd thought I'd removed it, woops!


> Why is this in a separate file from
> Translation.jsm? ISTM they could be combined, which would be fewer JS
> compartments and you might be able to short-circuit the
> datareportingservice.getProvider(...) bits.

It's a separate file only for organization. It shouldn't be a problem
to combine this into Translation.jsm


> >+TranslationMeasurement1.prototype = Object.freeze({
> 
> 
> I think you should include a detailed comment here explaining that this is a
> telemetry measurement in the FHR payload.

Definitely. I'll add some information about that to the docs as well.


> > this.Translation = {
> >   supportedSourceLanguages: ["en", "zh", "ja", "es", "de", "fr", "ru", "ar", "ko", "pt"],
> >   supportedTargetLanguages: ["en", "pl", "tr", "vi"],
> 
> I'm sure this comment belongs in another bug, but do we want this hardcoded?
> This seems like the kind of thing that belongs in a hidden pref.

Felipe? ^

Comment 24

[Benjamin Smedberg]

Oh gah, I'm contradicting myself, and I'm sorry about that. Let's go with my later version: we should not record the data locally unless telemetry is on. I can't remember what I was thinking in comment 15 :-(

Comment 25

[:Felipe Gomes (needinfo for replies!)]

> > > this.Translation = {
> > >   supportedSourceLanguages: ["en", "zh", "ja", "es", "de", "fr", "ru", "ar", "ko", "pt"],
> > >   supportedTargetLanguages: ["en", "pl", "tr", "vi"],
> > 
> > I'm sure this comment belongs in another bug, but do we want this hardcoded?
> > This seems like the kind of thing that belongs in a hidden pref.
> 
> Felipe? ^

I don't think this should be part of a pref because it is provider dependent, so it doesn't make sense to add languages that are not supported by the provider. With that said, this list is just a placeholder and we will expand it once we know all the languages that the provider support.

There's a valid case for a pref, if we decide to not include some supported languages if they are lower quality, or to not make the list too large, and still want to allow the users to use them if they really want somehow. But yeah that's for another bug :)

Comment 26

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8427412 [details] [diff] [review]
Patch 2 - Setup FHR provider for Translation project.

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

I didn't look closely in HealthReport.jsm, I'll leave that to rnewman's review. The API from the Translation point of view looks fine to me. The only detail is that the recordTranslation function needs to record the two languages (the original and the translated language), instead of just one.

Comment 27

[Richard Newman [:rnewman]]

Comment on attachment 8427412 [details] [diff] [review]
Patch 2 - Setup FHR provider for Translation project.

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

This seems sane to me. One substantive question for what's going on with run_next_test (tests shouldn't need to fudge with their harness), but we've hit my nit limit, so let's do one last pass for a rubberstamp before landing.

::: browser/components/translation/HealthReport.jsm
@@ +1,3 @@
> +/* 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/. */

Nit: your template is outdated. Correct license text:

http://www.mozilla.org/MPL/headers/

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

@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// TODO: RENAME THIS FILE HealthReport.jsm and export an infterface
> +// to yet-to-be-written helper methods which will call into the
> +// provider.

Looks like the name is right. File a follow-up for the rest, if necessary?

@@ +5,5 @@
> +// TODO: RENAME THIS FILE HealthReport.jsm and export an infterface
> +// to yet-to-be-written helper methods which will call into the
> +// provider.
> +
> +"use strict"

Trailing semicolon, please.

@@ +9,5 @@
> +"use strict"
> +
> +this.EXPORTED_SYMBOLS = [
> +  "TranslationProvider",
> +]

;

@@ +11,5 @@
> +this.EXPORTED_SYMBOLS = [
> +  "TranslationProvider",
> +]
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu} = Components;

You don't use Cc or Ci, so they can be removed.

@@ +20,5 @@
> +
> +const DAILY_COUNTER_FIELD = {type: Metrics.Storage.FIELD_DAILY_COUNTER};
> +const DAILY_LAST_TEXT_FIELD = {type: Metrics.Storage.FIELD_DAILY_LAST_TEXT};
> +
> +function TranslationMeasurement1() {

Elsewhere you use the pattern

  this.TranslationMeasurement1 = function...

IIRC in this context it doesn't matter which you use, but please be consistent.

@@ +46,5 @@
> +  },
> +
> +  shouldIncludeField: function (field) {
> +    if (!Services.prefs.getBoolPref("toolkit.telemetry.enabled")) {
> +      // This measurment should only be included when telemetry is

measurement

@@ +59,5 @@
> +    return function (data) {
> +      let result = serializer(data);
> +
> +      // Special case the serialization of these fields so that
> +      // they are sent as objects, not strinigified objects.

stringified

@@ +62,5 @@
> +      // Special case the serialization of these fields so that
> +      // they are sent as objects, not strinigified objects.
> +      if (result['translationOpportunityCountsByLanguage']) {
> +        result['translationOpportunityCountsByLanguage'] = JSON.parse(
> +          result['translationOpportunityCountsByLanguage']);

Nit: double quotes throughout.

@@ +67,5 @@
> +      }
> +
> +      if (result['pageTranslatedCountsByLanguage']) {
> +        result['pageTranslatedCountsByLanguage'] = JSON.parse(
> +          result['pageTranslatedCountsByLanguage']);

Lift out

  _parseInPlace: function(o, k) {
    if (k in o) {
      o[k] = JSON.parse(o[k]);
    }
  },

@@ +77,5 @@
> +});
> +
> +this.TranslationProvider = function () {
> +  Metrics.Provider.call(this);
> +}

;

@@ +115,5 @@
> +      }.bind(this));
> +    });
> +  },
> +
> +  recordTranslation: function (language, numCharacters, date=new Date()) {

recordTranslation and recordTranslationOpportunity are 99% identical. Break out a helper, perhaps named something like "recordIncrementedFieldForKey".

::: browser/components/translation/Translation.jsm
@@ +215,5 @@
> +   *        The language of the page.
> +   * @param numCharacters
> +   *        The number of characters that were translated
> +   */
> +   recordTranslation: function (language, numCharacters) {

These two methods are very similar. Let's reduce the opportunity for error by sharing logic.

::: browser/components/translation/test/unit/test_healthreport.js
@@ +1,4 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict"

;

@@ +2,5 @@
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict"
> +
> +const {utils: Cu, classes: Cc, interfaces: Ci} = Components;

Cc, Ci not used.

@@ +12,5 @@
> +
> +// Create a profile
> +let gProfile = do_get_profile();
> +
> +let run_test = run_next_test;

Err wat?

@@ +14,5 @@
> +let gProfile = do_get_profile();
> +
> +let run_test = run_next_test;
> +
> +// At end of test, restore original state

Trailing period.

@@ +22,5 @@
> +  Services.prefs.setBoolPref("toolkit.telemetry.enabled",
> +                             ORIGINAL_TELEMETRY_ENABLED);
> +});
> +
> +add_task(function test_contructor() {

test_constructor

@@ +60,5 @@
> +  Assert.equal(day.get("translationOpportunityCount"), 1);
> +
> +  Assert.ok(day.has("translationOpportunityCountsByLanguage"));
> +  let countsByLanguage = JSON.parse(day.get("translationOpportunityCountsByLanguage"));
> +  Assert.equal(countsByLanguage['fr'], 1);

Double quotes throughout.

Comment 28

[Steven MacLeod [:smacleod]]

(In reply to Richard Newman [:rnewman] from comment #27)
> Comment on attachment 8427412 [details] [diff] [review]
> Patch 2 - Setup FHR provider for Translation project.
> 
> Review of attachment 8427412 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems sane to me. One substantive question for what's going on with
> run_next_test (tests shouldn't need to fudge with their harness), but we've
> hit my nit limit, so let's do one last pass for a rubberstamp before landing.

This was just a shortcut so that I wouldn't have to write:
> function run_test() { run_next_test(); }
I've changed it anyway though.


> > +function TranslationMeasurement1() {
> 
> Elsewhere you use the pattern
> 
>   this.TranslationMeasurement1 = function...
> 
> IIRC in this context it doesn't matter which you use, but please be
> consistent.

I'm just following the convention I see where exported names are define using "this.<name> = ..." and then "function <name> () { ... }" for non-exported names.

Comment 29

[Steven MacLeod [:smacleod]]

Attachment: Patch 2 - Setup FHR provider for Translation project. v2

I've updated the patch based on the feedback. I also introduced more data to the measurement to make Bug 977770 simpler and so we wouldn't have to version the measurement immediately after landing it.

Felipe, I've also added in the method call to actually track translation opportunities (fixing Bug 973293). Where I have it now it doesn't take into account the prefs for never showing for a language or domain etc. as I wasn't sure exactly what we wanted to track. It's trivial to move it though, so what do you think?

Comment 30

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8431683 [details] [diff] [review]
Patch 2 - Setup FHR provider for Translation project. v2

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

r+ for the interaction with the translation system. I'll leave the FHR specific details for rnewman.

> Felipe, I've also added in the method call to actually track translation
> opportunities (fixing Bug 973293). Where I have it now it doesn't take into
> account the prefs for never showing for a language or domain etc. as I
> wasn't sure exactly what we wanted to track. It's trivial to move it though,
> so what do you think?

I think it is almost in the right place, but we also want to track an opportunity for users who have TRANSLATION_PREF_SHOWUI = false.

I guess that function will need to be modified a bit to account for this, as that main if() clause will need to happen regardless of the pref. But it should be simple.

Comment 31

[Steven MacLeod [:smacleod]]

(In reply to :Felipe Gomes from comment #30) 
> I think it is almost in the right place, but we also want to track an
> opportunity for users who have TRANSLATION_PREF_SHOWUI = false.
> 
> I guess that function will need to be modified a bit to account for this, as
> that main if() clause will need to happen regardless of the pref. But it
> should be simple.

Something like?

languageDetected: function(aBrowser, aDetectedLanguage) {
  if (this.supportedSourceLanguages.indexOf(aDetectedLanguage) == -1 ||
      aDetectedLanguage == this.defaultTargetLanguage)
    return;

  TranslationHealthReport.recordTranslationOpportunity(aDetectedLanguage);
  
  if (!Services.prefs.getBoolPref(TRANSLATION_PREF_SHOWUI))
    return;

  if (!aBrowser.translationUI)
    aBrowser.translationUI = new TranslationUI(aBrowser);

    
  aBrowser.translationUI.showTranslationUI(aDetectedLanguage);
}

Comment 32

[:Felipe Gomes (needinfo for replies!)]

Yep!

Comment 33

[Richard Newman [:rnewman]]

Comment on attachment 8431683 [details] [diff] [review]
Patch 2 - Setup FHR provider for Translation project. v2

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

See Comment 27 for review comments not yet addressed.

::: browser/components/translation/Translation.jsm
@@ +1,2 @@
>  /* 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,

See previous review comment re outdated template.

@@ +11,2 @@
>  
>  const {classes: Cc, interfaces: Ci, utils: Cu} = Components;

See previous review comment.

@@ +404,5 @@
> +  recordTranslation: function (langFrom, langTo, numCharacters, date=new Date()) {
> +    let m = this.getMeasurement(TranslationMeasurement1.prototype.name,
> +                                TranslationMeasurement1.prototype.version);
> +
> +    return this._enqueueTelemetryStorageTask(function* recordTask(){

See previous review comment re breaking out a helper.

::: browser/components/translation/test/unit/test_healthreport.js
@@ +1,4 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict"

See previous review comment (trailing ;)

@@ +2,5 @@
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict"
> +
> +const {utils: Cu, classes: Cc, interfaces: Ci} = Components;

See previous review comment.

Comment 34

[Steven MacLeod [:smacleod]]

Attachment: Patch 2 - Setup FHR provider for Translation project. v3

(In reply to Richard Newman [:rnewman] from comment #33) 
> ::: browser/components/translation/Translation.jsm
> @@ +1,2 @@
> >  /* 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,
> 
> See previous review comment re outdated template.

This isn't the same file you commented on, I didn't introduce this template. I'll attach another patch to update it.

> 
> @@ +11,2 @@
> >  
> >  const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
> 
> See previous review comment.

This is a different file. All three of these are used.

> 
> @@ +404,5 @@
> > +  recordTranslation: function (langFrom, langTo, numCharacters, date=new Date()) {
> > +    let m = this.getMeasurement(TranslationMeasurement1.prototype.name,
> > +                                TranslationMeasurement1.prototype.version);
> > +
> > +    return this._enqueueTelemetryStorageTask(function* recordTask(){
> 
> See previous review comment re breaking out a helper.

I did break logic out into two helpers, "_enqueueTelemetryStorageTask" and "_getDailyLastTextFieldAsJSON". The logic for "recordTranslation()" has also
changed so they are doing different things with the JSON in the field.

I don't believe it's worth trying to break anything else out, but if you can
see something obvious I'm missing please provide an example.


> 
> ::: browser/components/translation/test/unit/test_healthreport.js
> @@ +1,4 @@
> > +/* Any copyright is dedicated to the Public Domain.
> > +   http://creativecommons.org/publicdomain/zero/1.0/ */
> > +
> > +"use strict"
> 
> See previous review comment (trailing ;)
> 
> @@ +2,5 @@
> > +   http://creativecommons.org/publicdomain/zero/1.0/ */
> > +
> > +"use strict"
> > +
> > +const {utils: Cu, classes: Cc, interfaces: Ci} = Components;
> 
> See previous review comment.

Ah, missed the test file, my bad. Thanks for catching.

Comment 35

[Steven MacLeod [:smacleod]]

Attachment: Patch 3 - Update license header.

Comment 36

[Steven MacLeod [:smacleod]]

Attachment: Patch 2 - Setup FHR provider for Translation project. v4

Missed patch when squashing, same as previous but includes change to "languageDetected" suggested by Felipe.

Comment 37

[Richard Newman [:rnewman]]

Comment on attachment 8432324 [details] [diff] [review]
Patch 2 - Setup FHR provider for Translation project. v4

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

r+ if the tests failed with that typo, or you add tests such that they do!

Nits inline.

::: browser/components/translation/Translation.jsm
@@ +217,5 @@
> +        if (provider) {
> +          provider.recordTranslationOpportunity(language);
> +        }
> +      },
> +      Cu.reportError);

_withProvider: function (f) {
  this._getProvider().then(
    (provider) => {
      if (provider) {
        f(provider);
      }
    },
    Cu.reportError);
},

recordTranslationOpportunity: function (language) {
  this._withProvider((provider) => provider.recordTranslationOpportunity(language));
},

recordTranslation: function (langFrom, langTo, numCharacters) {
  this._withProvider((provider) => provider.recordTranslation(langFrom, langTo, numCharacters));
},

recordLanguageChange: ...

etc.?

You can inline _getProvider into _withProvider; it's not used elsewhere.

I'm a big fan of eliminating boilerplate.

@@ +233,5 @@
> +  recordTranslation: function (langFrom, langTo, numCharacters) {
> +    this._getProvider().then(
> +      (provider) => {
> +        if (provider) {
> +          provider.recordTranslation(langFrom, LangTo, numCharacters);

Typo: LangTo should be langTo. I thought this had test coverage? If not, it's worth adding some.

@@ +388,5 @@
> +  recordTranslationOpportunity: function (language, date=new Date()) {
> +    let m = this.getMeasurement(TranslationMeasurement1.prototype.name,
> +                                TranslationMeasurement1.prototype.version);
> +
> +    return this._enqueueTelemetryStorageTask(function* recordTask(){

Nit: space between () {.

@@ +407,5 @@
> +  recordTranslation: function (langFrom, langTo, numCharacters, date=new Date()) {
> +    let m = this.getMeasurement(TranslationMeasurement1.prototype.name,
> +                                TranslationMeasurement1.prototype.version);
> +
> +    return this._enqueueTelemetryStorageTask(function* recordTask(){

Same.

Comment 38

[Richard Newman [:rnewman]]

Question: does this feature apply to Android? If so, we'll need a new bug to implement similar recording there.

Comment 39

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Richard Newman [:rnewman] from comment #37)
> r+ if the tests failed with that typo, or you add tests such that they do!

AFAIUI, we would need to add a browser-chrome mochitest to actually test the FHR service and see whether the values are recorded as given. We should do this as a follow-up if we really want to do it.

Comment 40

[Tim Taubert [:ttaubert] (inactive)]

Filed bug 1019731.

Comment 41

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/5dc75955fb8f

Comment 42

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Richard Newman [:rnewman] from comment #38)
> Question: does this feature apply to Android? If so, we'll need a new bug to
> implement similar recording there.

This lives in browser/ for now. Adding Android support shouldn't be too hard but definitely its own project.

Comment 43

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5dc75955fb8f