Add percentage of progress in notification header.

Add percentage of progress in notification header.

Add percentage of progress in the header of new-style notification, so
that it would be consistent with the mock.

BUG=726245
TEST=manual, including environment with RTL language (Hebrew).
Android does not have this for progress bar at system level, so consistency wouldn't be a problem for this.

Review-Url: https://codereview.chromium.org/2945303006
Cr-Commit-Position: refs/heads/master@{#482574}
Committed: https://chromium.googlesource.com/chromium/src/+/0a7d1693d5a2effc4539153daa67023deccbc200

[tetsui, 2017-06-22 08:29:47]

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

[commit-bot: I haz the power, 2017-06-22 08:29:57]

Dry run: CQ is trying da patch.

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

[tetsui, 2017-06-22 08:30:41]

tetsui@chromium.org changed reviewers:
+ fukino@chromium.org, yoshiki@chromium.org

[tetsui, 2017-06-22 08:30:42]

PTAL. Thanks!

[tetsui, 2017-06-22 08:34:07]

Description was changed from

==========
Add percentage of progress in notification header.

Add percentage of progress in the header of new-style notification, so
that it would be consistent with the mock.

BUG=726245
TEST=manual
==========

to

==========
Add percentage of progress in notification header.

Add percentage of progress in the header of new-style notification, so
that it would be consistent with the mock.

BUG=726245
TEST=manual, including environment with RTL language (Hebrew).
Android does not have this for progress bar at system level, so consistency
wouldn't be a problem for this.
==========

[fukino, 2017-06-22 08:57:43]

https://codereview.chromium.org/2945303006/diff/1/ui/message_center/views/not...
File ui/message_center/views/notification_header_view.h (right):

https://codereview.chromium.org/2945303006/diff/1/ui/message_center/views/not...
ui/message_center/views/notification_header_view.h:25: void SetSummaryText(const
base::string16& summary_text);
optional nit: Instead of having a direct setter for the string, could you add
SetProgress (and SetTimestamp for other notification types) and leave the
responsibility of visual representation to NotificationHeaderView class?

https://codereview.chromium.org/2945303006/diff/1/ui/strings/ui_strings.grd
File ui/strings/ui_strings.grd (right):

https://codereview.chromium.org/2945303006/diff/1/ui/strings/ui_strings.grd#n...
ui/strings/ui_strings.grd:634: <message
name="IDS_MESSAGE_CENTER_NOTIFICATION_HEADER_DIVIDER_SYMBOL_WITH_SPACES"
desc="The divider symbol between different parts of the notification header
including spaces." translateable="false">
translatable?

https://codereview.chromium.org/2945303006/diff/1/ui/strings/ui_strings.grd#n...
ui/strings/ui_strings.grd:635: ''' • '''
I'm not sure if we can expect that all fonts we use have a glyph for •.
Maybe we should use an image for this separator?

[commit-bot: I haz the power, 2017-06-22 09:42:26]

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

[commit-bot: I haz the power, 2017-06-22 09:42:27]

Dry run: This issue passed the CQ dry run.

[yoshiki, 2017-06-22 13:27:38]

https://codereview.chromium.org/2945303006/diff/1/ui/message_center/views/not...
File ui/message_center/views/notification_view_md.cc (right):

https://codereview.chromium.org/2945303006/diff/1/ui/message_center/views/not...
ui/message_center/views/notification_view_md.cc:518:
header_row_->SetSummaryText(base::IntToString16(notification.progress()) +
Could you make this a localized string?

https://codereview.chromium.org/2945303006/diff/1/ui/strings/ui_strings.grd
File ui/strings/ui_strings.grd (right):

https://codereview.chromium.org/2945303006/diff/1/ui/strings/ui_strings.grd#n...
ui/strings/ui_strings.grd:635: ''' • '''
On 2017/06/22 08:57:43, fukino wrote:
> I'm not sure if we can expect that all fonts we use have a glyph for •.
> Maybe we should use an image for this separator?

Android uses character, so IMO this character is ok.
https://android.googlesource.com/platform/frameworks/base/+/master/core/res/r...

I think it's better to have it as a constant in source code. like:

base::char16 kNotificationHeaderDividerSymbol = 0x2022;

[tetsui, 2017-06-23 02:30:20]

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

[commit-bot: I haz the power, 2017-06-23 02:30:42]

Dry run: CQ is trying da patch.

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

[tetsui, 2017-06-23 02:33:04]

https://codereview.chromium.org/2945303006/diff/1/ui/message_center/views/not...
File ui/message_center/views/notification_header_view.h (right):

https://codereview.chromium.org/2945303006/diff/1/ui/message_center/views/not...
ui/message_center/views/notification_header_view.h:25: void SetSummaryText(const
base::string16& summary_text);
On 2017/06/22 08:57:43, fukino wrote:
> optional nit: Instead of having a direct setter for the string, could you add
> SetProgress (and SetTimestamp for other notification types) and leave the
> responsibility of visual representation to NotificationHeaderView class?

Done.

https://codereview.chromium.org/2945303006/diff/1/ui/strings/ui_strings.grd
File ui/strings/ui_strings.grd (right):

https://codereview.chromium.org/2945303006/diff/1/ui/strings/ui_strings.grd#n...
ui/strings/ui_strings.grd:634: <message
name="IDS_MESSAGE_CENTER_NOTIFICATION_HEADER_DIVIDER_SYMBOL_WITH_SPACES"
desc="The divider symbol between different parts of the notification header
including spaces." translateable="false">
On 2017/06/22 08:57:43, fukino wrote:
> translatable?

Removed. FYI: even though translateable seems misspelling, it's correct in
grit... https://cs.chromium.org/search/?q=translateable+grd&type=cs

https://codereview.chromium.org/2945303006/diff/1/ui/strings/ui_strings.grd#n...
ui/strings/ui_strings.grd:635: ''' • '''
On 2017/06/22 13:27:37, yoshiki wrote:
> On 2017/06/22 08:57:43, fukino wrote:
> > I'm not sure if we can expect that all fonts we use have a glyph for •.
> > Maybe we should use an image for this separator?
> 
> Android uses character, so IMO this character is ok.
>
https://android.googlesource.com/platform/frameworks/base/+/master/core/res/r...
> 
> I think it's better to have it as a constant in source code. like:
> 
> base::char16 kNotificationHeaderDividerSymbol = 0x2022;

Done.

[commit-bot: I haz the power, 2017-06-23 02:44:22]

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

[commit-bot: I haz the power, 2017-06-23 02:44:23]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[tetsui, 2017-06-23 02:53:38]

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

[commit-bot: I haz the power, 2017-06-23 02:53:47]

Dry run: 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-06-23 03:09:39]

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

[commit-bot: I haz the power, 2017-06-23 03:09:40]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[tetsui, 2017-06-23 06:49:06]

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

[commit-bot: I haz the power, 2017-06-23 06:49:15]

Dry run: 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-06-23 07:05:08]

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

