[webkit-reviews] review denied: [Bug 75302] ShadowContentElement should be able to use query. : [Attachment 121775] Patch

[bugzilla-daemon at webkit.org]

MORITA Hajime <morrita at google.com> has denied Shinya Kawanaka
<shinyak at chromium.org>'s request for review:
Bug 75302: ShadowContentElement should be able to use query.
https://bugs.webkit.org/show_bug.cgi?id=75302

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

------- Additional Comments from MORITA Hajime <morrita at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=121775&action=review


Hi, thanks for attacking this. This is what I hope to have!
Let us iterate for a few round.

> Source/WebCore/ChangeLog:41
> +

Please add an entry to dom/DOMAllInOne.cpp to make the Windows port happy.

> Source/WebCore/dom/ShadowContentElement.h:46
> +    static PassRefPtr<ShadowContentElement> create(Document*, const String&
select = String());

Please use AtomicString instead of String. It is what Attribute class uses.

> Source/WebCore/dom/ShadowContentElement.h:52
> +    const String& select() { return m_select; }

This should be implemented using getAttribute() because it should be also
represented as an HTML attribute.

> Source/WebCore/dom/ShadowContentElement.h:59
> +    ShadowContentElement(const QualifiedName&, Document*, const String&
select = String());

Ditto for AtomicString.

> Source/WebCore/dom/ShadowContentElement.h:67
> +    String m_select;

So we shouldn't need this.

> Source/WebCore/dom/ShadowContentSelectorQuery.cpp:37
> +    : m_matchesAll(element->select().isEmpty())

It looks we don' t need this because it's derivable from m_contentEleent.

> Source/WebCore/dom/ShadowContentSelectorQuery.cpp:78
> +    if (!targetNode->isElementNode())

This check can be done in matches(Node*).

> Source/WebCore/dom/ShadowContentSelectorQuery.h:37
> +#include <wtf/HashSet.h>

Do we need this...

> Source/WebCore/dom/ShadowContentSelectorQuery.h:38
> +#include <wtf/RefCounted.h>

and this?

> Source/WebCore/dom/ShadowContentSelectorQuery.h:50
> +    ShadowContentSelectorQuery(ShadowContentElement*);

Please make this explicit.

> Source/WebCore/dom/ShadowContentSelectorQuery.h:55
> +    struct SelectorData {

Let's extract SelectorData from SelectorQuery and share the code. 
I hope there are some reasonable way to extract stuff from SelectorQuery
without sacrificing performance...

> Source/WebCore/dom/ShadowContentSelectorQuery.h:66
> +    bool canUseIdLookup(const SelectorData&, Document*) const;

It looks we aren't using this.

> Source/WebCore/html/HTMLDetailsElement.cpp:45
> +	   : ShadowContentElement(HTMLNames::divTag, document, "")

Please use WTF::emptyAtom().

> Source/WebCore/html/HTMLDetailsElement.cpp:61
> +	   : ShadowContentElement(HTMLNames::divTag, document,
"summary:first-of-type")

Please use pre-defined static AtomicString instance instead of C string literal
to prevent an extra heap allocation.

> Source/WebCore/testing/Internals.cpp:137
> +    return elem.release();

Why do we need these two step?

> LayoutTests/ChangeLog:13
> +2012-01-06  Shinya Kawanaka	<shinyak at google.com>

Dup.