New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 9 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 651748



Sign in to add a comment
link

Issue 643712: Incorrect [[Class]] for DOM prototypes: "Foo" vs. "FooPrototype"

Reported by jsb...@chromium.org, Sep 2 2016 Project Member

Issue description

Revisiting  issue 239915  rather than re-opening.

The internal class for DOM prototypes is set to "InterfaceName". It should be set to "InterfaceNamePrototype". For example:

assertEquals('[object DocumentPrototype]',Object.prototype.toString.call(Document.prototype))

http://heycam.github.io/webidl/#interface-prototype-object

"The class string of an interface prototype object is the concatenation of the interface’s identifier and the string “Prototype”."

This causes many failures in WebIDL tests (e.g. LayoutTests/imported/wpt/*/idlharness.html)

Worried about performance issues with the WebIDL's spec requiring a @@toStringTag applied to every instance to override the prototype's name we proposed that the prototype and interface have the same "class string" (simply the interface name). This was implemented to resolve  issue 239915  but other browser implementers have pushed back.

https://www.w3.org/Bugs/Public/show_bug.cgi?id=28244
 

Comment 1 by jsb...@chromium.org, Sep 2 2016

Cc: domenic@chromium.org

Comment 2 by domenic@chromium.org, Sep 2 2016

We're at a spec impasse here. I strongly believe that what we're doing is better; it's more consistent with ES's builtins, and with the prototypal model in general. Boris doesn't agree. The current spec is written in terms of ES5, so it's not clear it even makes any sense in the modern world.

Maybe we can get WebKit or Microsoft to weigh in, but I really don't think we should change this in Chrome.

Comment 3 by adamk@chromium.org, Sep 2 2016

At the highest level of the question (should Blink change anything here?) I definitely agree with Domenic. I think the right path forward here is to continue to push for change on the spec side. The consistency-with-ES6 argument is a decent one, as is the idea that there's nothing particularly "special" about objects that act as prototypes: whenever possible (except for a few cases like Function.prototype), they're just plain objects, and I don't see why WebIDL should have anything in particular to say about the toString behavior of built-in prototypes.

Comment 4 by jsb...@chromium.org, Sep 2 2016

And just to be clear, I filed this as a tracking issue (so we can point test failures at it); I'm not advocating for a Blink change myself.

Comment 5 by jsb...@chromium.org, Sep 30 2016

Blocking: 651748

Comment 6 by rbyers@chromium.org, Dec 16 2016

Cc: rbyers@chromium.org
Labels: -Pri-3 Pri-2
This is a real pain for IDL tests.  Who is responsible for "continuing to push for change on the spec side here"?  Domenic?

If we're not actively pushing for consensus here, then perhaps we should just admit defeat (since ALL other major engines follow the spec)?

Comment 7 by domenic@chromium.org, Dec 16 2016

An alternative worth mentioning is to patch idlharness.js to not test this, since there's definitely not consensus, and it's only in the spec because the spec is edited by Mozilla.

Comment 8 by adamk@chromium.org, Dec 19 2016

When you say "all other major engines follow the spec", do they actually use @@toStringTag to do so? I don't have easy access to Firefox 51 at the moment, but Firefox 50 (current stable) doesn't even implement Symbol.toStringTag.

Comment 9 by domenic@chromium.org, Dec 19 2016

As I noted in https://www.w3.org/Bugs/Public/show_bug.cgi?id=28244#c19 no other engines follow the spec either; none of them use @@toStringTag.

Comment 10 by rbyers@chromium.org, Dec 19 2016

Sorry, by "follow the spec" I just meant that the default name was FooPrototype.

Comment 11 by ricea@chromium.org, Jan 20 2017

Blocking: 651761

Comment 12 by jsb...@chromium.org, Feb 1 2017

Cc: yukishiino@chromium.org
 Issue 687431  has been merged into this issue.

Comment 13 by mgiuca@chromium.org, Feb 1 2017

Cc: imch...@chromium.org mlamouri@chromium.org avayvod@chromium.org phil...@opera.com
 Issue 627604  has been merged into this issue.

Comment 14 by mgiuca@chromium.org, Feb 1 2017

Cc: mgiuca@chromium.org
I agree with #7 -- if there is no consensus on this we should remove the check from idlharness.js. We can't add new idlharness tests to Chrome while this is an issue. Note that there is now a presubmit warning telling you to use idlharness.

Is there any progress on this?

(I'm trying to be a good citizen when I add a new API but every time I do, there is some discrepancy between Blink and idlharness.)

Comment 15 by mgiuca@chromium.org, Feb 1 2017

#14: My bad, the presubmit warning tells you to use testharness, not idlharness.

Comment 16 by jsb...@chromium.org, Feb 1 2017

Yeah, idharness is just a helper library when using testharness. 

That said (as I mentioned on the other bug) it's okay to have testharness-based tests with expectation files with FAIL entries. IMHO it's preferable to use idlharness to validate the IDL rather than rolling your own.

And yes, we should keep pushing to resolve this at the working group/web platform testing level.

Comment 17 by mfo...@chromium.org, Feb 1 2017

The idlharness enforces this because it's required by WebIDL per Comment #1. If you think WebIDL should be changed then we should file an issue against WebIDL. Otherwise it's a Blink bug.

I am wondering if this is an incompatibility we are willing to live with so we don't have to generate additional code into the bindings, and take the memory hit of storing another string on every WebIDL prototype.  Does anyone on the bindings team have feedback here?

Comment 18 by phistuck@gmail.com, Feb 1 2017

#17 - is this a bug filed against WebIDL?
https://www.w3.org/Bugs/Public/show_bug.cgi?id=28244

Comment 19 by mfo...@chromium.org, Feb 1 2017

Yes that's a relevant bug and I'm not sure I see consensus happening soon.
In the meantime we'll just have to add FAIL expectations for Presentation API.

Comment 20 by yukishiino@chromium.org, Feb 2 2017

Re #c17: I'm Yuki on the bindings team.

I have a complex feeling on this issue.  Changing the spec to the Blink's way makes sense and it's ideal to me (just as my personal preference).  And, I'm fine to live with this incompatibility so far.  I've not heard that web developers have trouble on this issue.

If it's only us who have trouble with this incompatibility (not including web developers), then any of the following options would actually work, IMHO.
  a) work to change the spec
  b) simply ignore the failures (with FAIL expectations)
  c) update idlharness Blink-locally
And if so, I'd prefer a reasonably easy way.

Note that we don't currently set @@toStringTag on each platform object.
  domObj[Symbol.toStringTag]
  => look for @@toStringTag in the prototype chain
  => found domObj.__proto__[Symbol.toStringTag]
  # We share a single @@toStringTag property among many platform objects.
However, the spec requires that each platform object has @@toStringTag as an own property.
  domObj[Symbol.toStringTag]
  => found an own property of string "PrimaryInterfaceName"
  domObj.__proto__
  => "InterfaceNamePrototype"
So, the spec requires more memory, but the reason I personally prefer Blink's way is that it's more consistent with ES spec, as other guys said.

Comment 21 by mfo...@chromium.org, Feb 2 2017

Yeah, I don't have any strong feelings from a web developers or web compatibility point of view.  It's more of an inconvenience for me as idlharness is part of our web platform tests suite.  I'll be going with b).

Comment 22 by mgiuca@chromium.org, Feb 2 2017

I agree this isn't likely to affect web developers; it's the second thing I've found in idlharness that is exceptionally picky and is only a nuisance for us.

I think (b) is the easiest approach but bad for us -- it means we have to have a FAIL expectation for every single API that uses idlharness (which hopefully is all APIs going forward), and every single Blink developer who uses it is going to run into this problem.

I'd prefer (c).

There's also:
  d) remove this particular check from idlharness upstream (since there is no consensus on this part of the spec, maybe the W3C will be open to just not testing against it, until there is consensus).

Which would be my preference.

Comment 23 by foolip@chromium.org, Feb 3 2017

I would suggest first trying to comment out the check in question in idlharness.js upstream, with a comment pointing to the unresolved spec discussion. And, perhaps, so that the issue isn't hidden entirely in web-platform-tests, add a single separate test that fails. Perhaps that'll be an acceptable compromise.

Comment 24 by tkent@chromium.org, Mar 3 2017

Blocking: 651572

Comment 25 by tkent@chromium.org, May 9 2017

https://github.com/w3c/web-platform-tests/pull/5653 did #23.  This issue doesn't affect IDL tests for each of APIs any longer.

Comment 26 by yukishiino@chromium.org, May 9 2017

Blocking: -651761 -651572 -435727
Labels: -Pri-2 Pri-3
Thanks a lot, tkent@!!!

This issue is no longer blocking:
issue 435727
 issue 651572 
issue 651761

Comment 27 by sheriffbot@chromium.org, May 9 2018

Project Member
Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 28 by adithyas@chromium.org, May 10 2018

Status: Available (was: Untriaged)

Comment 29 by yukishiino@chromium.org, May 11 2018

Status: Fixed (was: Available)
Browser vendors agreed on removal of this rule from idlharness.js and it was already done.  We no longer need to apply this rule.

Comment 30 by foolip@chromium.org, May 11 2018

Status: Available (was: Fixed)
idlharness.js was changed so that not every test would fail over this, but the spec issue and interop problem remains unresolved:
https://github.com/heycam/webidl/pull/357

Sign in to add a comment