1463530 - Route all image requests through nsIFrame.

Status: RESOLVED INCOMPLETE

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

As a preliminar step to move them outside of style structs.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1463530: route all image requests through nsIFrame.

For now, this makes no difference, in the sense that the general setup doesn't
change and I just add two inline functions to handle all requests.

In the future, this will probably go through the document's image loader, and we
could just use ImageValue and such.

We probably need to refactor a bit of nsImageRenderer to avoid making that a
bunch of hashtable lookups, but we'll see, that's not done yet.

Review commit: https://reviewboard.mozilla.org/r/245850/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/245850/

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8979699 [details]
Bug 1463530: route all image requests through nsIFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/245850/diff/1-2/

Comment 3

[Cameron McCormack (:heycam)]

Can you explain what the future setup is going to look like, to help me review this intermediate step?  For example, if these nsIFrame methods are just going to end up calling in to something on the document or its image loader, it make instead make sense to be passing in the nsPresContext rather than the frame.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Yes, generally it'd be to call something on the document / the document's ImageLoader. Though I haven't decided yet whether we want a frame for that or not, chances are we do, given we track all the image registrations / deregistrations on frames. So I think passing a frame is a safer bet for now :)

Comment 5

[Cameron McCormack (:heycam)]

Comment on attachment 8979699 [details]
Bug 1463530: route all image requests through nsIFrame.

https://reviewboard.mozilla.org/r/245850/#review253224

::: layout/base/nsCSSFrameConstructor.h:468
(Diff revision 2)
>    already_AddRefed<nsIContent> CreateGeneratedContent(nsFrameConstructorState& aState,
>                                                        mozilla::dom::Element* aParentContent,
> +                                                      nsIFrame* aParentFrame,

Please document the new argument in the comment.

::: layout/generic/nsFrame.cpp:1175
(Diff revision 2)
>      if (newBorderImage) {
>        imageLoader->AssociateRequestToFrame(newBorderImage, this, 0);
>      }
>    }
>  
>    imgIRequest* oldShapeImage =

Change these ones to imgRequestProxy too?

::: layout/generic/nsIFrame.h:4104
(Diff revision 2)
> +  // Gets an image request for a given nsStyleImage, or null if the image is not
> +  // well, an image, but other thing like, e.g., a gradient.

I know at the moment it doesn't matter, since we don't do anything with the frame these methods are called on, but can you document what the expected relationship is between the frame and the nsStyleImage(Request) that we pass in?  For example, must the nsStyleImage(Request) be from the ComputedValues for the frame?

::: layout/painting/nsDisplayList.cpp:4023
(Diff revision 2)
> -      imgIRequest* imgreq = image->GetImageData();
> +      imgRequestProxy* imgreq =
> +        backgroundStyleFrame->GetImageRequest(*image->ImageRequest());

What will happen if we use backgroundStyleFrame here, and that frame differs from the frame we've been using previously to call GetImageRequest on for this nsStyleImageRequest?  (Nothing now, since GetImageRequest doesn't use the frame yet, but in the future?)  This depends on what the exact semantics of GetImageRequest on the frame will be, which is somsething I can't quite tell yet.

::: layout/painting/nsDisplayList.cpp:4243
(Diff revision 2)
>    // Of course, if there's only one frame in the flow, it doesn't matter.
>    if (mFrame->StyleBorder()->mBoxDecorationBreak ==
>          StyleBoxDecorationBreak::Clone ||
>        (!mFrame->GetPrevContinuation() && !mFrame->GetNextContinuation())) {
>      const nsStyleImageLayers::Layer& layer = mBackgroundStyle->StyleBackground()->mImage.mLayers[mLayer];
> -    if (layer.mImage.IsOpaque() && layer.mBlendMode == NS_STYLE_BLEND_NORMAL &&
> +    if (layer.mImage.IsOpaque(mFrame) &&

Why do we use mFrame here and not the background style frame?  (I don't know which makes sense.)

::: layout/painting/nsDisplayList.cpp:4294
(Diff revision 2)
>      // of the rounded corners.
>      return true;
>    }
>  
>    const nsStyleImageLayers::Layer &layer = mBackgroundStyle->StyleBackground()->mImage.mLayers[mLayer];
> -  if (layer.RenderingMightDependOnPositioningAreaSizeChange()) {
> +  if (layer.RenderingMightDependOnPositioningAreaSizeChange(mFrame)) {

Same here.

::: layout/style/nsStyleStruct.h:413
(Diff revision 2)
>     * @param aActualCropRect the computed actual crop rect.
>     * @param aIsEntireImage true iff |aActualCropRect| is identical to the
>     * source image bounds.
>     * @return true iff |aActualCropRect| holds a meaningful value.
>     */
> -  bool ComputeActualCropRect(nsIntRect& aActualCropRect,
> +  bool ComputeActualCropRect(nsIFrame* aForFrame,

In these methods please document what aForFrame is used for.

::: layout/xul/nsImageBoxFrame.cpp:671
(Diff revision 2)
>  
>    // If list-style-image changes, we have a new image.
>    nsCOMPtr<nsIURI> oldURI, newURI;
>    if (mImageRequest)
>      mImageRequest->GetURI(getter_AddRefs(oldURI));
> -  if (myList->GetListStyleImage())
> +  if (auto* request = GetImageRequest(myList->mListStyleImage)) {

Would prefer the type to be spelled out, since there are a couple of different types with "ImageRequest" in their name.

Comment 6

[Cameron McCormack (:heycam)]

Overall I feel a bit uncomfortable with having a bunch of methods that we pass the frame in to, for reasons I can't quite put my finger on.  Maybe because it's unclear what the implications are for doing say:

  nsStyleImage& image = ...;
  image.IsComplete(someFrame);
  image.IsLoaded(someOtherFrame);

What does it mean to be using two different frames here?  The frame is needed to generate an imgRequestProxy for the nsStyleImage, but could a different one be generated depending on what frame happens to get passed in?  Is there an expectation that some sort of registration happens at the time we produce a new imgRequestProxy?  Will registration happen for a frame even when the imgRequestProxy already exists?

It's hard for me to suggest something I'd like better without knowing how these functions are going to be re-written in the near future.

Comment 7

[Cameron McCormack (:heycam)]

Per IRC, closing this for now. Emilio will file a new bug for any further image request moving work.