General tracking bug for getting the Feedback XPI landed in tree.

We'll have trouble removing this with app update since app update can only remove files... same thing happened with talkback and DOMi.

As a workaround, the dir could be removed on Windows via the helper app that runs after update. This shouldn't be a problem on Mac and Linux since the user that performs the update will have write access and the Add-ons Mgr should remove the empty directories.

(In reply to comment #2) > We'll have trouble removing this with app update since app update can only > remove files... same thing happened with talkback and DOMi. Beltzner tells me that we don't expect to remove this, just stop packaging it.

Is the target for this _all_ builds, or just builds we're going to ship to end users ? I think we'd want a configure target if it's the latter.

My understanding was all builds (possibly all builds on the branch) but I shall let beltzner confirm

Once we branch and start doing nightly builds off that branch, I think we should include the Feedback Add-On in those builds as well, yes. (Was that the question?)

(In reply to comment #7) > Once we branch and start doing nightly builds off that branch, I think we > should include the Feedback Add-On in those builds as well, yes. (Was that the > question?) I think that is an answer. But I understand that we will be releasing the first beta off trunk, is that correct? If so then we need some more magic to make it only appear in the beta and not in trunk nightlies.

Sorry, I was trying to see if there was a distinction between developer and home brew builds and those that come of the MoCo build infra (nightlies & betas). A configure flag would allow us to default to not packaging Feedback, and then modify the mozconfigs for nightlies/betas so we do. Resolves the trunk nightly issue too.

(In reply to comment #9) > Sorry, I was trying to see if there was a distinction between developer and > home brew builds and those that come of the MoCo build infra (nightlies & > betas). A configure flag would allow us to default to not packaging Feedback, > and then modify the mozconfigs for nightlies/betas so we do. Resolves the trunk > nightly issue too. That works for me - we can just make it a configure flag, maybe something like "--is-beta" which can be used now and in the future to include all the beta metrics bits (/me dreams of a future with deeper instrumentation and things that scare privacy advocates)

Do we already have a flag along these lines? I seem to recall an old one. If not maybe --enable-beta-metrics or something?

I don't think there is a flag but we do evaluate based on the app version for other things http://mxr.mozilla.org/mozilla-central/source/config/printprereleasesuffix.py Not sure if that would meet all of the conditions though

This just imports the Feedback XPI into the tree

This is the build config bits to make the testpilot code make it into the final package. The decision was that we can just do this for builds where the update-channel is set to "beta" as this currently only happens for beta releases.

Comment on attachment 453220 [details] [diff] [review] Import Test Pilot code into the tree r=me on the import, but did not review the actual full test pilot code

Comment on attachment 453221 [details] [diff] [review] Build config for feedback XPI And for this patch, I think you need someone else to review :/ >+// this gets me a syntax error....? ^ ??? >+(function() { >+ var Cc = Components.classes; >+ var Cu = Components.utils; >+ var Ci = Components.interfaces;

Managed to munge that a bit, this is the real test pilot code import

And this is just the build config hopefully

> And for this patch, I think you need someone else to review :/ > > >+// this gets me a syntax error....? Hi Vlad, Sorry, I put that comment in during debugging and forget to take it out. The syntax error it referred to is fixed.

Do you care about commit history for line annotation/blame? For weave landing on trunk, it'll be landing from a hg convert.

(In reply to comment #20) > Do you care about commit history for line annotation/blame? For weave landing > on trunk, it'll be landing from a hg convert. Since this is just for use for the Firefox 4 betas I don't think it is worth the added hassle to do that.

Comment on attachment 453237 [details] [diff] [review] Build config for feedback XPI >diff --git a/browser/installer/package-manifest.in b/browser/installer/package-manifest.in >--- a/browser/installer/package-manifest.in >+++ b/browser/installer/package-manifest.in >@@ -354,6 +354,7 @@ > @BINPATH@/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/install.rdf > @BINPATH@/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/icon.png > @BINPATH@/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/preview.png >+@BINPATH@/extensions/testpilot@labs.mozilla.com/* You probably want #if MOZ_UPDATE_CHANNEL == beta here

Do you have an idea how to do this for l10n of feedback?

(In reply to comment #23) > Do you have an idea how to do this for l10n of feedback? We resolved this in a mail thread. We're going to add the "Feedback" strings to browser, and have the extension use them from there, so they're part of the regular Firefox localization.

This is the updated Test Pilot version that we are shipping. Vlad could you just rubber stamp the import again. The only difference from the real XPI version is that the locale files are now in Firefox's locale dir and the locale line is removed from the chrome.manifest.

This is the patch that enables packaging the locale part into Firefox's en-US.jar. There is also a pref set in the test automation script to stop testpilot downloading and prompting about studies during automated tests which stops it causing various focus related failures.

Comment on attachment 453899 [details] [diff] [review] Import Test Pilot code into the tree I read every line.

landed: http://hg.mozilla.org/mozilla-central/rev/7d25ab9aeef1 http://hg.mozilla.org/mozilla-central/rev/f3b7375747e9

l10n nitpick: * I think that these strings should be using the single unicode character … instead of ... (we did this change on Firefox years ago). * "testpilot.statusPage.extensions = %S estension" should probably be "%S extensions"

I don't understand the code piece around http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/extensions/testpilot@labs.mozilla.com/content/experiment-page.js#74, a) the mishmash between html code in the code and in l10n, and b) I don't find "upload-status" anywhere else.

(In reply to comment #30) > l10n nitpick: > * I think that these strings should be using the single unicode character … > instead of ... (we did this change on Firefox years ago). > * "testpilot.statusPage.extensions = %S estension" should probably be "%S > extensions" Can we perhps use our plural support here?

there's a mistake in testpilot.quitPage.quitFoever entity name

Please can you file bugs on the localisation issues in Mozilla Labs::Test Pilot so we can track and fix them all.

(In reply to comment #34) > Please can you file bugs on the localisation issues in Mozilla Labs::Test Pilot > so we can track and fix them all. (In reply to comment #15) > (From update of attachment 453220 [details] [diff] [review]) > r=me on the import, but did not review the actual full test pilot code I'd rather see a review of the test pilot code than file bugs on whatever triggers peoples attention.

(In reply to comment #35) > (In reply to comment #34) > > Please can you file bugs on the localisation issues in Mozilla Labs::Test Pilot > > so we can track and fix them all. > (In reply to comment #15) > > (From update of attachment 453220 [details] [diff] [review] [details]) > > r=me on the import, but did not review the actual full test pilot code > > I'd rather see a review of the test pilot code than file bugs on whatever > triggers peoples attention. A review of the code was performed in bug 561476

Well, that was apparently not good enough. I filed bug 575080, in the Firefox product as Labs doesn't have blocking flags.

(In reply to comment #31) > I don't understand the code piece around > http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/extensions/testpilot@labs.mozilla.com/content/experiment-page.js#74, > a) the mishmash between html code in the code and in l10n, and b) I don't find > "upload-status" anywhere else. "upload-status" will match the id of an html element provided in the web content of individual studies, server-side. That's why you don't see it anywhere else in the extension code. I'll add a comment explaining this. As for the mishmash of html code in the code and in main.properties, I'll fix that along with the other problems and attach a patch over in bug 575080.