516195 - Port |Bug 435296 - imagelib should support decode-on-draw| to SeaMonkey

Status: VERIFIED FIXED

(SeaMonkey :: OS Integration, defect)

Description

[Serge Gautherie (:sgautherie)]

{
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1252798402.1252801034.26422.gz
WINNT 5.2 comm-central-trunk build on 2009/09/12 16:33:22

e:/builds/slave/comm-central-trunk-win32/build/suite/shell/src/nsWindowsShellService.cpp(607) : error C2039: 'CopyCurrentFrame' : is not a member of 'imgIContainer'
        e:\builds\slave\comm-central-trunk-win32\build\objdir\mozilla\dist\include\imgIContainer.h(42) : see declaration of 'imgIContainer'
}

SM needs the same "1 line" update as FF got.

Comment 1

[Bobby Holley (:bholley)]

should be trivial to fix - CopyCurrentFrame is now CopyFrame with a FRAME_CURRENT parameter.

Comment 2

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Just copy Firefox change

(Untested but trivial...)

Comment 3

[Serge Gautherie (:sgautherie)]

Bobby, is bug 435296 going to land on 1.9.2?
Or do c-c needs some kind of "#ifdef 1.9.3+"?

Comment 4

[Bobby Holley (:bholley)]

there is no chance that it will land on 1.9.2.

Comment 5

[[e-mail]]

OK, so I forgot to search Bugzilla first and attached a patch to 435296...

I went with FRAME_CURRENT because that seemed to correspond better with the previous code; anyone know why Firefox has FRAME_FIRST there?

Comment 6

[Frank Wein [:mcsmurf]]

Comment on attachment 400323 [details] [diff] [review]
(Av1) Just copy Firefox change

Actually, yeah, use FRAME_CURRENT instead. If someone knows more about this, please comment.

Comment 7

[Serge Gautherie (:sgautherie)]

Attachment: (Av1a) Support both m-1.9.2 and m-c [Checkin: Comment 15]

Av1, with previous suggestion(s).

Comment 8

[Bobby Holley (:bholley)]

I made firefox use FRAME_FIRST. The function seems only to be called by nsWindowsShellService::SetDesktopBackground, and I figured that it was better to have more predictable behavior in the case of animated images, rather than just getting whatever frame was displaying at the moment.

We should probably use the same behavior for both firefox and seamonkey. I'm not particularly attached to either one, but if you do go with FRAME_CURRENT here file a bug on firefox to change it there too (maybe also ask some UX folks what they think).

Comment 9

[[e-mail]]

Quoting from mozilla.dev.usability:

> I think Copy Image and Set As Wallpaper should be consistent. I also 
> think that the Principle of Least Surprise says that the image used 
> should be the image the user is currently seeing.
> 
> If they select this from a context menu, and the animation is continuing 
> under the menu, then of course it's a bit more complicated to know quite 
> what the user expects to happen :-)
> 
> Gerv

Comment 10

[Bobby Holley (:bholley)]

(In reply to comment #9)
> Quoting from mozilla.dev.usability:
 
> > If they select this from a context menu, and the animation is continuing 
> > under the menu, then of course it's a bit more complicated to know quite 
> > what the user expects to happen :-)
> > 
> > Gerv

This was the situation I was thinking about. Again though, I'm fine with either one.

Comment 11

[Robert Kaiser]

Comment on attachment 400359 [details] [diff] [review]
(Av1a) Support both m-1.9.2 and m-c
[Checkin: Comment 15]

Forwarding review to Mark, I'm not convinced if we'll ever support 1.9.2 with comm-central at all, and I'd rather not clutter our build system with something we never will need. Mark may have a better feeling if we need it at all.

Comment 12

[Serge Gautherie (:sgautherie)]

Per irc discussion, could either add the var or error out, on 1.9.2.

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 400359 [details] [diff] [review]
(Av1a) Support both m-1.9.2 and m-c
[Checkin: Comment 15]

Thunderbird does want the 1.9.2 var at the moment. As long as ifdefs don't get too complicated I think it is reasonable to support 1.9.2 for the time being. Once we branch we'll be able to drop the 1.9.1 stuff anyway.

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 400359 [details] [diff] [review]
(Av1a) Support both m-1.9.2 and m-c
[Checkin: Comment 15]

Giving aSM2.0=Standard8 as this doesn't affect 2.0.

Comment 15

[Serge Gautherie (:sgautherie)]

Comment on attachment 400359 [details] [diff] [review]
(Av1a) Support both m-1.9.2 and m-c
[Checkin: Comment 15]


http://hg.mozilla.org/comm-central/rev/e5fceda01f27

Comment 16

[Serge Gautherie (:sgautherie)]

(In reply to comment #8)
> if you do go with FRAME_CURRENT
> here file a bug on firefox to change it there too (maybe also ask some UX folks
> what they think).

I leave this to you, as you know better than me what the Firefox situation is.

Comment 17

[Serge Gautherie (:sgautherie)]

(In reply to comment #15)
> (From update of attachment 400359 [details] [diff] [review])
> 
> http://hg.mozilla.org/comm-central/rev/e5fceda01f27

Frank, sorry I missed to cite you:
all these "bugzilla@" address are just confusing to work with :-/

Comment 18

[Serge Gautherie (:sgautherie)]

V.Fixed, per tinderbox.