853628 - Bookmarks/History favicons don't display after bug 716140

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Joe Drew (not getting mail)]

Attachment: 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.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

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?

Comment 2

[Joe Drew (not getting mail)]

(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

[IU]

(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 4

[Steven Michaud [:smichaud] (Retired)]

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 :-)

Comment 5

[Joe Drew (not getting mail)]

(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.

Comment 6

[Joe Drew (not getting mail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/69f965c0fd46

Comment 7

[Joe Drew (not getting mail)]

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

Comment 8

[Phil Ringnalda (:philor)]

Not you, it happened again above your backout.

Comment 9

[Phil Ringnalda (:philor)]

Relanded in https://hg.mozilla.org/integration/mozilla-inbound/rev/4e743f7bf114

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4e743f7bf114

Comment 11

[Gary [:streetwolf52]]

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

Comment 12

[Gary [:streetwolf52]]

Just noticed it says platform Mac and I'm on Windows 8. Is there a patch for Windows?