Adds sendWithStream method in MessageHandler. #8430

mukulmishra18 · 2017-05-19T20:25:16Z

This is second PR for the project Streams API in PDF.js.

This PR adds a method sendWithStream in MessageHandler at shared/utils. We can use this method to pass data between main and worker thread using Streams API. This method returns a ReadableStream.
The behaviour of stream is just like the standard ReadableStream Constructor.

This PR also contains tests for the functionalities of stream underlyingSource(pull, cancel) and streamSink(read data with blocking promise, error).

Snuffleupagus

I've left a couple of comments, based on a very cursory look at the code.

Also, I'm wondering if there's any reason that the new unit-tests cannot be run on Travis?
Otherwise, let's add the new test file to https://github.com/mozilla/pdf.js/blob/master/test/unit/clitests.json.

Snuffleupagus · 2017-05-20T07:44:17Z

src/shared/util.js

@@ -1219,16 +1219,111 @@ function MessageHandler(sourceName, targetName, comObj) {
  this.targetName = targetName;
  this.comObj = comObj;
  this.callbackIndex = 1;
+  this.streamId = 1;


Just above there's callbackIndex, so can we please use the same same suffix for both callback and stream to reduce confusion here!?

Yes, sure. I will change it to streamIndex

Yes, sure. I will change it to streamIndex

Thanks, but I now notice that this may become problematic with respect to the line length limit; it may be simpler to keep this.streamId and just change this.callbackIndex -> this.callbackId instead :-)

Looking at the existing code, the callbackId name is actually already used in most places, except here: https://github.com/mozilla/pdf.js/blob/master/src/shared/util.js#L1313.

but I now notice that this may become problematic with respect to the line length limit

"line length limit"? I think i am not getting this point, can you please elaborate?

I just noticed that there's a couple of fairly long lines in the patch already (for example this one), and it's possible that using streamIndex instead of streamId everywhere could perhaps unnecessarily go over the maximum limit of 80 characters per line (as enforced by ESLint).

However, it seems that we already use callbackId in most places in the existing code, hence I'm suggesting to keep this.streamId in this patch and just change the two existing occurrences of this.callbackIndex to this.callbackId instead.

Sorry about giving confusing/conflicting advice here!

I'm suggesting to keep this.streamId in this patch and just change the two existing occurrences of this.callbackIndex to this.callbackId instead.

Okay, sure. Thanks for clarification here.

Snuffleupagus · 2017-05-20T07:51:51Z

src/shared/util.js

