make it possible to use alternative icon for firefox shortcuts

VERIFIED FIXED in Firefox 55

Status

()

VERIFIED FIXED
a year ago
a year ago

People

(Reporter: bhearsum, Assigned: mhowell)

Tracking

unspecified
Firefox 55
Points:
---

Firefox Tracking Flags

(firefox53 wontfix, firefox54 wontfix, firefox55 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a year ago
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.
(Reporter)

Updated

a year ago
Blocks: 1353825
Comment hidden (mozreview-request)
(Assignee)

Comment 2

a year ago
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

a year ago
mozreview-review
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 hidden (mozreview-request)

Comment 5

a year ago
mozreview-review
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 hidden (mozreview-request)

Comment 7

a year ago
mozreview-review
Comment on attachment 8858953 [details]
Bug 1357161 - Replace shortcut icons on application update.

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

Thanks!
Attachment #8858953 - Flags: review?(agashlin) → review+

Comment 8

a year ago
Pushed by mhowell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cb5c42c65138
Replace shortcut icons on application update. r=agashlin
(Assignee)

Comment 9

a year ago
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
Attachment #8858953 - Flags: approval-mozilla-beta?
Attachment #8858953 - Flags: approval-mozilla-aurora?
Comment on attachment 8858953 [details]
Bug 1357161 - Replace shortcut icons on application update.

54 went to Beta today and Aurora's gone.
Attachment #8858953 - Flags: approval-mozilla-aurora?
Assignee: nobody → mhowell
status-firefox53: --- → wontfix
status-firefox54: --- → affected
status-firefox55: --- → affected

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cb5c42c65138
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Hi Florin,
Can you have someone in your team check if the patch works as expected?
Flags: qe-verify+
Flags: needinfo?(florin.mezei)
Moving this to Andrei so they can also cover this when testing the migration of Developer Edition to Beta (likely next week).
Flags: needinfo?(florin.mezei) → needinfo?(andrei.vaida)
(Assignee)

Comment 14

a year ago
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.
Attachment #8858953 - Flags: approval-mozilla-beta?
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: needinfo?(andrei.vaida)
(Reporter)

Updated

a year ago
No longer blocks: 1353825
Per comment #14, mark 54 won't fix.
status-firefox54: affected → wontfix
status-firefox55: fixed → verified
You need to log in before you can comment on or make changes to this bug.