Display ETLD+1 instead of the full domain in MacOSX native notifications

Display ETLD+1 instead of the full domain in MacOSX native notifications

BUG=

Review-Url: https://codereview.chromium.org/2827543003
Cr-Commit-Position: refs/heads/master@{#465242}
Committed: https://chromium.googlesource.com/chromium/src/+/d59da4c16d22505774ca9f9eae89402bec6efdc2

[Miguel Garcia, 2017-04-18 11:03:04]

The CQ bit was checked by miguelg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-18 11:03:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Miguel Garcia, 2017-04-18 11:07:55]

miguelg@chromium.org changed reviewers:
+ peter@chromium.org

[commit-bot: I haz the power, 2017-04-18 11:28:35]

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

[commit-bot: I haz the power, 2017-04-18 11:28:36]

Dry run: This issue passed the CQ dry run.

[Peter Beverloo, 2017-04-18 13:43:45]

https://codereview.chromium.org/2827543003/diff/1/chrome/browser/notification...
File chrome/browser/notifications/notification_platform_bridge_mac.mm (right):

https://codereview.chromium.org/2827543003/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_platform_bridge_mac.mm:199: NSString*
subtitle =
Could we maybe split this out into a helper function too, similar to
CreateNotificationTitle?

https://codereview.chromium.org/2827543003/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_platform_bridge_mac.mm:205:
INCLUDE_PRIVATE_REGISTRIES))
The concern I have with this is that IP addresses would return an empty string.
(That'd be an awesome test case to add.)

I guess we need something like:

if (!requires_attribution) {
  return notification.context_message()
} else {
  if (origin.HostIsIPAddress())
    return FormatOriginForSecurityDisplay()
  else
    return GetDomainAndRegistry()
}

[Miguel Garcia, 2017-04-18 14:44:24]

https://codereview.chromium.org/2827543003/diff/1/chrome/browser/notification...
File chrome/browser/notifications/notification_platform_bridge_mac.mm (right):

https://codereview.chromium.org/2827543003/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_platform_bridge_mac.mm:199: NSString*
subtitle =
On 2017/04/18 13:43:45, Peter Beverloo wrote:
> Could we maybe split this out into a helper function too, similar to
> CreateNotificationTitle?

Done.

https://codereview.chromium.org/2827543003/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_platform_bridge_mac.mm:205:
INCLUDE_PRIVATE_REGISTRIES))
True, localhost was also affected so I went with a slightly different version.

On 2017/04/18 13:43:45, Peter Beverloo wrote:
> The concern I have with this is that IP addresses would return an empty
string.
> (That'd be an awesome test case to add.)
> 
> I guess we need something like:
> 
> if (!requires_attribution) {
>   return notification.context_message()
> } else {
>   if (origin.HostIsIPAddress())
>     return FormatOriginForSecurityDisplay()
>   else
>     return GetDomainAndRegistry()
> }

[Peter Beverloo, 2017-04-18 14:48:54]

lgtm

https://codereview.chromium.org/2827543003/diff/20001/chrome/browser/notifica...
File chrome/browser/notifications/notification_platform_bridge_mac.mm (right):

https://codereview.chromium.org/2827543003/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:132:
NotificationCommon::Type notification_type) {
nit: I'd prefer to pass in `requires_attribution` as an argument as opposed to
`notification_type`. Keeps such type-based decisions together.

https://codereview.chromium.org/2827543003/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:144: //
localhost, raw IPs etc. are not handled by GetDomainAndRegistry
Maybe explicitly refer the reader to the GetDomainAndRegistry() documentation
for these edge-cases, instead of repeating them? Also a period to finish the
sentence.

[Miguel Garcia, 2017-04-18 14:58:12]

The CQ bit was checked by miguelg@chromium.org

[Miguel Garcia, 2017-04-18 14:58:13]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org
Link to the patchset: https://codereview.chromium.org/2827543003/#ps40001
(title: "format")

[commit-bot: I haz the power, 2017-04-18 14:58:26]

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

[Miguel Garcia, 2017-04-18 14:59:10]

https://codereview.chromium.org/2827543003/diff/20001/chrome/browser/notifica...
File chrome/browser/notifications/notification_platform_bridge_mac.mm (right):

https://codereview.chromium.org/2827543003/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:132:
NotificationCommon::Type notification_type) {
On 2017/04/18 14:48:54, Peter Beverloo wrote:
> nit: I'd prefer to pass in `requires_attribution` as an argument as opposed to
> `notification_type`. Keeps such type-based decisions together.

Done.

https://codereview.chromium.org/2827543003/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:144: //
localhost, raw IPs etc. are not handled by GetDomainAndRegistry
On 2017/04/18 14:48:54, Peter Beverloo wrote:
> Maybe explicitly refer the reader to the GetDomainAndRegistry() documentation
> for these edge-cases, instead of repeating them? Also a period to finish the
> sentence.

Added the period at the end.  I kind of like having a few examples right here
and GetDomainAndRegistry does not seem to talk about localhost apparently

[commit-bot: I haz the power, 2017-04-18 15:18:06]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492527492860080,
"parent_rev": "3c18743b967717d31f2a781542b667be8a5ff015", "commit_rev":
"d59da4c16d22505774ca9f9eae89402bec6efdc2"}

[commit-bot: I haz the power, 2017-04-18 15:18:15]

Description was changed from

==========
Display ETLD+1 instead of the full domain in MacOSX native notifications

BUG=
==========

to

==========
Display ETLD+1 instead of the full domain in MacOSX native notifications

BUG=

Review-Url: https://codereview.chromium.org/2827543003
Cr-Commit-Position: refs/heads/master@{#465242}
Committed:
https://chromium.googlesource.com/chromium/src/+/d59da4c16d22505774ca9f9eae89...
==========

[commit-bot: I haz the power, 2017-04-18 15:18:16]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/d59da4c16d22505774ca9f9eae89...