Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1220813013:
This patch enables "mailto" links in WebViews. (Closed)
|
Created:
5 years, 5 months ago by paulmeyer Modified:
5 years, 5 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThis patch enables "mailto" links in WebViews.
BUG=461329
Committed: https://crrev.com/f43bfadcc196352eeb871ee01b3f9e8317252da1
Cr-Commit-Position: refs/heads/master@{#339931}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Addressed comment. #
Total comments: 1
Patch Set 3 : Addressed comment. #Patch Set 4 : Added test. #Patch Set 5 : Rebased. #Patch Set 6 : Adjusted test to work with ChromeOS. #Patch Set 7 : Fixed test. Rebased. #
Messages
Total messages: 34 (14 generated)
paulmeyer@chromium.org changed reviewers: + lazyboy@chromium.org
Patchset #1 (id:1) has been deleted
https://chromiumcodereview.appspot.com/1220813013/diff/20001/chrome/browser/r... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://chromiumcodereview.appspot.com/1220813013/diff/20001/chrome/browser/r... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:533: !url.SchemeIs("mailto")) { use url::kMailToScheme https://chromiumcodereview.appspot.com/1220813013/diff/20001/extensions/brows... File extensions/browser/guest_view/web_view/web_view_guest.cc (right): https://chromiumcodereview.appspot.com/1220813013/diff/20001/extensions/brows... extensions/browser/guest_view/web_view/web_view_guest.cc:818: if (validated_url.SchemeIs("mailto")) This seems like a hack, can you explain what's going on? With your change, what happens when you click on mailto: link inside webview? This behavior mimics whatever happens when you click on mailto: in a tab?
https://codereview.chromium.org/1220813013/diff/20001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/1220813013/diff/20001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:533: !url.SchemeIs("mailto")) { On 2015/07/07 19:32:11, lazyboy wrote: > use url::kMailToScheme Done. https://codereview.chromium.org/1220813013/diff/20001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_guest.cc (right): https://codereview.chromium.org/1220813013/diff/20001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_guest.cc:818: if (validated_url.SchemeIs("mailto")) On 2015/07/07 19:32:11, lazyboy wrote: > This seems like a hack, can you explain what's going on? > > With your change, what happens when you click on mailto: link inside webview? > This behavior mimics whatever happens when you click on mailto: in a tab? Yes, it does exactly what happens in a tab. With a webview though, you also get a loadabort because the actual page in the webview didn't navigate. That's what I'm suppressing here.
https://codereview.chromium.org/1220813013/diff/20001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_guest.cc (right): https://codereview.chromium.org/1220813013/diff/20001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_guest.cc:818: if (validated_url.SchemeIs("mailto")) On 2015/07/08 17:20:16, Paul Meyer wrote: > On 2015/07/07 19:32:11, lazyboy wrote: > > This seems like a hack, can you explain what's going on? > > > > With your change, what happens when you click on mailto: link inside webview? > > This behavior mimics whatever happens when you click on mailto: in a tab? > > Yes, it does exactly what happens in a tab. With a webview though, you also get > a loadabort because the actual page in the webview didn't navigate. That's what > I'm suppressing here. When we click mailto: from a regular tab page, does that also trigger DidProvisionalLoad then? (And that seems odd).
https://codereview.chromium.org/1220813013/diff/20001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_guest.cc (right): https://codereview.chromium.org/1220813013/diff/20001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_guest.cc:818: if (validated_url.SchemeIs("mailto")) On 2015/07/10 19:46:50, lazyboy wrote: > On 2015/07/08 17:20:16, Paul Meyer wrote: > > On 2015/07/07 19:32:11, lazyboy wrote: > > > This seems like a hack, can you explain what's going on? > > > > > > With your change, what happens when you click on mailto: link inside > webview? > > > This behavior mimics whatever happens when you click on mailto: in a tab? > > > > Yes, it does exactly what happens in a tab. With a webview though, you also > get > > a loadabort because the actual page in the webview didn't navigate. That's > what > > I'm suppressing here. > When we click mailto: from a regular tab page, does that also trigger > DidProvisionalLoad then? (And that seems odd). > Yes it does, and it ends up calling NavigatorImpl::DidFailProvisionalLoadWithError, which for a WebView then triggers a loadabort. I don't want that to happen though so I suppress just that. Everything else that happens already happens in a regular tab, so I think it is normal.
OK, lgtm https://codereview.chromium.org/1220813013/diff/40001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_guest.cc (right): https://codereview.chromium.org/1220813013/diff/40001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_guest.cc:814: if (validated_url.SchemeIs("mailto")) url::kMailToScheme
paulmeyer@chromium.org changed reviewers: + thakis@chromium.org
+thakis@ for OWNER review of chrome_resource_dispatcher_host_delegate.cc
No test? This seems corner-casy enough that it's going to regress without anyone noticing if there's no test. Are there any security considerations with exposing this? Can you get a quick sanity check from someone on the security team?
kenrb@chromium.org changed reviewers: + kenrb@chromium.org
lgtm for security. This isn't meaningfully different from mailto: being available on a non-guest page.
I've added a test. PTAL.
lgtm
The CQ bit was checked by paulmeyer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lazyboy@chromium.org Link to the patchset: https://codereview.chromium.org/1220813013/#ps80001 (title: "Added test.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1220813013/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...) cast_shell_android on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...) cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_arm_compile on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_arm_compi...) linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...) win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) (exceeded global retry quota)
The CQ bit was checked by paulmeyer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org, thakis@chromium.org, lazyboy@chromium.org Link to the patchset: https://codereview.chromium.org/1220813013/#ps100001 (title: "Rebased.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1220813013/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by paulmeyer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org, thakis@chromium.org, lazyboy@chromium.org Link to the patchset: https://codereview.chromium.org/1220813013/#ps140001 (title: "Fixed test. Rebased.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1220813013/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by paulmeyer@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1220813013/140001
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/f43bfadcc196352eeb871ee01b3f9e8317252da1 Cr-Commit-Position: refs/heads/master@{#339931} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
