Linux native notifications: Add percentage to title for progress notifications

Linux native notifications: Add percentage to title for progress notifications

BUG=676220
R=peter@chromium.org

Review-Url: https://codereview.chromium.org/2866333002
Cr-Commit-Position: refs/heads/master@{#470738}
Committed: https://chromium.googlesource.com/chromium/src/+/6112abaaab5e7495941cddfe3a771a84f4c91bd7

[Tom (Use chromium acct), 2017-05-09 21:14:43]

Patchset #1 (id:1) has been deleted

[Peter Beverloo, 2017-05-09 22:51:54]

For this, as well as the following two CLs, could you see if there's a simple
way we can add a unit test?

https://codereview.chromium.org/2875443002/
https://codereview.chromium.org/2866363002/

lgtm

https://codereview.chromium.org/2866333002/diff/20001/chrome/browser/notifica...
File chrome/browser/notifications/notification_platform_bridge_linux.cc (right):

https://codereview.chromium.org/2866333002/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_linux.cc:437: summary
= base::IntToString(notification->progress()) + "% - " + summary;
nit: prefer base::FormatPercent()

You should probably be able to copy/paste the implementation here:

https://cs.chromium.org/chromium/src/chrome/browser/notifications/notificatio...

(We'll generalize on the third user, which I'm fairly sure is coming up soon.)

[Tom (Use chromium acct), 2017-05-10 20:10:20]

thomasanderson@google.com changed reviewers:
+ thestig@chromium.org

[Tom (Use chromium acct), 2017-05-10 20:10:20]

On 2017/05/09 22:51:54, Peter Beverloo wrote:
> For this, as well as the following two CLs, could you see if there's a simple
> way we can add a unit test?
> 

Done.

Also, +thestig for the unit test changes

https://codereview.chromium.org/2866333002/diff/20001/chrome/browser/notifica...
File chrome/browser/notifications/notification_platform_bridge_linux.cc (right):

https://codereview.chromium.org/2866333002/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_linux.cc:437: summary
= base::IntToString(notification->progress()) + "% - " + summary;
On 2017/05/09 22:51:54, Peter Beverloo wrote:
> nit: prefer base::FormatPercent()
> 
> You should probably be able to copy/paste the implementation here:
> 
>
https://cs.chromium.org/chromium/src/chrome/browser/notifications/notificatio...
> 
> (We'll generalize on the third user, which I'm fairly sure is coming up soon.)

Done.

[Lei Zhang, 2017-05-10 20:42:47]

On 2017/05/09 22:51:54, Peter Beverloo wrote:
> You should probably be able to copy/paste the implementation here:
> 
>
https://cs.chromium.org/chromium/src/chrome/browser/notifications/notificatio...
> 
> (We'll generalize on the third user, which I'm fairly sure is coming up soon.)

And when we do that, do we want to look into localization a wee bit more? i.e.
is "50% - Title" the right format for all languages?

[Lei Zhang, 2017-05-10 20:45:28]

lgtm

https://codereview.chromium.org/2866333002/diff/30001/chrome/browser/notifica...
File chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc
(right):

https://codereview.chromium.org/2866333002/diff/30001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc:69:
struct NotificationRequest {
Is this going to have more members later?

[Tom (Use chromium acct), 2017-05-10 21:05:44]

https://codereview.chromium.org/2866333002/diff/30001/chrome/browser/notifica...
File chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc
(right):

https://codereview.chromium.org/2866333002/diff/30001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc:69:
struct NotificationRequest {
On 2017/05/10 20:45:28, Lei Zhang wrote:
> Is this going to have more members later?

Yes:
https://codereview.chromium.org/2875443002/

[Tom (Use chromium acct), 2017-05-10 21:05:47]

The CQ bit was checked by thomasanderson@google.com

[Tom (Use chromium acct), 2017-05-10 21:05:48]

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/2866333002/#ps30001
(title: "Add unit test")

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

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 21:38:38]

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

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

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Tom (Use chromium acct), 2017-05-10 22:29:18]

The CQ bit was checked by thomasanderson@google.com

[commit-bot: I haz the power, 2017-05-10 22:30:00]

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 22:49:05]

CQ is committing da patch.

Bot data: {"patchset_id": 30001, "attempt_start_ts": 1494455358854090,
"parent_rev": "90e6006c41c0013b8f48c02263e3a44c32da0c9c", "commit_rev":
"6112abaaab5e7495941cddfe3a771a84f4c91bd7"}

[commit-bot: I haz the power, 2017-05-10 22:49:14]

Description was changed from

==========
Linux native notifications: Add percentage to title for progress notifications

BUG=676220
R=peter@chromium.org
==========

to

==========
Linux native notifications: Add percentage to title for progress notifications

BUG=676220
R=peter@chromium.org

Review-Url: https://codereview.chromium.org/2866333002
Cr-Commit-Position: refs/heads/master@{#470738}
Committed:
https://chromium.googlesource.com/chromium/src/+/6112abaaab5e7495941cddfe3a77...
==========

[commit-bot: I haz the power, 2017-05-10 22:49:15]

Committed patchset #2 (id:30001) as
https://chromium.googlesource.com/chromium/src/+/6112abaaab5e7495941cddfe3a77...