Show small notification icons in notification tray

Show small notification icons in notification tray

This patch shows small notification icons (up to 3) of the
current notifications in the notification tray.

This also makes the counter show the count of all the
notification excluding the visible small icons (previously
it was the unread count).

See the issue for screenshots.

BUG=633399
TEST=manual test (with and without MD mode)

Committed: https://crrev.com/42754bb876e40c0c87ed63607452026190e13f78
Cr-Commit-Position: refs/heads/master@{#411019}

[yoshiki, 2016-08-04 12:31:14]

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

[commit-bot: I haz the power, 2016-08-04 12:31:29]

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

[yoshiki, 2016-08-04 12:38:27]

Description was changed from

==========
Show small notification icons in notification tray

This patch shows small notification icons (up to 3) in the notification
tray.

BUG=633399
TEST=manual test
==========

to

==========
Show small notification icons in notification tray

This patch shows small notification icons (up to 3) of the
current notifications in the notification tray.

This also makes the counter show the count of all the
notification excluding the visible small icons (previously
it was the unread count).

See the issue for screenshots.

BUG=633399
TEST=manual test
==========

[yoshiki, 2016-08-04 12:38:27]

yoshiki@chromium.org changed reviewers:
+ oshima@chromium.org, xiyuan@chromium.org

[yoshiki, 2016-08-04 12:39:44]

Xiyuan, Oshima, PTAL. Thanks.

[commit-bot: I haz the power, 2016-08-04 12:41:16]

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

[commit-bot: I haz the power, 2016-08-04 12:41:17]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yoshiki, 2016-08-04 12:45:11]

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

[commit-bot: I haz the power, 2016-08-04 12:45:18]

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, 2016-08-04 12:54:24]

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

[commit-bot: I haz the power, 2016-08-04 12:54:25]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yoshiki, 2016-08-04 13:04:57]

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

[commit-bot: I haz the power, 2016-08-04 13:05:08]

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

[yoshiki, 2016-08-04 13:17:44]

Patchset #2 (id:20001) has been deleted

[yoshiki, 2016-08-04 13:18:17]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-08-04 13:43:04]

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

[commit-bot: I haz the power, 2016-08-04 13:43:05]

Dry run: 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_...)

[oshima, 2016-08-04 18:15:21]

https://codereview.chromium.org/2209443006/diff/40001/ash/common/system/web_n...
File ash/common/system/web_notification/web_notification_tray.cc (right):

https://codereview.chromium.org/2209443006/diff/40001/ash/common/system/web_n...
ash/common/system/web_notification/web_notification_tray.cc:72: constexpr
gfx::Size kTrayItemInnerBellIconSize = gfx::Size(18, 18);
I think you can do

kTrayItemInnerIconSize(16, 16)

please ignore if it does not work.

https://codereview.chromium.org/2209443006/diff/40001/ash/common/system/web_n...
ash/common/system/web_notification/web_notification_tray.cc:75: constexpr int
kTrayItemHeight = 26;
can this also be gfx::Size ?

https://codereview.chromium.org/2209443006/diff/40001/ash/common/system/web_n...
ash/common/system/web_notification/web_notification_tray.cc:83: }
gfx::ToRoundedInt

https://codereview.chromium.org/2209443006/diff/40001/ash/common/system/web_n...
ash/common/system/web_notification/web_notification_tray.cc:186: if
(IsHorizontalAlignment(tray_->shelf_alignment())) {
since it's used in many places now, it's probably better to just add
IsHorizontal(Alignment/Lyaout) or something like that.

https://codereview.chromium.org/2209443006/diff/40001/ash/common/system/web_n...
ash/common/system/web_notification/web_notification_tray.cc:194: }
I guess we can't just clip it because there are multiple of these?

Can you add comment why we need to update the preferred size?

https://codereview.chromium.org/2209443006/diff/40001/ash/common/system/web_n...
ash/common/system/web_notification/web_notification_tray.cc:201: // Overridden
from gfx::AnimationDelegate.
// gfx::AnimationDelegate:

