Bug 153231

Yes, we should add convertToASCIIUppercase. I have a whole set of casing related changes I want to make, and locally I did add that function.

Created attachment 269418 [details] Fixes the bug

Created attachment 269419 [details] Removed whitespaces in blank lines

Comment on attachment 269419 [details] Removed whitespaces in blank lines Attachment 269419 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/719895 New failing tests: fast/dom/Element/tagName-must-be-ASCII-uppercase-in-HTML-document.html

Created attachment 269425 [details] Archive of layout-test-results from ews100 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 269419 [details] Removed whitespaces in blank lines Attachment 269419 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/719887 New failing tests: fast/dom/Element/tagName-must-be-ASCII-uppercase-in-HTML-document.html

Created attachment 269426 [details] Archive of layout-test-results from ews104 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Comment on attachment 269419 [details] Removed whitespaces in blank lines Attachment 269419 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/719897 New failing tests: fast/dom/Element/tagName-must-be-ASCII-uppercase-in-HTML-document.html

Created attachment 269427 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5

Created attachment 269429 [details] Rebaselined the test

ping reviewers?

Comment on attachment 269429 [details] Rebaselined the test View in context: https://bugs.webkit.org/attachment.cgi?id=269429&action=review > Source/WTF/wtf/text/AtomicString.cpp:51 > +template<typename IsDesiredCaseType, typename ToDesiredCaseType, typename ConverterType> I think it would result in simpler / shorter call site code we we just had an "enum class Case { Lower, Upper };" template parameter. Then we would use ternaries inside this function to chose which methods / functions to call. > Source/WTF/wtf/text/AtomicString.cpp:67 > + if (UNLIKELY(!isDesiredCase(characters[i]))) { I think the original intent of the UNLIKELY() may apply to isASCIIUpper() only, not isASCIILower(). I think the rationale was probably that most characters are usually lowercase?

Created attachment 269536 [details] Addressed Chris' comment

I'm pretty sure the Windows build failure isn't a problem. When you touch a header in WTF without touching CMakeLists.txt (which isn't necessary) it will just require a clean build.

(In reply to comment #14) > I'm pretty sure the Windows build failure isn't a problem. When you touch a > header in WTF without touching CMakeLists.txt (which isn't necessary) it > will just require a clean build. Do we just have to live with this kind of problem forever, or is it something we can fix about the CMake build system?

Comment on attachment 269536 [details] Addressed Chris' comment View in context: https://bugs.webkit.org/attachment.cgi?id=269536&action=review > Source/WTF/wtf/text/AtomicString.cpp:52 > +template<AtomicString::CaseConvertType type> > +ALWAYS_INLINE AtomicString AtomicString::convertASCIICase() const Small comment: I think in future we should probably change our coding style so template goes on the same line rather than having a line break after it. I suppose we can debate that point elsewhere. > Source/WTF/wtf/text/AtomicString.cpp:82 > + RefPtr<StringImpl> convertedString = type == CaseConvertType::Lower ? impl->convertToASCIILowercase() : impl->convertToASCIIUppercase(); This should be Ref, not RefPtr. Which is why I think we should use auto instead of stating the type. > Source/WTF/wtf/text/StringImpl.h:857 > + template<CaseConvertType type, typename CharType> > + static Ref<StringImpl> convertASCIICase(StringImpl&, const CharType*, unsigned); Would read better on a single line. I suggest CharacterType instead of CharType.

(In reply to comment #15) > Do we just have to live with this kind of problem forever, or is it > something we can fix about the CMake build system? It's windows-specific because of the way I copy headers, and it can be fixed.

Committed r195501: <http://trac.webkit.org/changeset/195501>

(In reply to comment #18) > Committed r195501: <http://trac.webkit.org/changeset/195501> It broke the WinCairo and Apple Windows builds. See build.webkit.org for details.

(In reply to comment #19) > (In reply to comment #18) > > Committed r195501: <http://trac.webkit.org/changeset/195501> > > It broke the WinCairo and Apple Windows builds. > See build.webkit.org for details. Triggered clean builds as needed.