Remove other getPropertyCssValue-related interfaces.

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P3
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: emilio, Assigned: emilio)

Tracking

({dev-doc-complete, site-compat})

unspecified
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment)

No description provided.
Priority: -- → P3
Comment on attachment 8973969 [details]
Bug 1459871: Remove other getPropertyCSSValue-related interfaces.

https://reviewboard.mozilla.org/r/242316/#review249812

::: layout/style/nsDOMCSSRGBColor.h
(Diff revision 1)
>                     nsROCSSPrimitiveValue* aGreen,
>                     nsROCSSPrimitiveValue* aBlue,
>                     nsROCSSPrimitiveValue* aAlpha,
>                     bool aHasAlpha);
>  
> -  NS_INLINE_DECL_CYCLE_COLLECTING_NATIVE_REFCOUNTING(nsDOMCSSRGBColor)

So... In theory, we could in fact have cycles through our members.  It's probably OK as long as people don't mess up and have those nsROCSSPrimitiveValue instances point to this object.  But it slightly worries me to remove the cycle-collection here.

::: layout/style/nsDOMCSSRect.h
(Diff revision 1)
>    nsDOMCSSRect(nsROCSSPrimitiveValue* aTop,
>                 nsROCSSPrimitiveValue* aRight,
>                 nsROCSSPrimitiveValue* aBottom,
>                 nsROCSSPrimitiveValue* aLeft);
>  
> -  NS_DECL_CYCLE_COLLECTING_ISUPPORTS

Again, removing the cycle collection here has some risk.

::: layout/style/nsDOMCSSValueList.h
(Diff revision 1)
>  #include "nsTArray.h"
>  
>  class nsDOMCSSValueList final : public mozilla::dom::CSSValue
>  {
>  public:
> -  NS_DECL_CYCLE_COLLECTING_ISUPPORTS

And here.

::: layout/style/nsROCSSPrimitiveValue.h:28
(Diff revision 1)
>   * computed style.
>   */
>  class nsROCSSPrimitiveValue final : public mozilla::dom::CSSValue
>  {
>  public:
> -  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  enum : uint16_t {

Again, removing cycle collection here is a bit worrying.
Attachment #8973969 - Flags: review?(bzbarsky) → review+
I thought about it a bit.

All the usage of these types is relegated to getComputedStyle, which is to say, we only create them and call GetCssText on them.

If you create a cycle manually using C++ you overflow the stack on GetCssText, so I think it's reasonably safe to remove the cycle collection bits.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/66fa28c2f438
Remove other getPropertyCSSValue-related interfaces. r=bz
https://hg.mozilla.org/mozilla-central/rev/66fa28c2f438
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
:emilio, can you add some tests to wpt/css/cssom/historical.html for these, especially if WebKit still implements them?
Sure, I can try to get to it tomorrow :)
Flags: needinfo?(emilio)
(In reply to Geoffrey Sneddon [:gsnedders] from comment #8)
> :emilio, can you add some tests to wpt/css/cssom/historical.html for these,
> especially if WebKit still implements them?

https://github.com/w3c/web-platform-tests/pull/11163
Flags: needinfo?(emilio)
You need to log in before you can comment on or make changes to this bug.