505385 - Refactor Imagelib notifications

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Joe Drew (not getting mail)]

Right now, imgIDecoderObserver has two use-cases: for the decoder telling the container about the status of the decode, and for the container telling the observers about the status of the decode. For this reason, it's sort of schizophrenic. We should split it into two (or more!) interfaces that are single-purpose and more useful.

Comment 1

[Bobby Holley (:bholley)]

Note to self - We should be returning errors in onStopRequest, not onStopDecode (grep for this bug number in the code for more details). Decoding errors should be reported to imgRequest in the internal interface so that it can get rid of the container and save memory (maybe cancel itself too?)

Comment 2

[Bobby Holley (:bholley)]

Here's a rough sketch of what I'm thinking the next gen public img observer will look like. Some goals:
* Get rid of unnecessary calls, combine things based on what consumers seem to be doing
* Make more explicit the distinction between the "load" series and the "decode" series, since they're different post bug 435296
* Make things line up better with the status flags a la http://mxr.mozilla.org/mozilla-central/source/modules/libpr0n/public/imgIRequest.idl#94
* get rid of the 2 separate interfaces (imgIContainerObserver and imgIDecoderObserver)

joe and roc, I'd like your opinions on this. Once we get the general stuff worked out, I'll post a full IDL spec for your perusal.

name: imgIObserver? How does this relate to our desire to rename imgIContainer to mozilla::Image?

//
// Load Events
//
onStartLoad // Called when the load starts
onSizeAvailable // Called when width/height are available (we parse these at load)
onStopLoad(nsresult aLoadStatus) // Called when the load stops. Gives the load status.

// Decode events

onStartDecode // Called when a decode starts
onFrameAvailable // Called when the first frame is available
onStopDecode(nsresult aDecodeStatus) // Called when a decode stops. Gives the decode status. This will probably be ignored by consumers, because currently we don't even report decode status (onStopDecode actually gives load status atm)

// Other
onDiscard // Called when frames are discarded. 
onUpdate // more general than frameChanged - Generic "invalidate" event (with a rect of course). Also replaces onDataAvailable


Some questions:

* Right now we pass aIsLastPart in OnStopRequest for the case of multipart requests. However, currently this information is bogus due to bug 339610, so consumers can't be doing anything useful with it. Do we want to leave it in in the hopes that a) we fix bug 339610 and b) somebody finds something to do with it? What's the use case?

* My gut feeling is that since discarding is consumer visible (we generate a new series of decode events on redecode), consumers should have the ability to know when discard happens. However, I can't really think of a use case at the moment. Thoughts?

Comment 3

[Joe Drew (not getting mail)]

Is "Load" the right name for downloading & a little bit of parsing?

I think that onDiscard is just going to end up being a do-nothing notification that everyone has to implement, so we should probably omit it; onStartDecode will imply that discard has happened.

Comment 4

[Bobby Holley (:bholley)]

(In reply to comment #3)
> Is "Load" the right name for downloading & a little bit of parsing?

I think it's a pretty fair one - the data's coming in, from either the network or the fs. I think it makes perfect sense minus the header-only stuff, and that stuff is minor and pretty transparent to consumers.

> 
> I think that onDiscard is just going to end up being a do-nothing notification
> that everyone has to implement, so we should probably omit it; onStartDecode
> will imply that discard has happened.

Ok - makes sense.

roc?

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #2)
> name: imgIObserver? How does this relate to our desire to rename imgIContainer
> to mozilla::Image?

imgIObserver and mozilla::ImageObserver both sound good.

> onStartLoad // Called when the load starts

Why do we need this?

> onSizeAvailable // Called when width/height are available (we parse these at
> load)
> onStopLoad(nsresult aLoadStatus) // Called when the load stops. Gives the load
> status.
> 
> // Decode events
> 
> onStartDecode // Called when a decode starts
> onFrameAvailable // Called when the first frame is available

onFirstFrameAvailable?

> onStopDecode(nsresult aDecodeStatus) // Called when a decode stops. Gives the
> decode status. This will probably be ignored by consumers, because currently
> we don't even report decode status (onStopDecode actually gives load status
> atm)

The only difference between onFirstFrameAvailable and onStopDecode is for animated images, right? I guess I can see notification of the end of asynchronous decoding of an animated image being useful.

> // Other
> onDiscard // Called when frames are discarded.

When do we discard frames? Why do consumers care?

> onUpdate // more general than frameChanged - Generic "invalidate" event (with
> a rect of course). Also replaces onDataAvailable

> * Right now we pass aIsLastPart in OnStopRequest for the case of multipart
> requests. However, currently this information is bogus due to bug 339610, so
> consumers can't be doing anything useful with it. Do we want to leave it in in
> the hopes that a) we fix bug 339610 and b) somebody finds something to do with
> it? What's the use case?

