Closed Bug 1298774 Opened 8 years ago Closed 8 years ago

use URLValue / ImageValue for all computed url() value storage

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(10 files)

58 bytes, text/x-review-board-request
u459114
: review+
Details
58 bytes, text/x-review-board-request
u459114
: review+
Details
58 bytes, text/x-review-board-request
u459114
: review+
Details
58 bytes, text/x-review-board-request
u459114
: review+
Details
58 bytes, text/x-review-board-request
u459114
: review+
Details
58 bytes, text/x-review-board-request
u459114
: review+
Details
58 bytes, text/x-review-board-request
u459114
: review+
Details
58 bytes, text/x-review-board-request
u459114
: review+
Details
58 bytes, text/x-review-board-request
u459114
: review+
Details
58 bytes, text/x-review-board-request
u459114
: review+
Details
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.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Depends on: 1301245
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());
Attachment #8796792 - Flags: review?(cku) → review+
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
Attachment #8796791 - Flags: review?(cku) → review+
Comment on attachment 8796793 [details]
Bug 1298774 - Part 2: Rename URLValueData::mLocalURLFlag to match FragmentOrURL::mIsLocalRef naming.

https://reviewboard.mozilla.org/r/82528/#review81234
Attachment #8796793 - Flags: review?(cku) → review+
Comment on attachment 8796794 [details]
Bug 1298774 - Part 3: Copy helper functions from FragmentOrURL to URLValueData.

https://reviewboard.mozilla.org/r/82530/#review81236
Attachment #8796794 - Flags: review?(cku) → review+
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");
Attachment #8796796 - Flags: review?(cku) → review+
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
Attachment #8796795 - Flags: review?(cku) → review+
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
Attachment #8796797 - Flags: review?(cku) → review+
Comment on attachment 8796800 [details]
Bug 1298774 - Part 9: Remove FragmentOrURL.

https://reviewboard.mozilla.org/r/82542/#review81300
Attachment #8796800 - Flags: review?(cku) → review+
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.
Attachment #8796798 - Flags: review?(cku) → review+
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?
Attachment #8796799 - Flags: review?(cku) → review+
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
Blocks: 1288302
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.
Flags: needinfo?(cam)
No, relative URLs shouldn't have been broken.  I would be happy to investigate if/when you have further details.
Flags: needinfo?(cam)
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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: