1370458 - Disallow floats, negative values, and values longer than six characters in the hashless quirk

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Manish Goregaokar [:manishearth]]

https://quirks.spec.whatwg.org/#the-hashless-hex-color-quirk

We were failing a bunch of WPT tests because of this.

Comment 1

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1370458: Disallow floats, negative numbers, and long values in hashless color quirk;

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

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8874718 [details]
Bug 1370458: Disallow floats, negative numbers, and long values in hashless color quirk;

https://reviewboard.mozilla.org/r/146096/#review150004

r=me with comments addressed.

::: layout/style/nsCSSParser.cpp:6700
(Diff revision 1)
> +static
> +int
> +numberSize(uint32_t number) {

* please put `static` and the return type in the same line
* the return type should probably be `uint32_t` rather than impl-dependent sized `int`
* function name should be a verb phase and it should start with uppercase, and this seems to be very specific to the color case so probably `CountNumbersForHashlessColor`.

::: layout/style/nsCSSParser.cpp:6718
(Diff revision 1)
> +  } else if (number < 100000) {
> +    return 5;
> +  } else if (number < 1000000) {
> +    return 6;
> +  } else {
> +    return 100;

Probably add a comment stating that this is the wrong case, and we don't care the specific number as far as it is larger than 6.

Comment 3

[Manish Goregaokar [:manishearth]]

Comment on attachment 8874718 [details]
Bug 1370458: Disallow floats, negative numbers, and long values in hashless color quirk;

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

Comment 4

[Mats Palmgren (inactive)]

> +numberSize(uint32_t number) {

The param should be named aNumber to follow our naming conventions.

Comment 5

[Manish Goregaokar [:manishearth]]

Try was at https://treeherder.mozilla.org/#/jobs?repo=try&revision=174011af6f7a4f592f03514b93b2e66f92ad768b

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f061b418bf20
Disallow floats, negative numbers, and long values in hashless color quirk; r=xidorn

Comment 7

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

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

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Mats Palmgren (:mats) from comment #4)
> > +numberSize(uint32_t number) {
> 
> The param should be named aNumber to follow our naming conventions.

Yeah, you're right... Sorry for not catching this... Probably I read too much Rust code nowadays :/