Closed Bug 1301245 Opened 7 years ago Closed 6 years ago

stop trying to load SVG mask references as images

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: heycam, Assigned: u459114)

References

Details

Attachments

(3 files, 8 obsolete files)

Bug 1301245 - Part 1. Implement nsStyleImage::IsResolved.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1301245 - Part 2. Do not resolve a style image if the given url has a fragment.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1301245 - Part 3. Resolve a style image if nsSVGPaintingProperty can not handle it.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
We compute the mask-image property into two separate fields on the nsStyleImageLayers::Layer:

  nsStyleImage mImage;
  FragmentOrURL mSourceURI;

We set mImage regardless of whether the url() value has a fragment.  This means we end up starting an image load for values like |mask-image: url(external.svg#mask)|.  We should just set mImage for fragment-less url() values and mSourceURI for those with a fragment.
mask-image:url(xxx.svg#viewBoxID)

In this case, we still need to start image download
(In reply to C.J. Ku[:cjku](UTC+8) from comment #2)
> mask-image:url(xxx.svg#viewBoxID)
> 
> In this case, we still need to start image download

Oh, that is kind of annoying.  We don't know whether we will use it as an image or as a <mask> element reference until after loading it as an image already.
Is this still a valid bug to fix ?
Flags: needinfo?(cam)
Status: NEW → ASSIGNED
Priority: -- → P3
Attachment #8844387 - Flags: review?(cam)
Attachment #8844387 - Flags: review?(cam)
hm.. the patch is not really work. we still can not prevent image download
Attachment #8844387 - Attachment is obsolete: true
Attachment #8852922 - Attachment is obsolete: true
Attachment #8852923 - Attachment is obsolete: true
Attachment #8887999 - Attachment is obsolete: true
Attachment #8888635 - Attachment is obsolete: true
Attachment #8888001 - Attachment is obsolete: true
Attachment #8888636 - Attachment is obsolete: true
Attachment #8888000 - Flags: review?(cam)
Attachment #8890180 - Flags: review?(cam)
Attachment #8890181 - Flags: review?(cam)
Attachment #8890182 - Flags: review?(cam)
Attachment #8890182 - Attachment is obsolete: true
Attachment #8890182 - Flags: review?(cam)
Comment on attachment 8888000 [details]
Bug 1301245 - Part 1. Implement nsStyleImage::IsResolved.

https://reviewboard.mozilla.org/r/158874/#review166666

::: layout/style/nsStyleStruct.h:467
(Diff revision 3)
>    nsStyleGradient* GetGradientData() const {
>      NS_ASSERTION(mType == eStyleImageType_Gradient, "Data is not a gradient!");
>      return mGradient;
>    }
> +  bool IsResolved() const {
> +    return (mType == eStyleImageType_Image) ? GetImageRequest()->IsResolved()

Or, if you like:

  return mType == eStyleImageType_Image && GetImageRequest()->IsResolved();
Attachment #8888000 - Flags: review?(cam) → review+
Comment on attachment 8888000 [details]
Bug 1301245 - Part 1. Implement nsStyleImage::IsResolved.

https://reviewboard.mozilla.org/r/158874/#review166666

> Or, if you like:
> 
>   return mType == eStyleImageType_Image && GetImageRequest()->IsResolved();

Or, more correctly!

  return mType != eStyleImageType_Image || GetImageRequest()->IsResolved();

but that is less clear, so feel free to leave what you have.
Comment on attachment 8890180 [details]
Bug 1301245 - Part 2. Do not resolve a style image if the given url has a fragment.

https://reviewboard.mozilla.org/r/161246/#review166670

::: layout/svg/nsSVGIntegrationUtils.cpp:504
(Diff revision 3)
>  
>        aParams.imgParams.result &=
>          nsCSSRendering::PaintStyleImageLayerWithSC(params, *maskContext, aSC,
>                                                *aParams.frame->StyleBorder());
> +    } else {
> +      MOZ_ASSERT(!svgReset->mMask.mLayers[i].mImage.IsResolved());

I guess we don't need this assertion really since it's exactly what we just checked in the "else if" above.

::: layout/svg/nsSVGIntegrationUtils.cpp:505
(Diff revision 3)
>        aParams.imgParams.result &=
>          nsCSSRendering::PaintStyleImageLayerWithSC(params, *maskContext, aSC,
>                                                *aParams.frame->StyleBorder());
> +    } else {
> +      MOZ_ASSERT(!svgReset->mMask.mLayers[i].mImage.IsResolved());
> +      aParams.imgParams.result &= DrawResult::NOT_READY;

I'm not really familiar with DrawResult stuff, but should this be "|=" instead of "&="?
Attachment #8890180 - Flags: review?(cam) → review+
Comment on attachment 8890181 [details]
Bug 1301245 - Part 3. Resolve a style image if nsSVGPaintingProperty can not handle it.

https://reviewboard.mozilla.org/r/161248/#review166678

Please re-request review if you need to make that nsSVGFrameReferenceFromProperty change.

::: layout/svg/nsSVGEffects.h:363
(Diff revision 3)
> +  void ResolveImage(uint32_t aIndex);
> +
>  private:
>    virtual ~nsSVGMaskProperty() {}
>    nsTArray<RefPtr<nsSVGPaintingProperty>> mProperties;
> +  nsIFrame* mFrame;

I see that nsSVGFilterProperty uses an nsSVGFrameReferenceFromProperty to hold on to its frame.  Do we need the same?  I am not sure.  Please check.

::: layout/svg/nsSVGEffects.cpp:423
(Diff revision 3)
>  }
>  
> +void
> +nsSVGMaskProperty::ResolveImage(uint32_t aIndex)
> +{
> +  const nsStyleSVGReset *svgReset = mFrame->StyleSVGReset();

Nit: "*" next to type.

::: layout/svg/nsSVGEffects.cpp:424
(Diff revision 3)
>  
> +void
> +nsSVGMaskProperty::ResolveImage(uint32_t aIndex)
> +{
> +  const nsStyleSVGReset *svgReset = mFrame->StyleSVGReset();
> +  MOZ_ASSERT(aIndex <= svgReset->mMask.mImageCount);

Should that be "<"?

::: layout/svg/nsSVGEffects.cpp:436
(Diff revision 3)
> +    image.ResolveImage(mFrame->PresContext());
> +
> +    mozilla::css::ImageLoader* imageLoader =
> +      mFrame->PresContext()->Document()->StyleImageLoader();
> +    if (imgRequestProxy* req = image.GetImageData()) {
> +        imageLoader->AssociateRequestToFrame(req, mFrame);

Nit: indent wrong.
Attachment #8890181 - Flags: review?(cam) → review+
Comment on attachment 8890180 [details]
Bug 1301245 - Part 2. Do not resolve a style image if the given url has a fragment.

https://reviewboard.mozilla.org/r/161246/#review166670

> I'm not really familiar with DrawResult stuff, but should this be "|=" instead of "&="?

http://searchfox.org/mozilla-central/source/image/DrawResult.h#81

Looks odd but it actually should be "&="
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/76ec2bd0f251
Part 1. Implement nsStyleImage::IsResolved. r=heycam
https://hg.mozilla.org/integration/autoland/rev/ebe4713f3fba
Part 2. Do not resolve a style image if the given url has a fragment. r=heycam
https://hg.mozilla.org/integration/autoland/rev/319bc66e5d3a
Part 3. Resolve a style image if nsSVGPaintingProperty can not handle it. r=heycam
Depends on: 1402157
See Also: → 1486252
You need to log in before you can comment on or make changes to this bug.