Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure that a PDF is only downloaded once #102

Merged
merged 2 commits into from
Jul 24, 2019
Merged

Conversation

nswarup14
Copy link
Contributor

Fixes #54

Changes

  1. browser.py
  2. pdfviewer.py

Updates
At the present master HEAD (540c93f), if the mime_type, in __decide_policy_cb is application/pdf, a new GET request was being issued after setting up the PDF tab in pdfviewer.py/_download_from_http, where the context.download_uri(remote_uri) was the culprit.

Rather than make a new GET request, I used the response obtained from the first GET request to download the PDF directly, using the WebKit2.PolicyDecision.download(). The function also emits the download-started signal which is connected to pdfviewer.py/__download_started_cb, which handles the rest.

I found a thread in the WebKit2 mailing list (here), that seems to explain the issue in more detail.

@quozl @pro-panda Please let me know if I have approached this issue in the right step.

Thanks.

Tested on Ubuntu 18.04, Sugar v0.114

Copy link

@rhl-bthr rhl-bthr left a comment

Choose a reason for hiding this comment

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

Test via #54 (comment)

browser.py Outdated
response = WebKit2.ResponsePolicyDecision.get_response(policy_decision)
mimetype = WebKit2.URIResponse.get_mime_type(response)
response = policy_decision.get_response()
mimetype = response.get_mime_type()

if mimetype == 'application/pdf':
# FIXME: this causes two GET requests to the server; at

Choose a reason for hiding this comment

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

Remove FIXME

@quozl
Copy link
Contributor

quozl commented Jul 18, 2019

I've reviewed the patch. I can see the removal of download_uri, and the change from ignore to download, which makes sense. But I don't understand what the other changes are for, and the commit message didn't explain. (Please make sure the explanation and justification for changes is in the commit message, because that's what will end up in the git log, which is used by other developers through git bisect or git blame long after GitHub pull requests may have been archived.) I did re-read your pull request opening comment, but still didn't understand what the other changes are for.

@nswarup14
Copy link
Contributor Author

nswarup14 commented Jul 18, 2019

@quozl
Thanks
I will update the commit message as well.

I refactored the code which saved the metadata of the browse-activity in a journal instance. The state of a regular tab was a bytes object, which needs to be decoded and the state of a PDF tab was a regular dictionary. Hence I was handled both the cases in separate if-else cases.

I realize the other changes are not required after reading the documentation once again. I shall remove them immediately.

@quozl quozl merged commit 59f173b into sugarlabs:master Jul 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Every PDF is downloaded twice
3 participants