Ensure that we cancel any pending rendering operations when the viewer is closed (issue 7274) #7514
Conversation
Snuffleupagus
changed the title
|
@yurydelendik Review ping? /botio-linux preview |
| @@ -320,6 +331,7 @@ var PDFPageView = (function PDFPageViewClosure() { | |||
| draw: function PDFPageView_draw() { | |||
| if (this.renderingState !== RenderingStates.INITIAL) { | |||
| console.error('Must be in new state before drawing'); | |||
| return Promise.resolve(undefined); | |||
There was a problem hiding this comment.
This has a little bit weird meaning, can we replace it with reject or cache the promise returned below?
There was a problem hiding this comment.
I was simply trying to bring this code in line with the existing code in PDFThumbnailView, please see https://github.com/mozilla/pdf.js/blob/master/web/pdf_thumbnail_view.js#L277.
Do you want me to change both of them to e.g. Promise.reject(...) instead?
There was a problem hiding this comment.
I see. Not sure if we want to change both. For pages, we fall through to start new rendering (e.g. for different scale). For thumbs, we don't care about scale or if image is set after page rendering. Maybe we just leave it without change, just add comments for intent? Or, we reject in both cases, it try to catch places when we are failing during testing?
There was a problem hiding this comment.
Not sure if we want to change both. For pages, we fall through to start new rendering (e.g. for different scale). For thumbs, we don't care about scale or if image is set after page rendering.
Really good point about the differences between pages/thumbnails! After thinking about this, I'm no longer sure that we want to make too many functional changes here.
The only change I've made in the updated patch, is to add a this.reset(); call for the PDFPageView case, since I believe that we probably want to ensure that we reset various state on fall-through (to avoid weird state with e.g. multiple text/annotation layers).
Please do let me know if you don't think this is appropriate!
| @@ -135,15 +135,16 @@ var PDFThumbnailViewer = (function PDFThumbnailViewerClosure() { | |||
| this.thumbnails = []; | |||
| this._pagesRotation = 0; | |||
| this._pagesRequests = []; | |||
|
|
|||
| var container = this.container; | |||
| while (container.hasChildNodes()) { | |||
There was a problem hiding this comment.
this.container.textContent = ''; -- I think it will be easier to read (we can prefix it with the comment about the intent)
There was a problem hiding this comment.
Similar to the above, I was trying to make this file use the same form as the existing code in PDFViewer, please see https://github.com/mozilla/pdf.js/blob/master/web/pdf_viewer.js#L419.
Do you want both of those files changed here?
There was a problem hiding this comment.
Yeah, let's change both here.
There was a problem hiding this comment.
I've updated both files.
…nailViewer` are cancelled when the viewer is closed
…en the viewer is closed (issue 7274)
| if (true) { | ||
| return; | ||
| } | ||
| //#endif |
There was a problem hiding this comment.
We need to keep this if to preserve functionality in the non-build viewer
…id having to manually get that information in the event handler
|
/botio-linux preview |
From: Bot.io (Linux)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://107.21.233.14:8877/29bc541a93a2030/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/29bc541a93a2030/output.txt Total script time: 0.99 mins Published |
|
Merging with r=yurydelendik. Thank you for the patch! |
…ring-on-close Ensure that we cancel any pending rendering operations when the viewer is closed (issue 7274)
Tentative PR that fixes #7274.
@yurydelendik r?
This change is