1427712 - Integrate Beijing office's customizations into the full installer

Status: RESOLVED FIXED

(Firefox :: Installer, defect)

Description

[Hector Zhao [:hectorz]]

Firefox installers distributed by Beijing office offers several optional extensions, and we're maintaining our fork of the NSIS scripts to implement the UI. Our customizations was simplified with Fx 57.

This is the attempt to upstream our changes (gated behind optional configuration file inside distribution folder), hopefully we'll be able to start creating Fx China repacks for Windows as part of the partner_repacks.

Comment 1

[Hector Zhao [:hectorz]]

Attachment: Bug 1427712 - full installer page to install optional extensions with partner distribution.

Review commit: https://reviewboard.mozilla.org/r/209844/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/209844/

Comment 2

[Axel Hecht [:Pike]]

I like the idea of having less forks.

At the same time, I wonder what our product planning is for this. I know that some folks would like to see us only have a single installer at some point in the future, though not precisely defining what that one installer would do.

That question becomes a bit more pressing as this patch is technically opening up variants of Firefox in all languages, at least from an l10n POV.

So I'd love to know what the product vision is for regional variants of Firefox.

Just to throw out an alternative, could a install experience for the China customizations be delivered through Shield?

Comment 3

[Hector Zhao [:hectorz]]

