Bug 193101

Created attachment 358249 [details] Patch

We had a test failure in the StringConcatenate_Unsigned fixture on WPE as described in https://bugs.webkit.org/show_bug.cgi?id=180720. Seems the best fix for this was to create a new syntax for raw characters rather than trying testing platform specific behaviour as we were before.

Created attachment 358250 [details] Patch

Attachment 358250 [details] did not pass style-queue: ERROR: Source/WTF/wtf/text/LiteralCharacter.h:62: Do not use 'using namespace WTF::CharacterLiterals;'. [build/using_namespace] [4] ERROR: Source/WTF/wtf/text/LiteralCharacter.cpp:27: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 2 in 64 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to Build Bot from comment #4) > Attachment 358250 [details] did not pass style-queue: > > > ERROR: Source/WTF/wtf/text/LiteralCharacter.h:62: Do not use 'using > namespace WTF::CharacterLiterals;'. [build/using_namespace] [4] Why not? ASCIILiteral.h uses exactly the same technique. > ERROR: Source/WTF/wtf/text/LiteralCharacter.cpp:27: Found other header > before a header this file implements. Should be: config.h, primary header, > blank line, and then alphabetically sorted. [build/include_order] [4] #include "config.h" #include <wtf/text/LiteralCharacter.h> Don't see what's wrong here.

Bummer, you can't have LiteralCharacter<UChar> operator"" _ch(UChar ch); When UChar == unsigned short according to https://en.cppreference.com/w/cpp/language/user_literal. Worked for me since it's char16_t here, which apparently is allowed. Hmm, for UChar then, we don't get to use a user literal.

Created attachment 358255 [details] Patch

Attachment 358255 [details] did not pass style-queue: ERROR: Source/WTF/wtf/text/LiteralCharacter.h:61: Do not use 'using namespace WTF::CharacterLiterals;'. [build/using_namespace] [4] ERROR: Source/WebKit/UIProcess/glib/RemoteInspectorClient.cpp:156: Missing spaces around : [whitespace/init] [4] ERROR: Source/WTF/wtf/text/LiteralCharacter.cpp:27: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 3 in 70 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 358264 [details] Patch

Attachment 358264 [details] did not pass style-queue: ERROR: Source/WTF/wtf/text/LiteralCharacter.h:61: Do not use 'using namespace WTF::CharacterLiterals;'. [build/using_namespace] [4] ERROR: Source/WebKit/UIProcess/glib/RemoteInspectorClient.cpp:156: Missing spaces around : [whitespace/init] [4] ERROR: Source/WTF/wtf/text/LiteralCharacter.cpp:27: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 3 in 70 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 358269 [details] Patch

Attachment 358269 [details] did not pass style-queue: ERROR: Source/WTF/wtf/text/LiteralCharacter.h:61: Do not use 'using namespace WTF::CharacterLiterals;'. [build/using_namespace] [4] ERROR: Source/WebKit/UIProcess/glib/RemoteInspectorClient.cpp:156: Missing spaces around : [whitespace/init] [4] ERROR: Source/WTF/wtf/text/LiteralCharacter.cpp:27: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 3 in 70 files If any of these errors are false positives, please file a bug against check-webkit-style.

So these remaining build failures are to do with not updating the relevant build scripts for mac and windows AFAICT. I'll wait for a review on the patch before making changes to the build scripts for those platforms.

(In reply to Charlie Turner from comment #2) > We had a test failure in the StringConcatenate_Unsigned fixture on WPE as > described in https://bugs.webkit.org/show_bug.cgi?id=180720. Seems the best > fix for this was to create a new syntax for raw characters rather than > trying testing platform specific behaviour as we were before. I think the failure came from ICU using char16_t So the current test could be fixed using : #if PLATFORM(WIN) || U_ICU_VERSION_MAJOR_NUM >= 59 About your patch, is there any reason why LiteralCharacter<char> operator"" _ch(char ch) isn't inline ? Everything else (LiteralCharacter, StringTypeAdapter, makeString) seems to be defined in the headers.

(In reply to Loïc Yhuel from comment #14) > (In reply to Charlie Turner from comment #2) > > We had a test failure in the StringConcatenate_Unsigned fixture on WPE as > > described in https://bugs.webkit.org/show_bug.cgi?id=180720. Seems the best > > fix for this was to create a new syntax for raw characters rather than > > trying testing platform specific behaviour as we were before. > I think the failure came from ICU using char16_t > So the current test could be fixed using : > #if PLATFORM(WIN) || U_ICU_VERSION_MAJOR_NUM >= 59 Yep, we can condition the test to pass, but I wanted to see what it would take to fix it properly. I received what I thought was a bug comment, but must have been private mail, explaining the root reason is an out of data set of ICU headers in WebKit. When I get time I'll look at upgrading them to version 59 headers, which should help our issue here. For now, in the name of greenness, I'll go ahead an do a condition fix while I find time to fix it properly. > > > About your patch, is there any reason why LiteralCharacter<char> operator"" > _ch(char ch) isn't inline ? > Everything else (LiteralCharacter, StringTypeAdapter, makeString) seems to > be defined in the headers. It was to satisfy the ODR, but not reason why it can't be marked inline. I'll address that when I get round to the ICU headers upgrade.

If we trust Source/WTF/icu/README, the included headers should only be used on Mac. At least WPE and GTK use system headers, which can be older than 59 (no minimum version enforced, not sure there would be a reason to, Ubuntu 16.04 has ICU 55.1 for example).

(In reply to Loïc Yhuel from comment #16) > If we trust Source/WTF/icu/README, the included headers should only be used > on Mac. > > At least WPE and GTK use system headers, which can be older than 59 (no > minimum version enforced, not sure there would be a reason to, Ubuntu 16.04 > has ICU 55.1 for example). I've moved to temporarily guarding this away as suggested in https://bugs.webkit.org/show_bug.cgi?id=193319 Not sure what he conclusion is about whether the LiteralCharacter syntax is a worthwhile change for the future once the non-inlined UDO is fixed.

Workaround for now is proposed in https://bugs.webkit.org/show_bug.cgi?id=193332, but we need some kind of syntax to avoid the ambiguity described in that bug going forward.

Mmmm... I see the problem, but... nobody is going to remember to add the _ch suffix, and it won't be needed or useful unless you have older ICU, right? So only developers working with old ICU will notice. (That excludes anybody using our jhbuild moduleset.) Almost seems better to drop support for older ICU as soon as we're able. Per the GTK/WPE dependencies policy, we can drop support for Ubuntu 16.04 and require modern ICU in April. Not sure when Apple will be able to do so. If it will be a while, then we've got a tough question here to figure out how to resolve this thorny issue. :(

(In reply to Michael Catanzaro from comment #19) > Mmmm... I see the problem, but... nobody is going to remember to add the _ch > suffix You will get a compile errors in makeString if you don't use it. > , and it won't be needed or useful unless you have older ICU, right? Or if your platform (like Windows) defines it as wchar_t for instance. > So only developers working with old ICU will notice. (That excludes anybody > using our jhbuild moduleset.) > > Almost seems better to drop support for older ICU as soon as we're able. AFAIK, this is more a type-safety issue than an ICU upgrade, although the later exposed the problem. It's been mentioned elsewhere on bugzilla that we ought to have literal character syntax for this reason.

Created attachment 359008 [details] Patch Fix inlining issue

Attachment 359008 [details] did not pass style-queue: ERROR: Source/WTF/wtf/text/LiteralCharacter.h:64: Do not use 'using namespace WTF::CharacterLiterals;'. [build/using_namespace] [4] ERROR: Source/WebKit/UIProcess/glib/RemoteInspectorClient.cpp:156: Missing spaces around : [whitespace/init] [4] Total errors found: 2 in 68 files If any of these errors are false positives, please file a bug against check-webkit-style.

I guess I'm convinced... let's see what other reviewers think.

Comment on attachment 359008 [details] Patch Attachment 359008 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/10737036 New failing tests: http/tests/dom/set-document-location-host-to-accepted-values.html http/tests/xmlhttprequest/workers/referer.html fast/dom/DOMURL/parsing.html http/tests/security/cross-origin-cached-scripts.html http/tests/security/history-username-password.html http/tests/security/location-href-clears-username-password.html fast/dom/Element/hostname-host.html http/tests/security/cross-origin-cached-scripts-parallel.html http/tests/security/cross-origin-cached-images-with-memory-pressure.html http/tests/cache/xhr-vary-header.html http/tests/xmlhttprequest/origin-exact-matching.html http/tests/security/cross-origin-cached-images.html fast/dom/HTMLAnchorElement/anchor-element-href-parsing.html http/tests/security/cross-origin-cached-images-parallel.html http/tests/security/history-pushState-replaceState-from-sandboxed-iframe.html

Created attachment 359010 [details] Archive of layout-test-results from ews205 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews205 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

Scope of this work is much larger than expected and this patch is going in the wrong direction is seems.

Now that we’ve updated UChar to be char16_t and not uint16_t on all platforms, the problem we were trying to fix here should be gone for all platforms!