Bug 16404 - [WX] ASSERT failures in HashTable for FontPlatformData
Summary: [WX] ASSERT failures in HashTable for FontPlatformData
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit wx (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-12-11 12:50 PST by Kevin Watters
Modified: 2007-12-15 17:58 PST (History)
1 user (show)

See Also:


Attachments
Changes the FontPlatformData(Deleted) constructor so that m_font is -1 instead of 0 (4.67 KB, patch)
2007-12-11 12:53 PST, Kevin Watters
darin: review-
Details | Formatted Diff | Diff
cleaned up version of the previous patch (864 bytes, patch)
2007-12-11 17:25 PST, Kevin Watters
no flags Details | Formatted Diff | Diff
cleaned up patch with changelog diff (1.38 KB, patch)
2007-12-12 09:30 PST, Kevin Watters
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin Watters 2007-12-11 12:50:35 PST
The WX port fails randomly with an ASSERT(!isDeletedBucket...) on line 721 of HashTable.h when navigating to several websites.

FontPlatformData's "Deleted" constructor appears to be the fault, since it will compare equal to a FontPlatformData created with the default constructor.
Comment 1 Kevin Watters 2007-12-11 12:53:25 PST
Created attachment 17854 [details]
Changes the FontPlatformData(Deleted) constructor so that m_font is -1 instead of 0

This patch follows the other ports in using -1 as a "Deleted" value which must be different than the NULL used in the default constructor.
Comment 2 Darin Adler 2007-12-11 16:05:11 PST
Comment on attachment 17854 [details]
Changes the FontPlatformData(Deleted) constructor so that m_font is -1 instead of 0

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.
Comment 3 Kevin Watters 2007-12-11 17:25:40 PST
Created attachment 17856 [details]
cleaned up version of the previous patch

Darin: I addressed the issues mentioned in your comment; attached is a much smaller patch, changing only the FontPlatformData(Deleted) constructor, and the destructor. Functions remain inlined in the header file.

Just as a note: I made the "switch" indentation changes after reading the WebKit Coding Style Guidelines (http://webkit.org/coding/coding-style.html), so if the example there doesn't reflect current dogma, that page should probably change :)
Comment 4 David Kilzer (:ddkilzer) 2007-12-12 06:27:38 PST
Comment on attachment 17856 [details]
cleaned up version of the previous patch

Needs ChangeLog.

Don't forget to set the review? flag!  :)
Comment 5 Kevin Watters 2007-12-12 09:30:04 PST
Created attachment 17865 [details]
cleaned up patch with changelog diff

Cleaned up patch, including a ChangeLog diff.
Comment 6 Darin Adler 2007-12-14 15:26:07 PST
Comment on attachment 17865 [details]
cleaned up patch with changelog diff

r=me
Comment 7 Kevin Ollivier 2007-12-15 17:58:09 PST
landed in r28766