Refactor the getTempCanvas function in pdf_thumbnail_view.js to a factory, in preparation for ES6 conversion of the thumbnail related code
#8458
Conversation
… factory, in preparation for ES6 conversion of the thumbnail related code This patch intends to simplify future ES6 refactoring of the thumbnail code, since the current code isn't going to work well together with proper `Class`es.
There was a problem hiding this comment.
I have some questions about this, but I like the idea in general. Coincidentally I had been fiddling with this last weekend, but I couldn't figure out why it had to be so complex, so I left it in favor of the overlay manager PR. I'm hoping you can provide the answer for that concern :-)
| @@ -32,35 +32,52 @@ var THUMBNAIL_CANVAS_BORDER_WIDTH = 1; // px | |||
| * but increases the overall memory usage. The default value is false. | |||
| */ | |||
|
|
|||
| const TempImageFactory = (function TempImageFactoryClosure() { | |||
There was a problem hiding this comment.
Is there a particular reason to not make this a class like the other factories?
There was a problem hiding this comment.
Yes, I wanted to keep the actual canvas private, and that cannot be done using classes (at least not as they are specified in ES6, but hopefully that changes in a future EcmaScript version).
Also, please keep in mind that currently all PDFThumbnailViews share just one temporary canvas, so a class would be overkill here.
There was a problem hiding this comment.
Fair enough. Let's keep it this way then.
| @@ -405,11 +423,13 @@ var PDFThumbnailView = (function PDFThumbnailViewClosure() { | |||
| }, | |||
| }; | |||
|
|
|||
| PDFThumbnailView.cleanup = function() { | |||
There was a problem hiding this comment.
I do not really understand why we need this construction (and why we even have it at the moment). How I see it is that PDFThumbnailViewer manages all PDFThumbnailView instances. Wouldn't it be much easier and more logical to move this cleanup logic to PDFThumbnailViewer instead? PDFThumbnailViewer.cleanup would then:
- go over all
PDFThumbnailViewinstances and destroy them; - call the factory's
destroyCanvasmethod to delete the shared canvas.
Unless I'm missing something in my analysis, that should get rid of this construction altogether and have the PDFThumbnailViewer and PDFThumbnailView functionality completely split.
There was a problem hiding this comment.
Since the temporary canvas is only used by PDFThumbnailView, the PDFThumbnailViewer really shouldn't know anything about it (the current code isn't correct in my opinion).
What you're commenting on is a static method, i.e. one that is shared between all PDFThumbnailView instances, so there's nothing to iterate over here!
Edit: Note that the {PDFViewer, PDFThumbnailViewer}.cleanup methods are called from PDFViewerApplication.cleanup (see https://github.com/mozilla/pdf.js/blob/master/web/app.js#L1122). This is related to PDFRenderingQueue cleanup, and not destroying of documents on close (or similar).
There was a problem hiding this comment.
You're right. Maybe I'm worried for nothing; it looks a bit strange, but it's because the one canvas is shared among PDFThumbnailView instances. If there would be one per instance, it would be a lot simpler, but we don't want that for memory reasons. I'm fine with this then. Sorry for the noise!
There was a problem hiding this comment.
No worries; I suppose that I can see why this looks a bit strange on first sight!
With ES6 classes, this particular code becomes slightly less verbose, since static methods are placed among "regular" ones; using the static cleanup() { ... } notation.
There was a problem hiding this comment.
Good, I think having that method explicitly defined as static among the other ones would resolve the concern I had here in terms of code readability.
|
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 1 Live output at: http://54.67.70.0:8877/134460a85d92163/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/134460a85d92163/output.txt Total script time: 2.02 mins Published |
|
/botio lint |
From: Bot.io (Linux m4)ReceivedCommand cmd_lint from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/ca5638ffb8a2387/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_lint from @timvandermeij received. Current queue size: 1 Live output at: http://54.215.176.217:8877/2afa9728310e6f8/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/ca5638ffb8a2387/output.txt Total script time: 0.99 mins
|
|
Thank you for refactoring this! |
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/2afa9728310e6f8/output.txt Total script time: 3.35 mins
|
…geFactory Refactor the `getTempCanvas` function in `pdf_thumbnail_view.js` to a factory, in preparation for ES6 conversion of the thumbnail related code
This patch intends to simplify future ES6 refactoring of the thumbnail code, since the current code isn't going to work well together with proper
Classes.Edit: Somewhat simpler reviewing with https://github.com/mozilla/pdf.js/pull/8458/files?w=1.