[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]

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.