[Webkit-unassigned] [Bug 75302] ShadowContentElement should be able to use query.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=75302


MORITA Hajime <morrita at google.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #121775|review?                     |review-
               Flag|                            |




--- Comment #19 from MORITA Hajime <morrita at google.com>  2012-01-09 19:45:28 PST ---
(From update of attachment 121775)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.