https://codereview.chromium.org/2209443006/diff/40001/ash/common/system/web_n...
ash/common/system/web_notification/web_notification_tray.cc:568:
std::set<std::string> notification_ids;
unorderd_set or you can just use vector (which is probably faster), whichever
you like.

https://codereview.chromium.org/2209443006/diff/40001/ash/common/system/web_n...
File ash/common/system/web_notification/web_notification_tray.h (right):

https://codereview.chromium.org/2209443006/diff/40001/ash/common/system/web_n...
ash/common/system/web_notification/web_notification_tray.h:183:
std::map<std::string, WebNotificationImage*> visible_small_icons_;
is order important? if not, unordered_map

[yoshiki, 2016-08-05 06:20:25]

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

[commit-bot: I haz the power, 2016-08-05 06:20:49]

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, 2016-08-05 06:30:55]

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

[commit-bot: I haz the power, 2016-08-05 06:30:55]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yoshiki, 2016-08-05 07:36:47]

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

[commit-bot: I haz the power, 2016-08-05 07:37:02]

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

[yoshiki, 2016-08-05 07:41:11]

Patchset #2 (id:60001) has been deleted

[yoshiki, 2016-08-05 07:48:29]

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

[commit-bot: I haz the power, 2016-08-05 07:48:39]

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

[yoshiki, 2016-08-05 07:48:51]

Patchset #2 (id:80001) has been deleted

[commit-bot: I haz the power, 2016-08-05 08:10:10]

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

[commit-bot: I haz the power, 2016-08-05 08:10:11]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[yoshiki, 2016-08-05 09:12:00]

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

[commit-bot: I haz the power, 2016-08-05 09:12:21]

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, 2016-08-05 09:27:58]

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

[commit-bot: I haz the power, 2016-08-05 09:27:58]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[yoshiki, 2016-08-05 10:52:02]

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

[commit-bot: I haz the power, 2016-08-05 10:52:12]

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, 2016-08-05 11:22:20]

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

[commit-bot: I haz the power, 2016-08-05 11:22:21]

Dry run: 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_...)

[yoshiki, 2016-08-05 12:24:37]

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

[commit-bot: I haz the power, 2016-08-05 12:24:59]

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

[yoshiki, 2016-08-05 12:35:11]

Patchset #2 (id:100001) has been deleted

[yoshiki, 2016-08-05 12:35:32]

Patchset #2 (id:120001) has been deleted

[yoshiki, 2016-08-05 12:35:46]

Patchset #2 (id:140001) has been deleted

[yoshiki, 2016-08-05 13:30:48]

Oshima, Xiyuan, PTAL.

https://codereview.chromium.org/2209443006/diff/40001/ash/common/system/web_n...
File ash/common/system/web_notification/web_notification_tray.cc (right):

https://codereview.chromium.org/2209443006/diff/40001/ash/common/system/web_n...
ash/common/system/web_notification/web_notification_tray.cc:72: constexpr
gfx::Size kTrayItemInnerBellIconSize = gfx::Size(18, 18);
On 2016/08/04 18:15:21, oshima wrote:
> I think you can do
> 
> kTrayItemInnerIconSize(16, 16)
> 
> please ignore if it does not work.

Done.

https://codereview.chromium.org/2209443006/diff/40001/ash/common/system/web_n...
ash/common/system/web_notification/web_notification_tray.cc:75: constexpr int
kTrayItemHeight = 26;
On 2016/08/04 18:15:21, oshima wrote:
> can this also be gfx::Size ?

Done.

https://codereview.chromium.org/2209443006/diff/40001/ash/common/system/web_n...
ash/common/system/web_notification/web_notification_tray.cc:83: }
On 2016/08/04 18:15:20, oshima wrote:
> gfx::ToRoundedInt

Done.

https://codereview.chromium.org/2209443006/diff/40001/ash/common/system/web_n...
ash/common/system/web_notification/web_notification_tray.cc:186: if
(IsHorizontalAlignment(tray_->shelf_alignment())) {
On 2016/08/04 18:15:21, oshima wrote:
> since it's used in many places now, it's probably better to just add
> IsHorizontal(Alignment/Lyaout) or something like that.

Done.

https://codereview.chromium.org/2209443006/diff/40001/ash/common/system/web_n...
ash/common/system/web_notification/web_notification_tray.cc:194: }
On 2016/08/04 18:15:21, oshima wrote:
> I guess we can't just clip it because there are multiple of these?
> 
> Can you add comment why we need to update the preferred size?

Done.

https://codereview.chromium.org/2209443006/diff/40001/ash/common/system/web_n...
ash/common/system/web_notification/web_notification_tray.cc:201: // Overridden
from gfx::AnimationDelegate.
On 2016/08/04 18:15:21, oshima wrote:
> // gfx::AnimationDelegate:

Done.

https://codereview.chromium.org/2209443006/diff/40001/ash/common/system/web_n...
ash/common/system/web_notification/web_notification_tray.cc:568:
std::set<std::string> notification_ids;
On 2016/08/04 18:15:21, oshima wrote:
> unorderd_set or you can just use vector (which is probably faster), whichever
> you like.

Let me use unordered_map, since I want to show that duplication is not permitted
and this is not "hot-code".

https://codereview.chromium.org/2209443006/diff/40001/ash/common/system/web_n...
File ash/common/system/web_notification/web_notification_tray.h (right):

https://codereview.chromium.org/2209443006/diff/40001/ash/common/system/web_n...
ash/common/system/web_notification/web_notification_tray.h:183:
std::map<std::string, WebNotificationImage*> visible_small_icons_;
On 2016/08/04 18:15:21, oshima wrote:
> is order important? if not, unordered_map
Let me use unordered_map.

[commit-bot: I haz the power, 2016-08-05 13:41:04]

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

[commit-bot: I haz the power, 2016-08-05 13:41:05]

Dry run: 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_...)

[oshima, 2016-08-05 15:08:08]

https://codereview.chromium.org/2209443006/diff/160001/ash/common/system/web_...
File ash/common/system/web_notification/web_notification_tray.cc (right):

https://codereview.chromium.org/2209443006/diff/160001/ash/common/system/web_...
ash/common/system/web_notification/web_notification_tray.cc:183: // Note that
other tray items do the same effect.
This does not explain why you want to change the size instead of using clipping.
I assume this is necessary to shift the buttons themselves?

https://codereview.chromium.org/2209443006/diff/160001/ash/common/system/web_...
ash/common/system/web_notification/web_notification_tray.cc:209:
static_cast<double>(height()) / 2, 0.));
height() / 2.f

https://codereview.chromium.org/2209443006/diff/160001/ash/common/system/web_...
ash/common/system/web_notification/web_notification_tray.cc:212:
animation->CurrentValueBetween(static_cast<double>(width() / 2), 0.),
ditto

https://codereview.chromium.org/2209443006/diff/160001/ash/common/system/web_...
ash/common/system/web_notification/web_notification_tray.cc:270:
notification_count = 99;  // cap with 99.
std::min ?

https://codereview.chromium.org/2209443006/diff/160001/ash/common/system/web_...
ash/common/system/web_notification/web_notification_tray.cc:274: str =
base::ASCIIToUTF16("+") + str;
Does this work in RTL?

https://codereview.chromium.org/2209443006/diff/160001/ash/common/system/web_...
ash/common/system/web_notification/web_notification_tray.cc:344:
disable_animations_for_test = true;
Once one test set this, this will be used for all tests in the unit tests. Is it
intentional?

https://codereview.chromium.org/2209443006/diff/160001/ash/common/system/web_...
ash/common/system/web_notification/web_notification_tray.cc:580: }
nit: nuke {}

[yoshiki, 2016-08-05 16:06:02]

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

[commit-bot: I haz the power, 2016-08-05 16:06:20]

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

[yoshiki, 2016-08-05 16:09:32]

Patchset #3 (id:180001) has been deleted

[commit-bot: I haz the power, 2016-08-05 16:10:14]

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...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[yoshiki, 2016-08-05 16:13:37]

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

[commit-bot: I haz the power, 2016-08-05 16:13:52]

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

[yoshiki, 2016-08-05 16:15:38]

Oshima-san, PTAL.

https://codereview.chromium.org/2209443006/diff/160001/ash/common/system/web_...
File ash/common/system/web_notification/web_notification_tray.cc (right):

https://codereview.chromium.org/2209443006/diff/160001/ash/common/system/web_...
ash/common/system/web_notification/web_notification_tray.cc:183: // Note that
other tray items do the same effect.
On 2016/08/05 15:08:08, oshima wrote:
> This does not explain why you want to change the size instead of using
clipping.
> I assume this is necessary to shift the buttons themselves?

Yes, I'd like to shift the left buttons along with the animation.

https://codereview.chromium.org/2209443006/diff/160001/ash/common/system/web_...
ash/common/system/web_notification/web_notification_tray.cc:209:
static_cast<double>(height()) / 2, 0.));
On 2016/08/05 15:08:08, oshima wrote:
> height() / 2.f

Let me use "2." for double since it should be double.

https://codereview.chromium.org/2209443006/diff/160001/ash/common/system/web_...
ash/common/system/web_notification/web_notification_tray.cc:212:
animation->CurrentValueBetween(static_cast<double>(width() / 2), 0.),
On 2016/08/05 15:08:08, oshima wrote:
> ditto

Done.

https://codereview.chromium.org/2209443006/diff/160001/ash/common/system/web_...
ash/common/system/web_notification/web_notification_tray.cc:270:
notification_count = 99;  // cap with 99.
On 2016/08/05 15:08:08, oshima wrote:
> std::min ?

Done.

https://codereview.chromium.org/2209443006/diff/160001/ash/common/system/web_...
ash/common/system/web_notification/web_notification_tray.cc:274: str =
base::ASCIIToUTF16("+") + str;
On 2016/08/05 15:08:08, oshima wrote:
> Does this work in RTL?

Done.

https://codereview.chromium.org/2209443006/diff/160001/ash/common/system/web_...
ash/common/system/web_notification/web_notification_tray.cc:344:
disable_animations_for_test = true;
On 2016/08/05 15:08:08, oshima wrote:
> Once one test set this, this will be used for all tests in the unit tests. Is
it
> intentional?

Fixed. TrayItemView does same thing so I thought it's ok.

https://codereview.chromium.org/2209443006/diff/160001/ash/common/system/web_...
ash/common/system/web_notification/web_notification_tray.cc:580: }
On 2016/08/05 15:08:08, oshima wrote:
> nit: nuke {}

Done.

[xiyuan, 2016-08-05 16:18:59]

https://codereview.chromium.org/2209443006/diff/160001/ash/common/system/tray...
File ash/common/system/tray/tray_background_view.cc (right):

https://codereview.chromium.org/2209443006/diff/160001/ash/common/system/tray...
ash/common/system/tray/tray_background_view.cc:190: return
views::View::GetInsets() + insets_;
Can we use |insets_| and create a proper empty border in
TrayContainer::UpdateLayout instead?

[commit-bot: I haz the power, 2016-08-05 17:36:34]

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

[commit-bot: I haz the power, 2016-08-05 17:36:35]

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

[yoshiki, 2016-08-09 07:37:15]

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

[commit-bot: I haz the power, 2016-08-09 07:37:47]

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

[yoshiki, 2016-08-09 08:03:07]

Patchset #4 (id:220001) has been deleted

[commit-bot: I haz the power, 2016-08-09 08:07:57]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yoshiki, 2016-08-09 09:14:32]

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

[commit-bot: I haz the power, 2016-08-09 09:15:13]

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, 2016-08-09 10:39:59]

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

[commit-bot: I haz the power, 2016-08-09 10:40:00]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[yoshiki, 2016-08-09 16:24:47]

Description was changed from

==========
Show small notification icons in notification tray

This patch shows small notification icons (up to 3) of the
current notifications in the notification tray.

This also makes the counter show the count of all the
notification excluding the visible small icons (previously
it was the unread count).

See the issue for screenshots.

BUG=633399
TEST=manual test
==========

to

==========
Show small notification icons in notification tray

This patch shows small notification icons (up to 3) of the
current notifications in the notification tray.

This also makes the counter show the count of all the
notification excluding the visible small icons (previously
it was the unread count).

See the issue for screenshots.

BUG=633399
TEST=manual test (with and without material mode)
==========

[yoshiki, 2016-08-09 16:24:55]

Description was changed from

==========
Show small notification icons in notification tray

This patch shows small notification icons (up to 3) of the
current notifications in the notification tray.

This also makes the counter show the count of all the
notification excluding the visible small icons (previously
it was the unread count).

See the issue for screenshots.

BUG=633399
TEST=manual test (with and without material mode)
==========

to

==========
Show small notification icons in notification tray

This patch shows small notification icons (up to 3) of the
current notifications in the notification tray.

This also makes the counter show the count of all the
notification excluding the visible small icons (previously
it was the unread count).

See the issue for screenshots.

BUG=633399
TEST=manual test (with and without MD mode)
==========

[yoshiki, 2016-08-09 16:43:05]

Xiyuan, Oshima, PTAL

https://codereview.chromium.org/2209443006/diff/160001/ash/common/system/tray...
File ash/common/system/tray/tray_background_view.cc (right):

https://codereview.chromium.org/2209443006/diff/160001/ash/common/system/tray...
ash/common/system/tray/tray_background_view.cc:190: return
views::View::GetInsets() + insets_;
On 2016/08/05 16:18:59, xiyuan wrote:
> Can we use |insets_| and create a proper empty border in
> TrayContainer::UpdateLayout instead?

Done.

[xiyuan, 2016-08-09 16:50:05]

lgtm

https://codereview.chromium.org/2209443006/diff/240001/ash/common/system/web_...
File ash/common/system/web_notification/web_notification_tray.cc (right):

https://codereview.chromium.org/2209443006/diff/240001/ash/common/system/web_...
ash/common/system/web_notification/web_notification_tray.cc:283: }
We should add a string template to resource

+<ph name="number><ex>1</ex></ph>

and use one of l10n_util::GetStringFUTFxx to build the string.

But this would change strings, so I am okay to leave as-is but we should have a
TODO and fix it in M54.

[oshima, 2016-08-10 06:58:44]

lgtm with nits

https://codereview.chromium.org/2209443006/diff/240001/ash/common/system/web_...
File ash/common/system/web_notification/web_notification_tray.cc (right):

https://codereview.chromium.org/2209443006/diff/240001/ash/common/system/web_...
ash/common/system/web_notification/web_notification_tray.cc:81: static bool
disable_animations_for_test = false;
nit: you don't need static. anonymous namespace is internal linkage in c++11.

https://codereview.chromium.org/2209443006/diff/240001/ash/common/system/web_...
ash/common/system/web_notification/web_notification_tray.cc:587: for (auto i :
visible_small_icons_)
nit: auto pair

[yoshiki, 2016-08-10 09:09:39]

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

[commit-bot: I haz the power, 2016-08-10 09:10:03]

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, 2016-08-10 09:52:05]

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

[commit-bot: I haz the power, 2016-08-10 09:52:06]

Dry run: This issue passed the CQ dry run.

[yoshiki, 2016-08-10 10:18:53]

https://codereview.chromium.org/2209443006/diff/240001/ash/common/system/web_...
File ash/common/system/web_notification/web_notification_tray.cc (right):

https://codereview.chromium.org/2209443006/diff/240001/ash/common/system/web_...
ash/common/system/web_notification/web_notification_tray.cc:81: static bool
disable_animations_for_test = false;
On 2016/08/10 06:58:44, oshima wrote:
> nit: you don't need static. anonymous namespace is internal linkage in c++11.

Done.

https://codereview.chromium.org/2209443006/diff/240001/ash/common/system/web_...
ash/common/system/web_notification/web_notification_tray.cc:283: }
On 2016/08/09 16:50:05, xiyuan wrote:
> We should add a string template to resource
> 
> +<ph name="number><ex>1</ex></ph>
> 
> and use one of l10n_util::GetStringFUTFxx to build the string.
> 
> But this would change strings, so I am okay to leave as-is but we should have
a
> TODO and fix it in M54.

Got it. I'll do it in a separated patch.

