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

diracdeltas · 2017-11-29T22:54:55Z

fix #4651
probably fixes #2715 but ideally the workaround in brave/pdf.js should be removed too
may also fix other PDF loading errors

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.

Test Plan:

go to any page that contains PDF links such as https://letsencrypt.org/repository/
try to open the PDF links in a new session tab. it should work.

Reviewer Checklist:

Tests

Adequate test coverage exists to prevent regressions
Tests should be independent and work correctly when run individually or as a suite ref
New files have MPL2 license header

diracdeltas · 2017-11-29T22:56:00Z

app/pdfJS.js

 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


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.

codecov-io · 2017-11-29T23:17:16Z

Codecov Report

Merging #12138 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #12138      +/-   ##
==========================================
+ Coverage   55.29%    55.3%   +<.01%     
==========================================
  Files         276      276              
  Lines       26740    26745       +5     
  Branches     4310     4310              
==========================================
+ Hits        14786    14791       +5     
  Misses      11954    11954

Flag	Coverage Δ
#unittest	`55.3% <100%> (ø)`	⬆️

Impacted Files	Coverage Δ
js/lib/urlutil.js	`86.32% <100%> (+0.27%)`	⬆️

bsclifton · 2017-12-19T06:34:25Z

app/extensions.js

@@ -394,10 +393,6 @@ module.exports.init = () => {
    }
  })

-  ipcMain.on(messages.LOAD_URL_REQUESTED, (e, tabId, url) => {


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

@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.

Ah- perfect 😄 Thanks for the explanation!

bsclifton · 2017-12-19T06:39:27Z

app/pdfJS.js

+ *                         are read.
+ * @return {boolean} True if the resource is a PDF file.
+ */
+function isPdfFile (details) {


To match other places, this (and the function below) might be named with the same casing (ex: isPDFFile). If the functions aren't being exported, they could be changed to isFile and isDownloadable since this file is already named pdfJS.js

bsclifton · 2017-12-19T06:42:28Z

app/pdfJS.js

+ * @return {boolean} True if the resource is a PDF file.
+ */
+function isPdfFile (details) {
+  var header = details.responseHeaders['Content-Type']


nitpick: potentially falsey values could be weeded out to prevent null exception when called. For example:
var header = details && details.responseHeaders && details.responseHeaders['Content-Type']

bsclifton · 2017-12-19T06:54:18Z

app/extensions.js

+        name: 'PDF Viewer',
+        manifest_version: 2,
+        version: '1.9.457',
+        description: 'Uses HTML5 to display PDF files directly in the browser.',


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

@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.

bsclifton

Manually tested- works great 😄

Code reviewed, comments left. Otherwise, LGTM! 😄 👍

fix #4651 probably fixes #2715 but ideally the workaround in brave/pdf.js should be removed too may also fix other PDF loading errors

diracdeltas · 2017-12-19T19:31:50Z

@bsclifton ready for re-approval!

bsclifton

++ 😄

move pdf.js loading logic into browser-laptop

bsclifton · 2017-12-19T20:58:06Z

master d1a05f0
0.21.x 349a1f0
0.20.x 26abd99

pureooze · 2017-12-20T03:52:19Z

test/unit/lib/urlutilTest.js

+      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')


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

@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)

diracdeltas added this to the 0.20.x (Beta Channel) milestone Nov 29, 2017

diracdeltas self-assigned this Nov 29, 2017

diracdeltas requested a review from bsclifton November 29, 2017 22:54

diracdeltas commented Nov 29, 2017

View reviewed changes

diracdeltas added QA/test-plan-specified plugin/pdfjs labels Dec 4, 2017

bsclifton force-pushed the fix/integrated-pdfjs branch from 498bc75 to 68f188f Compare December 19, 2017 06:28

bsclifton reviewed Dec 19, 2017

View reviewed changes

bsclifton previously approved these changes Dec 19, 2017

View reviewed changes

move pdf.js loading logic into browser-laptop

ddf13d6

fix #4651 probably fixes #2715 but ideally the workaround in brave/pdf.js should be removed too may also fix other PDF loading errors

diracdeltas dismissed bsclifton’s stale review via ddf13d6 December 19, 2017 19:31

diracdeltas force-pushed the fix/integrated-pdfjs branch from 68f188f to ddf13d6 Compare December 19, 2017 19:31

bsclifton approved these changes Dec 19, 2017

View reviewed changes

bsclifton merged commit d1a05f0 into master Dec 19, 2017

bsclifton deleted the fix/integrated-pdfjs branch December 19, 2017 20:55

bsclifton added a commit that referenced this pull request Dec 19, 2017

Merge pull request #12138 from brave/fix/integrated-pdfjs

349a1f0

move pdf.js loading logic into browser-laptop

bsclifton added a commit that referenced this pull request Dec 19, 2017

Merge pull request #12138 from brave/fix/integrated-pdfjs

26abd99

move pdf.js loading logic into browser-laptop

pureooze mentioned this pull request Dec 19, 2017

Brave cannot load PDFs with JS PDF reader in session tabs #4651

Closed

pureooze reviewed Dec 20, 2017

View reviewed changes

diracdeltas mentioned this pull request Dec 21, 2017

PDFJS returns CORS error when PDF is loaded directly #2715

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

diracdeltas commented Nov 29, 2017 •

edited by bsclifton

Loading

diracdeltas Nov 29, 2017 •

edited

Loading

codecov-io commented Nov 29, 2017 •

edited

Loading

bsclifton Dec 19, 2017

diracdeltas Dec 19, 2017 •

edited

Loading

bsclifton Dec 19, 2017

bsclifton Dec 19, 2017

bsclifton Dec 19, 2017

bsclifton Dec 19, 2017

diracdeltas Dec 19, 2017

bsclifton left a comment

diracdeltas commented Dec 19, 2017

bsclifton left a comment

bsclifton commented Dec 19, 2017

pureooze Dec 20, 2017 •

edited

Loading

bsclifton Dec 20, 2017

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

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

Conversation

diracdeltas commented Nov 29, 2017 • edited by bsclifton Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

diracdeltas Nov 29, 2017 • edited Loading

Choose a reason for hiding this comment

codecov-io commented Nov 29, 2017 • edited Loading

Codecov Report

bsclifton Dec 19, 2017

Choose a reason for hiding this comment

diracdeltas Dec 19, 2017 • edited Loading

Choose a reason for hiding this comment

bsclifton Dec 19, 2017

Choose a reason for hiding this comment

bsclifton Dec 19, 2017

Choose a reason for hiding this comment

bsclifton Dec 19, 2017

Choose a reason for hiding this comment

bsclifton Dec 19, 2017

Choose a reason for hiding this comment

diracdeltas Dec 19, 2017

Choose a reason for hiding this comment

bsclifton left a comment

Choose a reason for hiding this comment

diracdeltas commented Dec 19, 2017

bsclifton left a comment

Choose a reason for hiding this comment

bsclifton commented Dec 19, 2017

pureooze Dec 20, 2017 • edited Loading

Choose a reason for hiding this comment

bsclifton Dec 20, 2017

Choose a reason for hiding this comment

diracdeltas commented Nov 29, 2017 •

edited by bsclifton

Loading

diracdeltas Nov 29, 2017 •

edited

Loading

codecov-io commented Nov 29, 2017 •

edited

Loading

diracdeltas Dec 19, 2017 •

edited

Loading

pureooze Dec 20, 2017 •

edited

Loading