Don't know

> * My gut feeling is that since discarding is consumer visible (we generate a
> new series of decode events on redecode), consumers should have the ability to
> know when discard happens. However, I can't really think of a use case at the
> moment. Thoughts?

Doesn't seem necessary. Someone who wants to capture all the frames of an image (do we ever do that?) can just capture the frame as each frame is done ... but I guess we don't have a notification that each frame is done?

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> Someone who wants to capture all the frames of an image (do we ever do that?)

Actually, trying to do this doesn't make sense IMHO. You can't do it for SVG for example.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> onStopLoad(nsresult aLoadStatus) // Called when the load stops. Gives the load
> status.

I think we should get rid of this as discussed on IRC. Instead onStopDecode should give the error. Consumers don't really care what happened with the data going into imglib, they only care about decoded image data.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm not sure why we need onStartDecode either.

Comment 9

[Bobby Holley (:bholley)]

(In reply to comment #8)
> I'm not sure why we need onStartDecode either.

for one, nsImageLoadingContent needs it in bug 512435

Comment 10

[Bobby Holley (:bholley)]

also note bug 521604.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

I wonder if it's worth having the decoding and the loading events on separate interfaces. It seems like they are unrelated concerns.

Comment 12

[Bobby Holley (:bholley)]

I'm working on this again.

Comment 13

[Daniel Holbert [:dholbert]]

Per bug 737778 comment 2, it'd be great if we could change our assumption that "OnStopRequest() means we can immediately set LOAD_COMPLETE [which tells our observers that we know our width/height]", because SVG images can only hold up to that bargain if they force-feed their parser instead of letting it parse asynchronously.  It'd be much better (for SVG images at least) if we could set LOAD_COMPLETE asynchronously, after the parser has finished (or at least gotten as far as <svg width...height...>) on its own schedule.

It sounds like that assumption-revisiting might happen in a followup bug rather than as part of this re-architecting, but I'm noting it here because bholley told me to. :)

Comment 14

[Jet Villegas (inactive)]

Re-assigning per :jdm , :bholley , & :jst 

The bug title should probably be updated to reflect both imgIContainerObserver & imgIDecoderObserver

Comment 15

[Bobby Holley (:bholley)]

(In reply to Jet Villegas (:jet) from comment #14)
> The bug title should probably be updated to reflect both
> imgIContainerObserver & imgIDecoderObserver

Oh, it's _much_ bigger than that. ;-)

Comment 16

[Josh Matthews [:jdm]]

For the record, my patch queue is being stored at http://hg.mozilla.org/users/josh_joshmatthews.net/imagelib-refactor/. I'm working on getting the various imagelib ownership refactors and new notification system to pass all tests before I start figuring out how to refine the notifications and fix the onStopRequest/onStopDecode madness.

Comment 17

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/?tree=Try&rev=396b310d50e7 shows my patch series as being green. At least point, I have built on Bobby's original patches to:
* make imgStatusTracker contain more state
* eliminate the use of imgIDecoder/ContainerObserver outside of image
* create the new imgINotificationObserver to replace them
* fix a problem where replacing a pending unstarted request in nsImageLoadingContent missed out on OnStartRequest in the cancelled request
* fix the OnStopDecode/OnStopRequest weirdness - OnStopDecode is now a decode notification, and OnStopRequest is a load notification, and both cause the image status to be set to STATUS_ERROR if either stage fails

Comment 18

[Josh Matthews [:jdm]]

Attachment: Remove the last vestiges of manual animation reset within imagelib. stuff should now all be taken care of by the AnimationConsumers infrastructure.

Comment 19

[Josh Matthews [:jdm]]

Attachment: Hoist proxy list into imgStatusTracker.

Comment 20

[Josh Matthews [:jdm]]

Attachment: Move notification dispatch into imgStatusTracker.

Comment 21

[Josh Matthews [:jdm]]

Attachment: Fix some warnings.

Comment 22

[Josh Matthews [:jdm]]

Attachment: Expose pointers to the imgRequest and Image on imgStatusTracker. will help us decouple imgRequest and imgRequestProxy.

Comment 23

[Josh Matthews [:jdm]]

Attachment: Expose imgRequest::GetStatusTracker, and ensure that it's never null. slowly turning imgStatusTracker into the central coordination point between imgRequest,

imgRequestProxy, and Image, so we need it to be a bit more visible.

Comment 24

[Josh Matthews [:jdm]]

Attachment: Initialize imgRequestProxy with a status tracker. is sort of symbolic for now, since we keep mOwner and mImage and just

pull them off the tracker, but we'll soon change that. ;-)

Comment 25

[Josh Matthews [:jdm]]

Attachment: Remove image reference from proxies, and delegate operations on images to the owning request or the status tracker.

Comment 26

[Josh Matthews [:jdm]]

Attachment: Hoist decoder and container observer out of imgRequest into imgStatusTracker.

Comment 27

[Josh Matthews [:jdm]]

Attachment: Remove GetConsumers from imgStatusTracker by delegating all remaining notification operations from imgRequest to the status tracker.

Comment 28

[Josh Matthews [:jdm]]

Attachment: Create a new imgINotificationObserver interface to replace all uses of imgIContainerObserver and imgIDecoderObserver outside of image/.

Comment 29

[Josh Matthews [:jdm]]

Attachment: Fix tests broken by interface changes, and ensure clones of static image proxies result in more static proxies.

Comment 30

[Josh Matthews [:jdm]]

Attachment: Remove OnStopContainer and make OnStopDecode a true decode notification.

Comment 31

[Josh Matthews [:jdm]]

Attachment: Update image request status on decoding errors.

Comment 32

[Josh Matthews [:jdm]]

Attachment: Fix cases where pending requests in nsImageLoadingContent could be cancelled and never receive OnStartRequest.

Comment 33

[Josh Matthews [:jdm]]

Attachment: Remove nsStubImageDecoderObserver.

Comment 34

[Josh Matthews [:jdm]]

Attachment: Clean up unused arguments in imgStatusTracker observers, document unclear things, etc.

Comment 35

[Josh Matthews [:jdm]]

Attachment: Cycle collect the scripted observer to avoid leaks.

Comment 36

[Josh Matthews [:jdm]]

Ack. I promise I'll add numbers to patches in the future, but these are currently in order.

Comment 37

[Joe Drew (not getting mail)]

mother of god

Comment 38

[Bobby Holley (:bholley)]

jdm++

Comment 39

[Josh Matthews [:jdm]]

Bobby, I would be interested in your thoughts about the refactoring. I don't think I completely fulfilled your vision of how much responsibility would ultimately be moved into imgStatusTracker, since some fields like mOwner ended up being pretty key to the work.

Comment 40

[Bobby Holley (:bholley)]

(In reply to Josh Matthews [:jdm] from comment #39)
> Bobby, I would be interested in your thoughts about the refactoring. I don't
> think I completely fulfilled your vision of how much responsibility would
> ultimately be moved into imgStatusTracker, since some fields like mOwner
> ended up being pretty key to the work.

That sounds fine - it wasn't an immutable vision. And I don't really have the cycles to dive into this patch stack right now. We could talk about it when we see each other on monday though?

Comment 41

[Josh Matthews [:jdm]]

That is an eminently reasonable idea.

Comment 42

[Josh Matthews [:jdm]]

Ok, let's start off with getting the internal imagelib changes right before we move on to the external interface. Here's a high-level summary of the important stuff:

imgStatusTracker now
* owns the proxy list, and performs all notification duties upon said list
* controls all blockOnLoad logic
* is the decoder observer

imgRequest
* owns the principal and controls access to it (except in case of a static imgRequestProxy, which duplicates it)
* owns the image image object and controls all accesses to it (ie. no more image in imgRequestProxy), except in the case of the static imgRequestProxy like usual

Basically, imgRequestProxy now does more delegation to the imgRequest, and imgRequest now does more delegation to imgStatusTracker, and imgStatusTracker is in charge of things that are sensible for it to be in charge of.

Comment 43

[Joe Drew (not getting mail)]

Comment on attachment 652926 [details] [diff] [review]
Initialize imgRequestProxy with a status tracker.  is sort of symbolic for now, since we keep mOwner and mImage and just

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

If all imgRequestProxys should have status trackers, perhaps we could MOZ_ASSERT or ABORT_IF_FALSE on aStatusTracker != nullptr. It wouldn't surprise me if there were cases where it needs to be null though.

Comment 44

[Joe Drew (not getting mail)]

Comment on attachment 652921 [details] [diff] [review]
Hoist proxy list into imgStatusTracker.

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

::: image/src/imgRequest.h
@@ -21,5 @@
>  
>  #include "nsCategoryCache.h"
>  #include "nsCOMPtr.h"
>  #include "nsString.h"
> -#include "nsTObserverArray.h"

You didn't remove imgRequest::mObservers though?

::: image/src/imgStatusTracker.cpp
@@ +39,5 @@
>    : mImage(aOther.mImage),
>      mState(aOther.mState),
>      mImageStatus(aOther.mImageStatus),
>      mHadLastPart(aOther.mHadLastPart)
> +    //    mConsumers(aOther.mConsumers)

Can you explain this part, or remove it? :)

@@ +286,5 @@
> +imgStatusTracker::RemoveConsumer(imgRequestProxy* aConsumer, nsresult aStatus,
> +                                 bool aOnlySendStopRequest)
> +{
> +  // Remove the proxy from the list.
> +  /*mozilla::DebugOnly<*/bool/*>*/ removed = mConsumers.RemoveElement(aConsumer);

aahhh good lord

Comment 45

[Joe Drew (not getting mail)]

Comment on attachment 652922 [details] [diff] [review]
Move notification dispatch into imgStatusTracker.

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

I like your solution to tracking things for static images, imgRequestProxyStatic!

::: image/src/imgRequestProxy.h
@@ +221,5 @@
>    // get multiple OnStartContainer calls.
>    bool mSentStartContainer;
>  };
>  
> +class imgRequestProxyStatic : public imgRequestProxy {

{ on new line

Also: A comment about when imgRequestProxyStatic is used would help.

Finally, it might be better to separate its implementation from imgRequestProxy, rather than having them intermingled. I'd accept just implementing GetImagePrincipal() in this .h file.

Comment 46

[Joe Drew (not getting mail)]

Comment on attachment 652923 [details] [diff] [review]
Fix some warnings.

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

::: image/decoders/nsICODecoder.cpp
@@ +463,2 @@
>                           static_cast<nsBMPDecoder*>(mContainedDecoder.get())->GetCompressedImageSize() +
>                           4 * numColors;

Can you reindent these two lines?

Comment 47

[Joe Drew (not getting mail)]

Comment on attachment 652927 [details] [diff] [review]
Remove image reference from proxies, and delegate operations on images to the owning request or the status tracker.

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

::: image/src/imgRequest.cpp
@@ +764,5 @@
>        // mImage won't be reusable due to format change or a new SVG part
>        // Reset the tracker and forget that we have data for OnDataAvailable to
>        // treat its next call as a fresh image.
>        imgStatusTracker* freshTracker = new imgStatusTracker(nullptr, this);
> +      freshTracker->AdoptConsumers(&GetStatusTracker());

Oh, for return type polymorphism in C++...

::: image/src/imgRequestProxy.cpp
@@ +840,5 @@
>    nsCOMPtr<imgIContainer> currentFrame;
> +  nsresult rv = image->ExtractFrame(
> +      imgIContainer::FRAME_CURRENT, rect,
> +      imgIContainer::FLAG_SYNC_DECODE,
> +      getter_AddRefs(currentFrame));

Can you put these arguments back after the ( of the function call, and vertically aligned, etc?

::: image/src/imgRequestProxy.h
@@ +93,5 @@
>      mDeferNotifications = aDeferNotifications;
>    }
>  
> +  // XXXbholley - This eventually gets folded into the new notification API.
> +  void HaveImage();

As long as this function name doesn't persist, I'm OK with it. :)

(Otherwise, "SetHasImage")

Comment 48

[Joe Drew (not getting mail)]

Comment on attachment 652928 [details] [diff] [review]
Hoist decoder and container observer out of imgRequest into imgStatusTracker.

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

::: image/src/imgRequestProxy.cpp
@@ +909,5 @@
>    // That's why this method uses mOwner->GetStatusTracker() instead of just
>    // mOwner->mStatusTracker -- we might have a null mImage and yet have an
>    // mOwner with a non-null mImage (and a null mStatusTracker pointer).
> +  //XXX this will break if used in the static proxy; is that a problem?
> +  return mOwner->GetStatusTracker();

In general, yes, this is a problem. I sort of expect printing (or at least print preview on Linux) to immediately crash because of this, actually...

Comment 49

[Joe Drew (not getting mail)]

(In reply to Joe Drew (:JOEDREW! \o/) from comment #48)
> Comment on attachment 652928 [details] [diff] [review]
> Hoist decoder and container observer out of imgRequest into imgStatusTracker.
> 
> Review of attachment 652928 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/src/imgRequestProxy.cpp
> @@ +909,5 @@
> >    // That's why this method uses mOwner->GetStatusTracker() instead of just
> >    // mOwner->mStatusTracker -- we might have a null mImage and yet have an
> >    // mOwner with a non-null mImage (and a null mStatusTracker pointer).
> > +  //XXX this will break if used in the static proxy; is that a problem?
> > +  return mOwner->GetStatusTracker();
> 
> In general, yes, this is a problem. I sort of expect printing (or at least
> print preview on Linux) to immediately crash because of this, actually...

naturally, you fix this in the next patch. Can you move the hunk from the previous patch to the next? If so, I retract my r-.

Comment 50

[Joe Drew (not getting mail)]

Comment on attachment 652929 [details] [diff] [review]
Remove GetConsumers from imgStatusTracker by delegating all remaining notification operations from imgRequest to the status tracker.

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

::: image/src/imgRequestProxy.cpp
@@ +915,5 @@
> +imgStatusTracker&
> +imgRequestProxyStatic::GetStatusTracker() const
> +{
> +  return mImage->GetStatusTracker();
> +}

this is the hunk that I want moved

::: image/src/imgRequestProxy.h
@@ +167,5 @@
>    // Return the imgStatusTracker associated with mOwner and/or mImage. It may
>    // live either on mOwner or mImage, depending on whether
>    //   (a) we have an mOwner at all
>    //   (b) whether mOwner has instantiated its image yet
> +  virtual imgStatusTracker& GetStatusTracker() const;

along with this

@@ +233,5 @@
>      mOwnerHasImage = true;
>    };
>  
>    NS_IMETHOD GetImagePrincipal(nsIPrincipal** aPrincipal);
> +  imgStatusTracker& GetStatusTracker() const MOZ_OVERRIDE;

and this

Comment 51

[Josh Matthews [:jdm]]

Comment on attachment 652930 [details] [diff] [review]
Create a new imgINotificationObserver interface to replace all uses of imgIContainerObserver and imgIDecoderObserver outside of image/.

It's review Tuesday! That means you can't go home until you review eight patches!

Comment 52

[Joe Drew (not getting mail)]

Comment on attachment 652930 [details] [diff] [review]
Create a new imgINotificationObserver interface to replace all uses of imgIContainerObserver and imgIDecoderObserver outside of image/.

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

I love all the removed lines in this patch :)

::: content/svg/content/src/nsSVGFilters.cpp
@@ +5765,5 @@
> +    aRequest->GetImage(getter_AddRefs(container));
> +    NS_ABORT_IF_FALSE(container, "who sent the notification then?");
> +    container->RequestDecode();
> +  }
> +  

whitespace

@@ +5769,5 @@
> +  
> +  if (aType == imgINotificationObserver::STOP_DECODE ||
> +      aType == imgINotificationObserver::FRAME_CHANGED ||
> +      aType == imgINotificationObserver::START_CONTAINER)
> +    Invalidate();

{}

::: image/public/imgIRequest.idl
@@ +15,5 @@
>  /**
>   * imgIRequest interface
>   *
>   * @author Stuart Parmenter <[e-mail]>
>   * @version 0.1

Change UUID

::: image/public/imgITools.idl
@@ +13,2 @@
>  
>  [scriptable, uuid(8e16f39e-7012-46bd-aa22-2a7a3265608f)]

change uuid

@@ +101,5 @@
>                                        in long aHeight,
>                                        [optional] in AString outputOptions);
> +
> +    imgINotificationObserver createScriptedObserver(
> +        in imgIScriptedNotificationObserver aObserver);

Just put this on a single line.

::: image/src/ScriptedNotificationObserver.h
@@ +19,5 @@
> +  virtual ~ScriptedNotificationObserver() {}
> +
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_IMGINOTIFICATIONOBSERVER
> +  

whitespace

::: layout/generic/nsBulletFrame.cpp
@@ +1439,4 @@
>  
> +    nsCOMPtr<imgIContainer> image;
> +    aRequest->GetImage(getter_AddRefs(image));
> +    

whitespace

@@ +1497,5 @@
> +    // animated.
> +    if (aRequest == mImageRequest) {
> +      nsLayoutUtils::RegisterImageRequest(PresContext(), mImageRequest,
> +                                          &mRequestRegistered);
> +    }    

whitespace

@@ +1503,5 @@
>  
> +  if (aType == imgINotificationObserver::FRAME_CHANGED) {
> +    // Invalidate the entire content area. Maybe it's not optimal but it's simple and
> +    // always correct.
> +    Invalidate(nsRect(0, 0, mRect.width, mRect.height));    

whitespace

@@ +1622,4 @@
>  {
>    if (!mFrame)
>      return NS_ERROR_FAILURE;
> +  return mFrame->Notify(aRequest, aType, aData);

oh I quite like this.

::: layout/generic/nsImageFrame.cpp
@@ +543,5 @@
> +    if (IsPendingLoad(aRequest)) {
> +      // We don't care
> +      return NS_OK;
> +    }
> +  

whitespace

@@ +550,5 @@
>  
> +    if (intrinsicSizeChanged && (mState & IMAGE_GOTINITIALREFLOW)) {
> +      // Now we need to reflow if we have an unconstrained size and have
> +      // already gotten the initial reflow
> +      if (!(mState & IMAGE_SIZECONSTRAINED)) { 

whitespace

@@ +553,5 @@
> +      // already gotten the initial reflow
> +      if (!(mState & IMAGE_SIZECONSTRAINED)) { 
> +        nsIPresShell *presShell = presContext->GetPresShell();
> +        NS_ASSERTION(presShell, "No PresShell.");
> +        if (presShell) { 

whitespace

@@ +572,5 @@
> +    if (!(mState & IMAGE_GOTINITIALREFLOW)) {
> +      // Don't bother to do anything; we have a reflow coming up!
> +      return NS_OK;
> +    }
> +  

whitespace

@@ +582,5 @@
> +    // Don't invalidate if the current visible frame isn't the one the data is
> +    // from
> +    //XXXjdm This code might be testing for a situation that never occurs
> +    /*if (!aCurrentFrame)
> +      return NS_OK;*/

There is no aCurrentFrame, so you can just delete this code.

@@ +596,5 @@
> +           rect->x, rect->y, rect->width, rect->height,
> +           r.x, r.y, r.width, r.height);
> +#endif
> +
> +    Invalidate(r);    

