875609 - Refactor jump list code to decode images on the main thread

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect)

Description

[Brian R. Bondy [:bbondy]]

As of bug 716140 we can no longer decode images off the main thread. We need to refactor the jumplist code.

Comment 1

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1.

The main difference is to decode the favicon on the main thread and then pass its data for encoding and writing to disk to the thread.

Comment 2

[Jim Mathies [:jimm]]

Comment on attachment 754539 [details] [diff] [review]
Patch v1.

Review of attachment 754539 [details] [diff] [review]:
-----------------------------------------------------------------

Seems crazy that we are moving decoding back onto the main thread! But ok.

Comment 3

[Brian R. Bondy [:bbondy]]

Agreed! Joe mentioned to me just now that encoding should happen on the main thread too but I don't think it's a problem at this time, so I'm going to leave it. :'(
https://www.youtube.com/watch?v=z5tZMDBXTRQ

Comment 4

[Brian R. Bondy [:bbondy]]

By the way Joe mentioned that he'd mentor bug 876687 (yay!), so we can eventually get this back off the main thread when that gets implemented.  I'll file a new bug to do that work dependent on bug 876687.

Comment 5

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6d827971b26

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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

Comment 7

[Brian R. Bondy [:bbondy]]

Comment on attachment 754539 [details] [diff] [review]
Patch v1.

This is a sister bug to bug 870529, it was needed to re-enable jumplists in bug 870529 because of an unrelated thing added in bug 716140.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 716140 but is blocking us from landing bug 870529 
User impact if declined: Windows 7+ jump lists on the taskbar will not work if not previously generated or if icon re-created.
Testing completed (on m-c, etc.): I will verify this on m-c before uplifting.
Risk to taking this patch (and alternatives if risky): Medium-Low, but extra QA testing around this feature can improve the risk.
String or IDL/UUID changes made by this patch: None

Comment 8

[Alex Keybl [:akeybl]]

Comment on attachment 754539 [details] [diff] [review]
Patch v1.

We're reconsidering bug 870529 for Beta given the risk here. Approving for Aurora in the meantime.

Since bug 870529 isn't a tracked bug and hasn't caused much user pain, we're strongly leaning towards minusing both for FF22. Are you OK with that bbondy?

Comment 9

[Brian R. Bondy [:bbondy]]

I think they should both go to Beta, I think a lot of Windows users use this feature a lot.

Comment 10

[Brian R. Bondy [:bbondy]]

I'll just add a bit more about why I think it should go on beta:
- All new installs, and possibly some larger installs currently have no menu come up from their taskbar icon on Firefox 21 (jump list).
- This means that things like private browsing, and new window are only available in the Firefox menu.
- Frequent sites that are displayed in the jump list will no longer be present.
- This may cause people to try to manually unpin their taskbar icon and repin, leading to a different taskbar icon appusermodelid, causing various potential problems.

Comment 11

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ee2b51de8b68

Comment 12

[xunxun]

(In reply to Brian R. Bondy [:bbondy] from comment #9)
> I think they should both go to Beta, I think a lot of Windows users use this
> feature a lot.

I try to port bug 875609 and bug 870529 patch to 22 beta 3, but Windows jumplist still not shows. Maybe another more patch should be landed in 22?

Comment 13

[Brian R. Bondy [:bbondy]]

(In reply to xunxun from comment #12)
> (In reply to Brian R. Bondy [:bbondy] from comment #9)
> > I think they should both go to Beta, I think a lot of Windows users use this
> > feature a lot.
> 
> I try to port bug 875609 and bug 870529 patch to 22 beta 3, but Windows
> jumplist still not shows. Maybe another more patch should be landed in 22?

I don't think any other patches are needed.

I assume you mean you applied the patches locally to a cloned mozilla-beta repo, built and then tried? I think you need to build an installer and use the installer because I think jump lists will only work when you have a shortcut lnk with the correct appusermodelid property on it.

Comment 14

[xunxun]

(In reply to Brian R. Bondy [:bbondy] from comment #13)
> I don't think any other patches are needed.
> 
> I assume you mean you applied the patches locally to a cloned mozilla-beta
> repo, built and then tried? I think you need to build an installer and use
> the installer because I think jump lists will only work when you have a
> shortcut lnk with the correct appusermodelid property on it.

Well, in fact, I have built the beta3, and patch it with bug 875609 and bug 870529 patch again, and rebuilt it again, in the end, I built the installer, but uncompress the installer, and run core/firefox.exe. But jumplist doesn't show. (I have used a clean profile)

At the same time, I download latest nightly zip, and uncompress it, and run firefox.exe, whose jumplist shows. (I have used a clean profile)

I don't know why.

And my OS is Win8 x64.

Comment 15

[Brian R. Bondy [:bbondy]]

Sounds like it'll be fine.

Comment 16

[xunxun]

(In reply to Brian R. Bondy [:bbondy] from comment #15)
> Sounds like it'll be fine.

Maybe someone can build a 22 beta try build in Mozilla try server to check it.

Comment 17

[Alex Keybl [:akeybl]]

Comment on attachment 754539 [details] [diff] [review]
Patch v1.

Support weighed in - we've gotten complaints about this issue, so approving for FF22. We'll back out if we find any critical regressions in these last three weeks.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/86f7fda1f568

Comment 19

[Mihaela Velimiroviciu (:mihaelav)]

Mozilla/5.0 (Windows NT 6.2; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0
Build Id: 20130605070403

Verified the fix on latest Firefox 22 beta 4 build: jump list appears correctly showing the items along with their favicons - tested with old&new profile, default&non-default Firefox and Firefox installed over an older (broken) version.

Comment 20

[xunxun]

(In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #19)
> Mozilla/5.0 (Windows NT 6.2; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0
> Build Id: 20130605070403
> 
> Verified the fix on latest Firefox 22 beta 4 build: jump list appears
> correctly showing the items along with their favicons - tested with old&new
> profile, default&non-default Firefox and Firefox installed over an older
> (broken) version.

I check official 22b4 on my Win8 x64 with old or new profile, but jumplist still not show, and at the same time, nightly's jumplist can show.

So as I said in Comment 12, maybe some another patches should be also backported for Win8.

Comment 21

[Mihaela Velimiroviciu (:mihaelav)]

(In reply to xunxun from comment #20)
> (In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #19)
> > Mozilla/5.0 (Windows NT 6.2; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0
> > Build Id: 20130605070403
> > 
> > Verified the fix on latest Firefox 22 beta 4 build: jump list appears
> > correctly showing the items along with their favicons - tested with old&new
> > profile, default&non-default Firefox and Firefox installed over an older
> > (broken) version.
> 
> I check official 22b4 on my Win8 x64 with old or new profile, but jumplist
> still not show, and at the same time, nightly's jumplist can show.

You have to wait a few minutes until the jump list gets populated

Comment 22

[xunxun]

(In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #21)
> (In reply to xunxun from comment #20)
> > (In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #19)
> > > Mozilla/5.0 (Windows NT 6.2; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0
> > > Build Id: 20130605070403
> > > 
> > > Verified the fix on latest Firefox 22 beta 4 build: jump list appears
> > > correctly showing the items along with their favicons - tested with old&new
> > > profile, default&non-default Firefox and Firefox installed over an older
> > > (broken) version.
> > 
> > I check official 22b4 on my Win8 x64 with old or new profile, but jumplist
> > still not show, and at the same time, nightly's jumplist can show.
> 
> You have to wait a few minutes until the jump list gets populated

But why nightly edition can show jump list immediately?

Comment 23

[Mihaela Velimiroviciu (:mihaelav)]

(In reply to xunxun from comment #22)
> (In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #21)
> > (In reply to xunxun from comment #20)
> > > (In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #19)
> > > > Mozilla/5.0 (Windows NT 6.2; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0
> > > > Build Id: 20130605070403
> > > > 
> > > > Verified the fix on latest Firefox 22 beta 4 build: jump list appears
> > > > correctly showing the items along with their favicons - tested with old&new
> > > > profile, default&non-default Firefox and Firefox installed over an older
> > > > (broken) version.
> > > 
> > > I check official 22b4 on my Win8 x64 with old or new profile, but jumplist
> > > still not show, and at the same time, nightly's jumplist can show.
> > 
> > You have to wait a few minutes until the jump list gets populated
> 
> But why nightly edition can show jump list immediately?

According to bug 609443, bug 598229, there is a delay until the jump list is created.
Maybe your nightly jump list was populated sometime before.

Comment 24

[xunxun]

(In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #23)
> (In reply to xunxun from comment #22)
> > (In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #21)
> > > (In reply to xunxun from comment #20)
> > > > (In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #19)
> > > > > Mozilla/5.0 (Windows NT 6.2; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0
> > > > > Build Id: 20130605070403
> > > > > 
> > > > > Verified the fix on latest Firefox 22 beta 4 build: jump list appears
> > > > > correctly showing the items along with their favicons - tested with old&new
> > > > > profile, default&non-default Firefox and Firefox installed over an older
> > > > > (broken) version.
> > > > 
> > > > I check official 22b4 on my Win8 x64 with old or new profile, but jumplist
> > > > still not show, and at the same time, nightly's jumplist can show.
> > > 
> > > You have to wait a few minutes until the jump list gets populated
> > 
> > But why nightly edition can show jump list immediately?
> 
> According to bug 609443, bug 598229, there is a delay until the jump list is
> created.
> Maybe your nightly jump list was populated sometime before.

Thanks for the explanation. 
My nightly jump list was populated yesterday.

After about 3 minutes, Fx22 b4 jump list shows.

Comment 25

[Brian R. Bondy [:bbondy]]

The about:config pref browser.taskbar.lists.refreshInSeconds controls how frequently jump lists generate.

Comment 26

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #25)
> The about:config pref browser.taskbar.lists.refreshInSeconds controls how
> frequently jump lists generate.

Note, if you set that too early it will kill your startup perf, so be careful about the value you use.

Comment 27

[Petruta Horea [:phorea], Desktop Test Engineering]

Verified as fixed on Fx 22, 23, 24 and latest nightly:
User Agent: Mozilla/5.0 (Windows NT 6.2; rv:22.0) Gecko/20100101 Firefox/22.0
Build Id: 20130618035212

User Agent 	Mozilla/5.0 (Windows NT 6.2; rv:23.0) Gecko/20100101 Firefox/23.0
Build Id: 20130722172257

User Agent 	Mozilla/5.0 (Windows NT 6.2; rv:24.0) Gecko/20130725 Firefox/24.0
Build Id: 20130725004004

User Agent Mozilla/5.0 (Windows NT 6.2; rv:25.0) Gecko/20130725 Firefox/25.0
Build Id: Build Id: 20130725171558