Fix #10554, #11059: remove check for .pdf in URL extensions, prefer response headers #13587
Conversation
| @@ -278,6 +279,20 @@ describe('frame tests', function () { | |||
| .url(url) | |||
| .waitForVisible('#viewerContainer') | |||
| }) | |||
|
|
|||
| it('loads HTML properly despite having .pdf extension', function * () { | |||
There was a problem hiding this comment.
thanks for adding a test! unfortunately our automated webdriver test environment is totally broken at the moment, so instead i ran the custom server and manually loaded the .pdf HTML file. confirmed that it is broken in latest release and fixed with this PR.
|
@humphd this needs a rebase since some of the urlutil tests were moved into a different file |
|
@diracdeltas noted. I'll push an update next week. Thanks for the review. |
|
Rebased and updated urlutil test change. |
|
Is this abandoned? |
|
No. It's fixed and rebased. I don't have the ability to land it, so you should ask the maintaners. I assume the Chrome rewrite work occupied them.On Jul 7, 2018 5:25 PM, "Jason B." <notifications@github.com> wrote:Is this abandoned?
—You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@humphd it needs a rebase actually. once that's done i'll move the milestone to 0.24 and merge |
|
@diracdeltas, @bsclifton I've rebased again, and this should be good to go now. Let me know if you need me to do more, or something different. |
| yield this.app.client | ||
| .tabByIndex(0) | ||
| .url(url) | ||
| .waitForUrl(url) |
There was a problem hiding this comment.
this test needs a .windowByUrl(Brave.browserWindowUrl) here to switch from the page context into the Brave window context or else it fails because there's no activeTabTitle component found.
There was a problem hiding this comment.
@diracdeltas OK, I've added this in 0224cd6.
|
Excellent, thanks @diracdeltas, and thanks for the reviews. Glad to contribute, love Brave. Hope I can do more. |
bsclifton
modified the milestones:
0.24.x (Developer Channel),
0.23.x Release 4 w/ Chromium 68 (BETA Channel)
|
Uplifted to 0.23.x with 34329d2 |
This fixes the issue in Brave where URLs that contain
.pdfin a file's extension are automatically assumed to be PDF resources, and get loaded with PDF.js. This turns out to be problematic for sites like Dropbox or GitHub, which allow user generated content to be named with a.pdfextension, but served as HTML (e.g., with a custom UI wrapped around it).In debugging this, I found that the bug was introduced in #8366 as a partial fix for #8364. The underlying issue has since been mostly fixed in other code, and all I needed to do was remove the temporary URL rewriting code in order to let requests get properly inspected. I've tested with URLs and cases from the original bug, as well as current ones, and all seem to work as expected.
I've also attempted to add a regression test for the case of a
.pdfURL being served withtext/html. The underlying test server didn't support overridingcontent-type, so I also included a fix to allow it. I realize you might want this done another way, or not at all. I would have upstreamed a fix tonode-static, but it seems mostly unmaintained. I'm not familiar with all of your test code, so please let me know if I can improve or correct anything I've done.Finally, I've written this up as a walkthrough for my students, who are learning open source development. Quite a few of them are either working on Brave, or hoping to, so I wanted to give them some guidance by fixing a bug myself and writing about the process. You can see that at https://github.com/humphd/browser-laptop/tree/good-first-experience-issue-10554#walkthrough-fixing-a-bug-in-the-brave-browser. I'll show them this next week.