Do not ship Feedback XPI with Firefox 4 final

RESOLVED FIXED in mozilla4.0b10

Status

defect
RESOLVED FIXED
9 years ago
5 months ago

People

(Reporter: mossop, Assigned: mossop)

Tracking

({qawanted})

Trunk
mozilla4.0b10
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [hardblocker])

Attachments

(2 attachments, 2 obsolete attachments)

Also decide if we want to add it to removed files to delete it from users who update.
Assignee

Updated

9 years ago
blocking2.0: --- → final+
Morphing slightly:

 - we do NOT want to ship the Feedback XPI with Firefox 4 final
 - we DO want to keep the Feedback XPI running for Firefox 4 users who joined our beta program (ie: we don't want to remove it from users who update)
Summary: Turn off Feedback for Firefox 4 final → Do not ship Feedback XPI with Firefox 4 final
So there are no surprises, the consequences of the current way we package Feedback are that if we leave it there for users who upgrade then they still won't have the option to uninstall it through the add-ons manager, they will be able to disable it.
Mossop, adding feedback is keyed off update channel set to beta ? So that it won't be packaged when we switch to release for 4.0rc1 build1 ?
That is probably true, we'll still need to turn this off proper so we don't end up shipping it with the 4.0.x spins right?
You mean with the 4.0.x versions that have the beta channel set? I actually think we *want* the Feedback XPI included with those.
From code inspection (danger, will robinson!) we only package testpilot at build time when the update channel is beta
  http://mxr.mozilla.org/mozilla-central/source/browser/installer/package-manifest.in#412
  http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/extensions/Makefile.in#49
So when we switch from beta to release at first 4.0rc1 build the exe/dmg/tar.bz's from won't contain testpilot. Historically, the update channel has changed then so that all the RCs are genuine candidates for final, without need for further recompile. That's all for new installs.

For existing installs, if we don't add the testpilot files to removed-files.in then _complete_ updates to 4.0rc1 (or later) will not remove it. But I expect a partial update between the last beta and 4.0rc1 will remove them, because it calculates a list of files that have been removed between the two mar files. Is that a problem, or has the extension already been copied to the profile ?
(In reply to comment #7)
> From code inspection (danger, will robinson!) we only package testpilot at
> build time when the update channel is beta
>  
> http://mxr.mozilla.org/mozilla-central/source/browser/installer/package-manifest.in#412
>  
> http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/extensions/Makefile.in#49
> So when we switch from beta to release at first 4.0rc1 build the
> exe/dmg/tar.bz's from won't contain testpilot. Historically, the update channel
> has changed then so that all the RCs are genuine candidates for final, without
> need for further recompile. That's all for new installs.

So sounds like there is nothing we need to do here to stop this shipping in final. If we want to make it ship in the 4.0.x RCs then we'd need to take action but that doesn't sound desirable.

> For existing installs, if we don't add the testpilot files to removed-files.in
> then _complete_ updates to 4.0rc1 (or later) will not remove it. But I expect a
> partial update between the last beta and 4.0rc1 will remove them, because it
> calculates a list of files that have been removed between the two mar files. Is
> that a problem, or has the extension already been copied to the profile ?

I do not believe that partial mars remove files between builds do they?
So I guess the only way to leave it in lace would be to only produce complete mars for beta -> rc/final updates?

Updated

9 years ago
Assignee: nobody → dtownsend

Updated

9 years ago
Whiteboard: [hardblocker]
Whiteboard: [hardblocker] → [hardblocker][hardblocker]
Whiteboard: [hardblocker][hardblocker] → [hardblocker]
Yup, so I think that makes this a releng bug, yes?
Keywords: qawanted
Tentatively over to Nick then, who I'm sure will reassign to the right person or tell me if I'm smoking crack.
Assignee: dtownsend → nrthomas

Comment 13

9 years ago
It doesn't sound like there's any changes I need to make in the Feedback code for this bug, right?  I just wanted to double-check that.  Please let me know ASAP if there are.
Don't think so, but what does Mossop think ?
Posted patch Potential fixSplinter Review
Ben, how about a temporary change in m-c like this ? (untested)

We'd have to move the UPDATE_PACKAGING_RXX tag to this, and revert it afterwards, but this should get us automatically-generated partials that don't remove Feedback at the bN -> RC1 transition.
Attachment #503403 - Flags: feedback?(bhearsum)
Seems OK to me. Am I right in thinking that 4.0[rcN|.N] -> 4.0[rcN|.N] will work because the from version doesn't have Test Pilot, and thus make_incremental_updates.py doesn't think to remove it?
Attachment #503403 - Flags: feedback?(bhearsum) → feedback+
Just to flip flop a little. If we take bug 474289 before the next beta then we can actually fix this by just adding feedback to the removed-files list as normal. Going to try to make that happen.
Assignee

Updated

9 years ago
Depends on: 474289
Taking to do the repackaging magic now that bug 474289 is fixed
Assignee: nrthomas → dtownsend
The current plan requires that we have the repackaging changes in place by the last beta, unfortunately I've realised some snags which might make both options we've thought of so far unusable
blocking2.0: final+ → betaN+
Here is the problem. In order to save time and hassle and still have Feedback be localised we placed all of the locale files in with the main browser locales, so Feedback's strings end up in en-US.jar.

If we were to follow the original plan here and just stop shipping feedback then users upgrading from beta to RC would have the feedback UI and code but no strings which would be broken.

For the new plan to ship feedback as a distribution add-on we need to do some additional steps to make the localised strings be a part of the extension rather than having then end up in en-US.jar.

This patch is my first pass at doing that, it is probably hacky and I'm hoping Axel can make a better suggestion but it does appear to work, en-US builds have the extension in the right place and a l10n repack correctly replaces the strings in the extension with one problem, the language code on the locale line in chrome.manifest for the extension doesn't get updated. This is probably not a big deal as it will still load the strings, I verified this in a German repack.

Basic plan of attack is this:

1. Have the localised strings get put in a jar outside of the normal chrome directory (dist/xpi-stage/feedback was chosen). 
2. Have the main en-US packaging code copy the additional strings and chrome.manifest from there into the extension's directory.
3. When the langpack build runs the new feedback strings end up in a slightly odd place, dist/xpi-stage/xpi-stage/feedback.
4. Before the repackage-zip step copy those into the langpack xpi directory
5. The repackage-zip step then copies them into the right place before packaging up the build.
Attachment #505615 - Flags: feedback?(l10n)
I'd personally just go on and make the feedback l10n files be part of fx unconditionally. Beta channel or not. For the dashboard, they're not conditional anyway, so localizers will assume they have to do it, if we package them or not.

That should be a two line change in jar.mn, right?
Posted patch packaging patch rev 1 (obsolete) — Splinter Review
Well I guess that is a simpler solution. This packages the feedback add-on as a packed xpi and adds the old location to removed-files.in as well as always packages the localisations.
Attachment #505668 - Flags: review?(robert.bugzilla)
Comment on attachment 505668 [details] [diff] [review]
packaging patch rev 1

>diff --git a/browser/installer/removed-files.in b/browser/installer/removed-files.in
>--- a/browser/installer/removed-files.in
>+++ b/browser/installer/removed-files.in
>@@ -89,16 +89,133 @@ defaults/profile/extensions/{972ce4c6-7e
>...
>+extensions/testpilot@labs.mozilla.com/skin/all/badge-default.png
>+extensions/testpilot@labs.mozilla.com/skin/all/bg.jpg
>+extensions/testpilot@labs.mozilla.com/skin/all/css/screen-standalone.css
>+extensions/testpilot@labs.mozilla.com/skin/all/dino_32x32.png
>+extensions/testpilot@labs.mozilla.com/skin/all/images
You've included the images directory
Comment on attachment 505668 [details] [diff] [review]
packaging patch rev 1

>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
>@@ -416,17 +416,17 @@
> 
> ; [Browser Chrome Files]
> @BINPATH@/chrome/browser@JAREXT@
> @BINPATH@/chrome/browser.manifest
> @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
> #if MOZ_UPDATE_CHANNEL == beta
>-@BINPATH@/extensions/testpilot@labs.mozilla.com/*
>+@BINPATH@/distribution/extensions/testpilot@labs.mozilla.com/*
Shouldn't this should be testpilot@labs.mozilla.com.xpi?
Comment on attachment 505668 [details] [diff] [review]
packaging patch rev 1

>diff --git a/browser/app/profile/extensions/Makefile.in b/browser/app/profile/extensions/Makefile.in
>--- a/browser/app/profile/extensions/Makefile.in
>+++ b/browser/app/profile/extensions/Makefile.in
>@@ -35,27 +35,31 @@
>...
> ifeq (beta,$(MOZ_UPDATE_CHANNEL))
> EXTENSIONS = \
>   testpilot@labs.mozilla.com \
>   $(NULL)
> 
>-define _INSTALL_EXTENSIONS
>-$(PYTHON) $(topsrcdir)/config/nsinstall.py $(wildcard $(srcdir)/$(dir)/*) $(DIST)/bin/extensions/$(dir)
>+define _INSTALL_EXTENSION
>+cd $(srcdir)/$(dir) && \
>+  $(ZIP) -r9D $(DISTROEXT)/$(dir).xpi *
What do you think of passing X to exclude extra file attributes as well?

Minusing to verify the fixes in an updated patch since if there are any problems they won't be caught until the beta is built.
Attachment #505668 - Flags: review?(robert.bugzilla) → review-
Comment on attachment 505615 [details] [diff] [review]
Change packaging and mangle locales

I like the new patch, so feedback- on this one.
Attachment #505615 - Flags: feedback?(l10n) → feedback-
Fixes the problems and the packaged add-on to removed-files for non-beta-channel builds so the RCs will not include it and will remove it from the app dir on upgrade from b10 or b11.

Ran tests by installing on windows over b9 with a nightly with the beta channel and then a nightly without the beta channel, in the first case feedback correctly got moved to the distro dir and installed as a profile add-on then in the second it got removed from the distro dir.
Attachment #505668 - Attachment is obsolete: true
Attachment #505872 - Flags: review?
Landed: http://hg.mozilla.org/mozilla-central/rev/60bd77aec10e
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite-
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b10
Attachment #505615 - Attachment is obsolete: true
Component: Build Config → General
Product: Firefox → Firefox Build System
Keywords: qawanted
Target Milestone: Firefox 4.0b10 → mozilla4.0b10
You need to log in before you can comment on or make changes to this bug.