Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2354443002:
Implement PluginsPermissionContext and hookup to flash download interception. (Closed)
DescriptionImplement PluginsPermissionContext and hookup to flash download interception.
This implements the basic logic for the PluginsPermissionContext which:
1) Accurately checks the content setting to determine whether to show the prompt.
2) Displays an infobar to refresh the page if the user clicks allow.
BUG=641615
Committed: https://crrev.com/f54094bfcc90193718623b611c7c55c5ecf51d71
Cr-Commit-Position: refs/heads/master@{#421076}
Patch Set 1 #
Total comments: 8
Patch Set 2 : . #Patch Set 3 : . #Patch Set 4 : Implement PluginsPermissionContext and hookup to flash download interception. #Patch Set 5 : Implement PluginsPermissionContext and hookup to flash download interception. #Patch Set 6 : Implement PluginsPermissionContext and hookup to flash download interception. #Patch Set 7 : Implement PluginsPermissionContext and hookup to flash download interception. #
Total comments: 6
Patch Set 8 : Implement PluginsPermissionContext and hookup to flash download interception. #Patch Set 9 : Implement PluginsPermissionContext and hookup to flash download interception. #Patch Set 10 : Implement PluginsPermissionContext and hookup to flash download interception. #Patch Set 11 : Implement PluginsPermissionContext and hookup to flash download interception. #
Messages
Total messages: 32 (11 generated)
raymes@chromium.org changed reviewers: + dominickn@chromium.org, tommycli@chromium.org
PTAL. I'll add some browser tests if the approach looks good. https://codereview.chromium.org/2354443002/diff/1/chrome/browser/plugins/flas... File chrome/browser/plugins/flash_download_interception.cc (left): https://codereview.chromium.org/2354443002/diff/1/chrome/browser/plugins/flas... chrome/browser/plugins/flash_download_interception.cc:63: page_url, page_url, CONTENT_SETTINGS_TYPE_PLUGINS, std::string(), We need to decide if we should be passing the resource identifier here.
Approach looks good to me https://codereview.chromium.org/2354443002/diff/1/chrome/browser/plugins/flas... File chrome/browser/plugins/flash_download_interception.cc (right): https://codereview.chromium.org/2354443002/diff/1/chrome/browser/plugins/flas... chrome/browser/plugins/flash_download_interception.cc:39: content::RenderFrameHost* rfh = source->GetMainFrame(); Nit: inline source->GetMainFrame() and use source->GetLastCommitedURL()? Then you don't need to #include render_frame_host.h
agree, overall looks good. Some questions below https://codereview.chromium.org/2354443002/diff/1/chrome/browser/plugins/flas... File chrome/browser/plugins/flash_download_interception.cc (right): https://codereview.chromium.org/2354443002/diff/1/chrome/browser/plugins/flas... chrome/browser/plugins/flash_download_interception.cc:31: void DoNothing(blink::mojom::PermissionStatus result) {} Don't we need to refresh the page after getting the permission? Or is that a TODO? https://codereview.chromium.org/2354443002/diff/1/chrome/browser/plugins/flas... chrome/browser/plugins/flash_download_interception.cc:73: if (status != blink::mojom::PermissionStatus::ASK) As I understand it, the code was moved because the Permission Manager needs this content settings check anyways?
I've added some tests. I need to work out how to get flash to run on the bots. PTAL https://codereview.chromium.org/2354443002/diff/1/chrome/browser/plugins/flas... File chrome/browser/plugins/flash_download_interception.cc (right): https://codereview.chromium.org/2354443002/diff/1/chrome/browser/plugins/flas... chrome/browser/plugins/flash_download_interception.cc:31: void DoNothing(blink::mojom::PermissionStatus result) {} On 2016/09/19 16:44:26, tommycli wrote: > Don't we need to refresh the page after getting the permission? Or is that a > TODO? This happens in FlashPermissionContext::UpdateTabContext. Currently we're just displaying an infobar which asks the user to refresh the page which we thought may be better, but we can refresh the page directly if it makes more sense. https://codereview.chromium.org/2354443002/diff/1/chrome/browser/plugins/flas... chrome/browser/plugins/flash_download_interception.cc:39: content::RenderFrameHost* rfh = source->GetMainFrame(); On 2016/09/19 05:54:22, dominickn wrote: > Nit: inline source->GetMainFrame() and use source->GetLastCommitedURL()? Then > you don't need to #include render_frame_host.h Done. https://codereview.chromium.org/2354443002/diff/1/chrome/browser/plugins/flas... chrome/browser/plugins/flash_download_interception.cc:73: if (status != blink::mojom::PermissionStatus::ASK) On 2016/09/19 16:44:26, tommycli wrote: > As I understand it, the code was moved because the Permission Manager needs this > content settings check anyways? Correct - the code is pretty much needed there so I think it makes sense just to call it through there.
On 2016/09/21 05:46:06, raymes wrote: > I've added some tests. I need to work out how to get flash to run on the bots. > PTAL > > https://codereview.chromium.org/2354443002/diff/1/chrome/browser/plugins/flas... > File chrome/browser/plugins/flash_download_interception.cc (right): > > https://codereview.chromium.org/2354443002/diff/1/chrome/browser/plugins/flas... > chrome/browser/plugins/flash_download_interception.cc:31: void > DoNothing(blink::mojom::PermissionStatus result) {} > On 2016/09/19 16:44:26, tommycli wrote: > > Don't we need to refresh the page after getting the permission? Or is that a > > TODO? > > This happens in FlashPermissionContext::UpdateTabContext. Currently we're just > displaying an infobar which asks the user to refresh the page which we thought > may be better, but we can refresh the page directly if it makes more sense. > > https://codereview.chromium.org/2354443002/diff/1/chrome/browser/plugins/flas... > chrome/browser/plugins/flash_download_interception.cc:39: > content::RenderFrameHost* rfh = source->GetMainFrame(); > On 2016/09/19 05:54:22, dominickn wrote: > > Nit: inline source->GetMainFrame() and use source->GetLastCommitedURL()? Then > > you don't need to #include render_frame_host.h > > Done. > > https://codereview.chromium.org/2354443002/diff/1/chrome/browser/plugins/flas... > chrome/browser/plugins/flash_download_interception.cc:73: if (status != > blink::mojom::PermissionStatus::ASK) > On 2016/09/19 16:44:26, tommycli wrote: > > As I understand it, the code was moved because the Permission Manager needs > this > > content settings check anyways? > > Correct - the code is pretty much needed there so I think it makes sense just to > call it through there. This looks good to me. Maybe the tests should be split off into a separate CL if the rest of the code is ready to commit. lgtm % the tests Regarding the tests: The Power Saver browsertests use a fake test plugin https://cs.chromium.org/chromium/src/chrome/browser/plugins/plugin_power_save... We should probably reuse that, since the Chromium bots don't have Flash - especially since we aren't directly testing any Flash functionality anyways. Just my suggestion.
On 2016/09/21 16:46:54, tommycli wrote: > On 2016/09/21 05:46:06, raymes wrote: > > I've added some tests. I need to work out how to get flash to run on the bots. > > PTAL > > > > > https://codereview.chromium.org/2354443002/diff/1/chrome/browser/plugins/flas... > > File chrome/browser/plugins/flash_download_interception.cc (right): > > > > > https://codereview.chromium.org/2354443002/diff/1/chrome/browser/plugins/flas... > > chrome/browser/plugins/flash_download_interception.cc:31: void > > DoNothing(blink::mojom::PermissionStatus result) {} > > On 2016/09/19 16:44:26, tommycli wrote: > > > Don't we need to refresh the page after getting the permission? Or is that a > > > TODO? > > > > This happens in FlashPermissionContext::UpdateTabContext. Currently we're just > > displaying an infobar which asks the user to refresh the page which we thought > > may be better, but we can refresh the page directly if it makes more sense. > > > > > https://codereview.chromium.org/2354443002/diff/1/chrome/browser/plugins/flas... > > chrome/browser/plugins/flash_download_interception.cc:39: > > content::RenderFrameHost* rfh = source->GetMainFrame(); > > On 2016/09/19 05:54:22, dominickn wrote: > > > Nit: inline source->GetMainFrame() and use source->GetLastCommitedURL()? > Then > > > you don't need to #include render_frame_host.h > > > > Done. > > > > > https://codereview.chromium.org/2354443002/diff/1/chrome/browser/plugins/flas... > > chrome/browser/plugins/flash_download_interception.cc:73: if (status != > > blink::mojom::PermissionStatus::ASK) > > On 2016/09/19 16:44:26, tommycli wrote: > > > As I understand it, the code was moved because the Permission Manager needs > > this > > > content settings check anyways? > > > > Correct - the code is pretty much needed there so I think it makes sense just > to > > call it through there. > > This looks good to me. Maybe the tests should be split off into a separate CL if > the rest of the code is ready to commit. > > lgtm % the tests > > Regarding the tests: The Power Saver browsertests use a fake test plugin > https://cs.chromium.org/chromium/src/chrome/browser/plugins/plugin_power_save... > > We should probably reuse that, since the Chromium bots don't have Flash - > especially since we aren't directly testing any Flash functionality anyways. > Just my suggestion. Done - I've moved the tests out into a separate CL.
raymes@chromium.org changed reviewers: + bauerb@chromium.org
+bauerb for plugins/
lgtm https://codereview.chromium.org/2354443002/diff/120001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_score.h (right): https://codereview.chromium.org/2354443002/diff/120001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_score.h:165: friend class FlashPermissionTestConfig; Sort these? https://codereview.chromium.org/2354443002/diff/120001/chrome/browser/plugins... File chrome/browser/plugins/flash_download_interception.cc (right): https://codereview.chromium.org/2354443002/diff/120001/chrome/browser/plugins... chrome/browser/plugins/flash_download_interception.cc:30: void DoNothing(blink::mojom::PermissionStatus result) {} Aside: we should really have a parametrized IgnoreArgument(T) method in bind_utils.h or something… https://codereview.chromium.org/2354443002/diff/120001/chrome/browser/plugins... File chrome/browser/plugins/flash_permission_context.cc (right): https://codereview.chromium.org/2354443002/diff/120001/chrome/browser/plugins... chrome/browser/plugins/flash_permission_context.cc:39: if (allowed) { Return early if not allowed?
https://codereview.chromium.org/2354443002/diff/1/chrome/browser/plugins/flas... File chrome/browser/plugins/flash_download_interception.cc (right): https://codereview.chromium.org/2354443002/diff/1/chrome/browser/plugins/flas... chrome/browser/plugins/flash_download_interception.cc:31: void DoNothing(blink::mojom::PermissionStatus result) {} On 2016/09/21 05:46:06, raymes wrote: > On 2016/09/19 16:44:26, tommycli wrote: > > Don't we need to refresh the page after getting the permission? Or is that a > > TODO? > > This happens in FlashPermissionContext::UpdateTabContext. Currently we're just > displaying an infobar which asks the user to refresh the page which we thought > may be better, but we can refresh the page directly if it makes more sense. Sounds good. The CL as-is makes sense, but I think we will probably want to refresh directly. At least, that's what Other Browsers do. Maybe play with it and give me your take?
On 2016/09/23 23:27:27, tommycli wrote: > https://codereview.chromium.org/2354443002/diff/1/chrome/browser/plugins/flas... > File chrome/browser/plugins/flash_download_interception.cc (right): > > https://codereview.chromium.org/2354443002/diff/1/chrome/browser/plugins/flas... > chrome/browser/plugins/flash_download_interception.cc:31: void > DoNothing(blink::mojom::PermissionStatus result) {} > On 2016/09/21 05:46:06, raymes wrote: > > On 2016/09/19 16:44:26, tommycli wrote: > > > Don't we need to refresh the page after getting the permission? Or is that a > > > TODO? > > > > This happens in FlashPermissionContext::UpdateTabContext. Currently we're just > > displaying an infobar which asks the user to refresh the page which we thought > > may be better, but we can refresh the page directly if it makes more sense. > > Sounds good. The CL as-is makes sense, but I think we will probably want to > refresh directly. At least, that's what Other Browsers do. > > Maybe play with it and give me your take? Refreshing felt a bit weird (kills the user's state) but it's easy to change to that if we decide we want it. I'll land this and you can give me your thoughts.
Thanks! https://codereview.chromium.org/2354443002/diff/120001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_score.h (right): https://codereview.chromium.org/2354443002/diff/120001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_score.h:165: friend class FlashPermissionTestConfig; On 2016/09/22 16:59:10, Bernhard (slow until Sep 27) wrote: > Sort these? Done. https://codereview.chromium.org/2354443002/diff/120001/chrome/browser/plugins... File chrome/browser/plugins/flash_download_interception.cc (right): https://codereview.chromium.org/2354443002/diff/120001/chrome/browser/plugins... chrome/browser/plugins/flash_download_interception.cc:30: void DoNothing(blink::mojom::PermissionStatus result) {} On 2016/09/22 16:59:10, Bernhard (slow until Sep 27) wrote: > Aside: we should really have a parametrized IgnoreArgument(T) method in > bind_utils.h or something… Yeah I looked for it but I couldn't find it. I wonder if this comes up enough to justify it. https://codereview.chromium.org/2354443002/diff/120001/chrome/browser/plugins... File chrome/browser/plugins/flash_permission_context.cc (right): https://codereview.chromium.org/2354443002/diff/120001/chrome/browser/plugins... chrome/browser/plugins/flash_permission_context.cc:39: if (allowed) { On 2016/09/22 16:59:10, Bernhard (slow until Sep 27) wrote: > Return early if not allowed? Done.
The CQ bit was checked by raymes@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommycli@chromium.org, bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/2354443002/#ps200001 (title: "Implement PluginsPermissionContext and hookup to flash download interception.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by raymes@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by raymes@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2016/09/26 10:10:59, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cq fail looks like flake.
The CQ bit was checked by raymes@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Implement PluginsPermissionContext and hookup to flash download interception. This implements the basic logic for the PluginsPermissionContext which: 1) Accurately checks the content setting to determine whether to show the prompt. 2) Displays an infobar to refresh the page if the user clicks allow. BUG=641615 ========== to ========== Implement PluginsPermissionContext and hookup to flash download interception. This implements the basic logic for the PluginsPermissionContext which: 1) Accurately checks the content setting to determine whether to show the prompt. 2) Displays an infobar to refresh the page if the user clicks allow. BUG=641615 Committed: https://crrev.com/f54094bfcc90193718623b611c7c55c5ecf51d71 Cr-Commit-Position: refs/heads/master@{#421076} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/f54094bfcc90193718623b611c7c55c5ecf51d71 Cr-Commit-Position: refs/heads/master@{#421076} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
