Adds mailto: URL support to app launching.

Adds mailto: URL support to app launching.

Known Mail client apps are managed by a singleton class of
MailtoURLRewriter. This CL adds support for the default system
Mail client app and also for Gmail.

BUG=711511
NOTRY=true

Review-Url: https://codereview.chromium.org/2852003002
Cr-Commit-Position: refs/heads/master@{#470544}
Committed: https://chromium.googlesource.com/chromium/src/+/94313a25439ae14d5544311a274e160f41334769

[pkl (ping after 24h if needed), 2017-05-01 03:09:15]

Description was changed from

==========
Adds back mailto: support to app launching.

Known Mail client apps are managed by a singleton class of
MailtoURLRewriter. This CL adds support for the default system
Mail client app and also for Gmail.

BUG=711511
==========

to

==========
Adds mailto: URL support to app launching.

Known Mail client apps are managed by a singleton class of
MailtoURLRewriter. This CL adds support for the default system
Mail client app and also for Gmail.

BUG=711511
==========

[pkl (ping after 24h if needed), 2017-05-03 00:21:34]

pkl@chromium.org changed reviewers:
+ jif@chromium.org, rohitrao@chromium.org

[pkl (ping after 24h if needed), 2017-05-03 00:21:35]

Adds a modular approach to mailto: handlers. This adds one for the
system-provided Mail app and another one for Gmail app.

[jif, 2017-05-03 13:01:35]

First quick pass

https://codereview.chromium.org/2852003002/diff/60001/ios/chrome/browser/web/...
File ios/chrome/browser/web/external_app_launcher.mm (right):

https://codereview.chromium.org/2852003002/diff/60001/ios/chrome/browser/web/...
ios/chrome/browser/web/external_app_launcher.mm:195: [[MailtoURLRewriter
sharedInstance] rewriteMailtoURL:gURL];
MailtoURLRewriter doesn't have to be a singleton.

You can:
* have ExternalAppLauncher own it (not recommended, likely premature
optimization).
* recreate it every time (recommended).

https://codereview.chromium.org/2852003002/diff/60001/ios/chrome/browser/web/...
File ios/chrome/browser/web/mailto_handler.h (right):

https://codereview.chromium.org/2852003002/diff/60001/ios/chrome/browser/web/...
ios/chrome/browser/web/mailto_handler.h:28: - (BOOL)isAvailable;
I think the subclass should return the Mail client's scheme instead:
mailto:// for Apple Mail.
googlegmail:// for Gmail.

 This way, we'd avoid duplicating the 
"[[UIApplication sharedApplication] canOpenURL:[NSURL URLWithString:" logic.

Also, this information may allow us to check at runtime that the
LSApplicationQueriesSchemes section of the app's .plist contains the schemes.
This can be done in a separate CL.

https://codereview.chromium.org/2852003002/diff/60001/ios/chrome/browser/web/...
File ios/chrome/browser/web/mailto_handler_gmail.mm (right):

https://codereview.chromium.org/2852003002/diff/60001/ios/chrome/browser/web/...
ios/chrome/browser/web/mailto_handler_gmail.mm:31: -
(NSString*)rewriteMailtoURL:(const GURL&)gURL {
Consider creating a function in the same vein as
https://github.com/mozilla-mobile/firefox-ios/blob/0da53b909b73ae61b480f353b7...

This will make the implementation of |rewriteMailtoURL:| for the other mail
clients simpler.

https://codereview.chromium.org/2852003002/diff/60001/ios/chrome/browser/web/...
File ios/chrome/browser/web/mailto_url_rewriter.mm (right):

https://codereview.chromium.org/2852003002/diff/60001/ios/chrome/browser/web/...
ios/chrome/browser/web/mailto_url_rewriter.mm:44: + (instancetype)sharedInstance
{
Assuming you agree with making this class not a singleton, you can simply
change:
+ (instancetype)sharedInstance;
to
- (instancetype)newMailtoURLRewriter;

https://codereview.chromium.org/2852003002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2852003002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:26022: +<histogram
name="MailtoURLRewriter.Rewritten">
Should we consider having an enum?
In the futur the enum could be:

-Not rewritten
-Rewritten to Gmail
-Rewritten to Outlook.

[pkl (ping after 24h if needed), 2017-05-03 22:17:18]

jif: I believe that I've addressed your comments.

https://codereview.chromium.org/2852003002/diff/60001/ios/chrome/browser/web/...
File ios/chrome/browser/web/external_app_launcher.mm (right):

https://codereview.chromium.org/2852003002/diff/60001/ios/chrome/browser/web/...
ios/chrome/browser/web/external_app_launcher.mm:195: [[MailtoURLRewriter
sharedInstance] rewriteMailtoURL:gURL];
On 2017/05/03 13:01:35, jif wrote:
> MailtoURLRewriter doesn't have to be a singleton.
> 
> You can:
> * have ExternalAppLauncher own it (not recommended, likely premature
> optimization).
> * recreate it every time (recommended).

Given how ExternalAppLauncher is used, it is likely that MailtoURLRewriter will
be recreated every time. May as well keep it simple and just recreate it when it
is needed.

https://codereview.chromium.org/2852003002/diff/60001/ios/chrome/browser/web/...
File ios/chrome/browser/web/mailto_handler.h (right):

https://codereview.chromium.org/2852003002/diff/60001/ios/chrome/browser/web/...
ios/chrome/browser/web/mailto_handler.h:28: - (BOOL)isAvailable;
On 2017/05/03 13:01:35, jif wrote:
> I think the subclass should return the Mail client's scheme instead:
> mailto:// for Apple Mail.
> googlegmail:// for Gmail.
> 
>  This way, we'd avoid duplicating the 
> "[[UIApplication sharedApplication] canOpenURL:[NSURL URLWithString:" logic.
> 
> Also, this information may allow us to check at runtime that the
> LSApplicationQueriesSchemes section of the app's .plist contains the schemes.
> This can be done in a separate CL.

May be I don't understand completely. Having this return the URL scheme does not
tell us whether Gmail is installed or not.
However, checking LSApplicationQUeriesSchemes section before calling
-canOpenURL: is a good idea. It is possible to DCHECK if the scheme is not
listed.

https://codereview.chromium.org/2852003002/diff/60001/ios/chrome/browser/web/...
File ios/chrome/browser/web/mailto_handler_gmail.mm (right):

https://codereview.chromium.org/2852003002/diff/60001/ios/chrome/browser/web/...
ios/chrome/browser/web/mailto_handler_gmail.mm:31: -
(NSString*)rewriteMailtoURL:(const GURL&)gURL {
On 2017/05/03 13:01:35, jif wrote:
> Consider creating a function in the same vein as
>
https://github.com/mozilla-mobile/firefox-ios/blob/0da53b909b73ae61b480f353b7...
> 
> This will make the implementation of |rewriteMailtoURL:| for the other mail
> clients simpler.

Instead, I moved this up to the base class so subclasses can just inherit the
implementation (if they wish).
I've also added -beginningScheme and -supportedHeaders.

https://codereview.chromium.org/2852003002/diff/60001/ios/chrome/browser/web/...
File ios/chrome/browser/web/mailto_url_rewriter.mm (right):

https://codereview.chromium.org/2852003002/diff/60001/ios/chrome/browser/web/...
ios/chrome/browser/web/mailto_url_rewriter.mm:44: + (instancetype)sharedInstance
{
On 2017/05/03 13:01:35, jif wrote:
> Assuming you agree with making this class not a singleton, you can simply
> change:
> + (instancetype)sharedInstance;
> to
> - (instancetype)newMailtoURLRewriter;

I'm using -init and initWithStandardHandlers.

https://codereview.chromium.org/2852003002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2852003002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:26022: +<histogram
name="MailtoURLRewriter.Rewritten">
On 2017/05/03 13:01:35, jif wrote:
> Should we consider having an enum?
> In the futur the enum could be:
> 
> -Not rewritten
> -Rewritten to Gmail
> -Rewritten to Outlook.

I set it up this way so there's no question about why we need to know which
non-System Mail client was used.

[jif, 2017-05-04 14:08:47]

https://codereview.chromium.org/2852003002/diff/60001/ios/chrome/browser/web/...
File ios/chrome/browser/web/mailto_handler.h (right):

https://codereview.chromium.org/2852003002/diff/60001/ios/chrome/browser/web/...
ios/chrome/browser/web/mailto_handler.h:28: - (BOOL)isAvailable;
On 2017/05/03 22:17:18, pkl -pls.ping.after.24h. wrote:
> On 2017/05/03 13:01:35, jif wrote:
> > I think the subclass should return the Mail client's scheme instead:
> > mailto:// for Apple Mail.
> > googlegmail:// for Gmail.
> > 
> >  This way, we'd avoid duplicating the 
> > "[[UIApplication sharedApplication] canOpenURL:[NSURL URLWithString:" logic.
> > 
> > Also, this information may allow us to check at runtime that the
> > LSApplicationQueriesSchemes section of the app's .plist contains the
schemes.
> > This can be done in a separate CL.
> 
> May be I don't understand completely. Having this return the URL scheme does
not
> tell us whether Gmail is installed or not.

Indeed it does not.
I'm suggesting factoring the code actually  doing the check out of the
subclasses,
to the base class.
Here's some pseudo code showing what I have in mind:




mailto_handler.mm would contain something like:

- (NSString*)applicationScheme {
  // This should be implemented by subclasses.
  return nil;
}

- (BOOL)isAvailable {
  NSString* scheme = [self applicationScheme];
  DCHECK( plist contains |scheme| );
  return [[UIApplication sharedApplication] canOpenURL:[NSURL
URLWithString:scheme]];
}




mailto_handler_system_mail.mm would contain something like:

- (NSString*)applicationScheme {
  return @"mailto://";
}




mailto_handler_gmail.mm would contain something like:

- (NSString*)applicationScheme {
  return @"googlemail://";
}

https://codereview.chromium.org/2852003002/diff/120001/ios/chrome/browser/web...
File ios/chrome/browser/web/mailto_handler.h (right):

https://codereview.chromium.org/2852003002/diff/120001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_handler.h:24: -
(instancetype)initWithName:(NSString*)appName
add NS_DESIGNATED_INITIALIZER and NS_UNAVAILABLE where necessary

https://codereview.chromium.org/2852003002/diff/120001/ios/chrome/browser/web...
File ios/chrome/browser/web/mailto_handler.mm (right):

https://codereview.chromium.org/2852003002/diff/120001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_handler.mm:57: if ([pair count] == 0U || [pair
count] > 2U)
Do we really want to handle the case where [pair count] == 1 ?

if the link is:
mailto:pkl@chromium.org?cc
do we really care about the presence of cc ?

https://codereview.chromium.org/2852003002/diff/120001/ios/chrome/browser/web...
File ios/chrome/browser/web/mailto_url_rewriter.h (right):

https://codereview.chromium.org/2852003002/diff/120001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_url_rewriter.h:38: // "opened" to launch the Mail
client app.
Can you mention that this can return nil? For example:

Can return nil for a number of reasons, including: no Mail client was selected,
the selected Mail client is not installed anymore, the selected Mail client is
not supported by Chrome anymore.

https://codereview.chromium.org/2852003002/diff/120001/ios/chrome/browser/web...
File ios/chrome/browser/web/mailto_url_rewriter.mm (right):

https://codereview.chromium.org/2852003002/diff/120001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_url_rewriter.mm:102: - (void)convertLegacyOptions
{
Add a TODO saying that we need to remove this method (associated with a... M62
bug?).

https://codereview.chromium.org/2852003002/diff/120001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_url_rewriter.mm:114: NSString* const
kLegacyShouldAutoOpenKey = @"ShouldAutoOpenLinks_422689480";
Add comment just in case somebody wonders where this constant comes from:
// Key was generated by |shouldAutoOpenLinksKey| in
google_native_app_metadata.mm.

Doesn't matter if |google_native_app_metadata.mm| is deleted, it would still be
useful for code archaeologists.

https://codereview.chromium.org/2852003002/diff/120001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_url_rewriter.mm:122: case 0:
add comment:
// kAutoOpenLinksNotSet

https://codereview.chromium.org/2852003002/diff/120001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_url_rewriter.mm:123: case 2:
// kAutoOpenLinksYes

https://codereview.chromium.org/2852003002/diff/120001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_url_rewriter.mm:126: case 1:
// kAutoOpenLinksNo

[pkl (ping after 24h if needed), 2017-05-04 22:25:58]

pkl@chromium.org changed reviewers:
+ asvitkine@chromium.org

[pkl (ping after 24h if needed), 2017-05-04 22:25:59]

jif: Your comments addressed.
rohitrao: Please do an OWNERS-review.
asvitkine: Please review tools/metrics/histograms/histograms.xml

https://codereview.chromium.org/2852003002/diff/120001/ios/chrome/browser/web...
File ios/chrome/browser/web/mailto_handler.h (right):

https://codereview.chromium.org/2852003002/diff/120001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_handler.h:24: -
(instancetype)initWithName:(NSString*)appName
On 2017/05/04 14:08:47, jif wrote:
> add NS_DESIGNATED_INITIALIZER and NS_UNAVAILABLE where necessary

It's actually not a designated initializer, but a convenience initializer for
subclasses to call.

https://codereview.chromium.org/2852003002/diff/120001/ios/chrome/browser/web...
File ios/chrome/browser/web/mailto_handler.mm (right):

https://codereview.chromium.org/2852003002/diff/120001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_handler.mm:57: if ([pair count] == 0U || [pair
count] > 2U)
On 2017/05/04 14:08:47, jif wrote:
> Do we really want to handle the case where [pair count] == 1 ?
> 
> if the link is:
> mailto:pkl@chromium.org?cc
> do we really care about the presence of cc ?

Good point. Checked some mailto query params docs and there shouldn't be any
acceptable params that do not take a =.

https://codereview.chromium.org/2852003002/diff/120001/ios/chrome/browser/web...
File ios/chrome/browser/web/mailto_url_rewriter.h (right):

https://codereview.chromium.org/2852003002/diff/120001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_url_rewriter.h:38: // "opened" to launch the Mail
client app.
On 2017/05/04 14:08:47, jif wrote:
> Can you mention that this can return nil? For example:
> 
> Can return nil for a number of reasons, including: no Mail client was
selected,
> the selected Mail client is not installed anymore, the selected Mail client is
> not supported by Chrome anymore.

It's probably unnecessary to enumerate all the possible reasons other than
"there are no Mail client app" available.
I just said the miniumum.

https://codereview.chromium.org/2852003002/diff/120001/ios/chrome/browser/web...
File ios/chrome/browser/web/mailto_url_rewriter.mm (right):

https://codereview.chromium.org/2852003002/diff/120001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_url_rewriter.mm:102: - (void)convertLegacyOptions
{
On 2017/05/04 14:08:47, jif wrote:
> Add a TODO saying that we need to remove this method (associated with a... M62
> bug?).

Done, but in the forward declaration earlier.

https://codereview.chromium.org/2852003002/diff/120001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_url_rewriter.mm:114: NSString* const
kLegacyShouldAutoOpenKey = @"ShouldAutoOpenLinks_422689480";
On 2017/05/04 14:08:47, jif wrote:
> Add comment just in case somebody wonders where this constant comes from:
> // Key was generated by |shouldAutoOpenLinksKey| in
> google_native_app_metadata.mm.
> 
> Doesn't matter if |google_native_app_metadata.mm| is deleted, it would still
be
> useful for code archaeologists.

Done.

https://codereview.chromium.org/2852003002/diff/120001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_url_rewriter.mm:122: case 0:
On 2017/05/04 14:08:47, jif wrote:
> add comment:
> // kAutoOpenLinksNotSet

Done.

https://codereview.chromium.org/2852003002/diff/120001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_url_rewriter.mm:123: case 2:
On 2017/05/04 14:08:47, jif wrote:
> // kAutoOpenLinksYes

Done.

https://codereview.chromium.org/2852003002/diff/120001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_url_rewriter.mm:126: case 1:
On 2017/05/04 14:08:47, jif wrote:
> // kAutoOpenLinksNo

Done.

[jif, 2017-05-05 09:49:39]

https://codereview.chromium.org/2852003002/diff/60001/ios/chrome/browser/web/...
File ios/chrome/browser/web/mailto_handler.h (right):

https://codereview.chromium.org/2852003002/diff/60001/ios/chrome/browser/web/...
ios/chrome/browser/web/mailto_handler.h:28: - (BOOL)isAvailable;
On 2017/05/04 14:08:47, jif wrote:
> On 2017/05/03 22:17:18, pkl -pls.ping.after.24h. wrote:
> > On 2017/05/03 13:01:35, jif wrote:
> > > I think the subclass should return the Mail client's scheme instead:
> > > mailto:// for Apple Mail.
> > > googlegmail:// for Gmail.
> > > 
> > >  This way, we'd avoid duplicating the 
> > > "[[UIApplication sharedApplication] canOpenURL:[NSURL URLWithString:"
logic.
> > > 
> > > Also, this information may allow us to check at runtime that the
> > > LSApplicationQueriesSchemes section of the app's .plist contains the
> schemes.
> > > This can be done in a separate CL.
> > 
> > May be I don't understand completely. Having this return the URL scheme does
> not
> > tell us whether Gmail is installed or not.
> 
> Indeed it does not.
> I'm suggesting factoring the code actually  doing the check out of the
> subclasses,
> to the base class.
> Here's some pseudo code showing what I have in mind:
> 
> 
> 
> 
> mailto_handler.mm would contain something like:
> 
> - (NSString*)applicationScheme {
>   // This should be implemented by subclasses.
>   return nil;
> }
> 
> - (BOOL)isAvailable {
>   NSString* scheme = [self applicationScheme];
>   DCHECK( plist contains |scheme| );
>   return [[UIApplication sharedApplication] canOpenURL:[NSURL
> URLWithString:scheme]];
> }
> 
> 
> 
> 
> mailto_handler_system_mail.mm would contain something like:
> 
> - (NSString*)applicationScheme {
>   return @"mailto://";
> }
> 
> 
> 
> 
> mailto_handler_gmail.mm would contain something like:
> 
> - (NSString*)applicationScheme {
>   return @"googlemail://";
> }

Seems like you missed this reply (which is in the Patch Set 4 ).

[pkl (ping after 24h if needed), 2017-05-05 13:15:09]

On 2017/05/05 09:49:39, jif wrote:
>
https://codereview.chromium.org/2852003002/diff/60001/ios/chrome/browser/web/...
> File ios/chrome/browser/web/mailto_handler.h (right):
> 
>
https://codereview.chromium.org/2852003002/diff/60001/ios/chrome/browser/web/...
> ios/chrome/browser/web/mailto_handler.h:28: - (BOOL)isAvailable;
> On 2017/05/04 14:08:47, jif wrote:
> > On 2017/05/03 22:17:18, pkl -pls.ping.after.24h. wrote:
> > > On 2017/05/03 13:01:35, jif wrote:
> > > > I think the subclass should return the Mail client's scheme instead:
> > > > mailto:// for Apple Mail.
> > > > googlegmail:// for Gmail.
> > > > 
> > > >  This way, we'd avoid duplicating the 
> > > > "[[UIApplication sharedApplication] canOpenURL:[NSURL URLWithString:"
> logic.
> > > > 
> > > > Also, this information may allow us to check at runtime that the
> > > > LSApplicationQueriesSchemes section of the app's .plist contains the
> > schemes.
> > > > This can be done in a separate CL.
> > > 
> > > May be I don't understand completely. Having this return the URL scheme
does
> > not
> > > tell us whether Gmail is installed or not.
> > 
> > Indeed it does not.
> > I'm suggesting factoring the code actually  doing the check out of the
> > subclasses,
> > to the base class.
> > Here's some pseudo code showing what I have in mind:
> > 
> > 
> > 
> > 
> > mailto_handler.mm would contain something like:
> > 
> > - (NSString*)applicationScheme {
> >   // This should be implemented by subclasses.
> >   return nil;
> > }
> > 
> > - (BOOL)isAvailable {
> >   NSString* scheme = [self applicationScheme];
> >   DCHECK( plist contains |scheme| );
> >   return [[UIApplication sharedApplication] canOpenURL:[NSURL
> > URLWithString:scheme]];
> > }
> > 
> > 
> > 
> > 
> > mailto_handler_system_mail.mm would contain something like:
> > 
> > - (NSString*)applicationScheme {
> >   return @"mailto://";
> > }
> > 
> > 
> > 
> > 
> > mailto_handler_gmail.mm would contain something like:
> > 
> > - (NSString*)applicationScheme {
> >   return @"googlemail://";
> > }
> 
> Seems like you missed this reply (which is in the Patch Set 4 ).

I did. Please check mail_handler.mm -isAvailable implementation. I just did not
change the name to applicationScheme. I thought about that but ended up keeping
the name beginningScheme.

[pkl (ping after 24h if needed), 2017-05-05 13:17:04]

On 2017/05/05 13:15:09, pkl -pls.ping.after.24h. wrote:
> On 2017/05/05 09:49:39, jif wrote:
> >
>
https://codereview.chromium.org/2852003002/diff/60001/ios/chrome/browser/web/...
> > File ios/chrome/browser/web/mailto_handler.h (right):
> > 
> >
>
https://codereview.chromium.org/2852003002/diff/60001/ios/chrome/browser/web/...
> > ios/chrome/browser/web/mailto_handler.h:28: - (BOOL)isAvailable;
> > On 2017/05/04 14:08:47, jif wrote:
> > > On 2017/05/03 22:17:18, pkl -pls.ping.after.24h. wrote:
> > > > On 2017/05/03 13:01:35, jif wrote:
> > > > > I think the subclass should return the Mail client's scheme instead:
> > > > > mailto:// for Apple Mail.
> > > > > googlegmail:// for Gmail.
> > > > > 
> > > > >  This way, we'd avoid duplicating the 
> > > > > "[[UIApplication sharedApplication] canOpenURL:[NSURL URLWithString:"
> > logic.
> > > > > 
> > > > > Also, this information may allow us to check at runtime that the
> > > > > LSApplicationQueriesSchemes section of the app's .plist contains the
> > > schemes.
> > > > > This can be done in a separate CL.
> > > > 
> > > > May be I don't understand completely. Having this return the URL scheme
> does
> > > not
> > > > tell us whether Gmail is installed or not.
> > > 
> > > Indeed it does not.
> > > I'm suggesting factoring the code actually  doing the check out of the
> > > subclasses,
> > > to the base class.
> > > Here's some pseudo code showing what I have in mind:
> > > 
> > > 
> > > 
> > > 
> > > mailto_handler.mm would contain something like:
> > > 
> > > - (NSString*)applicationScheme {
> > >   // This should be implemented by subclasses.
> > >   return nil;
> > > }
> > > 
> > > - (BOOL)isAvailable {
> > >   NSString* scheme = [self applicationScheme];
> > >   DCHECK( plist contains |scheme| );
> > >   return [[UIApplication sharedApplication] canOpenURL:[NSURL
> > > URLWithString:scheme]];
> > > }
> > > 
> > > 
> > > 
> > > 
> > > mailto_handler_system_mail.mm would contain something like:
> > > 
> > > - (NSString*)applicationScheme {
> > >   return @"mailto://";
> > > }
> > > 
> > > 
> > > 
> > > 
> > > mailto_handler_gmail.mm would contain something like:
> > > 
> > > - (NSString*)applicationScheme {
> > >   return @"googlemail://";
> > > }
> > 
> > Seems like you missed this reply (which is in the Patch Set 4 ).
> 
> I did. Please check mail_handler.mm -isAvailable implementation. I just did
not
> change the name to applicationScheme. I thought about that but ended up
keeping
> the name beginningScheme.

BTW, I did not add the check for scheme in plist which I think is unnecessary.

[jif, 2017-05-05 15:56:42]

lgtm

[rohitrao (ping after 24h), 2017-05-06 00:09:41]

https://codereview.chromium.org/2852003002/diff/180001/ios/chrome/browser/web...
File ios/chrome/browser/web/mailto_handler_system_mail.h (right):

https://codereview.chromium.org/2852003002/diff/180001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_handler_system_mail.h:11: @interface
MailtoHandlerSystemMail : MailtoHandler
Why do we maintain a handler for apple mail?  Won't the system default to apple
mail if we leave the URL untouched?

https://codereview.chromium.org/2852003002/diff/180001/ios/chrome/browser/web...
File ios/chrome/browser/web/mailto_url_rewriter.h (right):

https://codereview.chromium.org/2852003002/diff/180001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_url_rewriter.h:33: - (NSString*)defaultHandlerID;
Could this/should this be a property?

https://codereview.chromium.org/2852003002/diff/180001/ios/chrome/browser/web...
File ios/chrome/browser/web/mailto_url_rewriter.mm (right):

https://codereview.chromium.org/2852003002/diff/180001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_url_rewriter.mm:112: - (void)convertLegacyOptions
{
I don't fully follow the logic in this method.  It looks like
|kMailtoDefaultHandlerKey| is the new pref, and |kLegacyShouldAutoOpenKey| is
the old pref.  If we only have the old pref but not the new pref, we go through
migration.  That all makes sense.

But then there are places where we reset the new pref, for example if gmail is
no longer installed.  Does that mean that we might go through this migration
more than once?

https://codereview.chromium.org/2852003002/diff/180001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_url_rewriter.mm:136: // with Gmail, upgrade will
set Gmail app to be the default mailto: handler.
What's the rationale for choosing gmail to be the default handler?

https://codereview.chromium.org/2852003002/diff/180001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_url_rewriter.mm:138: [defaults
setObject:kGmailAppStoreID forKey:kMailtoDefaultHandlerKey];
If the user has migrated successfully in the past but now gmail is uninstalled,
do we clear the new pref at line 127 and then set it back to gmail here at line
138?

[Alexei Svitkine (slow), 2017-05-08 14:56:01]

https://codereview.chromium.org/2852003002/diff/220001/ios/chrome/browser/web...
File ios/chrome/browser/web/external_app_launcher.mm (right):

https://codereview.chromium.org/2852003002/diff/220001/ios/chrome/browser/web...
ios/chrome/browser/web/external_app_launcher.mm:201:
UMA_HISTOGRAM_BOOLEAN("MailtoURLRewriter.Rewritten", false);
Each UMA macro expands to a bunch of code. Please move this logging to below the
if in a single macro and pass in launchURL != nil

https://codereview.chromium.org/2852003002/diff/220001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2852003002/diff/220001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:26097: +<histogram
name="MailtoURLRewriter.Rewritten">
Please associate this histogram with an enum. e.g. enum="Rewritten" - create it
if it doesn't exist.

https://codereview.chromium.org/2852003002/diff/220001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:26103: +</histogram>
We don't recommend adding a new top-level prefix if it won't be used for a lot
metrics. Can this be nested under an existing prefix? Maybe under IOS.?

[pkl (ping after 24h if needed), 2017-05-08 17:07:56]

rohitrao & asvitkine: Addressed your comments. PTAL.

https://codereview.chromium.org/2852003002/diff/180001/ios/chrome/browser/web...
File ios/chrome/browser/web/mailto_handler_system_mail.h (right):

https://codereview.chromium.org/2852003002/diff/180001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_handler_system_mail.h:11: @interface
MailtoHandlerSystemMail : MailtoHandler
On 2017/05/06 00:09:41, rohitrao (ping after 24h) wrote:
> Why do we maintain a handler for apple mail?  Won't the system default to
apple
> mail if we leave the URL untouched?

In theory, we should be able to leave the URL alone. But treating system Mail in
the same way as 3rd party mail clients eliminates the need to special case the
UI implementation of in Settings (coming up next) for system-provided Mail. It
is "just one of the mail clients".

https://codereview.chromium.org/2852003002/diff/180001/ios/chrome/browser/web...
File ios/chrome/browser/web/mailto_url_rewriter.h (right):

https://codereview.chromium.org/2852003002/diff/180001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_url_rewriter.h:33: - (NSString*)defaultHandlerID;
On 2017/05/06 00:09:41, rohitrao (ping after 24h) wrote:
> Could this/should this be a property?

Indeed it can be declared as a property in the API. The internal implementation
does not need a backing ivar because the value is actually stored into
NSUserDefaults.

https://codereview.chromium.org/2852003002/diff/180001/ios/chrome/browser/web...
File ios/chrome/browser/web/mailto_url_rewriter.mm (right):

https://codereview.chromium.org/2852003002/diff/180001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_url_rewriter.mm:112: - (void)convertLegacyOptions
{
On 2017/05/06 00:09:41, rohitrao (ping after 24h) wrote:
> I don't fully follow the logic in this method.  It looks like
> |kMailtoDefaultHandlerKey| is the new pref, and |kLegacyShouldAutoOpenKey| is
> the old pref.  If we only have the old pref but not the new pref, we go
through
> migration.  That all makes sense.
> 
> But then there are places where we reset the new pref, for example if gmail is
> no longer installed.  Does that mean that we might go through this migration
> more than once?

The migration path should be executed just once. It is based on the existence of
the legacyValue (line 137) which is removed (line 152) as soon as one of the
migration step is taken.

https://codereview.chromium.org/2852003002/diff/180001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_url_rewriter.mm:136: // with Gmail, upgrade will
set Gmail app to be the default mailto: handler.
On 2017/05/06 00:09:41, rohitrao (ping after 24h) wrote:
> What's the rationale for choosing gmail to be the default handler?

This preserves the behavior in Google App Launcher.

https://codereview.chromium.org/2852003002/diff/180001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_url_rewriter.mm:138: [defaults
setObject:kGmailAppStoreID forKey:kMailtoDefaultHandlerKey];
On 2017/05/06 00:09:41, rohitrao (ping after 24h) wrote:
> If the user has migrated successfully in the past but now gmail is
uninstalled,
> do we clear the new pref at line 127 and then set it back to gmail here at
line
> 138?

If user deletes Gmail app after a successful migration, line 127 deletes the
MailtoDefaultHandler key. This puts the user in the same state as a new user who
never used any previous versions of Chrome and never used Gmail. That's the
intended behavior.

When the user re-installs Gmail app, this !legacyValue case sets Gmail to be the
new default.

https://codereview.chromium.org/2852003002/diff/220001/ios/chrome/browser/web...
File ios/chrome/browser/web/external_app_launcher.mm (right):

https://codereview.chromium.org/2852003002/diff/220001/ios/chrome/browser/web...
ios/chrome/browser/web/external_app_launcher.mm:201:
UMA_HISTOGRAM_BOOLEAN("MailtoURLRewriter.Rewritten", false);
On 2017/05/08 14:56:01, Alexei Svitkine (slow) wrote:
> Each UMA macro expands to a bunch of code. Please move this logging to below
the
> if in a single macro and pass in launchURL != nil

Great point! I was thinking of the number of times launchURL is tested, not from
the macro expansion. Fixed now.

https://codereview.chromium.org/2852003002/diff/220001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2852003002/diff/220001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:26097: +<histogram
name="MailtoURLRewriter.Rewritten">
On 2017/05/08 14:56:01, Alexei Svitkine (slow) wrote:
> Please associate this histogram with an enum. e.g. enum="Rewritten" - create
it
> if it doesn't exist.

I think enum="Boolean" is rather clear given the rename of
IOS.MailtoURLRewritten.
Let me know if there are other advantages of defining an enum that's "Rewritten"
/ "Not Rewritten" (over "True" and "False").

https://codereview.chromium.org/2852003002/diff/220001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:26103: +</histogram>
On 2017/05/08 14:56:01, Alexei Svitkine (slow) wrote:
> We don't recommend adding a new top-level prefix if it won't be used for a lot
> metrics. Can this be nested under an existing prefix? Maybe under IOS.?

Renamed to IOS.MailtoURLRewritten.

[Alexei Svitkine (slow), 2017-05-08 18:49:28]

lgtm

[rohitrao (ping after 24h), 2017-05-08 18:57:50]

https://codereview.chromium.org/2852003002/diff/240001/ios/chrome/browser/web...
File ios/chrome/browser/web/mailto_url_rewriter.mm (right):

https://codereview.chromium.org/2852003002/diff/240001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_url_rewriter.mm:155: [defaults
removeObjectForKey:kMailtoDefaultHandlerKey];
In what cases could this line (and line 169) have an effect?  It looks like if
any object is registered for this key, we would have already returned early at
line 133 above.

[pkl (ping after 24h if needed), 2017-05-08 22:15:43]

https://codereview.chromium.org/2852003002/diff/240001/ios/chrome/browser/web...
File ios/chrome/browser/web/mailto_url_rewriter.mm (right):

https://codereview.chromium.org/2852003002/diff/240001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_url_rewriter.mm:155: [defaults
removeObjectForKey:kMailtoDefaultHandlerKey];
On 2017/05/08 18:57:50, rohitrao (ping after 24h) wrote:
> In what cases could this line (and line 169) have an effect?  It looks like if
> any object is registered for this key, we would have already returned early at
> line 133 above.

This and the previous if-clause can be merged to handle the case when Gmail app
is not installed. In that case, the important part is blowing away the legacy
key so we won't try upgrade again.

Line 169 is relevant for users who previously turned off Gmail app for mailto:.
In the migrated state, we need to have kMailtoDefaultHandlerKey removed so
-defaultHandlerID (see above) returns the system Mail app.

[pkl (ping after 24h if needed), 2017-05-09 23:44:10]

rohitrao: Please review patchset 15-18.

I've renamed convertLegacyOptions to migrateLegacyOptions
because the name is a bit more appropriate, but the main change
is the order of checking for legacy options before the user-set
default handler.

Also changed the initialization so there's less boilerplate in the
unit tests.

Having the unit tests ahead of time allowed me to fearlessly
write and rewrite *LegacyOptions multiple times :)

