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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Oct 20 09:32:53 PDT 2014


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

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #240100|review?                     |review+
              Flags|                            |

--- Comment #8 from Darin Adler <darin at apple.com> ---
Comment on attachment 240100
  --> https://bugs.webkit.org/attachment.cgi?id=240100
Patch

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

Patch is OK, but I would like to see some refined versions if possible.

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

There’s a small unnecessary inefficiency here. This function takes a const String&, but the setRel function takes a const AtomicString&. That means that this call needs to construct an AtomicString from a String. This means we will be compiling in a check to see if a string is already in the atomic string table. All of that can be avoided two different ways:

a) Change the argument type for this function to const AtomicString&.
b) Move the code from here to the single call site, and get rid of this function entirely.

I think the name of this function is misleading, because it does not *set* the rel attribute. It simply responds to a change in the rel attribute.

So to summarize:

- This function’s name is not clear.
- This function’s implementation is slightly inefficient in an unnecessary way because of its argument type.
- This function is short and called in only one place.
- This function was public, but should be a completely private part of this class.

I suggest getting rid of it to fix all of these.

> Source/WebCore/html/HTMLAnchorElement.h:112
> +    void setRel(const String&);

This should be private, not protected.

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

What is this include needed for? I don’t see anything below that would require it.

> Source/WebCore/html/RelList.cpp:37
> +    setRel(value());

It’s good code sharing to not repeat what setRel() and value() do, but it’s a little strange to call two virtual functions to do this, even though the “final” optimization means that they will be as efficient as normal function calls. I’d be tempted to put the code in line here.

> Source/WebCore/html/RelList.h:38
> +    void setRel(const AtomicString&);

I probably would have named this updateRelAttribute or relAttributeChanged, or maybe even setValue.

> Source/WebCore/html/RelList.h:51
> +    mutable SpaceSplitString m_rels;

I probably would have named this m_relAttributeValue or m_value or even m_list.

-- 
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/20141020/d89e901d/attachment-0002.html>


More information about the webkit-unassigned mailing list