Closed Bug 815471 Opened 7 years ago Closed 7 years ago

Interact only with Images rather than directly with RasterImages or VectorImages

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(1 file, 5 obsolete files)

We want to start introducing new kinds of images - for example, images that represented transformed versions of existing images. In order to do that we need to avoid assuming anywhere in the code that an imgIContainer or Image object can always be safely cast to one of RasterImage or VectorImage.
Perhaps more subtle but equally important, we also need to move the responsibility for constructing specific types of images out of client code and into factory functions. This reduces the complexity of adding a new type of image and eliminates many of the dependencies we are concerned with here. Future patches (e.g. bug 790640 and bug 419588) will take advantage of this to add new features to imagelib.
Summary: Don't assume that all imgIContainers are either a RasterImage or a VectorImage → Interact only with imgIContainers rather than directly with RasterImages or VectorImages
Summary: Interact only with imgIContainers rather than directly with RasterImages or VectorImages → Interact only with Images rather than directly with RasterImages or VectorImages
This patch eliminates code that static_casts Image objects to RasterImage or
VectorImage (using different approaches on a case-by-case basis) and moves
responsibility for constructing Image objects out of imgRequest and into a
separate ImageFactory class, so that even imgRequest is unaware of the type of
Image it is dealing with. The only code which still deals with RasterImage or
VectorImage directly is imgTools::DecodeImageData. I didn't want to fix that
case in this bug, since a complete fix involves interface changes to publicly
exposed APIs.

Try job here: https://tbpl.mozilla.org/?tree=Try&rev=bf6b72d089fd
Attachment #686359 - Flags: review?(joe)
Blocks: 816362
Hmm, too many infrastructure failures affect that try job. I pushed another one. This one is also rebased to a newer version of mozilla-central:

https://tbpl.mozilla.org/?tree=Try&rev=fe03189b1915
Comment on attachment 686359 [details] [diff] [review]
Interact only with Images rather than directly with RasterImages or VectorImages

Review of attachment 686359 [details] [diff] [review]:
-----------------------------------------------------------------

This is all pretty great. In particular I'm really happy that imgRequest no longer needs to know which Image* to create.

::: image/src/ImageFactory.cpp
@@ +13,5 @@
> +#include "VectorImage.h"
> +
> +#include "ImageFactory.h"
> +
> +namespace mozilla { namespace image {

separate lines here

::: image/src/ImageFactory.h
@@ +11,5 @@
> +#include "imgStatusTracker.h"
> +
> +#include "Image.h"
> +
> +#define SVG_MIMETYPE "image/svg+xml"

I'm not super-enthusiastic about this being a #define in the global namespace. Is it possible for it to be static const char* in mozilla::image?

@@ +13,5 @@
> +#include "Image.h"
> +
> +#define SVG_MIMETYPE "image/svg+xml"
> +
> +namespace mozilla { namespace image {

separate lines please

@@ +27,5 @@
> +   * @param aURI           The URI of the image.
> +   * @param aIsMultiPart   Whether the image is part of a multipart request.
> +   * @param aInnerWindowId The window this image belongs to.
> +   */
> +  static already_AddRefed<Image> CreateContainer(nsIRequest* aRequest,

Can we call this CreateImage? We're trying to downplay the whole "imgIContainer" xpcom type.

::: image/src/RasterImage.h
@@ +159,2 @@
>    RasterImage(imgStatusTracker* aStatusTracker = nullptr);
> + 

delete spaces here please

@@ +253,5 @@
>     * value. Should this just return void?
>     */
>    nsresult AddSourceData(const char *aBuffer, uint32_t aCount);
>  
>    /* Called after the all the source data has been added with addSourceData. */

you should probably update these comments, and add one for OnImageDataAvailable

::: image/src/imgRequest.cpp
@@ +739,5 @@
>          imgStatusTracker* freshTracker = new imgStatusTracker(nullptr, this);
>          freshTracker->AdoptConsumers(&GetStatusTracker());
>          mStatusTracker = freshTracker;
> +
> +        mResniffMimeType = false;

Why did you move this inside the if? Did it fix a bug?

::: image/src/imgTools.cpp
@@ +58,5 @@
>    NS_ENSURE_ARG_POINTER(aInStr);
>  
> +  // XXX(seth) This needs to be switched over to use ImageFactory, but we need
> +  // to be sure that no callers actually provide an existing imgIContainer first.
> +

You should probably do this first.
Attachment #686359 - Flags: review?(joe) → review+
@Joe: Thanks for the review. I'll post an updated patch shortly.

> Why did you move this inside the if? Did it fix a bug?

It was required during an intermediate phase of the patch. I ended up changing the patch and as of now we could move it back out, but I felt that it made more sense to only set the flag to false if it was previously true, and that it made the code read a bit better. The behavior will be the same either way, of course. If you preferred it the other way I can change it back.

> You should probably do this first.

In some sense it is done, as a patch is already posted in bug 816362. I need to get an addon compatibility review for that. I'll request that now and get that bug ready to land also.
Updated with the requested changes.
Attachment #686359 - Attachment is obsolete: true
Trivial fix (added a missing null check).
Attachment #688020 - Attachment is obsolete: true
Trivial change: make some ImageFactory constructor arguments const.
Attachment #688978 - Attachment is obsolete: true
Trivial fixes (slightly reordering of statements in imgRequest::OnDataAvailable to fix an issue with a test_async_notification_404.js, removed some leftover debugging code).
Attachment #691991 - Attachment is obsolete: true
Try job looks good. Requesting checkin.
Keywords: checkin-needed
Sorry, but something in bug 815471, bug 821023, bug 816374, or bug 816362 was causing reftest failures on all platforms and Android mochitest-8 failures. See the TBPL link below. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/47bd1f6fd8ed

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=22f0a7ee5348
Attachment #692069 - Attachment is obsolete: true
Requesting checkin again. I'll do this one bug at a time this time instead of checking in my whole stack of imagelib refactorings at once.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b0c4f68563c3
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
(In reply to Joe Drew (:JOEDREW! \o/) from comment #4)
> ::: image/src/ImageFactory.h
> @@ +11,5 @@
> > +#include "imgStatusTracker.h"
> > +
> > +#include "Image.h"
> > +
> > +#define SVG_MIMETYPE "image/svg+xml"
> 
> I'm not super-enthusiastic about this being a #define in the global
> namespace. Is it possible for it to be static const char* in mozilla::image?

FWIW, we've already got http://mxr.mozilla.org/mozilla-central/source/netwerk/mime/nsMimeTypes.h#96 for this.
@Ms2ger, thanks for pointing that out. I'll file a separate bug to fix that.
Depends on: 820602
You need to log in before you can comment on or make changes to this bug.