PTAL

[rohitrao (ping after 24h), 2017-05-09 23:52:21]

https://codereview.chromium.org/2852003002/diff/340001/ios/chrome/browser/web...
File ios/chrome/browser/web/mailto_url_rewriter.mm (right):

https://codereview.chromium.org/2852003002/diff/340001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_url_rewriter.mm:190: // For users who have not
made an explict choice (kMailtoDefaultHandlerKey
Typo: explicit.

https://codereview.chromium.org/2852003002/diff/340001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_url_rewriter.mm:190: // For users who have not
made an explict choice (kMailtoDefaultHandlerKey
The absence of kMailtoDefaultHandlerKey could happen even if the user makes an
explicit choice to use system mail, right?

https://codereview.chromium.org/2852003002/diff/340001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_url_rewriter.mm:191: // not set), if Gmail is
detected, make an explicit choice for the user to
If I choose to use system mail, then later install the Gmail app, we will change
the default mailto handler in response.  Is that the intended behavior?

Actually, does this make it impossible to use system mail when gmail is
installed?  Will we constantly change the pref to gmail on startup?

[pkl (ping after 24h if needed), 2017-05-10 00:02:19]

Mostly answering your questions at this time. I will upload a patch with the
typo fix in a bit.

https://codereview.chromium.org/2852003002/diff/340001/ios/chrome/browser/web...
File ios/chrome/browser/web/mailto_url_rewriter.mm (right):

https://codereview.chromium.org/2852003002/diff/340001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_url_rewriter.mm:190: // For users who have not
made an explict choice (kMailtoDefaultHandlerKey
On 2017/05/09 23:52:20, rohitrao (ping after 24h) wrote:
> Typo: explicit.

Yes, will fix that.

https://codereview.chromium.org/2852003002/diff/340001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_url_rewriter.mm:190: // For users who have not
made an explict choice (kMailtoDefaultHandlerKey
On 2017/05/09 23:52:21, rohitrao (ping after 24h) wrote:
> The absence of kMailtoDefaultHandlerKey could happen even if the user makes an
> explicit choice to use system mail, right?

From Settings UI, user can set Mail app as the handler. That will go through
-setDefaultHandlerID: which sets kMailtoDefaultHandlerKey. This means that once
user has made an explicit choice through Settings UI, kMailtoDefaultHandler key
will never become unset.

https://codereview.chromium.org/2852003002/diff/340001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_url_rewriter.mm:191: // not set), if Gmail is
detected, make an explicit choice for the user to
On 2017/05/09 23:52:20, rohitrao (ping after 24h) wrote:
> If I choose to use system mail, then later install the Gmail app, we will
change
> the default mailto handler in response.  Is that the intended behavior?
> 
> Actually, does this make it impossible to use system mail when gmail is
> installed?  Will we constantly change the pref to gmail on startup?

If user has made a choice, this migration code will early return on line 188.
So, no, user will not be reverted to Gmail every time.

[rohitrao (ping after 24h), 2017-05-10 00:09:18]

Ok, lgtm

https://codereview.chromium.org/2852003002/diff/340001/ios/chrome/browser/web...
File ios/chrome/browser/web/mailto_url_rewriter.mm (right):

https://codereview.chromium.org/2852003002/diff/340001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_url_rewriter.mm:105: + (void)resetDefaultHandlerID
{
What code will call this function and reset the handler ID?  Can the user
trigger this operation somehow?

https://codereview.chromium.org/2852003002/diff/340001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_url_rewriter.mm:185: // If a default handler for
mailto: has already been registered, this means
The rest of this function after this line isn't really related to migration at
all.  Perhaps it's worth moving to a separate function, something like
switchHandlerToGmailIfInstalled?

[pkl (ping after 24h if needed), 2017-05-10 01:04:14]

Thank you!

https://codereview.chromium.org/2852003002/diff/340001/ios/chrome/browser/web...
File ios/chrome/browser/web/mailto_url_rewriter.mm (right):

https://codereview.chromium.org/2852003002/diff/340001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_url_rewriter.mm:105: + (void)resetDefaultHandlerID
{
On 2017/05/10 00:09:18, rohitrao (ping after 24h) wrote:
> What code will call this function and reset the handler ID?  Can the user
> trigger this operation somehow?

No. This is only used by unit tests. I renamed it to *ForTesting to make it
obvious.

https://codereview.chromium.org/2852003002/diff/340001/ios/chrome/browser/web...
ios/chrome/browser/web/mailto_url_rewriter.mm:185: // If a default handler for
mailto: has already been registered, this means
On 2017/05/10 00:09:18, rohitrao (ping after 24h) wrote:
> The rest of this function after this line isn't really related to migration at
> all.  Perhaps it's worth moving to a separate function, something like
> switchHandlerToGmailIfInstalled?

Done.

[pkl (ping after 24h if needed), 2017-05-10 01:04:29]

The CQ bit was checked by pkl@chromium.org

[pkl (ping after 24h if needed), 2017-05-10 01:04:30]

The patchset sent to the CQ was uploaded after l-g-t-m from jif@chromium.org,
asvitkine@chromium.org, rohitrao@chromium.org
Link to the patchset: https://codereview.chromium.org/2852003002/#ps380001
(title: "separate migration from auto-default even more")

[commit-bot: I haz the power, 2017-05-10 01:05:40]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 03:08:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 03:08:38]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has
not started yet; builder either lacks capacity or does not exist (misspelled?))
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet;
builder either lacks capacity or does not exist (misspelled?))
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
build has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))

[pkl (ping after 24h if needed), 2017-05-10 03:11:22]

The CQ bit was checked by pkl@chromium.org

[commit-bot: I haz the power, 2017-05-10 03:12:42]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 07:21:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 07:21:39]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pkl (ping after 24h if needed), 2017-05-10 12:47:03]

Description was changed from

==========
Adds mailto: URL support to app launching.

Known Mail client apps are managed by a singleton class of
MailtoURLRewriter. This CL adds support for the default system
Mail client app and also for Gmail.

BUG=711511
==========

to

==========
Adds mailto: URL support to app launching.

Known Mail client apps are managed by a singleton class of
MailtoURLRewriter. This CL adds support for the default system
Mail client app and also for Gmail.

BUG=711511
NOTRY=true
==========

[pkl (ping after 24h if needed), 2017-05-10 12:47:16]

The CQ bit was checked by pkl@chromium.org

[commit-bot: I haz the power, 2017-05-10 12:47:47]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 12:53:17]

CQ is committing da patch.

Bot data: {"patchset_id": 380001, "attempt_start_ts": 1494420436152550,
"parent_rev": "4c61cbbb6d255c2a846deaf9e3979843c5756b1b", "commit_rev":
"94313a25439ae14d5544311a274e160f41334769"}

[commit-bot: I haz the power, 2017-05-10 12:53:27]

Description was changed from

==========
Adds mailto: URL support to app launching.

Known Mail client apps are managed by a singleton class of
MailtoURLRewriter. This CL adds support for the default system
Mail client app and also for Gmail.

BUG=711511
NOTRY=true
==========

to

==========
Adds mailto: URL support to app launching.

Known Mail client apps are managed by a singleton class of
MailtoURLRewriter. This CL adds support for the default system
Mail client app and also for Gmail.

BUG=711511
NOTRY=true

Review-Url: https://codereview.chromium.org/2852003002
Cr-Commit-Position: refs/heads/master@{#470544}
Committed:
https://chromium.googlesource.com/chromium/src/+/94313a25439ae14d5544311a274e...
==========

[commit-bot: I haz the power, 2017-05-10 12:53:33]

Committed patchset #20 (id:380001) as
https://chromium.googlesource.com/chromium/src/+/94313a25439ae14d5544311a274e...