[Webkit-unassigned] [Bug 137860] Add relList to the anchor, area and link elements

[bugzilla-daemon at webkit.org]

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

--- Comment #3 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 240082
  --> https://bugs.webkit.org/attachment.cgi?id=240082
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=240082&action=review

I haven't looked at the tests yet. Just a few comments on the implementation for now.

> Source/WebCore/html/HTMLAnchorElement.cpp:317
> +    if (!m_relList) 

Is this really needed? I mean, if someone calls relList(), we are already doing lazy initialization of m_relList so why so we need to construct it here?

> Source/WebCore/html/HTMLAnchorElement.cpp:319
> +    m_relList.get()->setRel(value);

if (m_relList)
    m_relList->setRel(value);

> Source/WebCore/html/HTMLAnchorElement.cpp:326
> +    return *(m_relList.get());

return *m_relList;

> Source/WebCore/html/HTMLAnchorElement.h:30
> +#include "RelList.h"

I am not sure, just checking, but could this be forward declared by any chance?

> Source/WebCore/html/HTMLLinkElement.cpp:384
> +    return *(m_relList.get());

return *m_relList;

> Source/WebCore/html/HTMLLinkElement.cpp:433
> +        m_relList = std::make_unique<RelList>(*this);

Is this really needed? Doing lazy initialization and construct it only if needed would be nice.

> Source/WebCore/html/HTMLLinkElement.cpp:434
> +    m_relList.get()->setRel(value);

m_relList->setRel(value);

> Source/WebCore/html/RelList.cpp:46
> +    return hasRel() ? rels().size() : 0;

return rels().size() wouldn't work?

> Source/WebCore/html/RelList.cpp:52
> +        return AtomicString();

return nullAtom;

> Source/WebCore/html/RelList.cpp:63
> +    if (!hasRel()) {

Can we just check if value.isNull() is true?

> Source/WebCore/html/RelList.cpp:77
> +    return m_element.getAttribute(HTMLNames::relAttr);

I believe we can use fastGetAttribute() here as rel is not an SVG-Animated attribute (or the style attribute).

> Source/WebCore/html/RelList.cpp:85
> +bool RelList::hasRel() const

Why do we need this method at all? Why cannot we rely merely on m_rels?

> Source/WebCore/html/RelList.h:37
> +    RelList(Element& element)

I don't think we should inline this constructor.

> Source/WebCore/html/RelList.h:39
> +    {

Why don't we initialize m_rels here?

> Source/WebCore/html/RelList.h:57
> +    const SpaceSplitString& rels() const;

I would get rid of this getter since it is private and trivial. Just use m_rels at call sites.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20141019/1b8a6d84/attachment-0002.html>