whitespace

@@ +1943,5 @@
> +    nsImageFrame *frame;
> +    while (iter.HasMore()) {
> +      frame = iter.GetNext();
> +      frame->Invalidate(frame->GetRect());
> +    }    

whitespace

::: layout/svg/base/src/nsSVGImageFrame.cpp
@@ +36,5 @@
>                            imgIContainer *aContainer,
>                            const nsIntRect *aDirtyRect);
>    // imgIContainerObserver (override nsStubImageDecoderObserver)
>    NS_IMETHOD OnStartContainer(imgIRequest *aRequest,
> +  imgIContainer *aContainer);*/

you can just delete this code

::: layout/xul/base/src/nsImageBoxFrame.cpp
@@ +566,5 @@
> +    PRUint32 status;
> +    aRequest->GetImageStatus(&status);
> +    if (!(status & imgIRequest::STATUS_ERROR)) {
> +      // Fire an onload DOM event.
> +      FireImageDOMEvent(mContent, NS_LOAD);      

whitespace

@@ +584,5 @@
> +  }
> +
> +  if (aType == imgINotificationObserver::FRAME_CHANGED) {
> +    nsBoxLayoutState state(PresContext());
> +    this->Redraw(state);    

whitespace

::: toolkit/system/gnome/nsAlertsIconListener.cpp
@@ +76,5 @@
>  
> +    if (mIconRequest) {
> +      mIconRequest->Cancel(NS_BINDING_ABORTED);
> +      mIconRequest = nullptr;
> +    }    

whitespace

::: widget/cocoa/nsMenuItemIconX.mm
@@ +350,5 @@
> +
> +    PRInt32 origWidth = 0, origHeight = 0;
> +    imageContainer->GetWidth(&origWidth);
> +    imageContainer->GetHeight(&origHeight);
> +  

whitespace

@@ +363,5 @@
> +
> +    if (mImageRegionRect.IsEmpty()) {
> +      mImageRegionRect.SetRect(0, 0, origWidth, origHeight);
> +    }
> +  

whitespace

@@ +371,5 @@
> +                                             getter_AddRefs(frame));
> +    if (NS_FAILED(rv) || !frame) {
> +      [mNativeMenuItem setImage:nil];
> +      return NS_ERROR_FAILURE;
> +    }      

