Closed Bug 1296186 Opened 8 years ago Closed 8 years ago

stylo: Make ServoDeclarationBlock refcounted

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(2 files)

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 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 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 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 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 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)
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
https://hg.mozilla.org/mozilla-central/rev/8c7a0cfd5ae1
https://hg.mozilla.org/mozilla-central/rev/db66eec34981
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: