As per spec defined in https://wiki.mozilla.org/Loop/Data_Collection#User_Satisfaction. Note that two different payloads should be sent for desktop client & standalone.

This is the first part of the work; it adds support for supplementary fields to the FeedbackAPIClient object, and validates them to generate a consistent payload. Conversation router has been updated to add information for platform through the use of navigator.platform, though adding information for channel and version will involve exposing data from gecko through the mozLoop API, which is to be addressed in part 2.

Forgot to mention: current patch sits on top of pending patch for bug 1048834 (https://bug1048834.bugzilla.mozilla.org/attachment.cgi?id=8467772)

This should do all that's needed to make the channel, version and OS available from navigator.mozLoop.

Comment on attachment 8468343 [details] [diff] [review] Part 2: Extend MozLoopAPI with channel, version and OS details. Review of attachment 8468343 [details] [diff] [review]: ----------------------------------------------------------------- Looks just fine to me, minor comments. ::: browser/components/loop/MozLoopAPI.jsm @@ +249,5 @@ > + version: appInfo.version, > + OS: appInfo.OS > + }; > + } > + }, Nit: trailing comma (not a big issue as we're in chrome) ::: browser/components/loop/test/mochitest/browser_mozLoop_appVersionInfo.js @@ +9,5 @@ > +add_task(loadLoopPanel); > + > +add_task(function* test_mozLoop_appVersionInfo() { > + registerCleanupFunction(function () { > + Services.prefs.clearUserPref("loop.test"); Do we need this here? @@ +12,5 @@ > + registerCleanupFunction(function () { > + Services.prefs.clearUserPref("loop.test"); > + }); > + > + Assert.ok(gMozLoopAPI, "mozLoop should exist"); Isn't this tested in browser_mozLoop_doNotDisturb.js already?

(In reply to Nicolas Perriault (:NiKo`) from comment #4) > Nit: trailing comma (not a big issue as we're in chrome) I know Mike prefers this to reduce loc change when we add more items. > Do we need this here? Nope, removed. > > + Assert.ok(gMozLoopAPI, "mozLoop should exist"); > > Isn't this tested in browser_mozLoop_doNotDisturb.js already? Yes, but I left it in, just as a confirmation, as they can be run separately etc.

Fixed comments, I think we can get this reviewed and do the extra bit in the follow-up.

Comment on attachment 8469263 [details] [diff] [review] Part 2: Extend MozLoopAPI with channel, version and OS details. Review of attachment 8469263 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/loop/MozLoopAPI.jsm @@ +17,5 @@ > +XPCOMUtils.defineLazyGetter(this, "appInfo", function() { > + return Cc["@mozilla.org/xre/app-info;1"] > + .getService(Ci.nsIXULAppInfo) > + .QueryInterface(Ci.nsIXULRuntime); > +}); this'll bitrot when bug 1000771 lands. @@ +243,5 @@ > + */ > + appVersionInfo: { > + enumerable: true, > + get: function() { > + return { You can cache this information, as it's not very likely to change during a session ;) Apart from that, this object will cross chrome-content boundaries, so you'll need to use `Cu.cloneInto(versionInfo, targetWindow);` ::: browser/components/loop/test/mochitest/browser_mozLoop_appVersionInfo.js @@ +15,5 @@ > + > + Assert.ok(appVersionInfo, "should have appVersionInfo"); > + > + Assert.equal(appVersionInfo.channel, > + Services.prefs.getCharPref("app.update.channel"), If this is what's going on in UpdateChannel.jsm, why not use fetching this pref instead of loading a module that does the same thing?

Rebased previous patch on top of latest fx-team. Note: Part 2 needs to update conversation.jsx to make the FeedbackAPIClient object to use the new data exposed by the mozLoop API.

Comment on attachment 8469363 [details] [diff] [review] (Part 1) - Update Loop FeedbackAPIClient to allow attaching more metadata. Review of attachment 8469363 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, r=Standard8

Updated for comments, and also extended to set the correct values for the feedback client.

Comment on attachment 8469979 [details] [diff] [review] Part 2: Extend Loop feedback details with channel, version and OS details. Review of attachment 8469979 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/loop/MozLoopAPI.jsm @@ +256,5 @@ > + */ > + appVersionInfo: { > + enumerable: true, > + get: function() { > + let defaults = Services.prefs.getDefaultBranch(null); oops, you forgot to cache this!

Now with the missing caching.

Comment on attachment 8470097 [details] [diff] [review] Part 2: Extend MozLoopAPI with channel, version and OS details. Review of attachment 8470097 [details] [diff] [review]: ----------------------------------------------------------------- Awesome, ship it!

https://hg.mozilla.org/integration/fx-team/rev/eea2155f85dd https://hg.mozilla.org/integration/fx-team/rev/05d5db0c856e

https://hg.mozilla.org/mozilla-central/rev/eea2155f85dd https://hg.mozilla.org/mozilla-central/rev/05d5db0c856e

Does this need manual test coverage or is the automation coverage sufficient?

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #16) > Does this need manual test coverage or is the automation coverage sufficient? Mark, can you please answer this?

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #16) > Does this need manual test coverage or is the automation coverage sufficient? I believe there's already (or soon to be added) manual testing for feedback, so extending it to ensure that version, channel, and OS are set correctly in the received report would be good.

Paul, can you please test this to make sure it's working correctly?

(In reply to Mark Banner (:standard8) (away until 23rd Oct) from comment #18) > I believe there's already (or soon to be added) manual testing for feedback, > so extending it to ensure that version, channel, and OS are set correctly in > the received report would be good. After discussing with :pauly we realized that input.mozilla.org wasn't currently displaying any UA information sent to it, preventing this bug to be verified. I've filed bug 1085344 against the Input product, which is bocking verification here.

qe- based on comment 20