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 ?

hm.. the patch is not really work. we still can not prevent image download

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();

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 "&="?

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.

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

https://hg.mozilla.org/mozilla-central/rev/76ec2bd0f251 https://hg.mozilla.org/mozilla-central/rev/ebe4713f3fba https://hg.mozilla.org/mozilla-central/rev/319bc66e5d3a