Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

move pdf.js loading logic into browser-laptop #12138

Merged
merged 1 commit into from
Dec 19, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 20 additions & 7 deletions app/extensions.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ const contextMenus = require('./browser/extensions/contextMenus')
const extensionActions = require('./common/actions/extensionActions')
const config = require('../js/constants/config')
const appConfig = require('../js/constants/appConfig')
const messages = require('../js/constants/messages')
const {fileUrl} = require('../js/lib/appUrlUtil')
const {getExtensionsPath, getBraveExtUrl, getBraveExtIndexHTML} = require('../js/lib/appUrlUtil')
const {getSetting} = require('../js/settings')
Expand All @@ -17,7 +16,7 @@ const fs = require('fs')
const path = require('path')
const l10n = require('../js/l10n')
const {bravifyText} = require('./renderer/lib/extensionsUtil')
const {ipcMain, componentUpdater, session} = require('electron')
const {componentUpdater, session} = require('electron')

// Takes Content Security Policy flags, for example { 'default-src': '*' }
// Returns a CSP string, for example 'default-src: *;'
Expand Down Expand Up @@ -394,10 +393,6 @@ module.exports.init = () => {
}
})

ipcMain.on(messages.LOAD_URL_REQUESTED, (e, tabId, url) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious why this was removed? What is/was emitting this?

Copy link
Member Author

@diracdeltas diracdeltas Dec 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bsclifton Good question! This is actually being emitted in PDF.JS: https://github.com/brave/pdf.js/blob/master/extensions/chromium/pdfHandler.js#L136. However I moved all of that logic into this PR [1], so it no longer needs to be handled in browser-laptop once this is merged.

[1] Technically all of this code is still in the PDF.JS extension since we can't update browser-laptop and PDF.JS at the same time. However the manifest override in line 452 means that once someone updates to this version of browser-laptop, the deprecated background scripts in PDF.JS will no longer run.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah- perfect 😄 Thanks for the explanation!

appActions.loadURLRequested(tabId, url)
})

process.on('extension-load-error', (error) => {
console.error(error)
})
Expand Down Expand Up @@ -449,8 +444,26 @@ module.exports.init = () => {
}

