Use a `WeakMap` in `src/display/text_layer.js` #7588

timvandermeij · 2016-09-02T18:23:34Z

This is a part of #7584 that fixes the third and fourth item. Look at the commit messages for more details.

Snuffleupagus · 2016-09-02T18:41:13Z

src/display/text_layer.js

@@ -46,6 +46,8 @@ var getDefaultSetting = displayDOMUtils.getDefaultSetting;
 *   initially be set to empty array.
 * @property {number} timeout - (optional) Delay in milliseconds before
 *   rendering of the text  runs occurs.
+ * @property {boolean} enhanceTextSelection - Whether to turn on the text


Nit: similar to the timeout property above, let's add "(optional)" to this doc string.

Fixed in the new commit.

We pass many parameters to `appendText` while we might as well pass the `task` object that contains them. This saves a few lines of code and makes the signature of `appendText` more clear. We do the same for `expand`, which is useful for the next commit in which we replace `div.dataset` with a `WeakMap`. Furthermore, this patch adds a missing parameter to a comment block to make it clear which parameters remain.

Snuffleupagus · 2016-09-02T18:54:00Z

src/display/text_layer.js

@@ -585,7 +594,7 @@ var renderTextLayer = (function renderTextLayerClosure() {
        for (i = 0, ii = this._textDivs.length; i < ii; i++) {
          div = this._textDivs[i];
          div.style.padding = 0;
-          transform = div.dataset.originalTransform || '';
+          transform = divProperties.originalTransform || '';


divProperties isn't defined in the else branch, which probably explains why the text layer isn't reset properly on mouseup.

Fixed in the new commit. The structure was a bit strange here since the for loop and div fetching is equal in both the if and else branches, so I've slightly refactored that to prevent this issue.

timvandermeij · 2016-09-02T20:58:18Z

/botio-linux preview

pdfjsbot · 2016-09-02T20:58:18Z

From: Bot.io (Linux)

Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/ebf87c62c039c92/output.txt

pdfjsbot · 2016-09-02T20:59:19Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/ebf87c62c039c92/output.txt

Total script time: 1.00 mins

Published

timvandermeij · 2016-09-02T20:59:39Z

/botio test

pdfjsbot · 2016-09-02T20:59:39Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/e148a9b4df59be2/output.txt

pdfjsbot · 2016-09-02T20:59:39Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/745feb2df1ef803/output.txt

pdfjsbot · 2016-09-02T21:23:43Z

From: Bot.io (Windows)

Failed

Full output at http://107.22.172.223:8877/745feb2df1ef803/output.txt

Total script time: 24.05 mins

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

Image differences available at: http://107.22.172.223:8877/745feb2df1ef803/reftest-analyzer.html#web=eq.log

pdfjsbot · 2016-09-02T21:29:03Z

From: Bot.io (Linux)

Failed

Full output at http://107.21.233.14:8877/e148a9b4df59be2/output.txt

Total script time: 29.38 mins

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

Image differences available at: http://107.21.233.14:8877/e148a9b4df59be2/reftest-analyzer.html#web=eq.log

timvandermeij · 2016-09-02T22:09:50Z

I updated the second patch to fix the debugger as it relied on dataset for the font name. Since the font name is only required for the debugger and not for anything else, it's fine to use dataset only for that field. Manual testing confirms that the debugger now works again. For completeness, this is the diff with the tested version above, which should have no effect on the test run:

diff --git a/src/display/text_layer.js b/src/display/text_layer.js
index 3d57c58..86a85b4 100644
--- a/src/display/text_layer.js
+++ b/src/display/text_layer.js
@@ -65,7 +65,6 @@ var renderTextLayer = (function renderTextLayerClosure() {
     var textDivProperties = {
       angle: 0,
       canvasWidth: 0,
-      fontName: '',
       isWhitespace: false,
       originalTransform: '',
       originalWidth: 0,
@@ -113,9 +112,10 @@ var renderTextLayer = (function renderTextLayerClosure() {
     textDiv.textContent = geom.str;
     // |fontName| is only used by the Font Inspector. This test will succeed
     // when e.g. the Font Inspector is off but the Stepper is on, but it's
-    // not worth the effort to do a more accurate test.
+    // not worth the effort to do a more accurate test. We only use `dataset`
+    // here to make the font name available for the debugger.
     if (getDefaultSetting('pdfBug')) {
-      textDivProperties.fontName = geom.fontName;
+      textDiv.dataset.fontName = geom.fontName;
     }
     if (angle !== 0) {
       textDivProperties.angle = angle * (180 / Math.PI);

Snuffleupagus · 2016-09-03T07:53:28Z

src/display/text_layer.js

      for (var i = 0, len = textItems.length; i < len; i++) {
-        appendText(textDivs, viewport, textItems[i], styles, bounds,
-                   enhanceTextSelection);
+        appendText(this, textItems[i]);


Since you're passing in textItems separately here, I'm wondering if it wouldn't make sense to still pass in styles in a similar way too?
I.e. re-introduce var styles = this._textContent.styles; above and make this function call appendText(this, textItems[i], styles);?

The reason that I'm asking is that this would get rid of the, comparatively complex given the others, assignment var style = task._textContent.styles[geom.fontName]; in appendText and use the previous var style = styles[geom.fontName]; instead.

@yurydelendik

This patch improves performance by avoiding unnecessary type conversions, which also help the JIT for optimizations. Moreover, this patch fixes issues with the div expansion code where `textScale` would be undefined in a division. Because of the `dataset` usage, other comparisons evaluated to `true` while `false` would have been correct. This makes the expansion mode now work correctly for cases with, for example, each glyph in one div. The polyfill for `WeakMap` has been provided by @yurydelendik.

timvandermeij · 2016-09-03T18:12:43Z

The review comments have been addressed. These are the difference since the last version:

diff --git a/src/display/text_layer.js b/src/display/text_layer.js
index 86a85b4..f1aa9b8 100644
--- a/src/display/text_layer.js
+++ b/src/display/text_layer.js
@@ -59,7 +59,7 @@ var renderTextLayer = (function renderTextLayerClosure() {
     return !NonWhitespaceRegexp.test(str);
   }

-  function appendText(task, geom) {
+  function appendText(task, geom, styles) {
     // Initialize all used properties to keep the caches monomorphic.
     var textDiv = document.createElement('div');
     var textDivProperties = {
@@ -83,7 +83,7 @@ var renderTextLayer = (function renderTextLayerClosure() {

     var tx = Util.transform(task._viewport.transform, geom.transform);
     var angle = Math.atan2(tx[1], tx[0]);
-    var style = task._textContent.styles[geom.fontName];
+    var style = styles[geom.fontName];
     if (style.vertical) {
       angle += Math.PI / 2;
     }
@@ -210,8 +210,8 @@ var renderTextLayer = (function renderTextLayerClosure() {

       var transform = '';
       if (textDivProperties.canvasWidth !== 0 && width > 0) {
-        var textScale = textDivProperties.canvasWidth / width;
-        transform = 'scaleX(' + textScale + ')';
+        var scale = textDivProperties.canvasWidth / width;
+        transform = 'scaleX(' + scale + ')';
       }
       var rotation = textDivProperties.angle;
       if (rotation !== 0) {
@@ -537,8 +537,9 @@ var renderTextLayer = (function renderTextLayerClosure() {

     _render: function TextLayer_render(timeout) {
       var textItems = this._textContent.items;
+      var textStyles = this._textContent.styles;
       for (var i = 0, len = textItems.length; i < len; i++) {
-        appendText(this, textItems[i]);
+        appendText(this, textItems[i], textStyles);
       }

       if (!timeout) { // Render right away
@@ -568,22 +569,21 @@ var renderTextLayer = (function renderTextLayerClosure() {

         if (expandDivs) {
           var transform = '';
-          var width = divProperties.originalWidth;
-          var textScale = 1;
+          var scale = 1;

-          if (divProperties.canvasWidth !== 0 && width > 0) {
-            textScale = divProperties.canvasWidth / width;
-            transform = 'scaleX(' + textScale + ')';
+          if (divProperties.canvasWidth !== 0 &&
+              divProperties.originalWidth > 0) {
+            scale = divProperties.canvasWidth / divProperties.originalWidth;
+            transform = 'scaleX(' + scale + ')';
           }
-          var rotation = divProperties.angle;
-          if (rotation !== 0) {
-            transform = 'rotate(' + rotation + 'deg) ' + transform;
+          if (divProperties.angle !== 0) {
+            transform = 'rotate(' + divProperties.angle + 'deg) ' + transform;
           }
           if (divProperties.paddingLeft !== 0) {
             div.style.paddingLeft =
-              (divProperties.paddingLeft / textScale) + 'px';
+              (divProperties.paddingLeft / scale) + 'px';
             transform += ' translateX(' +
-              (-divProperties.paddingLeft / textScale) + 'px)';
+              (-divProperties.paddingLeft / scale) + 'px)';
           }
           if (divProperties.paddingTop !== 0) {
             div.style.paddingTop = divProperties.paddingTop + 'px';
@@ -591,7 +591,7 @@ var renderTextLayer = (function renderTextLayerClosure() {
           }
           if (divProperties.paddingRight !== 0) {
             div.style.paddingRight =
-              divProperties.paddingRight / textScale + 'px';
+              divProperties.paddingRight / scale + 'px';
           }
           if (divProperties.paddingBottom !== 0) {
             div.style.paddingBottom = divProperties.paddingBottom + 'px';
@@ -601,8 +601,8 @@ var renderTextLayer = (function renderTextLayerClosure() {
           }
         } else {
           div.style.padding = 0;
-          transform = divProperties.originalTransform;
-          CustomStyle.setProp('transform', div, transform);
+          CustomStyle.setProp('transform', div,
+                              divProperties.originalTransform);
         }
       }
     },

timvandermeij · 2016-09-04T12:53:00Z

/botio-linux preview

pdfjsbot · 2016-09-04T12:53:00Z

From: Bot.io (Linux)

Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/30c6c5700832f5d/output.txt

pdfjsbot · 2016-09-04T12:54:15Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/30c6c5700832f5d/output.txt

Total script time: 1.25 mins

Published

timvandermeij · 2016-09-04T12:54:43Z

/botio test

pdfjsbot · 2016-09-04T12:54:43Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/8dd36221519b4bd/output.txt

pdfjsbot · 2016-09-04T12:54:43Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/9cd50b3a500dcd4/output.txt

pdfjsbot · 2016-09-04T13:18:27Z

From: Bot.io (Windows)

Failed

Full output at http://107.22.172.223:8877/8dd36221519b4bd/output.txt

Total script time: 23.72 mins

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

Image differences available at: http://107.22.172.223:8877/8dd36221519b4bd/reftest-analyzer.html#web=eq.log

pdfjsbot · 2016-09-04T13:34:09Z

From: Bot.io (Linux)

Failed

Full output at http://107.21.233.14:8877/9cd50b3a500dcd4/output.txt

Total script time: 39.43 mins

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

Image differences available at: http://107.21.233.14:8877/9cd50b3a500dcd4/reftest-analyzer.html#web=eq.log

Snuffleupagus · 2016-09-04T14:18:02Z

src/display/text_layer.js

-        transform = '';
+      var transform = '';
+      if (textDivProperties.canvasWidth !== 0 && width > 0) {
+        var scale = textDivProperties.canvasWidth / width;


Optional comment: I'm really sorry that I didn't think to ask about this before now, but I wonder if you couldn't store the scale in textDivProperties.scale (with 1 as the default value above) instead?

That way you'd not need to either store textDivProperties.originalWidth, nor recompute the scale in expandTextDivs. Instead, below you'd just do e.g.

... var scale = divProperties.scale; if (scale !== 1) { transform = 'scaleX(' + scale + ')'; } ...

Excellent idea! I have done this in a separate commit (as it's not really related to the WeakMap or refactoring, but more to optimization).

Snuffleupagus · 2016-09-04T14:23:46Z

This looks great, thank you for working on this!

After looking through this once more, I've got one remaining question.
r=me, with or without #7588 (comment) addressed, so just go ahead and land this as-is if you prefer!

(We can improve this further in a follow-up, since I don't want to unnecessarily delay landing this any more just because of a last minute idea on my part.)

This patch avoids having to calculate the scale twice by saving it in the properties object. Moreover, we remove a temporary variable and place parentheses around a calculation inside a string concatenation.

timvandermeij · 2016-09-04T18:31:13Z

/botio-linux preview

pdfjsbot · 2016-09-04T18:31:13Z

From: Bot.io (Linux)

Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/5a67a88b94aca47/output.txt

pdfjsbot · 2016-09-04T18:32:26Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/5a67a88b94aca47/output.txt

Total script time: 1.21 mins

Published

timvandermeij · 2016-09-04T18:32:55Z

/botio test

pdfjsbot · 2016-09-04T18:32:55Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/37b43803865c96f/output.txt

pdfjsbot · 2016-09-04T18:32:55Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/dc125348339e012/output.txt

pdfjsbot · 2016-09-04T18:56:54Z

From: Bot.io (Windows)

Failed

Full output at http://107.22.172.223:8877/dc125348339e012/output.txt

Total script time: 23.99 mins

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

Image differences available at: http://107.22.172.223:8877/dc125348339e012/reftest-analyzer.html#web=eq.log

pdfjsbot · 2016-09-04T19:11:44Z

From: Bot.io (Linux)

Failed

Full output at http://107.21.233.14:8877/37b43803865c96f/output.txt

Total script time: 38.81 mins

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

Image differences available at: http://107.21.233.14:8877/37b43803865c96f/reftest-analyzer.html#web=eq.log

Snuffleupagus · 2016-09-04T19:25:10Z

Thank you for the patch!

/botio makeref

pdfjsbot · 2016-09-04T19:25:10Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/30ef2e339c3c514/output.txt

pdfjsbot · 2016-09-04T19:25:10Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/762431faf24a3bb/output.txt

pdfjsbot · 2016-09-04T19:49:07Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/762431faf24a3bb/output.txt

Total script time: 23.93 mins

Lint: Passed
Make references: Passed
Check references: Passed

pdfjsbot · 2016-09-04T20:03:25Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/30ef2e339c3c514/output.txt

Total script time: 38.24 mins

Lint: Passed
Make references: Passed
Check references: Passed

timvandermeij added performance text-selection labels Sep 2, 2016

Snuffleupagus reviewed Sep 2, 2016
View reviewed changes

timvandermeij force-pushed the text-layer-weakmap branch 4 times, most recently from 192624d to 521aa61 Compare September 2, 2016 20:57

timvandermeij assigned Snuffleupagus Sep 2, 2016

timvandermeij mentioned this pull request Sep 2, 2016

Prepare a new release #7589

Closed

timvandermeij force-pushed the text-layer-weakmap branch from 521aa61 to badeb6a Compare September 2, 2016 22:09

Snuffleupagus reviewed Sep 3, 2016
View reviewed changes

timvandermeij force-pushed the text-layer-weakmap branch from badeb6a to b3818d5 Compare September 3, 2016 18:12

Snuffleupagus reviewed Sep 4, 2016
View reviewed changes

Optimize scale calculation in text_layer.js

9659357

This patch avoids having to calculate the scale twice by saving it in the properties object. Moreover, we remove a temporary variable and place parentheses around a calculation inside a string concatenation.

Snuffleupagus merged commit 38c8503 into mozilla:master Sep 4, 2016

timvandermeij deleted the text-layer-weakmap branch September 4, 2016 20:34

timvandermeij mentioned this pull request Sep 4, 2016

Adding "proper" message port for fake worker. #6948

Closed

Use a WeakMap in src/display/text_layer.js #7588

Use a WeakMap in src/display/text_layer.js #7588

Conversation

timvandermeij commented Sep 2, 2016 • edited Loading

Snuffleupagus Sep 2, 2016

Choose a reason for hiding this comment

timvandermeij Sep 2, 2016

Choose a reason for hiding this comment

Snuffleupagus Sep 2, 2016

Choose a reason for hiding this comment

timvandermeij Sep 2, 2016

Choose a reason for hiding this comment

timvandermeij commented Sep 2, 2016

pdfjsbot commented Sep 2, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Sep 2, 2016

From: Bot.io (Linux)

Success

Published

timvandermeij commented Sep 2, 2016

pdfjsbot commented Sep 2, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Sep 2, 2016

From: Bot.io (Windows)

Received

pdfjsbot commented Sep 2, 2016

From: Bot.io (Windows)

Failed

pdfjsbot commented Sep 2, 2016

From: Bot.io (Linux)

Failed

timvandermeij commented Sep 2, 2016 • edited Loading

Snuffleupagus Sep 3, 2016 • edited Loading

Choose a reason for hiding this comment

timvandermeij commented Sep 3, 2016

timvandermeij commented Sep 4, 2016

pdfjsbot commented Sep 4, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Sep 4, 2016

From: Bot.io (Linux)

Success

Published

timvandermeij commented Sep 4, 2016

pdfjsbot commented Sep 4, 2016

From: Bot.io (Windows)

Received

pdfjsbot commented Sep 4, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Sep 4, 2016

From: Bot.io (Windows)

Failed

pdfjsbot commented Sep 4, 2016

From: Bot.io (Linux)

Failed

Snuffleupagus Sep 4, 2016 • edited Loading

Choose a reason for hiding this comment

timvandermeij Sep 4, 2016

Choose a reason for hiding this comment

Snuffleupagus commented Sep 4, 2016 • edited Loading

timvandermeij commented Sep 4, 2016

pdfjsbot commented Sep 4, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Sep 4, 2016

From: Bot.io (Linux)

Success

Published

timvandermeij commented Sep 4, 2016

pdfjsbot commented Sep 4, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Sep 4, 2016

From: Bot.io (Windows)

Received

pdfjsbot commented Sep 4, 2016

From: Bot.io (Windows)

Use a `WeakMap` in `src/display/text_layer.js` #7588

Use a `WeakMap` in `src/display/text_layer.js` #7588

timvandermeij commented Sep 2, 2016 •

edited

Loading

timvandermeij commented Sep 2, 2016 •

edited

Loading

Snuffleupagus Sep 3, 2016 •

edited

Loading

Snuffleupagus Sep 4, 2016 •

edited

Loading

Snuffleupagus commented Sep 4, 2016 •

edited

Loading