Restrict logging of errors with verbosity level

[andrewdacenko]

Allow control errors logging with PDFJS.verbosity level

[timvandermeij]

Can't we simplify this a lot by keeping a similar structure as

pdf.js/src/shared/util.js

Line 196 in fa0f09b

if (PDFJS.verbosity >= PDFJS.VERBOSITY_LEVELS.infos) {

? I see no need for another function and the bindings...

[andrewdacenko]

@timvandermeij

In my opinion this code looks much more elegant

// Fatal errors.
function err(fn) {
  if (PDFJS.verbosity >= PDFJS.VERBOSITY_LEVELS.errors) {
    fn();
  }
}

// Fatal errors that should trigger the fallback UI and halt execution by
// throwing an exception.
function error(msg) {
  // If multiple arguments were passed, pass them all to the log function.
  if (arguments.length > 1) {
    var logArguments = ['Error:'];
    logArguments.push.apply(logArguments, arguments);
    err(console.log.apply.bind(console.log, console, logArguments));
    // Join the arguments into a single string for the lines below.
    msg = [].join.call(arguments, ' ');
  } else {
    err(console.log.bind(console, 'Error: ' + msg));
  }
  err(console.log.bind(console, backtrace()));
  UnsupportedManager.notify(UNSUPPORTED_FEATURES.unknown);
  throw new Error(msg);
}

than this one

// Fatal errors that should trigger the fallback UI and halt execution by
// throwing an exception.
function error(msg) {
  // If multiple arguments were passed, pass them all to the log function.
  if (arguments.length > 1) {
    var logArguments = ['Error:'];
    logArguments.push.apply(logArguments, arguments);
    if (PDFJS.verbosity >= PDFJS.VERBOSITY_LEVELS.errors) {
      console.log.apply(console, logArguments);
    }
    // Join the arguments into a single string for the lines below.
    msg = [].join.call(arguments, ' ');
  } else {
    if (PDFJS.verbosity >= PDFJS.VERBOSITY_LEVELS.errors) {
      console.log('Error: ' + msg);
    }
  }
  if (PDFJS.verbosity >= PDFJS.VERBOSITY_LEVELS.errors) {
    console.log(backtrace());
  }
  UnsupportedManager.notify(UNSUPPORTED_FEATURES.unknown);
  throw new Error(msg);
}

Please, let me now, which you prefer.

[yurydelendik]

Please, let me now, which you prefer.

Neither solution above look good. Can you review calls of error() function and make it as a single argument function (similar to info/warn)? If multiple arguments are used, make it something like error(formatString("test", 1)) and add an utility function formatString.

Currently, the patch contains err() which is not really useful by itself.

[Snuffleupagus]

Can you review calls of error() function and make it as a single argument function (similar to info/warn)?

A very quick look through the code-base seem to indicate that no caller of error passes in more than one argument, but a thorough review is obviously necessary.
Since a fair number of callers are already pre-concatenating strings themselves, before calling error, I fully support the idea of removing the ability to pass multiple arguments to error.

[andrewdacenko]

@yurydelendik , @timvandermeij , @Snuffleupagus Reviewed code and found no caller with multiple arguments. Changed code to simple

// Fatal errors that should trigger the fallback UI and halt execution by
// throwing an exception.
function error(msg) {
  if (PDFJS.verbosity >= PDFJS.VERBOSITY_LEVELS.errors) {
    console.log(msg);
    console.log(backtrace());
  }
  UnsupportedManager.notify(UNSUPPORTED_FEATURES.unknown);
  throw new Error(msg);
}

multiple console logs for separate lines. Or can be done with "\n".

[Snuffleupagus]

This line should be console.log('Error: ' + msg);.

[Snuffleupagus]

@andrewdacenko Please squash the commits into one, see: https://github.com/mozilla/pdf.js/wiki/Squashing-Commits.

[andrewdacenko]

@Snuffleupagus Done.

@timvandermeij , @yurydelendik thanks for your attention )

[Snuffleupagus]

/botio test

[pdfjsbot]

From: Bot.io (Windows)

---

Received

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

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

[pdfjsbot]

From: Bot.io (Linux)

---

Received

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

Live output at: http://107.21.233.14:8877/7d10084e29f7068/output.txt

[pdfjsbot]

From: Bot.io (Windows)

---

Failed

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

Total script time: 2.87 mins

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

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

[pdfjsbot]

From: Bot.io (Linux)

---

Success

Full output at http://107.21.233.14:8877/7d10084e29f7068/output.txt

Total script time: 22.24 mins

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

[yurydelendik]

/botio-windows test

[pdfjsbot]

From: Bot.io (Windows)

---

Received

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

Live output at: http://107.22.172.223:8877/2c6750c128842c7/output.txt

[pdfjsbot]

From: Bot.io (Windows)

---

Success

Full output at http://107.22.172.223:8877/2c6750c128842c7/output.txt

Total script time: 17.92 mins

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

[Snuffleupagus]

Thanks for the patch!