Re: [Gnash-commit] /srv/bzr/gnash/trunk r11336: HTML font tag

[Benjamin Wolsey]

All this TextField work is very good and useful (from everyone working
on it), and I was very pleased that the testsuite passed when I got up
this morning.

The next stage in the process of virtuous hacking is to add tests
yourself before (or at least while) implementing things. The HTML code
kind of works, but only kind of. This is because there aren't really any
tests to see how it functions, so the implementation can only be a
guess. Presumably it's based on documentation (which is not very
accurate or complete) and on some standard use cases. But tests should
test the corner cases too, so that the dodgy bits of this implementation
would have been identified earlier.

>                              //font
> -                            log_unimpl("<font> html tag in TextField");
> +                            boost::uint16_t originalsize = _fontHeight;
> +                            if (attributes.find("color") != 
> attributes.end()) {
> +                                boost::uint8_t r = std::strtol(
> +                                    attributes["color"].substr(1,2).data(), 
> NULL, 16);

map::find returns an iterator, which you can use to avoid repeatedly
looking up attributes["color"]. Each lookup has a cost, which is
certainly negligible here, but it's still good style to avoid it and
make the code clearer.

> +                                boost::uint8_t g = std::strtol(
> +                                    attributes["color"].substr(3,2).data(), 
> NULL, 16);

std::strtol isn't nearly as flexible as a stringstream. Using the tests
I added (and your own new ones), it should be possible to add a nice
stringstream conversion function for the rgba class, which could take a
string and fill in the rgba slots (this could be a free function like:

parseRGBA(const std::string&, rgba&)

). That would save doing it all in the middle of nested ifs in
TextField.

> +                                boost::uint8_t b = std::strtol(
> +                                    attributes["color"].substr(5,2).data(), 
> NULL, 16);

This works well as long as the string is at least 5 characters. If it
isn't, string::substr will throw a std::out_of_range exception and crash
Gnash. I will add a test that demonstrates that (but with the code
disabled to avoid failing the testsuite). (Using stringstreams would
also get round this problem).

--
The current release of Gnash is 0.8.5
http://www.gnu.org/software/gnash/

Benjamin Wolsey, Software Developer - http://benjaminwolsey.de

signature.asc
Description: Dies ist ein digital signierter Nachrichtenteil