[webkit-reviews] review granted: [Bug 8954] Separate the GlyphMap
into its own class and make it cross-platform : [Attachment
8364] Pull the glyph map into its own files and re-implement
with HashMap.
bugzilla-request-daemon at opendarwin.org
bugzilla-request-daemon at opendarwin.org
Wed May 17 12:11:58 PDT 2006
Darin Adler <darin at apple.com> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 8954: Separate the GlyphMap into its own class and make it cross-platform
http://bugzilla.opendarwin.org/show_bug.cgi?id=8954
Attachment 8364: Pull the glyph map into its own files and re-implement with
HashMap.
http://bugzilla.opendarwin.org/attachment.cgi?id=8364&action=edit
------- Additional Comments from Darin Adler <darin at apple.com>
"Nasty hack" comment should move down to just before the determinePitch call.
I think GLYPH_PAGE_SIZE should be a constant instead of a macro, then it would
not be all captizls. Since this is C++. Same for NO_BREAK_SPACE and
ZERO_WIDTH_SPACE.
I think GlyphMapMac.cpp should include WebCoreSystemInterface.h instead of
declaring those globals.
I also think that WKGlyphVectorSize should be all capitals if it's a macro and
should be in WebCoreSystemInterface.h.
Lets make sure the files have newlines at the end.
Maybe m_primaryPage should just be a GlyphPage, rather than a GlyphPage*. Also,
if we almost always use at least one other GlyphPage, then I think m_pages
could also be a HashMap instead of a HashMap*.
Since we create glyph pages, and then immediately fill them in, I think it's
not good that GlyphData has a constructor. That means that the entire thing
will be filled with 0s and then later we have to go through and overwrite the
0s. That initial 0-fill is pointless.
I suggest that instead GlyphData just be a struct with glyph and fontData
members, and not have constructors at all.
Functions like setGlyphDataForIndex might be more efficient if they didn't
create a temporary GlyphData anyway.
I had some suggestions for how to streamline locatePage, but I decided they
were not good enough to suggest here.
+ buffer[(int)'\n'] = ' ';
+ buffer[(int)'\t'] = ' ';
Are these int casts necessary?
I think this is fine to land without my suggestions, so r=me, but please
consider the ideas.
More information about the webkit-reviews
mailing list