124661 2013-11-20 08:34:05 -0800 [GTK] Release compilation fails when defining "LOG_DISABLED=0" 2013-11-21 06:50:58 -0800 1 1 1 Unclassified WebKit DOM 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 agomez webkit-unassigned andersca cgarcia commit-queue darin esprehn+autocc gyuyoung.kim kling koivisto mario mrobinson oldest_to_newest 952601 0 agomez 2013-11-20 08:34:05 -0800 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 952614 1 217438 agomez 2013-11-20 08:57:06 -0800 Created attachment 217438 Patch 953049 2 217438 mario 2013-11-21 05:56:41 -0800 Comment on attachment 217438 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) 953052 3 217552 agomez 2013-11-21 06:03:35 -0800 Created attachment 217552 Patch 953053 4 agomez 2013-11-21 06:04:41 -0800 (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! 953057 5 217552 commit-queue 2013-11-21 06:10:55 -0800 Comment on attachment 217552 Patch Rejecting attachment 217552 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 953061 6 217556 agomez 2013-11-21 06:22:33 -0800 Created attachment 217556 Patch 953062 7 217556 mario 2013-11-21 06:25:21 -0800 Comment on attachment 217556 Patch It seems we forgot to include/check the Reviewed by NOBODY line... OOPS! 953071 8 217556 commit-queue 2013-11-21 06:50:55 -0800 Comment on attachment 217556 Patch Clearing flags on attachment: 217556 Committed r159623: <http://trac.webkit.org/changeset/159623> 953072 9 commit-queue 2013-11-21 06:50:58 -0800 All reviewed patches have been landed. Closing bug. 217438 2013-11-20 08:57:06 -0800 2013-11-21 06:03:25 -0800 Patch bug-124661-20131120185705.patch text/plain 1330 agomez U3VidmVyc2lvbiBSZXZpc2lvbjogMTU5Mjg2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggODNiZDAwZDljN2IxMjU0 YjNhMzUyNjc0MGU5MDcwMzM5MmZlNjdiZS4uOTFjNmI0MzZmMmMwYjcxZjBiMjNmNzZlOGMxYjVm ZTQxMDdjMWQyZSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDEzLTExLTIwICBBbmRy ZXMgR29tZXogIDxhZ29tZXpAaWdhbGlhLmNvbT4KKworICAgICAgICBbR1RLXSBSZWxlYXNlIGNv bXBpbGF0aW9uIGZhaWxzIHdoZW4gZGVmaW5pbmcgIkxPR19ESVNBQkxFRD0wIgorICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTI0NjYxCisKKyAgICAgICAg SW4gYSAiRGVidWciIGJ1aWxkIHRoZSBDU3RyaW5nLmggaGVhZGVyIGNvbWVzIGZyb20gYW5vdGhl cgorICAgICAgICBpbmRpcmVjdCBkZXBlbmRlbmN5LiBOb3csIHdlIGV4cGxpY2l0bHkgYWRkIHRo aXMgbWlzc2luZyBpbmNsdWRlCisgICAgICAgIHdoZW4gbG9nZ2luZyBpcyBub3QgZGlzYWJsZWQu CisKKyAgICAgICAgKiBodG1sL0hUTUxUcmFja0VsZW1lbnQuY3BwOiBFeHBsaWNpdGx5IGFkZGlu ZyBtaXNzaW5nIGluY2x1ZGUKKyAgICAgICAgd2hlbiBsb2dnaW5nIGlzIG5vdCBkaXNhYmxlZC4K KwogMjAxMy0xMS0xNCAgU2Vva2p1IEt3b24gIDxzZW9ranVAd2Via2l0Lm9yZz4KIAogICAgICAg ICBVc2UgW0ltcGxlbWVudGVkQXM9ZGVmYXVsdFN0YXR1c10gZm9yIFdpbmRvdy5kZWZhdWx0c3Rh dHVzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9odG1sL0hUTUxUcmFja0VsZW1lbnQuY3Bw IGIvU291cmNlL1dlYkNvcmUvaHRtbC9IVE1MVHJhY2tFbGVtZW50LmNwcAppbmRleCA4YzliNzU4 ZjNiYWZhMDM3YTlmY2I3OTA5OTFkNmM3YWE2MzllMzg1Li4xODMxOWQzYzQzZTJmM2NkMzc4MWY5 OTVkM2U0YWFmNGQ1ZDJkM2EyIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9odG1sL0hUTUxU cmFja0VsZW1lbnQuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL2h0bWwvSFRNTFRyYWNrRWxlbWVu dC5jcHAKQEAgLTMzLDYgKzMzLDkgQEAKICNpbmNsdWRlICJIVE1MTmFtZXMuaCIKICNpbmNsdWRl ICJMb2dnaW5nLmgiCiAjaW5jbHVkZSAiUnVudGltZUVuYWJsZWRGZWF0dXJlcy5oIgorI2lmICFM T0dfRElTQUJMRUQKKyNpbmNsdWRlIDx3dGYvdGV4dC9DU3RyaW5nLmg+CisjZW5kaWYKIAogbmFt ZXNwYWNlIFdlYkNvcmUgewogCg== 217552 2013-11-21 06:03:35 -0800 2013-11-21 06:22:25 -0800 Patch bug-124661-20131121160333.patch text/plain 1227 agomez U3VidmVyc2lvbiBSZXZpc2lvbjogMTU5Mjg2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggODNiZDAwZDljN2IxMjU0 YjNhMzUyNjc0MGU5MDcwMzM5MmZlNjdiZS4uYmExZjY3ZWM4NTlmZDZjN2QwNThjNjE4NmI0YTM0 ZTY5OGY2MjhlOSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDEzIEBACisyMDEzLTExLTIxICBBbmRy ZXMgR29tZXogIDxhZ29tZXpAaWdhbGlhLmNvbT4KKworICAgICAgICBbR1RLXSBSZWxlYXNlIGNv bXBpbGF0aW9uIGZhaWxzIHdoZW4gZGVmaW5pbmcgIkxPR19ESVNBQkxFRD0wIgorICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTI0NjYxCisKKyAgICAgICAg SW4gYSAiRGVidWciIGJ1aWxkIHRoZSBDU3RyaW5nLmggaGVhZGVyIGNvbWVzIGZyb20gYW5vdGhl cgorICAgICAgICBpbmRpcmVjdCBkZXBlbmRlbmN5LiBOb3csIHdlIGV4cGxpY2l0bHkgYWRkIHRo aXMgbWlzc2luZyBpbmNsdWRlLgorCisgICAgICAgICogaHRtbC9IVE1MVHJhY2tFbGVtZW50LmNw cDogRXhwbGljaXRseSBhZGRpbmcgbWlzc2luZyBpbmNsdWRlLgorCiAyMDEzLTExLTE0ICBTZW9r anUgS3dvbiAgPHNlb2tqdUB3ZWJraXQub3JnPgogCiAgICAgICAgIFVzZSBbSW1wbGVtZW50ZWRB cz1kZWZhdWx0U3RhdHVzXSBmb3IgV2luZG93LmRlZmF1bHRzdGF0dXMKZGlmZiAtLWdpdCBhL1Nv dXJjZS9XZWJDb3JlL2h0bWwvSFRNTFRyYWNrRWxlbWVudC5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9o dG1sL0hUTUxUcmFja0VsZW1lbnQuY3BwCmluZGV4IDhjOWI3NThmM2JhZmEwMzdhOWZjYjc5MDk5 MWQ2YzdhYTYzOWUzODUuLjA2OWIyN2E1ZWE0NjFkNDQzOWY5NzRkZjIxZWFkMzdjZTc3NWQ4ZWIg MTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL2h0bWwvSFRNTFRyYWNrRWxlbWVudC5jcHAKKysr IGIvU291cmNlL1dlYkNvcmUvaHRtbC9IVE1MVHJhY2tFbGVtZW50LmNwcApAQCAtMzMsNiArMzMs NyBAQAogI2luY2x1ZGUgIkhUTUxOYW1lcy5oIgogI2luY2x1ZGUgIkxvZ2dpbmcuaCIKICNpbmNs dWRlICJSdW50aW1lRW5hYmxlZEZlYXR1cmVzLmgiCisjaW5jbHVkZSA8d3RmL3RleHQvQ1N0cmlu Zy5oPgogCiBuYW1lc3BhY2UgV2ViQ29yZSB7CiAK 217556 2013-11-21 06:22:33 -0800 2013-11-21 06:50:54 -0800 Patch bug-124661-20131121162232.patch text/plain 1266 agomez U3VidmVyc2lvbiBSZXZpc2lvbjogMTU5Mjg2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggODNiZDAwZDljN2IxMjU0 YjNhMzUyNjc0MGU5MDcwMzM5MmZlNjdiZS4uM2IzMjgyYWJlMzg1YTg2NGRmMTVkNzExN2E2Y2Rj NjZlMWI1OWE2NyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDEzLTExLTIxICBBbmRy ZXMgR29tZXogIDxhZ29tZXpAaWdhbGlhLmNvbT4KKworICAgICAgICBbR1RLXSBSZWxlYXNlIGNv bXBpbGF0aW9uIGZhaWxzIHdoZW4gZGVmaW5pbmcgIkxPR19ESVNBQkxFRD0wIgorICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTI0NjYxCisKKyAgICAgICAg UmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgSW4gYSAiRGVidWciIGJ1aWxk IHRoZSBDU3RyaW5nLmggaGVhZGVyIGNvbWVzIGZyb20gYW5vdGhlcgorICAgICAgICBpbmRpcmVj dCBkZXBlbmRlbmN5LiBOb3csIHdlIGV4cGxpY2l0bHkgYWRkIHRoaXMgbWlzc2luZyBpbmNsdWRl LgorCisgICAgICAgICogaHRtbC9IVE1MVHJhY2tFbGVtZW50LmNwcDogRXhwbGljaXRseSBhZGRp bmcgbWlzc2luZyBpbmNsdWRlLgorCiAyMDEzLTExLTE0ICBTZW9ranUgS3dvbiAgPHNlb2tqdUB3 ZWJraXQub3JnPgogCiAgICAgICAgIFVzZSBbSW1wbGVtZW50ZWRBcz1kZWZhdWx0U3RhdHVzXSBm b3IgV2luZG93LmRlZmF1bHRzdGF0dXMKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL2h0bWwv SFRNTFRyYWNrRWxlbWVudC5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9odG1sL0hUTUxUcmFja0VsZW1l bnQuY3BwCmluZGV4IDhjOWI3NThmM2JhZmEwMzdhOWZjYjc5MDk5MWQ2YzdhYTYzOWUzODUuLjA2 OWIyN2E1ZWE0NjFkNDQzOWY5NzRkZjIxZWFkMzdjZTc3NWQ4ZWIgMTAwNjQ0Ci0tLSBhL1NvdXJj ZS9XZWJDb3JlL2h0bWwvSFRNTFRyYWNrRWxlbWVudC5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUv aHRtbC9IVE1MVHJhY2tFbGVtZW50LmNwcApAQCAtMzMsNiArMzMsNyBAQAogI2luY2x1ZGUgIkhU TUxOYW1lcy5oIgogI2luY2x1ZGUgIkxvZ2dpbmcuaCIKICNpbmNsdWRlICJSdW50aW1lRW5hYmxl ZEZlYXR1cmVzLmgiCisjaW5jbHVkZSA8d3RmL3RleHQvQ1N0cmluZy5oPgogCiBuYW1lc3BhY2Ug V2ViQ29yZSB7CiAK