Eliminate ambiguous inclusion of animation.h and CString.h
Created attachment 66453 [details] Eliminate ambiguous inclusion of animation.h and CString.h for Symbian
Comment on attachment 66453 [details] Eliminate ambiguous inclusion of animation.h and CString.h for Symbian > +#if PLATFORM(QT) > +#include <platform/animation/Animation.h> > +#else > #include "Animation.h" > +#endif Does this really need to be Qt-specific? > +#include <wtf/text/CString.h> Why is this necessary? CString.h is always referenced by wtf/text/CString.h AFAIK.
I agree with Andreas comments. Also I think these changes are not just for V8 but perhaps generally needed to make reliable builds on Symbian with JSC.
Comment on attachment 66453 [details] Eliminate ambiguous inclusion of animation.h and CString.h for Symbian > +#if PLATFORM(QT) > +#include <platform/animation/Animation.h> > +#else > #include "Animation.h" > +#endif This is a bad direction -- it does not seem like a good long-term solution to have platform-specific different ways of including header files. Can we instead fix the paths passed in to the compiler so that WebCore compiles will find WebCore headers first, before system headers? > +#include <wtf/text/CString.h> We don't want every single file that includes PlatformString to also include CString. Why is this a good idea?
Animation.h and cstring.h files are present in Symbian system include folders as well. At the time of the port there was conflict between system includes and webkit. It is fixed now by include order... (In reply to comment #2) > (From update of attachment 66453 [details]) > > +#if PLATFORM(QT) > > +#include <platform/animation/Animation.h> > > +#else > > #include "Animation.h" > > +#endif > This one prevents v8 binding compilation errors on symbian. "WebCore\bindings\v8\V8Proxy.cpp", line 196: Error: #409: function "WTF::String::utf8" returns incomplete type "WTF::CString" targetDocument->url().string().utf8().data() ^ Otherwise I need to include #include <wtf/text/CString.h> in many v8 binding files > Does this really need to be Qt-specific? > > > +#include <wtf/text/CString.h> > > Why is this necessary? CString.h is always referenced by wtf/text/CString.h AFAIK.
Tried #include <wtf/text/CString.h> in V8Proxy.h. Bindings compile fine, but generated files not :(
(In reply to comment #6) RVCT complains about incomplete type: FAILED compile for armv5_urel: generated\InspectorBackendDispatcher.cpp mmp: WebCore_0x200267C2.mmp "\webkit\WebCore\generated\InspectorBackendDispatcher.cpp", line 3688: Error: #409: function "WTF::String::utf8" returns incomplete type WTF::CString" reportProtocolError(callId, String::format("Protocol Error: Invalid command was received. '%s' wasn't found.", command.utf8().data())); ^ X:\webkit.trunk1\WebCore\generated\InspectorBackendDispatcher.cpp: 0 warnings, 1 error > Tried #include <wtf/text/CString.h> in V8Proxy.h. Bindings compile fine, but generated files not :(
Created attachment 66579 [details] added CString.h to QT section of WTFString.h I think I found compromise - added CString.h to QT section of WTFString.h
(In reply to comment #6) > Tried #include <wtf/text/CString.h> in V8Proxy.h. Bindings compile fine, but generated files not :( If this is indeed the problem, it would be better to modify the code generator script(s) to include the necessary header instead of messing with WTFString.h
Comment on attachment 66579 [details] added CString.h to QT section of WTFString.h I agree with kling. This doesn't seem like the right place to include the header. If it were the right place, we should do so unconditionally anyway.
Created attachment 67311 [details] Proposed patch
Comment on attachment 67311 [details] Proposed patch r=me Explicit inclusion is better than implicit :)
Comment on attachment 67311 [details] Proposed patch Clearing flags on attachment: 67311 Committed r67308: <http://trac.webkit.org/changeset/67308>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/67308 might have broken GTK Linux 32-bit Debug