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

Suppress dispatching to PDF viewer if plugins are disabled #9507

Merged
merged 7 commits into from May 22, 2017

Conversation

Projects
None yet
3 participants
@rreimann
Contributor

rreimann commented May 17, 2017

Suppress dispatching pdf resource requests to the native pdf viewer if plugins are disabled in BrowserWindow.webPreferences as suggested by @deepak1556 in #9412 (comment).

Note to the reviewer(s):
Since i've no clue about writing C++ code i tried to keep the changes as small as possible and skipped the following potential improvements:

  • web_contents_getter.Run() is now called twice during the request at ShouldInterceptResourceAsStream and at OnPdfResourceIntercepted. If i'd known the required syntax i would have called it just once and passed the resulting WebContents to OnPdfResourceIntercepted

  • After adding IsPluginsEnabled to the existing methods IsSandboxed and UsesNativeWindowOpen there are now three methods that seem widely redundant. I did't know if the preexisting redundancy was intentional so i didn't try to extract a common method for all of them.

Fixes #9412

@kevinsawicki kevinsawicki requested a review from deepak1556 May 17, 2017

@deepak1556

Thanks for this @rreimann , requires some minor changes.

content::WebContents* web_contents =
info->GetWebContentsGetterForRequest().Run();
if (mime_type == "application/pdf" && info->IsMainFrame() &&
WebContentsPreferences::IsPluginsEnabled(web_contents)) {

This comment has been minimized.

@deepak1556

deepak1556 May 18, 2017

Member

WebContentsPreferences is not thread safe, the check should be moved to OnPdfResourceIntercepted and implement a api::WebContents::DownloadURL like call when plugin is disabled so that will-download event of session is triggered.

if (!WebContentsPreferences::IsPluginsEnabled(web_contents)) {
    auto browser_context = web_contents->GetBrowserContext();
    auto download_manager =
      content::BrowserContext::GetDownloadManager(browser_context);

    download_manager->DownloadUrl(
        content::DownloadUrlParameters::CreateForWebContentsMainFrame(
            web_contents, original_url));
    return;
  }

This comment has been minimized.

@rreimann

rreimann May 19, 2017

Contributor

Thanks for the review. I moved the check in commit 5ce0e57

@@ -230,6 +230,22 @@ bool WebContentsPreferences::UsesNativeWindowOpen(
return use;
}
bool WebContentsPreferences::IsPluginsEnabled(

This comment has been minimized.

@deepak1556

deepak1556 May 18, 2017

Member

Its fine to have this helper, but like you suggested it would be better to have them unified into a single IsPreferenceEnabled. @kevinsawicki thoughts ?

This comment has been minimized.

@rreimann

rreimann May 19, 2017

Contributor

Extracted the redundant code in commit b85b701

@@ -199,7 +199,9 @@ void WebContentsPreferences::AppendExtraCommandLineSwitches(
}
}
bool WebContentsPreferences::IsSandboxed(content::WebContents* web_contents) {
bool WebContentsPreferences::IsPreferenceEnabled(

This comment has been minimized.

@deepak1556

deepak1556 May 19, 2017

Member

The helper can be moved to anonymous namespace.

@deepak1556

LGTM, thanks @rreimann !

@kevinsawicki

Left a few minor formatting comments.

@@ -199,7 +199,9 @@ void WebContentsPreferences::AppendExtraCommandLineSwitches(
}
}
bool WebContentsPreferences::IsSandboxed(content::WebContents* web_contents) {
bool WebContentsPreferences::IsPreferenceEnabled(
const std::string& attributeName,

This comment has been minimized.

@kevinsawicki

kevinsawicki May 19, 2017

Contributor

This should be attribute_name.

@@ -37,8 +37,11 @@ class WebContentsPreferences
static void AppendExtraCommandLineSwitches(
content::WebContents* web_contents, base::CommandLine* command_line);
static bool IsPreferenceEnabled(const std::string& attributeName,
content::WebContents* web_contents);

This comment has been minimized.

@kevinsawicki

kevinsawicki May 19, 2017

Contributor

This should be attribute_name.

it('should download a pdf when plugins are disabled', function (done) {
createBrowserWindow(false)
ipcRenderer.sendSync('set-download-option', false, false)
ipcRenderer.once('download-done', function (event, state, url,

This comment has been minimized.

@kevinsawicki

kevinsawicki May 19, 2017

Contributor

I don't think you need to wrap the arguments here, long lines are fine in specs.

If you do want to wrap, can you align the mimeType param with the event param?

bool sandboxed = false;
web_preferences.GetBoolean("sandbox", &sandboxed);
return sandboxed;
bool boolValue = false;

This comment has been minimized.

@kevinsawicki

kevinsawicki May 19, 2017

Contributor

This should be bool_value.

@rreimann

This comment has been minimized.

Contributor

rreimann commented May 22, 2017

Fixed the requested naming/formatting changes in commit 8f9e553. @kevinsawicki would it be ok if i open another PR to backport this fix into the 1-6-x brach after it was merged into master?

@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented May 22, 2017

Thanks for this @rreimann 👍 🚢

would it be ok if i open another PR to backport this fix into the 1-6-x brach after it was merged into master?

Yes, sounds good, thanks 👍

@kevinsawicki kevinsawicki merged commit 021b5a9 into electron:master May 22, 2017

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment