Bug 82042

Summary: Unavailable pre-processor macro in non-Qt environments
Product: WebKit Reporter: Víctor M. Jáquez L. <vjaquez>
Component: WebKit Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: hausmann, kenneth, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch proposal to fix missing symbols in non-Qt environments
webkit-ews: commit-queue-
Patch proposal to fix missing symbols in non-Qt environments
hausmann: review-
Patch proposal to fix missing symbols in non-Qt environments v3 none

Description Víctor M. Jáquez L. 2012-03-23 03:45:16 PDT
I tried to compile WebKitGTK+ with this configuration:
    
/configure --prefix=/opt/jhbuild  \
           --with-accelerated-compositing=opengl \
           --with-gstreamer=0.10 \
           --enable-introspection
    
And my toolchain is: gcc (Debian 4.6.3-1) 4.6.3
    
When compiling I got a processor error given that QT_VERSION and QT_VERSION_CHECK are not available in my setup.
Comment 1 Víctor M. Jáquez L. 2012-03-23 03:46:44 PDT
The error message is this:

  CXX    Source/WebCore/platform/graphics/texmap/libWebCore_la-TextureMapperGL.lo
Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:74:52: error: missing binary operator before token "("
make[1]: *** [Source/WebCore/platform/graphics/texmap/libWebCore_la-TextureMapperGL.lo] Error 1
Comment 2 Víctor M. Jáquez L. 2012-03-23 03:58:27 PDT
Created attachment 133454 [details]
Patch proposal to fix missing symbols in non-Qt environments
Comment 3 Víctor M. Jáquez L. 2012-03-23 03:59:57 PDT
I add Kenneth because he reviewed the bug 81113 which patch introduced this bug.
Comment 4 Early Warning System Bot 2012-03-23 04:13:49 PDT
Comment on attachment 133454 [details]
Patch proposal to fix missing symbols in non-Qt environments

Attachment 133454 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12114563
Comment 5 Xan Lopez 2012-03-23 07:01:30 PDT
Comment on attachment 133454 [details]
Patch proposal to fix missing symbols in non-Qt environments

View in context: https://bugs.webkit.org/attachment.cgi?id=133454&action=review

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:-74
> -#if PLATFORM(QT) && (QT_VERSION >= QT_VERSION_CHECK(5, 0, 0))

This seems to fail in the Qt EWS becasue they don't have Qt5 so GLContext is not defined (so I'd say the code was wrong to begin with?).

Other than that I'm not quite sure why the change in the preprocessor stuff is needed, I'd expect && to do the right thing and shortcircuit evaluation.
Comment 6 Víctor M. Jáquez L. 2012-03-23 07:07:44 PDT
I realized that my patch did not provides a valid path when Qt is previous than 5.0

I'll provide a new version of it after.
Comment 7 Víctor M. Jáquez L. 2012-03-23 07:55:14 PDT
Created attachment 133487 [details]
Patch proposal to fix missing symbols in non-Qt environments
Comment 8 Simon Hausmann 2012-03-26 06:26:52 PDT
Comment on attachment 133487 [details]
Patch proposal to fix missing symbols in non-Qt environments

I'd prefer if this was written without QT_VERSION_CHECK instead of introducing _QT5, i.e.:

#if PLATFORM(QT) && QT_VERSION >= 0x050000
Comment 9 Víctor M. Jáquez L. 2012-03-27 04:35:23 PDT
Created attachment 134023 [details]
Patch proposal to fix missing symbols in non-Qt environments v3
Comment 10 Víctor M. Jáquez L. 2012-03-27 04:36:11 PDT
(In reply to comment #8)
> (From update of attachment 133487 [details])
> I'd prefer if this was written without QT_VERSION_CHECK instead of introducing _QT5, i.e.:
> 
> #if PLATFORM(QT) && QT_VERSION >= 0x050000

The version 3 of the patch uses this approach. Thanks!
Comment 11 Simon Hausmann 2012-03-27 04:55:23 PDT
(In reply to comment #10)
> (In reply to comment #8)
> > (From update of attachment 133487 [details] [details])
> > I'd prefer if this was written without QT_VERSION_CHECK instead of introducing _QT5, i.e.:
> > 
> > #if PLATFORM(QT) && QT_VERSION >= 0x050000
> 
> The version 3 of the patch uses this approach. Thanks!

I suggest to mark the patch for review (r?) in order for the EWS to start building and us being able to review it :)
Comment 12 Víctor M. Jáquez L. 2012-03-27 06:25:32 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #8)
> > > (From update of attachment 133487 [details] [details] [details])
> > > I'd prefer if this was written without QT_VERSION_CHECK instead of introducing _QT5, i.e.:
> > > 
> > > #if PLATFORM(QT) && QT_VERSION >= 0x050000
> > 
> > The version 3 of the patch uses this approach. Thanks!
> 
> I suggest to mark the patch for review (r?) in order for the EWS to start building and us being able to review it :)

Oops! I guess now it's correct. Thanks!
Comment 13 Víctor M. Jáquez L. 2012-03-29 07:07:12 PDT
Comment on attachment 134023 [details]
Patch proposal to fix missing symbols in non-Qt environments v3

Simon would you, please, add the c-q + in this patch, because I'm not a committer. Thanks!
Comment 14 WebKit Review Bot 2012-03-29 07:32:11 PDT
Comment on attachment 134023 [details]
Patch proposal to fix missing symbols in non-Qt environments v3

Clearing flags on attachment: 134023

Committed r112530: <http://trac.webkit.org/changeset/112530>
Comment 15 WebKit Review Bot 2012-03-29 07:32:18 PDT
All reviewed patches have been landed.  Closing bug.