Re-factor `PDFFunction` to be class-like, and introduce a `classFactory` in `PDFDocument` that lazily creates an instance of `PDFFunction` #8931

Snuffleupagus · 2017-09-19T14:20:01Z

This requires us to pass around classFactory to quite a lot of existing code, however I don't see another way of handling this while still guaranteeing that we can access PDFFunction as freely as in the old code.

Please note that the patch passes all tests locally (unit, font, reference), and I very much hope that we have sufficient test-coverage for the code in question to catch any typos/mistakes in the re-factoring.

/cc @yurydelendik

Snuffleupagus · 2017-09-22T10:56:44Z

I just noticed that a fair number of function calls in ColorSpace were accidentally missing the PDFFunctionFactory parameter in a prior version of the PR, which has now been addressed.

However looking at the patch, in particular the code in src/core/colorspace.js, I'm starting to wonder if we should attempt a similar re-factoring of ColorSpace (by creating it lazily using a factory) to avoid having to clutter the code in this way?

yurydelendik · 2017-09-22T15:18:13Z

However looking at the patch, in particular the code in src/core/colorspace.js, I'm starting to wonder if we should attempt a similar re-factoring of ColorSpace (by creating it lazily using a factory) to avoid having to clutter the code in this way?

How likely this will cause a regression? Shall we uplift the patch?

Can you change PDFFunctionFactory to pdfFunctionFactory ?

Snuffleupagus · 2017-09-22T19:06:26Z

How likely this will cause a regression?

Having looked quickly at actually doing this, I think that the answer is that the risk is simply too high if we do everything at once (considering the size and scope of the necessary changes).
Edit: master...Snuffleupagus:classFactory-ColorSpace

Shall we uplift the patch?

If you mean to Firefox 57, then I'd say that it's not necessary at all. Considering that this PR fixes a small (potential) inconsistency when multiple documents are opened, which doesn't happen in FIREFOX/MOZCENTRAL builds, we should be good here.

Can you change PDFFunctionFactory to pdfFunctionFactory ?

Since I still would want to look into doing a similar conversion for ColorSpace later on, I figured that using the name classFactory instead would be good. Currently it only contains a getPDFFunction method, but in the future it could also hold getColorSpace (and possibly more).

Snuffleupagus · 2017-09-28T09:43:37Z

/botio test

pdfjsbot · 2017-09-28T09:43:37Z

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

pdfjsbot · 2017-09-28T09:43:37Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.215.176.217:8877/20e71bf13d84034/output.txt

pdfjsbot · 2017-09-28T10:00:11Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.67.70.0:8877/b024d16e6a67602/output.txt

Total script time: 16.55 mins

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

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

pdfjsbot · 2017-09-28T10:06:19Z

From: Bot.io (Windows)

Success

Full output at http://54.215.176.217:8877/20e71bf13d84034/output.txt

Total script time: 22.69 mins

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

Snuffleupagus · 2017-09-28T10:13:52Z

Please note: The failures above are known intermittents.

@yurydelendik Gentle review ping; as described in #8931 (comment), the only changes made since you last looked at this is:

Can you change PDFFunctionFactory to pdfFunctionFactory ?

Since I still would want to look into doing a similar conversion for ColorSpace later on, I figured that using the name classFactory instead would be good. Currently it only contains a getPDFFunction method, but in the future it could also hold getColorSpace (and possibly more).

yurydelendik

classFactory does not really look like a factory (due to the singelton/function). I recommend to change interface to be more factory-ish, e.g.

class PDFFunctionFactory {
  create(fn) { via .parse(fn) }
  createFromArray(array) { via .parseArray(array) }
  createFromIR(ir) { via .fromIR(ir) }
}

yurydelendik · 2017-09-28T12:56:32Z

src/core/document.js

