Adds Compose Email Settings UI for deprecating Native App Launcher

Adds Compose Email Settings UI for deprecating Native App Launcher

BUG=711511
Review-Url: https://codereview.chromium.org/2870783002
Cr-Commit-Position: refs/heads/master@{#471419}
Committed: https://chromium.googlesource.com/chromium/src/+/38f94f533d809b4202db39ac8c17be4442eb4256

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

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

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

Settings UI for selecting which app to handle mailto: URL.
The UI goes under Settings > Content Settings >.
I had to refactored some of the test rigs for mailto_url_rewriter for this.

[jif, 2017-05-11 14:03:58]

lgtm with nits

https://codereview.chromium.org/2870783002/diff/80001/ios/chrome/browser/ui/s...
File
ios/chrome/browser/ui/settings/compose_email_handler_collection_view_controller.h
(right):

https://codereview.chromium.org/2870783002/diff/80001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/settings/compose_email_handler_collection_view_controller.h:12:
// UI for Compose Email settings to allow user specify which installed Mail
...to allow users to specify...
?

https://codereview.chromium.org/2870783002/diff/80001/ios/chrome/browser/ui/s...
File
ios/chrome/browser/ui/settings/compose_email_handler_collection_view_controller.mm
(right):

https://codereview.chromium.org/2870783002/diff/80001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/settings/compose_email_handler_collection_view_controller.mm:112:
CollectionViewTextItem* newHandler =
we already have the new default engine with |selectedItem|.

In fact, I would move this logic (lines 111-116) right after line 96.

https://codereview.chromium.org/2870783002/diff/80001/ios/chrome/browser/ui/s...
File
ios/chrome/browser/ui/settings/content_settings_collection_view_controller.mm
(right):

https://codereview.chromium.org/2870783002/diff/80001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/settings/content_settings_collection_view_controller.mm:60:
MailtoURLRewriter* _mailtoURLRewriter;
optional:
add comment

https://codereview.chromium.org/2870783002/diff/80001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/settings/content_settings_collection_view_controller.mm:116:
[_mailtoURLRewriter setObserver:nil];
This is not necessary.

Filed a bug about removing "[_disablePopupsSetting setObserver:nil];" (
https://bugs.chromium.org/p/chromium/issues/detail?id=721339 ).

[pkl (ping after 24h if needed), 2017-05-11 17:23:16]

Thank you! I addressed jif's comments.
rohitrao: PTAL.

https://codereview.chromium.org/2870783002/diff/80001/ios/chrome/browser/ui/s...
File
ios/chrome/browser/ui/settings/compose_email_handler_collection_view_controller.h
(right):

https://codereview.chromium.org/2870783002/diff/80001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/settings/compose_email_handler_collection_view_controller.h:12:
// UI for Compose Email settings to allow user specify which installed Mail
On 2017/05/11 14:03:58, jif wrote:
> ...to allow users to specify...
> ?

Done-ish. Rewrote.

https://codereview.chromium.org/2870783002/diff/80001/ios/chrome/browser/ui/s...
File
ios/chrome/browser/ui/settings/compose_email_handler_collection_view_controller.mm
(right):

https://codereview.chromium.org/2870783002/diff/80001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/settings/compose_email_handler_collection_view_controller.mm:112:
CollectionViewTextItem* newHandler =
On 2017/05/11 14:03:58, jif wrote:
> we already have the new default engine with |selectedItem|.
> 
> In fact, I would move this logic (lines 111-116) right after line 96.

Integrating into the for-loop on line 100 makes even more sense.

https://codereview.chromium.org/2870783002/diff/80001/ios/chrome/browser/ui/s...
File
ios/chrome/browser/ui/settings/content_settings_collection_view_controller.mm
(right):

https://codereview.chromium.org/2870783002/diff/80001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/settings/content_settings_collection_view_controller.mm:60:
MailtoURLRewriter* _mailtoURLRewriter;
On 2017/05/11 14:03:58, jif wrote:
> optional:
> add comment

Done.

https://codereview.chromium.org/2870783002/diff/80001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/settings/content_settings_collection_view_controller.mm:116:
[_mailtoURLRewriter setObserver:nil];
On 2017/05/11 14:03:58, jif wrote:
> This is not necessary.
> 
> Filed a bug about removing "[_disablePopupsSetting setObserver:nil];" (
> https://bugs.chromium.org/p/chromium/issues/detail?id=721339 ).

Removed. Thanks.

[jif, 2017-05-11 17:44:12]

https://codereview.chromium.org/2870783002/diff/100001/ios/chrome/browser/ui/...
File
ios/chrome/browser/ui/settings/compose_email_handler_collection_view_controller.h
(right):

https://codereview.chromium.org/2870783002/diff/100001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/settings/compose_email_handler_collection_view_controller.h:14:
// installed before this UI should be shwon because there is no need to make
/shwon/shown/ :)

[pkl (ping after 24h if needed), 2017-05-11 21:20:45]

https://codereview.chromium.org/2870783002/diff/100001/ios/chrome/browser/ui/...
File
ios/chrome/browser/ui/settings/compose_email_handler_collection_view_controller.h
(right):

https://codereview.chromium.org/2870783002/diff/100001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/settings/compose_email_handler_collection_view_controller.h:14:
// installed before this UI should be shwon because there is no need to make
On 2017/05/11 17:44:12, jif wrote:
> /shwon/shown/ :)

Done.

[rohitrao (ping after 24h), 2017-05-12 01:57:58]

lgtm

https://codereview.chromium.org/2870783002/diff/120001/ios/chrome/browser/ui/...
File
ios/chrome/browser/ui/settings/compose_email_handler_collection_view_controller.mm
(right):

https://codereview.chromium.org/2870783002/diff/120001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/settings/compose_email_handler_collection_view_controller.mm:50:
- (instancetype)initWithStyle:(CollectionViewControllerStyle)style {
This implementation should not be necessary if you marked it as NS_UNAVAILABLE
in the header.

https://codereview.chromium.org/2870783002/diff/120001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/settings/compose_email_handler_collection_view_controller.mm:62:
// only one choice, it is considered a software error.
Why not always allow this UI to be shown, even if there is only one choice? 
It's a little silly, but at least the UI would always be present and always be
findable in the same place.

https://codereview.chromium.org/2870783002/diff/120001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/settings/compose_email_handler_collection_view_controller.mm:87:
base::mac::ObjCCastStrict<CollectionViewTextItem>(
ObjCCastStrict will DCHECK if the item is of a different class.  We're strict
about checking here, but not strict with the early return at line 90.  Feels
like we should consistently be either strict or not.

(Also, if the code stays unchanged, is it safe to assume that everything will be
a TextItem?  I think the answer is yes.)

https://codereview.chromium.org/2870783002/diff/120001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/settings/compose_email_handler_collection_view_controller.mm:89:
if (selectedItem.type != ItemTypeMailtoHandlers)
Do we display any rows that are not this type?  Could this be a DCHECK instead,
or is there some way to hit this early return in practice?

If this line actually does get hit, we should consider disabling selection for
those rows, instead of ignoring it after the fact.

https://codereview.chromium.org/2870783002/diff/120001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/settings/compose_email_handler_collection_view_controller.mm:98:
for (CollectionViewTextItem* item in
Are we somehow silently casting to CollectionViewTextItem here?  Should we use
ObjCCastStrict or ObjCCast?

https://codereview.chromium.org/2870783002/diff/120001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/settings/compose_email_handler_collection_view_controller.mm:100:
if (item.type != ItemTypeMailtoHandlers)
Same question -- could this be a DCHECK, or do we legitimately have items with a
different type?

https://codereview.chromium.org/2870783002/diff/120001/ios/chrome/browser/ui/...
File
ios/chrome/browser/ui/settings/content_settings_collection_view_controller.mm
(right):

https://codereview.chromium.org/2870783002/diff/120001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/settings/content_settings_collection_view_controller.mm:136:
[[_mailtoURLRewriter defaultHandlers] count] > 1) {
Consider always adding this page, rather than hiding it when there is only one
handler.

[pkl (ping after 24h if needed), 2017-05-12 18:01:20]

Addressed rohitrao's comments. I'm submitting in the current state, but can
change later based on decision of whether setting will appear all the time even
if there is only one choice available.

https://codereview.chromium.org/2870783002/diff/120001/ios/chrome/browser/ui/...
File
ios/chrome/browser/ui/settings/compose_email_handler_collection_view_controller.mm
(right):

https://codereview.chromium.org/2870783002/diff/120001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/settings/compose_email_handler_collection_view_controller.mm:50:
- (instancetype)initWithStyle:(CollectionViewControllerStyle)style {
On 2017/05/12 01:57:57, rohitrao (ping after 24h) wrote:
> This implementation should not be necessary if you marked it as NS_UNAVAILABLE
> in the header.

Done.

https://codereview.chromium.org/2870783002/diff/120001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/settings/compose_email_handler_collection_view_controller.mm:62:
// only one choice, it is considered a software error.
On 2017/05/12 01:57:57, rohitrao (ping after 24h) wrote:
> Why not always allow this UI to be shown, even if there is only one choice? 
> It's a little silly, but at least the UI would always be present and always be
> findable in the same place.

I've started a comment thread on the design doc. In the mean time, I'll submit
this CL in the current form. It's easy enough to change once a decision has been
reached.

https://codereview.chromium.org/2870783002/diff/120001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/settings/compose_email_handler_collection_view_controller.mm:87:
base::mac::ObjCCastStrict<CollectionViewTextItem>(
On 2017/05/12 01:57:57, rohitrao (ping after 24h) wrote:
> ObjCCastStrict will DCHECK if the item is of a different class.  We're strict
> about checking here, but not strict with the early return at line 90.  Feels
> like we should consistently be either strict or not.
> 
> (Also, if the code stays unchanged, is it safe to assume that everything will
be
> a TextItem?  I think the answer is yes.)

Using "strict" is right.

https://codereview.chromium.org/2870783002/diff/120001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/settings/compose_email_handler_collection_view_controller.mm:89:
if (selectedItem.type != ItemTypeMailtoHandlers)
On 2017/05/12 01:57:57, rohitrao (ping after 24h) wrote:
> Do we display any rows that are not this type?  Could this be a DCHECK
instead,
> or is there some way to hit this early return in practice?
> 
> If this line actually does get hit, we should consider disabling selection for
> those rows, instead of ignoring it after the fact.

Since the items are created in loadModel, the checking should be strict, so I
changed this early return into a DCHECK_EQ.

https://codereview.chromium.org/2870783002/diff/120001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/settings/compose_email_handler_collection_view_controller.mm:98:
for (CollectionViewTextItem* item in
On 2017/05/12 01:57:57, rohitrao (ping after 24h) wrote:
> Are we somehow silently casting to CollectionViewTextItem here?  Should we use
> ObjCCastStrict or ObjCCast?

Switched to use ObjCCastStrict<>.

https://codereview.chromium.org/2870783002/diff/120001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/settings/compose_email_handler_collection_view_controller.mm:100:
if (item.type != ItemTypeMailtoHandlers)
On 2017/05/12 01:57:57, rohitrao (ping after 24h) wrote:
> Same question -- could this be a DCHECK, or do we legitimately have items with
a
> different type?

...and made this a DCHECK_EQ

https://codereview.chromium.org/2870783002/diff/120001/ios/chrome/browser/ui/...
File
ios/chrome/browser/ui/settings/content_settings_collection_view_controller.mm
(right):

https://codereview.chromium.org/2870783002/diff/120001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/settings/content_settings_collection_view_controller.mm:136:
[[_mailtoURLRewriter defaultHandlers] count] > 1) {
On 2017/05/12 01:57:57, rohitrao (ping after 24h) wrote:
> Consider always adding this page, rather than hiding it when there is only one
> handler.

Acknowledged.

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

The CQ bit was checked by pkl@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from jif@chromium.org,
rohitrao@chromium.org
Link to the patchset: https://codereview.chromium.org/2870783002/#ps140001
(title: "replied to comments from rohitrao")

[commit-bot: I haz the power, 2017-05-12 18:03:34]

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-12 19:53:15]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1494612089444730,
"parent_rev": "55a3bc8f67171ac6a83e319458908c9ab3a02ec1", "commit_rev":
"38f94f533d809b4202db39ac8c17be4442eb4256"}

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

Description was changed from

==========
Adds Compose Email Settings UI for deprecating Native App Launcher

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

to

==========
Adds Compose Email Settings UI for deprecating Native App Launcher

BUG=711511

Review-Url: https://codereview.chromium.org/2870783002
Cr-Commit-Position: refs/heads/master@{#471419}
Committed:
https://chromium.googlesource.com/chromium/src/+/38f94f533d809b4202db39ac8c17...
==========

[commit-bot: I haz the power, 2017-05-12 19:53:36]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/38f94f533d809b4202db39ac8c17...