Bug 124661 – [GTK] Release compilation fails when defining "LOG_DISABLED=0"

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Andres Gomez Garcia]

This is happening at least in the GTK port, although I think it would happen also in other ports

$ export CPPFLAGS="-DLOG_DISABLED=0"
$ ./Tools/Scripts/build-webkit --gtk

...

  CXX    Source/WebCore/html/libWebCore_la-ImageData.lo                                                                                                                                                                                       
../../Source/WebCore/html/HTMLTrackElement.cpp: In member function 'virtual bool WebCore::HTMLTrackElement::canLoadUrl(const WebCore::URL&)':                                                                                                 
../../Source/WebCore/html/HTMLTrackElement.cpp:251:131: error: invalid use of incomplete type 'class WTF::CString'                                                                                                                            
In file included from ../../Source/WebCore/platform/URL.h:29:0,                                                                                                                                                                               
                 from ../../Source/WebCore/platform/URLHash.h:29,                                                                                                                                                                             
                 from ../../Source/WebCore/css/CSSValue.h:25,                                                                                                                                                                                 
                 from ../../Source/WebCore/css/CSSPrimitiveValue.h:26,                                                                                                                                                                        
                 from ../../Source/WebCore/dom/StyledElement.h:28,                                                                                                                                                                            
                 from ../../Source/WebCore/html/HTMLElement.h:26,                                                                                                                                                                             
                 from ../../Source/WebCore/html/HTMLTrackElement.h:30,                                                                                                                                                                        
                 from ../../Source/WebCore/html/HTMLTrackElement.cpp:28:                                                                                                                                                                      
../../Source/WTF/wtf/Forward.h:42:7: error: forward declaration of 'class WTF::CString'                                                                                                                                                       
make[1]: *** [Source/WebCore/html/libWebCore_la-HTMLTrackElement.lo] Error 1                                                                                                                                                                  
make[1]: *** Waiting for unfinished jobs....                                                                                                                                                                                                  
make[1]: Leaving directory `/opt/webkit/WebKit-local.git/WebKitBuild/Release'                                                                                                                                                                 
make: *** [all] Error 2

[Andres Gomez Garcia]

Created attachment 217438 [details]
Patch

[Mario Sanchez Prada]

Comment on attachment 217438 [details]
Patch

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

> Source/WebCore/html/HTMLTrackElement.cpp:36
> +#if !LOG_DISABLED

The usage of the CString class inside this file is not behind any !LOG_DISABLED guard so I think you should not put the include behind it either, because if you do it so it would be because you know how that macro is being defined, and not based in the information you have by looking at this implementation file only.

So, even though calls to LOG() won't ever be translated to anything requiring CString if !LOG_DISABLED (and that's why it does not fail otherwise), I believe it's better to be consistent and just include CString normally, as it's done in other places (e.g. HTMLMediaElement.cpp)

[Andres Gomez Garcia]

Created attachment 217552 [details]
Patch

[Andres Gomez Garcia]

(In reply to comment #2)
> (From update of attachment 217438 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217438&action=review
> 
> > Source/WebCore/html/HTMLTrackElement.cpp:36
> > +#if !LOG_DISABLED
> 
> The usage of the CString class inside this file is not behind any !LOG_DISABLED guard so I think you should not put the include behind it either, because if you do it so it would be because you know how that macro is being defined, and not based in the information you have by looking at this implementation file only.
> 
> So, even though calls to LOG() won't ever be translated to anything requiring CString if !LOG_DISABLED (and that's why it does not fail otherwise), I believe it's better to be consistent and just include CString normally, as it's done in other places (e.g. HTMLMediaElement.cpp)
...

Done.

Thanks for the review!

[WebKit Commit Bot]

Comment on attachment 217552 [details]
Patch

Rejecting attachment 217552 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 217552, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.appspot.com/results/31728045

[Andres Gomez Garcia]

Created attachment 217556 [details]
Patch

[Mario Sanchez Prada]

Comment on attachment 217556 [details]
Patch

It seems we forgot to include/check the Reviewed by NOBODY line... OOPS!

[WebKit Commit Bot]

Comment on attachment 217556 [details]
Patch

Clearing flags on attachment: 217556

Committed r159623: <http://trac.webkit.org/changeset/159623>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.