Removes the custom toString callback for the various DOM constructors.

Removes the custom toString callback for the various DOM constructors.

Currently, functions like (for example) HTMLElement.toString() invoke a callback that obtains HTMLElement.toString's toString function and applies it to HTMLElement.

This leads to some weird behaviour, like:

> HTMLDocument.toString()
  "function HTMLDocument() { [native code] }"
> HTMLHRElement.toString()
  "function HTMLHRElement() { [native code] }"
> HTMLDocument.toString.toString = function() { return "foobar"; }
  function () { return "foobar"; }
> HTMLHRElement.toString()
  "foobar"
> HTMLDivElement
  function "foobar"

If we don't install the custom toString callback, everything works as expected.

BUG=

Committed: https://crrev.com/6089c0dadd2f88366ce775da7a4de63d784978b6
Cr-Commit-Position: refs/heads/master@{#362998}

[epertoso, 2015-12-02 16:43:10]

epertoso@chromium.org changed reviewers:
+ haraken@chromium.org, jochen@chromium.org, verwaest@chromium.org

[epertoso, 2015-12-02 16:43:10]

epertoso@chromium.org changed required reviewers:
+ haraken@chromium.org, jochen@chromium.org

[sof, 2015-12-02 16:57:22]

dbc - commit messages that include the what/why/wherefore makes for
quicker-to-comprehend commits. Could you add some descriptive text next to
title?

[epertoso, 2015-12-02 17:24:35]

Description was changed from

==========
Removes the custom toString callback for the various DOM constructors.

BUG=
==========

to

==========
Removes the custom toString callback for the various DOM constructors.

Currently, functions like (for example) HTMLElement.toString() invoke a callback
that obtains HTMLElement.toString's toString function and applies it to
HTMLElement.

This leads to some weird behaviour, like:

> HTMLDocument.toString()
  "function HTMLDocument() { [native code] }"
> HTMLHRElement.toString()
  "function HTMLHRElement() { [native code] }"
> HTMLDocument.toString.toString = function() { return "foobar"; }
  function () { return "foobar"; }
> HTMLHRElement.toString()
  "foobar"
> HTMLDivElement
  function "foobar"

If we don't install the custom toString callback, everything works as expected.

BUG=
==========

[epertoso, 2015-12-02 17:26:21]

On 2015/12/02 at 16:57:22, sigbjornf wrote:
> dbc - commit messages that include the what/why/wherefore makes for
quicker-to-comprehend commits. Could you add some descriptive text next to
title?

Done. Let me know if you need more details. I'm waiting for haraken@ to comment,
he may have an explanation for why 'toString' was defined this way.

[sof, 2015-12-02 18:29:18]

On 2015/12/02 17:26:21, epertoso wrote:
> On 2015/12/02 at 16:57:22, sigbjornf wrote:
> > dbc - commit messages that include the what/why/wherefore makes for
> quicker-to-comprehend commits. Could you add some descriptive text next to
> title?
> 
> Done. Let me know if you need more details. I'm waiting for haraken@ to
comment,
> he may have an explanation for why 'toString' was defined this way.

Thank you for doing that, much clearer.

[jsbell, 2015-12-02 19:36:47]

jsbell@chromium.org changed reviewers:
+ jsbell@chromium.org

[jsbell, 2015-12-02 19:36:47]

cc: adamk who was involved in some changes here recently, IIRC

[adamk, 2015-12-02 22:10:15]

adamk@chromium.org changed reviewers:
+ adamk@chromium.org

[adamk, 2015-12-02 22:10:15]

I'm not sure that haraken is going to be able to provide any context here. I did
some git blame archeology and this dates back to at least
http://trac.webkit.org/changeset/45288 (June 2009) when the V8 bindings were
being upstreamed into WebKit, though I suspect this dates back even earlier than
that.

[adamk, 2015-12-02 22:10:49]

That being said, I'd love to hear if anyone can think of why this code would be
needed (or would have been needed in the past but isn't now).

[haraken, 2015-12-03 00:07:03]

Yeah, I don't know a context why we introduced the code.

Two questions:

- Does the new behavior align with the spec?
- Does the new behavior align with Firefox, Safari and IE?

If the answers are both yes, it would make sense to make this change.

[epertoso, 2015-12-03 10:09:37]

On 2015/12/03 at 00:07:03, haraken wrote:
> Yeah, I don't know a context why we introduced the code.
> 
> Two questions:
> 
> - Does the new behavior align with the spec?
> - Does the new behavior align with Firefox, Safari and IE?
> 
> If the answers are both yes, it would make sense to make this change.

- This returns what Function.prototype.toString() should return according to the
ECMAScript spec.
- This behaviour aligns with Firefox, but not with Safari and IE:

HTMLElement.toString()

Chromium (with and without this patch):
"function HTMLElement() { [native code] }"


Firefox:
"function HTMLElement() {
    [native code]
}"


Safari:
"[object HTMLElementConstructor]"


IE:
"[object HTMLElement]"

[haraken, 2015-12-03 10:28:14]

On 2015/12/03 10:09:37, epertoso wrote:
> On 2015/12/03 at 00:07:03, haraken wrote:
> > Yeah, I don't know a context why we introduced the code.
> > 
> > Two questions:
> > 
> > - Does the new behavior align with the spec?
> > - Does the new behavior align with Firefox, Safari and IE?
> > 
> > If the answers are both yes, it would make sense to make this change.
> 
> - This returns what Function.prototype.toString() should return according to
the
> ECMAScript spec.
> - This behaviour aligns with Firefox, but not with Safari and IE:
> 
> HTMLElement.toString()
> 
> Chromium (with and without this patch):
> "function HTMLElement() { [native code] }"
> 
> 
> Firefox:
> "function HTMLElement() {
>     [native code]
> }"
> 
> 
> Safari:
> "[object HTMLElementConstructor]"
> 
> 
> IE:
> "[object HTMLElement]"

