Re: [Patch] Add support for parsing writing-mode property v1

I still have not added any tests explicitly regarding the writing-mode property, however I have gotten everything to a stage where all tests are passing, which previously was not the case.

I could use some guidance writing parse/select tests for the property because even after studying the test programs and data files, it's not really obvious what the system is -- And beyond that, since I'm not really a CSS guru by any means, it is likely that I would write incorrect expectations when e.g. important is used.

It seems that there will also be a few more properties to add, including 'text-orientation' and 'direction', I'm not sure if I should add those as part of this patch or perhaps add those in subsequent chunks.

You're all busy people so when you have some time just give me a nod and I'll see what I can do.

On 2013-06-07, at 5:12 AM, John-Mark Bell <jmb@netsurf-browser.org> wrote:

> 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.
>
>
>
>