In article <1248363062.32517.509.ca...@duiker>,
John-Mark Bell <[email protected]> wrote:
> 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:
> > > 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.
<center> is identical to <div align=center>, anyway yeah, that's all a bit
of a mess on trunk right now. I'd be happy to lose support for that for a
while.
> > > 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.
I think it can be supported without invasive changes by adding another
font size to the computed style struct and a few changes in
nscss_compute_font_size(). I think the feature is worth slightly more
memory usage.
> > 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.
Also, it's not really css related, so I guess it could be called
render_dpi.
> > 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.
Can the static functions have the module name at the start anyway, please?
> > > +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.
Why is it an array of css_hint_length? Calling them pt units confused me a
lot when I first looked at this function. If I understand it, they're just
an array of unitless multipliers that could be an array of css_fixed?
> > > + } else if (property == CSS_PROP_COLOR) {
> > > + xmlChar *col;
> > > + css_error error;
> > > + bool is_link, is_visited;
> > > +
> > > + error = node_is_link(html, n, &is_link);
> > > + if (error != CSS_OK)
> > > + return error;
> > > +
> > > + if (is_link) {
> > > + xmlNode *body;
> > > + for (body = n; body != NULL && body->parent != NULL &&
> > > + body->parent->parent != NULL;
> > > + body = body->parent) {
> > > + if (body->parent->parent->parent == NULL)
> > > + break;
> > > + }
> > > +
> > > + error = node_is_visited(html, n, &is_visited);
If node_is_visited is a bottleneck, maybe we should only call it here if
the BODY element had a vlink attribute?
> > > + if (error != CSS_OK)
> > > + return error;
> > > +
> > > + if (is_visited)
> > > + col = xmlGetProp(body,
> > > + (const xmlChar *) "vlink");
> > > + else
> > > + col = xmlGetProp(body,
> > > + (const xmlChar *) "link");
> > > + } else if (strcmp((const char *) n->name, "body") == 0) {
> > > + col = xmlGetProp(n, (const xmlChar *) "text");
> > > + } else {
> > > + col = xmlGetProp(n, (const xmlChar *) "color");
> > > + }
> > > +
> > > + if (col == NULL)
> > > + return CSS_PROPERTY_NOT_SET;
> > > +
> > > + if (nscss_parse_colour((const char *) col, &hint->data.color)) {
> > > + hint->status = CSS_COLOR_COLOR;
> > > + } else {
> > > + xmlFree(col);
> > > + return CSS_PROPERTY_NOT_SET;
> > > + }
> > > +
> > > + xmlFree(col);
> > > +
> > > + return CSS_OK;
--
Michael Drake (tlsa) http://www.netsurf-browser.org/