Given that this matches with the spec and Firefox, I'd support the change.

Can we add a layout test?

[epertoso, 2015-12-03 10:33:13]

On 2015/12/03 at 10:28:14, haraken wrote:
> On 2015/12/03 10:09:37, epertoso wrote:
> > On 2015/12/03 at 00:07:03, haraken wrote:
> > > Yeah, I don't know a context why we introduced the code.
> > > 
> > > Two questions:
> > > 
> > > - Does the new behavior align with the spec?
> > > - Does the new behavior align with Firefox, Safari and IE?
> > > 
> > > If the answers are both yes, it would make sense to make this change.
> > 
> > - This returns what Function.prototype.toString() should return according to
the
> > ECMAScript spec.
> > - This behaviour aligns with Firefox, but not with Safari and IE:
> > 
> > HTMLElement.toString()
> > 
> > Chromium (with and without this patch):
> > "function HTMLElement() { [native code] }"
> > 
> > 
> > Firefox:
> > "function HTMLElement() {
> >     [native code]
> > }"
> > 
> > 
> > Safari:
> > "[object HTMLElementConstructor]"
> > 
> > 
> > IE:
> > "[object HTMLElement]"
> 
> Given that this matches with the spec and Firefox, I'd support the change.
> 
> Can we add a layout test?

We already have element-constructors-on-window.html, for example. This CL
doesn't change the output of <element constructor>.toString().

[haraken, 2015-12-03 10:37:30]

On 2015/12/03 10:33:13, epertoso wrote:
> On 2015/12/03 at 10:28:14, haraken wrote:
> > On 2015/12/03 10:09:37, epertoso wrote:
> > > On 2015/12/03 at 00:07:03, haraken wrote:
> > > > Yeah, I don't know a context why we introduced the code.
> > > > 
> > > > Two questions:
> > > > 
> > > > - Does the new behavior align with the spec?
> > > > - Does the new behavior align with Firefox, Safari and IE?
> > > > 
> > > > If the answers are both yes, it would make sense to make this change.
> > > 
> > > - This returns what Function.prototype.toString() should return according
to
> the
> > > ECMAScript spec.
> > > - This behaviour aligns with Firefox, but not with Safari and IE:
> > > 
> > > HTMLElement.toString()
> > > 
> > > Chromium (with and without this patch):
> > > "function HTMLElement() { [native code] }"
> > > 
> > > 
> > > Firefox:
> > > "function HTMLElement() {
> > >     [native code]
> > > }"
> > > 
> > > 
> > > Safari:
> > > "[object HTMLElementConstructor]"
> > > 
> > > 
> > > IE:
> > > "[object HTMLElement]"
> > 
> > Given that this matches with the spec and Firefox, I'd support the change.
> > 
> > Can we add a layout test?
> 
> We already have element-constructors-on-window.html, for example. This CL
> doesn't change the output of <element constructor>.toString().