[commit-bot: I haz the power, 2017-06-23 07:05:09]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[tetsui, 2017-06-26 02:15:26]

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

[commit-bot: I haz the power, 2017-06-26 02:15:30]

Dry run: 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-06-26 03:16:49]

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

[commit-bot: I haz the power, 2017-06-26 03:16:50]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[tetsui, 2017-06-26 03:17:55]

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

[commit-bot: I haz the power, 2017-06-26 03:17:58]

Dry run: 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-06-26 03:31:51]

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

[commit-bot: I haz the power, 2017-06-26 03:31:52]

Dry run: This issue passed the CQ dry run.

[tetsui, 2017-06-26 07:32:36]

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

[commit-bot: I haz the power, 2017-06-26 07:32:45]

Dry run: 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-06-26 07:40:46]

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

[commit-bot: I haz the power, 2017-06-26 07:40:47]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[tetsui, 2017-06-26 07:43:46]

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

[commit-bot: I haz the power, 2017-06-26 07:43:57]

Dry run: 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-06-26 08:43:06]

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

[commit-bot: I haz the power, 2017-06-26 08:43:07]

Dry run: This issue passed the CQ dry run.

[yoshiki, 2017-06-27 07:15:18]

lgtm with nits

https://codereview.chromium.org/2945303006/diff/60001/ui/message_center/views...
File ui/message_center/views/notification_header_view.cc (right):

