Use DOMSettableTokenList for {HTMLAnchorElement, HTMLAreaElement}.ping.

Use DOMSettableTokenList for {HTMLAnchorElement, HTMLAreaElement}.ping.

This CL is that replace DOMString with DOMSettableTokenList 
for {HTMLAnchorElement, HTMLAreaElement}.ping.
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/Z-RBeFLHRSU/0EY_qBmdtegJ

BUG=498219

[hyunjunekim2, 2015-07-13 13:39:36]

hyunjune.kim@samsung.com changed reviewers:
+ philipj@opera.com

[hyunjunekim2, 2015-07-13 13:39:54]

hyunjune.kim@samsung.com changed reviewers:
+ mkwst@chromium.org

[hyunjunekim2, 2015-07-13 13:40:46]

hyunjune.kim@samsung.com changed reviewers:
+ fs@opera.com, pdr@chromium.org, tkent@chromium.org

[Mike West, 2015-07-13 13:55:07]

Deferring this to Philip or tkent@. It's not clear to me that this is worth the
additional complexity that the implementation introduces, but if they think the
spec compliance is worth it, brilliant!

https://codereview.chromium.org/1235603003/diff/20001/Source/core/html/HTMLAn...
File Source/core/html/HTMLAnchorElement.h (right):

https://codereview.chromium.org/1235603003/diff/20001/Source/core/html/HTMLAn...
Source/core/html/HTMLAnchorElement.h:118: virtual void valueChanged() override;
Nit: Add something like "// DOMSettableTokenListObserver" so we know what that's
implementing.

[philipj_slow, 2015-07-13 14:31:31]

On 2015/07/13 13:55:07, Mike West (OOO through 16th) wrote:
> Deferring this to Philip or tkent@. It's not clear to me that this is worth
the
> additional complexity that the implementation introduces, but if they think
the
> spec compliance is worth it, brilliant!

It doesn't look like anyone else has turned HTMLAnchorElement.ping into a
DOMSettableTokenList yet, so this is indeed motivated by spec compliance and
nothing else.

I was assuming that DOMSettableTokenList already had pretty good infrastructure
so that this would be trivial, but having to inherit from
DOMSettableTokenListObserver and hooks things up manually for each class that
has a DOMSettableTokenList member seems very unfortunate, as does adding a
member for something that will almost never be accessed.

The only DOMTokenList we have exposed is Element.classList, and that's using a
special ClassList interface.

Perhaps we ought to discuss with other browser vendors if we really want to use
DOMTokenList in new contexts or not. tkent-san, WDYT?

[tkent, 2015-07-14 00:22:59]

On 2015/07/13 14:31:31, philipj wrote:
> Perhaps we ought to discuss with other browser vendors if we really want to
use
> DOMTokenList in new contexts or not. tkent-san, WDYT?

IMO, the implementation complexity is not so high, and the behavior difference
is not significant.  So, we may go ahead.
Upstreaming tests to W3C web-platform-tests would be nice for future
compatibility, but I'm not sure W3C web-platform-tests accepts tests for
WHATWG-only features.

[tkent, 2015-07-14 00:29:44]

https://codereview.chromium.org/1235603003/diff/40001/LayoutTests/fast/dom/pi...
File LayoutTests/fast/dom/ping-attribute-dom-binding.html (right):

https://codereview.chromium.org/1235603003/diff/40001/LayoutTests/fast/dom/pi...
LayoutTests/fast/dom/ping-attribute-dom-binding.html:30: 
Please add tests for DOMTokenList functions such as anchor.ping.add(), remove(),
...

https://codereview.chromium.org/1235603003/diff/40001/Source/core/html/HTMLAn...
File Source/core/html/HTMLAnchorElement.cpp (right):

https://codereview.chromium.org/1235603003/diff/40001/Source/core/html/HTMLAn...
Source/core/html/HTMLAnchorElement.cpp:68: m_ping->setObserver(nullptr);
This doesn't work on Oilpan because m_ping can be destructed before |this|.  On
Oilpan, DOMSettableTokenList::m_observer is a weak member, so we don't need to
do anything here.
We need #if here.

#if !ENABLE(OILPAN)
    m_ping->setObserver(nullptr)+
#endif

https://codereview.chromium.org/1235603003/diff/40001/Source/core/html/HTMLAn...
Source/core/html/HTMLAnchorElement.cpp:336: SpaceSplitString pingURLs =
m_ping->tokens();
should be |const SpaceSplitString&|.

https://codereview.chromium.org/1235603003/diff/40001/Source/core/html/HTMLAn...
File Source/core/html/HTMLAnchorElement.h (right):

https://codereview.chromium.org/1235603003/diff/40001/Source/core/html/HTMLAn...
Source/core/html/HTMLAnchorElement.h:60: class CORE_EXPORT HTMLAnchorElement :
public HTMLElement, public DOMURLUtils, public DOMSettableTokenListObserver {
should be |private DOMSettableTokenListObserver| because HTMLAnchorElement
object doesn't need to behave as a DOMSettableTokenListObserver publicly.

https://codereview.chromium.org/1235603003/diff/40001/Source/core/html/HTMLAn...
Source/core/html/HTMLAnchorElement.h:119: virtual void valueChanged() override;
|virtual| is not necessary.

[philipj_slow, 2015-07-14 08:21:10]

On 2015/07/14 00:22:59, tkent wrote:
> On 2015/07/13 14:31:31, philipj wrote:
> > Perhaps we ought to discuss with other browser vendors if we really want to
> use
> > DOMTokenList in new contexts or not. tkent-san, WDYT?
> 
> IMO, the implementation complexity is not so high, and the behavior difference
> is not significant.  So, we may go ahead.
> Upstreaming tests to W3C web-platform-tests would be nice for future
> compatibility, but I'm not sure W3C web-platform-tests accepts tests for
> WHATWG-only features.

I don't think that should be a problem, I'm sure Ms2ger or James Graham as
reviewers would accept it.

[hyunjunekim2, 2015-07-17 13:59:54]

I updated this patch.
@philipj
I will request the review to Ms2ger.

[Ms2ger, 2015-07-17 14:06:38]

Tests for all specs WHATWG are most welcome in web-platform-tests; it's
web-platform-tests, not w3c-tests.

[hyunjunekim2, 2015-07-17 14:21:28]

hyunjune.kim@samsung.com changed reviewers:
+ ms2ger@gmail.com

[philipj_slow, 2015-07-20 14:47:56]

I'll let this review sit for a bit while the tests are upstreamed, please ping
when I should take another look.

[hyunjunekim2, 2015-09-28 12:14:30]

On 2015/07/20 14:47:56, philipj wrote:
> I'll let this review sit for a bit while the tests are upstreamed, please ping
> when I should take another look.

@philipj, I'm so sorry, I will re-support this issue on tomorrow.

[hyunjunekim2, 2015-09-29 23:19:51]

On 2015/09/28 12:14:30, hyunjunekim2 wrote:
> On 2015/07/20 14:47:56, philipj wrote:
> > I'll let this review sit for a bit while the tests are upstreamed, please
ping
> > when I should take another look.
> 
> @philipj, I'm so sorry, I will re-support this issue on tomorrow.

@philipj, I re-based it. Could you check this patch? If there is problems, give
me feedback.

[philipj_slow, 2015-09-30 11:20:41]

Unless you can change the base URL
(https://chromium.googlesource.com/chromium/blink.git@master) I think you'll
have to create a new review.