Can we test something like what you described in the CL description? This CL
changes web-exposed behavior if you set HTMLDocument.toString.toString.

[epertoso, 2015-12-03 11:27:04]

On 2015/12/03 at 10:37:30, haraken wrote:
> On 2015/12/03 10:33:13, epertoso wrote:
> > On 2015/12/03 at 10:28:14, haraken wrote:
> > > On 2015/12/03 10:09:37, epertoso wrote:
> > > > On 2015/12/03 at 00:07:03, haraken wrote:
> > > > > Yeah, I don't know a context why we introduced the code.
> > > > > 
> > > > > Two questions:
> > > > > 
> > > > > - Does the new behavior align with the spec?
> > > > > - Does the new behavior align with Firefox, Safari and IE?
> > > > > 
> > > > > If the answers are both yes, it would make sense to make this change.
> > > > 
> > > > - This returns what Function.prototype.toString() should return
according to
> > the
> > > > ECMAScript spec.
> > > > - This behaviour aligns with Firefox, but not with Safari and IE:
> > > > 
> > > > HTMLElement.toString()
> > > > 
> > > > Chromium (with and without this patch):
> > > > "function HTMLElement() { [native code] }"
> > > > 
> > > > 
> > > > Firefox:
> > > > "function HTMLElement() {
> > > >     [native code]
> > > > }"
> > > > 
> > > > 
> > > > Safari:
> > > > "[object HTMLElementConstructor]"
> > > > 
> > > > 
> > > > IE:
> > > > "[object HTMLElement]"
> > > 
> > > Given that this matches with the spec and Firefox, I'd support the change.
> > > 
> > > Can we add a layout test?
> > 
> > We already have element-constructors-on-window.html, for example. This CL
> > doesn't change the output of <element constructor>.toString().
> 
> Can we test something like what you described in the CL description? This CL
changes web-exposed behavior if you set HTMLDocument.toString.toString.

Added one.

[jochen (gone - plz use gerrit), 2015-12-03 11:41:42]

for this test, it's probably better to write a testharness based test:
https://darobin.github.io/test-harness-tutorial/docs/using-testharness.html

https://codereview.chromium.org/1497503002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/Window/element-constructors-to-string.html
(right):

https://codereview.chromium.org/1497503002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/Window/element-constructors-to-string.html:1:
<html>
should start with <!DOCTYPE html>

https://codereview.chromium.org/1497503002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/Window/element-constructors-to-string.html:6:
testRunner.dumpAsText();
also 4sp indent in html/js

https://codereview.chromium.org/1497503002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/Window/element-constructors-to-string.html:23:
test("HTMLElement");
test() is declared without parameters

[Toon Verwaest, 2015-12-03 11:54:36]

Does the old code align better however? I presume not at all. 
Does FF have a custom .toString installed on each constructor, or does it just
use the one from the prototype chain?

[epertoso, 2015-12-03 12:04:18]

https://codereview.chromium.org/1497503002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/Window/element-constructors-to-string.html
(right):

https://codereview.chromium.org/1497503002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/Window/element-constructors-to-string.html:1:
<html>
On 2015/12/03 at 11:41:42, jochen wrote:
> should start with <!DOCTYPE html>

Done.

https://codereview.chromium.org/1497503002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/Window/element-constructors-to-string.html:6:
testRunner.dumpAsText();
On 2015/12/03 at 11:41:42, jochen wrote:
> also 4sp indent in html/js

Done.

