Bug 51955 – Missing DOM bindings for a ping

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Erik Arvidsson]

There is currently no DOM bindings for the ping attribute of a elements.

[Erik Arvidsson]

Created attachment 87569 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 87569 [details]
Patch

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

This test doesn't verify that setting the attribute through DOM actually makes ping happen. I think that it should.

r- due to not testing that the attribute has any effect.

> LayoutTests/ChangeLog:10
> +        * fast/dom/script-tests/ping-attribute-dom-binding.js: Added.

Please don't split tests into .html and .js parts. This is not helpful.

[Erik Arvidsson]

(In reply to comment #2)
> (From update of attachment 87569 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=87569&action=review
> 
> This test doesn't verify that setting the attribute through DOM actually makes ping happen. I think that it should.

This is already covered by other tests.

> r- due to not testing that the attribute has any effect.
> 
> > LayoutTests/ChangeLog:10
> > +        * fast/dom/script-tests/ping-attribute-dom-binding.js: Added.
> 
> Please don't split tests into .html and .js parts. This is not helpful.

In that case should we update?

http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree

[Alexey Proskuryakov]

> In that case should we update?
> 
> http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree

Please do! That was never agreed upon, and splitting tests into two parts causes nothing but trouble:
- you get more files that you need to navigate in order to see or modify the source;
- you can't use static HTML in the test;
- it's more difficult to incrementally create a test like this from a bug reduction.

Using boilerplate like js-test-pre.js is good, but there is no need to split tests into two parts.

[Erik Arvidsson]

Created attachment 87581 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 87581 [details]
Patch

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

This still doesn't verify that a ping is sent if you set the attribute via DOM. I don't understand how it can be covered by other tests if the DOM attribute didn't exist before.

> LayoutTests/ChangeLog:10
> +        * fast/dom/script-tests/ping-attribute-dom-binding.js: Added.

This file no longer exists.

[Alexey Proskuryakov]

In the past, we had some bugs where DOM attributes weren't properly "connected", so they appeared to work, but did not do the job. I see that you test multiple ways to get the attribute, and it's pretty unlikely that ping won't work if all these agree, but it still doesn't test it end to end.

[Erik Arvidsson]

Created attachment 87593 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 87593 [details]
Patch for landing

Clearing flags on attachment: 87593

Committed r82499: <http://trac.webkit.org/changeset/82499>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.