Update first download notification correctly if there are multiple notifications

Update first download notification correctly if there are multiple notifications

This patch fixes the issue on updating the first download notification by fixing the visibility check in DownloadItemNotification. This issue was the regression of https://crrev.com/49b2b0338e0eb698a24b045bf09603b853d7c4a7.

BUG=541633
TEST=trybot passes

Committed: https://crrev.com/795243b38d321e1e9d1f32d814265461194d74b0
Cr-Commit-Position: refs/heads/master@{#354177}

[yoshiki, 2015-10-09 19:43:37]

Patchset #1 (id:1) has been deleted

[yoshiki, 2015-10-09 19:43:43]

Patchset #1 (id:20001) has been deleted

[yoshiki, 2015-10-09 19:43:50]

Patchset #1 (id:40001) has been deleted

[yoshiki, 2015-10-09 19:43:54]

Patchset #1 (id:60001) has been deleted

[yoshiki, 2015-10-09 19:44:00]

Patchset #1 (id:80001) has been deleted

[yoshiki, 2015-10-10 05:22:59]

Patchset #1 (id:100001) has been deleted

[yoshiki, 2015-10-10 05:31:13]

Asanka, PTAL. Thanks.

https://codereview.chromium.org/1389273004/diff/120001/chrome/browser/downloa...
File chrome/browser/download/notification/download_item_notification.cc (right):

https://codereview.chromium.org/1389273004/diff/120001/chrome/browser/downloa...
chrome/browser/download/notification/download_item_notification.cc:334: if
(IsNotificationVisible()) {
The cause of the issue is that |visible_| is not updated correctly when the
notification is closed by DisablePopup(). So this patch removes this variable
and makes it use IsNotificationVisible() instead.

https://codereview.chromium.org/1389273004/diff/120001/chrome/browser/downloa...
chrome/browser/download/notification/download_item_notification.cc:905:
notification_ui_manager()->FindById(notification_id, profile_id);
Instead of generating notification-id-in-message-center, this retrieves it from
the actual notification. These ID must be same.

The reason why I changed is that StubNotificationUIManager generates the ID in
different way in test.

[yoshiki, 2015-10-10 05:31:26]

yoshiki@chromium.org changed reviewers:
+ asanka@chromium.org

[asanka, 2015-10-12 18:59:09]

https://codereview.chromium.org/1389273004/diff/120001/chrome/browser/downloa...
File chrome/browser/download/notification/download_notification_browsertest.cc
(right):

https://codereview.chromium.org/1389273004/diff/120001/chrome/browser/downloa...
chrome/browser/download/notification/download_notification_browsertest.cc:378:
NotificationUpdateObserver download_notification_periodically_update_observer;
Does this rely on the DownloadItem firing UpdateObservers on its own? The fact
that an in-progress download sends OnDownloadUpdated() periodically even when no
data is transferred is undocumented behavior and is something we'd like to get
rid of.

If that's what you are relying on, a better alternative is to manually trigger
an update (via DownloadItem::UpdateObservers()) and then seeing if that results
in the download notification being updated. That also means that the below
Wait() call doesn't stall for however long it takes for the next timer initiated
update to fire.

[yoshiki, 2015-10-14 06:53:38]

https://codereview.chromium.org/1389273004/diff/120001/chrome/browser/downloa...
File chrome/browser/download/notification/download_notification_browsertest.cc
(right):

https://codereview.chromium.org/1389273004/diff/120001/chrome/browser/downloa...
chrome/browser/download/notification/download_notification_browsertest.cc:378:
NotificationUpdateObserver download_notification_periodically_update_observer;
On 2015/10/12 18:59:08, asanka wrote:
> Does this rely on the DownloadItem firing UpdateObservers on its own? The fact
> that an in-progress download sends OnDownloadUpdated() periodically even when
no
> data is transferred is undocumented behavior and is something we'd like to get
> rid of.
> 
> If that's what you are relying on, a better alternative is to manually trigger
> an update (via DownloadItem::UpdateObservers()) and then seeing if that
results
> in the download notification being updated. That also means that the below
> Wait() call doesn't stall for however long it takes for the next timer
initiated
> update to fire.

Thank you for suggestion. Thanks. PTAL

[asanka, 2015-10-14 22:46:39]

lgtm

[yoshiki, 2015-10-15 00:21:21]

The CQ bit was checked by yoshiki@chromium.org

[commit-bot: I haz the power, 2015-10-15 00:23:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1389273004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1389273004/140001

[commit-bot: I haz the power, 2015-10-15 00:51:43]

Committed patchset #2 (id:140001)

[commit-bot: I haz the power, 2015-10-15 00:53:16]

Patchset 2 (id:??) landed as
https://crrev.com/795243b38d321e1e9d1f32d814265461194d74b0
Cr-Commit-Position: refs/heads/master@{#354177}

[benwells, 2015-10-15 06:04:07]

A revert of this CL (patchset #2 id:140001) has been created in
https://codereview.chromium.org/1403003004/ by benwells@chromium.org.

The reason for reverting is: This has caused memory errors on the ChromeOS
memory bots. See
http://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20...
for an example build.

Sample failure output:
==2253==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x7f0398815ca4 in operator==\u003Cstd::__1::allocator\u003Cchar> >
buildtools/third_party/libc++/trunk/include/string:3812:9
    #1 0x7f0398815ca4 in
message_center::NotificationList::GetNotification(std::__1::basic_string\u003Cchar,
std::__1::char_traits\u003Cchar>, std::__1::allocator\u003Cchar> > const&)
ui/message_center/notification_list.cc:319:0
    #2 0x7f0398815d85 in
message_center::NotificationList::RemoveNotification(std::__1::basic_string\u003Cchar,
std::__1::char_traits\u003Cchar>, std::__1::allocator\u003Cchar> > const&)
ui/message_center/notification_list.cc:123:21
    #3 0x7f03987f8691 in
message_center::MessageCenterImpl::RemoveNotifications(bool,
std::__1::vector\u003Cmessage_center::NotificationBlocker*,
std::__1::allocator\u003Cmessage_center::NotificationBlocker*> > const&)
ui/message_center/message_center_impl.cc:664:5
    #4 0x7f03987f8115 in
message_center::MessageCenterImpl::RemoveAllNotifications(bool)
ui/message_center/message_center_impl.cc:646:3
    #5 0x7f038daba090 in content::NotificationServiceImpl::Notify(int,
content::NotificationSource const&, content::NotificationDetails const&)
content/browser/notification_service_impl.cc:130:5
    #6 0x7f0386830ee7 in chrome::NotifyAppTerminating()
chrome/browser/lifetime/application_lifetime.cc:318:3
    #7 0x7f038ac73bc2 in BrowserList::RemoveBrowser(Browser*)
chrome/browser/ui/browser_list.cc:105:5
.....

  Uninitialized value was created by a heap deallocation
    #0 0x7f0382c9edd2 in __interceptor_free ??:0:0
    #1 0x7f038c16d632 in hb_buffer_destroy
third_party/harfbuzz-ng/src/hb-buffer.cc:785:3
    #2 0x7f038c127512 in
gfx::RenderTextHarfBuzz::ShapeRunWithFont(std::__1::basic_string\u003Cunsigned
short, base::string16_char_traits, std::__1::allocator\u003Cunsigned short> >
const&, std::__1::basic_string\u003Cchar, std::__1::char_traits\u003Cchar>,
std::__1::allocator\u003Cchar> > const&, gfx::FontRenderParams const&,
gfx::internal::TextRunHarfBuzz*) ui/gfx/render_text_harfbuzz.cc:1511:3
    #3 0x7f038c128417 in CompareFamily ui/gfx/render_text_harfbuzz.cc:1311:8
    #4 0x7f038c128417 in
gfx::RenderTextHarfBuzz::ShapeRun(std::__1::basic_string\u003Cunsigned short,
base::string16_char_traits, std::__1::allocator\u003Cunsigned short> > const&,
gfx::internal::TextRunHarfBuzz*) ui/gfx/render_text_harfbuzz.cc:1366:0
    #5 0x7f038c113e9b in ShapeRunList ui/gfx/render_text_harfbuzz.cc:1326:5
    #6 0x7f038c113e9b in gfx::RenderTextHarfBuzz::EnsureLayoutRunList()
ui/gfx/render_text_harfbuzz.cc:1529:0
    #7 0x7f038c11b493 in gfx::RenderTextHarfBuzz::EnsureLayout()
ui/gfx/render_text_harfbuzz.cc:1058:3
    #8 0x7f038c11505f in gfx::RenderTextHarfBuzz::GetStringSizeF()
ui/gfx/render_text_harfbuzz.cc:789:3
    #9 0x7f038c114ee1 in gfx::RenderTextHarfBuzz::GetStringSize()
ui/gfx/render_text_harfbuzz.cc:784:24
    #10 0x7f038a3c2e67 in views::Label::GetTextSize() const
ui/views/controls/label.cc:522:12
    #11 0x7f038a3c2674 in views::Label::GetPreferredSize() const
ui/views/controls/label.cc:228:18
    #12 0x7f038a449484 in MainAxisSizeForView
ui/views/layout/box_layout.cc:251:16
    #13 0x7f038a449484 in views::BoxLayout::Layout(views::View*)
ui/views/layout/box_layout.cc:65:0
    #14 0x7f038a4734ff in views::View::Layout() ui/views/view.cc:505:5
    #15 0x7f039884b041 in
message_center::NotificationView::CreateOrUpdateActionButtonViews(message_center::Notification
const&) ui/message_center/views/notification_view.cc:748:7
......

[benwells, 2015-10-15 06:10:59]

On 2015/10/15 06:04:07, benwells wrote:
> A revert of this CL (patchset #2 id:140001) has been created in
> https://codereview.chromium.org/1403003004/ by mailto:benwells@chromium.org.
> 
> The reason for reverting is: This has caused memory errors on the ChromeOS
> memory bots. See
>
http://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20...
> for an example build.
> 
> Sample failure output:
> ==2253==WARNING: MemorySanitizer: use-of-uninitialized-value
>     #0 0x7f0398815ca4 in operator==\u003Cstd::__1::allocator\u003Cchar> >
> buildtools/third_party/libc++/trunk/include/string:3812:9
>     #1 0x7f0398815ca4 in
>
message_center::NotificationList::GetNotification(std::__1::basic_string\u003Cchar,
> std::__1::char_traits\u003Cchar>, std::__1::allocator\u003Cchar> > const&)
> ui/message_center/notification_list.cc:319:0
>     #2 0x7f0398815d85 in
>
message_center::NotificationList::RemoveNotification(std::__1::basic_string\u003Cchar,
> std::__1::char_traits\u003Cchar>, std::__1::allocator\u003Cchar> > const&)
> ui/message_center/notification_list.cc:123:21
>     #3 0x7f03987f8691 in
> message_center::MessageCenterImpl::RemoveNotifications(bool,
> std::__1::vector\u003Cmessage_center::NotificationBlocker*,
> std::__1::allocator\u003Cmessage_center::NotificationBlocker*> > const&)
> ui/message_center/message_center_impl.cc:664:5
>     #4 0x7f03987f8115 in
> message_center::MessageCenterImpl::RemoveAllNotifications(bool)
> ui/message_center/message_center_impl.cc:646:3
>     #5 0x7f038daba090 in content::NotificationServiceImpl::Notify(int,
> content::NotificationSource const&, content::NotificationDetails const&)
> content/browser/notification_service_impl.cc:130:5
>     #6 0x7f0386830ee7 in chrome::NotifyAppTerminating()
> chrome/browser/lifetime/application_lifetime.cc:318:3
>     #7 0x7f038ac73bc2 in BrowserList::RemoveBrowser(Browser*)
> chrome/browser/ui/browser_list.cc:105:5
> .....
> 
>   Uninitialized value was created by a heap deallocation
>     #0 0x7f0382c9edd2 in __interceptor_free ??:0:0
>     #1 0x7f038c16d632 in hb_buffer_destroy
> third_party/harfbuzz-ng/src/hb-buffer.cc:785:3
>     #2 0x7f038c127512 in
> gfx::RenderTextHarfBuzz::ShapeRunWithFont(std::__1::basic_string\u003Cunsigned
> short, base::string16_char_traits, std::__1::allocator\u003Cunsigned short> >
> const&, std::__1::basic_string\u003Cchar, std::__1::char_traits\u003Cchar>,
> std::__1::allocator\u003Cchar> > const&, gfx::FontRenderParams const&,
> gfx::internal::TextRunHarfBuzz*) ui/gfx/render_text_harfbuzz.cc:1511:3
>     #3 0x7f038c128417 in CompareFamily ui/gfx/render_text_harfbuzz.cc:1311:8
>     #4 0x7f038c128417 in
> gfx::RenderTextHarfBuzz::ShapeRun(std::__1::basic_string\u003Cunsigned short,
> base::string16_char_traits, std::__1::allocator\u003Cunsigned short> > const&,
> gfx::internal::TextRunHarfBuzz*) ui/gfx/render_text_harfbuzz.cc:1366:0
>     #5 0x7f038c113e9b in ShapeRunList ui/gfx/render_text_harfbuzz.cc:1326:5
>     #6 0x7f038c113e9b in gfx::RenderTextHarfBuzz::EnsureLayoutRunList()
> ui/gfx/render_text_harfbuzz.cc:1529:0
>     #7 0x7f038c11b493 in gfx::RenderTextHarfBuzz::EnsureLayout()
> ui/gfx/render_text_harfbuzz.cc:1058:3
>     #8 0x7f038c11505f in gfx::RenderTextHarfBuzz::GetStringSizeF()
> ui/gfx/render_text_harfbuzz.cc:789:3
>     #9 0x7f038c114ee1 in gfx::RenderTextHarfBuzz::GetStringSize()
> ui/gfx/render_text_harfbuzz.cc:784:24
>     #10 0x7f038a3c2e67 in views::Label::GetTextSize() const
> ui/views/controls/label.cc:522:12
>     #11 0x7f038a3c2674 in views::Label::GetPreferredSize() const
> ui/views/controls/label.cc:228:18
>     #12 0x7f038a449484 in MainAxisSizeForView
> ui/views/layout/box_layout.cc:251:16
>     #13 0x7f038a449484 in views::BoxLayout::Layout(views::View*)
> ui/views/layout/box_layout.cc:65:0
>     #14 0x7f038a4734ff in views::View::Layout() ui/views/view.cc:505:5
>     #15 0x7f039884b041 in
>
message_center::NotificationView::CreateOrUpdateActionButtonViews(message_center::Notification
> const&) ui/message_center/views/notification_view.cc:748:7
> ......

Just saw yoshiki has fixed the tests. Awesome!