Closed Bug 875609 Opened 11 years ago Closed 11 years ago

Refactor jump list code to decode images on the main thread

Categories

(Core :: Widget: Win32, defect)

Product:
Core
Component:
Widget: Win32
Platform:
x86_64
Windows 8
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla24
Tracking Flags:
Tracking Status
firefox21 --- wontfix
firefox22 --- verified
firefox23 --- verified
firefox24 --- verified

People

(Reporter: bbondy, Assigned: bbondy)

References

(Depends on 2 open bugs)

Details

Attachments

(1 file)

Patch v1.
(deleted), patch
jimm
: review+
akeybl
: approval-mozilla-aurora+
akeybl
: approval-mozilla-beta+
Details | Diff | Splinter Review 
As of bug 716140 we can no longer decode images off the main thread. We need to refactor the jumplist code.
Attached patch Patch v1. (deleted) — — Splinter Review 
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.
Assignee: nobody → netzen
Status: NEW → ASSIGNED
Attachment #754539 - Flags: review?(jmathies)
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.
Attachment #754539 - Flags: review?(jmathies) → review+
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
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.
Depends on: 876690
https://hg.mozilla.org/mozilla-central/rev/f6d827971b26
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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
Attachment #754539 - Flags: approval-mozilla-beta?
Attachment #754539 - Flags: approval-mozilla-aurora?
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?
Attachment #754539 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I think they should both go to Beta, I think a lot of Windows users use this feature a lot.
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.
(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?
(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.
(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.
Sounds like it'll be fine.
(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 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.
Attachment #754539 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
(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.
(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
(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?
(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.
(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.
The about:config pref browser.taskbar.lists.refreshInSeconds controls how frequently jump lists generate.
(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.
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
Status: RESOLVED → VERIFIED
status-firefox22: fixed → verified
status-firefox23: fixed → verified
status-firefox24: fixed → verified
Depends on: 993302
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: