[webkit-reviews] review requested: [Bug 15229] [gtk] abstract font management by using pango : [Attachment 16340] Proposed Patch v2

[bugzilla-daemon at webkit.org]

Sven Herzberg <sven at imendio.com> has asked  for review:
Bug 15229: [gtk] abstract font management by using pango
http://bugs.webkit.org/show_bug.cgi?id=15229

Attachment 16340: Proposed Patch v2
http://bugs.webkit.org/attachment.cgi?id=16340&action=edit

------- Additional Comments from Sven Herzberg <sven at imendio.com>
(In reply to comment #3)
> * Many newly-added function calls have unneeded whitespace between the
function
> name and the opening parenthesis.

Fixed.

> * There are several places where you have extra whitespace around, or
incorrect
> placement of, the * in declarations of pointer variables.  In several places
> you have extra whitespace around the = operator in assignments.  More
> generally, we don't typically use whitespace to line up assignments and
> declarations.

Fixed.

> * In several places you have broken function calls across multiple lines for
no
> apparent reason.  In cases where it's wrapped due to one argument being the
> result of a function call, it may be preferable to use a local variable to
> simplify the code rather than jamming it all into the function call.	In
other
> cases, having all the arguments on the same line may be preferable.

Fixed. I just didn't fix this for two calls:
FontPlatformDataGdk.cpp: FontPlatformData::init(),
GlyphPageTreeNodeGdk.cpp: GlyphPage::fill() - introducing a local variable will
require adding braces - which in turn will lead up to code that looks almost as
scrammed as it does now.

> * When checking if a pointer is NULL, !theVariable is preferred over
> theVariable == 0.

I haven't done that, it was a plain int that I compared to, but this is fixed
too.

> * Declaring local variables at the point of first use is preferred over
> declaring at the top of a function then initializing later.

Fixed.

> Sorry for being so picky!  Hopefully someone can review the real meat of your

> patch soon.

Well, thanks for the review.