Adding relList attr to link and anchor element

Adding relList attr to link and anchor element

relList attribute has been added to HTMLLinkElement and HTMLAnchorElement in the w3c spec.
Firefox already supports this, we should also add the same.

link-element:
http://www.w3.org/html/wg/drafts/html/master/document-metadata.html#htmllinkelement
anchor-element:
http://www.w3.org/html/wg/drafts/html/master/document-metadata.html#htmlanchorelement

BUG=394770

[prashanth, 2014-07-17 17:57:49]

PTAL, I have added only for link element, once my changes are satisfactory, I'll
add for anchor element and also add Layout Tests for the same.

[Inactive, 2014-07-17 19:37:57]

https://codereview.chromium.org/400763002/diff/1/Source/core/html/ClassList.h
File Source/core/html/ClassList.h (right):

https://codereview.chromium.org/400763002/diff/1/Source/core/html/ClassList.h...
Source/core/html/ClassList.h:62: virtual void setValue(const AtomicString&
value) OVERRIDE { m_element->setAttribute(HTMLNames::classAttr, value); }
As per the spec, "The IDL attribute relList must reflect the rel content
attribute", not the class attribute. You probably shouldn't reuse ClassList.
You'll likely need your own RelList instead.

https://codereview.chromium.org/400763002/diff/1/Source/core/html/HTMLLinkEle...
File Source/core/html/HTMLLinkElement.cpp (right):

https://codereview.chromium.org/400763002/diff/1/Source/core/html/HTMLLinkEle...
Source/core/html/HTMLLinkElement.cpp:426: DOMTokenList*
HTMLLinkElement::relList() const
Could return a DOMTokenList&

https://codereview.chromium.org/400763002/diff/1/Source/core/html/HTMLLinkEle...
Source/core/html/HTMLLinkElement.cpp:428: return m_relList.get();
You could lazy initialize m_relList here instead of doing it unconditionally in
the constructor.

[prashanth, 2014-07-23 09:26:29]

Modified as per review comment, let me know if there are any changes needed.

[Inactive, 2014-07-23 13:26:44]

Obviously this needs layout tests.

https://codereview.chromium.org/400763002/diff/20001/Source/core/html/HTMLLin...
File Source/core/html/HTMLLinkElement.cpp (right):

https://codereview.chromium.org/400763002/diff/20001/Source/core/html/HTMLLin...
Source/core/html/HTMLLinkElement.cpp:427: setRelList(RelList::create(this));
 m_relList = RelList::create(this);

https://codereview.chromium.org/400763002/diff/20001/Source/core/html/HTMLLin...
Source/core/html/HTMLLinkElement.cpp:428: return *m_relList.get();
.get() is unnecessary

https://codereview.chromium.org/400763002/diff/20001/Source/core/html/HTMLLin...
Source/core/html/HTMLLinkElement.cpp:477: if (name == relAttr && m_relList)
I would do this in HTMLLinkElement::parseAttribute(). There is already a check
for relAttr there.

https://codereview.chromium.org/400763002/diff/20001/Source/core/html/HTMLLin...
File Source/core/html/HTMLLinkElement.h (right):

https://codereview.chromium.org/400763002/diff/20001/Source/core/html/HTMLLin...
Source/core/html/HTMLLinkElement.h:156: void
setRelList(PassOwnPtrWillBeRawPtr<RelList> relList) { m_relList = relList; }
We don't really need this getter. Especially not a public one.

https://codereview.chromium.org/400763002/diff/20001/Source/core/html/RelList...
File Source/core/html/RelList.cpp (right):

https://codereview.chromium.org/400763002/diff/20001/Source/core/html/RelList...
Source/core/html/RelList.cpp:2: * Copyright (C) 2014 Prashant Hiremath
(hiremathprashants@gmail.com).
Ditto about copyright.

https://codereview.chromium.org/400763002/diff/20001/Source/core/html/RelList...
Source/core/html/RelList.cpp:53: return
m_element->hasAttribute(HTMLNames::relAttr) ? relListAttr().size() : 0;
HTMLNames:: are unnecessary since you are "using" this namespace at the top.

