WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
153231
HTMLElement::nodeName should not upper case non-ASCII characters
https://bugs.webkit.org/show_bug.cgi?id=153231
Summary
HTMLElement::nodeName should not upper case non-ASCII characters
Ryosuke Niwa
Reported
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?
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
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Darin Adler
Comment 1
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.
Ryosuke Niwa
Comment 2
2016-01-20 19:47:52 PST
Created
attachment 269418
[details]
Fixes the bug
Ryosuke Niwa
Comment 3
2016-01-20 19:49:21 PST
Created
attachment 269419
[details]
Removed whitespaces in blank lines
Build Bot
Comment 4
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
Build Bot
Comment 5
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
Build Bot
Comment 6
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
Build Bot
Comment 7
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
Build Bot
Comment 8
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
Build Bot
Comment 9
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
Ryosuke Niwa
Comment 10
2016-01-20 21:25:04 PST
Created
attachment 269429
[details]
Rebaselined the test
Ryosuke Niwa
Comment 11
2016-01-21 14:52:37 PST
ping reviewers?
Chris Dumez
Comment 12
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?
Ryosuke Niwa
Comment 13
2016-01-21 18:39:05 PST
Created
attachment 269536
[details]
Addressed Chris' comment
Alex Christensen
Comment 14
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.
Darin Adler
Comment 15
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?
Darin Adler
Comment 16
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.
Alex Christensen
Comment 17
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.
Ryosuke Niwa
Comment 18
2016-01-22 18:04:49 PST
Committed
r195501
: <
http://trac.webkit.org/changeset/195501
>
Csaba Osztrogonác
Comment 19
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.
Ryosuke Niwa
Comment 20
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug