If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Combined nsImage* & gfxImageFrame

RESOLVED FIXED

Status

()

Core
ImageLib
RESOLVED FIXED
19 years ago
3 years ago

People

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

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(13 attachments, 14 obsolete attachments)

133.87 KB, patch
roc
: review+
Details | Diff | Splinter Review
9.21 KB, patch
Josh Aas
: review+
Details | Diff | Splinter Review
12.70 KB, patch
Details | Diff | Splinter Review
48.83 KB, patch
roc
: review+
Details | Diff | Splinter Review
9.22 KB, patch
Robert Longson
: review+
Details | Diff | Splinter Review
59.96 KB, patch
vlad
: review+
Details | Diff | Splinter Review
14.95 KB, patch
karlt
: review+
Details | Diff | Splinter Review
17.14 KB, patch
jimm
: review+
Details | Diff | Splinter Review
59.29 KB, patch
vlad
: review+
Details | Diff | Splinter Review
11.30 KB, patch
roc
: review+
Peter Weilbacher
: review+
Benjamin Smedberg
: review+
mfinkle
: review+
peterv
: review+
Details | Diff | Splinter Review
51.53 KB, patch
roc
: review+
Details | Diff | Splinter Review
383.80 KB, patch
Details | Diff | Splinter Review
2.46 KB, patch
Details | Diff | Splinter Review

Updated

19 years ago
Status: NEW → ASSIGNED

Description

19 years ago
Setting all current Open/Normal to M4.

Comment 1

19 years ago
per leger, assigning QA contacts to all open bugs without QA contacts according
to list at http://bugzilla.mozilla.org/describecomponents.cgi?product=Browser

Updated

19 years ago
QA Contact: 4110 → 1698

Comment 2

19 years ago
Reassigning qa contact to elig@netscape.com

Updated

19 years ago
Assignee: vidur → michaelp
Status: ASSIGNED → NEW
Target Milestone: M4 → M5

Comment 3

19 years ago
I presume Kipp meant that the methods should be NS_IMETHODs. I'm not sure why it
was assigned to me. Michael, I presume this will go to one of the new gfx
owners.

Updated

19 years ago
Assignee: michaelp → kmcclusk

Updated

19 years ago
Target Milestone: M5 → M6

Updated

19 years ago
Status: NEW → ASSIGNED

Updated

19 years ago
Target Milestone: M6 → M8

Updated

19 years ago
Target Milestone: M8 → M9
Moving to M9

Updated

19 years ago
Target Milestone: M9 → M10
This requires changes on all three platforms WIN32, Linux, Mac for the platform
dependant implementations + all of the places that call these methods need to be
modified.

Also need to modify the postscript directory.
Need to coordinate with platform owners, probably will need a carpool to
checkin.

CC'ing platform owners, ramiro, beard, and cone.

XPCODE makes calls to methods on the nsIImage interface in:
S:\mozilla\gfx\src\nsImageRenderer.cpp
S:\mozilla\gfx\tests\btest\BitTest.cpp

Updated

18 years ago
Target Milestone: M10 → M12
Moving to M12

Updated

18 years ago
Target Milestone: M12 → M14
Moving to M14

Updated

18 years ago
Target Milestone: M14 → M15
Don, could you look at this? 
Assignee: kmcclusk → dcone
Status: ASSIGNED → NEW

Updated

18 years ago
Status: NEW → ASSIGNED
Target Milestone: M15 → M17

Updated

18 years ago
Target Milestone: M17 → M20

Comment 9

18 years ago
This bug has been marked future because we have determined that it is not 
critical for netscape 6.0.  If you feel this is an error, or if it blocks your 
work in some way -- please attach your concern to the bug for reconsideration.
Target Milestone: M20 → Future

Updated

17 years ago
QA Contact: elig → petersen

Comment 10

17 years ago
Kevin, is this still an issue.. or does this need to be done.  
If you still want this done, assign it back to me.
Assignee: dcone → kmcclusk
Status: ASSIGNED → NEW
Reassigning to dcone.
Assignee: kmcclusk → dcone
pavlov, fyi

Comment 13

17 years ago
i should probably take this bug, since i've already done this as part of the new
imagelib work.

Comment 14

17 years ago
You can take this Pavlov
Assignee: dcone → pavlov

Updated

17 years ago
Blocks: 66967, 66979
Status: NEW → ASSIGNED
Component: Compositor → ImageLib
OS: Windows NT → All
Priority: P2 → --
Hardware: PC → All
Target Milestone: Future → mozilla0.9

Updated

17 years ago
Depends on: 70938

Comment 15

17 years ago
This is partially fixed.. moving to future to get off my immediate radar.
Target Milestone: mozilla0.9 → Future

Updated

17 years ago
Whiteboard: [imglib]

Comment 16

16 years ago
Moving bugs to new Image: GFX component
Component: ImageLib → Image: GFX

Comment 17

15 years ago
Pretty ol' bug eh? Still valid? ;-)

Comment 18

15 years ago
It looks still valid.

I might take this.  I'm currently looking into combining gfxIImageFrame and
nsIImage.  gfxIImageFrame already has an IDL.

Comment 19

15 years ago
I don't think stuart will mind if I reassign this bug to me. ;)
Status: ASSIGNED → NEW

Comment 20

15 years ago
reassigning for real this time
Assignee: pavlov → paper
Priority: -- → P1
Whiteboard: [imglib]
Target Milestone: Future → mozilla1.3alpha

Updated

15 years ago
Status: NEW → ASSIGNED

Comment 21

15 years ago
(This comment is really for my own reference, however, it might prove usefull to
reviewers/people interested in this bug)

Here's a list of classes that inherit from nsIImage
http://lxr.mozilla.org/seamonkey/search?string=public+nsIImage%5B%5Ea-z%5D&regexp=on

All of them except the mozilla/gfx2/ one need/will be updated.  Unlucky for me,
most of nsIImage functions did not return nsresults.

Here's the list of files using nsIImage:
http://lxr.mozilla.org/seamonkey/ident?i=nsIImage
69 referenced files, 11 of which are gfx2 and can be ignored.

Comment 22

15 years ago
this bug should be marked WONTFIX.  nsIImage isn't public.  gfxIImageFrame is
XPCOMized and should be used far before nsIImage.  The only reason I left it
open is cause I liked  having a 3 digit bug number :-)

Comment 23

15 years ago
but I want to combine nsIImage and gfxIImageFrame.  There's no need for both of
them.. a lot of the gfxIImageframe functions just call nsIImage.

Here's my proposed plan:
1) XPCOMize nsIImage, create nsImage, and have nsImage* inherit from nsImage.
2) Move exact-same-code functions from nsImage* to nsImage.
3) Combine nsIImage and gfxIImageFrame.  Move gfxImageFrame into nsImage.

thoughts?

Comment 24

15 years ago
I agree with Pavlov.. or at least we need to have a reason .. a good reason to
do this.  What would doing this buy us?  What are the risks?  

Comment 25

15 years ago
What's gained:

- code consolidation.  How often have we added something to nsImageWin and then
have to add almost the exact same code to nsImageGTK?  How often have we made a
function in gfxIImageFrame which calls nsImage* to do the task?  Quite often
judging by the code.

- Easier to understand.  Having nsIImageWin and gfxIImageFrame just adds
confusion to a new mozilla hacker.

I can't think of any risks at the moment, but I'd love to hear some.

Comment 26

15 years ago
it would be far easier to take the code from nsImage* and make a gfxImageFrame
impl with it.. then it could be done incrementally.

still, there have got to be more important things to do than this.

Comment 27

15 years ago
aah, I'm beginning to see the light now.

nsIImage is still not needed, so this bug is now the "eliminate nsIImage" bug.

Similar nsIImage* code can go into gfxImageFrame.  nsImage* can inherit from
gfxImageFrame.  We could even rename nsImage* to gfxImageFrame* (ie nsImageWin
to gfxImageFrameWin), but as stuart mentioned on IRC, renaming stuff sucks.
Summary: nsIImage* need to be xpcom'ized → eliminate nsIImage
Target Milestone: mozilla1.3alpha → mozilla1.3beta

Comment 28

15 years ago
I totally don't agree with eliminating nsIImage..  
nsIImage encapulates an image/bitmap.. I am sure that the gfxImageFrame
encapulates something else. 

Comment 29

15 years ago
gfxImageFrame encapsulates nsIImage and adds a (x,y) offset.  gfxImageFrame
contains a nsCOMPtr to one nsIImage, and almost every function inside
gfxImageFrame calls that nsIImage to get it's work done.  Perhaps a better
summary is in order:
Combined nsImage* & gfxImageFrame
                                       +----------+
                                       |nsImageGTK|
                                     _ +----------+
                                     /|
+--------------+    +-------------+ /  +----------+
|gfxIImageFrame| -> |gfxImageFrame| -> |nsImageWin|
+--------------+    +-------------+ \  +----------+
                                    _\|
                                       +---------------+
                                       |nsImageWhatever|
                                       +---------------+

Platform independent code in nsImage* would be moved to gfxImageFrame.  Things
like GetWidth, GetHeight, DecodedRect, GetBytesPix, etc.  All the platform
dependent code would stay in nsImage*.  Any calls/references to nsIImage would
be changed to gfxImageFrame.
Summary: eliminate nsIImage → Combined nsImage* & gfxImageFrame

Comment 30

15 years ago
I don't like combining those two.. at all.  gfxImageFrame does frame things and
nsImage handles bitmaps.  Its not a good place to combine classes.  I really
believe that nsImage.. of all classes is named right and does what it supposed
to do.  Its intuitive, encapulates what its supposed to..etc.  

Comment 31

15 years ago
dcone, have you looked at gfxImageFrame.cpp?  The only functions that have
considerable amount of code are SetImageData and SetAlphaData.. the rest of the
functions call nsImage* functions.

Even your patch for Bug 143046 adds a function to gfxIImageFrame, only to have
the code return something from nsImageWin.  It's a waste, and makes trying to
follow the code harder. gfxIImageFrame was intended to replace nsIImage.

Although I do agree the name gfxIImageFrame is less intuitive than nsIImage, I
don't feel I have enough authority to just rename gfxIImageFrame to nsIImage
(after the combine).  Some people might get gfxIImageFrame confused with
nsIImageFrame.

CCing tor.. I remember him having an interest this bug.  tor, any thought?

Comment 32

15 years ago
There are just a few things I don't like.. but there big things.

1.)  nsImage is for bitmaps/images.  I don't like the idea of having frame in
that class at all.  Its just a naming issue.. but I think its a big deal.  

2.)  Any frame methods at all in nsImage is wrong.  Its not the number of
lines.. its an encapsulation issue.. what are the boundaries of an object.

For me the bottom line is that the current implementation was true to OOD, which
makes things easier to follow.  If you have an image problem.. or want to extend
and image you know right were to go.  If we have things that need to work cross
platform then nsImage needs to have an implementation, but I really dont agree
at all with combining them.. I am on the other side.. I think it will make it
harder to maintain just because the name of the object that combines both will
not describe acurately what each does.  I think I understand what your trying to
say.. but its very important to me to have objects names and encapsulation
boundaries accurate.

Comment 33

15 years ago
Well, at least we do not have totally different views here. :)  Would this be to
more of your liking?

                           +----------+
                           |nsImageGTK|
                         _ +----------+
                         /|
+--------+    +-------+ /  +----------+
|nsIImage| -> |nsImage| -> |nsImageWin|
+--------+    +-------+ \  +----------+
                        _\|
                           +---------------+
                           |nsImageWhatever|
                           +---------------+

This is very similar looking to what we have now, with the exception of nsIImage
having 2 additional variables.. x and y.  Does it make sense that an image can
be set to a position?  
We do not need a whole class just to store x and y.  It's over abstraction. It's
like making a class for image without alpha layers and a seperate class class
for images with alpha layers.  Why do alpha layers belong in an image?  Because
opacity is part of an image.. just like where the image is located (x, y) is
also a part of the image.

Updated

15 years ago
Target Milestone: mozilla1.3beta → mozilla1.3alpha

Comment 34

15 years ago
I used "platform code" and "nsImage*" interchangably in this comment.  nsImage*
refers to all the classes that inherited from nsIImage (nsImageWin, nsImageGTK,
nsImageMac, to name a few).  Patches will be attached once I get them tested on
GTK and OS2.  :)
Here is the list of changes:

1) removed nsIImage.h, all references to the file have been removed, and all
references to nsIImage class have been changed to gfxIImageFrame
----------
2) removed "@mozilla.org/gfx/image/frame;2" and "@mozilla.org/gfx/image;1". 
Added "@mozilla.org/gfx/imageframe;1" and gave it the CID of GFX_IMAGEFRAME_CID.
 Is this okay, or should I assign a totally new CID?  My thinking is that
technically it's still the same class (gfxIImageFrame), with some functions from
nsIImage moved into it.  The only thing that changed is the @mozilla.org ID-name
thing.
----------
3) Made nsImage* inherit from nsImageFrame (which inherits from nsIImageFrame
which is an IDL).  See Comment #29 for nice piccy.
----------
4) replaced nsImage*::width, nsImage*::height, gfxImageFrame::mOffset, and
gfxImageFrame::mSize with gfxImageFrame::mBounds.
----------
5) All platforms had some variables in common.  These variable were moved to
gfxImageFrame as protected and are as follows:
     mRowBytes         Still set by platform code

     mAlphaRowBytes    Still set by platform code
     (mARowBytes)

     mBytesPerPixel    Set in gfxImageFrame::Init(). Removed setting variable
     (mNumBytesPixel)  from GTK, Mac, Win, BeOS, Photon, and XLib. OS2 never
                       had one (and set GetBytesPix incorrectly ;).  QT forces
                       mBytesPerPixel to 4.

     mAlphaWidth       Set to image width & height if mFormat said there was
     mAlphaHeight      an Alpha.  Otherwise 0.

     mAlphaDepth       Set in gfxImageFrame::Init() based on mFormat

     mIsOptimized      Defaults to false.  A couple of platforms are always
     (mOptimized)      optimized, so they override in their init to PR_TRUE.
                       The rest set it when the image actually gets optimized
                       (if at all)

     mIsTopToBottom    I only saw one function using this.. the rest used
                       "#if defined(XP_WIN) || defined(XP_OS2)" to determine
                       how the image was stored.  Defaults to PR_TRUE in
                       gfxImageFrame constructor.. Windows and OS2 change it
                       to PR_FALSE in Init()
