Just so bug 215083 is easier to review.

Comment on attachment 8976462 [details] Bug 1462272: Remove a weirdly named variable, and call LoadIcons sooner. https://reviewboard.mozilla.org/r/244592/#review250900 ::: commit-message-3a91e:1 (Diff revision 1) > +Bug 1462272: Remove a weirdly named variable. r?dholbert This patch also changes the order of some steps, right? (It's shifting the LoadIcons() call up a few lines, so that now it'll happen before the imageLoader->AddNativeObserver(...) call, rather than after it.) Is there a reason you're doing that reordering? And (assuming that change is intentional) that part seems worth calling that out in the commit message at least as much as the variable-removal, since that part is potentially a functional change whereas the removal is not really.

Comment on attachment 8976463 [details] Bug 1462272: Introduce nsImageFrame::GetCurrentRequest. https://reviewboard.mozilla.org/r/244594/#review250902 ::: layout/generic/nsImageFrame.cpp:280 (Diff revision 1) > > - // We have a PresContext now, so we need to notify the image content node > - // that it can register images. > + // We have a PresContext now, so we need to notify the image content node that > + // it can register images. > imageLoader->FrameCreated(this); > > - // Give image loads associated with an image frame a small priority boost! > + if (nsCOMPtr<imgIRequest> currentRequest = GetCurrentRequest()) { Right now it looks like the patch is removing this code-comment, but I don't think there's any reason to remove it (it establishes some context for the priority-bumping that's about to happen below). Probably resurrect it? ::: layout/generic/nsImageFrame.cpp:882 (Diff revision 1) > // this situation we should always use mIntrinsicSize, because > // GetNaturalWidth/Height will return 0, so we check CurrentRequestHasSize() > // below. See bug 1019840. We will fix this in bug 1141395. > > // Content may override our default dimensions. This is termed as overriding > // the intrinsic size by the spec, but all other consumers of mIntrinsic* > // values are being used to refer to the real/true size of the image data. > - if (imageLoader && imageLoader->CurrentRequestHasSize() && mImage && > + if (currentRequest && SizeIsAvailable(currentRequest) && mImage && Please update this comment to update its mention of CurrentRequestHasSize (an API that you're removing here).

Comment on attachment 8976464 [details] Bug 1462272: Remove an unneeded and ugly reinterpret_cast. https://reviewboard.mozilla.org/r/244596/#review250906

Comment on attachment 8976465 [details] Bug 1462272: Remove handling for an impossible condition. https://reviewboard.mozilla.org/r/244598/#review250908 [Setting r- not because of anything catastrophic; just so that you can re-request review and trigger bugmail once review nits are addressed] ::: commit-message-1e566:1 (Diff revision 1) > +Bug 1462272: Remove an impossible condition. r?dholbert s/Remove/Remove handling for/ ::: commit-message-1e566:3 (Diff revision 1) > +The frame is notified via the listener, which is an observer of > +nsImageLoadingContent. This last one only notifies for the current and pending Two nits: (1) "the listener" is kinda hand-wavy. Let's clarify to "nsImageListener" or "its mListener", assuming that's what you mean here. (2) I'm not sure what you mean by "this last one" here. Are you referring to nsImageLoadingContent? And can you provide a clearer justification for why we're sure that's only sent for those two request types? (I do see that nsImageLoadingContent::GetRequestType only returns CURRENT_REQUEST, PENDING_REQUEST, **or UNKNOWN_REQUEST** -- and I haven't dug further to be confident about whether or not it's possible for us to end up in a condition where we return that last UNKNOWN_REQUEST value here. And, if that is possible, I'm willing to believe that the graceful fallback codepath that you're removing here might be useful in that scenario...) ::: layout/generic/nsImageFrame.cpp (Diff revision 1) > - // Check what request type we're dealing with > - nsCOMPtr<nsIImageLoadingContent> imageLoader = do_QueryInterface(mContent); > - NS_ASSERTION(imageLoader, "Who's notifying us??"); > - int32_t loadType = nsIImageLoadingContent::UNKNOWN_REQUEST; > - imageLoader->GetRequestType(aRequest, &loadType); > - if (loadType != nsIImageLoadingContent::CURRENT_REQUEST && > - loadType != nsIImageLoadingContent::PENDING_REQUEST) { > - return NS_ERROR_FAILURE; > - } Assuming we're really justified in removing this code -- would it be worth preserving this as an assertion somehow?

Comment on attachment 8976466 [details] Bug 1462272: Minor cleanup of nsImageListener's ctor / dtors. https://reviewboard.mozilla.org/r/244600/#review250910 ::: layout/generic/nsImageFrame.cpp:2442 (Diff revision 1) > -nsImageListener::nsImageListener(nsImageFrame *aFrame) : > - mFrame(aFrame) > +nsImageListener::nsImageListener(nsImageFrame* aFrame) > + : mFrame(aFrame) > { > } > > -nsImageListener::~nsImageListener() > +nsImageListener::~nsImageListener() = default; Does this change (going from "{}" to "= default") mean it won't be possible to place a gdb breakpoint in the destructor? If this removes that ability, then I'd rather not make this change... If it's still possible though, then this seems fine.

Comment on attachment 8976462 [details] Bug 1462272: Remove a weirdly named variable, and call LoadIcons sooner. https://reviewboard.mozilla.org/r/244592/#review251064

Comment on attachment 8976465 [details] Bug 1462272: Remove handling for an impossible condition. https://reviewboard.mozilla.org/r/244598/#review251074 I don't have enough recent familiarity with the image-loading pipeline / notifications to be confident about the changes in this part (the assumptions around which notifications are sent/handled for which possible requests), so I'm less comfortable r+'ing this part. I'd rather tnikkel or aosmond review this part, since they've been doing more-substantial imagelib work more recently than I have. (If they can't, I could do a bit more research to get comfortable enough to review it, too.)

Sounds good, thank you!

Comment on attachment 8976465 [details] Bug 1462272: Remove handling for an impossible condition. https://reviewboard.mozilla.org/r/244598/#review251128 LGTM. The nsImageFrame::OnSizeAvailable path would be broken if we are seeing this behaviour today, so agreed, if there is a problem, we need to fix it.

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/24f85fad27a4 Remove a weirdly named variable, and call LoadIcons sooner. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/072d64b67a99 Introduce nsImageFrame::GetCurrentRequest. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/cd5d5045c6cc Remove an unneeded and ugly reinterpret_cast. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/fb0ec55f64b4 Remove handling for an impossible condition. r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/11f4d626ab56 Minor cleanup of nsImageListener's ctor / dtors. r=dholbert

https://hg.mozilla.org/mozilla-central/rev/24f85fad27a4 https://hg.mozilla.org/mozilla-central/rev/072d64b67a99 https://hg.mozilla.org/mozilla-central/rev/cd5d5045c6cc https://hg.mozilla.org/mozilla-central/rev/fb0ec55f64b4 https://hg.mozilla.org/mozilla-central/rev/11f4d626ab56