Implement Promise.prototype.finally Fixes #3520 #4668

rhuanjl · 2018-02-11T01:11:37Z

This PR implements Promise.prototype.finally.

Fixes #3520
Relevant ECMASpec: https://tc39.github.io/ecma262/#sec-promise.prototype.finally

Notes:

I worked out how to do this by reading the code for Promise.prototype.catch and the internal AsyncSpawnExecutorFunction - there is a chance that some of what I've done isn't quite right though it does all seem to work.
I've run this against the relevant testcases from test262 and it passes them.
It probably could do with a few more CI tests - I added what seemed good to me based on what was there for other promise methods.
The large size of the diff is as I had to regenerate the bytecode for built in methods after adding the entry point for finally to JnDirectFields.h

cc: @dilijev @ljharb

Thanks to my friend @fatcerberus for running the RegenerateBytecode script for me.

ljharb

I'm not qualified to review 99% of this PR, but i had this one comment :-)

ljharb · 2018-02-11T03:05:48Z

test/es6/ES6Promise.js

+            assert.isFalse(descriptor.enumerable, "Promise.prototype.finally.enumerable === false");
+            assert.isTrue(descriptor.configurable, "Promise.prototype.finally.configurable === true");
+            assert.areEqual('function', typeof descriptor.value, "typeof Promise.prototype.finally === 'function'");
+            assert.areEqual(1, Promise.prototype.finally.length, "Promise.prototype.finally.length === 1");


can you also add an assert that the .name is "finally"?

Done - and added equivalents for .then and .catch as they didn't exist either.

MSLaguana · 2018-02-11T04:30:13Z

lib/Runtime/Library/JavascriptPromise.cpp

+
+        JavascriptLibrary* library = scriptContext->GetLibrary();
+
+        Assert(JavascriptPromiseThenFinallyFunction::Is(function));


I believe that for some of these asserts (this one in particular) you should use AssertOrFailFast. The difference being, Assert is debug build only, while AssertOrFailFast still happens in release, and if somehow the function was the wrong kind of object, we don't want to accidentally do the wrong thing.

Actually, since you do that in the JavascriptPromiseThenFinallyFunction::FromVar method, you should just call that I think.

Out of curiosity (and for future reference on my part 😄), what is the criteria for whether something should be a regular assert vs. a FailFast assert? CC codebase has both kinds.

If it's something that could lead to type confusion / executing wrong code, it should typically be a failfast to avoid security issues. If it is something that shouldn't happen, and we'd like to know in tests when it happens so we can fix/understand it, but is recoverable then a plain assert is fine.

My thinking was that there was no existing code path to get there without that Assert being true and so no need to add any extra cost at runtime and future regressions could be detected by the Assert before making it into any releases. I've made the change though.

MSLaguana · 2018-02-11T04:44:24Z

lib/Runtime/Library/JavascriptPromise.cpp

+
+
+        //8. Return ? Invoke(promise, "then", <<valueThink>>)
+        JavascriptPromise* promise = UnsafeFromVar(promiseVar);


Why do you convert from Var to JavascriptPromise here? It looks to me like you just access properties on the object, none of the native methods of JavascriptPromise.

This was a mistake - I rewrote the logic in this function several times and this was a holdover from an earlier version, fixed.

fatcerberus

I'll give this a more thorough review (relatively) soon but for now just a small typo I noticed.

fatcerberus · 2018-02-11T05:14:58Z

lib/Runtime/Library/JavascriptPromise.cpp

+        JavascriptPromiseThunkFinallyFunction* Thunk = library->CreatePromiseThunkFinallyFunction(EntryThunkFinallyFunction, args[1], This->GetShouldThrow());
+
+
+        //8. Return ? Invoke(promise, "then", <<valueThink>>)


Typo in comment: Should be valueThunk, not "think"

boingoing

Looks good, just a few comments. We have to fix the one where we use args[1] without checking the arg count but others are mostly nitpicks.

boingoing · 2018-02-12T19:07:15Z

lib/Runtime/Library/JavascriptPromise.cpp

+
+        //5. If IsCallable(onFinally) is false
+        //a. Let thenFinally be onFinally
+        //b. let catchFinally be onFinally 


nitpick: 'let' here is not capitalized and the a and b items are not indented.

boingoing · 2018-02-12T19:10:18Z

lib/Runtime/Library/JavascriptPromise.cpp

+        //  d. Set thenFinally and catchFinally's [[OnFinally]] internal slots to onFinally.
+
+        Var thenFinally;
+        Var catchFinally;


