840402 - Add some more values to property_database.js

Status: RESOLVED FIXED

Description

[Jesse Ruderman]

Created attachment 712782 [details] [diff] [review]
patch

Comment 1

[Jesse Ruderman]

Created attachment 712783 [details]
i like snakes

Comment 2

[Jesse Ruderman]

I also tried to add these, but they caused failures:

not serialized this way?
  overflow: -moz-scrollbars-none, -moz-scrollbars-horizontal, -moz-scrollbars-vertical

relative things:
  font-stretch: narrower | wider
  font-size: larger | smaller

Comment 3

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

Comment on attachment 712782 [details] [diff] [review]
patch

Maybe leave the existing last invalid_values  and add your new one (background-color)?

Your final initial_values entry for color is wrong; please remove it.  It's a bug that we clamp rgb() to 0-255.

-moz-use-system-font as an initial value for 'font-family' is also kinda wrong; it only comes out initial when there's no system font in the 'font' shorthand, so I wouldn't include that.

All those new other_values for font-family are pretty weird -- you should at least have a code comment explaining that for font-family (unlike font) they're not special values, they're just identifiers -- and maybe use some that aren't -moz-* 

r=dbaron with those things fixed

Comment 4

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

(In reply to Jesse Ruderman from comment #2)
> I also tried to add these, but they caused failures:
> 
> not serialized this way?
>   overflow: -moz-scrollbars-none, -moz-scrollbars-horizontal,
> -moz-scrollbars-vertical

-moz-scrollbars-none ought towork (synonym for hidden), and the other two also ought to work once we implement two-value 'overflow'.  What failed?

> relative things:
>   font-stretch: narrower | wider

We don't implement these, and they were dropped from the spec.  Could test them in invalid_values, though.

>   font-size: larger | smaller

I'd think they'd have worked as other_values.  What failed?

Comment 5

[Jesse Ruderman]

> Maybe leave the existing last invalid_values and add your new one
> (background-color)?

The new test is stronger, since there's only one reason to reject the input.

> Your final initial_values entry for color is wrong; please remove it.  It's a bug 
> that we clamp rgb() to 0-255.

I see.  I'll leave it out, and let you add a more correct test in bug 515919.

> -moz-use-system-font as an initial value for 'font-family' is also kinda wrong; 
> it only comes out initial when there's no system font in the 'font' shorthand, so 
> I wouldn't include that.

When there is *not* a system font?

Anyway, this doesn't seem to fit in property_database.js, so I'll teach my fuzzer about it instead.  

> All those new other_values for font-family are pretty weird -- you should at 
> least have a code comment explaining that for font-family (unlike font) they're 
> not special values

Oops, they should have gone in font rather than font-family.

> ... they're just identifiers -- and maybe use some that aren't -moz-* 

I'll stick "-no-such-font-installed" in font-family as the "just an identifier" test.

>>   overflow: -moz-scrollbars-none, -moz-scrollbars-horizontal,
>>             -moz-scrollbars-vertical

> -moz-scrollbars-none ought to work (synonym for hidden), and the other two also 
> ought to work once we implement two-value 'overflow'.  What failed?

You're right, -moz-scrollbars-none does okay.  I'll just add that one for now.

>>   font-stretch: narrower | wider
> We don't implement these, and they were dropped from the spec.

That would explain it!

>>   font-size: larger | smaller
> I'd think they'd have worked as other_values.  What failed?

Yeah, I assumed incorrectly based on the font-stretch results.  Added these.

Comment 6

[Jesse Ruderman]

Created attachment 713787 [details] [diff] [review]
patch v2

Comment 7

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

Comment on attachment 713787 [details] [diff] [review]
patch v2

r=dbaron; thanks for doing this

Comment 8

[Jesse Ruderman]

I did it for my DOM fuzzer ;)

https://hg.mozilla.org/integration/mozilla-inbound/rev/71f76fd4fbab

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/71f76fd4fbab