+      let streamId = data.streamId;
+      switch (data.stream) {
+        case 'start_complete':
+          if (data.success === true || data.success === 'true') {


This looks slightly awkward to me; and why would/should success ever be anything other than a boolean?
Looking below at the postMessage calls, success is always a boolean, so please simplify this to if (data.success) { instead.

Please note: This comment applies in multiple places below as well.

Just to be at safer side, i thought maybe somehow while sending data between main and worker, success will change to string. But i am not sure.
Thanks for your suggestion, i will change this to if (data.success) {. This looks more clear.

Just to be at safer side, i thought maybe somehow while sending data between main and worker, success will change to string. But i am not sure.

That shouldn't be a problem, since the structured cloning algorithm maintains types (and throws on unsupported types).

If you want to check for yourself, look at the value/type of data.isReply in the current code.

That shouldn't be a problem, since the structured cloning algorithm maintains types (and throws on unsupported types).

Okay right, we are using structured cloning algorithm in postMessage.
Thanks for clarification.

Snuffleupagus · 2017-05-20T07:58:40Z

src/shared/util.js

+              data.desiredSize > 0) {
+            streamSinks[data.streamId].sinkCapability.resolve();
+          }
+          // reset desiredSize property of sink on every pull.


Nit: For improved readability of comments, please remember to use proper capitalization at the start of sentences.

Please note: This probably applies elsewhere in the patch as well.

Yeah, sure. Thanks.

Snuffleupagus · 2017-05-20T08:01:19Z

src/shared/util.js

+          break;
+        case 'pull':
+          // pull increases the desiredSize property of sink, so when it
+          // changes from -ve to +ve set ready property as resolved promise.


Unless one already knows what ve means, then this comment is unfortunately not very helpful. (This applies to another comment below as well.)

Okay, i will make it negative/positive instead.

Snuffleupagus · 2017-05-20T08:06:22Z

src/shared/util.js

+        let streamSink = {
+          enqueue(chunk, size) {
+            var lastDesiredSize = this.desiredSize;
+            this.desiredSize -= size !== undefined ? size : 1;


Why not change the signature to enqueue(chunk, size = 1) { instead, since this line can then be simplified to this.desiredSize -= size?

Great, this is short and more clear. We will do this way.

Snuffleupagus · 2017-05-20T08:16:20Z

src/shared/util.js

+   * @param {Object} queueingStrategy strategy to signal backpressure based on
+   *                 internal queue.
+   * @param {Array} [transfers] Optional list of transfers/ArrayBuffers.
+   * @return {ReadableStream} ReadableStream to read data in chunks.


Nit: Let's make this more consistent with how we generally write JSDoc comments (note that the other JSDoc comments in MessageHandler aren't entirely correct):

* @param {string} actionName - Action to call. * @param {JSON} data - JSON data to send. * @param {Object} queueingStrategy - strategy to signal backpressure based on * internal queue. * @param {Array} [transfers] - Optional list of transfers/ArrayBuffers. * @return {ReadableStream} ReadableStream to read data in chunks.

note that the other JSDoc comments in MessageHandler aren't entirely correct

I referred those comments to write this :)
Okay, i will do as you suggested, and also change other comments in MessageHandler to follow this.

Snuffleupagus · 2017-05-20T08:20:11Z

src/shared/util.js

+   * @return {ReadableStream} ReadableStream to read data in chunks.
+   */
+  sendWithStream(actionName, data, queueingStrategy, transfers) {
+      var streamId = this.streamId++;


Please reduce the indentation to only two spaces within this method.

Okay, sure.

Snuffleupagus · 2017-05-20T08:22:05Z

test/unit/util_spec.js

@@ -14,52 +14,52 @@
 */

 import {
-  ReadableStream, removeNullCharacters, stringToPDFString
+   ReadableStream, removeNullCharacters, stringToPDFString


Unnecessary change, please revert.

Snuffleupagus · 2017-05-20T08:23:23Z

test/unit/util_stream_spec.js

+
+import { MessageHandler } from '../../src/shared/util';
+
+describe('util stream', function () {


Nit: Please make this util_stream instead, since that's consistent with existing tests.

Snuffleupagus · 2017-05-20T08:23:59Z

test/unit/util_stream_spec.js

+    }
+
+    postMessage(obj) {
+      let e = {data: obj};


Please use a more descriptive variable name than e.

Okay, Thanks for your suggestions.

mukulmishra18 · 2017-05-21T10:09:07Z

Okay, but why it is failing the tests :-(

gulp unittestcli is just running fine on my local machine, with result:
412 specs, 0 failures, 11 pending specs

Snuffleupagus · 2017-05-21T10:22:04Z

It seems that one of the tests fail on Travis, and I've noticed that while all tests pass locally, there still seem to be some errors regarding accessing removed properties.

Note that you can easily run all (or part) of the unit-tests locally in a browser, by executing gulp server and then pointing your browser to http://localhost:8888/test/unit/unit_test.html. It's also possible to run part of the tests as well, using e.g. http://localhost:8888/test/unit/unit_test.html?spec=util_stream.
Doing that locally, I get two TypeErrors in the browser console, as indicated below.

TypeError: streamControllers[data.streamId] is undefined[Learn More] system.js%20line%204%20%3E%20eval:1206:19
TypeError: streamSinks[data.streamId] is undefined[Learn More] system.js%20line%204%20%3E%20eval:1215:1

This suggests that currently there's code trying to access certain properties after a delete ... expression has been run. Those errors need to be dealt with, since it indicates that the ordering of the various stream events isn't the expected one. (Fixing this might also fix the failure on Travis, since that error looks similar.)

mukulmishra18 · 2017-05-21T20:56:57Z

This suggests that currently there's code trying to access certain properties after a delete ... expression has been run.

Yes, you are right. Actually, as desiredSize property of sink in greater than sum of size of all data chunks, so it is not blocking the stream(we are using sink.ready property to block the stream when queue is full). Due to this, it is calling sink.close here, before calling pull. Decreasing the size of desiredSize is solving this problem.

Thanks for your help here :)

Snuffleupagus · 2017-05-22T10:44:06Z

Testing the latest version of the patch locally (on Windows), I get different behaviour in Firefox vs Chrome:

In Firefox Nightly (version 55), there's a Expected '012p' to equal '012' failure in the should read data with blocking promise and buffer whole data into stream test.
In Chrome Beta (version 59), the new unit-tests pass.

This unfortunately suggests that there're issues either in the streams polyfill, the MessageHandler implementation, or the unit-tests; or perhaps some combination of them.
(It might just be a timing issue with the e.g. the sleep function used in the unit-tests, or it might point to a deeper issue with regards to cross-browser compatibility.)

mukulmishra18 · 2017-05-22T15:53:17Z

Testing the latest version of the patch locally (on Windows), I get different behaviour in Firefox vs Chrome:

In Firefox Nightly (version 55), there's a Expected '012p' to equal '012' failure in the should read data with blocking promise and buffer whole data into stream test.

In Chrome Beta (version 59), the new unit-tests pass.

Yes, you are right, i have Chrome(version 52) locally, and it is failing on this also.

But in my opinion, 012p would be the correct value for log, as value of highWaterMark is 8, enqueue of chunk here does not fill the internal queue completely, so it will call the pull to get next chunk of data.

mukulmishra18 · 2017-05-23T08:14:09Z

It might just be a timing issue with the e.g. the sleep function used in the unit-tests, or it might point to a deeper issue with regards to cross-browser compatibility.

I am not sure, but potentially this seems to be a timing problem. Running the test with different timeout value(e.g. sleep(0), or sleep(1)[1]), gives different log values. I also think, this is not a fault of sleep function itself(as instead of sleep, i tried setTimeout function also).

[1] note: sleep(1) and sleep(10) has no difference on log values

yurydelendik

Please also rebase to new master.

yurydelendik · 2017-05-31T14:55:09Z

test/unit/util_stream_spec.js

+        }).then(() => {
+          sink.close();
+        });
+      });


return sleep(5); before exiting handler -- we need to be sure first sink.ready promise is resolved. Few place below needs that too.

yurydelendik · 2017-05-31T15:17:10Z

test/unit/util_stream_spec.js

+        expect(result.value).toEqual([1, 2, 3, 4]);
+        expect(result.done).toEqual(false);
+        return sleep(10);
+      }).then(() => {


add reason param here and use it instead of log for expect below.

Okay sure, done in new commit

Snuffleupagus · 2017-05-31T21:26:00Z

test/unit/util_stream_spec.js

+    });
+
+    it('should ignore any pull after close is called', function (done) {
+      function createPromiseCapability() {


Why are you duplicating this here? Please import it from shared/util.js instead!

Okay, sure. Thanks.

Snuffleupagus · 2017-05-31T21:28:58Z

src/shared/util.js

+      let streamId = data.streamId;
+      let sendStreamResponse = ({ stream, success }) => {
+        comObj.postMessage({ sourceName, targetName, stream,
+        success, streamId });


Nit: The indentation is off here, please alight the parameters like this:

comObj.postMessage({ sourceName, targetName, stream, success, streamId });

Snuffleupagus · 2017-05-31T21:29:40Z

src/shared/util.js

+        let capability = createPromiseCapability();
+        let sendStreamRequest = ({ stream, chunk, reason, success }) => {
+          comObj.postMessage({ sourceName, targetName, stream, streamId,
+            chunk, reason, success });


Similar to https://github.com/mozilla/pdf.js/pull/8430/files#r119478584.

timvandermeij · 2017-05-31T21:36:26Z

I have a remark that is related to, but more general than this pull request. The unit tests are clear and help understand what the Streams API does (because I'm not familiar with it yet myself), but I think it would really help (since this is going to be one of the core components) to have some documentation about the terminology, such as "high watermark", "sink" and "queueing strategy". I think you have a really good reference with your write-up at https://docs.google.com/document/d/1yTPvPBhGkFn7XUntAt8wdVPckxHovYylNAIJOGnGRXc/edit and just extending that should already suffice. This is not something that needs to be done right away, but I just wanted to let you know that I think that it would help a lot with understanding the code.

Other than that, this looks promising. Keep up the nice work!

yurydelendik · 2017-05-31T21:50:06Z

src/shared/util.js

-    if (data.isReply) {
-      var callbackId = data.callbackId;
+    if (data.stream) {
+      let sourceName = this.sourceName;


extract entire if (data.stream) into separate method e.g. '_processStreamMessage(data)'

Okay, done in new commit.

yurydelendik · 2017-05-31T21:50:55Z

src/shared/util.js

@@ -1269,6 +1357,51 @@ function MessageHandler(sourceName, targetName, comObj) {
            error: reason
          });
        });
+      } else if (data.streamId) {


extract entire if (data.streamId) into separate method e.g. '_createStreamSink(data)'

Done, thanks for suggestions.

yurydelendik · 2017-05-31T21:52:44Z

src/shared/util.js

+      let sourceName = this.sourceName;
+      let targetName = data.sourceName;
+      let streamId = data.streamId;
+      let sendStreamResponse = ({ stream, success }) => {


here and at sendStreamRequest add 'reason' parameter after success -- it shall be populated/used when success == false. reject on xxx_complete side shall accept this reason as argument.

yurydelendik · 2017-05-31T21:54:10Z

src/shared/util.js

+                             success, streamId });
+      };
+      let deleteStreamController = () => {
+        Promise.all([streamControllers[data.streamId].startCall,


nit: startCall on the new line

add .cancelCall promise here too

mukulmishra18 · 2017-05-31T21:56:47Z

but I think it would really help (since this is going to be one of the core components) to have some documentation about the terminology, such as "high watermark", "sink" and "queueing strategy".

Yes you are right, I am also thinking about the same. Having documentation about the code and terminology will help to understand it better, even i can understand later what i have done :-D
I will try to update the Streams API for PDF.js proposal as we proceed with the project. I think for documenting the terminology related to Streams API, i can take reference from Steams Spec.

Other than that, this looks promising. Keep up the nice work!

Thank You :)

timvandermeij · 2017-05-31T22:10:35Z

I did not notice the specification yet, but it's surprisingly good to read and understand. It's a really helpful resource (that I see you already mentioned in your proposal) and I think extracting the most important terminology from it is a good idea. Thank you for considering that; I'll leave it for now (to not digress from the topic of this particular PR) and watch how to proposal progresses.

yurydelendik

To improve readability and error handling

yurydelendik · 2017-06-01T13:48:42Z

src/shared/util.js

+  let callbacksCapabilities = this.callbacksCapabilities = Object.create(null);
+  let ah = this.actionHandler = Object.create(null);
+
+  let _processStreamMessage = (data) => {


looks good, but I wonder if we can move _processStreamMessage and _createStreamSink as a method of MessageHandler (we have this.comObj, this.streamSinks and this.streamControllers available to us)

Done in new commit.

I might be totally misunderstanding, but I'm interpreting #8430 (comment) as suggesting that they be moved into MessageHandler.prototype instead (since the updated patch doesn't really change anything otherwise)!?

I might be totally misunderstanding, but I'm interpreting #8430 (comment) as suggesting that they be moved into MessageHandler.prototype instead (since the updated patch doesn't really change anything otherwise)!?

I think i misunderstood #8439 (comment), sorry for that. I will move _processStreamMessage and _createStreamSink to MessageHandler.prototype

I just thought earlier that maybe doing something like: let _createStreamSink = this._createStreamSink = () => {...} will make _createStreamSink a method of MessageHandler.

it will, but from performance point of view this methods without closure (in this case members of the prototype) will be more effective and easier to debug.

yurydelendik · 2017-06-01T13:50:41Z

src/shared/util.js

+          streamControllers[data.streamId].startCall.resolve();
+        } else {
+          streamControllers[data.streamId].startCall.reject();
+        }


add utility function let resolveOrReject = (capability, success, reason) => { if (success) { capability.resolve(); ... and use it at xxxx_complete branches

Can we do this like: let resolveOrReject = ({ capability, success, reason }) => { if (success) { capability.resolve(); ..., as i think we don't have reason for success == true, and for consistency with sendStreamRequest and sendStreamResponse

it's fine as 3 args, you can add undefined for success == true or just don't supply it

yurydelendik · 2017-06-01T13:54:34Z

src/shared/util.js

+        streamSinks[data.streamId].desiredSize = data.desiredSize;
+        let pullResult = streamSinks[data.streamId].onPull !== null ?
+          streamSinks[data.streamId].onPull() : undefined;
+        Promise.resolve(pullResult).then(() => {


add let resolveCall = (fn, args) => { if (!fn) return Promise.resolve(undefined); return new Promise( (resolve, reject) => { resolve(fn.apply(null, args)); }); } and use it in pull, cancel and start cases.

We require to call action[0] with this = action[1] in start case here. So maybe this function shall be let resolveCall = ({ fn, _this, args}) => { if (!fn) return Promise.resolve(undefined); return new Promise( (resolve, reject) => { resolve(fn.apply(_this, args)); }); } instead.

sure, you make make its signature (fn, args, thisArg) and for pull/cancel call use thisArg == null.

yurydelendik · 2017-06-01T13:55:49Z

src/shared/util.js

+        break;
+      case 'error':
+        streamControllers[data.streamId].controller.error(data.reason);
+        break;


call deleteStreamController when error

yurydelendik · 2017-06-01T13:56:27Z

src/shared/util.js

+    Promise.resolve(actionResult).then(() => {
+      sendStreamRequest({ stream: 'start_complete', success: true });
+    }, () => {
+      sendStreamRequest({ stream: 'start_complete', success: false });


add reason here

yurydelendik · 2017-06-01T13:56:52Z

src/shared/util.js

+    let capability = createPromiseCapability();
+    let sendStreamRequest = ({ stream, chunk, success, reason }) => {
+      comObj.postMessage({ sourceName, targetName, stream, streamId,
+                           chunk, reason, success });


nit: switch 'success' and 'reason' places

yurydelendik · 2017-06-01T13:59:01Z

src/shared/util.js

+      // Delete streamController only when start, pull and
+      // cancel callbacks are resolved, to avoid "TypeError".
+      Promise.all([
+        streamControllers[data.streamId].startCall,


add let finalize = (promise) => { return Promise.resolve(promise).catch(() => {}); }; utility function and use it to wrap streamControllers[data.streamId].xxxCall here

I think i am not getting this utility function properly, can you please explain a bit.
We are going to use this function something like:
Promise.all([finalize(streamControllers[data.streamId].startCall), finalize(streamControllers[data.streamId].pullCall), finalize(streamControllers[data.streamId].cancelCall)]).then(() => {...}) ?

Snuffleupagus · 2017-06-02T19:49:51Z

src/shared/util.js

+          this.ready = this.sinkCapability.promise;
+        }
+        sendStreamRequest({ stream: 'enqueue', chunk });
+      },


Nit: Adding a newline here would aid readability of the code.

Snuffleupagus · 2017-06-02T19:50:01Z

src/shared/util.js

+      close() {
+        sendStreamRequest({ stream: 'close' });
+        delete that.streamSinks[streamId];
+      },


Nit: Adding a newline here would aid readability of the code.

Snuffleupagus · 2017-06-02T19:50:05Z

src/shared/util.js

+      },
+      error(reason) {
+        sendStreamRequest({ stream: 'error', reason });
+      },


Nit: Adding a newline here would aid readability of the code.

Snuffleupagus · 2017-06-02T19:50:26Z

src/shared/util.js

+  _processStreamMessage(data) {
+    let sourceName = this.sourceName;
+    let targetName = data.sourceName;
+    let streamId = data.streamId;


Nit: Adding a newline here would aid readability of the code.

Snuffleupagus · 2017-06-02T19:50:59Z

src/shared/util.js

+    let sendStreamResponse = ({ stream, success, reason }) => {
+      this.comObj.postMessage({ sourceName, targetName, stream,
+                                success, streamId, reason });
+    };


Nit: Adding a newline here would aid readability of the code.

Snuffleupagus · 2017-06-02T19:51:04Z

src/shared/util.js

+      } else {
+        capability.reject(reason);
+      }
+    };


Nit: Adding a newline here would aid readability of the code.

Snuffleupagus · 2017-06-02T19:51:08Z

src/shared/util.js

+    };
+    let finalize = (promise) => {
+      return Promise.resolve(promise).catch(() => {});
+    };


Nit: Adding a newline here would aid readability of the code.

Snuffleupagus · 2017-06-02T19:52:05Z

src/shared/util.js

+      this.comObj.postMessage({ sourceName, targetName, stream,
+                                success, streamId, reason });
+    };
+    let resolveOrReject = (capability, success, reason) => {


You can do function resolveOrReject(capability, success, reason) { instead here.

Edit: Please note that if you're not accessing this an arrow function isn't necessary!

Done, thanks for the suggestion. I will take care of that.

Snuffleupagus · 2017-06-02T19:52:31Z

src/shared/util.js

+        capability.reject(reason);
+      }
+    };
+    let finalize = (promise) => {


You can do function finalize(promise) { instead here.

Snuffleupagus · 2017-06-02T19:54:04Z

src/shared/util.js

+      onCancel: null,
+      desiredSize,
+      ready: null,
+    };


Nit: Adding a newline here would aid readability of the code.

yurydelendik · 2017-06-02T22:11:38Z

src/shared/util.js

+    }, queueingStrategy);
+  },
+
+  resolveCall(fn, args, thisArg = null) {


resolveCall does not need to be a member of MessageHandler. Can you move it out of the class (e.g. just before MessageHandler)?

resolveOrReject and finalize can go there too

Done in new commit. Thanks.

yurydelendik

looks good (with resolveOrReject, finalize and resolveCall before MessageHandler constructor)

Snuffleupagus · 2017-06-07T14:58:23Z

src/shared/util.js

+  }
+}
+
+function finalize(promise) {


Unless I'm missing something, you're not actually passing Promises to this function (at line https://github.com/mozilla/pdf.js/pull/8430/files#diff-8d2b578d58d4d8c8a2975020fbbc84d9R1492 and below).

Rather, it seems that what you're passing is actually promiseCapabilities, which means that this function currently doesn't really make sense to me; what am I missing here?

Oh, yes. You are right. Thanks for pointing out.

yurydelendik · 2017-06-07T15:10:25Z

src/shared/util.js

+      // Delete streamController only when start, pull and
+      // cancel callbacks are resolved, to avoid "TypeError".
+      Promise.all([
+        finalize(this.streamControllers[data.streamId].startCall),


sorry. let's map all capabilities to promises Promise.all([this.streamControllers[data.streamId].startCall, this.streamControllers[data.streamId].pullCall,....].map((capability) => { return capability && finalize(capability.promise); })).

Done in new commit. Thanks.

Adds functionality to accept Queueing Strategy in sendWithStream method. Using Queueing Strategy we can control the data that is enqueued into the sink, and hence regulated the flow of chunks from worker to main thread. Adds capability in pull and cancel methods. Adds ready and desiredSize property in streamSink. Adds unit test for ReadableStream and sendWithStream.

Snuffleupagus

r=me, modulo passing tests.

Snuffleupagus · 2017-06-07T15:42:23Z

/botio test

pdfjsbot · 2017-06-07T15:42:24Z

From: Bot.io (Windows)

Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.215.176.217:8877/b6e3d28eb33b99f/output.txt

pdfjsbot · 2017-06-07T15:42:24Z

From: Bot.io (Linux m4)

Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.67.70.0:8877/be28eabda275c01/output.txt

pdfjsbot · 2017-06-07T15:59:33Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/be28eabda275c01/output.txt

Total script time: 17.14 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

pdfjsbot · 2017-06-07T16:12:20Z

From: Bot.io (Windows)

Success

Full output at http://54.215.176.217:8877/b6e3d28eb33b99f/output.txt

Total script time: 29.94 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

yurydelendik · 2017-06-09T17:02:20Z

Thank you for the patch.

Adds sendWithStream method in MessageHandler.

timvandermeij added the core label May 19, 2017

Snuffleupagus reviewed May 20, 2017

View reviewed changes

mukulmishra18 force-pushed the sendWithStream branch 2 times, most recently from 0d9797b to 2dd23b0 Compare May 21, 2017 09:34

mukulmishra18 force-pushed the sendWithStream branch 3 times, most recently from 31e7fe6 to 572a661 Compare May 21, 2017 20:30

mukulmishra18 force-pushed the sendWithStream branch 3 times, most recently from 2147c2b to 6dc7dc5 Compare May 29, 2017 08:20

yurydelendik reviewed May 31, 2017

View reviewed changes

mukulmishra18 force-pushed the sendWithStream branch 2 times, most recently from 34ae2f5 to 834e634 Compare May 31, 2017 21:09

Snuffleupagus reviewed May 31, 2017

View reviewed changes

mukulmishra18 force-pushed the sendWithStream branch from 834e634 to 2d2ebd0 Compare May 31, 2017 21:36

yurydelendik reviewed May 31, 2017

View reviewed changes

mukulmishra18 force-pushed the sendWithStream branch from 2d2ebd0 to c390064 Compare June 1, 2017 12:32

yurydelendik reviewed Jun 1, 2017

View reviewed changes

Snuffleupagus reviewed Jun 2, 2017

View reviewed changes

mukulmishra18 force-pushed the sendWithStream branch from dd64b94 to 8e811cd Compare June 2, 2017 20:48

yurydelendik reviewed Jun 2, 2017

View reviewed changes

yurydelendik approved these changes Jun 2, 2017

View reviewed changes

mukulmishra18 force-pushed the sendWithStream branch 2 times, most recently from fc5b48a to ce08007 Compare June 6, 2017 09:28

yurydelendik requested a review from Snuffleupagus June 6, 2017 13:30

Snuffleupagus reviewed Jun 7, 2017

View reviewed changes

yurydelendik reviewed Jun 7, 2017

View reviewed changes

mukulmishra18 force-pushed the sendWithStream branch from ce08007 to bbd9968 Compare June 7, 2017 15:36

Snuffleupagus approved these changes Jun 7, 2017

View reviewed changes

yurydelendik merged commit 9342054 into mozilla:master Jun 9, 2017

movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018

Merge pull request mozilla#8430 from mukulmishra18/sendWithStream

e381892

Adds sendWithStream method in MessageHandler.


		import { MessageHandler } from '../../src/shared/util';

		describe('util stream', function () {

Adds sendWithStream method in MessageHandler. #8430

Adds sendWithStream method in MessageHandler. #8430

Conversation

mukulmishra18 commented May 19, 2017

Snuffleupagus 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

Snuffleupagus May 21, 2017 • 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

Snuffleupagus May 20, 2017 • 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

mukulmishra18 commented May 21, 2017

Snuffleupagus commented May 21, 2017 • edited Loading

mukulmishra18 commented May 21, 2017

Snuffleupagus commented May 22, 2017 • edited Loading

mukulmishra18 commented May 22, 2017 • edited Loading

mukulmishra18 commented May 23, 2017

yurydelendik 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

Snuffleupagus May 31, 2017 • 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

mukulmishra18 May 31, 2017 • edited Loading

Choose a reason for hiding this comment

timvandermeij commented May 31, 2017 • 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

mukulmishra18 commented May 31, 2017

timvandermeij commented May 31, 2017

yurydelendik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus Jun 2, 2017 • 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

mukulmishra18 Jun 1, 2017 • 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

Snuffleupagus May 21, 2017 •

edited

Loading

Snuffleupagus May 20, 2017 •

edited

Loading

Snuffleupagus commented May 21, 2017 •

edited

Loading

Snuffleupagus commented May 22, 2017 •

edited

Loading

mukulmishra18 commented May 22, 2017 •

edited

Loading

Snuffleupagus May 31, 2017 •

edited

Loading

mukulmishra18 May 31, 2017 •

edited

Loading

timvandermeij commented May 31, 2017 •

edited

Loading

Snuffleupagus Jun 2, 2017 •

edited

Loading

mukulmishra18 Jun 1, 2017 •

edited

Loading

mukulmishra18 Jun 1, 2017 •

edited

Loading