Re-factor `PDFDocumentProperties` such that it's properly reset when a new PDF file is opened (issue 8371) #8381

Snuffleupagus · 2017-05-05T14:35:13Z

This patch contains the following improvements:

Only fetch the various document properties once per PDF file opened, and cache the result (in a frozen object).
Always update the entire dialog at once, to prevent inconsistent UI state (issue 8371).
Ensure that the dialog, and all its internal properties, are reset when PDFViewerApplication.close is called.
Inline, and re-factor, the getProperties method in open, since that's the only call-site.
Always overwrite the fileSize with the value obtained from pdfDocument.getDownloadInfo, to ensure that it's correct.
ES6-ify the code that's touched in this patch.

Fixes #8371.

timvandermeij · 2017-05-05T21:54:52Z

/botio-linux preview

timvandermeij

In general this patch looks good, but I've got some minor questions.

timvandermeij · 2017-05-05T21:56:43Z

web/pdf_document_properties.js

    OverlayManager.register(this.overlayName, this.container,
                            this.close.bind(this));
  }

  /**
   * Open the document properties overlay.
+   *
+   * Note that since the document properties are fetched asynchronously,
+   * the dialog will *only* be updated if it's still open when the data arrives.


I'm a bit confused here, so maybe this needs to be rephrased a bit. If the dialog is closed (not visible), then it will still be updated in the background for the next time you open it. Open https://mozilla.github.io/pdf.js/web/viewer.html, wait two seconds and open the document properties and you'll immediately see the data. Or is that not what you meant here?

If the dialog is closed (not visible), then it will still be updated in the background for the next time you open it.

That should be the opposite of what the code does, both currently and with this patch.
Since we always re-fetch the data, and update the dialog every time it's opened, there's no point to updating anything if the dialog has been closed by the time e.g. this.pdfDocument.getMetadata() is resolved. (Keep in mind that all the operations involved here are asynchronous.)

So basically, I was trying to provide a general comment for these checks:

if (OverlayManager.active !== this.overlayName) { return; }

Open https://mozilla.github.io/pdf.js/web/viewer.html, wait two seconds and open the document properties and you'll immediately see the data.

Noting gets loaded until the dialog is successfully opened, so what you describe as "immediately" just seem instantaneous since two seconds after loading the tracemonkey.pdf file, there's probably not many heavy operations still occurring. So the asynchronous fetching of the various document properties thus becomes very fast.

Or is that not what you meant here?

No, that's not really what I meant :-)
So any suggestions on making this easier to understand would be helpful!
(I could also move, or even just remove, the comment if it doesn't help make sense of the code.)

Good point! I understand what you mean now. The comment itself looks clear after reading it again with this in mind, but I would indeed suggest to move it above the first OverlayManager check as the comment describes why we need such a check. Other than moving it, the wording seems fine to me.

timvandermeij · 2017-05-05T22:02:18Z

web/pdf_document_properties.js

+        if (OverlayManager.active !== this.overlayName) {
+          return;
+        }
+        let content = {


I think this is a more general question and not completely related to this patch per se, but what is the advantage of using let here over regular var? As I understood it, let is useful for block scoping, but is that necessary here?

As I understood it, let is useful for block scoping, but is that necessary here?

It's not technically necessary here, but you could also flip the question on its head and ask: "Why would you (generally) use var when let is now is a thing?" :-)
Basically, using let can actually help avoid some nasty bugs[1] related to e.g. hoisting of var.[2]

There's a lot of useful information in this MDN article: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let.

[1] See e.g. #8355 (comment), which shows how var can easily do something other than what you'd expect/intent.
[2] There's obviously cases where you want var specifically, but in most cases it doesn't matter, and then I think I'd favour let in new code. (The general Firefox frontend code-base also seem to be trending in this general direction, unless I'm not mistaken.)

I agree. That article is indeed really helpful and I see why let can be preferred over var now, even though it's not strictly necessary in this case. All is fine here then, thanks!

timvandermeij · 2017-05-05T22:03:49Z

web/pdf_document_properties.js

@@ -87,71 +113,71 @@ class PDFDocumentProperties {
   * @param {Object} pdfDocument - A reference to the PDF document.
   * @param {string} url - The URL of the document.
   */