https://codereview.chromium.org/2945303006/diff/60001/ui/message_center/views...
ui/message_center/views/notification_header_view.cc:33: constexpr base::char16
kNotificationHeaderDividerSymbol = 0x2022;
nit: could you explain about the character as a comment?

https://codereview.chromium.org/2945303006/diff/60001/ui/message_center/views...
ui/message_center/views/notification_header_view.cc:132: has_progress_ = true;
nit: has_summary_text_ might be better? it's up to you.

[tetsui, 2017-06-27 07:37:36]

Thank you!

https://codereview.chromium.org/2945303006/diff/60001/ui/message_center/views...
File ui/message_center/views/notification_header_view.cc (right):

https://codereview.chromium.org/2945303006/diff/60001/ui/message_center/views...
ui/message_center/views/notification_header_view.cc:33: constexpr base::char16
kNotificationHeaderDividerSymbol = 0x2022;
On 2017/06/27 07:15:18, yoshiki wrote:
> nit: could you explain about the character as a comment?

Done.

https://codereview.chromium.org/2945303006/diff/60001/ui/message_center/views...
ui/message_center/views/notification_header_view.cc:132: has_progress_ = true;
On 2017/06/27 07:15:18, yoshiki wrote:
> nit: has_summary_text_ might be better? it's up to you.

Done.

[tetsui, 2017-06-27 07:37:44]

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

[commit-bot: I haz the power, 2017-06-27 07:37:52]

Dry run: 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-06-27 08:44:25]

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

[commit-bot: I haz the power, 2017-06-27 08:44:28]

Dry run: This issue passed the CQ dry run.

[tetsui, 2017-06-27 08:46:29]

The CQ bit was checked by tetsui@chromium.org

[tetsui, 2017-06-27 08:46:30]

The patchset sent to the CQ was uploaded after l-g-t-m from yoshiki@chromium.org
Link to the patchset: https://codereview.chromium.org/2945303006/#ps80001
(title: "Resolve review comments.")

[commit-bot: I haz the power, 2017-06-27 08:46:46]

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-06-27 08:52:26]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1498553189612100,
"parent_rev": "1663db4b63adac8fc9d8b0676d4e0023cdfa63b7", "commit_rev":
"0a7d1693d5a2effc4539153daa67023deccbc200"}

[commit-bot: I haz the power, 2017-06-27 08:52:45]

Description was changed from

==========
Add percentage of progress in notification header.

Add percentage of progress in the header of new-style notification, so
that it would be consistent with the mock.

BUG=726245
TEST=manual, including environment with RTL language (Hebrew).
Android does not have this for progress bar at system level, so consistency
wouldn't be a problem for this.
==========

to

==========
Add percentage of progress in notification header.

Add percentage of progress in the header of new-style notification, so
that it would be consistent with the mock.

BUG=726245
TEST=manual, including environment with RTL language (Hebrew).
Android does not have this for progress bar at system level, so consistency
wouldn't be a problem for this.

Review-Url: https://codereview.chromium.org/2945303006
Cr-Commit-Position: refs/heads/master@{#482574}
Committed:
https://chromium.googlesource.com/chromium/src/+/0a7d1693d5a2effc4539153daa67...
==========

[commit-bot: I haz the power, 2017-06-27 08:52:46]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/0a7d1693d5a2effc4539153daa67...