573079 - Land Feedback XPI in the tree for 1.9.3 releases

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Dave Townsend [:mossop]]

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

Comment 1

[Dave Townsend [:mossop]]

Attachment: WIP build config changes

Comment 2

[Robert Strong (they/them - no direct email)]

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

Comment 3

[Robert Strong (they/them - no direct email)]

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.

Comment 4

[Dave Townsend [:mossop]]

(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.

Comment 5

[Nick Thomas [:nthomas] (UTC+12)]

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.

Comment 6

[Dave Townsend [:mossop]]

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

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

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?)

Comment 8

[Dave Townsend [:mossop]]

(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.

Comment 9

[Nick Thomas [:nthomas] (UTC+12)]

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.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

(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)

Comment 11

[Dave Townsend [:mossop]]

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

Comment 12

[Robert Strong (they/them - no direct email)]

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

Comment 13

[Dave Townsend [:mossop]]

Attachment: Import Test Pilot code into the tree

This just imports the Feedback XPI into the tree

Comment 14

[Dave Townsend [:mossop]]

Attachment: Build config for feedback XPI

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 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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 16

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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;

Comment 17

[Dave Townsend [:mossop]]

Attachment: Import Test Pilot code into the tree

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

Comment 18

[Dave Townsend [:mossop]]

Attachment: Build config for feedback XPI

And this is just the build config hopefully

Comment 19

[Jono Xia]

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

Comment 20

[Ed Lee :Mardak]

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

Comment 21

[Dave Townsend [:mossop]]

(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 22

[(not currently active) Ted Mielczarek]

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/[e-mail]/*

You probably want #if MOZ_UPDATE_CHANNEL == beta here

Comment 23

[Axel Hecht [:Pike]]

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

Comment 25

[Axel Hecht [:Pike]]

(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.

Comment 26

[Dave Townsend [:mossop]]

Attachment: Import Test Pilot code into the tree

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.

Comment 27

[Dave Townsend [:mossop]]

Attachment: Locale packaging and study disabling

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 28

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 453899 [details] [diff] [review]
Import Test Pilot code into the tree

I read every line.

Comment 29

[Dave Townsend [:mossop]]

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

Comment 30

[Francesco Lodolo [:flod]]

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"

Comment 31

[Axel Hecht [:Pike]]

I don't understand the code piece around http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/extensions/[e-mail]/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.

Comment 32

[Rimas Kudelis [use needinfo to ping]]

(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?

Comment 33

[Rimas Kudelis [use needinfo to ping]]

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

Comment 34

[Dave Townsend [:mossop]]

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

Comment 35

[Axel Hecht [:Pike]]

(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.

Comment 36

[Dave Townsend [:mossop]]

(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

Comment 37

[Axel Hecht [:Pike]]

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

Comment 38

[Jono Xia]

(In reply to comment #31)
> I don't understand the code piece around
> http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/extensions/[e-mail]/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.