1357161 - make it possible to use alternative icon for firefox shortcuts

Status: VERIFIED FIXED

(Firefox :: Installer, defect)

Description

[[e-mail] (:bhearsum)]

As part of the Dev Edition transition to Beta, we need to be able to make partner repacks be able to use custom icons. We'll be taking care of including the necessary icon file as part of automation, but we need to make sure shortcuts also have the correct logo.

I think this is Windows only, since Linux and Mac have no shortcuts.

Comment 1

[Molly Howell (she/her) (mostly inactive)]

Attachment: Bug 1357161 - Replace shortcut icons on application update.

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

Comment 2

[Molly Howell (she/her) (mostly inactive)]

The above patch may or may not need to land depending on the decision that comes out of bug 1353825 comment 25; it will not be needed if the decision is to use full builds. Otherwise, it will need beta uplift as soon as possible.

The patch also assumes the icons we should use are in a file called 'firefox.ico' but that can be changed easily.

Comment 3

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Comment on attachment 8858953 [details]
Bug 1357161 - Replace shortcut icons on application update.

https://reviewboard.mozilla.org/r/130954/#review133894

::: browser/installer/windows/nsis/shared.nsh:326
(Diff revision 1)
>      ClearErrors
>      ; The entries in the shortcut log are numbered, but we never actually
>      ; create more than one shortcut (or log entry) in each location.
>      ReadINIStr $R8 "$R9" "STARTMENU" "Shortcut0"
>      ${IfNot} ${Errors}
>        ${If} ${FileExists} "$SMPROGRAMS\$R8"

I'm a little concerned about the shortcuts now being recreated on every update for all users whether anything has changed or not. I assume this change is due to being uncertain about the icon, but ShellLink has a GetShortCutIconLocation and GetShortCutIconIndex that could be used.

I can't foresee exactly what this recreation could break, if it seems not worth the extra checks (or if they're not appropriate) go ahead and discount this issue.

::: browser/installer/windows/nsis/shared.nsh:376
(Diff revision 1)
>        ${If} ${FileExists} "$QUICKLAUNCH\User Pinned\TaskBar\$R8"
> -      ${AndIf} $R8 != "${BrandFullName}.lnk"
>          ShellLink::GetShortCutTarget "$QUICKLAUNCH\User Pinned\TaskBar\$R8"
>          Pop $R7
>          ${GetLongPath} "$R7" $R7
>          ${If} "$INSTDIR\${FileMainEXE}" == "$R7"

Unlike the above this doesn't handle firefox.ico disappearing. This could be fixed later when an update removing, rather than possibly adding, the icon is shipped, but for consistency it seems like it should be here.

If we only want to handle the icon being added, maybe the other shortcut updates should also only be triggered when the icon appears.

Comment 4

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8858953 [details]
Bug 1357161 - Replace shortcut icons on application update.

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

Comment 5

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Comment on attachment 8858953 [details]
Bug 1357161 - Replace shortcut icons on application update.

https://reviewboard.mozilla.org/r/130954/#review133974

::: browser/installer/windows/nsis/shared.nsh:345
(Diff revision 2)
> +          ${Else}
> +            StrCpy $R5 "0"
> +          ${EndIf}
> +
> +          ${If} $R5 == "1"
> +          ${OrIf} $R8 != "$SMPROGRAMS\${BrandFullName}.lnk"

Here and at 382 $R8 has only the file name, not the full path, so should be compared to ${BrandFullName}.lnk

Comment 6

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8858953 [details]
Bug 1357161 - Replace shortcut icons on application update.

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

Comment 7

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Comment on attachment 8858953 [details]
Bug 1357161 - Replace shortcut icons on application update.

https://reviewboard.mozilla.org/r/130954/#review133982

Thanks!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cb5c42c65138
Replace shortcut icons on application update. r=agashlin

Comment 9

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8858953 [details]
Bug 1357161 - Replace shortcut icons on application update.

Approval Request Comment
[Feature/Bug causing the regression]:
N/A

[User impact if declined]:
Aurora users being updated to beta as part of the Dawn project will have their desktop, start menu, and taskbar shortcuts change to the beta/release icons, instead of keeping the dev edition one.
This patch needs to be included in at least the first beta build pushed out on the Dawn project.

[Is this code covered by automated tests?]:
No.

[Has the fix been verified in Nightly?]:
Not yet.

[Needs manual test from QE? If yes, steps to reproduce]: 
This should be included in whatever QA work is being carried out for Dawn.

[List of other uplifts needed for the feature/fix]:
Bug 1354321

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
I've manually verified that the patch doesn't break shortcuts on Windows 10. I don't expect different behaviors on other supported Windows versions.

[String changes made/needed]:
None

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8858953 [details]
Bug 1357161 - Replace shortcut icons on application update.

54 went to Beta today and Aurora's gone.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cb5c42c65138

Comment 12

[Gerry Chang [:gchang]]

Hi Florin,
Can you have someone in your team check if the patch works as expected?

Comment 13

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Moving this to Andrei so they can also cover this when testing the migration of Developer Edition to Beta (likely next week).

Comment 14

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8858953 [details]
Bug 1357161 - Replace shortcut icons on application update.

My understanding is that the plan is now to do separate builds off of beta using the dev edition branding. In that case, there's no uplift needed here.

Comment 15

[Andrei Vaida [:avaida]]

This bug fix was part of our test coverage during the 54.0b6-build1 repack sign off. There were no issues found for the application icon.

Testing was done across platforms: Windows 7 x64, Windows 10 x64, macOS 10.12.4 and Ubuntu 16.04 x64.

Comment 16

[Gerry Chang [:gchang]]

Per comment #14, mark 54 won't fix.