Add dataset property to SVGElement interface.

Add dataset property to SVGElement interface.

Spec: https://svgwg.org/svg2-draft/types.html#InterfaceSVGElement

BUG=636108
Committed: https://crrev.com/25105789b773639799f91afaff54dec8f63b023c
Cr-Commit-Position: refs/heads/master@{#414654}

[ramya.v, 2016-08-11 06:17:52]

The CQ bit was checked by ramya.v@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-11 06:18:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-11 07:26:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-11 07:26:48]

Dry run: This issue passed the CQ dry run.

[ramya.v, 2016-08-11 08:38:57]

ramya.v@samsung.com changed reviewers:
+ foolip@chromium.org

[ramya.v, 2016-08-11 08:38:58]

PTAL!

Thanks

[foolip, 2016-08-11 08:53:16]

Description was changed from

==========
Add dataset property to SVG Element interface.

Spec: https://svgwg.org/svg2-draft/types.html#InterfaceSVGElement

BUG=636108
==========

to

==========
Add dataset property to SVGElement interface.

Spec: https://svgwg.org/svg2-draft/types.html#InterfaceSVGElement

BUG=636108
==========

[foolip, 2016-08-11 08:54:29]

Can you investigate and summarize the state of support in other engines?

[ramya.v, 2016-08-11 09:14:44]

On 2016/08/11 08:54:29, foolip wrote:
> Can you investigate and summarize the state of support in other engines?

WebKit - Supported on Element interface (indirectly supported for
HTML/SVG/MathML elements)
Blink - Currently supported on HTMLElement interface, now planning to support on
SVGElement interface.
Firefox - Currently supported on HTMLElement interface, now planning to support
on SVGElement interface. (https://bugzilla.mozilla.org/show_bug.cgi?id=921834)
IE - Currently supported on HTMLElement interface.

[foolip, 2016-08-11 12:10:13]

On 2016/08/11 09:14:44, ramya.v wrote:
> On 2016/08/11 08:54:29, foolip wrote:
> > Can you investigate and summarize the state of support in other engines?
> 
> WebKit - Supported on Element interface (indirectly supported for
> HTML/SVG/MathML elements)
> Blink - Currently supported on HTMLElement interface, now planning to support
on
> SVGElement interface.
> Firefox - Currently supported on HTMLElement interface, now planning to
support
> on SVGElement interface. (https://bugzilla.mozilla.org/show_bug.cgi?id=921834)
> IE - Currently supported on HTMLElement interface.

OK, thanks!

[foolip, 2016-08-11 12:11:55]

foolip@chromium.org changed reviewers:
+ chrishtr@chromium.org, pdr@chromium.org

[foolip, 2016-08-11 12:11:55]

chrishtr@, pdr@, I think this is trivial enough to not need an Intent to
Implement and Ship, would you agree?

Ramya, can you add a test or point to a test that already exercises dataset for
SVG elements? It's not unthinkable that it'd be broken for SVG even if it's on
Element internally.

[fs, 2016-08-11 17:05:08]

On 2016/08/11 at 12:11:55, foolip wrote:
> chrishtr@, pdr@, I think this is trivial enough to not need an Intent to
Implement and Ship, would you agree?
> 
> Ramya, can you add a test or point to a test that already exercises dataset
for SVG elements? It's not unthinkable that it'd be broken for SVG even if it's
on Element internally.

And, for history buffs: https://codereview.chromium.org/1189663004 =)

[chrishtr, 2016-08-11 17:35:08]

I know it's small, but please send an intent anyway. Will get quick approval if
it is ok.

[ramya.v, 2016-08-12 09:08:43]

On 2016/08/11 17:35:08, chrishtr wrote:
> I know it's small, but please send an intent anyway. Will get quick approval
if
> it is ok.


@philip
Added a test dataset-svg.html to test dataset on SVGElement which is similar to
existing fast/dom/dataset.html which tests only HTMLElement div. PTAL!

@chrishtr - Intent to Implement And Ship Link -
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/zXlpFPXhyvE

[chrishtr, 2016-08-23 19:48:11]

pdr, could you review the test?

[pdr., 2016-08-24 18:42:15]

https://codereview.chromium.org/2231053003/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/dom/dataset-svg.html (right):

https://codereview.chromium.org/2231053003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/dataset-svg.html:2: <script
src="../../resources/testharness.js"></script>
This test copies fast/dom/script-tests/dataset.js. Instead of copying that test,
can we modify it to work for both svg and html?

One way to do this is to wrap all of dataset.js in a function like so:

function testDataset(getTestElement) {
    function testGet(attr, expected)
    {
        var d = getTestElement();
        d.setAttribute(attr, "value");
        return d.dataset[expected] == "value";
    }

    shouldBeTrue("testGet('data-foo', 'foo')");
    shouldBeTrue("testGet('data-foo-bar', 'fooBar')");
...
    var element = getTestElement();
    shouldBe("element.dataset['foo'] = 'on dataset'; element.dataset.foo", "'on
dataset'");
...
}

Then at the bottom of the test write:
testDataset(function() { return document.createElement("div"); });
testDataset(function() { return
document.createElementNS("http://www.w3.org/2000/svg", "circle"); });

[ramya.v, 2016-08-25 03:47:55]

On 2016/08/24 18:42:15, pdr. wrote:
>
https://codereview.chromium.org/2231053003/diff/20001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/fast/dom/dataset-svg.html (right):
> 
>
https://codereview.chromium.org/2231053003/diff/20001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/fast/dom/dataset-svg.html:2: <script
> src="../../resources/testharness.js"></script>
> This test copies fast/dom/script-tests/dataset.js. Instead of copying that
test,
> can we modify it to work for both svg and html?
> 
> One way to do this is to wrap all of dataset.js in a function like so:
> 
> function testDataset(getTestElement) {
>     function testGet(attr, expected)
>     {
>         var d = getTestElement();
>         d.setAttribute(attr, "value");
>         return d.dataset[expected] == "value";
>     }
> 
>     shouldBeTrue("testGet('data-foo', 'foo')");
>     shouldBeTrue("testGet('data-foo-bar', 'fooBar')");
> ...
>     var element = getTestElement();
>     shouldBe("element.dataset['foo'] = 'on dataset'; element.dataset.foo",
"'on
> dataset'");
> ...
> }
> 
> Then at the bottom of the test write:
> testDataset(function() { return document.createElement("div"); });
> testDataset(function() { return
> document.createElementNS("http://www.w3.org/2000/svg", "circle"); });

If we wrap this way, getting the below error at line
shouldBeTrue("testGet('data-foo', 'foo')");
FAIL testGet('data-foo', 'foo') should be true. Threw exception ReferenceError:
testGet is not defined.

[ramya.v, 2016-08-25 10:51:33]

@pdr Updated as per review comments. PTAL!


Thanks

https://codereview.chromium.org/2231053003/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/dom/dataset-svg.html (right):

https://codereview.chromium.org/2231053003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/dataset-svg.html:2: <script
src="../../resources/testharness.js"></script>
On 2016/08/24 18:42:15, pdr. wrote:
> This test copies fast/dom/script-tests/dataset.js. Instead of copying that
test,
> can we modify it to work for both svg and html?
> 
> One way to do this is to wrap all of dataset.js in a function like so:
> 
> function testDataset(getTestElement) {
>     function testGet(attr, expected)
>     {
>         var d = getTestElement();
>         d.setAttribute(attr, "value");
>         return d.dataset[expected] == "value";
>     }
> 
>     shouldBeTrue("testGet('data-foo', 'foo')");
>     shouldBeTrue("testGet('data-foo-bar', 'fooBar')");
> ...
>     var element = getTestElement();
>     shouldBe("element.dataset['foo'] = 'on dataset'; element.dataset.foo",
"'on
> dataset'");
> ...
> }
> 
> Then at the bottom of the test write:
> testDataset(function() { return document.createElement("div"); });
> testDataset(function() { return
> document.createElementNS("http://www.w3.org/2000/svg", "circle"); });


If we wrap this way, getting the below error at line
shouldBeTrue("testGet('data-foo', 'foo')");
FAIL testGet('data-foo', 'foo') should be true. Threw exception ReferenceError:
testGet is not defined.
Since testGet is not detected inside quotes, modified the existing test into
testharness based test and now able to wrap in the suggested way.

Done.

[pdr., 2016-08-25 18:13:33]

Nice work combining the testcases. LGTM

https://codereview.chromium.org/2231053003/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/dom/dataset.html (right):

https://codereview.chromium.org/2231053003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/dataset.html:38: if (e instanceof
HTMLElement)
Can you add a comment above this line saying something like:
// HTML lowercases all attributes whereas SVG does not.

[chrishtr, 2016-08-25 18:56:00]

lgtm

[ramya.v, 2016-08-26 03:35:50]

The CQ bit was checked by ramya.v@samsung.com

[ramya.v, 2016-08-26 03:35:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2231053003/#ps80001
(title: "Added comment")

[ramya.v, 2016-08-26 03:36:01]

https://codereview.chromium.org/2231053003/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/dom/dataset.html (right):

https://codereview.chromium.org/2231053003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/dataset.html:38: if (e instanceof
HTMLElement)
On 2016/08/25 18:13:33, pdr. wrote:
> Can you add a comment above this line saying something like:
> // HTML lowercases all attributes whereas SVG does not.

Done.

[commit-bot: I haz the power, 2016-08-26 03:36:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 05:26:21]

Description was changed from

==========
Add dataset property to SVGElement interface.

Spec: https://svgwg.org/svg2-draft/types.html#InterfaceSVGElement

BUG=636108
==========

to

==========
Add dataset property to SVGElement interface.

Spec: https://svgwg.org/svg2-draft/types.html#InterfaceSVGElement

BUG=636108
==========

[commit-bot: I haz the power, 2016-08-26 05:26:22]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-26 05:28:36]

Description was changed from

==========
Add dataset property to SVGElement interface.

Spec: https://svgwg.org/svg2-draft/types.html#InterfaceSVGElement

BUG=636108
==========

to

==========
Add dataset property to SVGElement interface.

Spec: https://svgwg.org/svg2-draft/types.html#InterfaceSVGElement

BUG=636108

Committed: https://crrev.com/25105789b773639799f91afaff54dec8f63b023c
Cr-Commit-Position: refs/heads/master@{#414654}
==========

[commit-bot: I haz the power, 2016-08-26 05:28:37]

Patchset 5 (id:??) landed as
https://crrev.com/25105789b773639799f91afaff54dec8f63b023c
Cr-Commit-Position: refs/heads/master@{#414654}