Bookmarks/History favicons don't display after bug 716140

RESOLVED FIXED in mozilla22

Status

()

Core
Widget: Cocoa
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Joe Drew (not getting mail), Assigned: Joe Drew (not getting mail))

Tracking

Trunk
mozilla22
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Created attachment 727878 [details] [diff] [review]
don't wait for load to finish, wait for decode

nsMenuItemIconX makes some assumptions about the order events are delivered in that aren't true any more. This patch fixes them.
Attachment #727878 - Flags: review?(smichaud)
(Assignee)

Updated

5 years ago
Blocks: 716140
Comment on attachment 727878 [details] [diff] [review]
don't wait for load to finish, wait for decode

What's the specific problem that this patch fixes?

Is it that LOAD_COMPLETE now happens "too early", so that mIconRequest can get cancelled "too early"?

Would it *always* have made better sense to wait for DECODE_COMPLETE?
(Assignee)

Comment 2

5 years ago
(In reply to Steven Michaud from comment #1)
> What's the specific problem that this patch fixes?

In tonight's respun nightly, the History menu always shows blank icons the first time you open it; the second time, they draw properly.
 
> Is it that LOAD_COMPLETE now happens "too early", so that mIconRequest can
> get cancelled "too early"?

Right.

> Would it *always* have made better sense to wait for DECODE_COMPLETE?

Yes.

Comment 3

5 years ago
(In reply to Joe Drew (:JOEDREW! \o/) from comment #0)
> [->Insert Function<-] makes some assumptions about the order events are delivered
> in that aren't true any more. This patch fixes them.

Could something similar to this (i.e. waiting for load but not decode to complete) be at the heart of bug 853337?  I haven't yet reproduced but just wondering.
Comment on attachment 727878 [details] [diff] [review]
don't wait for load to finish, wait for decode

>> Would it *always* have made better sense to wait for DECODE_COMPLETE?
>
> Yes.

That's good enough for me :-)
Attachment #727878 - Flags: review?(smichaud) → review+
(Assignee)

Comment 5

5 years ago
(In reply to IU from comment #3)
> (In reply to Joe Drew (:JOEDREW! \o/) from comment #0)
> > [->Insert Function<-] makes some assumptions about the order events are delivered
> > in that aren't true any more. This patch fixes them.
> 
> Could something similar to this (i.e. waiting for load but not decode to
> complete) be at the heart of bug 853337?  I haven't yet reproduced but just
> wondering.

That's what I suspect is going on for the favicons, but right now I'm just guessing.
(Assignee)

Comment 7

5 years ago
Backed out since it was part of a push that mysteriously crashed in crashtests.

Pushed it on its own: https://tbpl.mozilla.org/?tree=Try&rev=4cd58fb0b7f0
Not you, it happened again above your backout.
https://hg.mozilla.org/mozilla-central/rev/4e743f7bf114
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 11

5 years ago
Patch didn't work for me Joe. I have to disable azure to get my favicons to always display.

Comment 12

5 years ago
Just noticed it says platform Mac and I'm on Windows 8. Is there a patch for Windows?
You need to log in before you can comment on or make changes to this bug.