Closed Bug 1461288 Opened 6 years ago Closed 6 years ago

Distinguish between specified and computed URLs.

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1461288: Distinguish between specified and computed URLs.
59 bytes, text/x-review-board-request
xidorn
: review+
Details 
      No description provided.
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.
Attachment #8975444 - Flags: review?(xidorn+moz) → review+
Priority: -- → P3
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.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/67553a1262de
Distinguish between specified and computed URLs. r=xidorn
https://hg.mozilla.org/mozilla-central/rev/67553a1262de
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: