Last Comment Bug 516195 - Port |Bug 435296 - imagelib should support decode-on-draw| to SeaMonkey
: Port |Bug 435296 - imagelib should support decode-on-draw| to SeaMonkey
Status: VERIFIED FIXED
: fixed-seamonkey2.0
Product: SeaMonkey
Classification: Client Software
Component: OS Integration (show other bugs)
: Trunk
: x86 Windows Server 2003
: -- blocker (vote)
: seamonkey2.1a1
Assigned To: Serge Gautherie (:sgautherie)
:
:
Mentors:
http://mxr.mozilla.org/comm-central/s...
Depends on: 435296
Blocks: CcMcBuildIssues 516858
  Show dependency treegraph
 
Reported: 2009-09-12 18:13 PDT by Serge Gautherie (:sgautherie)
Modified: 2009-09-17 18:09 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
(Av1) Just copy Firefox change (1.05 KB, patch)
2009-09-12 19:59 PDT, Serge Gautherie (:sgautherie)
bugzilla: review+
Details | Diff | Splinter Review
(Av1a) Support both m-1.9.2 and m-c [Checkin: Comment 15] (3.15 KB, patch)
2009-09-13 07:52 PDT, Serge Gautherie (:sgautherie)
standard8: review+
standard8: approval‑seamonkey2.0+
Details | Diff | Splinter Review

Description Serge Gautherie (:sgautherie) 2009-09-12 18:13:36 PDT
{
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) (busy with Stylo) 2009-09-12 18:19:08 PDT
should be trivial to fix - CopyCurrentFrame is now CopyFrame with a FRAME_CURRENT parameter.
Comment 2 Serge Gautherie (:sgautherie) 2009-09-12 19:59:27 PDT
Created attachment 400323 [details] [diff] [review]
(Av1) Just copy Firefox change

(Untested but trivial...)
Comment 3 Serge Gautherie (:sgautherie) 2009-09-12 20:07:01 PDT
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) (busy with Stylo) 2009-09-12 22:39:19 PDT
there is no chance that it will land on 1.9.2.
Comment 5 neil@parkwaycc.co.uk 2009-09-13 03:20:36 PDT
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] 2009-09-13 04:37:58 PDT
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) 2009-09-13 07:52:03 PDT
Created attachment 400359 [details] [diff] [review]
(Av1a) Support both m-1.9.2 and m-c
[Checkin: Comment 15]

Av1, with previous suggestion(s).
Comment 8 Bobby Holley (:bholley) (busy with Stylo) 2009-09-13 18:12:15 PDT
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 neil@parkwaycc.co.uk 2009-09-14 05:45:47 PDT
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) (busy with Stylo) 2009-09-14 11:37:57 PDT
(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 2009-09-16 07:43:36 PDT
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) 2009-09-16 12:40:12 PDT
Per irc discussion, could either add the var or error out, on 1.9.2.
Comment 13 Mark Banner (:standard8) 2009-09-17 04:29:20 PDT
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) 2009-09-17 04:30:11 PDT
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) 2009-09-17 07:59:59 PDT
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) 2009-09-17 08:09:46 PDT
(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) 2009-09-17 08:41:28 PDT
(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) 2009-09-17 08:52:22 PDT
V.Fixed, per tinderbox.

Note You need to log in before you can comment on or make changes to this bug.