[webkit-reviews] review denied: [Bug 16404] [WX] ASSERT failures in HashTable for FontPlatformData : [Attachment 17854] Changes the FontPlatformData(Deleted) constructor so that m_font is -1 instead of 0

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Kevin Watters
<kevinwatters at gmail.com>'s request for review:
Bug 16404: [WX] ASSERT failures in HashTable for FontPlatformData
http://bugs.webkit.org/show_bug.cgi?id=16404

Attachment 17854: Changes the FontPlatformData(Deleted) constructor so that
m_font is -1 instead of 0
http://bugs.webkit.org/attachment.cgi?id=17854&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Looks good! Good fix.

 45	FontPlatformData::FontPlatformData(const FontDescription& desc, const
AtomicString& family);

Explicitly qualifying the name here with the class name (X::X) is incorrect
syntax, and not all compilers accept it. Also, it's WebKit practice to omit
unnecessary argument names when the type of hte parameter already makes it
clear, so "desc" should be omitted.

 46	FontPlatformData(wxFont* f);

As should "f" here.

This patch makes a bunch of formerly-inline functions no longer inline. Was
that intentional? Will it make the code slower? I like that it reduces the
amount exposed in the header.

39	switch (family){
40	    case FontDescription::StandardFamily:

 41	switch (family) {
 42	case FontDescription::StandardFamily:

Here, you have changed the WebKit standard indentation of switch statements
(cases indented inside the switch, then contents indented one additional level)
to another format.

 83	if (m_font != 0 && m_font != DELETED_FONT)

WebKit coding style omits the != 0 in cases like this one.

review- because of the FontPlatformData::FontPlatformData issue. The other
issues are smaller ones.