I'd like if these were initialized to nullptr and have an assert after the conditional that each is not nullptr anymore. Should cause no overhead in release build.

boingoing · 2018-02-12T19:17:35Z

lib/Runtime/Library/JavascriptPromise.cpp

+
+        JavascriptLibrary* library = scriptContext->GetLibrary();
+
+        JavascriptPromiseThenFinallyFunction* This = JavascriptPromiseThenFinallyFunction::FromVar(function);


I'm not a big fan of naming this JavascriptPromiseThenFinallyFunction* as This. Can we call it finallyFunction or thenFinallyFunction or something?

OK changed (sorry Javascript is my native language was trying to make it look like this.property)

No worries, we just don't usually use this naming convention in our codebase. 👍

boingoing · 2018-02-12T19:17:48Z

lib/Runtime/Library/JavascriptPromise.cpp

+        Var result = CALL_FUNCTION(scriptContext->GetThreadContext(), This->GetOnFinally(), CallInfo(CallFlags_Value, 1), library->GetUndefined());
+
+        //4. Let C be F.[[Constructor]]
+        //5.Assert IsConstructor(C)


nitpick: A space here

boingoing · 2018-02-12T19:20:56Z

lib/Runtime/Library/JavascriptPromise.cpp

+
+        //7. Let valueThunk be equivalent to a function that returns value
+        //OR 7. Let thrower be equivalent to a function that throws reason
+        JavascriptPromiseThunkFinallyFunction* Thunk = library->CreatePromiseThunkFinallyFunction(EntryThunkFinallyFunction, args[1], This->GetShouldThrow());


Same naming nitpick as above. Can we not use Thunk here but something like thunkFinallyFunction?

boingoing · 2018-02-12T19:22:06Z

lib/Runtime/Library/JavascriptPromise.cpp

+        JavascriptPromiseThunkFinallyFunction* Thunk = library->CreatePromiseThunkFinallyFunction(EntryThunkFinallyFunction, args[1], This->GetShouldThrow());
+
+        //8. Return ? Invoke(promise, "then", <<valueThunk>>)
+        RecyclableObject* promise = RecyclableObject::UnsafeFromVar(promiseVar);


Are we sure UnsafeFromVar is ok here? Maybe regular FromVar is better unless CreateResolvedPromise has a guarantee.

I think this is fine - CreateResolvedPromise uses a FromVar on the constructor to check it's safe then uses it to generate the promise. For this promise to be invalid therefore without hitting the FailFast in CreateResolvedPromise would require CreateResolvedPromise to be broken. - Not changed - will change if you still want me to though.

The FromVar in CreateResolvedPromise is done to the constructor object, though. Here we are converting the promise object which was returned from CreateResolvedPromise which could be any Var, couldn't it? It's just whatever was returned from the constructor when we called it in CreatePromiseCapabilityRecord. Since we're calling as-constructor, the return should be an object, I guess. I do wonder if it would be possible to get it to be a non-object. Seems might be possible. I think we should be cautious and follow the Invoke rabbit-hole down to 7.3.2 GetV(V,P) (https://tc39.github.io/ecma262/#sec-getv) where we should do ToObject on promise. We do something very similar in EntryAll on a newly constructed promise Var.

OK I've put in an AssertOrFailFast for it being an object, I think that should do the job? If not, not quite sure what you wanted.

@rhuanjl I think he just wanted you to change the UnsafeFromVar to a FromVar.

Sorry for the confusion - I said a lot of words.

When I saw this code at first I wondered if UnsafeFromVar would be actually safe because promiseVar is a Var and it isn't clear that it always must be a RecyclableObject. That's why I thought FromVar would be safer because it will check at runtime.

After taking a look further, I believe it may be possible for CreateResolvedPromise to return a non-object because the constructor may be overridden via Symbol.species. If that's the case, we should actually throw a TypeError while doing Invoke according to ECMA. Invoke calls GetV which performs ToObject on V. Does that make sense? 🤔

I believe the conversion should look like this:

RecyclableObject* promise = JavascriptOperators::ToObject(promiseVar, scriptContext);

Yep that makes sense - well that aspect of the spec does not seem sensible at all but ah well. Will re-push after re-building and re-running tests.

Many aspects of the spec do not seem sensible. Thanks for your efforts. 😄

boingoing · 2018-02-12T19:24:58Z

lib/Runtime/Library/JavascriptPromise.cpp

+
+        //7. Let valueThunk be equivalent to a function that returns value
+        //OR 7. Let thrower be equivalent to a function that throws reason
+        JavascriptPromiseThunkFinallyFunction* Thunk = library->CreatePromiseThunkFinallyFunction(EntryThunkFinallyFunction, args[1], This->GetShouldThrow());


We need to make sure args[1] exists. This should check args.Info.Count and pass undefined if the then finally function was called without value.

Oops, I'd only been thinking of the normal usage pattern - forgot you could call this with no argument by overwriting .then(); Fixed.

boingoing · 2018-02-12T19:26:55Z

lib/Runtime/Library/JavascriptPromise.cpp

+        RecyclableObject* undefinedVar = scriptContext->GetLibrary()->GetUndefined();
+
+        return CALL_FUNCTION(scriptContext->GetThreadContext(),
+            func, Js::CallInfo(CallFlags_Value, 3),


Looks like we should just be passing 2 arguments here - this and valueThunk, right? I think you can remove undefinedVar and change arg count to 2.

Oops, fixed.

boingoing · 2018-02-12T19:28:09Z

lib/Runtime/Library/JavascriptPromise.cpp

+        ARGUMENTS(args, callInfo);
+        Assert(!(callInfo.Flags & CallFlags_New));
+
+        JavascriptPromiseThunkFinallyFunction* This = JavascriptPromiseThunkFinallyFunction::FromVar(function);


One more nitpick about This, can we name it something like thunkFinallyFunction?

boingoing · 2018-02-12T19:32:28Z

lib/Runtime/Library/JavascriptLibrary.cpp

+        Assert(scriptContext->GetConfig()->IsES6PromiseEnabled());
+
+        FunctionInfo* functionInfo = RecyclerNew(this->GetRecycler(), FunctionInfo, entryPoint);
+        DynamicType* type = CreateDeferredPrototypeFunctionType(entryPoint);


Can you use GetDeferredAnonymousFunctionTypeHandler() to avoid having to set the name property here?

How did I not notice that function... I spent 10-15 minutes searching JavascriptLibrary.cpp for a way to do that but didn't find it. Changed.

Note, this comment has moved down to the other CreateDeferredPrototypeFunctionType call not where you originally placed it.

rhuanjl · 2018-02-12T20:44:38Z

@boingoing thanks for the review - I've made the changes and re-pushed. I've also added an extra test case to confirm that thenFinally handles the situation of being called without an argument correctly (and supplies the undefined value).

Will update and squash later on assuming you're happy with my updates.

Edit: oops my new test case is wrong will update shortly

rhuanjl · 2018-02-12T23:43:03Z

@boingoing Thanks again for the review I've fixed all of your points now, the commit "Responding to feedback" contains all responses to your points. Please let me know when good to squash.

boingoing · 2018-02-12T23:46:32Z

@rhuanjl Looks good to me. Thanks a lot for responding to those feedback and sorry for being a bit pedantic about it.

If you squash these commits, I will begin the process of testing internally and shepherd the change through our checkin process.

rhuanjl · 2018-02-12T23:55:43Z

Thanks @boingoing squashed and re-pushed. Did @dilijev want to review as well?

dilijev · 2018-02-13T00:14:04Z

Taking a look.

kfarnung · 2018-02-13T00:35:14Z

lib/Runtime/Library/JavascriptPromise.cpp

@@ -559,6 +559,159 @@ namespace Js
        return CreateThenPromise(promise, fulfillmentHandler, rejectionHandler, scriptContext);
    }

+    // Promise.prototype.finally as described in the draft of ES 2018 Section 25.4.5.3


nit: ECMA section numbers aren't immutable, but the anchors are considered to be. Use #sec-promise.prototype.finally instead of a section number.

I was copying what was already in the file on this one - most of the Promise methods have comments referring to section numbers per ES2015 (which are also mostly out of date now) if I'm going to fix this point assume I should fix all of them at the same time?

Might as well fix this one now and follow up with the others (this guidance about using anchors instead of section numbers is relatively new so I'm not surprised there are still a lot of section numbers throughout the code).

In reply to: 167731238 [](ancestors = 167731238)

If we're going to update the other section numbers, I'd like to make sure we also do an audit to make sure no behavior in the functions has changed between spec versions. Then we can be sure the implementation holds to the new spec version and update the spec reference at the same time. So, probably best to follow-up on the other methods.

dilijev · 2018-02-13T00:42:08Z

Looks like we need tests of behavior (besides throwing behavior) for .finally(<non-callable>)

dilijev · 2018-02-13T00:56:49Z

lib/Runtime/Library/JavascriptPromise.cpp

+        }
+
+        JavascriptLibrary* library = scriptContext->GetLibrary();
+        RecyclableObject* promise = RecyclableObject::UnsafeFromVar(args[0]);


