Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2716053002:
[HBD] Add an Enable Flash context menu item for HBD placeholders. (Closed)
Description[HBD] Add an Enable Flash context menu item for HBD placeholders.
BUG=690304
Review-Url: https://codereview.chromium.org/2716053002
Cr-Commit-Position: refs/heads/master@{#456975}
Committed: https://chromium.googlesource.com/chromium/src/+/c0ff6ff2a46b05222e28f9782fee0e8bfb63291e
Patch Set 1 #Patch Set 2 : show domain name #
Total comments: 4
Patch Set 3 : remove origin #
Total comments: 2
Patch Set 4 : add comment #
Messages
Total messages: 39 (19 generated)
tommycli@chromium.org changed reviewers: + raymes@chromium.org
raymes: PTAL, thanks! https://codereview.chromium.org/2716053002/diff/20001/chrome/renderer/plugins... File chrome/renderer/plugins/chrome_plugin_placeholder.cc (right): https://codereview.chromium.org/2716053002/diff/20001/chrome/renderer/plugins... chrome/renderer/plugins/chrome_plugin_placeholder.cc:369: base::string16 display_origin = url_formatter::FormatUrlForSecurityDisplay( Same technique as https://cs.chromium.org/chromium/src/chrome/browser/ui/views/website_settings...
On 2017/02/24 20:09:02, tommycli wrote: > raymes: PTAL, thanks! > > https://codereview.chromium.org/2716053002/diff/20001/chrome/renderer/plugins... > File chrome/renderer/plugins/chrome_plugin_placeholder.cc (right): > > https://codereview.chromium.org/2716053002/diff/20001/chrome/renderer/plugins... > chrome/renderer/plugins/chrome_plugin_placeholder.cc:369: base::string16 > display_origin = url_formatter::FormatUrlForSecurityDisplay( > Same technique as > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/website_settings... screenshot on bug, thanks!
lg, thanks for looking at this :) However I'm not sure if we should include the origin? https://codereview.chromium.org/2716053002/diff/20001/chrome/renderer/plugins... File chrome/renderer/plugins/chrome_plugin_placeholder.cc (right): https://codereview.chromium.org/2716053002/diff/20001/chrome/renderer/plugins... chrome/renderer/plugins/chrome_plugin_placeholder.cc:371: url_formatter::SchemeDisplay::OMIT_CRYPTOGRAPHIC); What's the motivation for displaying the origin here? Is it necessary given that we will show the origin in the permission prompt?
raymes: Thanks! See comment below. https://codereview.chromium.org/2716053002/diff/20001/chrome/renderer/plugins... File chrome/renderer/plugins/chrome_plugin_placeholder.cc (right): https://codereview.chromium.org/2716053002/diff/20001/chrome/renderer/plugins... chrome/renderer/plugins/chrome_plugin_placeholder.cc:371: url_formatter::SchemeDisplay::OMIT_CRYPTOGRAPHIC); On 2017/02/27 03:51:45, raymes wrote: > What's the motivation for displaying the origin here? Is it necessary given that > we will show the origin in the permission prompt? Hey see https://bugs.chromium.org/p/chromium/issues/detail?id=690304#c9 and comment 10 also. Basically -- in the past, "Run this plugin" applied to that plugin instance only. However, triggering the permission prompt actually enables Flash on the whole top-level origin, so technically, including the origin in the menu item is more correct. However, if you think the permission prompt itself covers that information adequately, I'm happy to remove this code. It's not a big deal for me either way. We should probably hash it out on the bug itself.
https://codereview.chromium.org/2716053002/diff/20001/chrome/renderer/plugins... File chrome/renderer/plugins/chrome_plugin_placeholder.cc (right): https://codereview.chromium.org/2716053002/diff/20001/chrome/renderer/plugins... chrome/renderer/plugins/chrome_plugin_placeholder.cc:371: url_formatter::SchemeDisplay::OMIT_CRYPTOGRAPHIC); On 2017/02/27 19:15:33, tommycli wrote: > On 2017/02/27 03:51:45, raymes wrote: > > What's the motivation for displaying the origin here? Is it necessary given > that > > we will show the origin in the permission prompt? > > Hey see https://bugs.chromium.org/p/chromium/issues/detail?id=690304#c9 and > comment 10 also. > > Basically -- in the past, "Run this plugin" applied to that plugin instance > only. However, triggering the permission prompt actually enables Flash on the > whole top-level origin, so technically, including the origin in the menu item is > more correct. > > However, if you think the permission prompt itself covers that information > adequately, I'm happy to remove this code. It's not a big deal for me either > way. We should probably hash it out on the bug itself. Ahh thanks for explaining. I actually think it's somewhat confusing if we leave it in because it gives me a sense that the flash instance is being served from the origin that appears in the dialog, which it isn't necessarily. Given that there is confusion anyway I would suggest leaving it out.
The CQ bit was checked by tommycli@chromium.org to run a CQ dry run
raymes: PTAL again, ericde has approved the version without the domain in the context menu.
Dry run: 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
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by tommycli@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
On 2017/03/14 00:26:45, tommycli wrote: > raymes: PTAL again, ericde has approved the version without the domain in the > context menu. raymes: ping, thanks!
lgtm
The CQ bit was checked by tommycli@chromium.org
On 2017/03/14 23:15:58, raymes wrote: > lgtm thanks!
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
tommycli@chromium.org changed reviewers: + thestig@chromium.org
thestig: PTAL, need owner rubberstamp. raymes has already expert-reviewed it.
https://codereview.chromium.org/2716053002/diff/40001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2716053002/diff/40001/chrome/app/generated_re... chrome/app/generated_resources.grd:917: + <message name="IDS_CONTENT_CONTEXT_ENABLE_FLASH" desc="The name of the Enable Flash command on the blocked plugin context menu"> Is this only in the "use_titlecase" section because it's Mac-specific or because you forgot to duplicate it for the "not use_titlecase" case? If it's Mac-only, maybe make it more clear by putting it in a is_macosx section. Otherwise, make a second copy of the string above.
thestig: Thanks, addressed your comment below. https://codereview.chromium.org/2716053002/diff/40001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2716053002/diff/40001/chrome/app/generated_re... chrome/app/generated_resources.grd:917: + <message name="IDS_CONTENT_CONTEXT_ENABLE_FLASH" desc="The name of the Enable Flash command on the blocked plugin context menu"> On 2017/03/15 00:33:57, Lei Zhang (super slow) wrote: > Is this only in the "use_titlecase" section because it's Mac-specific or because > you forgot to duplicate it for the "not use_titlecase" case? If it's Mac-only, > maybe make it more clear by putting it in a is_macosx section. Otherwise, make a > second copy of the string above. Done. I added an explanatory comment.
lgtm
On 2017/03/15 00:55:52, Lei Zhang (super slow) wrote: > lgtm thank you sir!
The CQ bit was checked by tommycli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org Link to the patchset: https://codereview.chromium.org/2716053002/#ps60001 (title: "add comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489539363592830,
"parent_rev": "2498e3963e41aa8ecf924611c35bdf379f53056f", "commit_rev":
"4dbda1097b6f48d85bddaa5615e31cad7e7768a7"}
The CQ bit was unchecked by commit-bot@chromium.org
Prior attempt to commit was detected, but we were not able to check whether the issue was successfully committed. Please check Git history manually and re-check CQ or close this issue as needed.
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489547012378440,
"parent_rev": "040aad2ebc166c48c8c5bee6582d1d0ad226c7fe", "commit_rev":
"c0ff6ff2a46b05222e28f9782fee0e8bfb63291e"}
Message was sent while issue was closed.
Description was changed from ========== [HBD] Add an Enable Flash context menu item for HBD placeholders. BUG=690304 ========== to ========== [HBD] Add an Enable Flash context menu item for HBD placeholders. BUG=690304 Review-Url: https://codereview.chromium.org/2716053002 Cr-Commit-Position: refs/heads/master@{#456975} Committed: https://chromium.googlesource.com/chromium/src/+/c0ff6ff2a46b05222e28f9782fee... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/c0ff6ff2a46b05222e28f9782fee... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
