On Thu, 2009-07-23 at 16:36 +0200, Daniel Silverstone wrote:
> On Thu, 2009-07-23 at 01:22 +0100, John-Mark Bell wrote:
> > Its runtime status is unknown.
>
> Presumably it is at least functional on GTK?
My apologies. That sentence applies only to the Amiga version.
GTK/Framebuffer/RISC OS are known to compile and run correctly.
Since I wrote the above, Chris has confirmed that it both compiles and
runs for Amiga.
> > Known regressions wrt trunk:
> >
> > 1. HTML alignment hints are ignored (this means <center>).
> > The intention is to fix this on trunk after merging.
>
> What is the reasoning for not fixing this pre-merge?
It's a pretty major job, involving adding more functionality to libcss
and then modifying the layout code to cope with it, so I'd rather it was
done in a separate branch.
Note that trunk's support for HTML alignment (particularly <center>) is
fairly hit-and-miss, anyway.
> > 2. Layout relaxation for page-based media has been removed wholesale.
> > The correct solution is to make the layout engine cater for paged media
> > also.
> > For the timebeing, output to page-based media will be suboptimal.
>
> does suboptimal mean worthless, or just not as nice as it could be?
It means that wide layouts that can't be naturally reflowed to the page
width won't fit on pages correctly. It also means that images or lines
of text might be split across the page boundary.
There's probably other stuff I've forgotten, too.
TBH, paged output never was all that great, anyway.
> > 1. option_font_min_size is now ignored completely. This was introduced to
> > work around pages using tables with font sizes <100%. Instead, we
> > implement
> > a layout quirk that resets font properties to their defaults when
> > encountering a table element. See quirks.css for further details.
>
> Firefox still offers a min-font-size option. Would this not be useful in
> the case (for example) of users who have high res screens and small
> fonts in general use being otherwise boned by people who use <font
> size="-3"> type things. Hell, sometimes I wish people wouldn't use
> <small> but they do. A 50% font-size will make things illegible to
> me :-(
It's a complete pita to support. Which is why I removed it.
You'd need to completely change the way libcss calculates computed
length units, for starters.
> > Index: framebuffer/res/quirks.css
> > @@ -0,0 +1,9 @@
> > +/* Quirks mode stylesheet for NetSurf */
> > +
> > +table {
>
> A short comment saying *why* this is here would be useful so we don't
> lose the information.
Sure. I'd intended to add that, in fact.
> > Index: gtk/res/quirks.css
> > Index: !NetSurf/Resources/Quirks,f79
>
> AFAICT these are all the same content. Is it that the diff has gone
> wrong, or did you really add the same file in three places rather than
> in the !NetSurf dir and then use symlinks?
The former. It would appear that svn diff resolves symlinks.
> > Index: css/utils.c
> > +css_fixed nscss_len2pt(css_fixed length, css_unit unit)
> > +{
> > + /* Length must not be relative */
> > + assert(unit != CSS_UNIT_EM && unit != CSS_UNIT_EX);
> > +
> > + switch (unit) {
> > + /* We assume the screen and any other output has the same dpi */
>
> Given this assumption, we presumably can change nscss_screen_dpi, render
> a print output, and then change back, should we wish? If so, should it
> be nscss_render_dpi ?
Probably. As it stands, it offers the same functionality as trunk, so
I'm not inclined to change it at this point.
> > + case CSS_UNIT_CM: return FMUL(length, FLTTOFIX(28.452756));
> > + case CSS_UNIT_MM: return FMUL(length, FLTTOFIX(2.8452756));
>
> These magic values offend me. Can we have #defines for them with a
> comment stating what the numbers were derived from?
Sure, though I'd have to reverse engineer them :)
> > +css_fixed nscss_len2px(css_fixed length, css_unit unit,
> > + const css_computed_style *style)
> > + case CSS_UNIT_CM: return FDIV(lendpi, FLTTOFIX(2.54));
> > + case CSS_UNIT_MM: return FDIV(lendpi, FLTTOFIX(25.4));
>
> Again, magic numbers which could do with explanation.
Uhuh.
> > Index: css/dump.c
> > +
> > +/**
> > + * Dump a computed style to the give file handle
>
> Better:
>
> Dump computed style \a style, as valid CSS, to the given file handle \a
> stream.
>
> \note This does not dump a selector for the style beforehand. If a
> selector is wanted, ensure that it is written to \a stream first.
>
> (however, claiming valid CSS is a touch fraught, this would require the
> insertion of some semicolons and/or newlines into the output stream :-)
The only place this is used is as part of the box tree dumping. I'd not
want to make any claims about it producing valid CSS.
As far as selectors go, there's no way of recovering them:
a) No record is kept of which rules contributed to a computed style.
b) Boxes have no knowledge of which bit of the DOM generated them.
> > +void nscss_dump_computed_style(FILE *stream, const css_computed_style
> > *style)
> > + /* border-bottom-color */
> > + val = css_computed_border_bottom_color(style, &color);
>
> These four are begging to be refactored out into their own
> nscss_dump_colour(stream, "border-bottom-color", val, color); type
> calls.
[and many more]
Yes. This was simply grabbed from libcss' test directory for the sole
purpose of retaining the box dump stuff. No effort was made to tidy it
up.
> > +/******************************************************************************
> > + * Helper functions
> > *
> > +
> > ******************************************************************************/
>
> Helper for what/who?
The thousand-or-so line function above it :)
> If they're only used in here, they should be
> static. If they're exported, can they be nscss_dump_* please?
They are static, as declared at the top of the file.
> > +/**
> > + * Dump a fixed point value to the stream
>
> ...in a textual form.
Ok.
> > +#define ABS(x) (uint32_t)((x) < 0 ? -(x) : (x))
>
> Can you please use something more locally scoped namewise? Just in case
> some platform goes ahead and defines ABS. Esp. since your ABS casts too
> for fun.
Yes.
> > + uint32_t uintpart = FIXTOINT(ABS(f));
> > + /* + 500 to ensure round to nearest (division will truncate) */
> > + uint32_t fracpart = ((ABS(f) & 0x3ff) * 1000 + 500) / (1 << 10);
>
> This looks remarkably like we need a FIXTOFRACT type macro in libcss.
Probably. This is the only use case, however.
> > +#undef ABS
> > + size_t flen = 0;
> > + char tmp[20];
> > + size_t tlen = 0;
> > +
> [snip printout of number]
>
> Any particular reason for not doing fprintf(stream, "%s%d.%03d", (f <
> 0) ? "-" : "", uintpart, fracpart)
>
> ?
Not that I can think of, no.
> (may need a bit of format string or number manipulation frobbery)
>
> > +/**
> > + * Dump a dimension to the stream
>
> ...in a textual form.
Right.
> > +void dump_css_unit(FILE *stream, css_fixed val, css_unit unit)
>
> static, or nscss_dump_unit ?
Again, static.
> > Index: css/utils.h
> > +#ifndef netsurf_css_utils_h_
> > +#define netsurf_css_utils_h_
>
> Header guard defines really ought to be in capital letters.
Ok.
> > +static inline colour nscss_color_to_ns(css_color color)
>
> inline? safe for core?
Good catch. Unlikely.
I guess this gets to move to utils.c or be turned into a macro.
> > Index: css/dump.h
> > +#ifndef netsurf_css_dump_h_
> > +#define netsurf_css_dump_h_
>
> guards in caps please.
Ditto :)
> > +void nscss_dump_computed_style(FILE *stream, const css_computed_style
> > *style);
>
> That this is the only exported function implies those others really
> ought to have been static.
They were :)
> > Index: css/select.c
>
> [snip vast set of static predeclarations]
>
> Do we *really* need these predeclarations, or could things be better
> done if you reordered the file to not need them? Predeclarations reduce
> ease of maintenance. It's not hard to look for the handler table at the
> end of the file. In general, if I see a file with a bunch of static
> predeclarations like that, my first thought is "how incestuous is this
> code and could it be better rewritten to not be so?".
This is the style the entire of the rest of NetSurf uses. I don't think
it's sensible to start changing that on this branch.
> > +css_error nscss_compute_font_size(void *pw, const css_hint *parent,
> > + css_hint *size)
> > +{
> > + static const css_hint_length sizes[] = {
> > + { FLTTOFIX(0.5625), CSS_UNIT_PT }, /* xx-small */
> > + { FLTTOFIX(0.6250), CSS_UNIT_PT }, /* x-small */
> > + { FLTTOFIX(0.8125), CSS_UNIT_PT }, /* small */
> > + { FLTTOFIX(1.0000), CSS_UNIT_PT }, /* medium */
> > + { FLTTOFIX(1.1250), CSS_UNIT_PT }, /* large */
> > + { FLTTOFIX(1.5000), CSS_UNIT_PT }, /* x-large */
> > + { FLTTOFIX(2.0000), CSS_UNIT_PT } /* xx-large */
>
> Again, some indication of what those magic numbers are would be nice.
Yup.
> > +bool nscss_parse_colour(const char *data, css_color *result)
>
> Is this not functionality which belongs inside libcss? Or is it HTML
> specific?
It's HTML specific (or will be, if anyone implements the insanity that
is HTML5's colour parsing algorithm).
> > +css_error node_name(void *pw, void *node,
> > + lwc_context *dict, lwc_string **name)
>
> > + lerror = lwc_context_intern(dict, (const char *) n->name,
> > + strlen((const char *) n->name), name);
>
> It would be nice if we could snarf a ref to this name for the node, so
> that the next time the node's name is asked for, we don't have to wait
> for lwc_context_intern to spot it already has it. Dunno if that
> increases the complexity of being able to chuck the libxml node tree
> though.
I suspect it's simpler to wait for libdom, or at least a hubbub that
interns strings. All of these callbacks will need reworking for that
scenario, anyway.
> > +css_error named_ancestor_node(void *pw, void *node,
> > + lwc_string *name, void **ancestor)
> > +{
> > + xmlNode *n = node;
> > + size_t len = lwc_string_length(name);
> > + const char *data = lwc_string_data(name);
> > +
> > + *ancestor = NULL;
> > +
> > + for (n = n->parent; n != NULL && n->type == XML_ELEMENT_NODE;
> > + n = n->parent) {
> > + bool match = strlen((const char *) n->name) == len &&
> > + strncasecmp((const char *) n->name,
> > + data, len) == 0;
>
> All of a sudden, the ability to intern the strings in the xmlnode makes
> more sense, unless something else is interning them in a duplicated
> tree?
Nothing interns anything tree-related right now. Hubbub will, assuming
someone ever implements it.
We're mostly stuck until that happens and libdom is complete.
I'd rather not start adding hacks around libxml just to go and replace
it wholesale in a few months' time.
> > +css_error named_parent_node(void *pw, void *node,
> > + lwc_string *name, void **parent)
> > + strlen((const char *) n->parent->name) == len &&
> > + strncasecmp((const char *) n->parent->name,
> > + data, len) == 0)
>
> Yet more string comparison better done by lwc?
As above.
> > +css_error named_sibling_node(void *pw, void *node,
> > + lwc_string *name, void **sibling)
> > + if (n->prev != NULL && strlen((const char *) n->prev->name) == len &&
> > + strncasecmp((const char *) n->prev->name,
> > + data, len) == 0)
>
> Ditto?
As above.
> > +css_error node_has_name(void *pw, void *node,
> > + lwc_string *name, bool *match)
> > +{
> > + xmlNode *n = node;
> > + size_t len = lwc_string_length(name);
> > + const char *data = lwc_string_data(name);
> > +
> > + /* Element names are case insensitive in HTML */
> > + *match = strlen((const char *) n->name) == len &&
> > + strncasecmp((const char *) n->name, data, len) == 0;
>
> Another case for lwc comparison?
As above.
> > +css_error node_has_class(void *pw, void *node,
> > + lwc_string *name, bool *match)
>
> I get the feeling this function will become more lwcish when we can use
> libdom?
As above.
> > +css_error node_has_id(void *pw, void *node,
> > + lwc_string *name, bool *match)
>
> Again, interning an lwc_string against the node could help here.
As above.
> > +css_error node_has_attribute(void *pw, void *node,
> > + lwc_string *name, bool *match)
>
> > + buf = malloc(lwc_string_length(name) + 1);
> > + if (buf == NULL)
> > + return CSS_NOMEM;
> > +
> > + memcpy(buf, lwc_string_data(name), lwc_string_length(name));
> > + buf[lwc_string_length(name)] = '\0';
>
> I wish this copying wasn't necessary. Should we alter lwc to guarantee
> null termination (regardless of whether or not the string contains a
> NULL anyway)? Lua does that, despite being ptr+length, it guarantees to
> NULL terminate the strings it returns to you.
That would be helpful, yes.
> > +css_error node_has_attribute_dashmatch(void *pw, void *node,
> > + lwc_string *name, lwc_string *value,
> > + bool *match)
>
> > +
> > + memcpy(buf, lwc_string_data(name), lwc_string_length(name));
> > + buf[lwc_string_length(name)] = '\0';
>
> I'm beginning to think it's less stupid that it might have been.
:)
> > + strncasecmp(start,
> > + lwc_string_data(value),
> > + vlen) == 0) {
>
> *sob* We need something interning values. Roll on libdom.
Quite. You should see the profiles -- style selection is far and away
the largest performance sink. Without string interning, there's nothing
to be done, though.
> > +css_error node_is_hover(void *pw, void *node, bool *match)
> > +{
> > + *match = false;
>
> A \todo for this?
Would be good, yes.
> > +css_error node_is_active(void *pw, void *node, bool *match)
> > +{
> > + *match = false;
>
> Ditto
>
> > +css_error node_is_focus(void *pw, void *node, bool *match)
> > +{
> > + *match = false;
>
> Ditto.
>
> > +css_error node_is_lang(void *pw, void *node,
> > + lwc_string *lang, bool *match)
> > +{
> > + *match = false;
>
> Ditto.
Yup.
> > +css_error ua_default_for_property(void *pw, uint32_t property, css_hint
> > *hint)
>
> > + } else if (property == CSS_PROP_FONT_FAMILY) {
> > + hint->data.strings = NULL;
> > + hint->status = CSS_FONT_FAMILY_SANS_SERIF;
>
> Should we have an option for this, like IE and firefox do?
We do. I forgot it existed :)
> > +int cmp_colour_name(const void *a, const void *b)
> > +{
> > + const char *aa = a;
> > + const struct { const char *name; css_color color; } *bb = b;
>
> Where has this struct definition come from? It belongs in a header
> somewhere surely?
>
> > +bool parse_named_colour(const char *name, css_color *result)
> > +{
> > + static const struct colour_map {
> > + const char *name;
> > + css_color color;
> > + } named_colours[] = {
>
> Are these HTML defined colours?
They are.
> If not, can they belong to libcss? If so, should they belong to hubbub?
LibCSS has its own copy of these. Right now, we're stuck with having CSS
stuff wanting interned strings and nothing else supporting them.
Otherwise I'd have just made libcss do this stuff.
> > +bool parse_number(const char *data, bool maybe_negative, bool real,
> > + css_fixed *value, size_t *consumed)
>
> This function feels like it ought to be part of libcss. Or am I
> misunderstanding where numbers are defined?
It has one too. I was in two minds about exposing it, however.
> > +/**
> > + * Determine if a given character is whitespace
> > + *
> > + * \param c Character to consider
> > + * \return true if character is whitespace, false otherwise
> > + */
> > +bool isWhitespace(char c)
> > +{
> > + return c == ' ' || c == '\t' || c == '\f' || c == '\r' || c == '\n';
> > +}
>
> Given you take 'char' not 'int' (and thus cannot be doing unicode
> codepoints) what's wrong with ctype's isspace() here?
It's locale-specific. Which none of the core code can be.
> > Index: css/internal.h
> > +#ifndef netsurf_css_internal_h_
> > +#define netsurf_css_internal_h_
>
> Guard in caps please.
Ok.
> > Index: css/select.h
>
> > +#ifndef netsurf_css_select_h_
> > +#define netsurf_css_select_h_
>
> Guard in caps please.
Ok.
> > Index: beos/res/quirks.css
> > Index: amiga/resources/quirks.css
>
> Another two quirks files, or is this more case of diff not being sane in
> the face of new symlinks?
Latter :)
> > Index: render/box_normalise.c
> > ===================================================================
> > struct span_info {
> > + /* Number of rows this cell spans */
> > unsigned int row_span;
> > + /* The cell in this column spans all rows until the end of the table */
> > bool auto_row;
> > - bool auto_column;
> > };
>
> If you're going to add comments, please just turn the struct commentary
> into a doxygen documentation comment set.
Righto.
> > struct columns {
> > + /* Current column index */
> > unsigned int current_column;
> > - bool extra;
> > /* Number of columns in main part of table 1..max columns */
> > unsigned int num_columns;
> > - /* Information about columns in main table,
> > - array 0 to num_columns - 1 */
> > + /* Information about columns in main table, array [0, num_columns) */
> > struct span_info *spans;
> > - /* Number of columns that have cells after a colspan 0 */
> > - unsigned int extra_columns;
> > /* Number of rows in table */
> > unsigned int num_rows;
> > };
>
> Ditto.
>
> > gui_multitask();
>
> This is bare here, should it be in an ifdef riscos?
Pass. The whole multitasking thing needs redesigning. Again, it's a huge
upheaval, so we're stuck with the current mess until someone does
something about it. :(
> > @@ -282,38 +321,43 @@
> > - if (table->children == 0) {
> > + if (table->children == NULL) {
>
> Thank you for all these particular fixes. Here, have a virtual oatmeal
> cookie.
Noms.
> > @@ -596,42 +745,27 @@
> > assert(0);
>
> *pout* These kinds of assertions make baby Daniel cry.
I'm not entirely sure what to replace that with.
> > Index: render/html.c
> > @@ -343,7 +367,8 @@
> > @@ -467,7 +492,7 @@
> > + /*box_dump(stderr, c->data.html.layout->children, 0);*/
>
> Is box_dump not conditional on some debug thingy anyway? Or is this a
> case of needing a better debugging infrastructure?
box_dump isn't conditional on anything. However, you don't want to be
calling box_dump at all there without good reason -- it's a huge
performance drain for no benefit.
That only ever gets uncommented when someone's playing with the layout
code. Even then, it's rare. Hence why it's taken about 2 years for
someone to actually change the call to conform to the "new" box_dump
API :)
> > Index: render/html_redraw.c
>
> Out of interest, the css_computed_*() functions -- are they computing
> each time, or caching? If they're computing each time, that's gonna hurt
> performance, no?
Caching, mostly. They're mostly direct accessors to struct contents.
> > Index: render/layout.c
> > @@ -328,9 +335,10 @@
> ...
> > if (box->style) {
>
> Aww, you missed these throughout the file. I'm taking back that cookie.
Shame I'd eaten half of it :)
You don't want me to change all the conditionals in this file right now
-- it'd make a huge mess and just confuse the diff.
> > @@ -1183,33 +1224,37 @@
> > @@ -1217,72 +1262,91 @@
> > if (height) {
>
> if height is what?
>
> > @@ -1290,53 +1354,60 @@
> > if (max_width) {
>
> if max_width is what?
>
> I could go on, but I won't :-)
Phew.
> > @@ -1435,7 +1597,7 @@
> > +
> > + if (c->style) {
>
> Oh but I will here... Grrrrr Mister. It's one thing to not fix any you
> spot, it's another to *add* one.
Granted.
> > @@ -3567,21 +3842,25 @@
> > * ansestor with float_children, rather than
>
> ancestor
>
> Most of the rest of the code I've kinda skipped because I don't
> understand the vast majority anyway, especially in +/- littered diff
> world. Nothing jumped out at me though.
Good.
J.
