1459871 - Remove other getPropertyCssValue-related interfaces.

Status: RESOLVED FIXED

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

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1459871: Remove other getPropertyCSSValue-related interfaces.

I can land the removal behind a pref first if you want and all that instead.

Again, this doesn't remove the internal usage for getComputedStyle (yet).

Review commit: https://reviewboard.mozilla.org/r/242316/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/242316/

Comment 2

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8d91bd3025bcf867a0165cd56d388a5fdbf020f

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/66fa28c2f438
Remove other getPropertyCSSValue-related interfaces. r=bz

Comment 6

[Kohei Yoshino]

Updated the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2018/cssstyledeclaration-getpropertycssvalue-and-related-interfaces-have-been-removed/

Comment 7

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/66fa28c2f438

Comment 8

[Sam Sneddon [:gsnedders]]

:emilio, can you add some tests to wpt/css/cssom/historical.html for these, especially if WebKit still implements them?

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Sure, I can try to get to it tomorrow :)

Comment 10

[Emilio Cobos Álvarez (: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

Comment 11

[Will Bamberg [:wbamberg]]

I've updated compat status for:
https://developer.mozilla.org/en-US/docs/Web/API/CSSValue#Browser_compatibility
https://developer.mozilla.org/en-US/docs/Web/API/CSSPrimitiveValue#Browser_compatibility
https://developer.mozilla.org/en-US/docs/Web/API/CSSValueList#Browser_compatibility

Marking dev-doc-complete, but please let me know if we need anything else.