Closed Bug 1310560 Opened 9 years ago Closed 9 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: 9 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: