Hello,
This is a review of the lcneves/flexbox-tidy branch.
First off I think this is really good work. Almost all the
things I spotted range from minor to trivial.
Aside from the unset values for unit and length in the
min-{width|height} computed style getters, and the fact
that NetSurf is about to do a release[1], I'd be happy
to merge this as-is.
[1] I think NetSurf will need some minor changes to cope
with new values for the display property and the
min-{width|height} properties.
Add codes to flexbox properties
===============================
http://source.netsurf-browser.org/libcss.git/commit/?h=lcneves/flexbox-tidy&id=b1c9ff5a957db2ff86ebacb4b4e735458f7a60ab
+72 - align-content
+ <value> (14bits) :
+ 0 => stretch,
+ 1 => flex-start,
+ 2 => flex-end,
+ 3 => center,
+ 4 => space-between,
+ 5 => space-around,
+ 6 => space-evenly
+ other => Reserved for future expansion.
I'm a bit unsure about the space-evenly value.
CSS Flexible Box Layout Module Level 1 doesn't have this value:
https://www.w3.org/TR/css-flexbox-1/#propdef-align-content
CSS Box Alignment Module Level 3 does have this value and
a few others:
https://www.w3.org/TR/css-align-3/#alignment-values
I think its OK for us to have space-evenly. Same for the
justify-content property.
Parsing: Add support for parsing the flexbox properties
=======================================================
http://source.netsurf-browser.org/libcss.git/commit/?h=lcneves/flexbox-tidy&id=fd73ec6bf364fec71309528a823474533c73a226
src/parse/properties/flex.c:
http://source.netsurf-browser.org/libcss.git/diff/src/parse/properties/flex.c?h=lcneves/flexbox-tidy&id=fd73ec6bf364fec71309528a823474533c73a226
+/**
+ * Parse list-style
+ *
s/list-style/flex/
In css__parse_flex():
+ parserutils_vector_iterate(vector, ctx);
+ }
+
+ /* Handle auto, equivalent of flex: 1 1 auto; */
+ else if ((token->type == CSS_TOKEN_IDENT) &&
+ (lwc_string_caseless_isequal(
+ token->idata, c->strings[AUTO],
+ &match) == lwc_error_ok && match)) {
+ error = css__stylesheet_style_appendOPV(grow_style,
+ CSS_PROP_FLEX_GROW, 0, FLEX_GROW_SET);
Our code style is } else { on a single line, and the comment
would be inside the else block before the first line.
Could do something like:
parserutils_vector_iterate(vector, ctx);
} else if ((token->type == CSS_TOKEN_IDENT) &&
(lwc_string_caseless_isequal(
token->idata, c->strings[AUTO],
&match) == lwc_error_ok && match)) {
/* Handle auto, equivalent of flex: 1 1 auto; */
error = css__stylesheet_style_appendOPV(grow_style,
CSS_PROP_FLEX_GROW, 0, FLEX_GROW_SET);
Also I think we could simplfy handling of auto and none
by creating a copule more bools at the start of the function:
bool auto = false;
bool none = false;
Set those true if we get either of those tokens here, and
then below, in the /* defaults */ section, choose the
defaults that get set based on whether:
1. auto is set
2. none is set
3. neither are set
src/parse/properties/flex_flow.c:
http://source.netsurf-browser.org/libcss.git/diff/src/parse/properties/flex_flow.c?h=lcneves/flexbox-tidy&id=fd73ec6bf364fec71309528a823474533c73a226
+/**
+ * Parse list-style
+ *
s/list-style/flex-flow/
Selection: Add support for the flexbox properties
=================================================
http://source.netsurf-browser.org/libcss.git/commit/?h=lcneves/flexbox-tidy&id=2fd135cd43d6821e944110bbdf8f77bea5cc40fb
In src/select/computed.c:
http://source.netsurf-browser.org/libcss.git/diff/src/select/computed.c?h=lcneves/flexbox-tidy&id=2fd135cd43d6821e944110bbdf8f77bea5cc40fb
In css_computed_min_height():
- return get_min_height(style, length, unit);
+ uint8_t min_height = get_min_height(style, length, unit);
+ uint8_t display = get_display(style);
+
+ if (display != CSS_DISPLAY_FLEX && display !=
CSS_DISPLAY_INLINE_FLEX &&
+ min_height == CSS_MIN_HEIGHT_AUTO)
+ {
Spaces used for indent instead of tab on the first and last
inserted likes above. Also out code style doesn't put the
opening brace on a new line.
Same in css_computed_min_width().
For both css_computed_min_height() css_computed_min_width() we
could rearrange to avoid getting display unless the value is
auto. Something like:
uint8_t css_computed_min_height(const css_computed_style *style,
css_fixed *length, css_unit *unit)
{
uint8_t min_height = get_min_height(style, length, unit);
if (min_height == CSS_MIN_HEIGHT_AUTO) {
uint8_t display = get_display(style);
if (display != CSS_DISPLAY_FLEX &&
display != CSS_DISPLAY_INLINE_FLEX) {
min_height = CSS_MIN_HEIGHT_SET;
}
}
return min_height;
}
Finally, I don't think it's OK to override the value
to *_SET and not set appropriate values for length and unit
at the same time. When the computed value is set to *_AUTO
in the cascade it doesn't clear any values for length and
unit. So we should be them up for 0px where we override
min_height.
Same for both css_computed_min_height() and for
css_computed_min_width().
In css_computed_display():
+ } else if (display == CSS_DISPLAY_INLINE_FLEX) {
+ return CSS_DISPLAY_FLEX;
Spaces used for indent on the first inserted line.
In src/select/computed.h:
http://source.netsurf-browser.org/libcss.git/diff/src/select/computed.h?h=lcneves/flexbox-tidy&id=2fd135cd43d6821e944110bbdf8f77bea5cc40fb
+ * 35 yyybbbaa overflow-y | background-repeat | align-content
+ * 36 bbbbbbaj flex-basis | align-content | justify_content
+ * 37 fffcccjj flex-direction | clear | justify_content
Since align-content and justify-content are split over two
areas, I would say somthing to hint at that in the comments,
e.g.:
* 35 yyybbbaa overflow-y | background-repeat | align-content1
* 36 bbbbbbaj flex-basis | align-content2 | justify_content1
* 37 fffcccjj flex-direction | clear | justify_content2
In the long term perhaps we could consider using a uint32_t
array for the bit data than uint8_t, so its easier to pack
things in without splitting stuff.
+ css_fixed flex_basis;
+
+ css_fixed flex_grow;
+
+ css_fixed flex_shrink;
+
+ int32_t order;
Spaces used for indent.
In src/select/dispatch.c:
http://source.netsurf-browser.org/libcss.git/diff/src/select/dispatch.c?h=lcneves/flexbox-tidy&id=2fd135cd43d6821e944110bbdf8f77bea5cc40fb
+ },
+ {
+ PROPERTY_FUNCS(align_content),
Spaces for indent on the middle inserted line above.
In src/select/propget.h
http://source.netsurf-browser.org/libcss.git/diff/src/select/propget.h?h=lcneves/flexbox-tidy&id=2fd135cd43d6821e944110bbdf8f77bea5cc40fb
-#define BOX_SIZING_INDEX 34
+#define BOX_SIZING_INDEX 23
Good spot! Ditto for the similar change in propset.h.
In get_align_content():
+ /* Most significant bit out of three */
+ bits_b <<= 2;
+
+ uint8_t bits = bits_a | bits_b;
Spaces for indent on the first and last lines above.
Similar in get_justify_content().
Selection: Logic for the flexbox properties
===========================================
http://source.netsurf-browser.org/libcss.git/commit/?h=lcneves/flexbox-tidy&id=587926f812375472962628845226d18f5df95cf1
In src/select/properties/flex_basis.c:
http://source.netsurf-browser.org/libcss.git/diff/src/select/properties/flex_basis.c?h=lcneves/flexbox-tidy&id=587926f812375472962628845226d18f5df95cf1
In css__cascade_flex_basis():
+ uint16_t value = CSS_FLEX_BASIS_INHERIT;
+ css_fixed length = 0;
+ uint32_t unit = UNIT_PX;
Spaces for indent on last line above.
In src/select/properties/order.c:
http://source.netsurf-browser.org/libcss.git/diff/src/select/properties/order.c?h=lcneves/flexbox-tidy&id=587926f812375472962628845226d18f5df95cf1
+ /* Undo the <<10, because this is an integer */
+ order = *((css_fixed *) style->bytecode) >> 10;
Use CSS_RADIX_POINT instead of 10. (From include/libcss/fpmath.h)
Or use the FIXTOINT macro from include/libcss/fpmath.h.
--
Michael Drake http://www.codethink.co.uk/
