Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removes global PDFJS usage from the src/core/. #7053

Merged
merged 4 commits into from
Mar 25, 2016

Conversation

yurydelendik
Copy link
Contributor

No description provided.

@yurydelendik
Copy link
Contributor Author

TODOs:

  • move shared/global.js to display/global.js
  • test with examples and in pdfjs-dist configuration
  • transfer verbosity from main thread to the worker's
  • refactor PDFJS.WorkerMessageHandler
  • move some tests from test/unit/util_spec.js into test/unit/dom_utils_spec.js


var PDFJS = sharedGlobal.PDFJS;

var bidi = PDFJS.bidi = (function bidiClosure() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that this is gone, will the export line at the end of the file still work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line 120 below defines bidi function

@@ -526,7 +530,7 @@ var PDFDocument = (function PDFDocumentClosure() {
var pageFactory = {
createPage: function (pageIndex, dict, ref, fontCache) {
return new Page(self.pdfManager, self.xref, pageIndex, dict, ref,
fontCache);
fontCache, self.evaluatorOptions);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.pdfManage.evaluatorOptions ?

@Snuffleupagus
Copy link
Collaborator

Some of the functions that are moved from src/shared/util.js into src/display/dom_utils.js have unit tests. For consistency, can those tests be moved from test/unit/util_spec.js into test/unit/dom_utils_spec.js instead?

@Snuffleupagus
Copy link
Collaborator

The test failures are most likely caused by not passing in the CMap options when doing new Font(...) in evaluator.js, see evaluator.js#L1992 and evaluator.js#L2085.

Also, a nit, but shouldn't it ideally be named cMapOptions (note the upper-case M), such that it's consistent with the current cMapUrl/cMapPacked properties (given that these are Character Map related, as opposed to the also existing TrueType cmap table).

@yurydelendik
Copy link
Contributor Author

The test failures are most likely caused by not passing in the CMap options when doing new Font(...) in evaluator.js, see evaluator.js#L1992 and evaluator.js#L2085.

That was it (plus one another thing with passing options as argument in LocalPdfManager)

Also, a nit, but shouldn't it ideally be named cMapOptions (note the upper-case M), such that it's consistent with the current cMapUrl/cMapPacked properties

Renamed

/botio test

@yurydelendik
Copy link
Contributor Author

/botio test

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

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

Live output at: http://107.21.233.14:8877/02ad5968ac4805b/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux)


Failed

Full output at http://107.21.233.14:8877/401b4802a8f4a97/output.txt

Total script time: 13.78 mins

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

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

}
}(this, function (exports, sharedUtil, displayPatternHelper, displayWebGL) {
}(this, function (exports, sharedUtil, displayDOMUtils, displayPatternHelper,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are all the changes in this file necessary? As far as I can see you're just adding displayDOMUtils but not using it in the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

canvas.js has some PDFJS references (e.g. PDFJS.hasCanvasTypedArrays) and dom_utils.js has these -- it will be changed once we move all display/ PDFJS references in one file (display/global.js)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thanks for the clarification!

@pdfjsbot
Copy link

From: Bot.io (Linux)


Failed

Full output at http://107.21.233.14:8877/20eacbdff32384a/output.txt

Total script time: 3.92 mins

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

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

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://107.22.172.223:8877/027a5953f547b94/output.txt

Total script time: 20.47 mins

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

@yurydelendik
Copy link
Contributor Author

TEST-PASS | load test issue6108 | in firefox

Firefox is often stops after the test above: issue6961 - the #6961 is a performance issue. Maybe timeout needs to be increased. The problem does not appear when run on the botio-windows from command line. Will continue trying using gh botio interface with increased timeout.

/botio-windows test

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://107.22.172.223:8877/8085979e5920ad3/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://107.22.172.223:8877/8085979e5920ad3/output.txt

Total script time: 19.74 mins

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

@yurydelendik
Copy link
Contributor Author

/botio-windows test

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://107.22.172.223:8877/6eabc43343852d0/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://107.22.172.223:8877/6eabc43343852d0/output.txt

Total script time: 19.77 mins

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

@yurydelendik
Copy link
Contributor Author

/botio test

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

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

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

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

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

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

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

Total script time: 19.98 mins

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

@yurydelendik
Copy link
Contributor Author

/botio-windows test

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://107.22.172.223:8877/7b0f60e7be9f89a/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux)


Failed

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

Total script time: 60.00 mins

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://107.22.172.223:8877/7b0f60e7be9f89a/output.txt

Total script time: 19.83 mins

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

@yurydelendik
Copy link
Contributor Author

/botio test

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

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

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

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

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

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

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

Total script time: 19.98 mins

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

@pdfjsbot
Copy link

From: Bot.io (Linux)


Success

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

Total script time: 22.05 mins

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

@timvandermeij
Copy link
Contributor

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

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

Live output at: http://107.21.233.14:8877/7181a4628b9ff42/output.txt

@timvandermeij
Copy link
Contributor

/botio test

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

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

Live output at: http://107.21.233.14:8877/471377ca2a8b7b1/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

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

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

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

Total script time: 19.78 mins

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

@pdfjsbot
Copy link

From: Bot.io (Linux)


Success

Full output at http://107.21.233.14:8877/471377ca2a8b7b1/output.txt

Total script time: 22.11 mins

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

@brendandahl brendandahl merged commit df7afcf into mozilla:master Mar 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants