Breaking down Story into multiple smaller, easier-to-estimate bugs. These individual bugs are dependencies which block the completion of the whole story. The team provides point estimates to each of the individual bugs instead of the entire story.

Mass move of translation bugs to the new Translation component.

This won't be fixed by (and thus doesn't depend on) bug 1015525 as that only records the state of the translation prefs. This bug is about recording when a user clicks the "No, thanks" button when we offer to translate.

Marco, can you please update the backlog and assign this bug to Mano? Steven is out for some training in Toronto this week. Thanks!

Iteration 32.3 Backlog updated with Mano assigned to Bug 977774.

Hi Tim, based on today's update meeting you will confirm if this bug should be removed from the current iteration based on the progress made on the dependent Bug 978158.

I'll land bug 978158 later today.

Comment on attachment 8437486 [details] [diff] [review] add the metric and test it Review of attachment 8437486 [details] [diff] [review]: ----------------------------------------------------------------- Is the caller for recordDeniedTranslationOffer part of a different patch? just a note in advance: if the user clicks "Not Now", and then retrieves the infobar through the urlbar icon, and clicks not now again, I think we should not count that twice. (i.e, count only one dismissal per page)

(In reply to :Felipe Gomes from comment #8) > Comment on attachment 8437486 [details] [diff] [review] > add the metric and test it > > Review of attachment 8437486 [details] [diff] [review]: > ----------------------------------------------------------------- > > Is the caller for recordDeniedTranslationOffer part of a different patch? > Yes, that was my plan (in that patch, I'm also adding a test for the UI part). Please review this part of the work. > just a note in advance: if the user clicks "Not Now", and then retrieves the > infobar through the urlbar icon, and clicks not now again, I think we should > not count that twice. (i.e, count only one dismissal per page) Unfortunately, the TranslationUI object is designed in such a way that it's browser-centric, not document-centric. By resetting some flag in documentStateReceived for STATE_OFFER data, I could introduce some "document-initialization" mechanism. That would make this edge-case fixable. However, a more obvious and common case seems un-fixable in the current design: (1) Not Now a translation offer; (2) Click on some link; (3) Go back; (4) Not Now again. So, unless you think that this edge case is important enough to fix (leaving the back & foward issue in place), I'm inclined to keep it simple and just track not-now commands activation. Moving forward, probably in a new bug, I think that the content script should send a distinct message when a translation "session" beings. It could also report back & forward/whole-now state.

One more thing: the generic [x] button is in place in the "offer" state alongside the "Not Now" button. In practice, they do the same thing. Should it count for this metric? One may argue that if the user closed the info-bar this way, s/he might have closed the notification without reading its content at all.

Comment on attachment 8437486 [details] [diff] [review] add the metric and test it Review of attachment 8437486 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/healthreport/docs/dataformat.rst @@ +1553,5 @@ > Integer count of the number of times the user manually adjusted the detected > language after having first translated the page. > +deniedTranslationOffer > + Integer count of the numbers of times the user opted-out offered > + page translation. Please expand this comment by saying it's by clicking the Not Now or X close button in the translation infobar

(In reply to Mano (needinfo? for any questions; not reading general bugmail) from comment #9) > So, unless you think that this edge case is important enough to fix (leaving > the back & foward issue in place), I'm inclined to keep it simple and just > track not-now commands activation. Makes sense, let's keep it simple. > Moving forward, probably in a new bug, I think that the content script > should send a distinct message when a translation "session" beings. It could > also report back & forward/whole-now state. I didn't understand exactly what you mean by a translation "session", but let's leave it for another bug (In reply to Mano (needinfo? for any questions; not reading general bugmail) from comment #10) > One more thing: the generic [x] button is in place in the "offer" state > alongside the "Not Now" button. In practice, they do the same thing. Should > it count for this metric? One may argue that if the user closed the info-bar > this way, s/he might have closed the notification without reading its > content at all. I think it should count. Even if they didn't read it they are dismissing the offer, so it's still meaningful

Comment on attachment 8444099 [details] [diff] [review] the rest of it Review of attachment 8444099 [details] [diff] [review]: ----------------------------------------------------------------- The test looks great to me, excluding some nits marked below. The rest of the patch lgtm as well but I'd like to let Felipe give the final review. ::: browser/components/translation/test/browser_translation_fhr.js @@ +9,5 @@ > > +let MetricsChecker = { > + _metrics: { pageCount: 0, charCount: 0, deniedOffers: 0 }, > + _metricsTime: new Date(), > + _midnightError: new Error("Cannot get metrics on midnight"), Nit: Maybe "Getting metrics around midnight may fail sometimes"? @@ +31,5 @@ > + > + // .get() may return `undefined`, which we can't compute. > + this._metrics = { pageCount: day.get("pageTranslatedCount") || 0, > + charCount: day.get("charactersTranslatedCount") || 0, > + deniedOffers: day.get("deniedTranslationOffer") || 0 }; Formatting nit: this._metrics = { pageCount: day.get("pageTranslatedCount") || 0, charCount: day.get("charactersTranslatedCount") || 0, deniedOffers: day.get("deniedTranslationOffer") || 0 }; @@ +34,5 @@ > + charCount: day.get("charactersTranslatedCount") || 0, > + deniedOffers: day.get("deniedTranslationOffer") || 0 }; > + this._metricsTime = metricsTime; > + }), > + Nit: white space @@ +35,5 @@ > + deniedOffers: day.get("deniedTranslationOffer") || 0 }; > + this._metricsTime = metricsTime; > + }), > + > + checkAdditions: function* (additions) { checkAdditions: Task.async(function* (additions) { ... @@ +40,5 @@ > + let prevMetrics = this._metrics, prevMetricsTime = this._metricsTime; > + try { > + yield this._updateMetrics(); > + } > + catch(ex if ex == this._midnightError) { Nit: } catch (ex ...) { @@ +48,5 @@ > + // Check that it's still the same day of the month as when we started. This > + // prevents intermittent failures when the test starts before and ends after > + // midnight. > + if (this._metricsTime.getDate() != prevMetricsTime.getDate()) { > + for (let metric in prevMetrics) { for (let metric of Object.keys(prevMetrics)) { @@ +53,5 @@ > + prevMetrics[metric] = 0; > + } > + } > + > + for (let metric in additions) { Object.keys() ... @@ +130,5 @@ > + return Task.spawn(function* task_accept_translation_offer() { > + let browser = tab.linkedBrowser; > + getInfobarElement(browser, "toLanguage").value = to; > + getInfobarElement(browser, "toLanguage").doCommand(); > + yield waitForMessage(tab.linkedBrowser, "Translation:Finished"); Nit: yield waitForMessage(browser, ...); @@ +140,5 @@ > + let tab = yield offerTranslatationFor(text, from); > + yield acceptTranslationOffer(tab, to); > + if (closeTab) > + gBrowser.removeTab(tab); > + return tab; if (closeTab) { gBrowser.removeTab(tab); } else { return tab; } Returning the tab after closing it feels weird :)

Comment on attachment 8444311 [details] [diff] [review] comments addressed Review of attachment 8444311 [details] [diff] [review]: ----------------------------------------------------------------- r+ with renaming the new function in TranslationUI to infobarClosed and moving the state check inside it

sorry had backout this change in https://tbpl.mozilla.org/?tree=Fx-Team&onlyunstarred=1&rev=0b7d0cbaa2bd since one of this 2 patches seems to have caused test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=42331284&tree=Fx-Team

Relanded along with the patch for bug 1029363 that should fix that failure. https://hg.mozilla.org/integration/fx-team/rev/b37e754928cb

Comment on attachment 8444311 [details] [diff] [review] comments addressed [Approval Request Comment] Bug caused by (feature/regressing bug #): This bug is part of the automatic translation feature, which we want to A/B with a subset of Aurora 32 users. User impact if declined: Data about users clicking the "Not now" button won't be collected Testing completed (on m-c, etc.): landed on m-c Risk to taking this patch (and alternatives if risky): small String or IDL/UUID changes made by this patch: none

Felipe, note that you must land https://bugzilla.mozilla.org/show_bug.cgi?id=1029363 along with this patch (when I filed that one, I didn't realize tinderbox builds testing is actually in that on-its-own configuration, thus thought the patch here wouldn't be backed out).

https://hg.mozilla.org/releases/mozilla-aurora/rev/aa4cb3562ef1 https://hg.mozilla.org/releases/mozilla-aurora/rev/7717946aeb52