Refactors CMapFactory.create to make it async #7039

ManasJayanth · 2016-02-28T16:30:48Z

For #6352. Up for review

yurydelendik · 2016-03-02T21:05:46Z

src/core/cmap.js

-      return parseBinaryCMap(name, builtInCMapParams);
-    }
+      if (builtInCMapParams.packed) {
+        resolve(parseBinaryCMap(name, builtInCMapParams));


In production we mostly use parseBinaryCMap -- this shall be async too

yurydelendik · 2016-03-07T19:59:03Z

Review status: 0 of 3 files reviewed at latest revision, 18 unresolved discussions.

src/core/cmap.js, line 445 [r2] (raw file):
You can remove nonBinaryRequest branches and always use request.responseType = 'arraybuffer';

src/core/cmap.js, line 980 [r2] (raw file):
return cMap; here to resolve promise into cMap

src/core/cmap.js, line 1003 [r2] (raw file):
Here (and in XHR above) check if the url is HTTP (e.g. /^https?:/i.test(url);) and if not check status === 0.

src/core/evaluator.js, line 253 [r2] (raw file):
unneeded change

src/core/evaluator.js, line 344 [r2] (raw file):
unneeded change

src/core/evaluator.js, line 430 [r2] (raw file):
unneeded change

src/core/evaluator.js, line 508 [r2] (raw file):
unneeded change

src/core/evaluator.js, line 803 [r2] (raw file):
unneeded change

src/core/evaluator.js, line 827 [r2] (raw file):
unneeded change

src/core/evaluator.js, line 1187 [r2] (raw file):
unneeded change

src/core/evaluator.js, line 1555 [r2] (raw file):
there shall no be return, make it: var toUnicodePromise = toUnicode ? this.readToUnicode(toUnicode) : Promise.resolve(undefined);. Instead of wrapping the code below in then() you can split extractDataStructures into two functions and call rest of extractDataStructures when toUnicodePromise is resolved

src/core/evaluator.js, line 1653 [r2] (raw file):
you already handling then() -- Promise.resolve is not needed

Comments from the review on Reviewable.io

ManasJayanth · 2016-03-08T04:44:24Z

@yurydelendik [TYPO EDIT] I'll squash all the commits at the end when the code review is done. Okay?

I was using reviewable.io, but the tool failed at some point and I think I lost some of the comments :(

yurydelendik · 2016-03-08T17:55:39Z

@yurydelendik All squash all the commits at the end when the code review is done. Okay?

You can squash commits as you pushing updated version to this PR (reviewable shall be able to track that I hope).

ManasJayanth · 2016-03-10T12:08:03Z

Review status: 0 of 3 files reviewed at latest revision, 18 unresolved discussions.

src/core/cmap.js, line 973 [r1] (raw file):
Done.

src/core/cmap.js, line 985 [r1] (raw file):
Done.

src/core/cmap.js, line 445 [r2] (raw file):
Can you please explain this? Why are the nonBinaryRequest branches no longer necessary?

src/core/cmap.js, line 980 [r2] (raw file):
But processBinaryCMap alreadys returns a cMap

src/core/cmap.js, line 1003 [r2] (raw file):
Done.

src/core/evaluator.js, line 1554 [r1] (raw file):
Done.

src/core/evaluator.js, line 1560 [r1] (raw file):
Done.

src/core/evaluator.js, line 1688 [r1] (raw file):
Done.

src/core/evaluator.js, line 2086 [r1] (raw file):
Done.

src/core/evaluator.js, line 253 [r2] (raw file):
This was after Snuffleupaugus mentioned about the line breaks I made in the patch. He was referring to my patch alone and I guess I overdid.

Reverting the unneeded changes you've pointed out.

src/core/evaluator.js, line 344 [r2] (raw file):
Done.

src/core/evaluator.js, line 430 [r2] (raw file):
Done.

src/core/evaluator.js, line 508 [r2] (raw file):
Done.

src/core/evaluator.js, line 803 [r2] (raw file):
Done.

src/core/evaluator.js, line 827 [r2] (raw file):
Done.

src/core/evaluator.js, line 1187 [r2] (raw file):
Done.

src/core/evaluator.js, line 1555 [r2] (raw file):
Can you explain the reason behind this change?

src/core/evaluator.js, line 1653 [r2] (raw file):
Done.

Comments from the review on Reviewable.io

yurydelendik · 2016-03-10T14:31:33Z

Looks good so far. One or two thing are left: chaining promises at extractDataStructures and translateFont. Thanks

Reviewed 1 of 3 files at r1, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 7 unresolved discussions.

src/core/cmap.js, line 445 [r2] (raw file):
nonBinaryRequest was only added because sync XHR cannot handle binary data -- it's not need since we are making everything async and we can use arraybuffer with XHR.

src/core/cmap.js, line 980 [r2] (raw file):
Nevermind, I thought it's a Promise's then callback -- we will refactor this later

src/core/cmap.js, line 1003 [r2] (raw file):
shall be var expectedStatus = /^https?:/i.test(url) ? 200 : 0; ... if (request.status !== expectedStatus) ...

src/core/evaluator.js, line 2086 [r1] (raw file):
The same situation is at extractDataStructures above, we need to chain promise here: wait for CMapFactory.create to be resolve before we proceed with the code below.

src/core/evaluator.js, line 253 [r2] (raw file):
Extra change make it hard to review, it's better if only new/affected code is changed. Thanks

src/core/evaluator.js, line 1555 [r2] (raw file):
The code path below shall continue after this.readToUnicode is done, i.e. the cidToGidMap and differences properties must be assigned too

Comments from the review on Reviewable.io

ManasJayanth · 2016-03-11T04:15:42Z

Review status: all files reviewed at latest revision, 3 unresolved discussions.

src/core/cmap.js, line 445 [r2] (raw file):
So mapping responseText to an character array (ch & 255) is not necessary? resolve(new Uint8Array(request.response)); is enough? (cmap.js#L431)

Can you help me understand this?

      if (!nonBinaryRequest) {
        try {
          request.responseType = 'arraybuffer';
          nonBinaryRequest = request.responseType !== 'arraybuffer';
        } catch (e) {
          nonBinaryRequest = true;
        }
      }

Can this be safely removed too?

src/core/evaluator.js, line 253 [r2] (raw file):
I figured :) My bad

Comments from the review on Reviewable.io

ManasJayanth · 2016-03-11T04:51:59Z

Review status: all files reviewed at latest revision, 3 unresolved discussions.

src/core/cmap.js, line 445 [r2] (raw file):
I mean replace it with request.responseType = 'arraybuffer'

Comments from the review on Reviewable.io

ManasJayanth · 2016-03-11T04:54:19Z

Review status: 1 of 3 files reviewed at latest revision, 3 unresolved discussions.

src/core/evaluator.js, line 253 [r2] (raw file):
Done.

Comments from the review on Reviewable.io

yurydelendik · 2016-03-11T13:18:08Z

Reviewed 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

src/core/cmap.js, line 445 [r2] (raw file):
That's correct, you can use only request.responseType = 'arraybuffer' (We shall have polyfill in compatibility.js for browsers that does not support arraybuffer).

src/core/cmap.js, line 446 [r4] (raw file):
Add the same expectedStatus check as below here

Comments from the review on Reviewable.io

ManasJayanth · 2016-03-11T15:20:41Z

Review status: 2 of 3 files reviewed at latest revision, 2 unresolved discussions.

src/core/cmap.js, line 446 [r4] (raw file):
Done.

Comments from the review on Reviewable.io

yurydelendik · 2016-03-11T15:34:23Z

Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

src/core/cmap.js, line 429 [r5] (raw file):
async parameter shall be true here

src/core/evaluator.js, line 345 [r5] (raw file):
unneeded change?

Comments from the review on Reviewable.io

ManasJayanth · 2016-03-11T15:47:08Z

Review status: 1 of 3 files reviewed at latest revision, 3 unresolved discussions.

src/core/evaluator.js, line 345 [r5] (raw file):
Reverting

Comments from the review on Reviewable.io

yurydelendik · 2016-03-11T15:49:46Z

Review status: 1 of 3 files reviewed at latest revision, 5 unresolved discussions.

src/core/cmap.js, line 718 [r5] (raw file):
processBinaryCMap (aka BinaryCMapReader.read) calls the extend callback which can be async (it returns promise?). Let's not refactor it now, but return extend(useCMap).then(function () { return cMap; }); and fix parseBinaryCMap below

src/core/cmap.js, line 958 [r5] (raw file):
make extendCMap async and return its promise here (the createBuiltInCMap used by this function is async)

Comments from the review on Reviewable.io

ManasJayanth · 2016-03-11T16:00:28Z

Review status: 1 of 3 files reviewed at latest revision, 4 unresolved discussions.

src/core/cmap.js, line 958 [r5] (raw file):
Or simply return cmap from .read()?

Comments from the review on Reviewable.io

ManasJayanth · 2016-03-11T16:01:27Z

Review status: 1 of 3 files reviewed at latest revision, 4 unresolved discussions.

src/core/cmap.js, line 958 [r5] (raw file):
processBinaryCMap (i.e. .read()) returns a promise after all.

Comments from the review on Reviewable.io

yurydelendik · 2016-03-11T16:14:05Z

yurydelendik · 2016-03-11T16:21:01Z

Review status: 1 of 3 files reviewed at latest revision, 6 unresolved discussions.

src/core/cmap.js, line 958 [r5] (raw file):
Loading during extendCMap might be incomplete due to its createBuiltInCMap call but we will be signaling that we are done. So we have to make extendCMap async and wait on its promise here. extendCMap may or may not return result cMap, however read() call shall return the cMap we are loading.

Comments from the review on Reviewable.io

ManasJayanth · 2016-03-11T17:12:54Z

Review status: 1 of 3 files reviewed at latest revision, 5 unresolved discussions.

src/core/evaluator.js, line 1640 [r5] (raw file):
Move buildToUnicode here?

Comments from the review on Reviewable.io

yurydelendik · 2016-03-11T17:18:57Z

Reviewed 1 of 2 files at r7, 1 of 1 files at r8.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

src/core/cmap.js, line 938 [r8] (raw file):
Use right cMap here. Rename the function parameter into e.g. newCMap and assign it to the cMap.useCMap = newCMap. The function shall return cMap

src/core/evaluator.js, line 1640 [r5] (raw file):
That's correct,

return toUnicodePromise.then(function(toUnicode) {
   properties.toUnicode = toUnicode;
   return this.buildToUnicode(properties);
}.this(this)).then(function (toUnicode) {
  properties.toUnicode = toUnicode;
  return properties;
});

We will refactor the logic of buildToUnicode later.

Comments from the review on Reviewable.io

yurydelendik · 2016-03-11T17:25:14Z

I recommend to rebase it to the newest mozilla/master before moving buildToUnicode (I saw some of PRs landed that will affect it)

ManasJayanth · 2016-03-11T17:37:01Z

Sure. Thanks for the heads up

ManasJayanth · 2016-03-11T18:45:59Z

Review status: all files reviewed at latest revision, 13 unresolved discussions.

src/core/evaluator.js, line 1655 [r5] (raw file):
Tried indenting like Snuffleupagus recommended. How would you like it?

Comments from the review on Reviewable.io

yurydelendik · 2016-03-11T18:46:52Z

Review status: all files reviewed at latest revision, 12 unresolved discussions.

src/core/evaluator.js, line 1653 [r10] (raw file):
yes

Comments from the review on Reviewable.io

Snuffleupagus · 2016-03-11T18:47:53Z

src/core/evaluator.js

+
+      // The viewer's choice, just use an identity map.
+      return Promise.resolve(new IdentityToUnicodeMap(properties.firstChar,
+                                      properties.lastChar));


Nit: please indent this line a bit more, like this

return Promise.resolve(new IdentityToUnicodeMap(properties.firstChar, properties.lastChar));

ManasJayanth · 2016-03-11T18:57:42Z

Review status: all files reviewed at latest revision, 12 unresolved discussions.

src/core/evaluator.js, line 354 [r11] (raw file):
Not sure what you are referring to.

Comments from the review on Reviewable.io

yurydelendik · 2016-03-11T19:18:57Z

Review status: 1 of 3 files reviewed at latest revision, 14 unresolved discussions.

src/core/evaluator.js, line 2133 [r13] (raw file):
add }.bind(this)

src/core/evaluator.js, line 2232 [r13] (raw file):
add .bind(this)

Comments from the review on Reviewable.io

yurydelendik · 2016-03-11T19:33:51Z

Reviewed 1 of 2 files at r12, 1 of 1 files at r14.
Review status: all files reviewed at latest revision, 14 unresolved discussions.

Comments from the review on Reviewable.io

yurydelendik · 2016-03-11T19:40:24Z

cmap_spec needs to be fixed, e.g.:

@@ -7,16 +7,34 @@ var cMapUrl = '../../external/bcmaps/';
 var cMapPacked = true;

 describe('cmap', function() {
+  var TEST_TIMEOUT = 20000;
+  function waitsForPromiseResolved(promise, successCallback) {
+    var resolved = false;
+    promise.then(function(val) {
+        resolved = true;
+        successCallback(val);
+      },
+      function(error) {
+        // Shouldn't get here.
+        expect(error).toEqual('the promise should not have been rejected');
+      });
+    waitsFor(function() {
+      return resolved;
+    }, TEST_TIMEOUT);
+  }
+
   it('parses beginbfchar', function() {
     var str = '2 beginbfchar\n' +
               '<03> <00>\n' +
               '<04> <01>\n' +
               'endbfchar\n';
     var stream = new StringStream(str);
-    var cmap = CMapFactory.create(stream);
-    expect(cmap.lookup(0x03)).toEqual(String.fromCharCode(0x00));
-    expect(cmap.lookup(0x04)).toEqual(String.fromCharCode(0x01));
-    expect(cmap.lookup(0x05)).toBeUndefined();
+    var cmapPromise = CMapFactory.create(stream);
+    waitsForPromiseResolved(cmapPromise, function (cmap) {
+      expect(cmap.lookup(0x03)).toEqual(String.fromCharCode(0x00));
+      expect(cmap.lookup(0x04)).toEqual(String.fromCharCode(0x01));
+      expect(cmap.lookup(0x05)).toBeUndefined();
+    });
   });
   it('parses beginbfrange with range', function() {
     var str = '1 beginbfrange\n' +

yurydelendik · 2016-03-11T19:56:41Z

Review status: all files reviewed at latest revision, 14 unresolved discussions.

src/core/evaluator.js, line 354 [r11] (raw file):
Looking at https://github.com/mozilla/pdf.js/pull/7039/files#diff-0b94c2e77a5259f7a728122fdbf9f46aL345, there is some code movement is happening

Comments from the review on Reviewable.io

yurydelendik · 2016-03-11T20:31:30Z

src/core/evaluator.js

-          { url: PDFJS.cMapUrl, packed: PDFJS.cMapPacked }, null);
-        properties.vertical = properties.cMap.vertical;
+        return CMapFactory.create(cidEncoding,
+          { url: PDFJS.cMapUrl, packed: PDFJS.cMapPacked }, null).then(


chain that with this.buildToUnicode call -- we need properties.toUnicode populated before new Font() call.

So it would be

return CMapFactory.create(cidEncoding, { url: PDFJS.cMapUrl, packed: PDFJS.cMapPacked }, null).then( function (cMap) { properties.cMap = cMap; properties.vertical = properties.cMap.vertical; return this.buildToUnicode(cMap.properties) }.bind(this)).then(function (toUnicode) { properties.toUnicode = toUnicode; return new Font(fontName.name, fontFile, properties); });

make there return this.buildToUnicode(properties); though

yurydelendik · 2016-03-12T01:36:30Z

r+ when all tests pass, looks good in terms of refactored logic.

brendandahl · 2016-03-15T21:00:57Z

@prometheansacrifice There are still some test failures. Once those are fixed I'll review.

yurydelendik · 2016-03-21T17:41:21Z

/botio test

pdfjsbot · 2016-03-21T17:41:22Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/9b5649f35803541/output.txt

pdfjsbot · 2016-03-21T17:41:22Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/256d415292652fd/output.txt

pdfjsbot · 2016-03-21T18:01:53Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/9b5649f35803541/output.txt

Total script time: 20.52 mins

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

pdfjsbot · 2016-03-21T18:03:24Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/256d415292652fd/output.txt

Total script time: 22.04 mins

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

Refactors CMapFactory.create to make it async

yurydelendik · 2016-03-21T18:57:48Z

Thank you for the patch. Good work.

ManasJayanth · 2016-03-21T19:19:00Z

Thank you too yury. Appreciate your help in this

timvandermeij assigned yurydelendik Feb 28, 2016

timvandermeij added the core label Feb 28, 2016

ManasJayanth changed the title ~~[WIP] Refactors CMapFactory.create to make it async~~ Refactors CMapFactory.create to make it async Mar 2, 2016

yurydelendik reviewed Mar 2, 2016
View reviewed changes

Snuffleupagus reviewed Mar 11, 2016
View reviewed changes

yurydelendik reviewed Mar 11, 2016
View reviewed changes

Refactors CMapFactory.create to make it async

f6d28ca

yurydelendik added a commit that referenced this pull request Mar 21, 2016

Merge pull request #7039 from prometheansacrifice/async-cmap-factory

21ed8ff

Refactors CMapFactory.create to make it async

yurydelendik merged commit 21ed8ff into mozilla:master Mar 21, 2016

ManasJayanth deleted the async-cmap-factory branch March 21, 2016 19:15

timvandermeij mentioned this pull request Mar 21, 2016

Refactor CMap factory to be async #6352

Closed

Snuffleupagus mentioned this pull request Jul 18, 2016

Catch errors and continue parsing in parseCMap (issue 7492) #7493

Merged

Snuffleupagus mentioned this pull request Feb 11, 2017

Consume the current character when encountering illegal characters in Lexer.getObject, in order to prevent infinite loops during reading of streams (issue 8061) #8063

Merged

Refactors CMapFactory.create to make it async #7039

Refactors CMapFactory.create to make it async #7039

Conversation

ManasJayanth commented Feb 28, 2016

yurydelendik Mar 2, 2016

Choose a reason for hiding this comment

yurydelendik commented Mar 7, 2016

ManasJayanth commented Mar 8, 2016

yurydelendik commented Mar 8, 2016

ManasJayanth commented Mar 10, 2016

yurydelendik commented Mar 10, 2016

ManasJayanth commented Mar 11, 2016

ManasJayanth commented Mar 11, 2016

ManasJayanth commented Mar 11, 2016

yurydelendik commented Mar 11, 2016

ManasJayanth commented Mar 11, 2016

yurydelendik commented Mar 11, 2016

ManasJayanth commented Mar 11, 2016

yurydelendik commented Mar 11, 2016

ManasJayanth commented Mar 11, 2016

ManasJayanth commented Mar 11, 2016

yurydelendik commented Mar 11, 2016

yurydelendik commented Mar 11, 2016

ManasJayanth commented Mar 11, 2016

yurydelendik commented Mar 11, 2016

yurydelendik commented Mar 11, 2016

ManasJayanth commented Mar 11, 2016

ManasJayanth commented Mar 11, 2016

yurydelendik commented Mar 11, 2016

Snuffleupagus Mar 11, 2016

Choose a reason for hiding this comment

ManasJayanth commented Mar 11, 2016

yurydelendik commented Mar 11, 2016

yurydelendik commented Mar 11, 2016

yurydelendik commented Mar 11, 2016

yurydelendik commented Mar 11, 2016

yurydelendik Mar 11, 2016

Choose a reason for hiding this comment

ManasJayanth Mar 11, 2016

Choose a reason for hiding this comment

yurydelendik Mar 11, 2016

Choose a reason for hiding this comment

yurydelendik Mar 11, 2016

Choose a reason for hiding this comment

yurydelendik commented Mar 12, 2016

brendandahl commented Mar 15, 2016

yurydelendik commented Mar 21, 2016

pdfjsbot commented Mar 21, 2016

From: Bot.io (Windows)

Received

pdfjsbot commented Mar 21, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Mar 21, 2016

From: Bot.io (Windows)

Success

pdfjsbot commented Mar 21, 2016

From: Bot.io (Linux)

Success

yurydelendik commented Mar 21, 2016

ManasJayanth commented Mar 21, 2016