On Thu, 2009-07-23 at 17:33 +0100, Michael Drake wrote:
> 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.
It's not memory usage I care about. It's my sanity when modifying
libcss' accessors for the umpteenth time.
As it happens, minimum font size handling can be achieved with 2 minor
changes, which is significantly less invasive than any other suggestion
(and the previous implementation). I've committed this as r8736.
> > > 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.
I couldn't care less what it's called. It probably wants moving to
desktop/ at some point.
> > > 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?
Because that's what the code I copied them from did.
> 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?
Yes. 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?
Possibly, though I'm not actually seeing this call in profiles.
J.
