[webkit-reviews] review denied: [Bug 27734] WINCE PORT: font related changes : [Attachment 34178] remove unnecessary lines in FontCache.h

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Aug 6 19:48:50 PDT 2009


Eric Seidel <eric at webkit.org> has denied Yong Li <yong.li at torchmobile.com>'s
request for review:
Bug 27734: WINCE PORT: font related changes
https://bugs.webkit.org/show_bug.cgi?id=27734

Attachment 34178: remove unnecessary lines in FontCache.h
https://bugs.webkit.org/attachment.cgi?id=34178&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
Please collapse this line into one nicely named define:

 35 #if PLATFORM(CG) || PLATFORM(QT) || PLATFORM(GTK) || (PLATFORM(CHROMIUM) &&
PLATFORM(WIN_OS)) || PLATFORM(WINCE)

like STORE_FONT_POINTER or whatever the heck the ifdefed code does.

Please add more indetifying information than just "comment above":
 179	     // see comment above
otherwise if someone adds a comment between your two comments the latter
comment will be confusing.

// See comment about WINCE GDI handling near setGlyphDataForCharacter above.
or something.

Does WINCE only support horrizontal text?
3 #elif PLATFORM(WINCE)
 64 typedef float GlyphBufferAdvance;
6065 #else
A comment is need there to explain why it's just a float.

No need for arg name:
 51	FontCustomPlatformData* createFontCustomPlatformData(const
SharedBuffer* buffer);

Can you split this out?

r- mostly for the unexplained advances change.


More information about the webkit-reviews mailing list