1448763 - Remove nsCSSValue::AppendToString and some other unused stuff in nsCSSValue

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Xidorn Quan [:xidorn] UTC+10]

That is quite a bit of unused code now.

The only remaining uses as far as I can see are serialization of descriptors of @counter-style and @font-face. We should replace them with Servo serialization or just move the needed code to those files directly rather than keeping the huge serialization code.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

So there are two remaining references to the serialization code:
* nsComputedDOMStyle::DoGetAlignContent uses nsCSSValue::AppendAlignJustifyValueToString
* DOMIntersectionObserver::GetRootMargin uses nsCSSRect::AppendToString

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1448763 part 1 - Make DOMIntersectionObserver use nsStyleSides for mRootMargin, and use Servo code to serialize it.

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

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1448763 part 2 - Remove serialization code for specified value.

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

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1448763 part 3 - Remove nsCSSValueTokenStream.

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

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1448763 part 4 - Remove nsCSSValueGradient and its friends.

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

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1448763 part 5 - Remove all color stuff from nsCSSValue.

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

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1448763 part 6 - Remove nsCSSValueTriplet.

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

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1448763 part 7 - Remove nsCSSCornerSizes.

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

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1448763 part 8 - Remove CSSCalc.h.

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

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1448763 part 9 - Remove nsCSSRect.

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

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

I believe majority of units and methods of nsCSSValue can be removed nowadays, but that need some more time to investigate.

I guess it probably makes more sense to remove the whole nsCSSValue at some point when we can drop all dependencies to it.

Comment 12

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8965211 [details]
Bug 1448763 part 1 - Make DOMIntersectionObserver use nsStyleSides for mRootMargin, and use Servo code to serialize it.

https://reviewboard.mozilla.org/r/233912/#review239554

::: dom/base/DOMIntersectionObserver.cpp:125
(Diff revision 1)
>  }
>  
>  void
>  DOMIntersectionObserver::GetRootMargin(mozilla::dom::DOMString& aRetVal)
>  {
> -  mRootMargin.AppendToString(eCSSProperty_DOM, aRetVal);
> +  nsString& retVal = aRetVal;

Huh, is this needed? I'd expect &aRetVal to just work.

Comment 13

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8965212 [details]
Bug 1448763 part 2 - Remove serialization code for specified value.

https://reviewboard.mozilla.org/r/233914/#review239556

Nice!

Comment 14

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8965213 [details]
Bug 1448763 part 3 - Remove nsCSSValueTokenStream.

https://reviewboard.mozilla.org/r/233916/#review239558

Comment 15

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8965214 [details]
Bug 1448763 part 4 - Remove nsCSSValueGradient and its friends.

https://reviewboard.mozilla.org/r/233918/#review239560

Comment 16

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8965215 [details]
Bug 1448763 part 5 - Remove all color stuff from nsCSSValue.

https://reviewboard.mozilla.org/r/233920/#review239562

Comment 17

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8965216 [details]
Bug 1448763 part 6 - Remove nsCSSValueTriplet.

https://reviewboard.mozilla.org/r/233922/#review239564

Comment 18

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8965217 [details]
Bug 1448763 part 7 - Remove nsCSSCornerSizes.

https://reviewboard.mozilla.org/r/233924/#review239566

Comment 19

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8965218 [details]
Bug 1448763 part 8 - Remove CSSCalc.h.

https://reviewboard.mozilla.org/r/233926/#review239568

Comment 20

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8965219 [details]
Bug 1448763 part 9 - Remove nsCSSRect.

https://reviewboard.mozilla.org/r/233928/#review239570

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8965211 [details]
Bug 1448763 part 1 - Make DOMIntersectionObserver use nsStyleSides for mRootMargin, and use Servo code to serialize it.

https://reviewboard.mozilla.org/r/233912/#review239554

> Huh, is this needed? I'd expect &aRetVal to just work.

Yes, it is needed, it cannot automatically convert a `DOMString*` to `nsAString*`, we need a semi-explicit conversion.

Comment 22

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Servo PR

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d746dd4fe0e8
part 1 - Make DOMIntersectionObserver use nsStyleSides for mRootMargin, and use Servo code to serialize it. r=emilio
https://hg.mozilla.org/integration/autoland/rev/ce6f0d98da1f
part 2 - Remove serialization code for specified value. r=emilio
https://hg.mozilla.org/integration/autoland/rev/64aab4d047bd
part 3 - Remove nsCSSValueTokenStream. r=emilio
https://hg.mozilla.org/integration/autoland/rev/9c43f7ce61e1
part 4 - Remove nsCSSValueGradient and its friends. r=emilio
https://hg.mozilla.org/integration/autoland/rev/1754cae1e273
part 5 - Remove all color stuff from nsCSSValue. r=emilio
https://hg.mozilla.org/integration/autoland/rev/e56285e2657c
part 6 - Remove nsCSSValueTriplet. r=emilio
https://hg.mozilla.org/integration/autoland/rev/f43518ccb6f9
part 7 - Remove nsCSSCornerSizes. r=emilio
https://hg.mozilla.org/integration/autoland/rev/c44d6b666b2f
part 8 - Remove CSSCalc.h. r=emilio
https://hg.mozilla.org/integration/autoland/rev/7c0093bf53b5
part 9 - Remove nsCSSRect. r=emilio

Comment 24

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/d746dd4fe0e8
https://hg.mozilla.org/mozilla-central/rev/ce6f0d98da1f
https://hg.mozilla.org/mozilla-central/rev/64aab4d047bd
https://hg.mozilla.org/mozilla-central/rev/9c43f7ce61e1
https://hg.mozilla.org/mozilla-central/rev/1754cae1e273
https://hg.mozilla.org/mozilla-central/rev/e56285e2657c
https://hg.mozilla.org/mozilla-central/rev/f43518ccb6f9
https://hg.mozilla.org/mozilla-central/rev/c44d6b666b2f
https://hg.mozilla.org/mozilla-central/rev/7c0093bf53b5