https://codereview.chromium.org/1497503002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/Window/element-constructors-to-string.html:23:
test("HTMLElement");
On 2015/12/03 at 11:41:42, jochen wrote:
> test() is declared without parameters

Now using testharness.js.

[jochen (gone - plz use gerrit), 2015-12-03 12:09:11]

https://codereview.chromium.org/1497503002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/Window/element-constructors-to-string-expected.txt
(right):

https://codereview.chromium.org/1497503002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/Window/element-constructors-to-string-expected.txt:1:
Test that HTML element constructors' toString is just
Function.prototype.toString, and that changing <HTML ELEMENT>.toString.toString
doesn't change the output of <HTML ELEMENT>.toString().
i'm surprised you need the expectation? maybe if you leave out the <p> it'll be
happy without the expectation?

https://codereview.chromium.org/1497503002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/Window/element-constructors-to-string.html
(right):

https://codereview.chromium.org/1497503002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/Window/element-constructors-to-string.html:2:
<body>
you don't need a body

[epertoso, 2015-12-03 13:01:00]

https://codereview.chromium.org/1497503002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/Window/element-constructors-to-string-expected.txt
(right):

https://codereview.chromium.org/1497503002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/Window/element-constructors-to-string-expected.txt:1:
Test that HTML element constructors' toString is just
Function.prototype.toString, and that changing <HTML ELEMENT>.toString.toString
doesn't change the output of <HTML ELEMENT>.toString().
On 2015/12/03 at 12:09:10, jochen wrote:
> i'm surprised you need the expectation? maybe if you leave out the <p> it'll
be happy without the expectation?

Indeed.

https://codereview.chromium.org/1497503002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/Window/element-constructors-to-string.html
(right):

https://codereview.chromium.org/1497503002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/Window/element-constructors-to-string.html:2:
<body>
On 2015/12/03 at 12:09:11, jochen wrote:
> you don't need a body

Done.

[haraken, 2015-12-03 14:50:29]

LGTM on my side. Let's wait for other reviewer's approval.

[jochen (gone - plz use gerrit), 2015-12-03 14:54:57]

lgtm

[epertoso, 2015-12-03 14:56:47]

The CQ bit was checked by epertoso@chromium.org

[commit-bot: I haz the power, 2015-12-03 14:57:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1497503002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1497503002/60001

[commit-bot: I haz the power, 2015-12-03 17:06:38]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-03 17:07:37]

Description was changed from

==========
Removes the custom toString callback for the various DOM constructors.

Currently, functions like (for example) HTMLElement.toString() invoke a callback
that obtains HTMLElement.toString's toString function and applies it to
HTMLElement.

This leads to some weird behaviour, like:

> HTMLDocument.toString()
  "function HTMLDocument() { [native code] }"
> HTMLHRElement.toString()
  "function HTMLHRElement() { [native code] }"
> HTMLDocument.toString.toString = function() { return "foobar"; }
  function () { return "foobar"; }
> HTMLHRElement.toString()
  "foobar"
> HTMLDivElement
  function "foobar"

If we don't install the custom toString callback, everything works as expected.

BUG=
==========

to

==========
Removes the custom toString callback for the various DOM constructors.

Currently, functions like (for example) HTMLElement.toString() invoke a callback
that obtains HTMLElement.toString's toString function and applies it to
HTMLElement.

This leads to some weird behaviour, like:

> HTMLDocument.toString()
  "function HTMLDocument() { [native code] }"
> HTMLHRElement.toString()
  "function HTMLHRElement() { [native code] }"
> HTMLDocument.toString.toString = function() { return "foobar"; }
  function () { return "foobar"; }
> HTMLHRElement.toString()
  "foobar"
> HTMLDivElement
  function "foobar"

If we don't install the custom toString callback, everything works as expected.

BUG=

Committed: https://crrev.com/6089c0dadd2f88366ce775da7a4de63d784978b6
Cr-Commit-Position: refs/heads/master@{#362998}
==========

[commit-bot: I haz the power, 2015-12-03 17:07:38]

Patchset 4 (id:??) landed as
https://crrev.com/6089c0dadd2f88366ce775da7a4de63d784978b6
Cr-Commit-Position: refs/heads/master@{#362998}