----------
6) The following were moved from nsIImage to gfxIImageFrame. Names below are
after xpidl makes the header file:
     GetImageBytesPix was nsIImage::GetBytesPix

     GetBitInfo       now returns length of BitInfo as well as the pointer

     GetHasAlphaMask  gfxImageFrame returns true if mAlphaDepth > 0.
                      Platforms overide with a null check to their private
                      AlphaBits.

     GetAlphaDepth

     drawToImage      nsIImage had two draw functions. IDL doesn't support
     draw             overloading, so one was renamed to DrawScaled.  In alot
     drawScaled       of platforms, draw() just called drawScaled, so that's
     drawTile         what gfxImageFrame::draw does.  GTK, BeOS, Photon, QT,
                      and Xlib all override draw().  Everyone (must) overide
                      drawToImage, drawScaled, and drawTile.  Although in the
                      future, we should implement a generic
                      gfxImageFrame::DrawToImage.  GTK, Windows, and other
                      platforms use almost the same DrawToImage code.
                      gfxIImageFrame had a function called drawTo.  This was
                      renamed to the more descriptive drawToImage.

     ImageUpdated     Code calling ImageUpdated was changed so it didn't
                      need to use nsIInterfaceRequestor to get nsIImage
                      from gfxImageFrame.
                      Moved the SetDecodedRect call from SetImageData to here
                      and made ever nsImage* call gfxImageFrame::ImageUpdate

     SetDecodedRect   This really should be removed (in another bug).. I
                      think ImageUpdated should be doing the decoded
                      variable(s) setting
----------
7) The following functions (below) were moved to gfxImageFrame as protected. 
This means no external classes/code called them.  They can be IDL'd later (when
something actually uses them..) or deleted.
     IsOptimized
     Optimize           SetMutable calls optimize

     GetBits            mainly used by GetImageData

     GetAlphaBits       mainly used by GetAlphaData

     GetAlphaWidth      Neither are currently used
     GetAlphaHeight

     Set/GetAlphaLevel  Not used or implemented at all.  A few platforms store
                        this to mAlphaLevel, but never use it.  I was very
                        tempted to remove the functions all together.

     GetDecoded[XY][12] Currently not used at all.  We should move to a nsRect
                        structure like nsImageOS2 does anyway.

     GetIsRowOrderTopToBottom

     GetColorMap        Was only used by Windows' nsImageClipboard.  I see no
                        evidence that the nsImageClipboard is used (or works),
                        so I replaced it with something I think would work
                        equally as well (and doesn't use GetColorMap)
----------
8) The following functions from nsIImage were removed:
     GetNaturalWidth    \
     SetNaturalWidth     |__ Removed variables too.  See Bug 116649
     GetNaturalHeight    |
     SetNaturalHeight   /

     GetLineStride       gfxIImageFrame already had GetImageBytesPerRow
     GetAlphaLineStride      "             "     "  GetAlphaBytesPerRow

     UnlockImagePixels   These two functions locked either Alpha or Data
     LockImagePixels     depending on the passed in flag.  gfxIImageFrame
                         already had LockImageData, LockAlphaData,
                         UnlockImageData, UnlockAlphaData, so both nsImage*
                         functions were split into two and renamed.
                         Several platforms don't lock alpha/image pixels so
                         gfxImageFrame::(Un)LockImagePixels just returns NS_OK
                         (the other platforms override with (un)locking)
----------
9) nsIInterfaceRequestor removed from gfxIImageFrame.  From what I can tell, it
was only used to request the nsIImage stored inside gfxImageFrame.
----------
10) nsRenderingContextImpl::DrawImage and
nsRenderingContextImpl::DrawScaledImage would convert the src and dst coords so
that they are relative to the image data.  For example, if an imageframe started
at (10,10) and we needed to draw starting at (15,15), 10 was stripped from sr.x
and sr.y, leaving (5,5) into the image data.   This functionality has been moved
to gfxImageFrame as SetRelativeToImageData(), and is called upon
nsImage*::Draw() and nsImage*::DrawScaled.

Comment 35

15 years ago
erk. Regarding Comment 3
That's gfxImageFrame, not nsImageFrame 

Comment 36

15 years ago
Stuart has some major gripes.. changing target milestone (again :P).  The gripes
I remember are:

1) move draw* stuff to nsRenderingContext*

2) Get rid of decoded rect stuff.  Layout should be doing that.  Write code in
layout to remember decoded sections.
Priority: P1 → --
Target Milestone: mozilla1.3alpha → ---

Comment 37

15 years ago
I totally disagree with that plan.. your plan in 33 was acceptable.. I dont like
at all moving anything from nsIImage into gfxIImageFrame or anything that has
the word frame on it.  The image stuff should stay image.. it has nothing to do
with frames.
I will fight tooth and nail not to let happen what your proposing.. its just
plain and simply wrong.  I CANT STRESS THAT ENOUGH!!!!!

Also.. I am trying to get in some GIF changes. so messing around in this area
will be a major nightmare.  If you feel the need to do something.. and you have
some reasons for doing it .. then do what you propose in comment 33.  But Do not
re-distribute or change nsIImage.. that is a bitmap class.... PLEASE.

Comment 38

15 years ago
don't worry, this bug is far from being resolved.  I want to clean up nsIImage's
stucture first.  That's structure, not really code, nor combining classes.  I'll
be filing other bugs for that, and this one will probably have no action for a
while (re: long time). I'll be sure to cc you, dcone.

One thing that hasn't changed though, is that I still believe it's silly to have
all those calls in gfxIImageFrame just calling nsImage*.

Comment 39

15 years ago
As far as "not having a reason": These are some perfectly legitimate reasons to
schedule the landing of such a fix among other fixes that touch this same code,
in loose order of importance:
- code size reduction (includes reducing code duplication)
- performance improvement
- reduced dependencies
- refactoring that makes the code easier to understand and maintain

This bug seems to be driven by at least a few of these reasons, and I think we
should do what we can to fix this the right way (And that means dcone and
stuart/paper coming to an agreement) - there's no need for drama but if we reach
an impasse over specific parts of the proposed changes, then we need to bring
the plan before the layout module owners and the super-reviewers, to decide what
the best course of action is.

Comment 40

15 years ago
Fortunately, a lot of the work can be done without (hopefully) touching the
differences of opinion over this bug.

I've created Bug 181695 and Bug 182658, and I think there will be less issues
with them. :)  There's also Bug 181917, which isn't directly decending from this
bug, but still may be relative to people CC'd here.
QA Contact: chrispetersen → image.gfx
(Assignee)

Updated

9 years ago
Blocks: 467112
(Assignee)

Comment 41

9 years ago
I'm currently working on this.
Assignee: paper → joe
Component: Image: Painting → ImageLib
QA Contact: image.gfx → imagelib
(Assignee)

