[webkit-reviews] review denied: [Bug 71152] selectors should match attribute name with case sensitivity based on element & document type : [Attachment 207753] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Rob Buis <rwlbuis at gmail.com>'s request
for review:
Bug 71152: selectors should match attribute name with case sensitivity based on
element & document type
https://bugs.webkit.org/show_bug.cgi?id=71152

Attachment 207753: Patch
https://bugs.webkit.org/attachment.cgi?id=207753&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=207753&action=review


I was close to a review+ on this, but I think the really tricky changes to
QualifiedName and Attribute::matches need at least some minor tweaks before
this is OK to land.

> Source/WebCore/dom/Attribute.h:54
> +    bool matches(const QualifiedName&, const AtomicString&) const;

This second argument needs a argument name. It’s not at all clear what it is.
And I think this function is now not self-explanatory any more, so we probably
need a comment, now, too.

> Source/WebCore/dom/Attribute.h:75
> +inline bool Attribute::matches(const QualifiedName& qualifiedName, const
AtomicString& lname) const

Please don’t use an abbreviation, “lname”. You could use localName and then
write localName != this->localName(). Or some other non-abbreviation.

> Source/WebCore/dom/QualifiedName.cpp:156
>  const AtomicString& QualifiedName::localNameUpper() const
>  {
> -    if (!m_impl->m_localNameUpper)
> -	   m_impl->m_localNameUpper = m_impl->m_localName.upper();
> -    return m_impl->m_localNameUpper;
> +    if (!m_impl->m_localNameUpperLower)
> +	   m_impl->m_localNameUpperLower = m_impl->m_localName.upper();
> +    return m_impl->m_localNameUpperLower;
> +}
> +
> +const AtomicString& QualifiedName::localNameLower() const
> +{
> +    if (!m_impl->m_localNameUpperLower)
> +	   m_impl->m_localNameUpperLower = m_impl->m_localName.lower();
> +    return m_impl->m_localNameUpperLower;
>  }

How does this work? It seems that if you call localNameUpper and then
localNameLower, you will get an uppercased copy. That seems like a very
dangerous design, even if there is some crazy reason it works!

If we really are going to rely on people calling only one or the other on each
QualifiedName, then I think we need a debug-only flag to catch if we ever do
that wrong at runtime at least.