Re: [Libreoffice] [PUSHED] [PATCH] Fix Bug 34666

Thorsten Behrens píše v Pá 18. 03. 2011 v 13:29 +0100:
> Christina Roßmanith wrote:
> > first bug fix. Built successfully and could open testdoc.html correctly.

Congratulations! I am looking forward to see more fixes from you.

> Very cool, checked that - it's pushed! :)

Heh, I have tried the patch yesterday and did not see any difference in
the HTML import. I was not sure what it actually fixed, so I asked
Cedric for more details (he created the simplified test case). I havn't
got the response yet.


Note that you did the hard job with finding the right place to fix, ...
Just please excuse my nitpicking. I would suggest 3 changes:

1. IMHO, it should check for the size of aToken to avoid a potential
crash:

  - if ('/' == aToken.GetChar(aToken.Len()-1)) {
  + if (aToken.Len() >= 1 && '/' == aToken.GetChar(aToken.Len()-1)) {


2. #i34666# associates OOo isusezilla. The bug is evidently from Free
Desktop bugzilla, so there should be fdo#34666 instead of #i34666# in
the comment.

3. I prefer commit messages that gives a rough idea about what has been
fixed. The bug number is important but it is not that helpful when you
look a fix in a specific area. Next time, It would suggest somethig
like:

  --- cut ---
  comment import problems with unsupported HTML tag (fdo#34666)
  --- cut ---

I have done the first two suggested changes by
http://cgit.freedesktop.org/libreoffice/libs-gui/commit/?id=95e301d0a55b8145ce0cdb037e438bbcfcd4a4eb


Best Regards,
Petr