Closed Bug 853628 Opened 11 years ago Closed 11 years ago

Bookmarks/History favicons don't display after bug 716140

Categories

(Core :: Widget: Cocoa, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: joe, Assigned: joe)

References

Details

Attachments

(1 file)

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)
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?
(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.
(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+
(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.
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
Closed: 11 years ago
Resolution: --- → FIXED
Patch didn't work for me Joe. I have to disable azure to get my favicons to always display.
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.