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.

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

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?

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.

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

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

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

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

I'm not sure why we need onStartDecode either.

(In reply to comment #8) > I'm not sure why we need onStartDecode either. for one, nsImageLoadingContent needs it in bug 512435

also note bug 521604.

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

I'm working on this again.

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

Re-assigning per :jdm , :bholley , & :jst The bug title should probably be updated to reflect both imgIContainerObserver & imgIDecoderObserver

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

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.

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

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

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

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

mother of god

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.

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

That is an eminently reasonable idea.

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

(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 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 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 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 <stuart@mozilla.com> > * @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

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

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

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.

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

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?

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.

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.

https://tbpl.mozilla.org/?tree=Try&rev=124cddb0bd9d Lay on, Macduff, and damn'd be him that first cries, "Hold, enough!"

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.

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

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.

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

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

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