[LayoutTests] add Symbols to webexposed global-interface-listing

[LayoutTests] add Symbols to webexposed global-interface-listing

BUG=

Committed: https://crrev.com/ba8fbf5a22905e9b688d7a6f6a96d827b4a49ce6
Cr-Commit-Position: refs/heads/master@{#380960}

[caitp (gmail), 2016-03-11 20:10:50]

https://codereview.chromium.org/1788503005/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/webexposed/resources/global-interface-listing.js
(right):

https://codereview.chromium.org/1788503005/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/webexposed/resources/global-interface-listing.js:139:
var protoProperties = new Set(Object.keys(object.prototype).concat(
I'm not sure if it's intentional to use Object.keys(), which omits
non-enumerable properties or not --- since "not-a-constructor" branch doesn't
only look at enumerable properties.

[jsbell, 2016-03-11 20:12:25]

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

[jsbell, 2016-03-11 20:12:25]

FYI, run-webkit-tests with this file/directory list should be fast and hit all
the permutations:

webexposed virtual/stable/webexposed http/tests/serviceworker/webexposed
virtual/stable/http/tests/serviceworker/webexposed

[jsbell, 2016-03-11 20:21:21]

thanks for taking this on!

[jsbell, 2016-03-11 20:23:46]

Somehow my inline comments were lost in the last message. trying again?

https://codereview.chromium.org/1788503005/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/webexposed/resources/global-interface-listing.js
(right):

https://codereview.chromium.org/1788503005/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/webexposed/resources/global-interface-listing.js:114:
function collectPropertyInfo(object, propertyName, output) {
Can we s/propertyName/propertyKey/ while we're here?

https://codereview.chromium.org/1788503005/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/webexposed/resources/global-interface-listing.js:115:
var propertyString = wellKnownSymbols.get(propertyName) ||
propertyName.toString();
Is this map necessary? Seems annoying to maintain. I'd be fine with
"Symbol(Symbol.iterator)" appearing in the output.

Or .replace(/^Symbol\(Symbol\.(.*)\)$/, '@@$1') if we want to be clever.

https://codereview.chromium.org/1788503005/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/webexposed/resources/global-interface-listing.js:139:
var protoProperties = new Set(Object.keys(object.prototype).concat(
On 2016/03/11 20:10:49, caitp wrote:
> I'm not sure if it's intentional to use Object.keys(), which omits
> non-enumerable properties or not --- since "not-a-constructor" branch doesn't
> only look at enumerable properties.

I'm okay with leaving this as-is for now, maybe with a TODO to improve it.

[caitp (gmail), 2016-03-11 20:29:01]

https://codereview.chromium.org/1788503005/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/webexposed/resources/global-interface-listing.js
(right):

https://codereview.chromium.org/1788503005/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/webexposed/resources/global-interface-listing.js:114:
function collectPropertyInfo(object, propertyName, output) {
On 2016/03/11 20:23:46, jsbell wrote:
> Can we s/propertyName/propertyKey/ while we're here?

Acknowledged.

https://codereview.chromium.org/1788503005/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/webexposed/resources/global-interface-listing.js:115:
var propertyString = wellKnownSymbols.get(propertyName) ||
propertyName.toString();
On 2016/03/11 20:23:46, jsbell wrote:
> Is this map necessary? Seems annoying to maintain. I'd be fine with
> "Symbol(Symbol.iterator)" appearing in the output.
> 
> Or .replace(/^Symbol\(Symbol\.(.*)\)$/, '@@$1') if we want to be clever.

hmm, it doesn't seem that hard --- I just wanted to distuingish between
ECMAScript-defined Symbols, and arbitrary ones introduced elsewhere --- which
would be harder and more expensive to do with a regexp. This seems like it would
make it easier to identify mistakenly added Symbols, but yeah, can avoid that.

I'm not really expecting the list of well known symbols to be expanded very
often, but if it does happen and does impact bindings in blink, having the map
provides an easy way to update the list.

[caitp (gmail), 2016-03-12 00:14:54]

I've updated test expectations, added a FIXME, and fixed a bug in the test
script

[jsbell, 2016-03-12 01:07:50]

lgtm

https://codereview.chromium.org/1788503005/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/webexposed/resources/global-interface-listing.js
(right):

https://codereview.chromium.org/1788503005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webexposed/resources/global-interface-listing.js:139:
// FIXME: Don't exclude  non-enumerable properties
nit: there's an extra space in "exclude  non-enumerable"
nit: we say 'TODO(username)' in Blink now too. (username doesn't need to be who
will fix it, just someone with a clue)

[caitp (gmail), 2016-03-12 05:14:17]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2016-03-12 05:14:18]

The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org
Link to the patchset: https://codereview.chromium.org/1788503005/#ps60001
(title: "fix nits")

[commit-bot: I haz the power, 2016-03-12 05:14:27]

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

[commit-bot: I haz the power, 2016-03-12 05:23:18]

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

[commit-bot: I haz the power, 2016-03-12 05:23:19]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[caitp (gmail), 2016-03-12 05:26:39]

Description was changed from

==========
[LayoutTests] add Symbols to webexposed global-interface-listing

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

to

==========
[LayoutTests] add Symbols to webexposed global-interface-listing

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

[caitp (gmail), 2016-03-12 05:26:39]

caitpotter88@gmail.com changed reviewers:
+ philipj@opera.com

[caitp (gmail), 2016-03-12 05:27:25]

On 2016/03/12 05:23:19, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

+philipj for test expectations sign off

[Yuki, 2016-03-14 08:58:43]

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

[Yuki, 2016-03-14 08:58:43]

The CQ bit was checked by yukishiino@chromium.org

[Yuki, 2016-03-14 08:58:44]

lgtm

[commit-bot: I haz the power, 2016-03-14 08:58:55]

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

[commit-bot: I haz the power, 2016-03-14 09:07:00]

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

[commit-bot: I haz the power, 2016-03-14 09:07:03]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[philipj_slow, 2016-03-14 09:28:28]

rs lgtm

[caitp (gmail), 2016-03-14 12:48:53]

The CQ bit was checked by caitpotter88@gmail.com

[commit-bot: I haz the power, 2016-03-14 12:49:02]

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

[commit-bot: I haz the power, 2016-03-14 12:54:06]

Description was changed from

==========
[LayoutTests] add Symbols to webexposed global-interface-listing

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

to

==========
[LayoutTests] add Symbols to webexposed global-interface-listing

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

[commit-bot: I haz the power, 2016-03-14 12:54:07]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-14 12:55:14]

Description was changed from

==========
[LayoutTests] add Symbols to webexposed global-interface-listing

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

to

==========
[LayoutTests] add Symbols to webexposed global-interface-listing

BUG=

Committed: https://crrev.com/ba8fbf5a22905e9b688d7a6f6a96d827b4a49ce6
Cr-Commit-Position: refs/heads/master@{#380960}
==========

[commit-bot: I haz the power, 2016-03-14 12:55:15]

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