let loadExtension = (extensionId, extensionPath, manifest = {}, manifestLocation = 'unpacked') => {
if (extensionId === config.PDFJSExtensionId) {
const isPDF = extensionId === config.PDFJSExtensionId
if (isPDF) {
manifestLocation = 'component'
// Override the manifest. TODO: Update manifest in pdf.js itself after enough
// clients have updated to Brave 0.20.x+
manifest = {
name: 'PDF Viewer',
manifest_version: 2,
version: '1.9.457',
description: 'Uses HTML5 to display PDF files directly in the browser.',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By having this here, do we lose any translation that was being done? Should this be loading using locale.translation()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bsclifton I don't think translation would be lost. It was also a literal string in the old manifest: https://github.com/brave/pdf.js/blob/master/extensions/chromium/manifest.json#L5. AFAIK passing a manifest object to the extension-loading function is the same as reading a manifest.json file, so if it was translated before, it would be translated now.

icons: {
'128': 'icon128.png',
'48': 'icon48.png',
'16': 'icon16.png'
},
permissions: ['storage', '<all_urls>'],
content_security_policy: "script-src 'self'; object-src 'self'",
incognito: 'split',
key: config.PDFJSExtensionPublicKey
}
}
if (!extensionInfo.isLoaded(extensionId) && !extensionInfo.isLoading(extensionId)) {
extensionInfo.setState(extensionId, extensionStates.LOADING)
Expand Down
96 changes: 88 additions & 8 deletions app/pdfJS.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,23 +5,102 @@

const UrlUtil = require('../js/lib/urlutil')
const Filtering = require('./filtering')
const config = require('../js/constants/config')
const appActions = require('../js/actions/appActions')
const getSetting = require('../js/settings').getSetting
const settings = require('../js/constants/settings')

const pdfjsBaseUrl = `chrome-extension://${config.PDFJSExtensionId}/`
const viewerBaseUrl = `${pdfjsBaseUrl}content/web/viewer.html`
const getViewerUrl = UrlUtil.getPDFViewerUrl
Copy link
Member Author

@diracdeltas diracdeltas Nov 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

most of the changes below are just copied from pdfhandler.js and extension-router.js in our pdf.js fork. Those scripts used to run as background scripts in the pdf.js extension but this PR deprecates them.


/**
* Check if the request is a PDF file.
* @param {Object} details First argument of the webRequest.onHeadersReceived
* event. The properties "responseHeaders" and "url"
* are read.
* @return {boolean} True if the resource is a PDF file.
*/
function isPDFFile (details) {
var header = details.responseHeaders && details.responseHeaders['Content-Type']
if (header) {
if (header.includes('application/pdf')) {
return true
}
if (header.includes('application/octet-stream')) {
if (details.url.toLowerCase().indexOf('.pdf') > 0) {
return true
}
var cdHeader = details.responseHeaders['Content-Disposition']
if (cdHeader && /\.pdf(["']|$)/i.test(cdHeader[0])) {
return true
}
}
}
}

/**
* @param {Object} details First argument of the webRequest.onHeadersReceived
* event. The property "url" is read.
* @return {boolean} True if the PDF file should be downloaded.
*/
function isPDFDownloadable (details) {
if (details.url.indexOf('pdfjs.action=download') >= 0) {
return true
}
// Display the PDF viewer regardless of the Content-Disposition header if the
// file is displayed in the main frame, since most often users want to view
// a PDF, and servers are often misconfigured.
// If the query string contains "=download", do not unconditionally force the
// viewer to open the PDF, but first check whether the Content-Disposition
// header specifies an attachment. This allows sites like Google Drive to
// operate correctly (#6106).
if (details.type === 'main_frame' &&
details.url.indexOf('=download') === -1) {
return false
}
var cdHeader = (details.responseHeaders &&
details.responseHeaders['Content-Disposition'])
return (cdHeader && /^attachment/i.test(cdHeader[0]))
}

/**
* Takes a set of headers, and set "Content-Disposition: attachment".
* @param {Object} details First argument of the webRequest.onHeadersReceived
* event. The property "responseHeaders" is read and
* modified if needed.
* @return {Object|undefined} The return value for the responseHeaders property
*/
function getHeadersWithContentDispositionAttachment (details) {
var headers = details.responseHeaders
var cdHeader = headers['Content-Disposition'] || []
cdHeader.push('attachment')
headers['Content-Disposition'] = cdHeader
return headers
}

const onBeforeRequest = (details) => {
const result = { resourceName: 'pdfjs' }
if (!(details.resourceType === 'mainFrame' &&
if (details.resourceType === 'mainFrame' &&
UrlUtil.isFileScheme(details.url) &&
UrlUtil.isFileType(details.url, 'pdf'))) {
return result
UrlUtil.isFileType(details.url, 'pdf')) {
appActions.loadURLRequested(details.tabId, getViewerUrl(details.url))
result.cancel = true
}
return result
}

const onHeadersReceived = (details) => {
const result = { resourceName: 'pdfjs' }
// Don't intercept POST requests until http://crbug.com/104058 is fixed.
if (details.resourceType === 'mainFrame' && details.method === 'GET' && isPDFFile(details)) {
if (isPDFDownloadable(details)) {
// Force download by ensuring that Content-Disposition: attachment is set
result.responseHeaders = getHeadersWithContentDispositionAttachment(details)
return result
}

// Replace frame with viewer
appActions.loadURLRequested(details.tabId, getViewerUrl(details.url))
result.cancel = true
}
appActions.loadURLRequested(details.tabId, `${viewerBaseUrl}?file=${encodeURIComponent(details.url)}`)
result.cancel = true
return result
}

Expand All @@ -31,5 +110,6 @@ const onBeforeRequest = (details) => {
module.exports.init = () => {
if (getSetting(settings.PDFJS_ENABLED)) {
Filtering.registerBeforeRequestFilteringCB(onBeforeRequest)
Filtering.registerHeadersReceivedFilteringCB(onHeadersReceived)
}
}
8 changes: 7 additions & 1 deletion js/lib/urlutil.js
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,12 @@ const UrlUtil = {
return url.replace(`chrome-extension://${pdfjsExtensionId}/`, '')
},

getPDFViewerUrl: function (url) {
const pdfjsBaseUrl = `chrome-extension://${pdfjsExtensionId}/`
const viewerBaseUrl = `${pdfjsBaseUrl}content/web/viewer.html`
return `${viewerBaseUrl}?file=${encodeURIComponent(url)}`
},

/**
* Converts a potential PDF URL to the PDFJS URL.
* XXX: This only looks at the URL file extension, not MIME types.
Expand All @@ -357,7 +363,7 @@ const UrlUtil = {
*/
toPDFJSLocation: function (url) {
if (url && UrlUtil.isHttpOrHttps(url) && UrlUtil.isFileType(url, 'pdf')) {
return `chrome-extension://${pdfjsExtensionId}/${url}`
return UrlUtil.getPDFViewerUrl(url)
}
return url
},
Expand Down
12 changes: 11 additions & 1 deletion test/unit/lib/urlutilTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ describe('urlutil', function () {
describe('toPDFJSLocation', function () {
const baseUrl = 'chrome-extension://jdbefljfgobbmcidnmpjamcbhnbphjnb/'
it('pdf', function () {
assert.equal(urlUtil.toPDFJSLocation('http://abc.com/test.pdf'), baseUrl + 'http://abc.com/test.pdf')
assert.equal(urlUtil.toPDFJSLocation('http://abc.com/test.pdf'), baseUrl + 'content/web/viewer.html?file=http%3A%2F%2Fabc.com%2Ftest.pdf')
})
it('non-pdf', function () {
assert.equal(urlUtil.toPDFJSLocation('http://abc.com/test.pdf.txt'), 'http://abc.com/test.pdf.txt')
Expand All @@ -208,6 +208,16 @@ describe('urlutil', function () {
})
})

describe('getPDFViewerUrl', function () {
const baseUrl = 'chrome-extension://jdbefljfgobbmcidnmpjamcbhnbphjnb/content/web/viewer.html?file='
it('regular url', function () {
assert.equal(urlUtil.getPDFViewerUrl('http://example.com'), baseUrl + 'http%3A%2F%2Fexample.com')
})
it('file url', function () {
assert.equal(urlUtil.getPDFViewerUrl('file:///Users/yan/some files/test.pdf'), baseUrl + 'file%3A%2F%2F%2FUsers%2Fyan%2Fsome%20files%2Ftest.pdf')
Copy link

@pureooze pureooze Dec 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly a silly question, but will this path work on other machines? Seems to me the path is hardcoded.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pureooze since the function just builds the path (using the provided input), the actual input itself isn't important 😄 The important part to note is that the output is prefixed with baseUrl (and the parts afterword are URL encoded)

})
})

describe('getHostname', function () {
it('returns undefined if the URL is invalid', function () {
assert.equal(urlUtil.getHostname(null), undefined)
Expand Down