Bug 145132 – [ES6] Arrow function syntax. Arrow function specific features. Lexical bind "arguments"

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[GSkachkov]

It is necessary to add lexical bind of the "arguments", "super", "new.target"

[GSkachkov]

Created attachment 259264 [details]
Patch

init version

[Build Bot]

Comment on attachment 259264 [details]
Patch

Attachment 259264 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/71653

New failing tests:
js/arrowfunction-lexical-bind-arguments.html

[Build Bot]

Created attachment 259265 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 259264 [details]
Patch

Attachment 259264 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/71661

New failing tests:
js/arrowfunction-lexical-bind-arguments.html

[Build Bot]

Created attachment 259266 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[GSkachkov]

Created attachment 259267 [details]
Patch

Fix tests

[Build Bot]

Comment on attachment 259267 [details]
Patch

Attachment 259267 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/72002

New failing tests:
js/arrowfunction-lexical-bind-arguments.html

[Build Bot]

Created attachment 259268 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 259267 [details]
Patch

Attachment 259267 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/72012

New failing tests:
js/arrowfunction-lexical-bind-arguments.html

[Build Bot]

Created attachment 259269 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[GSkachkov]

Created attachment 259270 [details]
Patch

Fix layout tests

[Build Bot]

Comment on attachment 259270 [details]
Patch

Attachment 259270 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/72159

New failing tests:
js/arrowfunction-lexical-bind-arguments.html

[Build Bot]

Created attachment 259274 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 259270 [details]
Patch

Attachment 259270 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/72163

New failing tests:
js/arrowfunction-lexical-bind-arguments.html

[Build Bot]

Created attachment 259275 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[GSkachkov]

Created attachment 259279 [details]
Patch

Fix layout tests

[GSkachkov]

Created attachment 259292 [details]
Patch

Small changes

[GSkachkov]

Created attachment 259366 [details]
Patch

Small improvementes

[GSkachkov]

Comment on attachment 259366 [details]
Patch

Looks like something wrong with build

[Blaze Burg]

(In reply to comment #19)
> Comment on attachment 259366 [details]
> Patch
> 
> Looks like something wrong with build

../../Source/JavaScriptCore/bytecode/BytecodeBasicBlock.cpp: In function 'void JSC::computeBytecodeBasicBlocks(JSC::CodeBlock*, WTF::Vector<WTF::RefPtr<JSC::BytecodeBasicBlock> >&)':
../../Source/JavaScriptCore/bytecode/BytecodeBasicBlock.cpp:229:1: internal compiler error: Segmentation fault

For GTK & EFL, this is almost certainly the result of the EWS machine running out of disk space, memory, or some other system resource. It's a known sporadic issue. You can ignore it.

[GSkachkov]

(In reply to comment #20)
> (In reply to comment #19)
> > Comment on attachment 259366 [details]
> > Patch
> > 
> > Looks like something wrong with build
> 
> ../../Source/JavaScriptCore/bytecode/BytecodeBasicBlock.cpp: In function
> 'void JSC::computeBytecodeBasicBlocks(JSC::CodeBlock*,
> WTF::Vector<WTF::RefPtr<JSC::BytecodeBasicBlock> >&)':
> ../../Source/JavaScriptCore/bytecode/BytecodeBasicBlock.cpp:229:1: internal
> compiler error: Segmentation fault
> 
> For GTK & EFL, this is almost certainly the result of the EWS machine
> running out of disk space, memory, or some other system resource. It's a
> known sporadic issue. You can ignore it.

OK. Thanks for info

[Saam Barati]

Comment on attachment 259366 [details]
Patch

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

The overall approach here is good. There needs to be some significant reworking in the bytecode generator
(and maybe the parser) here.
Also, we need many more tests.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:70
> +    if (m_needToInitializeArguments) {

It doesn't seem like you're doing any prevention of allocating the different arguments object while inside
an arrow function. This is bad. We need to prevent those allocations.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:73
> +            initializeVariable(variable(propertyNames().arguments), m_arrowFunctionArguments.get());

You can just use m_argumentsRegister here.
Also, we don't prefix local variables with "m_".

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2305
> +    RegisterID* m_tmpArgumentsRegister = emitGetFromScope(newTemporary(), scope, var, DoNotThrowIfNotFound);

This is pretty shady. We should just be using m_argumentsRegister here.
We shouldn't resolve anything. What this requires is that we now must note
that an arguments object must be created for functions that have arrow functions
nested inside them. We can get smart here, and we should if it's not too difficult, and only allocate these
arguments objects as being needed, i.e it the arrow function uses "arguments" or eval. Maybe a good first take before
optimizing is just report that any function with an arrow function nested inside of it needs to
allocate an arguments object (unless the parent function is an arrow function itself, in which
case it should get its arguments object through op_get_arrowfunction_arguments).

Once you get this working, you can optimize cases like this:
"function foo() { var x = () => 20; }". Obviously foo doesn't need an arguments object
"function foo() { var x = (p) => eval(p); }". foo does need arguments object
"function foo() { var x = () => { var x = () => arguments; } }". foo does need an arguments object if there is a chain from a non-arrow-function through only arrow functions that use arguments or eval.
"function foo() { var x = () => { var x = () { function bar() { var x = () => arguments; } } } }". bar function does need arguments. foo doesn't.

> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-arguments.js:1
> +var testCase = function (actual, expected, message) {

I don't think we're covering enough tests here.
JSC has three different arguments objects it allocates.
We should be writing tests to target each of these.
We also need to ensure different properties of the
arguments objects hold. For example, we shouldn't
be able to overwrite local variables for strict mode arguments
object. 

Does lexical binding of arguments object impose any restrictions on the type
of arguments object? Or does it not matter. If it doesn't matter, we need
tests for sloppy mode argument assignment inside an arrow function
which change "local" arguments parameters.

[GSkachkov]

Created attachment 268011 [details]
Patch

Upload new version of the patch

[Build Bot]

Comment on attachment 268011 [details]
Patch

Attachment 268011 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/627910

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 268017 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 268011 [details]
Patch

Attachment 268011 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/627923

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 268018 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 268011 [details]
Patch

Attachment 268011 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/627927

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 268019 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[GSkachkov]

Created attachment 268098 [details]
Patch

Fix tests

[Build Bot]

Comment on attachment 268098 [details]
Patch

Attachment 268098 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/638982

New failing tests:
inspector/css/generate-css-rule-string.html
inspector/css/stylesheet-events-inspector-stylesheet.html
imported/w3c/web-platform-tests/streams-api/readable-streams/garbage-collection.html
imported/w3c/web-platform-tests/streams-api/readable-streams/bad-underlying-sources.html
imported/w3c/web-platform-tests/streams-api/byte-length-queuing-strategy.html
inspector/console/messageRepeatCountUpdated.html
imported/w3c/web-platform-tests/streams-api/readable-streams/tee.html
imported/w3c/web-platform-tests/streams-api/readable-streams/bad-strategies.html
imported/w3c/web-platform-tests/streams-api/readable-streams/readable-stream-reader.html
imported/w3c/web-platform-tests/streams-api/readable-streams/pipe-through.html
imported/w3c/web-platform-tests/streams-api/readable-streams/count-queuing-strategy-integration.html
imported/w3c/web-platform-tests/streams-api/readable-streams/templated.html
inspector/css/stylesheet-events-basic.html
imported/w3c/web-platform-tests/streams-api/readable-streams/general.html
imported/w3c/web-platform-tests/streams-api/readable-streams/cancel.html
imported/w3c/web-platform-tests/streams-api/readable-streams/brand-checks.html
inspector/console/command-line-api.html
inspector/css/stylesheet-events-multiple-documents.html
imported/w3c/web-platform-tests/streams-api/count-queuing-strategy.html
inspector/network/client-blocked-load.html
inspector/model/stack-trace.html

[Build Bot]

Created attachment 268100 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 268098 [details]
Patch

Attachment 268098 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/638988

New failing tests:
inspector/css/stylesheet-events-inspector-stylesheet.html
imported/w3c/web-platform-tests/streams-api/readable-streams/bad-underlying-sources.html
imported/w3c/web-platform-tests/streams-api/readable-streams/count-queuing-strategy-integration.html
inspector/css/stylesheet-events-multiple-documents.html
imported/w3c/web-platform-tests/streams-api/byte-length-queuing-strategy.html
imported/w3c/web-platform-tests/streams-api/readable-streams/garbage-collection.html
inspector/console/messageRepeatCountUpdated.html
inspector/css/stylesheet-events-basic.html
imported/w3c/web-platform-tests/streams-api/count-queuing-strategy.html
inspector/console/command-line-api.html
imported/w3c/web-platform-tests/streams-api/readable-streams/cancel.html
inspector/dom/setOuterHTML.html
imported/w3c/web-platform-tests/streams-api/readable-streams/tee.html
imported/w3c/web-platform-tests/streams-api/readable-streams/general.html
imported/w3c/web-platform-tests/streams-api/readable-streams/brand-checks.html
inspector/model/stack-trace.html
inspector/css/generate-css-rule-string.html
imported/w3c/web-platform-tests/streams-api/readable-streams/bad-strategies.html
imported/w3c/web-platform-tests/streams-api/readable-streams/readable-stream-reader.html
imported/w3c/web-platform-tests/streams-api/readable-streams/pipe-through.html
imported/w3c/web-platform-tests/streams-api/readable-streams/templated.html
inspector/network/client-blocked-load.html

[Build Bot]

Created attachment 268101 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 268098 [details]
Patch

Attachment 268098 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/638994

New failing tests:
inspector/css/stylesheet-events-inspector-stylesheet.html
imported/w3c/web-platform-tests/streams-api/readable-streams/bad-underlying-sources.html
imported/w3c/web-platform-tests/streams-api/readable-streams/count-queuing-strategy-integration.html
inspector/css/stylesheet-events-multiple-documents.html
imported/w3c/web-platform-tests/streams-api/byte-length-queuing-strategy.html
imported/w3c/web-platform-tests/streams-api/readable-streams/garbage-collection.html
inspector/console/messageRepeatCountUpdated.html
inspector/css/stylesheet-events-basic.html
imported/w3c/web-platform-tests/streams-api/count-queuing-strategy.html
inspector/console/command-line-api.html
imported/w3c/web-platform-tests/streams-api/readable-streams/cancel.html
inspector/dom/setOuterHTML.html
imported/w3c/web-platform-tests/streams-api/readable-streams/tee.html
imported/w3c/web-platform-tests/streams-api/readable-streams/general.html
imported/w3c/web-platform-tests/streams-api/readable-streams/brand-checks.html
inspector/model/stack-trace.html
inspector/css/generate-css-rule-string.html
imported/w3c/web-platform-tests/streams-api/readable-streams/bad-strategies.html
imported/w3c/web-platform-tests/streams-api/readable-streams/readable-stream-reader.html
imported/w3c/web-platform-tests/streams-api/readable-streams/pipe-through.html
imported/w3c/web-platform-tests/streams-api/readable-streams/templated.html
inspector/network/client-blocked-load.html

[Build Bot]

Created attachment 268102 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[GSkachkov]

Created attachment 268144 [details]
Patch

Fix tests

[Build Bot]

Comment on attachment 268144 [details]
Patch

Attachment 268144 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/644887

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 268147 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 268144 [details]
Patch

Attachment 268144 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/644890

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 268148 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 268144 [details]
Patch

Attachment 268144 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/644899

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 268149 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[GSkachkov]

Created attachment 268192 [details]
Patch

Fix WebKit tests

[Build Bot]

Comment on attachment 268192 [details]
Patch

Attachment 268192 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/648878

New failing tests:
inspector/css/generate-css-rule-string.html
inspector/css/stylesheet-events-inspector-stylesheet.html
inspector/dom/setOuterHTML.html
inspector/css/stylesheet-events-basic.html
inspector/console/command-line-api.html
inspector/model/stack-trace.html
inspector/css/stylesheet-events-multiple-documents.html
inspector/console/messageRepeatCountUpdated.html
inspector/network/client-blocked-load.html

[Build Bot]

Created attachment 268196 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 268192 [details]
Patch

Attachment 268192 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/648877

New failing tests:
inspector/css/generate-css-rule-string.html
inspector/css/stylesheet-events-inspector-stylesheet.html
inspector/dom/setOuterHTML.html
inspector/css/stylesheet-events-basic.html
inspector/network/client-blocked-load.html
inspector/model/stack-trace.html
inspector/css/stylesheet-events-multiple-documents.html
inspector/console/messageRepeatCountUpdated.html
inspector/console/command-line-api.html

[Build Bot]

Created attachment 268198 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 268192 [details]
Patch

Attachment 268192 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/648887

New failing tests:
inspector/css/generate-css-rule-string.html
inspector/css/stylesheet-events-inspector-stylesheet.html
inspector/dom/setOuterHTML.html
inspector/css/stylesheet-events-basic.html
inspector/console/command-line-api.html
inspector/model/stack-trace.html
inspector/css/stylesheet-events-multiple-documents.html
inspector/console/messageRepeatCountUpdated.html
inspector/network/client-blocked-load.html

[Build Bot]

Created attachment 268199 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[GSkachkov]

Created attachment 268326 [details]
Patch

Fix WebKit tests#2

[Saam Barati]

Comment on attachment 268326 [details]
Patch

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

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:83
> +            if (m_codeBlock->isStrictMode())
> +                emitTDZCheck(argumentsRegister);

why would we ever need a TDZ check here?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4123
> +RegisterID* BytecodeGenerator::emitPutArgumentsToArrowFunctionContextBlockScope()

I see what you're going for here in this function, but this seems a bit hacky to me.
I think there might be an alternate solution that might be cleaner:

We currently don't allow functions to declare the variable named "arguments"
as being captured (you can see this in the "collectFreeVariables" function inside Parser.h).
But, in the case of an arrow function, we really do want "arguments" to act like a captured variable.
If we teach the parser we can really capture "arguments", then we could probably do away
with a lot of this specialized code. We wouldn't need this function (because "arguments" would already
be in whatever environment is represented by scopeRegister()). We probably wouldn't need a specialized
argumentsLocalPrivateName either. We would need some BytecodeGenerator code just to teach it
that we don't have a local "arguments" that we created, but that's about it. I think most of this change
could be a parser change.

You would basically have to teach the parser that any arrow function implicitly captures the name "arguments".
Or we could be smart about it, and only capture "arguments" if we use "arguments" or "eval" inside the
arrow function.

[GSkachkov]

Created attachment 269291 [details]
Patch

Fix comments

[GSkachkov]

Created attachment 269298 [details]
Patch

Remove redundant code

[GSkachkov]

Comment on attachment 268326 [details]
Patch

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

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4123
>> +RegisterID* BytecodeGenerator::emitPutArgumentsToArrowFunctionContextBlockScope()
> 
> I see what you're going for here in this function, but this seems a bit hacky to me.
> I think there might be an alternate solution that might be cleaner:
> 
> We currently don't allow functions to declare the variable named "arguments"
> as being captured (you can see this in the "collectFreeVariables" function inside Parser.h).
> But, in the case of an arrow function, we really do want "arguments" to act like a captured variable.
> If we teach the parser we can really capture "arguments", then we could probably do away
> with a lot of this specialized code. We wouldn't need this function (because "arguments" would already
> be in whatever environment is represented by scopeRegister()). We probably wouldn't need a specialized
> argumentsLocalPrivateName either. We would need some BytecodeGenerator code just to teach it
> that we don't have a local "arguments" that we created, but that's about it. I think most of this change
> could be a parser change.
> 
> You would basically have to teach the parser that any arrow function implicitly captures the name "arguments".
> Or we could be smart about it, and only capture "arguments" if we use "arguments" or "eval" inside the
> arrow function.

Fixed. Oh I did not know about this collectFreeVariables. This approach is much better because allow to decrease amount of code

[Saam Barati]

Comment on attachment 269298 [details]
Patch

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

r=me with one suggestion and a few comments:
Can you add a test to make sure it works with generators:
function* foo(a, b, c) {
    yield () => arguments;
}
foo(10, 20).value[0] === 10
foo(10, 20).value[1] === 20

> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-arguments-strict.js:32
> +    // https://bugs.webkit.org/show_bug.cgi?id=152570

why does it lead to a reference error?

> Source/WebInspectorUI/UserInterface/Base/Object.js:63
> -        let wrappedCallback = () => {
> -            this.removeEventListener(eventType, wrappedCallback, null);
> +        let that = this;
> +        let wrappedCallback = function () {
> +            that.removeEventListener(eventType, wrappedCallback, null);

why is this necessary?

[GSkachkov]

Created attachment 269693 [details]
Patch

Fix comments

[GSkachkov]

Comment on attachment 269298 [details]
Patch

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

Also I've added test of Generator

>> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-arguments-strict.js:32
>> +    // https://bugs.webkit.org/show_bug.cgi?id=152570
> 
> why does it lead to a reference error?

Now I understand that it is wrong place for this test. I've removed it. The idea is that Global scope does not contain variable 'arguments', and access to the 'arguments' from global scope should lead to ReferenceError. Current test is executed in JSC app where arguments is empty array.

>> Source/WebInspectorUI/UserInterface/Base/Object.js:63
>> +            that.removeEventListener(eventType, wrappedCallback, null);
> 
> why is this necessary?

See line below. Without this change, but with current patch - arguments will start contain values of singleFireEventListener instead of values that is passed during wrappedCallback invocation.

[Saam Barati]

Comment on attachment 269693 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Base/Object.js:61
> +        let that = this;

It's inspector style to use .bind(function, this).

[GSkachkov]

Created attachment 269753 [details]
Patch

Fix comments

[Joseph Pecoraro]

Comment on attachment 269753 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Base/Object.js:61
> -        let wrappedCallback = () => {
> +        let wrappedCallback = function () {

Style Nit: "function ()" => "function()"

Good find!

[GSkachkov]

Created attachment 269784 [details]
Patch

Fix style comments[D

[GSkachkov]

Comment on attachment 269753 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Base/Object.js:61
>> +        let wrappedCallback = function () {
> 
> Style Nit: "function ()" => "function()"
> 
> Good find!

Fixed

[WebKit Commit Bot]

Comment on attachment 269784 [details]
Patch

Clearing flags on attachment: 269784

Committed r195581: <http://trac.webkit.org/changeset/195581>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.