Updated

9 years ago
Blocks: 435294
(Assignee)

Comment 42

8 years ago
Created attachment 383823 [details] [diff] [review]
create imgFrame, a combination of nsThebesImage and gfxImageFrame (v0)

This is not complete yet (most of Gecko is not converted), but libpr0n compiles and links.

Basically, we want to disallow access to individual frames as the frames themselves, and instead add accessors to imgContainer for the relevant bits. This ensures that nobody has to worry about frames being a different size from the image, and also disallows access to the bits of an image from outside imagelib (modulo making a separate copy in a gfxImageSurface, which will be offered as the solution). This lets us keep images optimized, and stops the silly LockImagePixels usage that nobody should need.

My #1 concern, and the reason for my sr request right now, is the imgContainer API: how it looks, how it feels, and whether it's okay to make a bunch of stuff C++-only. I suspect it is, but I want to defer to people smarter & more experienced than myself.

Incidentally:
 40 files changed, 1589 insertions(+), 2503 deletions(-)
Attachment #383823 - Flags: superreview?(vladimir)
(Assignee)

Comment 43

8 years ago
Created attachment 384137 [details] [diff] [review]
imgFrame patch v0.1

This patch removes all remaining references to nsIImage and gfxIImageFrame. libpr0n still compiles, but I haven't tried compiling anything else.

 90 files changed, 1866 insertions(+), 3032 deletions(-)
Attachment #383823 - Attachment is obsolete: true
Attachment #383823 - Flags: superreview?(vladimir)
(Assignee)

Updated

8 years ago
Duplicate of this bug: 467112
(Assignee)

Comment 45

8 years ago
Created attachment 384515 [details] [diff] [review]
imgFrame patch v1

This patch compiles on OS X. (It crashes on startup.) I've sent it to the try server for compiles on other operating systems.

At this point, I'd like super-review of the API-level changes from Vlad, and, if possible, the changes in each individual's area (svg, layout, content, etc). Most of the changes in the users of imagelib are somewhat mechanical, but it's possible I've messed things up right good.

My main concern with regard to API change is the fact that I've removed Javascript users' access to raw image data - nsIImage and gfxIImageFrame are gone, and imgIContainer exposes gfxASurface and gfxImageSurface to noscript callers only. (I don't think we actually had users of that raw image data, but it's something to keep in mind.)
Attachment #384137 - Attachment is obsolete: true
Attachment #384515 - Flags: superreview?
Attachment #384515 - Flags: review?(bzbarsky)
(Assignee)

Updated

8 years ago
Attachment #384515 - Flags: superreview?(vladimir)
Attachment #384515 - Flags: superreview?
Attachment #384515 - Flags: review?(jwatt)
(Assignee)

Updated

8 years ago
Attachment #384515 - Flags: review?(roc)
(Assignee)

Updated

8 years ago
Attachment #384515 - Flags: review?(jmathies)
(Assignee)

Updated

8 years ago
Attachment #384515 - Flags: review?(joshmoz)
(Assignee)

Updated

8 years ago
Attachment #384515 - Flags: review?(mozbugz)
(Assignee)

Comment 46

8 years ago
Comment on attachment 384515 [details] [diff] [review]
imgFrame patch v1

surely there are some people who work on platform not r? on this bug.
Comment on attachment 384515 [details] [diff] [review]
imgFrame patch v1

on file: modules/libpr0n/public/imgIContainer.idl line 72
> interface imgIContainer : nsISupports

is imgIContainer still the right name for this?


on file: modules/libpr0n/public/imgIContainer.idl line 117
>   [noscript] imgIContainer extractCurrentFrame([const] in nsIntRect aRegion);

extractCurrentFrame is only used by border-image, apparently because it wants
to use the layout helpers which need an imgIContainer.  Would be nice to be
able to remove this function -or- to at least consolidate it with the other
getters (see below).

Also, the argument is an nsIntRect; change the param to aRect, not aRegion.


on file: modules/libpr0n/public/imgIContainer.idl line 139
>   [noscript] void getCurrentFrameRect(in nsIntRect rect);

It seems a lot of the APIs operate either on an arbitrary frame N or the
current frame.  If we want to have both, I'd suggest that they mirror
eachother exactly; that is, have both a getCurrentFrameRect and a
getFrameRect(n).

Another (potentially cleaner) option is to have a getFrameContainer(n) that
would return a new imgIContainer for frame N, but would share data with the
main/parent container. 


on file: modules/libpr0n/public/imgIContainer.idl line 172
>   [noscript] readonly attribute gfxImageSurface currentFrame;

This is a readonly attribute, whereas the very similar
getCurrentFrameSurface() below is a function.  Should probably pick one or the
other.


on file: modules/libpr0n/public/imgIContainer.idl line 178
>   [noscript] gfxASurface getCurrentFrameSurface();

There are 4 different getters here:

imgIContainer extractCurrentFrame(nsIntRect r);
gfxImageSurface getFrame(uint num);
gfxImageSurface getCurrentFrame();
gfxASurface getCurrentFrameSurface();

It seems like these could be consolidated.  I mentioned extractCurrentFrame
above; might be able to get rid of it entirely.

For the others, I would suggest having everything return gfxASurface, and have
callers pass a flag saying whether they want an image surface or would be ok
with a platform-native surface.  Also, a flag could be passed that indicates
whether the caller wants a surface it can modify, or whether a read-only
(shared) surface is fine.  That would get us down to 2 API calls, a
getCurrentFrame and getFrame(int n).

At that point, getCurrentFrame becomes a convenience function, since we can
already grab the right index from currentFrameIndex.  Is it worth having that
convenience function around?  I'd say yes, since 90% of the time it's what you
want, but it would be a smaller API surface to not have it.
Some comments on imgIContainer.idl. Generally I like it.

I think '* @see "gfx2"' can go.

imgIContainer needs a big fat comment explaining exactly what an imgIContainer represents. People need to understand what the difference is between an "image" and an "image container". Or maybe it should just be called nsIImage and lose the "Container"... or heck, even mozilla::IImage?

Do the frame-combining constants at the start of imgIContainer really need to be exposed?

+   * Whether the current frame needs the background painted behind it.
+   */
+  readonly attribute boolean currentFrameNeedsBackground;

Wouldn't isOpaque be more obvious?

Can we make imgIContainer non-scriptable? That would make it easier for C++ users to use.