whitespace

@@ +381,5 @@
> +    }
> +
> +    bool createSubImage = !(mImageRegionRect.x == 0 && mImageRegionRect.y == 0 &&
> +                            mImageRegionRect.width == origWidth && mImageRegionRect.height == origHeight);
> +  

whitespace

@@ +387,5 @@
> +    if (createSubImage) {
> +      // if mImageRegionRect is set using CSS, we need to slice a piece out of the overall 
> +      // image to use as the icon
> +      finalImage = ::CGImageCreateWithImageInRect(origImage, 
> +                                                  ::CGRectMake(mImageRegionRect.x, 

whitespace

@@ +394,5 @@
> +                                                               mImageRegionRect.height));
> +      ::CGImageRelease(origImage);
> +      if (!finalImage) {
> +        [mNativeMenuItem setImage:nil];
> +        return NS_ERROR_FAILURE;  

whitespace

@@ +420,5 @@
> +    }
> +    CGRect iconRect = ::CGRectMake(0, 0, kIconWidth, kIconHeight);
> +    ::CGContextClearRect(bitmapContext, iconRect);
> +    ::CGContextDrawImage(bitmapContext, iconRect, finalImage);
> +  

whitespace

@@ +426,5 @@
> +
> +    ::CGImageRelease(finalImage);
> +    ::CGContextRelease(bitmapContext);
> +    free(bitmap);
> + 

whitespace

@@ +431,5 @@
> +    if (!iconImage) return NS_ERROR_FAILURE;
> +
> +    NSImage *newImage = nil;
> +    rv = nsCocoaUtils::CreateNSImageFromCGImage(iconImage, &newImage);
> +    if (NS_FAILED(rv) || !newImage) {    

whitespace

@@ +438,5 @@
> +      return NS_ERROR_FAILURE;
> +    }
> +
> +    [mNativeMenuItem setImage:newImage];
> +  

whitespace

Comment 53

[Josh Matthews [:jdm]]

Note to self: remove the START_DECODE notification, because it doesn't appear to have any consumers.

Comment 54

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 652938 [details] [diff] [review]
Cycle collect the scripted observer to avoid leaks.

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

This should be moved into the patch that creates the leaks.

Comment 55

[Joe Drew (not getting mail)]

Comment on attachment 652931 [details] [diff] [review]
Fix tests broken by interface changes, and ensure clones of static image proxies result in more static proxies.

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

::: image/src/imgRequestProxy.cpp
@@ +511,5 @@
>    // XXXldb That's not true anymore.  Stuff from imgLoader adds the
>    // request to the loadgroup.
>    clone->SetLoadFlags(mLoadFlags);
> +  //imgStatusTracker* tracker = aAllocFn == NewProxy ? &GetStatusTracker() :
> +  //    &static_cast<imgRequestProxyStatic*>(clone.get())->GetStatusTracker();

Can you remove this commented-out code?

Comment 56

[Joe Drew (not getting mail)]

Comment on attachment 652932 [details] [diff] [review]
Remove OnStopContainer and  make OnStopDecode a true decode notification.

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

::: image/public/imgIDecoderObserver.idl
@@ +112,5 @@
>     * a companion to onStopDecode to signal success or failure. This will be
>     * revisited in bug 505385. If you're thinking of doing something new with
>     * this, please talk to bholley first.
>     */
> +  void onStopDecode(in imgIRequest aRequest, in nsresult status);

Change the UUID

::: image/public/imgINotificationObserver.idl
@@ -21,5 @@
>    const long START_CONTAINER = 2;
>    const long START_FRAME = 3;
>    const long DATA_AVAILABLE = 4;
>    const long STOP_FRAME = 5;
> -  const long STOP_CONTAINER = 6;

Change the UUID

::: image/public/imgIScriptedNotificationObserver.idl
@@ -16,5 @@
>    void startFrame(in imgIRequest aRequest);
>    void startDecode(in imgIRequest aRequest);
>    void dataAvailable(in imgIRequest aRequest);
>    void stopFrame(in imgIRequest aRequest);
> -  void stopContainer(in imgIRequest aRequest);

Change the UUID

Comment 57

[Joe Drew (not getting mail)]

Comment on attachment 652937 [details] [diff] [review]
Clean up unused arguments in imgStatusTracker observers, document unclear things, etc.

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

::: image/public/imgIContainerObserver.idl
@@ +21,5 @@
>   */
>  [scriptable, uuid(f01efdb3-4b20-4313-a636-a2aa01a4ef5d)]
>  interface imgIContainerObserver : nsISupports
>  {
> +  [noscript] void frameChanged([const] in nsIntRect aDirtyRect);

uuid

::: image/public/imgIDecoderObserver.idl
@@ +52,5 @@
>     * called at the same time that nsIRequestObserver::onStartRequest would be
>     * (used only for observers of imgIRequest objects, which are nsIRequests,
>     * not imgIDecoder objects)
>     */
> +  void onStartRequest();

uuid

Comment 58

[Josh Matthews [:jdm]]

Attachment: Part 19: Reduce number of notifications - START_REQUEST/START_CONTAINER -> SIZE_AVAILABLE, remove START_DECODE and START_FRAME. Rename remaining ones for clarity.

Here we go - this is green on try, but I'm not really sure if I'm breaking some invariants by eliminating START_REQUEST and not firing any notifications until the renamed SIZE_AVAILABLE occurs. This means that any consumers viewing network failures will see LOAD_COMPLETE (formerly STOP_REQUEST) and nothing more; however, no consumers were watching for any kind of start notification so this didn't seem to be a problem.

Comment 59

[Josh Matthews [:jdm]]

Oh right, I also merged DATA_AVAILABLE/FRAME_CHANGED into FRAME_UPDATE, and there was one consumer that worried me, that being nsImageFrame. Please consider the OnDataAvailable and FrameChanged methods that currently exist and let me know if there is a sensible way to not break something I don't understand.

Comment 60

[Joe Drew (not getting mail)]

Comment on attachment 669710 [details] [diff] [review]
Part 19: Reduce number of notifications - START_REQUEST/START_CONTAINER -> SIZE_AVAILABLE, remove START_DECODE and START_FRAME. Rename remaining ones for clarity.

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

::: image/public/imgINotificationObserver.idl
@@ +13,5 @@
>  %}
>  
>  [ptr] native nsIntRect(nsIntRect);
>  
>  [scriptable, builtinclass, uuid(bf9ed307-02a5-4732-b3eb-659bde5de84f)]

may as well change this uuid too

::: layout/generic/nsImageFrame.cpp
@@ -549,5 @@
> -  if (aType == imgINotificationObserver::FRAME_CHANGED) {
> -    nsCOMPtr<imgIContainer> image;
> -    aRequest->GetImage(getter_AddRefs(image));
> -    return FrameChanged(aRequest, image);
> -  }

So when do we call FrameChanged now? Do we at all?

Comment 61

[Josh Matthews [:jdm]]

Matt, Joe suggested you might be able to answer this question: nsImageFrame currently calls InvalidateFrame/InvalidateFrameWithRect while the first frame of an image is loading, and InvalidLayer for any subsequent frames. My patch here currently ignores the second bit and continues calling the first two methods for all frames of an animation. Is this something with performance implications? Should I replicate the old behaviour?

Comment 62

[Robert Lickenbrock [:rclick]]

I would think that rect-based invalidation would be preferable because animation frames don't always change the entire image, so minding the rect imagelib outputs  may prevent over-invalidation. I'm not sure about the performance implications, though.

Comment 63

[Matt Woodrow (:mattwoodrow)]

The current behaviour would be preferable. Sometimes we put an animated image into its own layer, and calling InvalidateLayer lets us update the image without hitting display-list and layer construction.

Passing the invalid rect into InvalidateLayer would be better again, but it looks like that isn't implemented yet.

Comment 64

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/?tree=Try&rev=124cddb0bd9d

Lay on, Macduff, and damn'd be him that first cries, "Hold, enough!"

Comment 65

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/?tree=Try&rev=5c05c7521f59 for the broken OS X push on the first one. Between the two of them, the results are looking exceedingly green.

Comment 66

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dab0a2f7a5ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/2440e35a985f
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a20e6883f78
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c2493ac96c7
https://hg.mozilla.org/integration/mozilla-inbound/rev/09090d1ae8a0
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bcf64f22658
https://hg.mozilla.org/integration/mozilla-inbound/rev/eba7368e4128
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c750723535f
https://hg.mozilla.org/integration/mozilla-inbound/rev/1544cc1b035d
https://hg.mozilla.org/integration/mozilla-inbound/rev/0645314aa7c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/307acd23def9
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d223dd76a62
https://hg.mozilla.org/integration/mozilla-inbound/rev/3cd2146a9d6e
https://hg.mozilla.org/integration/mozilla-inbound/rev/64a67cc6691b
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f3390a9eec7
https://hg.mozilla.org/integration/mozilla-inbound/rev/84629d3f4c74
https://hg.mozilla.org/integration/mozilla-inbound/rev/9449ddf1199e
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4ec385f733b
https://hg.mozilla.org/integration/mozilla-inbound/rev/d35c748b03b0

Comment 67

[Josh Matthews [:jdm]]

Well, that didn't last long!

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2adc0ce03dba
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ea7bf4553d66
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cb6023c5b636

/me plots revenge on khuey.

Comment 68

[Ed Morley [:emorley]]

(In reply to Josh Matthews [:jdm] from comment #67)
> Well, that didn't last long!
> 
> remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2adc0ce03dba
> remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ea7bf4553d66
> remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cb6023c5b636
> 
> /me plots revenge on khuey.

Please can you use the backout script (/mercurial extension) next time - the wrong bug number in this backout meant I had to manually correct every changeset in comment 66, in m-cMerge before I could submit the bug changes (and obviously makes hg annotate confusing).

http://blog.bonardo.net/2011/11/22/even-easier-backouts-from-the-trees

:-)

Comment 69

[Josh Matthews [:jdm]]

Trying again: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=78f0949318a5

Comment 70

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5953ad4a6951
https://hg.mozilla.org/mozilla-central/rev/d23ba35302e6
https://hg.mozilla.org/mozilla-central/rev/6d585c429449
https://hg.mozilla.org/mozilla-central/rev/f11329484155
https://hg.mozilla.org/mozilla-central/rev/3dce664cb663
https://hg.mozilla.org/mozilla-central/rev/a8c623232db4
https://hg.mozilla.org/mozilla-central/rev/ea55032b7fd9
https://hg.mozilla.org/mozilla-central/rev/8b08ff14b246
https://hg.mozilla.org/mozilla-central/rev/c401bda70ef9
https://hg.mozilla.org/mozilla-central/rev/423503422b51
https://hg.mozilla.org/mozilla-central/rev/a913c8c0de54
https://hg.mozilla.org/mozilla-central/rev/3187167bb0c9
https://hg.mozilla.org/mozilla-central/rev/163205db950d
https://hg.mozilla.org/mozilla-central/rev/92f3a3fd032f
https://hg.mozilla.org/mozilla-central/rev/395f1c3e4bc3
https://hg.mozilla.org/mozilla-central/rev/353b9b430a96
https://hg.mozilla.org/mozilla-central/rev/90b6d2a44348
https://hg.mozilla.org/mozilla-central/rev/96f82111d090
https://hg.mozilla.org/mozilla-central/rev/78f0949318a5