https://codereview.chromium.org/400763002/diff/20001/Source/core/html/RelList...
Source/core/html/RelList.cpp:70: if (!m_relAttrNames) {
curly brackets are unnecessary here.

https://codereview.chromium.org/400763002/diff/20001/Source/core/html/RelList...
Source/core/html/RelList.cpp:73: return *m_relAttrNames.get();
.get() is not needed.

https://codereview.chromium.org/400763002/diff/20001/Source/core/html/RelList...
Source/core/html/RelList.cpp:78: if (m_relAttrNames) {
We usually do the opposite: return early.

if (!m_relAttrNames)
  return;
...

https://codereview.chromium.org/400763002/diff/20001/Source/core/html/RelList...
Source/core/html/RelList.cpp:80: relAttrList.set(relAttrvalue, false);
simply: m_relAttrNames->set(relAttrValue, false);

https://codereview.chromium.org/400763002/diff/20001/Source/core/html/RelList.h
File Source/core/html/RelList.h (right):

https://codereview.chromium.org/400763002/diff/20001/Source/core/html/RelList...
Source/core/html/RelList.h:2: * Copyright (C) 2014 Prashant Hiremath
(hiremathprashants@gmail.com).
You should keep the Google copyright line considering how much code is shared.

https://codereview.chromium.org/400763002/diff/20001/Source/core/html/RelList...
Source/core/html/RelList.h:54: 
Exta line is not needed here.

https://codereview.chromium.org/400763002/diff/20001/Source/core/html/RelList...
Source/core/html/RelList.h:70: virtual const AtomicString& value() const
OVERRIDE { return m_element->getAttribute(HTMLNames::relAttr); }
You can use fastGetAttribute() here.

[prashanth, 2014-07-24 11:45:19]

Added relList attribute to anchor element and added LayoutTests for link and
anchor element. PTAL.

[prashanth, 2014-07-24 11:46:36]

Hi Chris, Why Trybot is not running is it because of [WorkInProgress] tag?

[Inactive, 2014-07-24 15:23:36]

On 2014/07/24 11:46:36, prashanth wrote:
> Hi Chris, Why Trybot is not running is it because of [WorkInProgress] tag?

You need try bots permissions to be able to trigger the try bots. You probably
don't have the rights. Sadly, I don't have the rights to trigger them for other
people (only myself). A Googler could do this for you though.

[Inactive, 2014-07-24 15:36:41]

Once you have made those remaining changes, tkent@ would likely be a good
reviewer for this.

https://codereview.chromium.org/400763002/diff/40001/LayoutTests/fast/dom/rel...
File LayoutTests/fast/dom/relList_on_anchor.html (right):

https://codereview.chromium.org/400763002/diff/40001/LayoutTests/fast/dom/rel...
LayoutTests/fast/dom/relList_on_anchor.html:2: <html>
We don't really need <html> / <head> or <body> here. You can omit them.

https://codereview.chromium.org/400763002/diff/40001/LayoutTests/fast/dom/rel...
LayoutTests/fast/dom/relList_on_anchor.html:11: var anchorrellist =
document.querySelectorAll('a[rel]');
Just add an id to your element and do a getElementById(), this is more efficient
and simpler.

https://codereview.chromium.org/400763002/diff/40001/LayoutTests/fast/dom/rel...
LayoutTests/fast/dom/relList_on_anchor.html:12: var anchor = anchorrellist[0];
Could you add the following check?

shouldBe("anchor.relList.__proto__", "DOMTokenList.prototype");

https://codereview.chromium.org/400763002/diff/40001/LayoutTests/fast/dom/rel...
LayoutTests/fast/dom/relList_on_anchor.html:13:
shouldBe("anchor.relList.contains('shortcut')", "true");
Use shouldBeTrue() and shouldBeFalse() as needed.

https://codereview.chromium.org/400763002/diff/40001/LayoutTests/fast/dom/rel...
LayoutTests/fast/dom/relList_on_anchor.html:14:
shouldBe("anchor.relList.contains('icon')", "true");
It would be nice to check the indexed getters as well (i.e. anchor.relList[0])

https://codereview.chromium.org/400763002/diff/40001/LayoutTests/fast/dom/rel...
LayoutTests/fast/dom/relList_on_anchor.html:22: </script>
Could you also try to remove the rel attribute and check relList?

https://codereview.chromium.org/400763002/diff/40001/LayoutTests/fast/dom/rel...
File LayoutTests/fast/dom/relList_on_link.html (right):

https://codereview.chromium.org/400763002/diff/40001/LayoutTests/fast/dom/rel...
LayoutTests/fast/dom/relList_on_link.html:11: var linkrellist =
document.querySelectorAll('link[rel]');
Same comments as for the other layout test.

https://codereview.chromium.org/400763002/diff/40001/Source/core/html/RelList...
File Source/core/html/RelList.cpp (right):

https://codereview.chromium.org/400763002/diff/40001/Source/core/html/RelList...
Source/core/html/RelList.cpp:84: visitor->trace(m_element);
You probably need to trace m_relAttrNames as well

https://codereview.chromium.org/400763002/diff/40001/Source/core/html/RelList.h
File Source/core/html/RelList.h (right):

https://codereview.chromium.org/400763002/diff/40001/Source/core/html/RelList...
Source/core/html/RelList.h:72: mutable OwnPtr<SpaceSplitString> m_relAttrNames;
oilpan: OwnPtrWillBeMember

[prashanth, 2014-07-25 11:40:02]

PTAL

[Inactive, 2014-07-25 12:55:51]

https://codereview.chromium.org/400763002/diff/60001/LayoutTests/fast/dom/rel...
File LayoutTests/fast/dom/relList_on_anchor.html (right):

https://codereview.chromium.org/400763002/diff/60001/LayoutTests/fast/dom/rel...
LayoutTests/fast/dom/relList_on_anchor.html:1: <a id="anchorId" rel="shortcut
icon" href="../images/resources/1bit.ico"/>
Please add back the DOCTYPE.

https://codereview.chromium.org/400763002/diff/60001/LayoutTests/fast/dom/rel...
LayoutTests/fast/dom/relList_on_anchor.html:8:
shouldBeTrue("anchor.relList.contains('shortcut')");
The issue with contains() is that you don't verify that the value is at the
expected index. How about using:
shouldBeEqualToString("anchor.relList[0]", "shortcut");

https://codereview.chromium.org/400763002/diff/60001/LayoutTests/fast/dom/rel...
File LayoutTests/fast/dom/relList_on_link.html (right):

https://codereview.chromium.org/400763002/diff/60001/LayoutTests/fast/dom/rel...
LayoutTests/fast/dom/relList_on_link.html:1: <link id="linkId" rel="shortcut
icon" href="../images/resources/1bit.ico"/>
Please add back the DOCTYPE.

https://codereview.chromium.org/400763002/diff/60001/LayoutTests/fast/dom/rel...
LayoutTests/fast/dom/relList_on_link.html:8:
shouldBeTrue("link.relList.contains('shortcut')");
Same comment about using indexed getter.

https://codereview.chromium.org/400763002/diff/60001/Source/core/html/HTMLAnc...
File Source/core/html/HTMLAnchorElement.h (right):

https://codereview.chromium.org/400763002/diff/60001/Source/core/html/HTMLAnc...
Source/core/html/HTMLAnchorElement.h:31: #include "core/html/RelList.h"
Cannot this be forward-declared?

https://codereview.chromium.org/400763002/diff/60001/Source/core/html/HTMLLin...
File Source/core/html/HTMLLinkElement.h (right):

https://codereview.chromium.org/400763002/diff/60001/Source/core/html/HTMLLin...
Source/core/html/HTMLLinkElement.h:36: #include "core/html/RelList.h"
Cannot this be forward-declared?

https://codereview.chromium.org/400763002/diff/60001/Source/core/html/RelList...
File Source/core/html/RelList.cpp (right):

https://codereview.chromium.org/400763002/diff/60001/Source/core/html/RelList...
Source/core/html/RelList.cpp:63: bool RelList::containsInternal(const
AtomicString& token) const
When is this method used? I don't see any callers, do we really need it?

[prashanth, 2014-07-25 14:18:03]

https://codereview.chromium.org/400763002/diff/60001/Source/core/html/RelList...
File Source/core/html/RelList.cpp (right):

https://codereview.chromium.org/400763002/diff/60001/Source/core/html/RelList...
Source/core/html/RelList.cpp:63: bool RelList::containsInternal(const
AtomicString& token) const
On 2014/07/25 12:55:51, Chris Dumez wrote:
> When is this method used? I don't see any callers, do we really need it?

I'm also not sure who is caller for it, but it is pure virtual method
in base class(DOMTokenList). so need to implement it.

[Inactive, 2014-07-25 14:30:53]

https://codereview.chromium.org/400763002/diff/60001/Source/core/html/RelList...
File Source/core/html/RelList.cpp (right):

https://codereview.chromium.org/400763002/diff/60001/Source/core/html/RelList...
Source/core/html/RelList.cpp:63: bool RelList::containsInternal(const
AtomicString& token) const
On 2014/07/25 14:18:03, prashanth wrote:
> On 2014/07/25 12:55:51, Chris Dumez wrote:
> > When is this method used? I don't see any callers, do we really need it?
> 
> I'm also not sure who is caller for it, but it is pure virtual method
> in base class(DOMTokenList). so need to implement it.

Oh right, I missed that it was virtual. Ignore my comment then.

[tkent, 2014-07-28 00:21:16]

Please post "Intent to implement and ship" mail to blink-dev.

[prashanth, 2014-07-28 05:49:00]

On 2014/07/28 00:21:16, tkent wrote:
> Please post "Intent to implement and ship" mail to blink-dev.

Hi Tamura,
I had one small doubt (can you please comment on this), even though I'm ready to
post, adding this attribute in blink-dev. I was thinking this is not a big
feature (relList attr just reflects rel attribute), do you still consider this
has to go through Intent to implement process?

[tkent, 2014-07-28 06:15:42]

On 2014/07/28 05:49:00, prashanth wrote:
> On 2014/07/28 00:21:16, tkent wrote:
> > Please post "Intent to implement and ship" mail to blink-dev.
> 
> Hi Tamura,
> I had one small doubt (can you please comment on this), even though I'm ready
to
> post, adding this attribute in blink-dev. I was thinking this is not a big
> feature (relList attr just reflects rel attribute), do you still consider this
> has to go through Intent to implement process?

Yes.

This change has some code complexity by RelList class.  We should discuss if
it's worth to add RelList class just to support relList IDL attribute.

[sof, 2014-08-23 19:22:19]

https://codereview.chromium.org/400763002/diff/60001/Source/core/html/RelList.h
File Source/core/html/RelList.h (right):

https://codereview.chromium.org/400763002/diff/60001/Source/core/html/RelList...
Source/core/html/RelList.h:58: virtual void setValue(const AtomicString&
relAttrvalue) OVERRIDE;
The parameter's purpose ought to be clear, so no need to give it a name --
change to "(const AtomicString&)" ?

https://codereview.chromium.org/400763002/diff/60001/Source/core/html/RelList...
Source/core/html/RelList.h:72: mutable OwnPtrWillBeMember<SpaceSplitString>
m_relAttrNames;
SpaceSplitString is not on the Oilpan heap, so you can keep this as
OwnPtr<SpaceSplitString> (and remove its tracing from the trace() method.)

[Anton Obzhirov, 2016-09-02 10:02:15]

a.obzhirov@samsung.com changed reviewers:
+ a.obzhirov@samsung.com

[Anton Obzhirov, 2016-09-02 10:02:16]

hi guys,

Is this patch is still being worked on?
It affects also HTMLAreaElement and issue
https://bugs.chromium.org/p/chromium/issues/detail?id=605552.