stylo: Make ServoDeclarationBlock refcounted

RESOLVED FIXED in Firefox 51

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

Trunk
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

css::Declaration is refcounted, so that multiple nsAttrValues can hold the same css::Declaration at the same time. The existing code path heavily relies on this, so having a sole owned ServoDeclarationBlock sharing the same path could be very dangerous. Thus we should create a refcounted wrapper for it and have it owned by nsAttrValue instead.
Summary: stylo: Create a refcounted wrapper for ServoDeclarationBlock → stylo: Make ServoDeclarationBlock refcounted
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8782784 [details]
Bug 1296186 part 1 - Make borrowed type just alias of corresponding pointer type.

I haven't looked at this patch yet, but Manish just implemented this stuff, so I'll let him weigh in here on the proposed changes before I do so.
Attachment #8782784 - Flags: feedback?(manishearth)

Comment 4

a year ago
mozreview-review
Comment on attachment 8782784 [details]
Bug 1296186 part 1 - Make borrowed type just alias of corresponding pointer type.

https://reviewboard.mozilla.org/r/72834/#review70886

::: layout/style/ServoBindings.h:59
(Diff revision 1)
>  class nsStyleCoord;
>  struct nsStyleDisplay;
>  struct ServoDeclarationBlock;
>  
> +#define DECL_REF_TYPE_FOR(type_)          \
> +  typedef type_* type_##Borrowed;         \

I had a new type because there's a bit of extra safety on the Rust side -- the bindings will show a different type rather than a pointer typedef, and you deal with that explicitly (by adding it to regen.py). But that's not much additional safety, so this is ok.
Comment on attachment 8782784 [details]
Bug 1296186 part 1 - Make borrowed type just alias of corresponding pointer type.

While this is slightly less explicit, I'm okay with this. It's also more interoperable, which is good.
Attachment #8782784 - Flags: feedback?(manishearth) → feedback+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 8

a year ago
mozreview-review-reply
Comment on attachment 8782784 [details]
Bug 1296186 part 1 - Make borrowed type just alias of corresponding pointer type.

https://reviewboard.mozilla.org/r/72834/#review70886

> I had a new type because there's a bit of extra safety on the Rust side -- the bindings will show a different type rather than a pointer typedef, and you deal with that explicitly (by adding it to regen.py). But that's not much additional safety, so this is ok.

As far as we give it a different name (even with typedef), we can deal with it explicitly on the Rust side as well. (I've made bindgen support blacklisting type alias.)

Comment 9

a year ago
mozreview-review
Comment on attachment 8782784 [details]
Bug 1296186 part 1 - Make borrowed type just alias of corresponding pointer type.

https://reviewboard.mozilla.org/r/72834/#review71122

Ok then.
Attachment #8782784 - Flags: review?(bobbyholley) → review+

Comment 10

a year ago
mozreview-review
Comment on attachment 8782785 [details]
Bug 1296186 part 2 - Make ServoDeclarationBlock refcounted.

https://reviewboard.mozilla.org/r/72836/#review71126

This one looks fine as well. Stealing review.
Attachment #8782785 - Flags: review+
Attachment #8782785 - Flags: review?(cam)

Comment 11

a year ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c7a0cfd5ae1
part 1 - Make borrowed type just alias of corresponding pointer type. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/db66eec34981
part 2 - Make ServoDeclarationBlock refcounted. r=heycam

Comment 12

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8c7a0cfd5ae1
https://hg.mozilla.org/mozilla-central/rev/db66eec34981
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.