Closed Bug 584841 Opened 9 years ago Closed 9 years ago

Create private mozilla::imagelib::Image API for imgIContainer implementations to inherit from

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta5+

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(14 files, 8 obsolete files)

7.47 KB, patch
bholley
: review+
Details | Diff | Splinter Review
28.28 KB, patch
bholley
: review+
Details | Diff | Splinter Review
2.99 KB, patch
bholley
: review+
Details | Diff | Splinter Review
36.91 KB, patch
bholley
: review-
Details | Diff | Splinter Review
2.39 KB, patch
bholley
: review+
Details | Diff | Splinter Review
7.79 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
8.66 KB, patch
bholley
: review+
Details | Diff | Splinter Review
2.35 KB, patch
bholley
: review+
Details | Diff | Splinter Review
46.00 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
2.04 KB, patch
bholley
: review+
Details | Diff | Splinter Review
45.73 KB, patch
bholley
: review+
Details | Diff | Splinter Review
8.73 KB, patch
bholley
: review+
Details | Diff | Splinter Review
12.11 KB, patch
bholley
: review+
Details | Diff | Splinter Review
140.97 KB, patch
Details | Diff | Splinter Review
Per discussion today in #gfx, bholley & joe & I propose splitting imgContainer.idl into:
 * imgIContainer.idl
  - the existing public interface, pruned to only contain "public" methods

 * mozilla::imagelib::Image (<imagelib>/src/Image.h & cpp)
  - contains imagelib-private methods
  - contains shared functionality between image types (e.g. owning & managing the AnimationSuspender in the proposed patch on bug 359608, and the imgStatusTracker that was added in bug 572520)

 * mozilla::imagelib::RasterImage  (<imagelib>/src/RasterImage.h & cpp)
  - Inherits from mozilla::imagelib::Image
  - This is just the existing "imgContainer" class. (minus the functionality that gets moved to Image & will be inherited from it)

 * mozilla::imagelib::VectorImage (<imagelib>/src/VectorImage.h & cpp)
  - Inherits from mozilla::imagelib::Image
  - This will be the image type added in bug 276431 for SVG images. (might end up adding it as a "stub" in the patches on this bug here, though)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Blocks: 584491
This first patch just adds the mozilla::imagelib::Image class. (which extends imgIContainer & which is implemented by imgContainer)

This patch also promotes two imgContainer methods (IsInitialized & GetStatusTracker) to be methods on the "Image" superclass, too.  This lets us refer to "Image" instead of "imgContainer" in a number of places, which lets us be more generic, per bug 584491.
Attachment #463788 - Flags: review?(bobbyholley+bmo)
This second patch just switches us to using "Image" instead of "imgContainer" in a few places, to be more generic (as suggested at the end of my previous comment).  (This will allow SVG Images to be used in those spots.)
Attachment #463789 - Flags: review?(bobbyholley+bmo)
So, an upcoming patch will rename imgContainer to mozilla::imagelib::RasterImage -- but it turns out that in order to do that, imgContainer's *friend* classes need to be in the mozilla::imagelib namespace, too.  (otherwise their 'friend' declaration lines are just ignored)

