Bug 153231 - HTMLElement::nodeName should not upper case non-ASCII characters
Summary: HTMLElement::nodeName should not upper case non-ASCII characters
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-01-19 03:09 PST by Ryosuke Niwa
Modified: 2016-01-23 00:10 PST (History)
14 users (show)

See Also:


Attachments
Fixes the bug (18.59 KB, patch)
2016-01-20 19:47 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Removed whitespaces in blank lines (18.20 KB, patch)
2016-01-20 19:49 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-yosemite (742.31 KB, application/zip)
2016-01-20 21:03 PST, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (787.10 KB, application/zip)
2016-01-20 21:03 PST, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-yosemite (980.18 KB, application/zip)
2016-01-20 21:08 PST, Build Bot
no flags Details
Rebaselined the test (19.00 KB, patch)
2016-01-20 21:25 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Addressed Chris' comment (18.82 KB, patch)
2016-01-21 18:39 PST, Ryosuke Niwa
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2016-01-19 03:09:30 PST
https://dom.spec.whatwg.org/#dom-element-tagname
1. Let qualifiedName be context object’s qualified name.
2. If the context object is in the HTML namespace and its node document is an HTML document, let qualified name be converted to ASCII uppercase.
3. Return qualified name.

So we shouldn't really be upper-casing the localName for real.  We need an equivalent of convertToASCIILowercase for uppercasing. convertToASCIIUppercase?
Comment 1 Darin Adler 2016-01-19 10:52:20 PST
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.
Comment 2 Ryosuke Niwa 2016-01-20 19:47:52 PST
Created attachment 269418 [details]
Fixes the bug
Comment 3 Ryosuke Niwa 2016-01-20 19:49:21 PST
Created attachment 269419 [details]
Removed whitespaces in blank lines
Comment 4 Build Bot 2016-01-20 21:03:00 PST
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
Comment 5 Build Bot 2016-01-20 21:03:05 PST
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 6 Build Bot 2016-01-20 21:03:22 PST
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
Comment 7 Build Bot 2016-01-20 21:03:27 PST
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 8 Build Bot 2016-01-20 21:08:49 PST
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
Comment 9 Build Bot 2016-01-20 21:08:53 PST
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
Comment 10 Ryosuke Niwa 2016-01-20 21:25:04 PST
Created attachment 269429 [details]
Rebaselined the test
Comment 11 Ryosuke Niwa 2016-01-21 14:52:37 PST
ping reviewers?
Comment 12 Chris Dumez 2016-01-21 16:24:16 PST
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?
Comment 13 Ryosuke Niwa 2016-01-21 18:39:05 PST
Created attachment 269536 [details]
Addressed Chris' comment
Comment 14 Alex Christensen 2016-01-21 21:56:06 PST
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.
Comment 15 Darin Adler 2016-01-22 08:51:42 PST
(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 16 Darin Adler 2016-01-22 08:55:36 PST
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.
Comment 17 Alex Christensen 2016-01-22 09:51:29 PST
(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.
Comment 18 Ryosuke Niwa 2016-01-22 18:04:49 PST
Committed r195501: <http://trac.webkit.org/changeset/195501>
Comment 19 Csaba Osztrogonác 2016-01-22 21:51:01 PST
(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.
Comment 20 Ryosuke Niwa 2016-01-23 00:10:28 PST
(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.