Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2861133003:
Only resort to etl+1 when the full domain does not fit. (Closed)
|
Created:
3 years, 7 months ago by Miguel Garcia Modified:
3 years, 7 months ago Reviewers:
Peter Beverloo CC:
chromium-reviews, Peter Beverloo, mlamouri+watch-notifications_chromium.org, awdf+watch_chromium.org, mac-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionOnly resort to etl+1 when the full domain does not fit.
Mac native notifications have a somewhat constrained space for the origin
Unlike chrome notifications it's harder to decide when to elide so instead
we either show the full domain (if it fits in the very conservative limit
we know works) or resort to etld+1 when it doesn't.
BUG=717725
Review-Url: https://codereview.chromium.org/2861133003
Cr-Commit-Position: refs/heads/master@{#469953}
Committed: https://chromium.googlesource.com/chromium/src/+/8c70962f2be02fc57fc5bea12c7f7052605709ce
Patch Set 1 #
Total comments: 20
Patch Set 2 : review #
Messages
Total messages: 14 (9 generated)
The CQ bit was checked by miguelg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
miguelg@chromium.org changed reviewers: + peter@chromium.org
lgtm https://codereview.chromium.org/2861133003/diff/1/chrome/browser/notification... File chrome/browser/notifications/notification_platform_bridge_mac.mm (right): https://codereview.chromium.org/2861133003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_mac.mm:136: // OSX notifications don't provide a good way to elide the domain (or tell you nit: OSX -> Mac OS https://codereview.chromium.org/2861133003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_mac.mm:137: // the maximum widh of the subtitle field) Therefore we use a bit of a mixed nit: widh -> width nit: period after the closing parenthesis https://codereview.chromium.org/2861133003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_mac.mm:138: // approach We have experimentally determined the maximum number of characters nit: period after "approach". Or just remove "Therefore...approach" altogether. https://codereview.chromium.org/2861133003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_mac.mm:139: // that fit using the widest possible character (m) If the domain fits in nit: period after (m) https://codereview.chromium.org/2861133003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_mac.mm:140: // those character we show it completely Otherwise we use etld+1 nit: period after "completely" nit: etld+1 -> eTLD + 1. (capitalisation of "TLD" is significant) https://codereview.chromium.org/2861133003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_mac.mm:143: // android platforms. ??????????????????????? https://codereview.chromium.org/2861133003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_mac.mm:149: const uint kMaxDomainLenghtBanner = 21; constexpr size_t kMaxDomainLength{Alert,Banner} https://codereview.chromium.org/2861133003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_mac.mm:151: uint max_characters = notification.never_timeout() ? kMaxDomainLenghtAlert size_t here too (we don't use the "uint" type w/o a length) https://codereview.chromium.org/2861133003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_mac.mm:151: uint max_characters = notification.never_timeout() ? kMaxDomainLenghtAlert Do we have to consider progress notifications here too? https://codereview.chromium.org/2861133003/diff/1/chrome/browser/notification... File chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm (right): https://codereview.chromium.org/2861133003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:446: "https://superduperlongdomaintests.peter.sh", https://somereallylongsubdomainthatactuallyisanaliasfortests.peter.sh/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by miguelg@chromium.org
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/2861133003/#ps20001 (title: "review")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2861133003/diff/1/chrome/browser/notification... File chrome/browser/notifications/notification_platform_bridge_mac.mm (right): https://codereview.chromium.org/2861133003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_mac.mm:136: // OSX notifications don't provide a good way to elide the domain (or tell you On 2017/05/05 14:55:51, Peter Beverloo wrote: > nit: OSX -> Mac OS Done. https://codereview.chromium.org/2861133003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_mac.mm:137: // the maximum widh of the subtitle field) Therefore we use a bit of a mixed On 2017/05/05 14:55:51, Peter Beverloo wrote: > nit: widh -> width > nit: period after the closing parenthesis Done. https://codereview.chromium.org/2861133003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_mac.mm:138: // approach We have experimentally determined the maximum number of characters On 2017/05/05 14:55:51, Peter Beverloo wrote: > nit: period after "approach". Or just remove "Therefore...approach" altogether. Done. https://codereview.chromium.org/2861133003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_mac.mm:139: // that fit using the widest possible character (m) If the domain fits in On 2017/05/05 14:55:51, Peter Beverloo wrote: > nit: period after (m) Done. https://codereview.chromium.org/2861133003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_mac.mm:140: // those character we show it completely Otherwise we use etld+1 On 2017/05/05 14:55:51, Peter Beverloo wrote: > nit: period after "completely" > nit: etld+1 -> eTLD + 1. (capitalisation of "TLD" is significant) Done. https://codereview.chromium.org/2861133003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_mac.mm:143: // android platforms. On 2017/05/05 14:55:51, Peter Beverloo wrote: > ??????????????????????? Acknowledged. https://codereview.chromium.org/2861133003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_mac.mm:149: const uint kMaxDomainLenghtBanner = 21; On 2017/05/05 14:55:51, Peter Beverloo wrote: > constexpr size_t kMaxDomainLength{Alert,Banner} For variables I think const and constexpr are the same right? https://codereview.chromium.org/2861133003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_mac.mm:151: uint max_characters = notification.never_timeout() ? kMaxDomainLenghtAlert On 2017/05/05 14:55:50, Peter Beverloo wrote: > size_t here too (we don't use the "uint" type w/o a length) Done. https://codereview.chromium.org/2861133003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_mac.mm:151: uint max_characters = notification.never_timeout() ? kMaxDomainLenghtAlert On 2017/05/05 14:55:51, Peter Beverloo wrote: > Do we have to consider progress notifications here too? Let's add it just for safety although since progress notifications don't require attribution this should not be a problem. https://codereview.chromium.org/2861133003/diff/1/chrome/browser/notification... File chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm (right): https://codereview.chromium.org/2861133003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:446: "https://superduperlongdomaintests.peter.sh", On 2017/05/05 14:55:51, Peter Beverloo wrote: > https://somereallylongsubdomainthatactuallyisanaliasfortests.peter.sh/ Done.
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1494238728831120,
"parent_rev": "505d1e6979447633985e1a297568c04d993a0f48", "commit_rev":
"8c70962f2be02fc57fc5bea12c7f7052605709ce"}
Message was sent while issue was closed.
Description was changed from ========== Only resort to etl+1 when the full domain does not fit. Mac native notifications have a somewhat constrained space for the origin Unlike chrome notifications it's harder to decide when to elide so instead we either show the full domain (if it fits in the very conservative limit we know works) or resort to etld+1 when it doesn't. BUG=717725 ========== to ========== Only resort to etl+1 when the full domain does not fit. Mac native notifications have a somewhat constrained space for the origin Unlike chrome notifications it's harder to decide when to elide so instead we either show the full domain (if it fits in the very conservative limit we know works) or resort to etld+1 when it doesn't. BUG=717725 Review-Url: https://codereview.chromium.org/2861133003 Cr-Commit-Position: refs/heads/master@{#469953} Committed: https://chromium.googlesource.com/chromium/src/+/8c70962f2be02fc57fc5bea12c7f... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/8c70962f2be02fc57fc5bea12c7f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
