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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Sep 21 08:03:52 PDT 2007

Sven Herzberg <sven at imendio.com> has asked  for review:
Bug 15229: [gtk] abstract font management by using pango

Attachment 16340: Proposed Patch v2

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


> * There are several places where you have extra whitespace around, or
> 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.


> * In several places you have broken function calls across multiple lines for
> 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
> 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

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


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

> patch soon.

Well, thanks for the review.

More information about the webkit-reviews mailing list