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

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Daniel Holbert [:dholbert]]

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)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: patch 1: create mozilla::imagelib::Image

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.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: patch 2: change some imgContainer pointers to be Image pointers

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

Comment 3

[Daniel Holbert [:dholbert]]

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

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: patch 4: rename imgDiscardTracker files

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.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: patch 5: rename imgContainer 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)

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: patch 6: fix whitespace after imgContainer rename

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.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: patch 7: rename imgContainer files

This patch does the "hg mv" for imgContainer.[h|cpp] -->  RasterImage.[h|cpp], and updates #include lines as appropriate.

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: patch 8: add imgIContainer::GetImageType

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.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: patch 9: Move methods from imgIContainer to RasterImage

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

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: patch 10: give imgStatusTracker an Image*

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.

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: patch 11: move imagelib-private methods to Image

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.

Comment 12

[Daniel Holbert [:dholbert]]

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

Comment 13

[Daniel Holbert [:dholbert]]

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

Comment 14

[Joe Drew (not getting mail)]

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

Comment 15

[Daniel Holbert [:dholbert]]

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

[Bobby Holley (:bholley)]

Comment on attachment 463788 [details] [diff] [review]
patch 1: create mozilla::imagelib::Image

r=bholley

Comment 17

[Bobby Holley (:bholley)]

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

Comment 18

[Bobby Holley (:bholley)]

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. ;-)

Comment 19

[Bobby Holley (:bholley)]

Comment on attachment 463791 [details] [diff] [review]
patch 4: rename imgDiscardTracker files

r- for the same reasons as patch 3.

Comment 20

[Bobby Holley (:bholley)]

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.

Comment 21

[Bobby Holley (:bholley)]

Comment on attachment 463794 [details] [diff] [review]
patch 6: fix whitespace after imgContainer rename

r=bholley

Comment 22

[Bobby Holley (:bholley)]

Comment on attachment 463795 [details] [diff] [review]
patch 7: rename imgContainer files

r=bholley

Comment 23

[Bobby Holley (:bholley)]

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.

Comment 24

[Bobby Holley (:bholley)]

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

Comment 25

[Bobby Holley (:bholley)]

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

[Bobby Holley (:bholley)]

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. ;-)

Comment 27

[Bobby Holley (:bholley)]

Comment on attachment 463800 [details] [diff] [review]
patch 10: give imgStatusTracker an Image*

r=bholley

Comment 28

[Bobby Holley (:bholley)]

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.

Comment 29

[Daniel Holbert [:dholbert]]

Attachment: patch 2 v2: change some imgContainer pointers to be Image pointers [r=bholley]

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

Comment 30

[Daniel Holbert [:dholbert]]

Attachment: patch 3 v2: rename imgDiscardTracker class

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

Comment 31

[Daniel Holbert [:dholbert]]

Attachment: patch 4 v2: rename imgDiscardTracker files

(In reply to comment #19)
> patch 4: rename imgDiscardTracker files
> r- for the same reasons as patch 3.

Applied s/ImageDiscardTracker/DiscardTracker/ here, too.

Comment 32

[Daniel Holbert [:dholbert]]

Attachment: patch 5 v2: rename imgContainer class [r=bholley]

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

Comment 33

[Daniel Holbert [:dholbert]]

Attachment: patch 8 v2: add imgIContainer::GetImageType

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

Comment 34

[Daniel Holbert [:dholbert]]

Attachment: patch 8 v2a: add imgIContainer::GetImageType

(reposting patch 8 with a tweak to header-comment for the C++ GetType accessor.)

Comment 35

[Daniel Holbert [:dholbert]]

Attachment: patch 9 v2: Move methods from imgIContainer to RasterImage

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

Comment 36

[Daniel Holbert [:dholbert]]

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

Comment 37

[Daniel Holbert [:dholbert]]

er, meant to say "... but bug 168048 bundled ..."

Comment 38

[Daniel Holbert [:dholbert]]

Attachment: patch 9 v2a: Move methods from imgIContainer to RasterImage

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

Comment 39

[Daniel Holbert [:dholbert]]

Attachment: patch 11 v2: move imagelib-private methods to Image

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

Comment 40

[Bobby Holley (:bholley)]

Comment on attachment 464507 [details] [diff] [review]
patch 3 v2: rename imgDiscardTracker class

r=bholley

Comment 41

[Bobby Holley (:bholley)]

Comment on attachment 464508 [details] [diff] [review]
patch 4 v2: rename imgDiscardTracker files

r=bholley

Comment 42

[Bobby Holley (:bholley)]

Comment on attachment 464526 [details] [diff] [review]
patch 8 v2a: add imgIContainer::GetImageType

r=bholley

Comment 43

[Bobby Holley (:bholley)]

Comment on attachment 464537 [details] [diff] [review]
patch 9 v2a: Move methods from imgIContainer to RasterImage

r=bholley

You're on a roll.

Comment 44

[Bobby Holley (:bholley)]

Comment on attachment 464539 [details] [diff] [review]
patch 11 v2: move imagelib-private methods to Image

r=bholley

Hooray!

Comment 45

[Daniel Holbert [:dholbert]]

Attachment: patch 12: s/container/image/ in WriteToContainer, vars, comments

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

Comment 46

[Bobby Holley (:bholley)]

Comment on attachment 464584 [details] [diff] [review]
patch 12: s/container/image/ in WriteToContainer, vars, comments

woohoo! r=bholley

Comment 47

[Daniel Holbert [:dholbert]]

Attachment: rollup patch, for convenience

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

Comment 48

[Bobby Holley (:bholley)]

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.

Comment 49

[Daniel Holbert [:dholbert]]

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

Comment 50

[Daniel Holbert [:dholbert]]

Landed:
part 1:  http://hg.mozilla.org/mozilla-central/rev/d67e09aff8e1
part 2:  http://hg.mozilla.org/mozilla-central/rev/58bfbc65ae13
part 3:  http://hg.mozilla.org/mozilla-central/rev/51ebadcb2916
part 4:  http://hg.mozilla.org/mozilla-central/rev/9ecc5f41e0aa
part 5:  http://hg.mozilla.org/mozilla-central/rev/0c6319c01688
part 6:  http://hg.mozilla.org/mozilla-central/rev/20adb37475a5
part 7:  http://hg.mozilla.org/mozilla-central/rev/1893ccde199e
part 8:  http://hg.mozilla.org/mozilla-central/rev/3d1f9740a61a
part 9:  http://hg.mozilla.org/mozilla-central/rev/205795b2962a
part 10: http://hg.mozilla.org/mozilla-central/rev/7695eea587d4
part 11: http://hg.mozilla.org/mozilla-central/rev/f99cedbbc5f6
part 12: http://hg.mozilla.org/mozilla-central/rev/85ed3b142c08

Comment 51

[Daniel Holbert [:dholbert]]

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

Comment 52

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

    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.