Bug 16351 - FontFallbackList.h doesn't include wtf/PassRefPtr.h
Summary: FontFallbackList.h doesn't include wtf/PassRefPtr.h
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Other
: P3 Trivial
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-12-07 20:01 PST by Robert Norris
Modified: 2007-12-14 12:57 PST (History)
0 users

See Also:


Attachments
Patch to include PassRefPtr.h in FontFallbackList.h (925 bytes, patch)
2007-12-08 14:55 PST, Robert Norris
darin: review-
Details | Formatted Diff | Diff
patch (5.12 KB, patch)
2007-12-09 08:42 PST, Darin Adler
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Norris 2007-12-07 20:01:35 PST
WebCore/platform/graphics/FontFallbackList.h uses PassRefPtr but doesn't include wtf/PassRefPtr.h. This causes the following build error on my port:

In file included from ../../../WebCore/platform/graphics/FontFallbackList.cpp:30:
../../../WebCore/platform/graphics/FontFallbackList.h:47: error: 'PassRefPtr' has not been declared
../../../WebCore/platform/graphics/FontFallbackList.h:47: error: expected ',' or '...' before '<' token
../../../WebCore/platform/graphics/FontFallbackList.cpp:45: error: prototype for 'void WebCore::FontFallbackList::invalidate(WTF::PassRefPtr<WebCore::FontSelector>)' does not match any in class 'WebCore::FontFallbackList'
../../../WebCore/platform/graphics/FontFallbackList.h:47: error: candidate is: void WebCore::FontFallbackList::invalidate(int)
make[1]: *** [tmp/FontFallbackList.o] Error 1

Adding the include resolves this. On my port nearly all my platform files are currently no-op stubs, so I guess on other ports PassRefPtr.h is being brought in elsewhere.

This at least occurs in r28509, but it doesn't seem to have been changed yet.
Comment 1 David Kilzer (:ddkilzer) 2007-12-08 14:18:18 PST
Please post a patch with a ChangeLog entry for review.  Thanks!

http://webkit.org/coding/contributing.html

Comment 2 Robert Norris 2007-12-08 14:55:04 PST
Created attachment 17791 [details]
Patch to include PassRefPtr.h in FontFallbackList.h
Comment 3 Darin Adler 2007-12-09 06:57:52 PST
Comment on attachment 17791 [details]
Patch to include PassRefPtr.h in FontFallbackList.h

This change is wrong.

When using PassRefPtr in a header, you don't need to include <wtf/PassRefPtr.h>. Instead you can include <wtf/Forward.h>.

But also, we have a test that FontFallbackList.h already includes everything it needs to be compiled on its own. That's the fact that FontFallbackList.cpp includes it first. If there was any missing dependency, that would catch it because it would fail to compile.

The way it gets Forward.h included is that it includes FontData.h, which includes FontPlatformData.h, which includes StringImpl.h, which includes Forward.h.

If there was any other header needed for PassRefPtr, it would need to be included by StringImpl.h.

In fact, there's no need for FontFallbackList.h to include RefCounted.h or Vector.h -- they are both included by StringImpl.h, so the correct patch would be to remove both of those wtf includes.
Comment 4 Darin Adler 2007-12-09 07:02:12 PST
Oh, I overlooked the fact that the FontPlatformData.h file in question was the Mac one. I suppose there's no guarantee that StringImpl.h will be included. So given that, it's fine to include <wtf/Forward.h> in FontFallbackList.h.

But I would also think we would need to include RefPtr.h -- was PassRefPtr,h alone really enough in your testing?
Comment 5 Darin Adler 2007-12-09 08:42:36 PST
Created attachment 17805 [details]
patch
Comment 6 Robert Norris 2007-12-09 16:03:54 PST
RefPtr.h was being included by GlyphPageTreeNode.h, like so:

    GlyphPageTreeNode.h
    wtf/HashMap.h
    wtf/HashTable.h
    wtf/HashTraits.h
    wtf/HashFunctions.h
    wtf/RefPtr.h

This is no longer a problem. At the time I had an empty FontPlatformData.h which was making this break. Now I don't (I include StringImpl.h and other things) and it doesn't happen.

I filed the bug because I thought it odd that things missing from my own files would cause other files to break. If you don't think thats a problem then there is no bug on my end :)

Patch looks good.
Comment 7 Darin Adler 2007-12-14 12:57:26 PST
Committed revision 28721.