Closed
Bug 584841
Opened 14 years ago
Closed 14 years ago
Create private mozilla::imagelib::Image API for imgIContainer implementations to inherit from
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | beta5+ |
People
(Reporter: dholbert, Assigned: dholbert)
References
Details
Attachments
(14 files, 8 obsolete files)
(deleted),
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bholley
:
review-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bholley
:
review+
vlad
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bholley
:
review+
vlad
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bholley
:
review+
vlad
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
(deleted),
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 | ||
Updated•14 years ago
|
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•14 years ago
|
||
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)
Assignee | ||
Comment 2•14 years ago
|
||
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)
Assignee | ||
Comment 3•14 years ago
|
||
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)
Assignee | ||
Comment 4•14 years ago
|
||
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)
Assignee | ||
Updated•14 years ago
|
Attachment #463790 -
Attachment description: patch 3: s/imgDiscardTracker/mozilla::imagelib::ImageDiscardTracker/ → patch 3: rename imgDiscardTracker class
Assignee | ||
Comment 5•14 years ago
|
||
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)
Assignee | ||
Comment 6•14 years ago
|
||
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)
Assignee | ||
Updated•14 years ago
|
Attachment #463794 -
Attachment is patch: true
Attachment #463794 -
Attachment mime type: application/octet-stream → text/plain
Assignee | ||
Comment 7•14 years ago
|
||
This patch does the "hg mv" for imgContainer.[h|cpp] --> RasterImage.[h|cpp], and updates #include lines as appropriate.
Assignee | ||
Updated•14 years ago
|
Attachment #463795 -
Flags: review?(bobbyholley+bmo)
Assignee | ||
Comment 8•14 years ago
|
||
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)
Assignee | ||
Comment 9•14 years ago
|
||
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.)
Assignee | ||
Updated•14 years ago
|
Attachment #463799 -
Flags: review?(bobbyholley+bmo)
Assignee | ||
Comment 10•14 years ago
|
||
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)
Assignee | ||
Comment 11•14 years ago
|
||
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)
Assignee | ||
Comment 12•14 years ago
|
||
... 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.
Assignee | ||
Comment 13•14 years ago
|
||
(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•14 years ago
|
||
(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.
Assignee | ||
Comment 15•14 years ago
|
||
(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•14 years ago
|
||
Comment on attachment 463788 [details] [diff] [review] patch 1: create mozilla::imagelib::Image r=bholley
Attachment #463788 -
Flags: review?(bobbyholley+bmo) → review+
Comment 17•14 years ago
|
||
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 18•14 years ago
|
||
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 19•14 years ago
|
||
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 20•14 years ago
|
||
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 21•14 years ago
|
||
Comment on attachment 463794 [details] [diff] [review] patch 6: fix whitespace after imgContainer rename r=bholley
Attachment #463794 -
Flags: review?(bobbyholley+bmo) → review+
Comment 22•14 years ago
|
||
Comment on attachment 463795 [details] [diff] [review] patch 7: rename imgContainer files r=bholley
Attachment #463795 -
Flags: review?(bobbyholley+bmo) → review+
Comment 23•14 years ago
|
||
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-
Comment 24•14 years ago
|
||
(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•14 years ago
|
||
(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•14 years ago
|
||
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 27•14 years ago
|
||
Comment on attachment 463800 [details] [diff] [review] patch 10: give imgStatusTracker an Image* r=bholley
Attachment #463800 -
Flags: review?(bobbyholley+bmo) → review+
Comment 28•14 years ago
|
||
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-
Assignee | ||
Updated•14 years ago
|
blocking2.0: --- → ?
Assignee | ||
Comment 29•14 years ago
|
||
(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+
Assignee | ||
Comment 30•14 years ago
|
||
(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)
Assignee | ||
Updated•14 years ago
|
Attachment #464507 -
Attachment description: patch 3: rename imgDiscardTracker class → patch 3 v2: rename imgDiscardTracker class
Assignee | ||
Comment 31•14 years ago
|
||
(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)
Assignee | ||
Updated•14 years ago
|
Attachment #463791 -
Attachment is obsolete: true
Assignee | ||
Comment 32•14 years ago
|
||
(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+
Assignee | ||
Updated•14 years ago
|
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]
Assignee | ||
Comment 33•14 years ago
|
||
(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)
Assignee | ||
Comment 34•14 years ago
|
||
(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)
Assignee | ||
Comment 35•14 years ago
|
||
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)
Assignee | ||
Comment 36•14 years ago
|
||
(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.
Assignee | ||
Comment 38•14 years ago
|
||
(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)
Assignee | ||
Comment 39•14 years ago
|
||
(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)
Updated•14 years ago
|
blocking2.0: ? → beta5+
Comment 40•14 years ago
|
||
Comment on attachment 464507 [details] [diff] [review] patch 3 v2: rename imgDiscardTracker class r=bholley
Attachment #464507 -
Flags: review?(bobbyholley+bmo) → review+
Comment 41•14 years ago
|
||
Comment on attachment 464508 [details] [diff] [review] patch 4 v2: rename imgDiscardTracker files r=bholley
Attachment #464508 -
Flags: review?(bobbyholley+bmo) → review+
Comment 42•14 years ago
|
||
Comment on attachment 464526 [details] [diff] [review] patch 8 v2a: add imgIContainer::GetImageType r=bholley
Attachment #464526 -
Flags: review?(bobbyholley+bmo) → review+
Comment 43•14 years ago
|
||
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 44•14 years ago
|
||
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+
Assignee | ||
Comment 45•14 years ago
|
||
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 46•14 years ago
|
||
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+
Assignee | ||
Comment 47•14 years ago
|
||
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•14 years ago
|
||
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.
Assignee | ||
Comment 49•14 years ago
|
||
(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+
Assignee | ||
Comment 50•14 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 51•14 years ago
|
||
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•14 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•