UnsafeFromVar(args[0]); [](start = 54, length = 23)

Is it okay to call UnsafeFromVar(args[0]) here? -- do we know for sure this has to be a RecyclableObject at this point? It is possible to call with this as, say, a number?

Or, because the check above covers that it is an object -- is it necessarily a RecyclableObject or could it be something else (I'm not sure the answer to this question)

In reply to: 167733902 [](ancestors = 167733902)

We did a check above to make sure we have an args[0] and that it is an object. Should be guaranteed here.

There's a type error thrown a couple of lines above if the arg is not an Object.

I'll also add a new test case that confirms that a type error is thrown if this is called with any non-promise object.

dilijev · 2018-02-15T01:32:24Z

@rhuanjl looks like there's a conflict with the latest version of bytecode. You'll want to merge with master and regenerate bytecode before the final squash.

fatcerberus · 2018-02-15T06:46:43Z

I wonder if there should be a test for what happens if a promise has been rejected and the resulting .finally() continuation throws. According to ES spec (unless I misread it) in this case the chained promise should reject with whatever you threw instead of the original rejection reason.

fatcerberus

LGTM, save for a nitpick re: comment style and a concern about tests (see my comment above).

fatcerberus · 2018-02-15T06:51:53Z

lib/Runtime/Library/JavascriptPromise.cpp

+
+        JavascriptPromiseThunkFinallyFunction* thunkFinallyFunction = library->CreatePromiseThunkFinallyFunction(EntryThunkFinallyFunction, valueOrReason, thenFinallyFunction->GetShouldThrow());
+
+        //8. Return ? Invoke(promise, "then", <<valueThunk>>)


Nit: Prevailing convention seems to be to have a space between // and the comment text. In any case comments are easier to read with that space IMO.

ljharb · 2018-02-15T06:56:00Z

@fatcerberus your interpretation of the spec is correct.

rhuanjl · 2018-02-15T18:24:18Z

@boingoing I think this is good to go now?

I've updated to latest and addressed all review points - I've also added extra test cases for the case @fatcerberus and @ljharb mentioned, hopefully not controversial to do that - they're the last two cases in ES6PromiseAsync.js

boingoing · 2018-02-15T18:36:11Z

@rhuanjl Yes, looks good to me. I will take a final look over the code and pull the branch down to do internal testing before landing. Thank you for your patience.

fatcerberus · 2018-02-15T18:45:42Z

lib/Runtime/Library/JavascriptPromise.cpp

+        RecyclableObject* promise = RecyclableObject::UnsafeFromVar(args[0]);
+        //3. Let C be ? SpeciesConstructor(promise, %Promise%).
+        RecyclableObject* constructor = JavascriptOperators::SpeciesConstructor(promise, scriptContext->GetLibrary()->GetPromiseConstructor(), scriptContext);
+        //4. Assert IsConstructor(C)


nit: space after //

fatcerberus · 2018-02-15T18:46:08Z

lib/Runtime/Library/JavascriptPromise.cpp

+
+        JavascriptLibrary* library = scriptContext->GetLibrary();
+        RecyclableObject* promise = RecyclableObject::UnsafeFromVar(args[0]);
+        //3. Let C be ? SpeciesConstructor(promise, %Promise%).


nit: space after //

fatcerberus · 2018-02-15T18:46:24Z

lib/Runtime/Library/JavascriptPromise.cpp

+
+        // 5. If IsCallable(onFinally) is false
+        //   a. Let thenFinally be onFinally
+        //  b. Let catchFinally be onFinally 


nit: missing space

fatcerberus · 2018-02-15T18:46:45Z

lib/Runtime/Library/JavascriptPromise.cpp

+
+        Var valueOrReason = nullptr;
+
+        if(args.Info.Count > 1)


nit: missing space after if

rhuanjl · 2018-02-15T19:46:55Z

@boingoing I've added some spaces I was missing that @fatcerberus noticed - I'll hold on squashing until you've done your final review.

boingoing · 2018-02-15T19:52:34Z

@rhuanjl I added a commit to update the bytecode cache GUID. We need this for internal processes. Otherwise we are good to go from my perspective. Please squash it and I'll run the internal testing once more. 👍

rhuanjl · 2018-02-15T19:57:40Z

@boingoing ok squashed, thank you and sorry that this has taken a lot of your time.

@dilijev

Merge pull request #4668 from rhuanjl:experiment This PR implements Promise.prototype.finally. Fixes #3520 Relevant ECMASpec: https://tc39.github.io/ecma262/#sec-promise.prototype.finally **Notes:** 1. I worked out how to do this by reading the code for Promise.prototype.catch and the internal AsyncSpawnExecutorFunction - there is a chance that some of what I've done isn't quite right though it does all seem to work. 2. I've run this against the relevant testcases from test262 and it passes them. 3. It probably could do with a few more CI tests - I added what seemed good to me based on what was there for other promise methods. 4. The large size of the diff is as I had to regenerate the bytecode for built in methods after adding the entry point for finally to JnDirectFields.h **cc:** @dilijev @ljharb Thanks to my friend @fatcerberus for running the RegenerateBytecode script for me.

boingoing · 2018-02-15T22:04:17Z

@rhuanjl Thanks for your contribution, this is merged! No worries about my time, by the way, you've saved us a lot of time by implementing it in the first place. 👍

rhuanjl · 2018-02-15T22:34:51Z

Ok, thanks for landing this @boingoing and thanks to everyone else for the reviews.

Also thanks again @fatcerberus for regenerating the bytecode for me (couldn't see a way to do that on macOS, and have never learnt how to do anything on Windows other than use Excel)

ljharb · 2018-02-16T03:50:44Z

Yay! Any idea when to expect it in Edge?

dilijev · 2018-02-16T03:53:27Z

@rhuanjl the byte code regenerate script really just calls ch with some parameters. In theory it should work on xplat although you'd have to rewrite the logic in bash or something. This is something quick we should probably add.

dilijev · 2018-02-16T03:55:28Z

@ljharb hard to say exactly, but think it will be in insiders flights in a few months -- general availability whenever the following version of windows is released.

rhuanjl · 2018-02-16T06:55:43Z

@dilijev I think a key hurdle would be that the current byte code regen method requires a 32bit binary and a 64bit binary - whereas at the moment ChakraCore on xplat is 64 bit only. #4091

Refs: chakra-core/ChakraCore#4668

Refs: chakra-core/ChakraCore#4668 Fixes: #464 PR-URL: #568

Refs: chakra-core/ChakraCore#4668 Fixes: #464 PR-URL: #568 Reviewed-By: Jimmy Thomson <jithomso@microsoft.com>

Refs: chakra-core/ChakraCore#4668 Fixes: nodejs#464 PR-URL: nodejs#568 Reviewed-By: Jimmy Thomson <jithomso@microsoft.com>

rhuanjl force-pushed the experiment branch 4 times, most recently from fe5200c to 1430bc2 Compare February 11, 2018 02:49

ljharb reviewed Feb 11, 2018

View reviewed changes

ljharb mentioned this pull request Feb 11, 2018

Path to Stage 4! tc39/proposal-promise-finally#18

Closed

25 tasks

MSLaguana reviewed Feb 11, 2018

View reviewed changes

fatcerberus suggested changes Feb 11, 2018

View reviewed changes

rhuanjl force-pushed the experiment branch 4 times, most recently from 960a7e8 to b3f2360 Compare February 11, 2018 16:18

dilijev added the External Contributor label Feb 12, 2018

boingoing suggested changes Feb 12, 2018

View reviewed changes

sethbrenith approved these changes Feb 12, 2018

View reviewed changes

dilijev self-requested a review February 12, 2018 21:34

rhuanjl force-pushed the experiment branch from d1aa9ac to fdccad8 Compare February 12, 2018 23:14

MSLaguana mentioned this pull request Feb 12, 2018

fs.promises functions use promise.prototype.finally which is not supported in chakracore nodejs/node-chakracore#464

Closed

boingoing approved these changes Feb 12, 2018

View reviewed changes

rhuanjl force-pushed the experiment branch from fdccad8 to c8a61ee Compare February 12, 2018 23:53

kfarnung assigned boingoing Feb 13, 2018

kfarnung reviewed Feb 13, 2018

View reviewed changes

dilijev reviewed Feb 13, 2018

View reviewed changes

fatcerberus approved these changes Feb 15, 2018

View reviewed changes

rhuanjl force-pushed the experiment branch from 55eba68 to 4cbf91a Compare February 15, 2018 17:57

fatcerberus suggested changes Feb 15, 2018

View reviewed changes

Promise.prototype.finally

b8a38ed

rhuanjl force-pushed the experiment branch from 7a2e062 to b8a38ed Compare February 15, 2018 19:56

chakrabot merged commit b8a38ed into chakra-core:master Feb 15, 2018

rhuanjl deleted the experiment branch February 15, 2018 21:58

lll000111 mentioned this pull request Feb 18, 2018

Add support for Promise's .finally() method. facebook/flow#5810

Closed

kfarnung added a commit to nodejs/node-chakracore that referenced this pull request Jul 10, 2018

test: enable Promise.finally tests

1ec4f4c

Refs: chakra-core/ChakraCore#4668

kfarnung added a commit to nodejs/node-chakracore that referenced this pull request Jul 10, 2018

test: enable Promise.finally tests

a77f1fb

Refs: chakra-core/ChakraCore#4668

kfarnung added a commit to nodejs/node-chakracore that referenced this pull request Jul 11, 2018

test: enable Promise.finally tests

0016bb5

Refs: chakra-core/ChakraCore#4668

kfarnung added a commit to nodejs/node-chakracore that referenced this pull request Jul 11, 2018

test: enable Promise.finally tests

0582bd8

Refs: chakra-core/ChakraCore#4668 Fixes: #464 PR-URL: #568

kfarnung added a commit to nodejs/node-chakracore that referenced this pull request Jul 11, 2018

test: enable Promise.finally tests

ceb2129

Refs: chakra-core/ChakraCore#4668 Fixes: #464 PR-URL: #568

kfarnung added a commit to nodejs/node-chakracore that referenced this pull request Jul 11, 2018

test: enable Promise.finally tests

601810d

Refs: chakra-core/ChakraCore#4668 Fixes: #464 PR-URL: #568 Reviewed-By: Jimmy Thomson <jithomso@microsoft.com>

kfarnung added a commit to kfarnung/node-chakracore that referenced this pull request Jul 12, 2018

test: enable Promise.finally tests

44c7450

Refs: chakra-core/ChakraCore#4668 Fixes: nodejs#464 PR-URL: nodejs#568 Reviewed-By: Jimmy Thomson <jithomso@microsoft.com>


		JavascriptLibrary* library = scriptContext->GetLibrary();

		Assert(JavascriptPromiseThenFinallyFunction::Is(function));



		//8. Return ? Invoke(promise, "then", <<valueThink>>)
		JavascriptPromise* promise = UnsafeFromVar(promiseVar);

		JavascriptPromiseThunkFinallyFunction* Thunk = library->CreatePromiseThunkFinallyFunction(EntryThunkFinallyFunction, args[1], This->GetShouldThrow());


		//8. Return ? Invoke(promise, "then", <<valueThink>>)


		JavascriptLibrary* library = scriptContext->GetLibrary();

		JavascriptPromiseThenFinallyFunction* This = JavascriptPromiseThenFinallyFunction::FromVar(function);

Implement Promise.prototype.finally Fixes #3520 #4668

Implement Promise.prototype.finally Fixes #3520 #4668

Conversation

rhuanjl commented Feb 11, 2018 • edited by dilijev Loading

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhuanjl Feb 11, 2018 • edited Loading

Choose a reason for hiding this comment

fatcerberus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boingoing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhuanjl Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boingoing Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhuanjl commented Feb 12, 2018 • edited Loading

rhuanjl commented Feb 12, 2018 • edited Loading

boingoing commented Feb 12, 2018

rhuanjl commented Feb 12, 2018 • edited Loading

dilijev commented Feb 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dilijev Feb 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dilijev commented Feb 13, 2018

dilijev Feb 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dilijev commented Feb 15, 2018

fatcerberus commented Feb 15, 2018

fatcerberus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb commented Feb 15, 2018

rhuanjl commented Feb 15, 2018 • edited Loading

boingoing commented Feb 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhuanjl commented Feb 15, 2018

rhuanjl commented Feb 11, 2018 •

edited by dilijev

Loading

rhuanjl Feb 11, 2018 •

edited

Loading

rhuanjl Feb 12, 2018 •

edited

Loading

boingoing Feb 12, 2018 •

edited

Loading

rhuanjl commented Feb 12, 2018 •

edited

Loading

rhuanjl commented Feb 12, 2018 •

edited

Loading

rhuanjl commented Feb 12, 2018 •

edited

Loading

dilijev Feb 13, 2018 •

edited

Loading

dilijev Feb 13, 2018 •

edited

Loading

rhuanjl commented Feb 15, 2018 •

edited

Loading

rhuanjl commented Feb 15, 2018 •

edited

Loading