1461858 - Simplify code around building ComputedImageUrl

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Xidorn Quan [:xidorn] UTC+10]

See bug 1461288 comment 5, there are something inefficient, and something can be removed.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1461858 part 1 - Make creating CssUrl infallible.

There were a check in CssUrl::parse_from_string for extra data, which
was removed as part of servo/servo#16241, so it never fails now.

CssUrl::from_url_value_data doesn't seem to need Result from the very
beginning. It is unclear why it was made that way.

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

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1461858 part 2 - Make from_image_request infallible.

All callsites already assert, so moving the assertion into the method
should be fine. It is not expected to handle a null image value anyway.

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

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1461858 part 3 - Have from_image_request reuse ImageValue from image request directly.

And also remove ComputedImageUrl::from_url_value_data.

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

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1461858 part 4 - Rename from_url_value_data to from_url_value and reuse URLValue passed in for ComputedUrl.

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

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8976035 [details]
Bug 1461858 part 1 - Make creating CssUrl infallible.

https://reviewboard.mozilla.org/r/244230/#review250210

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8976036 [details]
Bug 1461858 part 2 - Make from_image_request infallible.

https://reviewboard.mozilla.org/r/244232/#review250212

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8976037 [details]
Bug 1461858 part 3 - Have from_image_request reuse ImageValue from image request directly.

https://reviewboard.mozilla.org/r/244234/#review250214

::: layout/style/ServoBindings.cpp:1675
(Diff revision 1)
>  
> -const mozilla::css::URLValueData*
> -Gecko_GetURLValue(const nsStyleImage* aImage)
> +const nsStyleImageRequest*
> +Gecko_GetImageRequest(const nsStyleImage* aImage)
>  {
> -  MOZ_ASSERT(aImage && aImage->GetType() == eStyleImageType_Image);
> -  return aImage->GetURLValue();
> +  MOZ_ASSERT(aImage);
> +  return aImage->GetImageRequest();

We should probably rename GetImageRequest() to ImageRequest(), since it can't return null really.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8976038 [details]
Bug 1461858 part 4 - Rename from_url_value_data to from_url_value and reuse URLValue passed in for ComputedUrl.

https://reviewboard.mozilla.org/r/244236/#review250216

Thanks for cleaning this up! :)

::: servo/components/style/gecko/conversions.rs:682
(Diff revision 1)
>      impl<'a> From<&'a StyleShapeSource> for ClippingShape {
>          fn from(other: &'a StyleShapeSource) -> Self {
>              match other.mType {
>                  StyleShapeSourceType::URL => unsafe {
>                      let shape_image = &*other.mShapeImage.mPtr;
> -                    let other_url = &(**shape_image.__bindgen_anon_1.mURLValue.as_ref());
> +                    let other_url = RefPtr::from_ptr_ref(shape_image.__bindgen_anon_1.mURLValue.as_ref());

Huh, naming of from_ptr_ref is really confusing...

Anyhow this looks fine.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8976038 [details]
Bug 1461858 part 4 - Rename from_url_value_data to from_url_value and reuse URLValue passed in for ComputedUrl.

https://reviewboard.mozilla.org/r/244236/#review250216

> Huh, naming of from_ptr_ref is really confusing...
> 
> Anyhow this looks fine.

Perhaps you can use just RefPtr::new(), instead of from_ptr_ref + clone? It'd be slightly easier to read imo.

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8976037 [details]
Bug 1461858 part 3 - Have from_image_request reuse ImageValue from image request directly.

https://reviewboard.mozilla.org/r/244234/#review250214

> We should probably rename GetImageRequest() to ImageRequest(), since it can't return null really.

Let's do it in a followup bug. It seems to me that nsStyleImageRequest also has some method can be renamed.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7358416f54f4
part 1 - Make creating CssUrl infallible. r=emilio
https://hg.mozilla.org/integration/autoland/rev/c728ab64fa12
part 2 - Make from_image_request infallible. r=emilio
https://hg.mozilla.org/integration/autoland/rev/ae886b67a96f
part 3 - Have from_image_request reuse ImageValue from image request directly. r=emilio
https://hg.mozilla.org/integration/autoland/rev/2e6a54985ec6
part 4 - Rename from_url_value_data to from_url_value and reuse URLValue passed in for ComputedUrl. r=emilio

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8976038 [details]
Bug 1461858 part 4 - Rename from_url_value_data to from_url_value and reuse URLValue passed in for ComputedUrl.

https://reviewboard.mozilla.org/r/244236/#review250216

> Perhaps you can use just RefPtr::new(), instead of from_ptr_ref + clone? It'd be slightly easier to read imo.

Oh... I missed this issue. So yeah, I agree. It is written this way because I initially have from_url_value accept `&RefPtr<URLValue>`, then I realized that `to_safe()` actually clones, thus I change that function to take ownership instead, and at that point, I just add clone everywhere... but yeah, using `::new` should be better. I'll push a followup.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/235b702765bb
followup - Use RefPtr::new instead of RefPtr::from_ptr_ref + clone.

Comment 14

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/7358416f54f4
https://hg.mozilla.org/mozilla-central/rev/c728ab64fa12
https://hg.mozilla.org/mozilla-central/rev/ae886b67a96f
https://hg.mozilla.org/mozilla-central/rev/2e6a54985ec6
https://hg.mozilla.org/mozilla-central/rev/235b702765bb