https://codereview.chromium.org/2209443006/diff/240001/ash/common/system/web_...
ash/common/system/web_notification/web_notification_tray.cc:587: for (auto i :
visible_small_icons_)
On 2016/08/10 06:58:44, oshima wrote:
> nit: auto pair

Done.

[yoshiki, 2016-08-10 10:19:11]

The CQ bit was checked by yoshiki@chromium.org

[yoshiki, 2016-08-10 10:19:12]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/2209443006/#ps260001
(title: "Rebased and addressed comments")

[commit-bot: I haz the power, 2016-08-10 10:19:29]

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, 2016-08-10 10:24:21]

Description was changed from

==========
Show small notification icons in notification tray

This patch shows small notification icons (up to 3) of the
current notifications in the notification tray.

This also makes the counter show the count of all the
notification excluding the visible small icons (previously
it was the unread count).

See the issue for screenshots.

BUG=633399
TEST=manual test (with and without MD mode)
==========

to

==========
Show small notification icons in notification tray

This patch shows small notification icons (up to 3) of the
current notifications in the notification tray.

This also makes the counter show the count of all the
notification excluding the visible small icons (previously
it was the unread count).

See the issue for screenshots.

BUG=633399
TEST=manual test (with and without MD mode)
==========

[commit-bot: I haz the power, 2016-08-10 10:24:23]

Committed patchset #5 (id:260001)

[commit-bot: I haz the power, 2016-08-10 10:26:24]

Description was changed from

==========
Show small notification icons in notification tray

This patch shows small notification icons (up to 3) of the
current notifications in the notification tray.

This also makes the counter show the count of all the
notification excluding the visible small icons (previously
it was the unread count).

See the issue for screenshots.

BUG=633399
TEST=manual test (with and without MD mode)
==========

to

==========
Show small notification icons in notification tray

This patch shows small notification icons (up to 3) of the
current notifications in the notification tray.

This also makes the counter show the count of all the
notification excluding the visible small icons (previously
it was the unread count).

See the issue for screenshots.

BUG=633399
TEST=manual test (with and without MD mode)

Committed: https://crrev.com/42754bb876e40c0c87ed63607452026190e13f78
Cr-Commit-Position: refs/heads/master@{#411019}
==========

[commit-bot: I haz the power, 2016-08-10 10:26:26]

Patchset 5 (id:??) landed as
https://crrev.com/42754bb876e40c0c87ed63607452026190e13f78
Cr-Commit-Position: refs/heads/master@{#411019}

[huangs, 2016-08-10 13:44:33]

I'm suspecting this CL of causing ASAN failure due to memory leak:

https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2...

Going to revert to see if it fixes things.

[huangs, 2016-08-10 13:45:23]

huangs@chromium.org changed reviewers:
+ huangs@chromium.org

[huangs, 2016-08-10 13:45:25]

https://codereview.chromium.org/2209443006/diff/260001/ash/common/system/web_...
File ash/common/system/web_notification/web_notification_tray.cc (right):

https://codereview.chromium.org/2209443006/diff/260001/ash/common/system/web_...
ash/common/system/web_notification/web_notification_tray.cc:249: view_ = new
views::ImageView();
Possible memory leak?

https://codereview.chromium.org/2209443006/diff/260001/ash/common/system/web_...
ash/common/system/web_notification/web_notification_tray.cc:266: view_ = new
views::Label();
Possible memory leak?

[huangs, 2016-08-10 13:46:24]

A revert of this CL (patchset #5 id:260001) has been created in
https://codereview.chromium.org/2231003002/ by huangs@chromium.org.

The reason for reverting is: Suspecting this CL of causing ASAN failure, from
memory leak:
https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2....

[xiyuan, 2016-08-10 15:21:25]

https://codereview.chromium.org/2209443006/diff/260001/ash/common/system/web_...
File ash/common/system/web_notification/web_notification_tray.cc (right):

https://codereview.chromium.org/2209443006/diff/260001/ash/common/system/web_...
ash/common/system/web_notification/web_notification_tray.cc:266: view_ = new
views::Label();
On 2016/08/10 13:45:25, huangs wrote:
> Possible memory leak?

This one is the reported leak, happens when SetNotificationCount is not called
to add it to the view hierarchy.