1383296 - Remove the nsCSSValue::Serialization enum and remove decision logic in functions that use it

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Brad Werth [:bradwerth]]

If eAuthorSpecified is removed in Bug 1302513, then the only remaining value for nsCSSValue::Serialization will be eNormalized. Unless we want to maintain this enum for future serialization options, we can remove this parameter in all functions that use it, and the corresponding logic.

Comment 1

[Brad Werth [:bradwerth]]

Any reason to keep this code with only one value in the enum?

Comment 2

[Daniel Holbert [:dholbert]]

Superficial answer: doesn't seem like it.

(For a non-superficial answer, I'd suggest looking up the history of the enum using "hg blame" on e.g. searchfox.org, to see when/why it was added, to verify that its reasons to exist [back then] will now be gone.  It's possible that might might turn up additional simplifications that could be made at the same time, too.)

Comment 3

[Brad Werth [:bradwerth]]

Cameron, fully removing serialization enum would undo much of the work in Bug 731271. Do you feel that we should retain the serialization parameter (now with only one value in the enum)?

Comment 4

[Brad Werth [:bradwerth]]

Attachment: Bug 1383296 Part 1: Remove all uses of the nsCSSValue::Serialization enum, now that it no longer has multiple values.

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

Comment 5

[Cameron McCormack (:heycam)]

Removing the enum sounds like the right thing to do, thank you!

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8888999 [details]
Bug 1383296 Part 1: Remove all uses of the nsCSSValue::Serialization enum, now that it no longer has multiple values.

https://reviewboard.mozilla.org/r/160034/#review165564

Thanks for cleaning this up!

::: layout/style/nsCSSValue.h:995
(Diff revision 1)
>    void AppendBasicShapePositionToString(
> -           nsAString& aResult,
> +           nsAString& aResult) const;

Nit: this could probably fit on one line now.

Comment 7

[Brad Werth [:bradwerth]]

Comment on attachment 8888999 [details]
Bug 1383296 Part 1: Remove all uses of the nsCSSValue::Serialization enum, now that it no longer has multiple values.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160034/diff/1-2/

Comment 8

[Brad Werth [:bradwerth]]

Comment on attachment 8888999 [details]
Bug 1383296 Part 1: Remove all uses of the nsCSSValue::Serialization enum, now that it no longer has multiple values.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160034/diff/2-3/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/acf2dcc2c76a
Part 1: Remove all uses of the nsCSSValue::Serialization enum, now that it no longer has multiple values. r=heycam

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/acf2dcc2c76a