We should use URLValue or ImageValue for the computed value storage of all CSS properties taking url() values. This will let us create and compare these values OMT, which is needed for stylo. This will involve moving the current FragmentOrURL functionality into URLValueData.

Comment on attachment 8796791 [details] Bug 1298774 - Part 0: Move refcounting from URLValue and ImageValue up to URLValueData. https://reviewboard.mozilla.org/r/82524/#review81228 ::: layout/style/nsCSSValue.h:117 (Diff revision 1) > already_AddRefed<PtrHolder<nsIURI>> aReferrer, > already_AddRefed<PtrHolder<nsIPrincipal>> aOriginPrincipal); > > +public: > bool operator==(const URLValueData& aOther) const; > SizeOfExcludingThis and DefinitelyEqualURIs can be protected.

Comment on attachment 8796792 [details] Bug 1298774 - Part 1: Rename URLValueData::operator== so that we don't blithely call it OMT. https://reviewboard.mozilla.org/r/82526/#review81230 ::: layout/style/nsCSSValue.h:120 (Diff revision 1) > public: > - bool operator==(const URLValueData& aOther) const; > + // Returns true iff all fields of the two URLValueData objects are equal. > + // > + // Only safe to call on the main thread, since this will call Equals on the > + // nsIURI and nsIPrincipal objects stored on the URLValueData objects. > + bool Equals(const URLValueData& aOther) const; Should we hide equal operator by declare it in private section? ::: layout/style/nsCSSValue.cpp:2675 (Diff revision 1) > MOZ_ASSERT(mOriginPrincipal); > } > > bool > -css::URLValueData::operator==(const URLValueData& aOther) const > +css::URLValueData::Equals(const URLValueData& aOther) const > { Add MOZ_ASSERT(NS_IsMainThread());

Comment on attachment 8796791 [details] Bug 1298774 - Part 0: Move refcounting from URLValue and ImageValue up to URLValueData. https://reviewboard.mozilla.org/r/82524/#review81232

Comment on attachment 8796793 [details] Bug 1298774 - Part 2: Rename URLValueData::mLocalURLFlag to match FragmentOrURL::mIsLocalRef naming. https://reviewboard.mozilla.org/r/82528/#review81234

Comment on attachment 8796794 [details] Bug 1298774 - Part 3: Copy helper functions from FragmentOrURL to URLValueData. https://reviewboard.mozilla.org/r/82530/#review81236

Comment on attachment 8796796 [details] Bug 1298774 - Part 5: Make nsStyleSVGPaint use css::URLValue for url() storage instead of FragmentOrURL. https://reviewboard.mozilla.org/r/82534/#review81240 ::: layout/style/nsStyleStruct.h:3576 (Diff revision 1) > + MOZ_ASSERT(mType == eStyleSVGPaintType_Color); > + return mPaint.mColor; > + } > + > + mozilla::css::URLValue* GetPaintServer() const { > + MOZ_ASSERT(mType == eStyleSVGPaintType_Server); Is that ok to return const URLValue* here? ::: layout/style/nsStyleStruct.cpp:1404 (Diff revision 1) > + aOther.mFallbackColor); > + break; > + case eStyleSVGPaintType_ContextFill: > + case eStyleSVGPaintType_ContextStroke: > + SetContextValue(aOther.mType, aOther.mFallbackColor); > + break; Should we also handle nsStyleSVGPaintType(0) here? ::: layout/style/nsStyleStruct.cpp:1462 (Diff revision 1) > + mFallbackColor == aOther.mFallbackColor; > + case eStyleSVGPaintType_ContextFill: > + case eStyleSVGPaintType_ContextStroke: > + return mFallbackColor == aOther.mFallbackColor; > + default: > - return true; > + return true; MOZ_ASSERT(mType == eStyleSVGPaintType_None || mType == nsStyleSVGPaintType(0), "Unexpected SVG paint type");

Comment on attachment 8796795 [details] Bug 1298774 - Part 4: Make ShapeStyleSource use css::URLValue for url() storage instead of FragmentOrURL. https://reviewboard.mozilla.org/r/82532/#review81250 ::: layout/style/nsStyleStruct.h:2708 (Diff revision 1) > StyleShapeSourceType GetType() const > { > return mType; > } > > - FragmentOrURL* GetURL() const > + css::URLValue* GetURL() const How about returning const css:URLValue* ::: layout/svg/nsSVGEffects.cpp:949 (Diff revision 1) > + if (!aURL->EqualsExceptRef(baseURI)) > + baseURI = content->OwnerDoc()->GetBaseURI(); > + } > + > + return aURL->ResolveLocalRef(baseURI); > +} Please rebase on patches in Bug 1302779

Comment on attachment 8796797 [details] Bug 1298774 - Part 6: Make SVG marker properties use css::URLValue for storage instead of FragmentOrURL. https://reviewboard.mozilla.org/r/82536/#review81298

Comment on attachment 8796800 [details] Bug 1298774 - Part 9: Remove FragmentOrURL. https://reviewboard.mozilla.org/r/82542/#review81300

Comment on attachment 8796798 [details] Bug 1298774 - Part 7: Make nsStyleFilter use css::URLValue for url() storage instead of FragmentOrURL. https://reviewboard.mozilla.org/r/82538/#review81306 ::: layout/style/nsStyleStruct.h:3765 (Diff revision 1) > void SetFilterParameter(const nsStyleCoord& aFilterParameter, > int32_t aType); > > - mozilla::FragmentOrURL* GetURL() const { > + mozilla::css::URLValue* GetURL() const { > NS_ASSERTION(mType == NS_STYLE_FILTER_URL, "wrong filter type"); > return mURL; We call GetURL() only when mType == NS_STYLE_FILTER_URL and mURL != nullptr. Depend on you, but I think we should promote NS_ASSERION to MOZ_ASSERT here and similair GetURL functions.

Comment on attachment 8796799 [details] Bug 1298774 - Part 8: Make mask-image use css::URLValueData for url() storage instead of FragmentOrURL. https://reviewboard.mozilla.org/r/82540/#review81316 ::: layout/style/nsRuleNode.cpp:6755 (Diff revision 1) > + const nsCSSValueList* aSpecifiedValue, > + RefPtr<css::URLValueData>& aComputedValue, > + RuleNodeCacheConditions& aConditions) > + { > + switch (aSpecifiedValue->mValue.GetUnit()) { > + case eCSSUnit_Null: Why not assign nullptr to aComputedValue in this case? ::: layout/style/nsRuleNode.cpp:6760 (Diff revision 1) > + case eCSSUnit_Null: > + break; > + case eCSSUnit_URL: > + case eCSSUnit_Image: > + aComputedValue = aSpecifiedValue->mValue.GetURLOrImageStructValue(); > + break; Calling mValue->GetURLStructValue() when mValue->GetUnit() returns eCSSUnit_URL, and calling mValue->GetImageStructValue() when mValue->GetUnit() returns eCSSUnit_Image is clearer to me. Do we really need to compose GetURLStructValue & GetImageStructValue into GetURLOrImageStructValue?

Comment on attachment 8796791 [details] Bug 1298774 - Part 0: Move refcounting from URLValue and ImageValue up to URLValueData. https://reviewboard.mozilla.org/r/82524/#review81228 > SizeOfExcludingThis and DefinitelyEqualURIs can be protected. I want to call DefinitelyEqualURIs from later patches, so I will keep it public. SizeOfIncludingThis can indeed be protected.

Comment on attachment 8796792 [details] Bug 1298774 - Part 1: Rename URLValueData::operator== so that we don't blithely call it OMT. https://reviewboard.mozilla.org/r/82526/#review81230 > Should we hide equal operator by declare it in private section? There's no default operator== to hide, so I think it's fine just to leave the renamed method here. (We still do need to be able to call it from nsCSSValue::operator==.)

Comment on attachment 8796795 [details] Bug 1298774 - Part 4: Make ShapeStyleSource use css::URLValue for url() storage instead of FragmentOrURL. https://reviewboard.mozilla.org/r/82532/#review81250 > How about returning const css:URLValue* Seems to be too much propagation of const needed to do this. :(

Comment on attachment 8796796 [details] Bug 1298774 - Part 5: Make nsStyleSVGPaint use css::URLValue for url() storage instead of FragmentOrURL. https://reviewboard.mozilla.org/r/82534/#review81240 > Is that ok to return const URLValue* here? Looks like too much work, too. > Should we also handle nsStyleSVGPaintType(0) here? I will add an assertion that aOther's type isn't nsStyleSVGPaintType(0). > MOZ_ASSERT(mType == eStyleSVGPaintType_None || > mType == nsStyleSVGPaintType(0), > "Unexpected SVG paint type"); I think it's unexpected to have nsStyleSVGPaintType(0) here, since that would mean we didn't initialize the paint value, so I will assert that it's None.

Comment on attachment 8796799 [details] Bug 1298774 - Part 8: Make mask-image use css::URLValueData for url() storage instead of FragmentOrURL. https://reviewboard.mozilla.org/r/82540/#review81316 > Why not assign nullptr to aComputedValue in this case? In all of the nsRuleNode::ComputeXXXData functions, we are performing the cascade. If a specified value has eCSSUnit_Null, it means there were no matching rules that specified that property value, and it means we should leave the computed value on the struct alone. (This is because we might have started computing a given style struct by copying an existing one, and only filling in values for properties that were specified in the matching rules.) > Calling mValue->GetURLStructValue() when mValue->GetUnit() returns eCSSUnit_URL, and calling mValue->GetImageStructValue() when mValue->GetUnit() returns eCSSUnit_Image is clearer to me. > Do we really need to compose GetURLStructValue & GetImageStructValue into GetURLOrImageStructValue? No, we don't need to. I can have separate GetURLStructValue() / GetImageStructValue() calls.

Pushed by cmccormack@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ee2f29d9d25d Part 0: Move refcounting from URLValue and ImageValue up to URLValueData. r=cjku https://hg.mozilla.org/integration/autoland/rev/c2f79a8799b7 Part 1: Rename URLValueData::operator== so that we don't blithely call it OMT. r=cjku https://hg.mozilla.org/integration/autoland/rev/a6d0275a3a4c Part 2: Rename URLValueData::mLocalURLFlag to match FragmentOrURL::mIsLocalRef naming. r=cjku https://hg.mozilla.org/integration/autoland/rev/54c0e9788b73 Part 3: Copy helper functions from FragmentOrURL to URLValueData. r=cjku https://hg.mozilla.org/integration/autoland/rev/cedae7f22195 Part 4: Make ShapeStyleSource use css::URLValue for url() storage instead of FragmentOrURL. r=cjku https://hg.mozilla.org/integration/autoland/rev/5b295c18eaa0 Part 5: Make nsStyleSVGPaint use css::URLValue for url() storage instead of FragmentOrURL. r=cjku https://hg.mozilla.org/integration/autoland/rev/e6b25df5b0b4 Part 6: Make SVG marker properties use css::URLValue for storage instead of FragmentOrURL. r=cjku https://hg.mozilla.org/integration/autoland/rev/2830c496e2d2 Part 7: Make nsStyleFilter use css::URLValue for url() storage instead of FragmentOrURL. r=cjku https://hg.mozilla.org/integration/autoland/rev/1b9a7dd6153a Part 8: Make mask-image use css::URLValueData for url() storage instead of FragmentOrURL. r=cjku https://hg.mozilla.org/integration/autoland/rev/0b72cc61989e Part 9: Remove FragmentOrURL. r=cjku

https://hg.mozilla.org/mozilla-central/rev/ee2f29d9d25d https://hg.mozilla.org/mozilla-central/rev/c2f79a8799b7 https://hg.mozilla.org/mozilla-central/rev/a6d0275a3a4c https://hg.mozilla.org/mozilla-central/rev/54c0e9788b73 https://hg.mozilla.org/mozilla-central/rev/cedae7f22195 https://hg.mozilla.org/mozilla-central/rev/5b295c18eaa0 https://hg.mozilla.org/mozilla-central/rev/e6b25df5b0b4 https://hg.mozilla.org/mozilla-central/rev/2830c496e2d2 https://hg.mozilla.org/mozilla-central/rev/1b9a7dd6153a https://hg.mozilla.org/mozilla-central/rev/0b72cc61989e

Pushed by cku@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/cf26d9dfd600 followup: fix compile error by disable mask-as-shorthand. r=me

We have a partner add-on using -moz-binding: url("../../component.xml#something") that seems to have stopped working after this change. Is it possible this change broke correct resolution of such relative paths? And if so, was that an expected outcome? Note: we are still waiting for their confirmation, so this is mostly an exploratory request at this stage, just in case you have an off-hand idea about it.

No, relative URLs shouldn't have been broken. I would be happy to investigate if/when you have further details.

Nevermind for now, looks like the range is a bit uncertain. Changing the relative path to absolute didn't help, so the mozregression range is likely a bit off. I'll move on with the investigation and eventually will come back when I get some more details.

I can finally confirm this change didn't cause the issue.