So, this patch renames the friend class imgDiscardTracker to be inside the namespace, with new name "mozilla::imagelib::ImageDiscardTracker."  (This just renames the class -- I'll rename the file in the next patch.)
Attachment #463790 - Flags: review?(bobbyholley+bmo)
This completes the previous patch by renaming the actual files imgDiscardTracker.[h|cpp] (with "hg mv" -- not visible in bugzilla's diff viewer, but present as "rename from"/"rename to" in the raw patch).

This also updates "#include imgDiscardTracker.h" lines to refer to the new name.
Attachment #463791 - Flags: review?(bobbyholley+bmo)
Attachment #463790 - Attachment description: patch 3: s/imgDiscardTracker/mozilla::imagelib::ImageDiscardTracker/ → patch 3: rename imgDiscardTracker class
This patch renames the imgContainer class to mozilla::imagelib::RasterImage.

This also renames other similarly-named tokens -- e.g. NS_IMGCONTAINER_CID and "@mozilla.org/image/container;3" in nsImageModule.cpp.

To make this patch as bitesize as possible, this patch leaves out:
 - whitespace cleanup from aligning function signatures (that'll be patch 6)
 - imgContainer.* file-renames (that'll be patch 7)
Attachment #463793 - Flags: review?(bobbyholley+bmo)
This patch is just whitespace changes in imgContainer.*, to:
 (a) fix function-signature-alignment after the previous patch's rename
 (b) put the signature's return type on its own line, to save on line-length & to match the convention in the rest of Gecko.
Attachment #463794 - Flags: review?(bobbyholley+bmo)
Attachment #463794 - Attachment is patch: true
Attachment #463794 - Attachment mime type: application/octet-stream → text/plain
This patch does the "hg mv" for imgContainer.[h|cpp] -->  RasterImage.[h|cpp], and updates #include lines as appropriate.
Attachment #463795 - Flags: review?(bobbyholley+bmo)
This patch adds a 'GetImageType' method to imgIContainer.idl, so we can behave differently depending on which type of image we're dealing with.  (and so we can double-check the type before static_cast'ing to a specific base class, in places where we need to do that.)

Note: This method is specifically on the public imgIContainer.idl interface instead of the imagelib-private Image.h interface, because I need to use it from layout code (e.g. nsImageFrame.cpp) to do slightly different things for SVG vs raster images.
Attachment #463797 - Flags: review?(bobbyholley+bmo)
This patch moves raster-image-specific methods & constants from the imagelib-private chunk of imgIContainer.idl over to the RasterImage class.

These are basically the methods that get used by decoders -- and so now the decoders all deal with a RasterImage rather than an imgIContainer. 

NOTE: Now that these methods are purely C++, they could probably use some deCOMtamination, but I'm avoiding that here to minimize impact.  (The one thing I did deCOM in this patch was "attribute long loopCount", whose Getter never actually gets used, and whose setter's return-value is never checked -- so that one was trivial.)
Attachment #463799 - Flags: review?(bobbyholley+bmo)
This is a helper-patch that's required in order for the next patch to compile.

In m-c right now, imgStatusTracker has a pointer to an imgIContainer (the public interface), but it makes calls to "imagelib-private" methods on that object (e.g. GetCurrentFrameRect, GetCurrentFrameIndex). The next patch will move those "private" methods out of imgIContainer and into Image -- so to keep imgStatusTracker compileable, this patch changes its imgIContainer pointer into an Image pointer.
Attachment #463800 - Flags: review?(bobbyholley+bmo)
This patch moves the remaining imagelib-private methods to Image.h & revs the UUID of imgIContainer.

I left the animation-related chunks (start/stop/resetAnimation, kDontAnimMode, etc) behind in imgIContainer.idl, since those aren't actually imagelib-private.  (That's all used in layout code currently.)

As was the case in comment 9, most of the methods that this patch moves could use some deCOM (since they're now purely C++).  I avoided doing that for now, to minimize impact & any potential for unexpected behavior-changes.
Attachment #463801 - Flags: review?(bobbyholley+bmo)
... and that's it for my patches on this bug!

Sorry for the large number -- I'm hoping that they're bite-size enough to be easily digested. :)  (They also should compile & run successfully at each intermediate step.)

In case it helps with review, the patches fall (roughly) into these buckets:
 * Patches 1-2:  Create & use the abstract "mozilla::imagelib::Image" class.
 * Patches 3-7:  Rename imgContainer to mozilla::imagelib::RasterImage.
 * Patches 8-11: Migrate the "private" parts of the public imgIContainer interface into Image & RasterImage.
(In reply to comment #12)
> (They also should compile & run successfully at each
> intermediate step.)

D'oh -- the patches compiled fine locally in an incremental build, but apparently a full rebuild fails (at least on tryserver) with issues linking to libimgicon.

That's related to this change from patch 9, which patches modules/libpr0n/decoders/icon/Makefile.in:
>-IS_COMPONENT    = 1
>+FORCE_STATIC_LIB = 1
That change makes this Makefile match the rest of the decoder subdirectories' Makefiles, and it fixed linker errors that I was getting with resolving RasterImage methods from within the icon directory.  Apparently that change has side effects, though.

I won't be able to investigate that today (about to leave for a friend's wedding), but I'll look into it tomorrow.  It shouldn't affect the rest of this bug's patches, in any case.
(In reply to comment #3)
> Created attachment 463790 [details] [diff] [review]
> patch 3: rename imgDiscardTracker class
> 
> So, an upcoming patch will rename imgContainer to
> mozilla::imagelib::RasterImage -- but it turns out that in order to do that,
> imgContainer's *friend* classes need to be in the mozilla::imagelib namespace,
> too.  (otherwise their 'friend' declaration lines are just ignored)
> 
> So, this patch renames the friend class imgDiscardTracker to be inside the
> namespace, with new name "mozilla::imagelib::ImageDiscardTracker."  (This just
> renames the class -- I'll rename the file in the next patch.)

You can probably say "friend class ::imgDiscardTracker", but this is Just Fine too.
(In reply to comment #13)
> D'oh -- the patches compiled fine locally in an incremental build, but
> apparently a full rebuild fails (at least on tryserver) with issues linking to
> libimgicon.

Turns out it was libxul (tryserver) vs. non-libxul (my local build) that's the problem.  Non-libxul builds are fine with this bug's patches. Investigating the libxul issue...

(In reply to comment #14)
> You can probably say "friend class ::imgDiscardTracker", but this is Just Fine
> too.

Ah, thanks -- the "::" prefix must be the magic sauce I was missing there.  But yeah, I think I marginally prefer keeping imgDiscardTracker namespaced at this point, since we're moving in the direction of namespaces, and the patches are already there.
Comment on attachment 463788 [details] [diff] [review]
patch 1: create mozilla::imagelib::Image

r=bholley
Attachment #463788 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 463789 [details] [diff] [review]
patch 2: change some imgContainer pointers to be Image pointers

diff --git a/modules/libpr0n/src/imgRequest.h b/modules/libpr0n/src/imgRequest.h
--- a/modules/libpr0n/src/imgRequest.h
+++ b/modules/libpr0n/src/imgRequest.h
@@ -41,7 +41,7 @@
 #ifndef imgRequest_h__
 #define imgRequest_h__
.
-#include "imgContainer.h"
+#include "Image.h"

Can't we just forward-declare mozilla::imagelib::Image?

Also, imgRequest.h actually includes imgContainer.h twice right now, so remove the other one.

diff --git a/modules/libpr0n/src/imgRequestProxy.h b/modules/libpr0n/src/imgRequestProxy.h
--- a/modules/libpr0n/src/imgRequestProxy.h
+++ b/modules/libpr0n/src/imgRequestProxy.h
@@ -44,7 +44,7 @@
 #include "imgIDecoderObserver.h"
 #include "nsISecurityInfoProvider.h"
.
-#include "imgIContainer.h"
+#include "Image.h"

forward-declare.

r=bholley
Attachment #463789 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 463790 [details] [diff] [review]
patch 3: rename imgDiscardTracker class

diff --git a/modules/libpr0n/src/imgContainer.h b/modules/libpr0n/src/imgContainer.h
--- a/modules/libpr0n/src/imgContainer.h
+++ b/modules/libpr0n/src/imgContainer.h
@@ -326,14 +326,14 @@ private: // data
.
   // Discard members
   PRUint32                   mLockCount;
-  imgDiscardTrackerNode      mDiscardTrackerNode;
+  mozilla::imagelib::ImageDiscardTrackerNode    mDiscardTrackerNode;

Line these up.

I don't think the "img" needs to translate to "Image" here. How about mozilla::imagelib::DiscardTracker and mozilla::imagelib::DiscardTrackerNode.

r- on account of the rename since this patch is more or less just the rename. ;-)
Attachment #463790 - Flags: review?(bobbyholley+bmo) → review-
Comment on attachment 463791 [details] [diff] [review]
patch 4: rename imgDiscardTracker files

r- for the same reasons as patch 3.
Attachment #463791 - Flags: review?(bobbyholley+bmo) → review-
Comment on attachment 463793 [details] [diff] [review]
patch 5: rename imgContainer class

diff --git a/modules/libpr0n/src/ImageDiscardTracker.h b/modules/libpr0n/src/ImageDiscardTracker.h
--- a/modules/libpr0n/src/ImageDiscardTracker.h
+++ b/modules/libpr0n/src/ImageDiscardTracker.h
@@ -40,28 +40,28 @@

+// Struct to make an RasterImage insertable into the tracker list. This

"a RasterImage".

+// on RasterImage construction. Thus, an RasterImage must always call

here too.



If you do a grep "^+" | grep -i container on your patch, you'll find several lines of the form:

+  RasterImage* container = static_cast<RasterImage*>(iContainer.get());

We shouldn't use the name "container" here. Call it image, aImage, or something similar.

+  rv = inStr->ReadSegments(RasterImage::WriteToContainer,

Please add another patch at the top of your patch stack that renames WriteToContainer to WriteToImage or WriteImageData or something similar.

r=bholley with those fixes.
Attachment #463793 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 463794 [details] [diff] [review]
patch 6: fix whitespace after imgContainer rename

r=bholley
Attachment #463794 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 463795 [details] [diff] [review]
patch 7: rename imgContainer files

r=bholley
Attachment #463795 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 463797 [details] [diff] [review]
patch 8: add imgIContainer::GetImageType

I'd rather this be xpcomish so we can access it from script (tests etc), so make this a readonly attribute.

Also, I'd prefer imgIContainer::TYPE_RASTER to imgIContainer::IMAGE_TYPE_RASTER.

This will also need sr.
Attachment #463797 - Flags: review?(bobbyholley+bmo) → review-
(In reply to comment #23)
> Comment on attachment 463797 [details] [diff] [review]
> patch 8: add imgIContainer::GetImageType

> Also, I'd prefer imgIContainer::TYPE_RASTER to
> imgIContainer::IMAGE_TYPE_RASTER.

On that note, can we call it GetType() too instead?
(In reply to comment #23)
> Comment on attachment 463797 [details] [diff] [review]
> patch 8: add imgIContainer::GetImageType
> 
> I'd rather this be xpcomish so we can access it from script (tests etc), so
> make this a readonly attribute.

annnd, because I'm so fickle, we just decided on irc that the best way to go here is to add a c++-only method that returns the value directly (useful for assertions). So we get the best of both worlds!
Comment on attachment 463799 [details] [diff] [review]
patch 9: Move methods from imgIContainer to RasterImage

diff --git a/modules/libpr0n/public/imgIContainer.idl b/modules/libpr0n/public/imgIContainer.idl
--- a/modules/libpr0n/public/imgIContainer.idl
+++ b/modules/libpr0n/public/imgIContainer.idl

-  /* Add compressed source data to the imgContainer.
-   *
-   * The decoder will use this data, either immediately or at draw time, do
-   * decode the image.
-   */
-  [noscript] void addSourceData([array, size_is(aCount), const] in char data,
-                                 in unsigned long aCount);
-
   /* Called after the all the source data has been added with addSourceData. */
   [noscript] void sourceDataComplete();


sourceDataComplete(), newSourceData(), and setSourceSizeHint() should go anywhere addSourceData goes.


diff --git a/modules/libpr0n/src/RasterImage.h b/modules/libpr0n/src/RasterImage.h
--- a/modules/libpr0n/src/RasterImage.h
+++ b/modules/libpr0n/src/RasterImage.h

+   * Sets the size of the container. This should only be called by the decoder. This function may be called multiple

This line is too long

+  /* Enumerated values */

I don't think that line is particularly useful. 

Oh man this patch is righteous. r- only because I want to look at it again, r+ in spirit. ;-)
Attachment #463799 - Flags: review?(bobbyholley+bmo) → review-
Comment on attachment 463800 [details] [diff] [review]
patch 10: give imgStatusTracker an Image*

r=bholley
Attachment #463800 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 463801 [details] [diff] [review]
patch 11: move imagelib-private methods to Image

diff --git a/modules/libpr0n/src/Image.h b/modules/libpr0n/src/Image.h
--- a/modules/libpr0n/src/Image.h
+++ b/modules/libpr0n/src/Image.h

+  // XXXdholbert this should probably be RasterImage-only.

As discussed on IRC, can you move these to RasterImage now?


diff --git a/modules/libpr0n/src/imgTools.cpp b/modules/libpr0n/src/imgTools.cpp
--- a/modules/libpr0n/src/imgTools.cpp
+++ b/modules/libpr0n/src/imgTools.cpp


+  // If the caller didn't provide an ImageContainer, create one
+  if (*aContainer) {
+    NS_ABORT_IF_FALSE((*aContainer)->GetImageType() ==
+                      imgIContainer::IMAGE_TYPE_RASTER,
+                      "wrong type of imgIContainer for decoding into");
+    image = static_cast<Image*>(*aContainer);
+  } else {.
+    *aContainer = image = new RasterImage();
+    NS_ADDREF(image);

Can we stop calling them containers?

Looks great otherwise.

this needs sr too.
Attachment #463801 - Flags: review?(bobbyholley+bmo) → review-
blocking2.0: --- → ?
(In reply to comment #17)
> Can't we just forward-declare mozilla::imagelib::Image?

Done -- yes, we can fwd-declare, but first I have to move imgRequest::IsReusable() definition from the imgRequest.h to imgRequest.cpp file in order for that to work, since that definition makes calls to a method on mImage. (This patch moves the method-definition now.)

> Also, imgRequest.h actually includes imgContainer.h twice right now, so remove
> the other one.

Done -- a later patch did this before -- doing it here now.

> --- a/modules/libpr0n/src/imgRequestProxy.h
> +#include "Image.h"
> 
> forward-declare.

Done.
Attachment #463789 - Attachment is obsolete: true
Attachment #464504 - Flags: review+
(In reply to comment #18)
> b/modules/libpr0n/src/imgContainer.h
> --- a/modules/libpr0n/src/imgContainer.h
> +++ b/modules/libpr0n/src/imgContainer.h
>    PRUint32                   mLockCount;
> -  imgDiscardTrackerNode      mDiscardTrackerNode;
> +  mozilla::imagelib::ImageDiscardTrackerNode    mDiscardTrackerNode;
> 
> Line these up.

The misalignment is only temporary -- they get lined up in patch 5, where imgContainer enters the mozilla::imagelib namespace, and we drop the "mozilla::imagelib::" prefix here.

> I don't think the "img" needs to translate to "Image" here. How about
> mozilla::imagelib::DiscardTracker and mozilla::imagelib::DiscardTrackerNode.

Done.  (This patch is identical to previous version - just with s/ImageDiscardTracker/DiscardTracker/ applied globally.)
Attachment #463790 - Attachment is obsolete: true
Attachment #464507 - Flags: review?(bobbyholley+bmo)
Attachment #464507 - Attachment description: patch 3: rename imgDiscardTracker class → patch 3 v2: rename imgDiscardTracker class
(In reply to comment #19)
> patch 4: rename imgDiscardTracker files
> r- for the same reasons as patch 3.

Applied s/ImageDiscardTracker/DiscardTracker/ here, too.
Attachment #464508 - Flags: review?(bobbyholley+bmo)
Attachment #463791 - Attachment is obsolete: true
(In reply to comment #20)
> diff --git a/modules/libpr0n/src/ImageDiscardTracker.h
> "a RasterImage".
> here too.
Thanks, good catches!

> We shouldn't use the name "container" here. Call it image, aImage, or something
> similar.

I expect that to touch a lot of lines of code -- every xxxContainer variable declaration & every usage of those variables.  I prefer to do that mass-variable-renaming in a new patch, probably the same one you suggested for renaming WriteToContainer, so that this patch here can remain fairly focused.

> Please add another patch at the top of your patch stack that renames
> WriteToContainer to WriteToImage or WriteImageData or something similar.

Will do, with the variable-renames in that patch as well (per above).
Attachment #463793 - Attachment is obsolete: true
Attachment #464513 - Flags: review+
Attachment #464504 - Attachment description: patch 2 v2: change some imgContainer pointers to be Image pointers [r=bholley → patch 2 v2: change some imgContainer pointers to be Image pointers [r=bholley]
(In reply to comment #23)
> I'd rather this be xpcomish so we can access it from script (tests etc), so
> make this a readonly attribute.

Done.

> Also, I'd prefer imgIContainer::TYPE_RASTER to

Done.

> This will also need sr.

Requesting sr=vlad. (I think you spoke to him about being an SR for this in IRC yesterday.)

> On that note, can we call it GetType() too instead?

Done.

> annnd, because I'm so fickle, we just decided on irc that the best way to go
> here is to add a c++-only method that returns the value directly

Done.
Attachment #463797 - Attachment is obsolete: true
Attachment #464522 - Flags: superreview?(vladimir)
Attachment #464522 - Flags: review?(bobbyholley+bmo)
(reposting patch 8 with a tweak to header-comment for the C++ GetType accessor.)
Attachment #464522 - Attachment is obsolete: true
Attachment #464526 - Flags: superreview?(vladimir)
Attachment #464526 - Flags: review?(bobbyholley+bmo)
Attachment #464522 - Flags: superreview?(vladimir)
Attachment #464522 - Flags: review?(bobbyholley+bmo)
Here's the updated version of patch 9.

First, RE the build issues brought up in comment 13 & comment 15 -- so in non-libxul builds, we need to change "IS_COMPONENT" to "FORCE_STATIC_LIB" in the Makefile, in order to link against RasterImage.cpp code from /decoders/icon.  But in libxul builds, we *don't* need that change -- and in fact, that change keeps us from linking libxul correctly.

So, this patch just uses "#if MOZ_ENABLE_LIBXUL ... else ... endif" to make both types of builds happy.

On to bholley's review comments...
(In reply to comment #26)
> sourceDataComplete(), newSourceData(), and setSourceSizeHint() should go
> anywhere addSourceData goes.

Done. (this moves all of them now, and does the necessary casting in imgRequest.cpp for that to work)

> This line is too long

Done. (Re-wrapped the line)

> +  /* Enumerated values */
> 
> I don't think that line is particularly useful. 

Removed.

> Oh man this patch is righteous. r- only because I want to look at it again, r+
> in spirit. ;-)

Thanks! :)
Attachment #464529 - Flags: review?(bobbyholley+bmo)
(In reply to comment #35)
> First, RE the build issues brought up in comment 13 & comment 15

Note that these build issues are specific to /decoders/icon because icon is specifically its own library, separate from imagelib.  The other decoders used to all be their own libraries, too, but 168048 bundled almost all of them into imagelib.  Seems like "icon" probably should do the same, but I don't want to do that in this bug.
er, meant to say "... but bug 168048 bundled ..."
(In reply to comment #28)
> diff --git a/modules/libpr0n/src/imgTools.cpp
> +  // If the caller didn't provide an ImageContainer, create one
> Can we stop calling them containers?

The above was review feedback for patch 10, but its chunk is now in patch 9, so I'm updating the comment there.  (I actually replaced "ImageContainer" with "imgIContainer" in the comment, since that's what the argument actually is -- ok'd this with bholley on IRC.)

Also requesting sr?vlad on the API changes here (private-method-removal in imgIContainer).
Attachment #464529 - Attachment is obsolete: true
Attachment #464537 - Flags: superreview?(vladimir)
Attachment #464537 - Flags: review?(bobbyholley+bmo)
Attachment #464529 - Flags: review?(bobbyholley+bmo)
(In reply to comment #28)
> diff --git a/modules/libpr0n/src/Image.h b/modules/libpr0n/src/Image.h
> +  // XXXdholbert this should probably be RasterImage-only.
> 
> As discussed on IRC, can you move these to RasterImage now?
Done. (All *SourceData* methods get moved to RasterImage in patch 9 now.)

> Can we stop calling them containers?
Tweaked the comment in patch 9, per comment 38.

> Looks great otherwise.
Thanks!

> this needs sr too.
Requesting sr?vlad.
Attachment #463801 - Attachment is obsolete: true
Attachment #464539 - Flags: superreview?(vladimir)
Attachment #464539 - Flags: review?(bobbyholley+bmo)
blocking2.0: ? → beta5+
Comment on attachment 464507 [details] [diff] [review]
patch 3 v2: rename imgDiscardTracker class

r=bholley
Attachment #464507 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 464508 [details] [diff] [review]
patch 4 v2: rename imgDiscardTracker files

r=bholley
Attachment #464508 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 464526 [details] [diff] [review]
patch 8 v2a: add imgIContainer::GetImageType

r=bholley
Attachment #464526 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 464537 [details] [diff] [review]
patch 9 v2a: Move methods from imgIContainer to RasterImage

r=bholley

You're on a roll.
Attachment #464537 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 464539 [details] [diff] [review]
patch 11 v2: move imagelib-private methods to Image

r=bholley

Hooray!
Attachment #464539 - Flags: review?(bobbyholley+bmo) → review+
Here's the patch I promised in comment 32. It does the following:
  - s/WriteToContainer/WriteToRasterImage/
  - s/container/image/ on some local variables
  - s/container/Image/ or /RasterImage/ on some comments
  - applies minor whitespace cleanup in contextual code
Attachment #464584 - Flags: review?(bobbyholley+bmo)
Comment on attachment 464584 [details] [diff] [review]
patch 12: s/container/image/ in WriteToContainer, vars, comments

woohoo! r=bholley
Attachment #464584 - Flags: review?(bobbyholley+bmo) → review+
Depends on: 582004
At bholley's request, here's a single convenience rollup patch that contains all this bug's patches (plus bug 582004's two patches, which this stacks on top of).
I discovered a bug in this patch stack over the past 4 hours of hair-tearing. ;-)

Basically, the build changes in decoders/icon/Makefile.in break icons in non-libxul builds. To reproduce, just navigate your browser (on mac for me) to your home directory, and notice that file icons aren't showing up properly.

What's happening is that the moz-icon:// protocol handler isn't getting properly registered when decoders/icon is forced static.

Specifically, we're failing here, because the Contract ID doesn't appear to be registered:
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsIOService.cpp#434

nsIconModule is a separate module for various linkage-related reasons related to the icon channel stuff, and I don't think it's a simple fix to change that.

The reason I discovered this is that I'm actually working on a separate patch stack to deCOM and clean up the image decoders (bug 513681). The first patch in the stack moves nsIconDecoder from decoders/icon to decoders/bmp, so that it can get built into the imagelib module and not the separate icon module. I couldn't for the life of me figure out why I was breaking protocol handling, until I realized that it was broken in the roll-up patch underneath ;-). One thing that made things trickier to diagnose is that some of the issues didn't show up without a full make -f client.mk. Ugh.

So anyway, I don't think there's a clear fix for the problem here as long as nsIconDecoder is in a separate module from the rest of imagelib. So I'd recommend including my nsIconDecoder-moving patch in your stack. ping me and we'll talk about it.
Depends on: 586566
(In reply to comment #48)
Ouch - thanks for figuring that out, bholley!

> So anyway, I don't think there's a clear fix for the problem here as long as
> nsIconDecoder is in a separate module from the rest of imagelib. So I'd
> recommend including my nsIconDecoder-moving patch in your stack.

I filed Bug 586566 on this, per IRC discussion.
Attachment #464526 - Flags: superreview?(vladimir) → superreview+
Attachment #464537 - Flags: superreview?(vladimir) → superreview+
Attachment #464539 - Flags: superreview?(vladimir) → superreview+
For the record: this push included some patch-tweaks to fix Windows build issues that I'd run into on TryServer:

- imgLoader.cpp was having build issues from including "windows.h".  This was happening because I removed the "#include imgContainer.h" from imgRequest.h, which is included by imgTools.h, which is included by imgTools.cpp. I fixed this in patch 2 by moving the "#undef LoadImage" from the .h file to the .cpp file, right after the #include imgContainer.h:
http://hg.mozilla.org/mozilla-central/diff/58bfbc65ae13/modules/libpr0n/src/imgLoader.h
http://hg.mozilla.org/mozilla-central/diff/58bfbc65ae13/modules/libpr0n/src/imgLoader.cpp

 - nsJPEGDecoder was having build issues with conflicting definitions of INT32.  The first definition is from some jpeg-specific header, and the second was from "windows.h" (which nsJPEGDecoder now includes, courtesy of RasterImage.h).  I fixed this in patch 9 by moving the RasterImage.h-include to nsJPEGDecoder.h before everything else, and undeffing INT32 right after that, so that we get the jpeg version of INT32.
http://hg.mozilla.org/mozilla-central/diff/205795b2962a/modules/libpr0n/decoders/jpeg/nsJPEGDecoder.h

 - Turns out widget/src/windows/nsWindowGfx.cpp was using the "imagelib-private" method imgIContainer::GetNumFrames, and then basically throwing away the result. I removed that chunk as part of patch 9 (since GetNumFrames is RasterImage-only now):
http://hg.mozilla.org/mozilla-central/diff/205795b2962a/widget/src/windows/nsWindowGfx.cpp
    20.1 --- a/modules/libpr0n/src/imgRequest.cpp
    20.2 +++ b/modules/libpr0n/src/imgRequest.cpp
    20.3 @@ -87,6 +87,7 @@
    20.4  
    20.5  #define DISCARD_PREF "image.mem.discardable"
    20.6  #define DECODEONDRAW_PREF "image.mem.decodeondraw"
    20.7 +#define SVG_MIMETYPE "image/svg+xml"

For future reference, we've got <http://mxr.mozilla.org/mozilla-central/source/netwerk/mime/nsMimeTypes.h> for this kind of define.
Duplicate of this bug: 584491
No longer depends on: 614392
Depends on: 614392
Duplicate of this bug: 503973
Depends on: 550686
No longer depends on: 550686
Depends on: 713889
You need to log in before you can comment on or make changes to this bug.