Closed Bug 1310463 Opened 7 years ago Closed 7 years ago

support list-style-image in stylo

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:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(5 files)

Bug 1310463 - Part 1: Make list-style-image use nsStyleImageRequest for storage.
58 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1310463 - Part 2: Simplify nsComputedDOMStyle::DoGetListStyleImage a little.
58 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1310463 - Part 3: Add FFI functions for setting list-style-image.
58 bytes, text/x-review-board-request
xidorn
: review+
Details
Regenerate bindings.
58 bytes, text/x-review-board-request
Details
Support list-style-image in stylo.
58 bytes, text/x-review-board-request
manishearth
: review+
Details 
      No description provided.
Comment on attachment 8801559 [details]
Support list-style-image in stylo.

https://reviewboard.mozilla.org/r/86262/#review84906

::: servo/components/style/properties/gecko.mako.rs:1376
(Diff revision 1)
> +    pub fn set_list_style_image(&mut self, image: longhands::list_style_image::computed_value::T) {
> +        use values::computed::UrlOrNone;
> +        match image {
> +            UrlOrNone::None => {
> +                unsafe {
> +                    Gecko_SetListStyleImageNone(&mut self.gecko);

We probably should just have a function that lets us decref an image request and set it to null.

The samge goes for setting the image, it's better if there's a single function that's used by all.

Probably makes more sense as a pass in my FFI bindings work. Changes are fine as is, but once these are all done I'll try to refactor it a bit to reduce the number of FFI functions.
Comment on attachment 8801559 [details]
Support list-style-image in stylo.

https://reviewboard.mozilla.org/r/86262/#review84908
Attachment #8801559 - Flags: review?(manishearth) → review+
Comment on attachment 8801559 [details]
Support list-style-image in stylo.

https://reviewboard.mozilla.org/r/86262/#review84906

> We probably should just have a function that lets us decref an image request and set it to null.
> 
> The samge goes for setting the image, it's better if there's a single function that's used by all.
> 
> Probably makes more sense as a pass in my FFI bindings work. Changes are fine as is, but once these are all done I'll try to refactor it a bit to reduce the number of FFI functions.

Yeah, it seems we could simplify things by just having an FFI function that produces a new nsStyleImageRequest object, and let the rust code do all the refcounting and setting.  Happy to leave that for you to look at later.
Comment on attachment 8801555 [details]
Bug 1310463 - Part 1: Make list-style-image use nsStyleImageRequest for storage.

https://reviewboard.mozilla.org/r/86254/#review84952
Attachment #8801555 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8801556 [details]
Bug 1310463 - Part 2: Simplify nsComputedDOMStyle::DoGetListStyleImage a little.

https://reviewboard.mozilla.org/r/86256/#review84954

::: layout/style/nsComputedDOMStyle.cpp:3538
(Diff revision 1)
> -  if (!list->GetListStyleImage()) {
> +  // XXXheycam As in SetValueToStyleImage, we might want to use the
> +  // URL stored in the nsStyleImageRequest's mImageValue if we
> +  // failed to resolve the imgRequestProxy.

Probably also add a comment to SetValueToStyleImage stating that if that gets fixed, this function should be fixed in the same way.
Attachment #8801556 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8801557 [details]
Bug 1310463 - Part 3: Add FFI functions for setting list-style-image.

https://reviewboard.mozilla.org/r/86258/#review84964
Attachment #8801557 - Flags: review?(xidorn+moz) → review+
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/869787233418
Part 1: Make list-style-image use nsStyleImageRequest for storage. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/23bad27dfeb3
Part 2: Simplify nsComputedDOMStyle::DoGetListStyleImage a little. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ebcedea1fa5
Part 3: Add FFI functions for setting list-style-image. r=xidorn
https://hg.mozilla.org/mozilla-central/rev/869787233418
https://hg.mozilla.org/mozilla-central/rev/23bad27dfeb3
https://hg.mozilla.org/mozilla-central/rev/5ebcedea1fa5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Backed out for being on top of 1288302, which I backed out for the bug 1311921 Talos regressions: https://hg.mozilla.org/integration/mozilla-inbound/rev/7fd0f297a13d
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fd7c7cad759
Part 1: Make list-style-image use nsStyleImageRequest for storage. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/0520d9787931
Part 2: Simplify nsComputedDOMStyle::DoGetListStyleImage a little. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/be398e3e2b20
Part 3: Add FFI functions for setting list-style-image. r=xidorn
You need to log in before you can comment on or make changes to this bug.