1352096 - Remove nsStyleImageLayers::Layer::mSourceURI

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[u459114]

1. Remove this data member can reduce change need at stylo side in bug 1341667.
2. We can actually store url information right in nsStyleImage

Comment 1

[u459114]

Attachment: Bug 1352096 - Part 1. Implement nsStyleImage::SetURL and GetURLValueData.

Prerequisites of removing Layer::mSourceURI.

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

Comment 2

[u459114]

Attachment: Bug 1352096 - Part 2. Remove Layer::mSourceURI.

With Part 1, we can actually store url right in nsStyleImage. So
Layer::mSourceURI is useless now.

There are three advantages to remove Layer::mSourceURI
1. Redue the size of nsStyleImage::Layer. In part 1, we store url in
nsStyleImage's union, which means the size of nsStyleImage does not increase.
2. Since style image and url information are all stored in nsStyleImage, many
verbose comments can be removed. Since we do not need to explain why we use
mSourceURI here, or why we use nsStyleImage there.
3. Since all inforamtion store in on place, nsStyleImage, we only need to call
Gecko_SetUrlImageValue at stylo side.

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

Comment 3

[u459114]

Comment on attachment 8853001 [details]
Bug 1352096 - Part 2. Remove Layer::mSourceURI.

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

Comment 4

[u459114]

Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee5aad506e775724c101c11cd91de969ee654e34

Comment 5

[u459114]

Comment on attachment 8853000 [details]
Bug 1352096 - Part 1. Implement nsStyleImage::SetURL and GetURLValueData.

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

Comment 6

[u459114]

Comment on attachment 8853001 [details]
Bug 1352096 - Part 2. Remove Layer::mSourceURI.

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

Comment 7

[u459114]

Comment on attachment 8853001 [details]
Bug 1352096 - Part 2. Remove Layer::mSourceURI.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125124/diff/3-4/

Comment 8

[u459114]

Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd9f94ab60ea7ffe63bac6ae0be2fb6ca0aebb2a

Comment 9

[u459114]

recook part 1 to make code even simpler

Comment 10

[u459114]

Comment on attachment 8853000 [details]
Bug 1352096 - Part 1. Implement nsStyleImage::SetURL and GetURLValueData.

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

Comment 11

[u459114]

Comment on attachment 8853001 [details]
Bug 1352096 - Part 2. Remove Layer::mSourceURI.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125124/diff/4-5/

Comment 12

[Cameron McCormack (:heycam)]

Comment on attachment 8853000 [details]
Bug 1352096 - Part 1. Implement nsStyleImage::SetURL and GetURLValueData.

https://reviewboard.mozilla.org/r/125122/#review130234

::: layout/style/nsStyleStruct.h:367
(Diff revision 3)
> -
> +  URLValueData* GetURLValueData() const {
> +    return mImageValue;
> +  }

We don't really need this function, since the caller can just use GetImageValue() instead.

::: layout/style/nsStyleStruct.h:396
(Diff revision 3)
>  enum nsStyleImageType {
>    eStyleImageType_Null,
>    eStyleImageType_Image,
>    eStyleImageType_Gradient,
> -  eStyleImageType_Element
> +  eStyleImageType_Element,
> +  eStyleImageType_Url

Nit: I would write this as "eStyleImageType_URL".  ("Url" is more of a Rust-like way to write abbreviations.)

::: layout/style/nsStyleStruct.h:441
(Diff revision 3)
>    void SetNull();
>    void SetImageRequest(already_AddRefed<nsStyleImageRequest> aImage);
>    void SetGradientData(nsStyleGradient* aGradient);
>    void SetElementId(const char16_t* aElementId);
>    void SetCropRect(mozilla::UniquePtr<nsStyleSides> aCropRect);
> +  void SetURLValueData(already_AddRefed<URLValueData> aData);

I suggest making this "SetURLValue(already_AddRefed<URLValue>)", since we only ever pass in a concrete css::URLValue object.

::: layout/style/nsStyleStruct.h:562
(Diff revision 3)
> +    URLValueData* mUrlData; // See the comment in SetStyleImage's 'case
> +                            // eCSSUnit_URL' section to know why we need to
> +                            // keep url other then mImage.

And then we can make this |URLValue* mURLValue|.

::: layout/style/nsStyleStruct.h:564
(Diff revision 3)
>    union {
>      nsStyleImageRequest* mImage;
>      nsStyleGradient* mGradient;
> +    URLValueData* mUrlData; // See the comment in SetStyleImage's 'case
> +                            // eCSSUnit_URL' section to know why we need to
> +                            // keep url other then mImage.

s/keep url other then/store URLValues separately from mImage/.

::: layout/style/nsStyleStruct.cpp:2442
(Diff revision 3)
> +    case eStyleImageType_Url:
>        return true;

I guess it is correct to always return true here (and in IsLoaded) because eStyleImageType_Url is only used for url() values that are fragment only or which refer to the same document, and so the document must always be loaded, right?  (I see that IsComplete is used in nsSVGIntegrationUtils::IsMaskResourceReady.)

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8853001 [details]
Bug 1352096 - Part 2. Remove Layer::mSourceURI.

https://reviewboard.mozilla.org/r/125124/#review130244

::: commit-message-0ceec:3
(Diff revision 5)
> +Bug 1352096 - Part 2. Remove Layer::mSourceURI.
> +
> +Now, remove Layer::mSourceURI, there are several benifit of doing this:

benefit

::: commit-message-0ceec:4
(Diff revision 5)
> +Bug 1352096 - Part 2. Remove Layer::mSourceURI.
> +
> +Now, remove Layer::mSourceURI, there are several benifit of doing this:
> +1. Redue the size of nsStyleImage::Layer.

Reduce

::: commit-message-0ceec:8
(Diff revision 5)
> +Now, remove Layer::mSourceURI, there are several benifit of doing this:
> +1. Redue the size of nsStyleImage::Layer.
> +2. By storing style image and url information in nsStyleImage, we can remove
> +many verbose comments. That is becasue there is no need to explain why we use
> +mSourceURI here, or why we use nsStyleImage there anymore.
> +3. Since all inforamtion store in on place, nsStyleImage, we can setup image

"all information is stored in one place"

::: commit-message-0ceec:9
(Diff revision 5)
> +1. Redue the size of nsStyleImage::Layer.
> +2. By storing style image and url information in nsStyleImage, we can remove
> +many verbose comments. That is becasue there is no need to explain why we use
> +mSourceURI here, or why we use nsStyleImage there anymore.
> +3. Since all inforamtion store in on place, nsStyleImage, we can setup image
> +request or ulr by one single Gecko_SetUrlImageValue call.

URLs

::: layout/style/nsStyleStruct.cpp:3011
(Diff revision 5)
> -         DefinitelyEqualURIs(mSourceURI, aOther.mSourceURI);
> +         DefinitelyEqualURIs(mImage.GetURLValueData(),
> +                             aOther.mImage.GetURLValueData());

We don't need this, since we compare mImage above.

Comment 14

[u459114]

Comment on attachment 8853000 [details]
Bug 1352096 - Part 1. Implement nsStyleImage::SetURL and GetURLValueData.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125122/diff/3-4/

Comment 15

[u459114]

Comment on attachment 8853001 [details]
Bug 1352096 - Part 2. Remove Layer::mSourceURI.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125124/diff/5-6/

Comment 16

[u459114]

Comment on attachment 8853001 [details]
Bug 1352096 - Part 2. Remove Layer::mSourceURI.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125124/diff/6-7/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1d1403165215
Part 1. Implement nsStyleImage::SetURL and GetURLValueData. r=heycam
https://hg.mozilla.org/integration/autoland/rev/39a3c3fbfabd
Part 2. Remove Layer::mSourceURI. r=heycam

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1d1403165215
https://hg.mozilla.org/mozilla-central/rev/39a3c3fbfabd