Implement PlatformUtil::OpenExternal() so that mailto: can work

Implement PlatformUtil::OpenExternal() so that mailto: can work

Clicking a mailto link inside an iframe doesn't work on android.
This is because PlatformUtil::OpenExternal() is not implemented.
This CL adds the implementation for this feature.

BUG=523491
Committed: https://crrev.com/d4599b4ab7e5f934c434bb7ead0363c765731261
Cr-Commit-Position: refs/heads/master@{#347774}

[qinmin, 2015-09-04 17:09:22]

Patchset #1 (id:1) has been deleted

[qinmin, 2015-09-04 17:10:44]

qinmin@chromium.org changed reviewers:
+ sky@chromium.org, tedchoc@chromium.org

[qinmin, 2015-09-04 17:10:45]

PTAL
+tedchoc for android/
+sky for chrome/

[Ted C, 2015-09-04 17:32:26]

https://codereview.chromium.org/1310873003/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/PlatformUtil.java
(right):

https://codereview.chromium.org/1310873003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/PlatformUtil.java:23:
intent.setFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
should we add:

intent.addCategory(Intent.CATEGORY_BROWSABLE);

I don't know if that is explicitly required for SENDTO, but it is started by the
browser so it seems like the right thing to do?

https://codereview.chromium.org/1310873003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/PlatformUtil.java:24:
context.startActivity(intent);
I would catch ActivityNotFoundException just in case here.

https://codereview.chromium.org/1310873003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/PlatformUtil.java:25: }
should we be sending a generic view intent in the else case or is this truly
only for mailto?

https://codereview.chromium.org/1310873003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/PlatformUtil.java:26: }
And looking a bit more at the intent class, it looks like we might be able to
VIEW even for mailto:

http://developer.android.com/reference/android/content/Intent.html#ACTION_VIEW

So maybe this can be even more generic?

[qinmin, 2015-09-04 17:51:38]

https://codereview.chromium.org/1310873003/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/PlatformUtil.java
(right):

https://codereview.chromium.org/1310873003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/PlatformUtil.java:23:
intent.setFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
On 2015/09/04 17:32:26, Ted C wrote:
> should we add:
> 
> intent.addCategory(Intent.CATEGORY_BROWSABLE);
> 
> I don't know if that is explicitly required for SENDTO, but it is started by
the
> browser so it seems like the right thing to do?

Done.

https://codereview.chromium.org/1310873003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/PlatformUtil.java:24:
context.startActivity(intent);
On 2015/09/04 17:32:26, Ted C wrote:
> I would catch ActivityNotFoundException just in case here.

Done.

https://codereview.chromium.org/1310873003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/PlatformUtil.java:25: }
On 2015/09/04 17:32:26, Ted C wrote:
> should we be sending a generic view intent in the else case or is this truly
> only for mailto?

Changed the Intent action to ACTION_VIEW and removed the if statement, so all
other types of url will be handled by the generic handler.

https://codereview.chromium.org/1310873003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/PlatformUtil.java:26: }
On 2015/09/04 17:32:26, Ted C wrote:
> And looking a bit more at the intent class, it looks like we might be able to
> VIEW even for mailto:
> 
> http://developer.android.com/reference/android/content/Intent.html#ACTION_VIEW
> 
> So maybe this can be even more generic?

Done.

[Ted C, 2015-09-04 17:54:28]

lgtm

[sky, 2015-09-08 15:09:15]

LGTM

https://codereview.chromium.org/1310873003/diff/40001/chrome/browser/platform...
File chrome/browser/platform_util_android.cc (right):

https://codereview.chromium.org/1310873003/diff/40001/chrome/browser/platform...
chrome/browser/platform_util_android.cc:64: 
nit: only one newline here.

[qinmin, 2015-09-08 18:49:52]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2015-09-08 18:49:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1310873003/#ps60001
(title: "nit")

[commit-bot: I haz the power, 2015-09-08 18:50:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1310873003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1310873003/60001

[commit-bot: I haz the power, 2015-09-08 20:06:26]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-09-08 20:07:07]

Patchset 3 (id:??) landed as
https://crrev.com/d4599b4ab7e5f934c434bb7ead0363c765731261
Cr-Commit-Position: refs/heads/master@{#347774}

[PhistucK, 2015-09-10 12:46:46]

phistuck@gmail.com changed reviewers:
+ phistuck@gmail.com

[PhistucK, 2015-09-10 12:46:47]

Sorry - is this allowing more than just mailto:?
I do not see anything specific to mailto: here...

I think Chrome for Android used to support launching external protocol handlers
within iFrames (perhaps at all?), but it was dropped in favor of a
funny-syntax-URI that gives a bit more power (launches a protocol handler if it
is installed, goes to the store if it is not installed and it also supports an
option to pass parameters), but it is not supported in iFrames or something
similar for security (or user experience) reasons.