+  [noscript] void getCurrentFrameRect(in nsIntRect rect);
+  unsigned long getFrameImageDataLength(in unsigned long framenumber);
+  void getFrameColormap(in unsigned long framenumber, 
+                        [array, size_is(paletteLength)] out PRUint8 paletteData,
+                        out unsigned long paletteLength);
+  void setFrameDisposalMethod(in unsigned long framenumber, in PRInt32 aDisposalMethod);
+  void setFrameBlendMethod(in unsigned long framenumber, in PRInt32 aBlendMethod);
+  void setFrameTimeout(in unsigned long framenumber, in PRInt32 aTimeout);
+  void setFrameHasNoAlpha(in unsigned long framenumber);
+  [noscript] void ensureCleanFrame(in unsigned long aFramenum, in PRInt32 aX, in PRInt32 aY, in PRInt32 aWidth, in PRInt32 aHeight, in gfxImageFormat aFormat,
+                                   [array, size_is(imageLength)] out PRUint8 imageData, out unsigned long imageLength);
+  [noscript] void appendFrame(in PRInt32 aX, in PRInt32 aY, in PRInt32 aWidth, in PRInt32 aHeight, in gfxImageFormat aFormat,
+                              [array, size_is(imageLength)] out PRUint8 imageData, out unsigned long imageLength);
+  [noscript] void appendPalettedFrame(in PRInt32 aX, in PRInt32 aY, in PRInt32 aWidth, in PRInt32 aHeight, in gfxImageFormat aFormat, in PRUint8 aPaletteDepth, 
+                                      [array, size_is(imageLength)] out PRUint8 imageData, out unsigned long imageLength,
+                                      [array, size_is(paletteLength)] out PRUint8 paletteData, out unsigned long paletteLength);
+  [noscript] void frameUpdated(in unsigned long framenum, in nsIntRect aNewRect);
   void endFrameDecode(in unsigned long framenumber);
   void decodingComplete();
  [noscript] void setDiscardable(in string aMimeType);
  [noscript] void addRestoreData([array, size_is(aCount), const] in char data,
                                 in unsigned long aCount);
  [noscript] void restoreDataDone();

Can we avoid exposing these? Looks like only imglib uses them, and outsiders shouldn't be using them. I think it's probably worth having an abstract interface that clients use and a concrete subclass that imglib uses. init() should move to the concrete class too. It's actually quite important we ensure that outsiders don't try to get the frame count, for example, since we'll be able to have animated SVG images that don't actually have a frame count.

At the very least, the methods that outsiders use should be clearly separated in the IDL and documentation.

  attribute unsigned short animationMode;
  void startAnimation();
  void stopAnimation();
  void resetAnimation();
  attribute long loopCount;

There must be some redundancy here. Seems like at least we could get rid of animationMode and rename loopCount to animationCount; then animationCount==0 would mean no animation, animationCount==1 would mean just 1 cycle and then stop, animationCount==INT32_MAX would mean unlimited cycles. Fodder for another bug perhaps.

+  [noscript] readonly attribute gfxImageSurface currentFrame;
+  [noscript] gfxASurface getCurrentFrameSurface();

Echoing Vlad, I don't know why we need to have both currentFrame and getCurrentFrameSurface. Shouldn't getCurrentFrameSurface be enough? If someone needs a gfxImageSurface they can check the type of the surface returned by getCurrentFrameSurface; if it's not a gfxImageSurface already, they can copy the gfxASurface to a gfxImageSurface manually. That's hardly any code, and if they don't need to optimize for the already-gfxImageSurface case, it's even less code. I don't think Vlad's flag idea is necessary, but I wouldn't object.

+  [noscript] gfxImageSurface getFrame(in unsigned long framenumber);

I'd quite like to change this since it's going to be a problem for animated SVG images, which don't have numbered frames. It looks like all current users are either getting frame 0 or the last decoded frame. So how about
  gfxASurface getFirstFrameSurface();
  gfxASurface getLastFrameSurface();
?

I would like to keep extractCurrentFrame, since it lets you get an object representing a subregion without necessarily going through a gfxASurface. For SVG images that could be very useful.
> I don't think we actually had users of that raw image data

We sure do: extensions.  At least last I checked.  They might be able to move to <canvas> imagedata, especially if we expose a way for them to draw the objects involved into a canvas...  But worth checking AMO for such extensions and looking into exactly what they're doing.
(Assignee)

Comment 50

8 years ago
Mark Finkle used the AMO source code search and found that there are no users of gfxIImageFrame::getImageData, which is good news - everyone already uses canvas, it seems.
(In reply to comment #50)
> Mark Finkle used the AMO source code search and found that there are no users
> of gfxIImageFrame::getImageData, which is good news - everyone already uses
> canvas, it seems.

Note that the AMO source code search would only hit JS source, not C++ in extensions.  joe suggested that C++-based extensions move to use gfxImageSurface instead, if I remember correctly - and I probably don't.  <canvas> can't be used from any language other than JS.  I have no idea how frozen gfxImageSurface is, given that it's not IDL.  If this is as "frozen" as nsIFrame (i.e. not at all), this may mean there will be future crashes.
Given bug 315357, I would hope there are no JS callers.
this patch is pretty bitrotted in the places where it touches other code (I get lots of rejections based on june commits by roc and dbaron). Any chance of merging the conflicts and posting one that applies cleanly?
(Assignee)

Comment 54

8 years ago
Created attachment 384890 [details] [diff] [review]
updated to tip

This is an updated-to-tip, slightly-fixed version of this patch. (PNGs don't display, but GIFs do!)
(Assignee)

Comment 55

8 years ago
Created attachment 384895 [details] [diff] [review]
updated to tip redux

Whoops, hg qclone didn't do what I wanted it to. This is a more up-to-date patch including fixes to gifs not decoding, etc.
Attachment #384890 - Attachment is obsolete: true
I'm building my decode-on-draw patch on top of this one, and I've run into issues with ExtractCurrentFrame. It seems wrong to me, because this is the only case where we create an imgContainer for something that didn't come from a source image (jpg, png, gif, etc). I'm trying to associate a meaningful mimetype and decoder with each imgContainer, so this case sticks out.

 What's more, this function really _wants_ to return an imgFrame, not an imgContainer. Currently, the only use case is border-image, where essentially we extract this imgContainer, hold onto it for a while, and then call Draw() on it, which in turn just pulls the imgFrame back out and calls Draw() on the frame. So more or less it's just boxing/unboxing.

I'm not positive (I'm still relatively new to XPCOM), but my impression is that the reasoning here is that we have no imgIFrame interface, and we only want to return interfaces in idl specifications. If this is true, and if we agree with roc that we want a way to get image frames without using gfxASurface, maybe it's worth considering making an imgIFrame (certainly with less stuff than the gfxImageFrame that we're eliminating with this patch).

thoughts?
I wouldn't want to expose imgIFrame just for this one case. That adds complexity for image users. Having an Extract operation that returns the same type of object is nice in layout because it means all our image-drawing helpers only need to have one kind of signature.

To implement this, you could have a single completely different imgIContainer implementation that points to the original imgIContainer and contains the slice coordinates. Its draw() method would delegate to the original draw() method appropriately. That would work, and could be better optimized if we feel the need. How does that sound?
(Assignee)

Comment 58

8 years ago
Created attachment 385492 [details] [diff] [review]
updated to tip, PNGs work

This patch is updated to tip. PNGs and JPEGs both work, and GIF non-animations work. Unfortunately there's still a bug in paletted GIF animations - it looks like we're either sampling the wrong part of the palette, or we've got the wrong palette on each frame, I'm not sure which.
Attachment #384895 - Attachment is obsolete: true

Updated

8 years ago
Attachment #384515 - Flags: review?(joshmoz)
Blocks: 435296
Blocks: 501490
So... unless you want everyone on the reviewer list to review the whole patch, it might be nice to say which one is supposed to review what.
(Assignee)

Comment 60

8 years ago
I just want an initial check-for-sanity from the reviewers, not a full review yet.

bzbarsky: content
jwatt: svg
roc: layout
jmathies: win32 bits
karlt: gtk bits

And, if it interests any of you, a check-for-sanity on the imgIContainer interface changes, as roc and vlad did above.
Attachment #385492 - Attachment description: Updated to tip, PNGs work → win32 and mac clipboards seem to use the current frame, but gtk seems to use the initial frame. Can gtk be made consistent with the others? (If nsImageToPixbuf::PatternToPixbuf() is still no longer needed then that can be removed.)
(Assignee)

Comment 61

8 years ago
Comment on attachment 385492 [details] [diff] [review]
updated to tip, PNGs work

Suspect karl meant to put this here:

 win32 and mac clipboards seem to use the current frame, but gtk seems to use the initial frame. Can gtk be made consistent with the others? (If nsImageToPixbuf::PatternToPixbuf() is still no longer needed then that can be removed.)
Attachment #385492 - Attachment description: win32 and mac clipboards seem to use the current frame, but gtk seems to use the initial frame. Can gtk be made consistent with the others? (If nsImageToPixbuf::PatternToPixbuf() is still no longer needed then that can be removed.) → updated to tip, PNGs work
Created attachment 386605 [details] [diff] [review]
Compiles on windows

Haven't run tests but this will compile and the browser runs and looks ok.
Attachment #385492 - Attachment is obsolete: true
(Assignee)

Comment 63

8 years ago
Created attachment 386671 [details] [diff] [review]
updated to tip, integrate robarnold's changes, GIFs work

The previous patches reflected a fundamental misunderstanding on my part of what datatype a palette/colormap should be stored as, and it turns out that fixing that fixes my GIF animation problems too.

This patch also integrates Rob Arnold's changes (I believe all of them - had some interdiff problems, so please check) to make it compile on Windows and be updated to tip.
Attachment #386605 - Attachment is obsolete: true
Blocks: 502710
(Assignee)

Comment 64

8 years ago
Created attachment 387311 [details] [diff] [review]
Fix imgITools xpcshell test
Attachment #386671 - Attachment is obsolete: true
(Assignee)

Comment 65

8 years ago
Created attachment 387745 [details] [diff] [review]
fixed drawing images to canvas and border-image; merged to m-c

This patch is currently making its way through the try server.
Attachment #387311 - Attachment is obsolete: true
(Assignee)

Comment 66

8 years ago
Created attachment 387949 [details] [diff] [review]
fix icon files not having an alpha channel; fix some compilation errors

This is very close. Still remaining are any further build failures (currently running through the try server) and updating for reviewers' comments earlier.

I will be splitting this into quite a number of patches for the purposes of review.
Attachment #384515 - Attachment is obsolete: true
Attachment #387745 - Attachment is obsolete: true
Attachment #384515 - Flags: superreview?(vladimir)
Attachment #384515 - Flags: review?(roc)
Attachment #384515 - Flags: review?(mozbugz)
Attachment #384515 - Flags: review?(jwatt)
Attachment #384515 - Flags: review?(jmathies)
Attachment #384515 - Flags: review?(bzbarsky)
(Assignee)

Comment 67

8 years ago
Oh, also, all our tests pass now, at least on OS X.
(Assignee)

Comment 68

8 years ago
Created attachment 388181 [details] [diff] [review]
fix SVG image reftest; fix compilation on Linux

This patch is updated to tip again. It also fixes an SVG image scaling reftest and compilation on Linux. Currently pushed to try.
Attachment #387949 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Blocks: 503973
(Assignee)

Comment 69

