Closed Bug 821023 Opened 7 years ago Closed 7 years ago

imgStatusTracker should not know about imgRequest

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(1 file, 3 obsolete files)

imgStatusTracker knows too much. Specifically, it knows which imgRequest it is associated with, and it grabs the current Image and the current imgStatusTracker from that imgRequest when it could manage that information itself. This makes it impossible to safely create images that wrap other images, which is needed for bug 790640.

The only risk associated with fixing this is that it might slightly change the notification behavior for multipart images. (In particular, it seems that async notifications for one part currently may be delivered by the imgStatusTracker associated with the next part if the switchover happens at the wrong time.) It doesn't seem like anything should be relying on this behavior, so this should be fine.
This patch removes all knowledge of imgRequest from imgStatusTracker.
Attachment #692120 - Flags: review?(joe)
Depends on: 816374
Comment on attachment 692120 [details] [diff] [review]
imgStatusTracker should not know about imgRequest.

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

I'm a *little* concerned that we're going from an ideologically pure world where imgStatusTrackers have exactly one owner to a loosy-goosy world where they're refcounted, but it's still probably the right direction. Nice work!

::: image/src/Image.h
@@ +152,5 @@
>  
>    uint64_t mInnerWindowId;
>  
>    // Member data shared by all implementations of this abstract class
> +  nsRefPtr<imgStatusTracker> mStatusTracker;

You need one more space before the "m" in this line.

::: image/src/RasterImage.cpp
@@ +351,5 @@
>                     imgIContainerDebug)
>  #endif
>  
>  //******************************************************************************
> +RasterImage::RasterImage(imgStatusTracker* aStatusTracker, nsIURI* aURI) :

Change the last argument here to

  nsIURI* aURI /* = nullptr */

for greater documentation.

::: image/src/VectorImage.cpp
@@ +168,5 @@
>  
>  //------------------------------------------------------------------------------
>  // Constructor / Destructor
>  
> +VectorImage::VectorImage(imgStatusTracker* aStatusTracker, nsIURI* aURI) :

Change the last argument here to
  nsIURI* aURI /* = nullptr */
for greater documentation.

::: image/src/imgRequestProxy.cpp
@@ +675,5 @@
> +void imgRequestProxy::OnStartDecode()
> +{
> +  // This notification is deliberately not propagated since there are no
> +  // listeners who care about it.
> + 

extra whitespace

@@ +681,5 @@
> +    /* In the case of streaming jpegs, it is possible to get multiple OnStartDecodes which
> +       indicates the beginning of a new decode.
> +       The cache entry's size therefore needs to be reset to 0 here.  If we do not do this,
> +       the code in imgStatusTrackerObserver::OnStopFrame will continue to increase the data size cumulatively.
> +    */

Can you make this a // C++ style comment?

@@ +750,5 @@
>      // Hold a ref to the listener while we call it, just in case.
>      nsCOMPtr<imgINotificationObserver> kungFuDeathGrip(mListener);
>      mListener->Notify(this, imgINotificationObserver::DISCARD, nullptr);
>    }
> + 

extra whitespace

::: image/src/imgRequestProxy.h
@@ +26,5 @@
> +{ /* 8bbe72b4-4991-4de4-a42a-4ab35f0efdbb */         \
> +     0x8bbe72b4,                                     \
> +     0x4991,                                         \
> +     0x4de4,                                         \
> +    {0xa4, 0x2a, 0x4a, 0xb3, 0x5f, 0x0e, 0xfd, 0xbb} \

You don't need to change CIDs when you're changing the interface; CIDs define *implementations*, not interface details.
Attachment #692120 - Flags: review?(joe) → review+
(In reply to Joe Drew (:JOEDREW! \o/) from comment #3)

Thanks for the review, Joe!

> I'm a *little* concerned that we're going from an ideologically pure world
> where imgStatusTrackers have exactly one owner to a loosy-goosy world where
> they're refcounted, but it's still probably the right direction. Nice work!

Yeah, I would've avoided that if I could, but it didn't seem safe to let imgRequestNotifyRunnable hold a reference to the imgStatusTracker without making it refcounted.

> You don't need to change CIDs when you're changing the interface; CIDs
> define *implementations*, not interface details.

Cool, I wasn't sure so I did it just in case.

Will upload a new version with the requested fixes shortly.
Made changes requested in review. (No need for a new try run as everything is comment / whitespace / CID related.)
Attachment #692120 - Attachment is obsolete: true
Everything 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
Blocks: 822526
@Ryan, I think we've now confirmed that this is the patch causing the reftest failure. I'm investigating now.
Blocks: 822846
Fixed a trivial bug (reversed boolean condition) that seems to have been responsible for the failures on try.
Attachment #692489 - Attachment is obsolete: true
Looks 100% green on try now: https://tbpl.mozilla.org/?tree=Try&rev=d56196ae6ba8

Now that we have the fix in comment 10 I think this is ready to be checked in.
Keywords: checkin-needed
Crud, hold on, this needs a rebase. Let me do that and another try before checkin.
Keywords: checkin-needed
Attachment #693968 - Attachment is obsolete: true
Try job for the rebased version here: https://tbpl.mozilla.org/?tree=Try&rev=7b69d44a3acd
Try run looks good. Requesting checkin.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0ea9204be1bb
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
This patch removed the assignment to mURIString, but left the member and GetURIString(), both of which are always empty.  :(
You need to log in before you can comment on or make changes to this bug.