@@ -37,14 +37,15 @@ var Page = (function PageClosure() {
  }

  function Page(pdfManager, xref, pageIndex, pageDict, ref, fontCache,


can we change classFactory to pdfFunctionFactory and also pass parameters as an object

…n `PDFImage`, and change a couple of methods to use Objects rather than plain parameters The `inline` parameter which is passed to a number of methods/functions in `PDFImage`, despite not actually being used. Its value is never checked, nor is it ever assigned to the current `PDFImage` instance (i.e. no `this.inline = inline` exists). Looking briefly at the history of this code, I was also unable to find a point in time where `inline` was being used. As far as I'm concerned, `inline` does nothing more than add clutter to already very unwieldy method/function signatures, hence why I'm proposing that we just remove it. To further simplify call-sites using `PDFImage`/`NativeImageDecoder`, a number of methods/functions are changed to take Objects rather than a bunch of (somewhat) randomly ordered parameters.

…ry` in `PDFDocument` that lazily creates an instance of `PDFFunction` *Follow-up to PR 8909.* This requires us to pass around `classFactory` to quite a lot of existing code, however I don't see another way of handling this while still guaranteeing that we can access `PDFFunction` as freely as in the old code. Please note that the patch passes all tests locally (unit, font, reference), and I *very* much hope that we have sufficient test-coverage for the code in question to catch any typos/mistakes in the re-factoring.

Snuffleupagus · 2017-09-28T14:37:02Z

classFactory does not really look like a factory (due to the singelton/function). I recommend to change interface to be more factory-ish, e.g.

class PDFFunctionFactory {
create(fn) { via .parse(fn) }
createFromArray(array) { via .parseArray(array) }
createFromIR(ir) { via .fromIR(ir) }
}

I'm not entirely sure that I understand completely what you're asking for here, and it feels like I'm just having problems "connecting the dots" so to speak.
To try and aid the discussion, I've pushed a partial and thus non-working WIP commit (to avoid doing a lot of work in the wrong direction), and I'd ask that you please check to see if this is approximately what you asked for.
If not, I'd very much appreciate a few additional pointers here!

yurydelendik · 2017-09-28T15:44:19Z

Sorry, by looking at our PDFFunction, I can see why it can be confusing -- it plays kind of role of a factory, but it has not clear definition. So we can refactor/rename PDFFunction to be PDFFunctionFactory and its method can have 'createXXXX' form as I mentioned above. The new PDFFunctionFactory(isEvalSupported) will create an instance of this factory, and this can be directly used as pdfFunctionFactory (without _getInstance). I hope this explains that a bit.

Snuffleupagus · 2017-09-28T18:29:06Z

@yurydelendik Is master...Snuffleupagus:PDFFunctionFactory-2 closer to what you had in mind?

yurydelendik · 2017-09-28T19:41:19Z

Is master...Snuffleupagus:PDFFunctionFactory-2 closer to what you had in mind?

It is what I had in mind. Thank you for making this happen.

Snuffleupagus · 2017-09-28T20:35:40Z

It is what I had in mind. Thank you for making this happen.

Thanks for all the feedback :-)
Closing as superseded by PR #8968.

Snuffleupagus added the core label Sep 19, 2017

Snuffleupagus requested a review from yurydelendik September 19, 2017 14:20

mozilla deleted a comment from pdfjsbot Sep 22, 2017

mozilla deleted a comment from pdfjsbot Sep 28, 2017

yurydelendik reviewed Sep 28, 2017

View reviewed changes

Snuffleupagus added 3 commits September 28, 2017 15:34

WIP

c95de13

Snuffleupagus closed this Sep 28, 2017

Snuffleupagus deleted the PDFFunctionFactory branch September 28, 2017 20:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-factor `PDFFunction` to be class-like, and introduce a `classFactory` in `PDFDocument` that lazily creates an instance of `PDFFunction` #8931

Re-factor `PDFFunction` to be class-like, and introduce a `classFactory` in `PDFDocument` that lazily creates an instance of `PDFFunction` #8931

Snuffleupagus commented Sep 19, 2017 •

edited

Loading

Snuffleupagus commented Sep 22, 2017 •

edited

Loading

yurydelendik commented Sep 22, 2017

Snuffleupagus commented Sep 22, 2017 •

edited

Loading

Snuffleupagus commented Sep 28, 2017

pdfjsbot commented Sep 28, 2017

pdfjsbot commented Sep 28, 2017

pdfjsbot commented Sep 28, 2017

pdfjsbot commented Sep 28, 2017

Snuffleupagus commented Sep 28, 2017

yurydelendik left a comment

yurydelendik Sep 28, 2017

Snuffleupagus commented Sep 28, 2017 •

edited

Loading

yurydelendik commented Sep 28, 2017 •

edited

Loading

Snuffleupagus commented Sep 28, 2017 •

edited

Loading

yurydelendik commented Sep 28, 2017

Snuffleupagus commented Sep 28, 2017

		@@ -37,14 +37,15 @@ var Page = (function PageClosure() {
		}

		function Page(pdfManager, xref, pageIndex, pageDict, ref, fontCache,

Re-factor PDFFunction to be class-like, and introduce a classFactory in PDFDocument that lazily creates an instance of PDFFunction #8931

Re-factor PDFFunction to be class-like, and introduce a classFactory in PDFDocument that lazily creates an instance of PDFFunction #8931

Conversation

Snuffleupagus commented Sep 19, 2017 • edited Loading

Snuffleupagus commented Sep 22, 2017 • edited Loading

yurydelendik commented Sep 22, 2017

Snuffleupagus commented Sep 22, 2017 • edited Loading

Snuffleupagus commented Sep 28, 2017

pdfjsbot commented Sep 28, 2017

From: Bot.io (Linux m4)

Received

pdfjsbot commented Sep 28, 2017

From: Bot.io (Windows)

Received

pdfjsbot commented Sep 28, 2017

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Sep 28, 2017

From: Bot.io (Windows)

Success

Snuffleupagus commented Sep 28, 2017

yurydelendik left a comment

Choose a reason for hiding this comment

yurydelendik Sep 28, 2017

Choose a reason for hiding this comment

Snuffleupagus commented Sep 28, 2017 • edited Loading

yurydelendik commented Sep 28, 2017 • edited Loading

Snuffleupagus commented Sep 28, 2017 • edited Loading

yurydelendik commented Sep 28, 2017

Snuffleupagus commented Sep 28, 2017

Re-factor `PDFFunction` to be class-like, and introduce a `classFactory` in `PDFDocument` that lazily creates an instance of `PDFFunction` #8931

Re-factor `PDFFunction` to be class-like, and introduce a `classFactory` in `PDFDocument` that lazily creates an instance of `PDFFunction` #8931

Snuffleupagus commented Sep 19, 2017 •

edited

Loading

Snuffleupagus commented Sep 22, 2017 •

edited

Loading

Snuffleupagus commented Sep 22, 2017 •

edited

Loading

Snuffleupagus commented Sep 28, 2017 •

edited

Loading

yurydelendik commented Sep 28, 2017 •

edited

Loading

Snuffleupagus commented Sep 28, 2017 •

edited

Loading