On Fri, 2013-06-07 at 04:09 -0400, Caitlin Potter wrote:
> This patch is intended to support the parsing and storage of the
> "writing-mode" property, which is one of the building blocks for
> vertical text in css (support for which is currently required by the
> HTML5 track element and obviously is a pretty important localization
> feature -- and just simply looks cool, too).
Excellent. Thanks for your work on this.
[...]
> It is quite likely that there are things substantially wrong,
> including the lack of test data (I haven't figured out how to get the
> test suite to even build and run yet, `make test` seems to fail
> substantially here.)
You're using a Mac, right? I've no current idea why make test isn't
working for you on that platform, as the test infrastructure is pretty
trivial. We're investigating that now.
> So some review would be good, I'd like to get this approved by netsurf
> devs before trying to squeeze it into mozilla-servo.
In general, this looks fine to me (modulo the lack of test data, which
you've mentioned above). There are a few minor whitespace nits, which
can be fixed when we merge the change in. Additionally, there's a stray
commented-out chunk in css__set_writing_mode_from_hint. This, too is
trivially fixed, so I'm not concerned by it.
J.
No comments:
Post a Comment