Bug 42447

Summary: DeviceOrientationEvent.h should not forward-declare DeviceOrientation
Product: WebKit Reporter: Hans Wennborg <hans>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, steveblock
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch none

Description Hans Wennborg 2010-07-16 04:51:10 PDT
Because DeviceOrientationEvent has a member m_orientation of type RefPtr<DeviceOrientation>, the generated ~DeviceOrientationEvent() will destruct the RefPtr and eventually end up doing ptr->deref() in PassRefPtr.h:58, with ptr as an incomplete type.

Clang gives an error about this when building Chrome:

In file included from third_party/WebKit/WebCore/dom/Document.cpp:27:
In file included from third_party/WebKit/WebCore/dom/Document.h:29:
In file included from third_party/WebKit/WebCore/loader/CachedResourceHandle.h:29:
In file included from third_party/WebKit/WebCore/loader/CachedResource.h:28:
In file included from third_party/WebKit/WebCore/platform/text/PlatformString.h:28:
In file included from third_party/WebKit/JavaScriptCore/wtf/text/WTFString.h:28:
In file included from third_party/WebKit/JavaScriptCore/wtf/text/StringImpl.h:28:
In file included from third_party/WebKit/JavaScriptCore/wtf/CrossThreadRefCounted.h:35:
third_party/WebKit/JavaScriptCore/wtf/PassRefPtr.h:58:16: error: member access into incomplete type
      'WebCore::DeviceOrientation'
            ptr->deref();
               ^
In file included from third_party/WebKit/WebCore/dom/Document.cpp:27:
In file included from third_party/WebKit/WebCore/dom/Document.h:29:
In file included from third_party/WebKit/WebCore/loader/CachedResourceHandle.h:29:
In file included from third_party/WebKit/WebCore/loader/CachedResource.h:28:
In file included from third_party/WebKit/WebCore/platform/text/PlatformString.h:28:
In file included from third_party/WebKit/JavaScriptCore/wtf/text/WTFString.h:28:
In file included from third_party/WebKit/JavaScriptCore/wtf/text/StringImpl.h:32:
In file included from third_party/WebKit/JavaScriptCore/wtf/Vector.h:28:
In file included from third_party/WebKit/JavaScriptCore/wtf/VectorTraits.h:25:
third_party/WebKit/JavaScriptCore/wtf/RefPtr.h:56:35: note: in instantiation of function template specialization
      'WTF::derefIfNotNull<WebCore::DeviceOrientation>' requested here
        ALWAYS_INLINE ~RefPtr() { derefIfNotNull(m_ptr); }
                                  ^
In file included from third_party/WebKit/WebCore/dom/Document.cpp:47:
third_party/WebKit/WebCore/dom/DeviceOrientationEvent.h:35:7: note: in instantiation of member function
      'WTF::RefPtr<WebCore::DeviceOrientation>::~RefPtr' requested here
class DeviceOrientationEvent : public Event {
      ^
third_party/WebKit/WebCore/dom/DeviceOrientationEvent.h:33:7: note: forward declaration of
      'WebCore::DeviceOrientation'
class DeviceOrientation;
Comment 1 Hans Wennborg 2010-07-16 04:59:11 PDT
Created attachment 61795 [details]
Patch
Comment 2 Steve Block 2010-07-16 05:11:16 PDT
Comment on attachment 61795 [details]
Patch

r=me
Comment 3 WebKit Commit Bot 2010-07-16 06:50:47 PDT
Comment on attachment 61795 [details]
Patch

Clearing flags on attachment: 61795

Committed r63544: <http://trac.webkit.org/changeset/63544>
Comment 4 WebKit Commit Bot 2010-07-16 06:50:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Darin Adler 2010-07-16 09:36:55 PDT
Adding the include is one solution in a case like this. Another is to explicitly declare the virtual destructor and define it in the .cpp file. It will have just an empty body, but it will make sure that people who include the header don’t have to compile the destructor.
Comment 6 Hans Wennborg 2010-07-16 09:47:48 PDT
(In reply to comment #5)
> Adding the include is one solution in a case like this. Another is to explicitly declare the virtual destructor and define it in the .cpp file. It will have just an empty body, but it will make sure that people who include the header don’t have to compile the destructor.

Ah, I didn't think of that. Do you think we should change it?
Comment 7 Hans Wennborg 2010-07-16 10:19:42 PDT
Uploaded patch to Bug 42466 that implements Darin's suggestion, as it reduces the header size.