1461288 - Distinguish between specified and computed URLs.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1461288: Distinguish between specified and computed URLs.

This is needed to serialize computed URLs correctly from getComputedStyle.

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

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8975444 [details]
Bug 1461288: Distinguish between specified and computed URLs.

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

Comment 3

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8975444 [details]
Bug 1461288: Distinguish between specified and computed URLs.

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

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8975444 [details]
Bug 1461288: Distinguish between specified and computed URLs.

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

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8975444 [details]
Bug 1461288: Distinguish between specified and computed URLs.

https://reviewboard.mozilla.org/r/243732/#review249844

::: layout/style/ServoBindings.cpp:2140
(Diff revision 4)
> +  MOZ_ASSERT(aURL);
> +  MOZ_ASSERT(aOut);
> +  if (aURL->IsLocalRef()) {
> +    aOut->Assign(aURL->GetString());
> +    return;
> +  }
> +  if (nsIURI* uri = aURL->GetURI()) {
> +    nsresult rv = uri->GetSpec(*aOut);
> +    if (NS_SUCCEEDED(rv)) {
> +      return;
> +    }
> +  }

Can we just update `URLValueData::GetSourceString` to take a `nsCString` instead (and move the conversion into its only caller `nsComputedDOMStyle::SetValueToURLValue`) so that we can reuse existing code?

::: servo/components/style/gecko/url.rs:218
(Diff revision 4)
>      /// Convert from nsStyleImageRequest to SpecifiedUrl.
>      pub unsafe fn from_image_request(image_request: &nsStyleImageRequest) -> Result<Self, ()> {
>          if image_request.mImageValue.mRawPtr.is_null() {
>              return Err(());
>          }
>  
>          let image_value = image_request.mImageValue.mRawPtr.as_ref().unwrap();
>          let url_value_data = &image_value._base;
>          Self::from_url_value_data(url_value_data)
>      }

This looks quite inefficient... We probably should just feed the `mImageValue` into `image_value` field rather than using `from_css_url` to create a new one. And it seems to me `from_url_value_data` shouldn't be needed at all.

This should probably be done in a separate bug.

::: servo/components/style/gecko/url.rs:318
(Diff revision 4)
> +impl ToAnimatedValue for ComputedUrl {
> +    type AnimatedValue = Self;
> +
> +    fn to_animated_value(self) -> Self { self }
> +    fn from_animated_value(v: Self) -> Self { v }
> +}

You can use `trivial_to_animated_value!` for this. Actually, you probably just want to update the lines in animated mod rather than doing it here: https://searchfox.org/mozilla-central/rev/2b9779c59390ecc47be7a70d99753653d8eb5afc/servo/components/style/values/animated/mod.rs#263-265

::: servo/components/style/gecko/url.rs:326
(Diff revision 4)
> +    /// Convert from URLValueData to ComputedUrl.
> +    pub unsafe fn from_url_value_data(url: &URLValueData) -> Result<Self, ()> {
> +        Ok(ComputedUrl(SpecifiedUrl::from_url_value_data(url)?))
> +    }

It's probably better moving the function body of this method from `SpecifiedUrl` to here. I don't see why the specified value would need this if they are no longer computed value at the same time.

::: servo/components/style/gecko/url.rs:346
(Diff revision 4)
> +    /// Convert from URLValueData to ComputedImageUrl.
> +    pub unsafe fn from_url_value_data(url: &URLValueData) -> Result<Self, ()> {
> +        Ok(ComputedImageUrl(SpecifiedImageUrl::from_url_value_data(url)?))
> +    }
> +
> +    /// Convert from nsStyleImageReques to ComputedImageUrl.
> +    pub unsafe fn from_image_request(req: &nsStyleImageRequest) -> Result<Self, ()> {
> +        Ok(ComputedImageUrl(SpecifiedImageUrl::from_image_request(req)?))
> +    }

Ditto.

Comment 6

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8975444 [details]
Bug 1461288: Distinguish between specified and computed URLs.

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

Comment 7

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8975444 [details]
Bug 1461288: Distinguish between specified and computed URLs.

https://reviewboard.mozilla.org/r/243732/#review249844

> Can we just update `URLValueData::GetSourceString` to take a `nsCString` instead (and move the conversion into its only caller `nsComputedDOMStyle::SetValueToURLValue`) so that we can reuse existing code?

Hmm, not quite, URL serialization in the old style system also happens in nsROCSSPrimitiveValue.

I'll keep it as-is for now since I don't think there's too much duplication, and that code will go away soon.

> This looks quite inefficient... We probably should just feed the `mImageValue` into `image_value` field rather than using `from_css_url` to create a new one. And it seems to me `from_url_value_data` shouldn't be needed at all.
> 
> This should probably be done in a separate bug.

Yeah indeed.

> You can use `trivial_to_animated_value!` for this. Actually, you probably just want to update the lines in animated mod rather than doing it here: https://searchfox.org/mozilla-central/rev/2b9779c59390ecc47be7a70d99753653d8eb5afc/servo/components/style/values/animated/mod.rs#263-265

I missed that one, thanks!

> It's probably better moving the function body of this method from `SpecifiedUrl` to here. I don't see why the specified value would need this if they are no longer computed value at the same time.

I did that.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/67553a1262de
Distinguish between specified and computed URLs. r=xidorn

Comment 9

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/67553a1262de