Closed Bug 1310560 Opened 8 years ago Closed 8 years ago

stylo: support url() values in cursor

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(3 files, 2 obsolete files)

Bug 1310560 - Part 1: Make cursor use nsStyleImageRequest for url() value storage.
58 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1310560 - Part 2: Remove unused EqualImages and EqualURIs functions.
58 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1310560 - Part 3: Add FFI functions for setting cursor url() lists.
58 bytes, text/x-review-board-request
xidorn
: review+
Details 
      No description provided.
Comment on attachment 8801672 [details]
Bug 1310560 - Part 1: Make cursor use nsStyleImageRequest for url() value storage.

https://reviewboard.mozilla.org/r/86352/#review85014

r=me with the following issue addressed.

::: layout/style/nsRuleNode.cpp:5093
(Diff revision 1)
> -        imgRequestProxy* req =
> +        SetStyleImageRequest([&](nsStyleImageRequest* req) {
> -          GetImageRequest(aContext->PresContext(), arr->Item(0));
> -        if (req) {
>            nsCursorImage* item = ui->mCursorImages.AppendElement();
> -          item->SetImage(req);
> +          item->mImage = req;
>            if (arr->Item(1).GetUnit() != eCSSUnit_Null) {
>              item->mHaveHotspot = true;
>              item->mHotspotX = arr->Item(1).GetFloatValue();
>              item->mHotspotY = arr->Item(2).GetFloatValue();
>            }
> +        }, aContext->PresContext(), arr->Item(0));

It seems you shouldn't use default flags for cursor images.
Attachment #8801672 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8801676 [details]
Support cursor property url() values in stylo.

https://reviewboard.mozilla.org/r/86360/#review85018

::: servo/components/style/properties/longhand/pointing.mako.rs:109
(Diff revision 1)
> +            images: None,
> +            keyword: computed_value::Keyword::AutoCursor
> +        }
> +    }
> +
> +    fn parse_keyword(input: &mut Parser) -> Result<computed_value::Keyword, ()> {

Implement this as `Parse` on `Keyword`

::: servo/components/style/properties/longhand/pointing.mako.rs:122
(Diff revision 1)
> -            .map(SpecifiedValue::SpecifiedCursor)
> +            .map(computed_value::Keyword::SpecifiedCursor)
>          }
>      }
> +
> +    #[cfg(feature = "gecko")]
> +    fn parse_image(context: &ParserContext, input: &mut Parser) -> Result<computed_value::Image, ()> {

Implement this as `Parse` on `Image`?
Comment on attachment 8801673 [details]
Bug 1310560 - Part 2: Remove unused EqualImages and EqualURIs functions.

https://reviewboard.mozilla.org/r/86354/#review85024
Attachment #8801673 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8801674 [details]
Bug 1310560 - Part 3: Add FFI functions for setting cursor url() lists.

https://reviewboard.mozilla.org/r/86356/#review85026
Attachment #8801674 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8801672 [details]
Bug 1310560 - Part 1: Make cursor use nsStyleImageRequest for url() value storage.

https://reviewboard.mozilla.org/r/86352/#review85014

> It seems you shouldn't use default flags for cursor images.

Oh, yes.  The evils of default argument values, which maybe I shouldn't have used. :(
Comment on attachment 8801676 [details]
Support cursor property url() values in stylo.

https://reviewboard.mozilla.org/r/86360/#review85018

> Implement this as `Parse` on `Image`?

I guess this one can't be, since it needs the ParserContext.
Comment on attachment 8801676 [details]
Support cursor property url() values in stylo.

https://reviewboard.mozilla.org/r/86360/#review85022
Attachment #8801676 - Flags: review?(manishearth) → review+
Attachment #8801675 - Attachment is obsolete: true
Attachment #8801676 - Attachment is obsolete: true
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7af6d1b072ddc950cbbd6d76603e03f779c03d8a

I'm making some changes to Part 1 for serializing cursor url() values.  Now we will use the specified string values in the URLValueData if we don't have a usable nsIURI to serialize.  I'll make similar changes for other properties in a later bug (which will address the XXX comments I've added recently for other nsStyleImageRequest serializations).
Attachment #8801672 - Flags: review+ → review?(xidorn+moz)
Comment on attachment 8801672 [details]
Bug 1310560 - Part 1: Make cursor use nsStyleImageRequest for url() value storage.

https://reviewboard.mozilla.org/r/86352/#review90646

r=me with the issues addressed.

::: layout/style/nsComputedDOMStyle.cpp:2417
(Diff revision 4)
> +      return;
>      }
>    }
> +
> +  // Otherwise, serialize the specified URL value.
> +  nsString source;

Better using nsAutoString.

::: layout/style/nsComputedDOMStyle.cpp:2420
(Diff revision 4)
> +
> +  // Otherwise, serialize the specified URL value.
> +  nsString source;
> +  aURL->GetSourceString(source);
> +
> +  nsString url;

Same here. It seems nsROCSSPrimitiveValue::SetString doesn't actually reuse the string buffer.

::: layout/style/nsComputedDOMStyle.cpp:2422
(Diff revision 4)
> +  nsString source;
> +  aURL->GetSourceString(source);
> +
> +  nsString url;
> +  url.AppendLiteral(u"url(\"");
> +  url.Append(source);

You probably should use nsStyleUtil::AppendEscapedCSSString() instead of appending it directly.
Attachment #8801672 - Flags: review?(xidorn+moz) → review+
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dfd05cb27d66
Part 1: Make cursor use nsStyleImageRequest for url() value storage. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/fe6a89edfd16
Part 2: Remove unused EqualImages and EqualURIs functions. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/2b664591c407
Part 3: Add FFI functions for setting cursor url() lists. r=xidorn
https://hg.mozilla.org/mozilla-central/rev/dfd05cb27d66
https://hg.mozilla.org/mozilla-central/rev/fe6a89edfd16
https://hg.mozilla.org/mozilla-central/rev/2b664591c407
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Depends on: 1320423
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: