Closed Bug 1176156 Opened 4 years ago Closed 4 years ago

make sure images are asked to decode if we draw the alt feedback

Categories

(Core :: Layout: Images, Video, and HTML Frames, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: tnikkel, Assigned: tnikkel)

Details

Attachments

(1 file, 1 obsolete file)

If we weren't drawing the alt feedback the Draw() call on the image would kick off decoding. But if we do draw the alt feedback we won't Draw() the image and it's possible that nothing will ask it to decode. We should just ask the image to decode if we are drawing the alt feedback for it.
Attached patch patch (obsolete) — Splinter Review
We need a way to ask to decode with async notifications, this is most of this patch.

The patch for bug 1151359 would change this patch, we could call requestDecodeForSize and pass the flags we need. But that wouldn't be quite enough. That patch as written needs mImage (an imgIContainer), but we might not have one yet. We should consider making MaybeDecodeForPredictedSize work without having an image yet.
Attachment #8624608 - Flags: review?(seth)
This is one of the reason that dynamically inserted images are slow to decode and we show the annoying "loading" placeholder. If the first reflow of the image after it is inserted doesn't place it somewhere that is visible, but a subsequent reflow moves the image to be visible this could happen.
Comment on attachment 8624608 [details] [diff] [review]
patch

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

::: image/imgIContainer.idl
@@ +419,5 @@
>    /*
> +   * Like requestDecode but no notifications are sent during this function call
> +   * even if they are already queued for dispatching. No decoding is done either.
> +   */
> +  void requestDecodeAsyncNotify();

So you could just pass FLAG_ASYNC_NOTIFY to requestDecodeForSize() to get this behavior, right?

::: image/imgIRequest.idl
@@ +157,5 @@
>     * container already exists, or calls it once it gets OnStartContainer if the
>     * container does not yet exist.
>     */
>    void requestDecode();
> +  void requestDecodeAsyncNotify();

Likewise, this should probably just call requestDecodeForSize() once the image is available.
(In reply to Seth Fowler [:seth] from comment #4)
> So you could just pass FLAG_ASYNC_NOTIFY to requestDecodeForSize() to get
> this behavior, right?

Yes, except what size to pass? I don't think there is a way to tell requestDecodeForSize to decode at the intrinsic size and bug 1151359 has the code to compute the size.

After bug 1151359 lands we could do that but we'd want to add requestDecodeForSize to imgRequestProxy and imgRequest so that we can request a decode for a specific size before we have an Image.
(In reply to Timothy Nikkel (:tn) from comment #5)
> Yes, except what size to pass? I don't think there is a way to tell
> requestDecodeForSize to decode at the intrinsic size and bug 1151359 has the
> code to compute the size.

Yeah, that's true; if we haven't done a size decode yet (so we can't call GetWidth()/GetHeight()), there's a problem. (Actually it should work to just pass (0,0) in that case, since if we don't have a size we just request a decode at the intrinsic size. I don't like that from a style perspective, though, and I actually consider that behavior incorrect, so it'll stop working in the future.)

Now that I think about it though, can't we just call RequestDecode() here? FLAG_ASYNC_NOTIFY only matters if we do a sync decode, which we never will in response to RequestDecode(). I may have missed something, though.

> After bug 1151359 lands we could do that but we'd want to add
> requestDecodeForSize to imgRequestProxy and imgRequest so that we can
> request a decode for a specific size before we have an Image.

Yeah. That's something I want to resolve soon.
Comment on attachment 8624608 [details] [diff] [review]
patch

Okay. I'll update the patch to just use RequestDecode.
Attachment #8624608 - Flags: review?(seth)
Attached patch patch v2Splinter Review
Attachment #8624608 - Attachment is obsolete: true
Attachment #8631807 - Flags: review?(seth)
Comment on attachment 8631807 [details] [diff] [review]
patch v2

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

The patch is so much smaller now. =)
Attachment #8631807 - Flags: review?(seth) → review+
https://hg.mozilla.org/mozilla-central/rev/52bf22e25780
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Product: Core → Core Graveyard
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.