| Summary: | [GTK] Release compilation fails when defining "LOG_DISABLED=0" | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Andres Gomez Garcia <agomez> | ||||||||
| Component: | DOM | Assignee: | Nobody <webkit-unassigned> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | andersca, cgarcia, commit-queue, darin, esprehn+autocc, gyuyoung.kim, kling, koivisto, mario, mrobinson | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
Created attachment 217438 [details]
Patch
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) Created attachment 217552 [details]
Patch
(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! 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 Created attachment 217556 [details]
Patch
Comment on attachment 217556 [details]
Patch
It seems we forgot to include/check the Reviewed by NOBODY line... OOPS!
Comment on attachment 217556 [details] Patch Clearing flags on attachment: 217556 Committed r159623: <http://trac.webkit.org/changeset/159623> All reviewed patches have been landed. Closing bug. |
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