(In reply to Axel Hecht [:Pike] from comment #2)
> 
> That question becomes a bit more pressing as this patch is technically
> opening up variants of Firefox in all languages, at least from an l10n POV.

IIUC, installers for different locales are currently built during the l10n repack? We're okay with further limiting the changes in this patch to zh-CN only, if that'll address your concerns.

Comment 4

[Molly Howell (she/her) (no longer active)]

(In reply to Axel Hecht [:Pike] from comment #2)

> So I'd love to know what the product vision is for regional variants of Firefox.

I'll CC Romain, who is the installer product manager. I don't feel that I understand your concern well enough to address it myself.

> Just to throw out an alternative, could a install experience for the China
> customizations be delivered through Shield?

We want bundled extensions to be installed already when the browser is first launched (from the user's perspective); the best that Shield or something like it could do would be to install them in the background after the browser has started, and that would be some pretty rough UX.

Comment 5

[Molly Howell (she/her) (no longer active)]

Comment on attachment 8939516 [details]
Bug 1427712 - full installer page to install optional extensions with partner distribution.

https://reviewboard.mozilla.org/r/209844/#review216030

I'm really happy to see this getting done, been looking forward to it for quite a while now. Need to work out a few issues with this patch first, though.

::: browser/installer/windows/nsis/installer.nsi:602
(Diff revision 1)
> +
> +  ${If} $InstallOptionalExtensions == "0"
> +  ${AndIf} ${FileExists} "$INSTDIR\distribution\setup.ini"
> +    StrCpy $0 0
> +    ${Do}
> +      ReadINIStr $1 "$INSTDIR\distribution\setup.ini" "OptionalExtensions" \

Need a ClearErrors before ReadINIStr (on the first iteration, after that the one at the end of the loop takes care of it).

::: browser/installer/windows/nsis/installer.nsi:610
(Diff revision 1)
> +        ${ExitDo}
> +      ${EndIf}
> +
> +      ${LogMsg} "Removing optional extension: $1"
> +      ${If} ${FileExists} "$INSTDIR\distribution\extensions\$1.xpi"
> +        Delete "$INSTDIR\distribution\extensions\$1.xpi"

I'd prefer if we could implement disabling the optional extensions by not copying the files into $INSTDIR in the first place. I realize that's more complicated to implement, but I think it's more robust; it's possible that copying the files over succeeds but then deleting them could fail (for instance, they might be locked by an antivirus that's trying to scan them).

::: browser/installer/windows/nsis/installer.nsi:1037
(Diff revision 1)
> +  ; Abort if no optional extensions configured in distribution/setup.ini
> +  ${If} ${FileExists} "$EXEDIR\core\distribution\setup.ini"
> +    ReadINIStr $0 "$EXEDIR\core\distribution\setup.ini" "OptionalExtensions" \
> +                                                        "extension.0.id"
> +    ${If} ${Errors}
> +      ClearErrors

ClearErrors needs to be before the ReadINIStr instruction, to make sure that ${If} ${Errors} doesn't find an error flag that was set by some earlier operation.

::: browser/installer/windows/nsis/installer.nsi:1341
(Diff revision 1)
>    WriteINIStr "$PLUGINSDIR\components.ini" "Field 2" Flags  "GROUP"
>  
> +  StrCpy $R9 0
> +  ${If} ${FileExists} "$EXEDIR\core\distribution\setup.ini"
> +    ${Do}
> +      ReadINIStr $R8 "$EXEDIR\core\distribution\setup.ini" \

Need a ClearErrors before ReadINIStr.

::: browser/installer/windows/nsis/installer.nsi:1347
(Diff revision 1)
> +        "OptionalExtensions" "extension.$R9.name"
> +      ${If} ${Errors}
> +        ${ExitDo}
> +      ${EndIf}
> +
> +      ; "R8" or "extension.0.name" should be text of "Field 3"

I'm not sure this comment is very helpful, I think it can be removed.

::: browser/installer/windows/nsis/installer.nsi:1350
(Diff revision 1)
> +      ${EndIf}
> +
> +      ; "R8" or "extension.0.name" should be text of "Field 3"
> +      IntOp $R7 $R9 + 3
> +
> +      ; Each row moves down by 13 (whatever unit that is)

The unit is called "dialog units", it's what Windows uses to lay out dialog boxes independently of the display resolution/scaling. Please change this comment.

::: browser/installer/windows/nsis/installer.nsi:1379
(Diff revision 1)
> +  WriteINIStr "$PLUGINSDIR\extensions.ini" "Field 1" Left   "0"
> +  WriteINIStr "$PLUGINSDIR\extensions.ini" "Field 1" Right  "-1"
> +  WriteINIStr "$PLUGINSDIR\extensions.ini" "Field 1" Top    "5"
> +  WriteINIStr "$PLUGINSDIR\extensions.ini" "Field 1" Bottom "25"
> +
> +  WriteINIStr "$PLUGINSDIR\extensions.ini" "Field 2" Type   "checkbox"

We should really have one checkbox per extension, instead of just the global one, if at all possible.

::: browser/locales/en-US/installer/custom.properties:35
(Diff revision 1)
>  OPTIONAL_COMPONENTS_DESC=The Maintenance Service will allow you to update $BrandShortName silently in the background.
>  MAINTENANCE_SERVICE_CHECKBOX_DESC=Install &Maintenance Service
> +EXTENSIONS_PAGE_TITLE=Set Up Optional Extensions
> +EXTENSIONS_PAGE_SUBTITLE=Optional Recommended Extensions
> +OPTIONAL_EXTENSIONS_DESC=The recommended extensions will enhance your online experience.
> +OPTIONAL_EXTENSIONS_CHECKBOX_DESC=Install &Extensions listed below

These strings needs to be reviewed by a UX person. I realize we're not actually using the English versions, at least not right away, but as soon as we land them they get harder to change, so they really need to be right the first time. In particular I think OPTIONAL_EXTENSIONS_DESC needs to say something pretty different in English.

::: toolkit/mozapps/installer/windows/nsis/common.nsh:5102
(Diff revision 1)
>              ${Else}
>                ; Installing the service always requires elevation.
>                ${ElevateUAC}
>              ${EndIf}
>  
> +            ReadINIStr $R8 $R7 "Install" "OptionalExtensions"

We'll need to remember to add this option to the wiki page that documents the INI file (https://wiki.mozilla.org/Installer:Command_Line_Arguments) once this patch lands.

Comment 6

[Hector Zhao [:hectorz]]

(In reply to Matt Howell [:mhowell] from comment #5)
> 
> I'm really happy to see this getting done, been looking forward to it for
> quite a while now. Need to work out a few issues with this patch first,
> though.

Hi Matt, thanks for the feedback.

> 
> ::: browser/installer/windows/nsis/installer.nsi:610
> (Diff revision 1)
> > +        ${ExitDo}
> > +      ${EndIf}
> > +
> > +      ${LogMsg} "Removing optional extension: $1"
> > +      ${If} ${FileExists} "$INSTDIR\distribution\extensions\$1.xpi"
> > +        Delete "$INSTDIR\distribution\extensions\$1.xpi"
> 
> I'd prefer if we could implement disabling the optional extensions by not
> copying the files into $INSTDIR in the first place. I realize that's more
> complicated to implement, but I think it's more robust; it's possible that
> copying the files over succeeds but then deleting them could fail (for
> instance, they might be locked by an antivirus that's trying to scan them).

I think anything in "$EXEDIR\core" would be copied to "$INSTDIR", but moving files out of the "core" directory would require change to the partner repack scripts. Do you think it's okay if these optional extensions are placed in "$EXEDIR\core\distribution\optional-extensions" and then moved into "$INSTDIR\distribution\extensions" if selected?

> 
> ::: browser/installer/windows/nsis/installer.nsi:1379
> (Diff revision 1)
> > +  WriteINIStr "$PLUGINSDIR\extensions.ini" "Field 1" Left   "0"
> > +  WriteINIStr "$PLUGINSDIR\extensions.ini" "Field 1" Right  "-1"
> > +  WriteINIStr "$PLUGINSDIR\extensions.ini" "Field 1" Top    "5"
> > +  WriteINIStr "$PLUGINSDIR\extensions.ini" "Field 1" Bottom "25"
> > +
> > +  WriteINIStr "$PLUGINSDIR\extensions.ini" "Field 2" Type   "checkbox"
> 
> We should really have one checkbox per extension, instead of just the global
> one, if at all possible.

I kinda thought it would be easier for the patch to be accepted into m-c if I can keep the custom logic as simple as possible, happy to introduce checkbox for each of the extensions.

> 
> ::: browser/locales/en-US/installer/custom.properties:35
> (Diff revision 1)
> >  OPTIONAL_COMPONENTS_DESC=The Maintenance Service will allow you to update $BrandShortName silently in the background.
> >  MAINTENANCE_SERVICE_CHECKBOX_DESC=Install &Maintenance Service
> > +EXTENSIONS_PAGE_TITLE=Set Up Optional Extensions
> > +EXTENSIONS_PAGE_SUBTITLE=Optional Recommended Extensions
> > +OPTIONAL_EXTENSIONS_DESC=The recommended extensions will enhance your online experience.
> > +OPTIONAL_EXTENSIONS_CHECKBOX_DESC=Install &Extensions listed below
> 
> These strings needs to be reviewed by a UX person. I realize we're not
> actually using the English versions, at least not right away, but as soon as
> we land them they get harder to change, so they really need to be right the
> first time. In particular I think OPTIONAL_EXTENSIONS_DESC needs to say
> something pretty different in English.

Any suggestions on how should I get the strings reviewed? thanks.

Comment 7

[Molly Howell (she/her) (no longer active)]

(In reply to Hector Zhao [:hectorz] from comment #6)
> I think anything in "$EXEDIR\core" would be copied to "$INSTDIR", but moving
> files out of the "core" directory would require change to the partner repack
> scripts. Do you think it's okay if these optional extensions are placed in
> "$EXEDIR\core\distribution\optional-extensions" and then moved into
> "$INSTDIR\distribution\extensions" if selected?

I would prefer skipping copying the files into $INSTDIR at all, but I do like this idea as a workaround, it covers the case I was worried about. You probably thought of this, but please be sure to delete the optional-extensions directory from $INSTDIR if the extensions are disabled.

> I kinda thought it would be easier for the patch to be accepted into m-c if
> I can keep the custom logic as simple as possible, happy to introduce
> checkbox for each of the extensions.

Yeah, I'm afraid I have to insist on a checkbox per extension. I have to be happy with the level of user experience provided by new (or upstreamed, in this case) installer features, and I'm not quite there with having just one checkbox.

> Any suggestions on how should I get the strings reviewed? thanks.

Hey Verdi, would you be able to take a look at the strings that this patch adds to the full installer and give some advice/feedback? To be clear, we don't have any plans to use this functionality outside of the China repack installer, this patch is just to synchronize some customizations they've made with the mainline installer, so the English strings won't actually show up anywhere, but we need to land something and I want them to be as good as we can get them the first time. Thanks.

Comment 8

[Romain Testard [:RT]]

(In reply to Matt Howell [:mhowell] from comment #4)
> (In reply to Axel Hecht [:Pike] from comment #2)
> 
> > So I'd love to know what the product vision is for regional variants of Firefox.
> 
> I'll CC Romain, who is the installer product manager. I don't feel that I
> understand your concern well enough to address it myself.
> 
Correct me if I'm wrong but my understanding is that China is the only country where we have a custom installer. 
If as suggested in Comment 3 we're good with limiting the changes in this patch to zh-CN only then it sounds like this should not cause extra work on the l10n front.

Comment 9

[Hector Zhao [:hectorz]]

Comment on attachment 8939516 [details]
Bug 1427712 - full installer page to install optional extensions with partner distribution.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209844/diff/1-2/

Comment 10

[Molly Howell (she/her) (no longer active)]

Comment on attachment 8939516 [details]
Bug 1427712 - full installer page to install optional extensions with partner distribution.

https://reviewboard.mozilla.org/r/209844/#review217952

I found one bug that needs to be fixed, and we're still waiting for UX feedback. Aside from those things, r=me. Thanks for doing this and for making those changes.

::: browser/installer/windows/nsis/installer.nsi:615
(Diff revisions 1 - 2)
> +          CopyFiles /SILENT "$INSTDIR\distribution\optional-extensions\$1.xpi" \
> +                            "$INSTDIR\distribution\extensions"

This doesn't work because the directory "$INSTDIR\distribution\extensions" doesn't exist yet; it thinks that "extensions" is the name of the file you want to copy to. You'll have to add a CreateDirectory instruction before this.

Comment 11

[Verdi [:verdi] Best to slack me]

(In reply to Matt Howell [:mhowell] from comment #7)
> 
> Hey Verdi, would you be able to take a look at the strings that this patch
> adds to the full installer and give some advice/feedback? 

Can I get a screenshot so I can see where and how these will show up? Thanks.

Comment 12

[Molly Howell (she/her) (no longer active)]

Attachment: Screenshot

Comment 13

[mheubusch]

Adding Meridel to review/revise strings. 

Also NI'ing Matt Howell to ask if there are any error messages that accompany this new installer flow that we need to review/write?

Comment 14

[Molly Howell (she/her) (no longer active)]

(In reply to mheubusch from comment #13)
> Also NI'ing Matt Howell to ask if there are any error messages that
> accompany this new installer flow that we need to review/write?

No, there aren't any errors that we would show the user for this; just a couple of new log messages.

Comment 15

[Meridel [:meridel]]

Here is revised copy with some questions in the margins:  https://docs.google.com/document/d/1LMSVBAv4WIKj8ZbuZnuhOnXQ-fxV7RG_nxbQI-cWhyU/edit?usp=sharing

Matt, I'll set up some time to discuss with you.

Comment 16

[Hector Zhao [:hectorz]]

(In reply to Matt Howell [:mhowell] from comment #12)
> Created attachment 8942231 [details]
> Screenshot

Thanks for figuring out the set of extensions in Fx China repack and providing the screenshot so quickly.

Comment 17

[Meridel [:meridel]]

Final copy has been approved by Hector and Matt: https://docs.google.com/document/d/1LMSVBAv4WIKj8ZbuZnuhOnXQ-fxV7RG_nxbQI-cWhyU/edit?usp=sharing

Comment 18

[Hector Zhao [:hectorz]]

Comment on attachment 8939516 [details]
Bug 1427712 - full installer page to install optional extensions with partner distribution.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209844/diff/2-3/

Comment 19

[Hector Zhao [:hectorz]]

I just pushed an updated patch to mozreview.

(In reply to Matt Howell [:mhowell] from comment #10)
> 
> ::: browser/installer/windows/nsis/installer.nsi:615
> (Diff revisions 1 - 2)
> > +          CopyFiles /SILENT "$INSTDIR\distribution\optional-extensions\$1.xpi" \
> > +                            "$INSTDIR\distribution\extensions"
> 
> This doesn't work because the directory "$INSTDIR\distribution\extensions"
> doesn't exist yet; it thinks that "extensions" is the name of the file you
> want to copy to. You'll have to add a CreateDirectory instruction before
> this.

This was fixed.

(In reply to Meridel from comment #17)
> Final copy has been approved by Hector and Matt:
> https://docs.google.com/document/d/1LMSVBAv4WIKj8ZbuZnuhOnXQ-fxV7RG_nxbQI-
> cWhyU/edit?usp=sharing

The strings were updated, with one exception: I've no idea how to make "Add-ons" bold.

Another change I've done: initial status of checkboxes are now controlled by "extension.{index}.checked" in distribution/setup.ini so that a custom install will default to the same set of extensions as a standard install. Matt, would you please have another look at it? Thanks.

Now I only need to figure out how to limit this to zh-CN (and maybe en-US too), something like oneoff_en-US.nsh in https://hg.mozilla.org/mozilla-central/rev/08ec6ef03ae3 ?

Comment 20

[Meridel [:meridel]]

If making text bold is not an option, can we instead add quotation marks around "Add-ons"?

Comment 21

[Molly Howell (she/her) (no longer active)]

(In reply to Hector Zhao [:hectorz] from comment #19)
> I just pushed an updated patch to mozreview.

Thanks!

> The strings were updated, with one exception: I've no idea how to make
> "Add-ons" bold.

I should have thought of that sooner, it's going to be more trouble than it's worth; let's go with just quoting it as Meridel suggested.

> Another change I've done: initial status of checkboxes are now controlled by
> "extension.{index}.checked" in distribution/setup.ini so that a custom
> install will default to the same set of extensions as a standard install.
> Matt, would you please have another look at it? Thanks.

That's a good idea; I've got a few small comments on that code which I'll post shortly.

> Now I only need to figure out how to limit this to zh-CN (and maybe en-US
> too), something like oneoff_en-US.nsh in
> https://hg.mozilla.org/mozilla-central/rev/08ec6ef03ae3 ?

I wouldn't worry about separating out the code, if that's what you mean; for just the strings, yes, that method should work fine.

Comment 22

[Molly Howell (she/her) (no longer active)]

Comment on attachment 8939516 [details]
Bug 1427712 - full installer page to install optional extensions with partner distribution.

https://reviewboard.mozilla.org/r/209844/#review220640

::: browser/installer/windows/nsis/installer.nsi:1458
(Diff revisions 2 - 3)
>        IntOp $R1 $R2 + 10
>  
> +      ReadINIStr $R0 "$EXEDIR\core\distribution\setup.ini" \
> +        "OptionalExtensions" "extension.$R9.checked"
> +      ${If} ${Errors}
> +        ClearErrors

As before, move the ClearErrors up before ReadINIStr.

::: browser/installer/windows/nsis/installer.nsi:1460
(Diff revisions 2 - 3)
> +      ReadINIStr $R0 "$EXEDIR\core\distribution\setup.ini" \
> +        "OptionalExtensions" "extension.$R9.checked"
> +      ${If} ${Errors}
> +        ClearErrors
> +        StrCpy $R0 ${BST_CHECKED}
> +      ${ElseIf} $R0 == ${BST_UNCHECKED}

Since this is reading out of our configuration file and not the UI definition, we don't need to use the Windows constant; I'd recommend just using 0 so that nobody thinks they have to look up what BST_UNCHECKED is.

::: browser/installer/windows/nsis/installer.nsi:1495
(Diff revisions 2 - 3)
> -  WriteINIStr "$PLUGINSDIR\extensions.ini" "Field 2" Left   "0"
> -  WriteINIStr "$PLUGINSDIR\extensions.ini" "Field 2" Right  "-1"
> -  WriteINIStr "$PLUGINSDIR\extensions.ini" "Field 2" Top    "27"
> -  WriteINIStr "$PLUGINSDIR\extensions.ini" "Field 2" Bottom "37"
> -  WriteINIStr "$PLUGINSDIR\extensions.ini" "Field 2" State  "1"
> -  WriteINIStr "$PLUGINSDIR\extensions.ini" "Field 2" Flags  "GROUP|NOTIFY"
> +  WriteINIStr "$PLUGINSDIR\extensions.ini" "Field $R9" Type   "label"
> +  WriteINIStr "$PLUGINSDIR\extensions.ini" "Field $R9" Text   "$(OPTIONAL_EXTENSIONS_DESC)"
> +  WriteINIStr "$PLUGINSDIR\extensions.ini" "Field $R9" Left   "0"
> +  WriteINIStr "$PLUGINSDIR\extensions.ini" "Field $R9" Right  "-1"
> +  WriteINIStr "$PLUGINSDIR\extensions.ini" "Field $R9" Top    "-15"
> +  WriteINIStr "$PLUGINSDIR\extensions.ini" "Field $R9" Bottom "-5"

This label needs a little more height; it's going to a second line of text for me and most of that line is cut off.

Comment 23

[Hector Zhao [:hectorz]]

Comment on attachment 8939516 [details]
Bug 1427712 - full installer page to install optional extensions with partner distribution.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209844/diff/3-4/

Comment 24

[Hector Zhao [:hectorz]]

(In reply to Meridel from comment #20)
> If making text bold is not an option, can we instead add quotation marks
> around "Add-ons"?

(In reply to Matt Howell [:mhowell] from comment #21)
> 
> I should have thought of that sooner, it's going to be more trouble than
> it's worth; let's go with just quoting it as Meridel suggested.

Updated as suggested.

(In reply to Matt Howell [:mhowell] from comment #21)
> 
> > Now I only need to figure out how to limit this to zh-CN (and maybe en-US
> > too), something like oneoff_en-US.nsh in
> > https://hg.mozilla.org/mozilla-central/rev/08ec6ef03ae3 ?
> 
> I wouldn't worry about separating out the code, if that's what you mean; for
> just the strings, yes, that method should work fine.

I've also updated the patch with this.

Comment 25

[Hector Zhao [:hectorz]]

Comment on attachment 8939516 [details]
Bug 1427712 - full installer page to install optional extensions with partner distribution.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209844/diff/4-5/

Comment 26

[Molly Howell (she/her) (no longer active)]

Comment on attachment 8939516 [details]
Bug 1427712 - full installer page to install optional extensions with partner distribution.

https://reviewboard.mozilla.org/r/209844/#review220956

I don't see any issues here, I think we're finally good to go!

Thanks again for doing this work, and for putting up with me when I kept adding more work to it.

Comment 27

[Hector Zhao [:hectorz]]

(In reply to Matt Howell [:mhowell] from comment #26)
> 
> I don't see any issues here, I think we're finally good to go!
> 
> Thanks again for doing this work, and for putting up with me when I kept
> adding more work to it.

Thank you for guiding me through this work. I'll push it to try for a Windows build once my hg access is restored before requesting checkin.

Comment 28

[Hector Zhao [:hectorz]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f981039f1571b1ca0db784a14f68b099a2224e4b

Nice to find out it's possible to create l10n installers with a try push.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Autoland can't push this until all pending issues in MozReview are marked as resolved.

Comment 30

[Hector Zhao [:hectorz]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #29)
> Autoland can't push this until all pending issues in MozReview are marked as
> resolved.

I'm marking it as fixed now, but leaving the ni? here to actually update the doc "once this patch lands".

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2e321934b38b
full installer page to install optional extensions with partner distribution. r=mhowell

Comment 32

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/2e321934b38b

Comment 33

[Hector Zhao [:hectorz]]

(In reply to Matt Howell [:mhowell] from comment #5)
> 
> ::: toolkit/mozapps/installer/windows/nsis/common.nsh:5102
> (Diff revision 1)
> >              ${Else}
> >                ; Installing the service always requires elevation.
> >                ${ElevateUAC}
> >              ${EndIf}
> >  
> > +            ReadINIStr $R8 $R7 "Install" "OptionalExtensions"
> 
> We'll need to remember to add this option to the wiki page that documents
> the INI file (https://wiki.mozilla.org/Installer:Command_Line_Arguments)
> once this patch lands.

(In reply to Hector Zhao [:hectorz] from comment #30)
> 
> I'm marking it as fixed now, but leaving the ni? here to actually update the
> doc "once this patch lands".

https://wiki.mozilla.org/index.php?title=Installer%3ACommand_Line_Arguments&type=revision&diff=1187598&oldid=1171107

Comment 34

[Mike Kaply [:mkaply]]

Is this worth requesting uplift? I think the sooner we get this done, the better.

Comment 35

[Mike Kaply [:mkaply]]

Hector, can you request uplift?

Comment 36

[Hector Zhao [:hectorz]]

(In reply to Mike Kaply [:mkaply] from comment #35)
> Hector, can you request uplift?

Hi Mike, I was out for a few days. Yesterday I created test builds based on recent Nightly with partner-repacks.py and I'll request uplift once our QA finishs testing it.

Comment 37

[Hector Zhao [:hectorz]]

Comment on attachment 8939516 [details]
Bug 1427712 - full installer page to install optional extensions with partner distribution.

Approval Request Comment
[Feature/Bug causing the regression]: Not a regression, part of the work in bug 1249804
[User impact if declined]: Not for user. This should simplify the release process of Fx China repack a lot and :mkaply suggested this should be uplifted in comment 34
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes, QA in Beijing office verified the Nightly based test builds
[Needs manual test from QE? If yes, steps to reproduce]: Not from global QE team.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Manually verified installer only changes, also limited to en-US & zh-CN
[String changes made/needed]: Just en-US & zh-CN strings in the patch, no further l10n necessary.

Comment 38

[Mike Kaply [:mkaply]]

relman: Please contact me if you need more information on this.

Comment 39

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8939516 [details]
Bug 1427712 - full installer page to install optional extensions with partner distribution.

Helping out with partner repack issues sounds good.
This should make it into the beta 10 build for Thursday.

Comment 40

[Francesco Lodolo [:flod]]

checkin-needed is only for landing on trunk, the approval-mozilla-beta+ flag will flag sheriffs for the landing by itself.

Comment 41

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ab285921713c