Wraps mozL10n to async calls; splits firefox and generic l10n libs. #8394

yurydelendik · 2017-05-09T13:23:06Z

No description provided.

yurydelendik · 2017-05-12T14:57:21Z

Needed for #8402 too

yurydelendik · 2017-05-12T15:24:27Z

/botio-windows preview

pdfjsbot · 2017-05-12T15:24:28Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.215.176.217:8877/74930242f69674f/output.txt

pdfjsbot · 2017-05-12T15:43:37Z

From: Bot.io (Windows)

Success

Full output at http://54.215.176.217:8877/74930242f69674f/output.txt

Total script time: 19.14 mins

Published

Snuffleupagus

Generally this looks really good, thank you!

However, given the size of the PR and that there's a couple of parameter/naming issues which cause UI glitches, I'd like to look at/test this again once the comments are addressed.

Snuffleupagus · 2017-05-15T10:00:56Z

web/ui_utils.js

@@ -30,8 +30,29 @@ var RendererType = {
  SVG: 'svg',
 };

-var mozL10n = typeof document !== 'undefined' ?
-  (document.mozL10n || document.webL10n) : undefined;
+// replace {{arguments}} with their values


Grammar nit: // Replace {{arguments}} with their values.

Snuffleupagus · 2017-05-15T10:10:49Z

web/genericl10n.js

+  }
+
+  getDirection() {
+    return this._ready.then((webL10n) => {


Here, and below, can we avoid shadowing the webL10n name, given the var webL10n = document.webL10n; definition above?
Initially, this name confused me for a second, and naming this for example l10n instead wouldn't hurt in my opinion.

Snuffleupagus · 2017-05-15T10:17:46Z

web/annotation_layer_builder.js

@@ -24,6 +24,7 @@ import { SimpleLinkService } from './pdf_link_service';
 * @property {boolean} renderInteractiveForms
 * @property {IPDFLinkService} linkService
 * @property {DownloadManager} downloadManager
+ * @property {IL10n} l10n


Nit: For consistency, let's make this
* @property {IL10n} l10n - Localization service.

Snuffleupagus · 2017-05-15T10:18:18Z

web/pdf_thumbnail_viewer.js

@@ -26,6 +26,7 @@ var THUMBNAIL_SCROLL_MARGIN = -19;
 *   elements.
 * @property {IPDFLinkService} linkService - The navigation/linking service.
 * @property {PDFRenderingQueue} renderingQueue - The rendering queue object.
+ * @property {IL10n} l10n


Nit: For consistency, let's make this
* @property {IL10n} l10n - Localization service.

Snuffleupagus · 2017-05-15T10:20:11Z

web/interfaces.js

@@ -118,9 +118,39 @@ class IPDFAnnotationLayerFactory {
  /**
   * @param {HTMLDivElement} pageDiv
   * @param {PDFPage} pdfPage
+   * @param {IL10n} l10n,


Nit: Remove the comma.

Snuffleupagus · 2017-05-15T10:58:37Z

web/app.js

@@ -740,27 +772,27 @@ var PDFViewerApplication = {
   *                            and optionally a 'stack' property.
   */
  error: function pdfViewError(message, moreInfo) {
-    var moreInfoText = mozL10n.get('error_version_info',
+    var moreInfoText = [this.l10n.get('error_version_info',


Nit: Since you're already touching this line, let's change var to let.

Snuffleupagus · 2017-05-15T11:01:06Z

web/app.js

      'PDF documents are not allowed to use their own colors: ' +
      '\'Allow pages to choose their own colors\' ' +
-      'is deactivated in the browser.'));
+      'is deactivated in the browser.').then((msg) => {


Nit: Let's indent the above three lines with two more spaces, to improve readability here.

Snuffleupagus · 2017-05-15T11:12:31Z

web/app.js

+  _initializeL10n() {
+    if ((typeof PDFJSDev === 'undefined' ||
+        !PDFJSDev.test('FIREFOX || MOZCENTRAL')) &&
+        PDFViewerApplication.viewerPrefs['pdfBugEnabled']) {


The above condition isn't exactly equal to the current ones, since in master it's possible to change the #locale in dev-mode (i.e. when using gulp server) without having the 'pdfBugEnabled' pref set.
I think it'd be nice if we could keep that behaviour, so this needs to look something like:

if (((typeof PDFJSDev === 'undefined' || !PDFJSDev.test('PRODUCTION')) || this.viewerPrefs['pdfBugEnabled']) && (typeof PDFJSDev === 'undefined' || !PDFJSDev.test('FIREFOX || MOZCENTRAL'))) {

Obviously that isn't as readable as one would like, but it may be possible to simplify the condition somewhat.

Snuffleupagus · 2017-05-15T11:15:45Z

web/app.js

+      !PDFViewerApplication.supportsDocumentFonts) {
+    PDFJS.disableFontFace = true;
+    PDFViewerApplication.l10n.get('web_fonts_disabled', null,
+        'Web fonts are disabled: unable to use embedded PDF fonts.').


Nit: Let's indent this line with two more spaces, to improve readability here.

Snuffleupagus · 2017-05-15T11:22:56Z

web/password_prompt.js

      }

-      this.label.textContent = promptString;
+      promptString.then((msg) => {
+        this.label.textContent = promptString;


Needs to be this.label.textContent = msg;.

yurydelendik · 2017-05-18T16:55:21Z

@Snuffleupagus thanks for the review

/botio-windows preview

pdfjsbot · 2017-05-18T16:55:22Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.215.176.217:8877/21dff0cbfb857ad/output.txt

pdfjsbot · 2017-05-18T17:14:51Z

From: Bot.io (Windows)

Success

Full output at http://54.215.176.217:8877/21dff0cbfb857ad/output.txt

Total script time: 19.47 mins

Published

Snuffleupagus

I've left a couple of final comments; and please also add createL10n to DefaultExternalServices in https://github.com/mozilla/pdf.js/blob/master/web/app.js#L67.

Please note that when rebasing this, the changes to the hand_tool.js and secondary_toolbar.js files are no longer needed!

This looks really good, thanks for working on this!

Snuffleupagus · 2017-05-22T08:41:09Z

web/pdf_viewer.js

@@ -899,16 +902,19 @@ var PDFViewer = (function pdfViewer() {
     * @param {HTMLDivElement} pageDiv
     * @param {PDFPage} pdfPage
     * @param {boolean} renderInteractiveForms
+     * @param {L10n} l10n


Shouldn't the type be {IL10n} here, since that's used everywhere else?

Snuffleupagus · 2017-05-22T08:41:18Z

web/annotation_layer_builder.js

@@ -96,15 +96,18 @@ class DefaultAnnotationLayerFactory {
   * @param {HTMLDivElement} pageDiv
   * @param {PDFPage} pdfPage
   * @param {boolean} renderInteractiveForms
+   * @param {L10n} l10n


Shouldn't the type be {IL10n} here, since that's used everywhere else?

Snuffleupagus · 2017-05-22T08:43:25Z

web/app.js

+    // It is not possible to change locale for Firefox extension builds.
+    if (typeof PDFJSDev === 'undefined' || !PDFJSDev.test('PRODUCTION') ||
+        (!PDFJSDev.test('FIREFOX || MOZCENTRAL') &&
+         PDFViewerApplication.viewerPrefs['pdfBugEnabled'])) {


This can be this.viewerPrefs['pdfBugEnabled'], given the current scope here (and you're already using this.externalServices below).

Snuffleupagus · 2017-05-22T08:45:04Z

web/pdf_thumbnail_view.js

@@ -30,6 +30,7 @@ var THUMBNAIL_CANVAS_BORDER_WIDTH = 1; // px
 * @property {boolean} disableCanvasToImageConversion - (optional) Don't convert
 *   the canvas thumbnails to images. This prevents `toDataURL` calls,
 *   but increases the overall memory usage. The default value is false.
+ * @property {L10n} l10n - Localization service.


Shouldn't the type be {IL10n} here, since that's used everywhere else?

Snuffleupagus · 2017-05-22T08:45:31Z

web/pdf_thumbnail_view.js

@@ -64,6 +65,7 @@ var PDFThumbnailView = (function PDFThumbnailViewClosure() {
  /**
   * @constructs PDFThumbnailView
   * @param {PDFThumbnailViewOptions} options
+   * @param {IL10n} l10n


Please remove this line, since it's passed in with the options now.

Snuffleupagus · 2017-05-22T08:49:36Z

web/toolbar.js

+        return;
+      }
+
+      let selectScaleOption = (value, scale, l10n) => {


Please remove the l10n parameter, since it's now unused.

timvandermeij · 2017-05-27T13:08:02Z

external/webL10n/l10n.js

@@ -998,7 +998,6 @@ document.webL10n = (function(window, document, undefined) {
      loadLocale(lang, function() {
        if (callback)
          callback();
-        translateFragment();


Mention this change at https://github.com/yurydelendik/pdf.js/blob/6f76510a74f086de4fdb9a21a677649de9618c56/external/webL10n/l10n.js#L24 as well. If we update webL10n in the future, this change may be accidentally reverted if it's not mentioned there.

yurydelendik · 2017-05-31T14:38:48Z

/botio-windows preview

pdfjsbot · 2017-05-31T14:38:49Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.215.176.217:8877/28e5a95d406952c/output.txt

pdfjsbot · 2017-05-31T15:00:43Z

From: Bot.io (Windows)

Success

Full output at http://54.215.176.217:8877/28e5a95d406952c/output.txt

Total script time: 21.89 mins

Published

Wraps mozL10n to async calls; splits firefox and generic l10n libs.

yurydelendik added the 4-work-in-progress label May 9, 2017

yurydelendik force-pushed the l10n branch 3 times, most recently from 0406f58 to cbab3b4 Compare May 9, 2017 14:03

timvandermeij added viewer addon labels May 9, 2017

yurydelendik removed the 4-work-in-progress label May 12, 2017

yurydelendik force-pushed the l10n branch from cbab3b4 to 74b5639 Compare May 12, 2017 15:24

yurydelendik force-pushed the l10n branch 3 times, most recently from 6c67e24 to 8f549ef Compare May 12, 2017 16:54

Snuffleupagus self-requested a review May 12, 2017 21:47

Snuffleupagus reviewed May 15, 2017

View reviewed changes

yurydelendik force-pushed the l10n branch from 8f549ef to 6f76510 Compare May 18, 2017 16:54

Snuffleupagus approved these changes May 22, 2017

View reviewed changes

Snuffleupagus mentioned this pull request May 26, 2017

Convert the overlay manager to ES6 syntax #8450

Merged

timvandermeij reviewed May 27, 2017

View reviewed changes

yurydelendik force-pushed the l10n branch from 6f76510 to 3d8d69c Compare May 31, 2017 14:15

Wraps mozL10n to async calls; splits firefox and generic l10n libs.

5438ce9

yurydelendik force-pushed the l10n branch from 3d8d69c to 5438ce9 Compare May 31, 2017 14:22

Snuffleupagus merged commit 4593354 into mozilla:master May 31, 2017

Snuffleupagus mentioned this pull request May 31, 2017

Add a stub createL10n to DefaultExternalServices in app.js #8466

Merged

Snuffleupagus mentioned this pull request Jun 10, 2017

Prevent the Zoom dropdown from intermittently displaying an incorrect custom scale in Firefox (PR 8394 follow-up) #8510

Merged

Snuffleupagus mentioned this pull request Jul 5, 2017

Ensure that PDFFindBar._adjustWidth is called in all situations where the width of the findbar might have changed #8609

Merged

Snuffleupagus mentioned this pull request Aug 28, 2017

Ensure that PDFViewerApplication.error outputs proper messages in FIREFOX/MOZCENTRAL builds #8835

Merged

timvandermeij mentioned this pull request Mar 19, 2018

l10n get() not as a promise #9582

Closed

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

Merge pull request mozilla#8394 from yurydelendik/l10n

da9aa67

Wraps mozL10n to async calls; splits firefox and generic l10n libs.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wraps mozL10n to async calls; splits firefox and generic l10n libs. #8394

Wraps mozL10n to async calls; splits firefox and generic l10n libs. #8394

yurydelendik commented May 9, 2017

yurydelendik commented May 12, 2017

yurydelendik commented May 12, 2017

pdfjsbot commented May 12, 2017

pdfjsbot commented May 12, 2017

Snuffleupagus left a comment

Snuffleupagus May 15, 2017

Snuffleupagus May 15, 2017

Snuffleupagus May 15, 2017

Snuffleupagus May 15, 2017

Snuffleupagus May 15, 2017

Snuffleupagus May 15, 2017

Snuffleupagus May 15, 2017

Snuffleupagus May 15, 2017

Snuffleupagus May 15, 2017

Snuffleupagus May 15, 2017

yurydelendik commented May 18, 2017

pdfjsbot commented May 18, 2017

pdfjsbot commented May 18, 2017

Snuffleupagus left a comment

Snuffleupagus May 22, 2017

Snuffleupagus May 22, 2017

Snuffleupagus May 22, 2017

Snuffleupagus May 22, 2017

Snuffleupagus May 22, 2017

Snuffleupagus May 22, 2017

timvandermeij May 27, 2017

yurydelendik commented May 31, 2017

pdfjsbot commented May 31, 2017

pdfjsbot commented May 31, 2017

Wraps mozL10n to async calls; splits firefox and generic l10n libs. #8394

Wraps mozL10n to async calls; splits firefox and generic l10n libs. #8394

Conversation

yurydelendik commented May 9, 2017

yurydelendik commented May 12, 2017

yurydelendik commented May 12, 2017

pdfjsbot commented May 12, 2017

From: Bot.io (Windows)

Received

pdfjsbot commented May 12, 2017

From: Bot.io (Windows)

Success

Published

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

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

yurydelendik commented May 18, 2017

pdfjsbot commented May 18, 2017

From: Bot.io (Windows)

Received

pdfjsbot commented May 18, 2017

From: Bot.io (Windows)

Success

Published

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurydelendik commented May 31, 2017

pdfjsbot commented May 31, 2017

From: Bot.io (Windows)

Received

pdfjsbot commented May 31, 2017

From: Bot.io (Windows)

Success

Published