[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

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #240082|review?                     |review-
              Flags|                            |

--- Comment #5 from Darin Adler <darin 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

Looks pretty good.

I read through the patch and all of Chris’s comments. I agree with all of his comments and I have a few of my own. Please make some fixes and put this up for review again!

> Source/WebCore/html/HTMLAnchorElement.cpp:314
>  {

This function really needs to be made private. It would be really unsafe if anyone called it from outside the class. It should *only* be called in response to a change in the rel attribute.

In fact, I think it might be cleaner to just put the four lines of code inside the parseAttribute function instead of factoring it out into a separate function.

>> Source/WebCore/html/HTMLAnchorElement.h:30
>> +#include "RelList.h"
> 
> I am not sure, just checking, but could this be forward declared by any chance?

We should be able to forward-declare instead of including. It might require some other small tweaks, but I think we should do that.

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

We should just put this one line (actually two, with a null check) into the HTMLLinkElement::parseAttribute function. We shouldn’t add this code.

> Source/WebCore/html/HTMLLinkElement.h:36
> +#include "RelList.h"

Should be able to forward declare RelList.

> Source/WebCore/html/HTMLLinkElement.h:108
>  private:

I don’t understand why we have a second "private" here. This should be removed.

>> Source/WebCore/html/RelList.cpp:63
>> +    if (!hasRel()) {
> 
> Can we just check if value.isNull() is true?

I’ll go further and say that we might not need this optimization at all. And if we do need it, we might want to put it into the SpaceSplitString::set function rather than only have it here in RelList.

> Source/WebCore/html/RelList.cpp:72
> +    return hasRel() && rels().contains(token);

We don’t need this hasRel() check.

> Source/WebCore/html/RelList.h:48
> +    virtual void ref() override;
> +    virtual void deref() override;
> +
> +    virtual unsigned length() const override;
> +    virtual const AtomicString item(unsigned index) const override;
> +
> +    virtual Element* element() const override;

These should all be private.

> LayoutTests/ChangeLog:17
> +        (gc):

We should remove lines like this if we aren’t using them to comment on functions in JavaScript files. They just make the change log harder to read.

-- 
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/2cb2058c/attachment-0002.html>