Emilio Cobos Álvarez (:emilio) Assignee
	Description • 6 years ago

Just so bug 215083 is easier to review.

	Daniel Holbert [:dholbert]
	Comment 6 • 6 years ago
mozreview-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.

	Daniel Holbert [:dholbert]
	Comment 7 • 6 years ago
mozreview-review

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

	Daniel Holbert [:dholbert]
	Comment 8 • 6 years ago
mozreview-review

Comment on attachment 8976464 [details]
Bug 1462272: Remove an unneeded and ugly reinterpret_cast.

https://reviewboard.mozilla.org/r/244596/#review250906

	Daniel Holbert [:dholbert]
	Comment 9 • 6 years ago
mozreview-review

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?

	Daniel Holbert [:dholbert]
	Comment 10 • 6 years ago
mozreview-review

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.

	Daniel Holbert [:dholbert]
	Comment 16 • 6 years ago
mozreview-review

Comment on attachment 8976462 [details]
Bug 1462272: Remove a weirdly named variable, and call LoadIcons sooner.

https://reviewboard.mozilla.org/r/244592/#review251064

	Daniel Holbert [:dholbert]
	Comment 17 • 6 years ago
mozreview-review

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

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 18 • 6 years ago

Sounds good, thank you!

	Andrew Osmond [:aosmond] (he/him)
	Comment 19 • 6 years ago
mozreview-review

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.

	Pulsebot
	Comment 20 • 6 years ago

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

	Cristina Coroiu [:ccoroiu]
	Comment 21 • 6 years ago
bugherder

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