8 years ago
(In reply to comment #47)
> (From update of attachment 384515 [details] [diff] [review])
> on file: modules/libpr0n/public/imgIContainer.idl line 72
> > interface imgIContainer : nsISupports
> 
> is imgIContainer still the right name for this?

No; bug 503973 will track the changes required to split imgIContainer into two separate interfaces, the non-libpr0n version should probably be called mozilla::Image. (Does xpidl support namespaces?)

(In reply to comment #48)
> Can we make imgIContainer non-scriptable? That would make it easier for C++
> users to use.

No, because imgITools uses it.
 
> At the very least, the methods that outsiders use should be clearly separated
> in the IDL and documentation.

Agreed; I'll do this in this bug, and the rest of the changes in bug 503973.
 
>   attribute unsigned short animationMode;
>   void startAnimation();
>   void stopAnimation();
>   void resetAnimation();
>   attribute long loopCount;
> 
> There must be some redundancy here. Seems like at least we could get rid of
> animationMode and rename loopCount to animationCount; then animationCount==0
> would mean no animation, animationCount==1 would mean just 1 cycle and then
> stop, animationCount==INT32_MAX would mean unlimited cycles. Fodder for another
> bug perhaps.

Yeah, please file another bug.
 
> +  [noscript] readonly attribute gfxImageSurface currentFrame;
> +  [noscript] gfxASurface getCurrentFrameSurface();
> 
> Echoing Vlad, I don't know why we need to have both currentFrame and
> getCurrentFrameSurface. Shouldn't getCurrentFrameSurface be enough? If someone
> needs a gfxImageSurface they can check the type of the surface returned by
> getCurrentFrameSurface; if it's not a gfxImageSurface already, they can copy
> the gfxASurface to a gfxImageSurface manually. That's hardly any code, and if
> they don't need to optimize for the already-gfxImageSurface case, it's even
> less code. I don't think Vlad's flag idea is necessary, but I wouldn't object.

I'd prefer Vlad's idea; no need to have boilerplate code converting to gfxImageSurfaces all over the place.

> +  [noscript] gfxImageSurface getFrame(in unsigned long framenumber);
> 
> I'd quite like to change this since it's going to be a problem for animated SVG
> images, which don't have numbered frames. It looks like all current users are
> either getting frame 0 or the last decoded frame. So how about
>   gfxASurface getFirstFrameSurface();
>   gfxASurface getLastFrameSurface();

Is it going to be easy to get an SVG file's first frame surface? If not, I see no reason why we can't just give consumers current frame only. Most places that want the 0th frame just want "a" frame, and IMO current is as good as first.

> I would like to keep extractCurrentFrame, since it lets you get an object
> representing a subregion without necessarily going through a gfxASurface. For
> SVG images that could be very useful.

extractCurrentFrame is sort of ugly, but necessary for border-image without making a lot of other changes. If we want to get rid of it, I'd prefer
(Assignee)

Comment 70

8 years ago
Don't know how that got cut off. If we want to get rid of it, I'd prefer someone to file a followup bug on layout, and I'll do the fix in libpr0n.
(Assignee)

Comment 71

8 years ago
Created attachment 388406 [details] [diff] [review]
code complete

This patch compiles on Windows, Linux, and OS X, and I believe it also passes all our tests (though I'm running through the try server to be sure). It fixes crashes caused by the gfx module not being initialized by tests that don't explicitly use anything inside the gfx module, and it also implements the API changes asked for by roc and vlad in initial API review.

Please look this over; I will be splitting this patch up into multiple parts for formal review tomorrow.
Attachment #388181 - Attachment is obsolete: true
(Assignee)

Comment 72

8 years ago
Created attachment 388529 [details] [diff] [review]
The new API this patch introduces, as well as changes in libpr0n required for it

This is the actual API changes, and the part that needs super-review. I'd like both roc and vlad to SR this, but everyone should take a look to make sure that imgIContainer makes sense for their uses.
Attachment #388529 - Flags: superreview?(vladimir)
Attachment #388529 - Flags: review?(roc)
(Assignee)

Comment 73

8 years ago
Created attachment 388530 [details] [diff] [review]
cocoa widget changes
Attachment #388530 - Flags: review?(joshmoz)
(Assignee)

Comment 74

8 years ago
Created attachment 388531 [details] [diff] [review]
content changes (no svg)
Attachment #388531 - Flags: review?(bzbarsky)
(Assignee)

Comment 75

8 years ago
Created attachment 388532 [details] [diff] [review]
layout changes (no SVG)
Attachment #388532 - Flags: review?(roc)
(Assignee)

Comment 76

8 years ago
Created attachment 388534 [details] [diff] [review]
SVG changes (content and layout)
Attachment #388534 - Flags: review?
(Assignee)

Updated

8 years ago
Attachment #388534 - Flags: review? → review?(jwatt)
(Assignee)

Comment 77

8 years ago
Created attachment 388536 [details] [diff] [review]
changes to image decoders required by changes to imgIContainer API
Attachment #388536 - Flags: review?(vladimir)
(Assignee)

Comment 78

8 years ago
Created attachment 388537 [details] [diff] [review]
changes to GTK and GNOME-specific widget code
Attachment #388537 - Flags: review?(mozbugz)
(Assignee)

Comment 79

8 years ago
Created attachment 388538 [details] [diff] [review]
Windows widget changes
Attachment #388538 - Flags: review?
(Assignee)

Updated

8 years ago
Attachment #388538 - Flags: review? → review?(jmathies)
(Assignee)

Comment 80

8 years ago
Created attachment 388539 [details] [diff] [review]
removal of nsIImage and gfxIImage in gfx/
Attachment #388539 - Flags: review?(vladimir)
(Assignee)

Comment 81

8 years ago
Created attachment 388540 [details] [diff] [review]
rest of changes required (os2, qt widget changes; libeditor changes; xpwidget changes; build changes)
Attachment #388540 - Flags: review?(roc)
(Assignee)

Updated

8 years ago
Attachment #388540 - Flags: review?(mozilla)
(Assignee)

Updated

8 years ago
Attachment #388540 - Flags: review?(benjamin)
(Assignee)

Updated

8 years ago
Attachment #388540 - Flags: review?(mark.finkle)
(Assignee)

Updated

8 years ago
Attachment #388540 - Flags: review?(peterv)
Comment on attachment 388540 [details] [diff] [review]
rest of changes required (os2, qt widget changes; libeditor changes; xpwidget changes; build changes)

Qt part builds and runs fine
Attachment #388540 - Flags: review?(mark.finkle) → review+
(Assignee)

Updated

8 years ago
Attachment #388534 - Flags: review?(jwatt) → review?(longsonr)
Comment on attachment 388529 [details] [diff] [review]
The new API this patch introduces, as well as changes in libpr0n required for it

+  [noscript] void getCurrentFrameRect(in nsIntRect aFrameRect);
+  readonly attribute unsigned long currentFrameIndex;
+  readonly attribute unsigned long numFrames;

Shouldn't these be internal-only?

+  /**
+   * Get a copy of the current frame that you can write to and otherwise
+   * inspect the pixels of.
+   */
+  [noscript] gfxImageSurface copyCurrentFrame();

The comment should be clearer that the caller gets exclusive ownership of and access to the surface.
+  [noscript] void onDataAvailable(in imgIRequest aRequest, in unsigned long aFrame, [const] in nsIntRect aRect);

Instead of perpetuating public frame indices --- which need to go away --- how about just a boolean aIsCurrentFrame here?
Comment on attachment 388532 [details] [diff] [review]
layout changes (no SVG)

Looks good, except for the part where we still have to deal with frame indices.
Attachment #388540 - Flags: review?(roc) → review+
Comment on attachment 388540 [details] [diff] [review]
rest of changes required (os2, qt widget changes; libeditor changes; xpwidget changes; build changes)

xpwidgets changes are good

Comment 87

8 years ago
Comment on attachment 388538 [details] [diff] [review]
Windows widget changes

win widget changes look good to me.
Attachment #388538 - Flags: review?(jmathies) → review+
Comment on attachment 388531 [details] [diff] [review]
content changes (no svg)

> +  return imgContainer.forget().get();

That should just be .forget(), without the .get().

With that change, r=bzbarsky.
Attachment #388531 - Flags: review?(bzbarsky) → review+

Updated

8 years ago
Attachment #388534 - Flags: review?(longsonr) → review+
(Assignee)

Comment 89

8 years ago
(In reply to comment #83)
> (From update of attachment 388529 [details] [diff] [review])
> +  [noscript] void getCurrentFrameRect(in nsIntRect aFrameRect);
> +  readonly attribute unsigned long currentFrameIndex;
> +  readonly attribute unsigned long numFrames;
> 
> Shouldn't these be internal-only?

Yes - I will add an external |readonly attribute boolean animated| to handle the current users, which are really just checking for that.

(In reply to comment #84)
> +  [noscript] void onDataAvailable(in imgIRequest aRequest, in unsigned long
> aFrame, [const] in nsIntRect aRect);
> 
> Instead of perpetuating public frame indices --- which need to go away --- how
> about just a boolean aIsCurrentFrame here?

OK.

Updated

8 years ago
Attachment #388540 - Flags: review?(mozilla) → review+

Comment 90

8 years ago
Comment on attachment 388540 [details] [diff] [review]
rest of changes required (os2, qt widget changes; libeditor changes; xpwidget changes; build changes)

Joe, thanks for thinking of the OS/2 code and for the chance to review this.

>-  nsCOMPtr<gfxIImageFrame> frame;
>-  aCursor->GetFrameAt(0, getter_AddRefs(frame));
>+  nsRefPtr<imgIContainer> frame;
>+  aCursor->CopyCurrentFrame(getter_AddRefs(frame));

This gives me

M:/central/mozilla/widget/src/os2/nsWindow.cpp:1684: error: no matching function for call to 'imgIContainer::CopyCurrentFrame(nsRefPtrGetterAddRefs<imgIContainer>)'
../../../dist/include/imgIContainer.h:99: note: candidates are: virtual nsresult imgIContainer::CopyCurrentFrame(gfxImageSurface**)

Using nsRefPtr<gfxImageSurface> as suggested by the compiler works much better.

>   // if the image is ridiculously large, exit because
>   // it will be unrecognizable when shrunk to 32x32
>-  PRInt32 width, height;
>-  frame->GetWidth(&width);
>-  frame->GetHeight(&height);
>+  PRInt32 width = frame->Width();
>+  PRInt32 height = frame->Height();;

Nit: please remove the second semicolon here.

>-HBITMAP nsWindow::CreateTransparencyMask(gfx_format format,
>+HBITMAP nsWindow::CreateTransparencyMask(gfxASurface::gfxImageFormat format,

Please adapt the prototype in nsWindow.h accordingly.

Otherwise this seems to work. :-) So r+ me with these changes.

Updated

8 years ago
Attachment #388540 - Flags: review?(benjamin) → review+
(Assignee)

Comment 91

8 years ago
Created attachment 389077 [details] [diff] [review]
changes as requested by roc in review (interdiff)
Attachment #389077 - Flags: review?(roc)
(Assignee)

Comment 92

8 years ago
Created attachment 389078 [details] [diff] [review]
Full patch including requested changes
Attachment #388406 - Attachment is obsolete: true
Attachment #389077 - Flags: review?(roc) → review+
Attachment #388539 - Flags: review?(vladimir) → review+
Attachment #388536 - Flags: review?(vladimir) → review+
Attachment #388529 - Flags: superreview?(vladimir) → superreview+
Attachment #388529 - Flags: review?(roc) → review+
+/* [noscript] void onDataAvailable (in imgIRequest request, in unsigned long frame, [const] in nsIntRect rect); */
 NS_IMETHODIMP imgRequest::OnDataAvailable(imgIRequest *request,

You need to fix the type of the second parameter in the comment
Attachment #388532 - Flags: review?(roc) → review+
Attachment #388540 - Flags: review?(peterv) → review+

Comment 94

8 years ago
A note for drivers, a number of win7 specific features rely on this, so if possible it would be great if we could get this into 3.6.
Comment on attachment 388537 [details] [diff] [review]
changes to GTK and GNOME-specific widget code

>- * An interface that allows converting an nsIImage to a GdkPixbuf*.
>+ * An interface that allows converting the first frame of an imgIContainer to a GdkPixbuf*.

"the _current_ frame" would describe what is currently happening.

>-    return PatternToPixbuf(pattern, width, height);

Please remove the unused PatternToPixbuf method.

>     // Get first image frame
>-    nsCOMPtr<gfxIImageFrame> frame;
>-    aCursor->GetFrameAt(0, getter_AddRefs(frame));
>-    if (!frame)
>-        return NS_ERROR_NOT_AVAILABLE;
>-
>-    nsCOMPtr<nsIImage> img(do_GetInterface(frame));
>-    if (!img)
>-        return NS_ERROR_NOT_AVAILABLE;
>-
>-    GdkPixbuf* pixbuf = nsImageToPixbuf::ImageToPixbuf(img);
>+    GdkPixbuf* pixbuf = nsImageToPixbuf::ImageToPixbuf(aCursor);

Is the current frame likely to be the first frame?
If so, please add a comment to explain.

Or, are you changing the behavior because you think the current frame is as
good as the first?  If so, the comment needs updating.
Comment on attachment 388537 [details] [diff] [review]
changes to GTK and GNOME-specific widget code

r+ assuming the comments above are addressed
Attachment #388537 - Flags: review?(mozbugz) → review+

Updated

8 years ago
Attachment #388530 - Flags: review?(joshmoz) → review+
(Assignee)

Comment 97

8 years ago
Created attachment 389536 [details] [diff] [review]
Final patch, including all review changes

This patch includes all changes requested by review, and is ready to go.
Attachment #389078 - Attachment is obsolete: true

Comment 98

8 years ago
Hi Joe, I was working on your patch and found a small mistake.

@@ -1682,37 +1680,19 @@ nsCSSRendering::PaintBackgroundWithSC(ns
+    PRBool isOpaque;
+    if (NS_SUCCEEDED(image->GetCurrentFrameIsOpaque(&isOpaque)) && !isOpaque)
+      drawBackgroundColor = PR_TRUE;

This should be:

+    if (NS_SUCCEEDED(image->GetCurrentFrameIsOpaque(&isOpaque)) && isOpaque)
+      drawBackgroundColor = PR_FALSE;
Yeah -- to provide a little more code-context, note that the chunk Ryo quotes in comment 98 is inside an "if( ... && drawBackgroundColor) {" clause.  So, drawBackgroundColor is already flipped *on* when we hit that code (and we want to flip it *off* as an optimization, if the frame is opaque).
(Assignee)

Comment 100

8 years ago
Created attachment 389579 [details] [diff] [review]
small changes

Wow, thanks so much! If you find anything else, please let me know.

This patch is some small changes noticed by others - the mistake Ryo noticed, as well as some code that re-used surfaces if it could, left over from before copyCurrentFrame was explicitly called "copy."
(Assignee)

Comment 101

8 years ago
http://hg.mozilla.org/mozilla-central/rev/b94bc4be53ca
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Depends on: 505473

Updated

8 years ago
Depends on: 505474

Updated

8 years ago
Blocks: 505525
No longer blocks: 505525
Depends on: 505525

Comment 102

8 years ago
We also take care of
content/canvas/src/nsCanvasRenderingContextGL.cpp
content/canvas/src/nsCanvasRenderingContextGL.h
content/canvas/src/nsCanvasRenderingContextGLWeb20.cpp

Comment 103

8 years ago
(In reply to comment #102)
Filed to bug 505663.
Comment on attachment 389536 [details] [diff] [review]
Final patch, including all review changes

>+EXTRA_DSO_LDOPTS += $(LIBXUL_DIST)/lib/$(LIB_PREFIX)thebes.$(LIB_SUFFIX)
Are these symbols exported from xul.dll or otherwise accessible from xulrunner?
(Assignee)

Comment 105

8 years ago
Yes, Thebes is exported from xul.
Blocks: 504617

Comment 106

8 years ago
(In reply to comment #100)

> Wow, thanks so much! If you find anything else, please let me know.
Joe,
the OS/2 build breaks, cause somehow in your final version of the patch you changed in widget/src/os2/nsWindow.cpp 2nd hunk
  nsRefPtr<imgIContainer> frame;
(in the first version, https://bugzilla.mozilla.org/attachment.cgi?id=388540, that got r+ from peterw) that works and is in all other widget files
to
  nsRefPtr<gfxImageFrame> frame;
which doesn't work as you removed it.
Is it possible to fix this typo here or do you want me to file a follow-up bug?

Comment 107

8 years ago
(In reply to comment #106)
> (In reply to comment #100)
Ah, from comment #90 it should be nsRefPtr<gfxImageSurface> and not nsRefPtr<gfxImageFrame> and that indeed works. In comment #106 I was a bit to early for pressing commit nsRefPtr<imgIContainer> doesn't compile.
(Assignee)

Comment 108

8 years ago
If there remains a problem, please file a followup bug, and I'll fix it. :)

Updated

8 years ago
Depends on: 505887
"can't read .../gfx/src/shared/Makefile.in: No such file or directory"

Please, remove the following line too:

/toolkit/toolkit-makefiles.sh
    * line 122 -- gfx/src/shared/Makefile
(Assignee)

Comment 110

8 years ago
Please file followup bugs for any remaining issues.

Updated

8 years ago
Depends on: 505663
(Assignee)

Updated

8 years ago
Depends on: 506113
Depends on: 513738
You need to log in before you can comment on or make changes to this bug.