Add relList to the anchor, area and link elements

• Component: DOM
• Status: RESOLVED FIXED

[Dhi Aurrahman]

Add relList to the anchor, area and link elements

[Dhi Aurrahman]

Created attachment 240081 [details]
Patch

[Dhi Aurrahman]

Created attachment 240082 [details]
Patch

[Chris Dumez]

Comment on attachment 240082 [details]
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.

[Chris Dumez]

Comment on attachment 240082 [details]
Patch

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

That's a lot of testing, Benjamin will love it :)

> LayoutTests/fast/dom/HTMLAnchorElement/rel-list-gc.html:7
> +<script src="script-tests/rel-list-gc.js"></script>

Please embed the JS in this file instead of adding a new js file whenever possible. I know a lot of layout tests are doing this but this is the *old* style.

> LayoutTests/fast/dom/HTMLAnchorElement/rel-list.html:7
> +<script src="script-tests/rel-list.js"></script>

Please embed the JS in this file.

> LayoutTests/fast/dom/HTMLAnchorElement/script-tests/rel-list-gc.js:3
> +function gc()

I am pretty sure js-test-pre.js already has a gc() function?

> LayoutTests/fast/dom/HTMLAnchorElement/script-tests/rel-list.js:136
> +function shouldThrowDOMException(f, ec)

We already have shouldThrow() in js-test-pre.js for all this, please use the existing test infrastructure whenever possible.

> LayoutTests/fast/dom/HTMLAreaElement/rel-list-gc.html:7
> +<script src="script-tests/rel-list-gc.js"></script>

embed js here

> LayoutTests/fast/dom/HTMLAreaElement/rel-list.html:7
> +<script src="script-tests/rel-list.js"></script>

embed js here

> LayoutTests/fast/dom/HTMLAreaElement/script-tests/rel-list-gc.js:3
> +function gc()

Reuse function in js-test-pre.js

[Darin Adler]

Comment on attachment 240082 [details]
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.

[Chris Dumez]

Comment on attachment 240082 [details]
Patch

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

>>> 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.

I think this would require updating SpaceSplitString, because there is currently the following assertion in SpaceSplitStringData::create(const AtomicString& keyString):
ASSERT(!keyString.isNull());

which is called from SpaceSplitString::set().

[Dhi Aurrahman]

Created attachment 240100 [details]
Patch

[Darin Adler]

Comment on attachment 240100 [details]
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.

[Chris Dumez]

Comment on attachment 240100 [details]
Patch

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

>> 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.

a) seems like a good idea because this function is only called from HTMLAnchorElement::parseAttribute() and always with an AtomicString.

[Benjamin Poulain]

Comment on attachment 240100 [details]
Patch

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

r+ from me too.

Dhi, since you are not yet a committer, you will need to update the patch here on bugzilla. You don't need to have another review since you already had r+, you can pass -no-review to webkit-patch when updating. I'll land your patch manually.

> LayoutTests/ChangeLog:15
> +        * fast/dom/rel-list-expected.txt: Added.
> +        * fast/dom/rel-list-gc-expected.txt: Added.
> +        * fast/dom/rel-list-gc.html: Added.
> +        * fast/dom/rel-list.html: Added.
> +        * perf/rel-list-remove-expected.txt: Added.
> +        * perf/rel-list-remove.html: Added.

Fantastic tests!

> LayoutTests/fast/dom/rel-list-gc.html:10
> +function test(){

"type" could be an argument of test().

[Dhi Aurrahman]

Created attachment 240152 [details]
Patch

[Dhi Aurrahman]

Created attachment 240155 [details]
Patch

[Dhi Aurrahman]

(In reply to comment #10)
> Comment on attachment 240100 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240100&action=review
> 
> r+ from me too.
> 

Thank you! Thanks Christhope, Darin and Benjamin!

> Dhi, since you are not yet a committer, you will need to update the patch
> here on bugzilla. You don't need to have another review since you already
> had r+, you can pass -no-review to webkit-patch when updating. I'll land
> your patch manually.
> 

OK

> > LayoutTests/ChangeLog:15
> > +        * fast/dom/rel-list-expected.txt: Added.
> > +        * fast/dom/rel-list-gc-expected.txt: Added.
> > +        * fast/dom/rel-list-gc.html: Added.
> > +        * fast/dom/rel-list.html: Added.
> > +        * perf/rel-list-remove-expected.txt: Added.
> > +        * perf/rel-list-remove.html: Added.
> 
> Fantastic tests!
> 
> > LayoutTests/fast/dom/rel-list-gc.html:10
> > +function test(){
> 
> "type" could be an argument of test().

I have updated the patch. Thanks

[Darin Adler]

Comment on attachment 240155 [details]
Patch

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

Patch looks fine, but for some reason tests are failing on the bots. So I am not setting commit-queue+ on this until we find out what’s wrong. I guess Benjamin said he would land this by hand.

> Source/WebCore/html/HTMLAnchorElement.cpp:267
> +        if (SpaceSplitString::spaceSplitStringContainsValue(value, "noreferrer", true))
> +            m_linkRelations |= RelationNoReferrer;

Now that you have moved this here, I spotted a mistake in it. Something about having it right here in parseAttribute makes it clear to me: This sets RelationNoReferrer, but never clears it if the rel attribute is changed back to a value that does not include "noreferrer". We should return to this code later and fix this bug, even though it’s not new.

> Source/WebCore/html/RelList.cpp:36
> +    m_relAttributeValue.set(element.fastGetAttribute(HTMLNames::relAttr), false);

Maybe SpaceSplitString has a constructor we could use instead of calling set.

[Dhi Aurrahman]

Created attachment 240196 [details]
Patch

[Benjamin Poulain]

Created attachment 240257 [details]
Patch for landing

[Benjamin Poulain]

Comment on attachment 240257 [details]
Patch for landing

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

> Source/WebCore/html/RelList.cpp:35
> +    , m_relAttributeValue(SpaceSplitString(element.fastGetAttribute(HTMLNames::relAttr), false))

I moved this from the constructor's body to the initialisation list for the landing patch.

[WebKit Commit Bot]

Comment on attachment 240257 [details]
Patch for landing

Clearing flags on attachment: 240257

Committed r175028: <http://trac.webkit.org/changeset/175028>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.