Closed Bug 1459871 Opened 2 years ago Closed 2 years ago

Remove other getPropertyCssValue-related interfaces.

Categories

(Core :: CSS Parsing and Computation, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(1 file)

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: 2 years ago
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.