Skip to content

Commit

Permalink
Re-factor PDFDocumentProperties such that it's properly reset when …
Browse files Browse the repository at this point in the history 
…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.
  • Loading branch information
@Snuffleupagus
Snuffleupagus committed May 7, 2017
1 parent c5b45d2 commit 8c0de6d
Show file tree
Hide file tree
Showing 4 changed files with 121 additions and 85 deletions.
4 changes: 2 additions & 2 deletions web/app.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -561,6 +561,7 @@ var PDFViewerApplication = {
this.pdfThumbnailViewer.setDocument(null);
this.pdfViewer.setDocument(null);
this.pdfLinkService.setDocument(null, null);
this.pdfDocumentProperties.setDocument(null, null);
}
this.store = null;
this.isInitialViewSet = false;
Expand Down Expand Up @@ -847,8 +848,6 @@ var PDFViewerApplication = {

this.pdfDocument = pdfDocument;

this.pdfDocumentProperties.setDocumentAndUrl(pdfDocument, this.url);

var downloadedPromise = pdfDocument.getDownloadInfo().then(function() {
self.downloadComplete = true;
self.loadingBar.hide();
Expand All @@ -869,6 +868,7 @@ var PDFViewerApplication = {
baseDocumentUrl = location.href.split('#')[0];
}
this.pdfLinkService.setDocument(pdfDocument, baseDocumentUrl);
this.pdfDocumentProperties.setDocument(pdfDocument, this.url);

var pdfViewer = this.pdfViewer;
pdfViewer.currentScale = scale;
Expand Down
179 changes: 106 additions & 73 deletions web/pdf_document_properties.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,12 @@
* limitations under the License.
*/

import { getPDFFileNameFromURL, mozL10n } from './ui_utils';
import { cloneObj, getPDFFileNameFromURL, mozL10n } from './ui_utils';
import { createPromiseCapability } from './pdfjs';
import { OverlayManager } from './overlay_manager';

const DEFAULT_FIELD_CONTENT = '-';

/**
* @typedef {Object} PDFDocumentPropertiesOptions
* @property {string} overlayName - Name/identifier for the overlay.
Expand All @@ -29,21 +31,16 @@ class PDFDocumentProperties {
/**
* @param {PDFDocumentPropertiesOptions} options
*/
constructor(options) {
this.overlayName = options.overlayName;
this.fields = options.fields;
this.container = options.container;
constructor({ overlayName, fields, container, closeButton, }) {
this.overlayName = overlayName;
this.fields = fields;
this.container = container;

this.rawFileSize = 0;
this.url = null;
this.pdfDocument = null;
this._reset();

// Bind the event listener for the Close button.
if (options.closeButton) {
options.closeButton.addEventListener('click', this.close.bind(this));
if (closeButton) { // Bind the event listener for the Close button.
closeButton.addEventListener('click', this.close.bind(this));
}
this._dataAvailableCapability = createPromiseCapability();

OverlayManager.register(this.overlayName, this.container,
this.close.bind(this));
}
Expand All @@ -52,9 +49,51 @@ class PDFDocumentProperties {
* Open the document properties overlay.
*/
open() {
let freezeFieldData = (data) => {
Object.defineProperty(this, 'fieldData', {
value: Object.freeze(data),
writable: false,
enumerable: true,
configurable: true,
});
};

Promise.all([OverlayManager.open(this.overlayName),
this._dataAvailableCapability.promise]).then(() => {
this._getProperties();
// If the document properties were previously fetched (for this PDF file),
// just update the dialog immediately to avoid redundant lookups.
if (this.fieldData) {
this._updateUI();
return;
}
// Get the document properties.
this.pdfDocument.getMetadata().then(({ info, metadata, }) => {
freezeFieldData({
'fileName': getPDFFileNameFromURL(this.url),
'fileSize': this._parseFileSize(this.maybeFileSize),
'title': info.Title,
'author': info.Author,
'subject': info.Subject,
'keywords': info.Keywords,
'creationDate': this._parseDate(info.CreationDate),
'modificationDate': this._parseDate(info.ModDate),
'creator': info.Creator,
'producer': info.Producer,
'version': info.PDFFormatVersion,
'pageCount': this.pdfDocument.numPages,
});
this._updateUI();

// Get the correct fileSize, since it may not have been set (if
// `this.setFileSize` wasn't called) or may be incorrectly set.
return this.pdfDocument.getDownloadInfo();
}).then(({ length, }) => {
let data = cloneObj(this.fieldData);
data['fileSize'] = this._parseFileSize(length);

freezeFieldData(data);
this._updateUI();
});
});
}

Expand All @@ -65,19 +104,6 @@ class PDFDocumentProperties {
OverlayManager.close(this.overlayName);
}

/**
* Set the file size of the PDF document. This method is used to
* update the file size in the document properties overlay once it
* is known so we do not have to wait until the entire file is loaded.
*
* @param {number} fileSize - The file size of the PDF document.
*/
setFileSize(fileSize) {
if (fileSize > 0) {
this.rawFileSize = fileSize;
}
}

/**
* Set a reference to the PDF document and the URL in order
* to populate the overlay fields with the document properties.
Expand All @@ -87,68 +113,75 @@ 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) {
this._reset();
this._updateUI(true);
}
if (!pdfDocument) {
return;
}
this.pdfDocument = pdfDocument;
this.url = url;

this._dataAvailableCapability.resolve();
}

/**
* @private
* Set the file size of the PDF document. This method is used to
* update the file size in the document properties overlay once it
* is known so we do not have to wait until the entire file is loaded.
*
* @param {number} fileSize - The file size of the PDF document.
*/
_getProperties() {
if (!OverlayManager.active) {
// If the dialog was closed before `_dataAvailableCapability` was
// resolved, don't bother updating the properties.
return;
setFileSize(fileSize) {
if (typeof fileSize === 'number' && fileSize > 0) {
this.maybeFileSize = fileSize;
}
// Get the file size (if it hasn't already been set).
this.pdfDocument.getDownloadInfo().then((data) => {
if (data.length === this.rawFileSize) {
return;
}
this.setFileSize(data.length);
this._updateUI(this.fields['fileSize'], this._parseFileSize());
});
}

// Get the document properties.
this.pdfDocument.getMetadata().then((data) => {
var content = {
'fileName': getPDFFileNameFromURL(this.url),
'fileSize': this._parseFileSize(),
'title': data.info.Title,
'author': data.info.Author,
'subject': data.info.Subject,
'keywords': data.info.Keywords,
'creationDate': this._parseDate(data.info.CreationDate),
'modificationDate': this._parseDate(data.info.ModDate),
'creator': data.info.Creator,
'producer': data.info.Producer,
'version': data.info.PDFFormatVersion,
'pageCount': this.pdfDocument.numPages
};

// Show the properties in the dialog.
for (var identifier in content) {
this._updateUI(this.fields[identifier], content[identifier]);
}
});
/**
* @private
*/
_reset() {
this.pdfDocument = null;
this.url = null;

this.maybeFileSize = 0;
delete this.fieldData;
this._dataAvailableCapability = createPromiseCapability();
}

/**
* Always updates all of the dialog fields, to prevent inconsistent UI state.
* NOTE: If the contents of a particular field is neither a non-empty string,
* nor a number, it will fall back to `DEFAULT_FIELD_CONTENT`.
* @private
*/
_updateUI(field, content) {
if (field && content !== undefined && content !== '') {
field.textContent = content;
_updateUI(reset = false) {
if (reset || !this.fieldData) {
for (let id in this.fields) {
this.fields[id].textContent = DEFAULT_FIELD_CONTENT;
}
return;
}
if (OverlayManager.active !== this.overlayName) {
// Don't bother updating the dialog if has already been closed,
// since it will be updated the next time `this.open` is called.
return;
}
for (let id in this.fields) {
let content = this.fieldData[id];
this.fields[id].textContent = (content || content === 0) ?
content : DEFAULT_FIELD_CONTENT;
}
}

/**
* @private
*/
_parseFileSize() {
var fileSize = this.rawFileSize, kb = fileSize / 1024;
_parseFileSize(fileSize = 0) {
let kb = fileSize / 1024;
if (!kb) {
return;
} else if (kb < 1024) {
Expand All @@ -167,14 +200,14 @@ class PDFDocumentProperties {
* @private
*/
_parseDate(inputDate) {
if (!inputDate) {
return;
}
// This is implemented according to the PDF specification, but note that
// Adobe Reader doesn't handle changing the date to universal time
// and doesn't use the user's time zone (they're effectively ignoring
// the HH' and mm' parts of the date string).
var dateToParse = inputDate;
if (dateToParse === undefined) {
return '';
}
let dateToParse = inputDate;

// Remove the D: prefix if it is available.
if (dateToParse.substring(0, 2) === 'D:') {
Expand Down
12 changes: 2 additions & 10 deletions web/preferences.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
* limitations under the License.
*/

import { cloneObj } from './ui_utils';

var defaultPreferences = null;
function getDefaultPreferences() {
if (!defaultPreferences) {
Expand All @@ -38,16 +40,6 @@ function getDefaultPreferences() {
return defaultPreferences;
}

function cloneObj(obj) {
var result = {};
for (var i in obj) {
if (Object.prototype.hasOwnProperty.call(obj, i)) {
result[i] = obj[i];
}
}
return result;
}

/**
* BasePreferences - Abstract base class for storing persistent settings.
* Used for settings that should be applied to all opened documents,
Expand Down
11 changes: 11 additions & 0 deletions web/ui_utils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -422,6 +422,16 @@ function normalizeWheelEventDelta(evt) {
return delta;
}

function cloneObj(obj) {
var result = {};
for (var i in obj) {
if (Object.prototype.hasOwnProperty.call(obj, i)) {
result[i] = obj[i];
}
}
return result;
}

/**
* Promise that is resolved when DOM window becomes visible.
*/
Expand Down Expand Up @@ -582,6 +592,7 @@ export {
MAX_AUTO_SCALE,
SCROLLBAR_PADDING,
VERTICAL_PADDING,
cloneObj,
RendererType,
mozL10n,
EventBus,
Expand Down

0 comments on commit 8c0de6d

Please sign in to comment.