Bug 45157 – [Qt] V8 port: Fix "WTF::String::utf8" returns incomplete type WTF::CString

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Vlad]

Eliminate ambiguous inclusion of animation.h and CString.h

[Vlad]

Created attachment 66453 [details]
Eliminate ambiguous inclusion of animation.h and CString.h for Symbian

[Andreas Kling]

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.

[Laszlo Gombos]

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.

[Darin Adler]

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?

[Vlad]

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.

[Vlad]

Tried #include <wtf/text/CString.h> in V8Proxy.h. Bindings compile fine, but generated files not :(

[Vlad]

(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 :(

[Vlad]

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

[Andreas Kling]

(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

[Adam Barth]

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.

[Andreas Kling]

Created attachment 67311 [details]
Proposed patch

[Simon Hausmann]

Comment on attachment 67311 [details]
Proposed patch

r=me

Explicit inclusion is better than implicit :)

[Andreas Kling]

Comment on attachment 67311 [details]
Proposed patch

Clearing flags on attachment: 67311

Committed r67308: <http://trac.webkit.org/changeset/67308>

[Andreas Kling]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/67308 might have broken GTK Linux 32-bit Debug