Bug 70913

Created attachment 112493 [details] patch

For this bug and bug 70914 you need to at least flag the patch with an "r?" so reviewers can notice they exist.

Attachment 112493 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSClassRef.cpp',..." exit_code: 1 Source/JavaScriptCore/wtf/unicode/WTFUTF8.h:26: #ifndef header guard has wrong style, please use: WTF_WTFUTF8_h [build/header_guard] [5] Source/JavaScriptCore/wtf/unicode/WTFUTF8.h:37: Code inside a namespace should not be indented. [whitespace/indent] [4] Source/JavaScriptCore/wtf/unicode/WTFUTF8.h:45: One space before end of line comments [whitespace/comments] [5] Source/JavaScriptCore/wtf/unicode/WTFUTF8.h:46: One space before end of line comments [whitespace/comments] [5] Source/JavaScriptCore/wtf/unicode/WTFUTF8.h:47: One space before end of line comments [whitespace/comments] [5] Source/JavaScriptCore/wtf/unicode/WTFUTF8.h:48: One space before end of line comments [whitespace/comments] [5] Source/JavaScriptCore/wtf/unicode/WTFUTF8.h:53: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Source/JavaScriptCore/wtf/unicode/WTFUTF8.h:75: The parameter name "a" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 8 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 112493 [details] patch Attachment 112493 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10213584

Comment on attachment 112493 [details] patch Attachment 112493 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10208754

Comment on attachment 112493 [details] patch Attachment 112493 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10213599

> it confuses wtf/unicode/UTF8.h with unicode/utf8.h What exactly are the errors? The idea is that WTF header is always included as <wtf/unicode/UTF8.h>, while WebCore one is included as "unicode/utf8.h". I don't see how a compiler can be confused about that. No other ICU headers should be in the search path.

(In reply to comment #0) > it confuses wtf/unicode/UTF8.h with unicode/utf8.h > > this patch fixes that by renaming UTF8.h to WTFUTF8.h > > It patches all includes and patches all build systems Please, post the error message.

Local .h and .cpp files (internal icu and internal wtf/unicode) include them as "utf8.h" and "UTF8.h" respectively. (E.g. wtf/unicode/UTF8.cpp) Since we cannot fix icu, and we cannot simply swap the include order, we have to rename UTF8.h. (Or use a case sensitive drive, ...) I've refreshed the patch, removing the stale U_DISABLE_RENAMING, should fix other platforms building it.

Created attachment 112645 [details] refreshed, removing the U_DISABLE_RENAMING

this is the error without the patch: In file included from /Users/ogorter/WebKit/Source/WebCore/platform/text/TextCodecUTF8.cpp:28: /Users/ogorter/WebKit/Source/WebCore/platform/text/TextCodecUTF8.h:50: error: 'U8_MAX_LENGTH' was not declared in this scope /Users/ogorter/WebKit/Source/WebCore/platform/text/TextCodecUTF8.cpp: In member function 'void WebCore::TextCodecUTF8::consumePartialSequenceByte()': /Users/ogorter/WebKit/Source/WebCore/platform/text/TextCodecUTF8.cpp:156: error: 'm_partialSequence' was not declared in this scope /Users/ogorter/WebKit/Source/WebCore/platform/text/TextCodecUTF8.cpp: In member function 'void WebCore::TextCodecUTF8::handlePartialSequence(UChar*&, const uint8_t*&, const uint8_t*, bool, bool, bool&)': /Users/ogorter/WebKit/Source/WebCore/platform/text/TextCodecUTF8.cpp:173: error: 'm_partialSequence' was not declared in this scope /Users/ogorter/WebKit/Source/WebCore/platform/text/TextCodecUTF8.cpp:178: error: 'm_partialSequence' was not declared in this scope /Users/ogorter/WebKit/Source/WebCore/platform/text/TextCodecUTF8.cpp: In member function 'virtual WTF::String WebCore::TextCodecUTF8::decode(const char*, size_t, bool, bool, bool&)': /Users/ogorter/WebKit/Source/WebCore/platform/text/TextCodecUTF8.cpp:271: error: 'm_partialSequence' was not declared in this scope /Users/ogorter/WebKit/Source/WebCore/platform/text/TextCodecUTF8.cpp: In member function 'virtual WTF::CString WebCore::TextCodecUTF8::encode(const UChar*, size_t, WebCore::UnencodableHandling)': /Users/ogorter/WebKit/Source/WebCore/platform/text/TextCodecUTF8.cpp:310: error: 'U8_APPEND_UNSAFE' was not declared in this scope make[2]: *** [WebCore/CMakeFiles/webcore_efl.dir/platform/text/TextCodecUTF8.cpp.o] Error 1 make[1]: *** [WebCore/CMakeFiles/webcore_efl.dir/all] Error 2 make: *** [all] Error 2 U8_MAC_LENGTH is defined in unicode/utf8.h included (very) indirectly from unicode/uchar.h but the include there redirects to wtf/unicode/UTF8.h on a case sensitive drive ...

Attachment 112645 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSClassRef.cpp',..." exit_code: 1 Source/JavaScriptCore/wtf/unicode/WTFUTF8.h:26: #ifndef header guard has wrong style, please use: WTF_WTFUTF8_h [build/header_guard] [5] Source/JavaScriptCore/wtf/unicode/WTFUTF8.h:37: Code inside a namespace should not be indented. [whitespace/indent] [4] Source/JavaScriptCore/wtf/unicode/WTFUTF8.h:45: One space before end of line comments [whitespace/comments] [5] Source/JavaScriptCore/wtf/unicode/WTFUTF8.h:46: One space before end of line comments [whitespace/comments] [5] Source/JavaScriptCore/wtf/unicode/WTFUTF8.h:47: One space before end of line comments [whitespace/comments] [5] Source/JavaScriptCore/wtf/unicode/WTFUTF8.h:48: One space before end of line comments [whitespace/comments] [5] Source/JavaScriptCore/wtf/unicode/WTFUTF8.h:53: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Source/JavaScriptCore/wtf/unicode/WTFUTF8.h:75: The parameter name "a" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 8 in 19 files If any of these errors are false positives, please file a bug against check-webkit-style.

> Since we cannot fix icu I still don't understand what the problem is, but if really needed, we could changes our copy of ICU headers. Again, building with any other ICU headers is not supposed to work.

> building with any other ICU headers well ... that doesn't make much sense, but that is not the issue actually, you might be right on how the headers are included: ICU headers use <unicode/utf8.h> or "utf8.h" and wtf uses <wtf/unicode/UTF8.h> or "UTF8.h" so it might just work if I remove the wtf/unicode from the search path *and* move the order around. If that works, the issue can be solved in the CMake (or any other build system that might run into this).

*** Bug 72152 has been marked as a duplicate of this bug. ***

*** Bug 88307 has been marked as a duplicate of this bug. ***

(In reply to comment #16) > *** Bug 88307 has been marked as a duplicate of this bug. *** I made digging and you can find what the exact problem is: https://bugs.webkit.org/show_bug.cgi?id=88307#c2 In nutshell: An ICU header (ICU/include/unicode/utf.h) includes "#include "unicode/utf8.h". It wants to include its own unicode/utf8.h, not WTF's unicode/UTF8.h. They are completely different. (In reply to comment #13) > > Since we cannot fix icu > > I still don't understand what the problem is, but if really needed, we could changes our copy of ICU headers. Again, building with any other ICU headers is not supposed to work. I think only Apple port uses the ICU headers checked into the WebKit source, all other ports use the installed system headers. Similar to other 3rd party dev packages.

Created attachment 146502 [details] Patch Based on my previous patch in Bug 88307 and some style fix with check-webkit-stlye suggested in the previous bug.

Attachment 146502 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSClassRef.cpp',..." exit_code: 1 Source/WTF/wtf/unicode/WTFUTF8.h:45: One space before end of line comments [whitespace/comments] [5] Source/WTF/wtf/unicode/WTFUTF8.h:46: One space before end of line comments [whitespace/comments] [5] Source/WTF/wtf/unicode/WTFUTF8.h:47: One space before end of line comments [whitespace/comments] [5] Source/WTF/wtf/unicode/WTFUTF8.h:48: One space before end of line comments [whitespace/comments] [5] Source/WTF/wtf/unicode/WTFUTF8.h:79: The parameter name "a" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WTF/wtf/unicode/WTFUTF8.cpp:65: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WTF/wtf/unicode/WTFUTF8.cpp:264: More than one command on the same line in if [whitespace/parens] [4] Source/WTF/wtf/unicode/WTFUTF8.cpp:266: More than one command on the same line in if [whitespace/parens] [4] Source/WTF/wtf/unicode/WTFUTF8.cpp:268: More than one command on the same line in if [whitespace/parens] [4] Source/WTF/wtf/unicode/WTFUTF8.cpp:273: More than one command on the same line in if [whitespace/parens] [4] Source/WTF/wtf/unicode/WTFUTF8.cpp:275: More than one command on the same line in if [whitespace/parens] [4] Source/WTF/wtf/unicode/WTFUTF8.cpp:277: More than one command on the same line in if [whitespace/parens] [4] Source/WTF/wtf/unicode/WTFUTF8.cpp:279: More than one command on the same line in if [whitespace/parens] [4] Source/WTF/wtf/unicode/WTFUTF8.cpp:281: More than one command on the same line in if [whitespace/parens] [4] Source/WTF/wtf/unicode/WTFUTF8.cpp:284: More than one command on the same line in if [whitespace/parens] [4] Total errors found: 15 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 146502 [details] Patch Attachment 146502 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12917443

Comment on attachment 146502 [details] Patch Attachment 146502 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12909577 New failing tests: svg/text/non-bmp-positioning-lists.svg fast/parser/entities-in-xhtml.xhtml fast/url/anchor.html fast/encoding/xml-utf-8-default.xml

Created attachment 146518 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

> I think only Apple port uses the ICU headers checked into the WebKit > source, all other ports use the installed system headers. Similar to > other 3rd party dev packages. This sounds like the root of the problem then. Using a different copy of headers does nothing good, and just causes build issues. If there is something new in system copy, you generally still cannot use it in WebKit code without breaking other platforms anyway. > WTFUTF8.h I hate this name.

Created attachment 146876 [details] Patch Solving the issue differently, following comment #7.

(In reply to comment #23) > > I think only Apple port uses the ICU headers checked into the WebKit > > source, all other ports use the installed system headers. Similar to > > other 3rd party dev packages. > > This sounds like the root of the problem then. Using a different copy of headers does nothing good, and just causes build issues. If there is something new in system copy, you generally still cannot use it in WebKit code without breaking other platforms anyway. No, the root of the problem isn't the checked in ICU headers (used by only Apple, because we can't expect from developers to install ICU-dev packages). The root of the problem is that clashes between wtf/unicode/UTF8.h (which is absolutely independent of ICU, it is a WebKit header) and ICU's unicode/utf8.h header (which has same name in WebKit/Source/WTF/icu and other ICU's) Otherwise other ports shouldn't/can't use checked in ICU headers, so renaming checked in ICU headers won't work. See WebKit/Source/WTF/icu/README: "The headers in this directory are for compiling on Mac OS X 10.4. The Mac OS X 10.4 release includes the ICU binary, but not ICU headers. For other platforms, installed ICU headers should be used rather than these. They are specific to Mac OS X 10.4." > > WTFUTF8.h > I hate this name. It wasn't so constructive. Could you suggest a better name, please? ( I prefer renaming a WebKit header instead of suggesting renaming a 3rd header ... )

Comment on attachment 146876 [details] Patch r=me, we really need this fix. side note: Unfortunately this is a Qt only fix, and it can occur again in the future if somebody adds back WTF/wtf to the searchpath for some reason. Otherwise other ports have same problem can follow this way.

Committed r120076: <http://trac.webkit.org/changeset/120076>