Removes error() #8581

yurydelendik · 2017-06-28T20:53:48Z

Part of #8506

timvandermeij

In general this looks good. I really like the idea of specific exception classes.

timvandermeij · 2017-06-28T21:26:07Z

src/core/colorspace.js

@@ -771,8 +772,8 @@ var CalGrayCS = (function CalGrayCSClosure() {

    // Validate variables as per spec.
    if (this.XW < 0 || this.ZW < 0 || this.YW !== 1) {
-      error('Invalid WhitePoint components for ' + this.name +
-            ', no fallback available');
+      throw new FormatError('Invalid WhitePoint components for ' + this.name +


Let's use the $(this.name) syntax here as well.

Unable to use that since string must be broken in two line, and lint errors with "Strings must use singlequote"

timvandermeij · 2017-06-28T21:26:32Z

src/core/colorspace.js

@@ -909,8 +911,8 @@ var CalRGBCS = (function CalRGBCSClosure() {

    // Validate variables as per spec.
    if (XW < 0 || ZW < 0 || YW !== 1) {
-      error('Invalid WhitePoint components for ' + this.name +
-            ', no fallback available');
+      throw new FormatError('Invalid WhitePoint components for ' + this.name +


Let's use the $(this.name) syntax here as well.

timvandermeij · 2017-06-28T21:28:28Z

src/core/font_renderer.js

@@ -76,7 +76,7 @@ var FontRendererFactory = (function FontRendererFactoryClosure() {
      }
      return ranges;
    }
-    error('not supported cmap: ' + format);
+    throw new FormatError(`not supported cmap: ${format}`);


Nit: not supported -> unsupported

timvandermeij · 2017-06-28T21:31:06Z

src/core/image.js

-      error('Invalid image width: ' + this.width + ' or height: ' +
-            this.height);
+      throw new FormatError(`Invalid image width: ${this.width} or ` +
+                                `height: ${this.height}`);


The alignment is a bit off here.

timvandermeij · 2017-06-28T21:31:40Z

src/core/image.js

@@ -132,8 +135,8 @@ var PDFImage = (function PDFImageClosure() {
            colorSpace = Name.get('DeviceCMYK');
            break;
          default:
-            error('JPX images with ' + this.numComps +
-                  ' color components not supported.');
+            throw new Error('JPX images with ' + this.numComps +


Let's use the $(this.numComps) syntax here as well.

timvandermeij · 2017-06-28T21:45:11Z

src/display/canvas.js

@@ -553,7 +553,7 @@ var CanvasGraphics = (function CanvasGraphicsClosure() {
        ctx.putImageData(chunkImgData, 0, i * FULL_CHUNK_HEIGHT);
      }
    } else {
-      error('bad image kind: ' + imgData.kind);
+      throw new Error(`bad image kind: ${imgData.kind}`);


Shouldn't this be a FormatError?

It comes from already parsed data -- FormatError usually thrown as result of PDF parsing.

timvandermeij · 2017-06-28T21:45:50Z

src/display/pattern_helper.js

@@ -415,7 +414,7 @@ var TilingPattern = (function TilingPatternClosure() {
            context.strokeStyle = cssColor;
            break;
          default:
-            error('Unsupported paint type: ' + paintType);
+            throw new FormatError('Unsupported paint type: ' + paintType);


Let's use the $(paintType) syntax here as well.

timvandermeij · 2017-06-28T21:46:30Z

src/display/pattern_helper.js

@@ -140,8 +140,7 @@ var createMeshCanvas = (function createMeshCanvasClosure() {
        }
        break;
      default:
-        error('illigal figure');
-        break;
+        throw new Error('illigal figure');


Nit: illigal -> illegal

timvandermeij · 2017-06-28T21:49:07Z

src/shared/util.js

-// Fatal errors that should trigger the fallback UI and halt execution by
-// throwing an exception.
-function error(msg) {
-  if (verbosity >= VERBOSITY_LEVELS.errors) {


Do we still use the verbosity anywhere now? If not, we should remove all traces of it as well.

we will keep that for compatibility, also we may add error() back, but without throwing for recording critical stuff (e.g. recovery that we often hide under warn())

timvandermeij · 2017-06-28T21:49:28Z

src/shared/util.js

@@ -499,6 +481,18 @@ var XRefParseException = (function XRefParseExceptionClosure() {
  return XRefParseException;
 })();

+var FormatError = (function FormatErrorClosure() {


var -> let?

The initial comments I had have been resolved.

timvandermeij · 2017-06-30T21:48:08Z

In general this looks good to me. I won't be able to review this entirely in the next two weeks, so if @Snuffleupagus or @brendandahl could do a final review, that would be great so we can land it.

Snuffleupagus · 2017-07-06T11:51:21Z

/botio test

pdfjsbot · 2017-07-06T11:51:22Z

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/8e3d3d0ed96b06a/output.txt

pdfjsbot · 2017-07-06T11:51:22Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.215.176.217:8877/64797c8b6fc01c9/output.txt

pdfjsbot · 2017-07-06T12:07:56Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/8e3d3d0ed96b06a/output.txt

Total script time: 16.56 mins

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

pdfjsbot · 2017-07-06T12:20:51Z

From: Bot.io (Windows)

Success

Full output at http://54.215.176.217:8877/64797c8b6fc01c9/output.txt

Total script time: 29.46 mins

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

Snuffleupagus

r=me, with two minor comments:

The recently added throw statement in https://github.com/mozilla/pdf.js/blob/master/src/core/pattern.js#L808 should probably be a FormatError instead now.
I'm not certain that it's used anywhere, but let's convert error(...) in https://github.com/mozilla/pdf.js/blob/master/src/shared/fonts_utils.js too.

Looks good, thanks for doing this cleanup!

Removes error()

yurydelendik force-pushed the rm-error branch from 72232e8 to 250578a Compare June 28, 2017 20:59

timvandermeij added the core label Jun 28, 2017

yurydelendik force-pushed the rm-error branch from 250578a to 739f90d Compare June 28, 2017 21:22

timvandermeij previously requested changes Jun 28, 2017

View reviewed changes

yurydelendik force-pushed the rm-error branch 2 times, most recently from fe9393d to 565a186 Compare June 29, 2017 16:07

Snuffleupagus approved these changes Jul 7, 2017

View reviewed changes

yurydelendik force-pushed the rm-error branch from 565a186 to ecc6cc5 Compare July 7, 2017 14:38

Removes error()

d028c26

yurydelendik force-pushed the rm-error branch from ecc6cc5 to d028c26 Compare July 7, 2017 14:40

yurydelendik merged commit 7b4887d into mozilla:master Jul 7, 2017

yurydelendik deleted the rm-error branch July 7, 2017 14:43

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

Merge pull request mozilla#8581 from yurydelendik/rm-error

7328f3b

Removes error()

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removes error() #8581

Removes error() #8581

yurydelendik commented Jun 28, 2017

timvandermeij left a comment

timvandermeij Jun 28, 2017

yurydelendik Jun 28, 2017

timvandermeij Jun 28, 2017

timvandermeij Jun 28, 2017

timvandermeij Jun 28, 2017

timvandermeij Jun 28, 2017

timvandermeij Jun 28, 2017

yurydelendik Jun 29, 2017

timvandermeij Jun 28, 2017

timvandermeij Jun 28, 2017

timvandermeij Jun 28, 2017

yurydelendik Jun 29, 2017

timvandermeij Jun 28, 2017

timvandermeij commented Jun 30, 2017 •

edited

Loading

Snuffleupagus commented Jul 6, 2017

pdfjsbot commented Jul 6, 2017

pdfjsbot commented Jul 6, 2017

pdfjsbot commented Jul 6, 2017

pdfjsbot commented Jul 6, 2017

Snuffleupagus left a comment

Removes error() #8581

Removes error() #8581

Conversation

yurydelendik commented Jun 28, 2017

timvandermeij 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvandermeij commented Jun 30, 2017 • edited Loading

Snuffleupagus commented Jul 6, 2017

pdfjsbot commented Jul 6, 2017

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jul 6, 2017

From: Bot.io (Windows)

Received

pdfjsbot commented Jul 6, 2017

From: Bot.io (Linux m4)

Success

pdfjsbot commented Jul 6, 2017

From: Bot.io (Windows)

Success

Snuffleupagus left a comment

Choose a reason for hiding this comment

timvandermeij commented Jun 30, 2017 •

edited

Loading