-  setDocumentAndUrl(pdfDocument, url) {
+  setDocument(pdfDocument, url) {
+    if (this.pdfDocument) {


I think there is no real need to check for this here since this._reset() does the check too.

Moreover, shouldn't this.pdfDocument be private as well, i.e., this._pdfDocument, as it may only be set using this setter (like this._url)?

I think there is no real need to check for this here since this._reset() does the check too.

There's a very good reason, and it's that usually setFileSize is called before setDocument. If we'd just unconditionally call reset here, then that would complete remove the point of setFileSize.
Maybe some sort of comment might be in order here!?

Edit: Also, note how in other files, e.g. PDFViewer.setDocument, we check this.pdfDocument. So I suppose that if we should get rid of anything here, that ought to be the check in _reset() instead!?
The reason for the additional check in _reset(), was to avoid updating the UI when initializing the class (don't know if this is just a needless micro-optimization).

Moreover, shouldn't this.pdfDocument be private as well, i.e., this._pdfDocument, as it may only be set using this setter (like this._url)?

Hmm, I'm guessing that I probably didn't change that since this.pdfDocument is used in a lot of web/ files. Actually, I'm wondering if I should simply revert the naming in this patch to its prior state, since I'm not sure if marking these as "private" properties really adds anything?

The checks are fine then, but a small comment could indeed be helpful here to avoid any confusion.

I'm fine with reverting the naming changes. It will make the diff smaller and for now it does not add much. We can change all of them at a later time if we want, but then we should indeed do all of them instead of just a few.

timvandermeij · 2017-05-05T22:10:33Z

web/pdf_document_properties.js

+    if (this.pdfDocument) {
+      // Reset the dialog when a new PDF file is opened (issue 8371).
+      for (let id in this.fields) {
+        this._updateField(this.fields[id]);


I would expect this to be this._clearField(this.fields[id]) or this._updateField(this.fields[id], DEFAULT_FIELD_CONTENT) instead because using the same method for both updating and clearing seems a bit counterintuitive given the name and the signature of the method. In this case, it's not entirely clear to me from reading this line that it's performing a reset.

Moreover, shouldn't this.fields be private as well?

this._updateField(this.fields[id], DEFAULT_FIELD_CONTENT)

That'd be completely redundant, since _updateField will always fallback to DEFAULT_FIELD_CONTENT when no (valid) content is provided (there's a comment to that effect in the method).
Having two different methods for this just seem unnecessary to me; would a better comment help here?

You're right, but to me it just looked strange to update a field without providing a value. The fallback is implicit if you just read that line without going to the actual updateField method. Adding a comment above this line saying that the fallback value is used will indeed help to avoid confusion when reading this file.

Snuffleupagus · 2017-05-06T14:46:37Z

@timvandermeij I've just pushed a new version of the patch, which re-works quite a bit more of this code.

Please take a look at the updated #8381 (comment), where the first two points are a significant change from the first version of the patch (and the current master as well).

I've tried to take your questions/comments into account when writing, and commenting, the new code.
Unfortunately this largely invalidates your previous review, for which I apologise, but I hope that this new approach makes more sense!

timvandermeij · 2017-05-06T15:00:02Z

This new version looks really good and appears to resolve my comments. I'll do a full review later today.

timvandermeij · 2017-05-06T15:02:18Z

/botio-linux preview

…a new PDF file is opened (issue 8371) This patch contains the following improvements: - Only fetch the various document properties *once* per PDF file opened, and cache the result (in a frozen object). - Always update the *entire* dialog at once, to prevent inconsistent UI state (issue 8371). - Ensure that the dialog, and all its internal properties, are reset when `PDFViewerApplication.close` is called. - Inline, and re-factor, the `getProperties` method in `open`, since that's the only call-site. - Always overwrite the fileSize with the value obtained from `pdfDocument.getDownloadInfo`, to ensure that it's correct. - ES6-ify the code that's touched in this patch. Fixes 8371.

Snuffleupagus · 2017-05-07T08:22:17Z

@timvandermeij Sorry about the back and forth in this patch, but I realized that there were a number of opportunities to clean-up the code I've written.
I think that I'm now finally happy with the patch, since I've been able to simplify the code quite a bit compared to prior versions. Notably, the diff size for pdf_document_properties.js is almost cut in half.

pdfjsbot · 2017-05-07T08:22:43Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.67.70.0:8877/8c131461e2f3e5c/output.txt

pdfjsbot · 2017-05-07T08:24:44Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/8c131461e2f3e5c/output.txt

Total script time: 2.01 mins

Published

timvandermeij · 2017-05-07T15:44:11Z

Nice improvement; thank you for fixing and simplifying this!

…s-reset Re-factor `PDFDocumentProperties` such that it's properly reset when a new PDF file is opened (issue 8371)

Snuffleupagus added the viewer label May 5, 2017

Snuffleupagus requested review from yurydelendik and timvandermeij May 5, 2017 14:35

Snuffleupagus changed the title ~~Re-factor PDFDocumentProperties such it's properly reset when a new PDF file is opened (issue 8371)~~ Re-factor PDFDocumentProperties such that it's properly reset when a new PDF file is opened (issue 8371) May 5, 2017

timvandermeij reviewed May 5, 2017

View reviewed changes

Snuffleupagus added 4-work-in-progress and removed 4-work-in-progress labels May 6, 2017

Snuffleupagus changed the title ~~Re-factor PDFDocumentProperties such that it's properly reset when a new PDF file is opened (issue 8371)~~ [WIP] Re-factor PDFDocumentProperties such that it's properly reset when a new PDF file is opened (issue 8371) May 6, 2017

Snuffleupagus changed the title ~~[WIP] Re-factor PDFDocumentProperties such that it's properly reset when a new PDF file is opened (issue 8371)~~ Re-factor PDFDocumentProperties such that it's properly reset when a new PDF file is opened (issue 8371) May 7, 2017

Snuffleupagus removed the 4-work-in-progress label May 7, 2017

timvandermeij approved these changes May 7, 2017

View reviewed changes

timvandermeij merged commit b3e4361 into mozilla:master May 7, 2017

timvandermeij removed the request for review from yurydelendik May 7, 2017 15:44

Snuffleupagus deleted the document-properties-reset branch May 7, 2017 19:02

movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018

Merge pull request mozilla#8381 from Snuffleupagus/document-propertie…

c3b98bd

…s-reset Re-factor `PDFDocumentProperties` such that it's properly reset when a new PDF file is opened (issue 8371)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-factor `PDFDocumentProperties` such that it's properly reset when a new PDF file is opened (issue 8371) #8381

Re-factor `PDFDocumentProperties` such that it's properly reset when a new PDF file is opened (issue 8371) #8381

Snuffleupagus commented May 5, 2017 •

edited

Loading

timvandermeij commented May 5, 2017

timvandermeij left a comment

timvandermeij May 5, 2017

Snuffleupagus May 5, 2017 •

edited

Loading

timvandermeij May 6, 2017

timvandermeij May 5, 2017

Snuffleupagus May 5, 2017 •

edited

Loading

timvandermeij May 6, 2017

timvandermeij May 5, 2017

Snuffleupagus May 5, 2017 •

edited

Loading

timvandermeij May 6, 2017

timvandermeij May 5, 2017

Snuffleupagus May 5, 2017

timvandermeij May 6, 2017

Snuffleupagus commented May 6, 2017 •

edited

Loading

timvandermeij commented May 6, 2017

timvandermeij commented May 6, 2017

Snuffleupagus commented May 7, 2017

pdfjsbot commented May 7, 2017

pdfjsbot commented May 7, 2017

timvandermeij commented May 7, 2017

Re-factor PDFDocumentProperties such that it's properly reset when a new PDF file is opened (issue 8371) #8381

Re-factor PDFDocumentProperties such that it's properly reset when a new PDF file is opened (issue 8371) #8381

Conversation

Snuffleupagus commented May 5, 2017 • edited Loading

timvandermeij commented May 5, 2017

timvandermeij left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus May 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus May 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus May 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus commented May 6, 2017 • edited Loading

timvandermeij commented May 6, 2017

timvandermeij commented May 6, 2017

Snuffleupagus commented May 7, 2017

pdfjsbot commented May 7, 2017

From: Bot.io (Linux m4)

Received

pdfjsbot commented May 7, 2017

From: Bot.io (Linux m4)

Success

Published

timvandermeij commented May 7, 2017

Re-factor `PDFDocumentProperties` such that it's properly reset when a new PDF file is opened (issue 8371) #8381

Re-factor `PDFDocumentProperties` such that it's properly reset when a new PDF file is opened (issue 8371) #8381

Snuffleupagus commented May 5, 2017 •

edited

Loading

Snuffleupagus May 5, 2017 •

edited

Loading

Snuffleupagus May 5, 2017 •

edited

Loading

Snuffleupagus May 5, 2017 •

edited

Loading

Snuffleupagus commented May 6, 2017 •

edited

Loading