New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 755891 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Sep 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

Pinned status is not updated on popup

Project Member Reported by yoshiki@chromium.org, Aug 16 2017

Issue description

Popup notification must show the close button regardless of the status of "pinned" flag. But it is not reflected sometimes.
 
Labels: -Pri-3 M-61 OS-Chrome Pri-1
Owner: yoshiki@chromium.org
Status: Started (was: Untriaged)
This is because the "pinned" status is overridden only on creation.
Project Member

Comment 2 by bugdroid1@chromium.org, Aug 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a701b1b2c9b63981ad0a56bc793b9175cb33ba1f

commit a701b1b2c9b63981ad0a56bc793b9175cb33ba1f
Author: yoshiki iguchi <yoshiki@chromium.org>
Date: Thu Aug 17 13:06:11 2017

Create the flag to override the pinned state in MessageView

We should ignore the pinned state on popup notification. This CL adds a flag to override the state.

This CL also removes the duplicated "pinned" flag in ArcNotificationItem, since we already have the flag in MessageView. 

This change affects only for Chrome OS, since the pinned state is efficient only on Chrome OS.

Bug:  755891 
Change-Id: I51c626a29f4e8f8bf9a292d0c63b8afa86b95907
Reviewed-on: https://chromium-review.googlesource.com/609393
Commit-Queue: Yoshiki Iguchi <yoshiki@chromium.org>
Reviewed-by: Yuichiro Hanada <yhanada@chromium.org>
Cr-Commit-Position: refs/heads/master@{#495154}
[modify] https://crrev.com/a701b1b2c9b63981ad0a56bc793b9175cb33ba1f/ui/arc/notification/arc_notification_content_view.cc
[modify] https://crrev.com/a701b1b2c9b63981ad0a56bc793b9175cb33ba1f/ui/arc/notification/arc_notification_content_view.h
[modify] https://crrev.com/a701b1b2c9b63981ad0a56bc793b9175cb33ba1f/ui/arc/notification/arc_notification_content_view_unittest.cc
[modify] https://crrev.com/a701b1b2c9b63981ad0a56bc793b9175cb33ba1f/ui/arc/notification/arc_notification_item.h
[modify] https://crrev.com/a701b1b2c9b63981ad0a56bc793b9175cb33ba1f/ui/arc/notification/arc_notification_item_impl.cc
[modify] https://crrev.com/a701b1b2c9b63981ad0a56bc793b9175cb33ba1f/ui/arc/notification/arc_notification_item_impl.h
[modify] https://crrev.com/a701b1b2c9b63981ad0a56bc793b9175cb33ba1f/ui/arc/notification/arc_notification_view.cc
[modify] https://crrev.com/a701b1b2c9b63981ad0a56bc793b9175cb33ba1f/ui/arc/notification/arc_notification_view.h
[modify] https://crrev.com/a701b1b2c9b63981ad0a56bc793b9175cb33ba1f/ui/message_center/views/message_center_view.cc
[modify] https://crrev.com/a701b1b2c9b63981ad0a56bc793b9175cb33ba1f/ui/message_center/views/message_list_view.cc
[modify] https://crrev.com/a701b1b2c9b63981ad0a56bc793b9175cb33ba1f/ui/message_center/views/message_popup_collection.cc
[modify] https://crrev.com/a701b1b2c9b63981ad0a56bc793b9175cb33ba1f/ui/message_center/views/message_view.cc
[modify] https://crrev.com/a701b1b2c9b63981ad0a56bc793b9175cb33ba1f/ui/message_center/views/message_view.h
[modify] https://crrev.com/a701b1b2c9b63981ad0a56bc793b9175cb33ba1f/ui/message_center/views/notification_view.cc
[modify] https://crrev.com/a701b1b2c9b63981ad0a56bc793b9175cb33ba1f/ui/message_center/views/notification_view.h
[modify] https://crrev.com/a701b1b2c9b63981ad0a56bc793b9175cb33ba1f/ui/message_center/views/notification_view_md.cc
[modify] https://crrev.com/a701b1b2c9b63981ad0a56bc793b9175cb33ba1f/ui/message_center/views/notification_view_unittest.cc

Labels: Merge-Request-61

Comment 4 by ketakid@google.com, Aug 18 2017

Labels: -Merge-Request-61 Merge-Approved-61
Approving merge to M61 Chrome OS.
Project Member

Comment 5 by sheriffbot@chromium.org, Aug 21 2017

Cc: ketakid@google.com
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 6 by sheriffbot@chromium.org, Aug 24 2017

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 31 2017

Labels: -merge-approved-61 merge-merged-3163
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/033d4edf16c6a9191f1eea32de0417179c4731ba

commit 033d4edf16c6a9191f1eea32de0417179c4731ba
Author: yoshiki iguchi <yoshiki@chromium.org>
Date: Thu Aug 31 09:24:59 2017

Create the flag to override the pinned state in MessageView

We should ignore the pinned state on popup notification. This CL adds a flag to override the state.

This CL also removes the duplicated "pinned" flag in ArcNotificationItem, since we already have the flag in MessageView.

This change affects only for Chrome OS, since the pinned state is efficient only on Chrome OS.

TBR=yoshiki@chromium.org

(cherry picked from commit a701b1b2c9b63981ad0a56bc793b9175cb33ba1f)

Bug:  755891 
Change-Id: I51c626a29f4e8f8bf9a292d0c63b8afa86b95907
Reviewed-on: https://chromium-review.googlesource.com/609393
Commit-Queue: Yoshiki Iguchi <yoshiki@chromium.org>
Reviewed-by: Yuichiro Hanada <yhanada@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#495154}
Reviewed-on: https://chromium-review.googlesource.com/645409
Reviewed-by: Yoshiki Iguchi <yoshiki@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#1030}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/033d4edf16c6a9191f1eea32de0417179c4731ba/ui/arc/notification/arc_notification_content_view.cc
[modify] https://crrev.com/033d4edf16c6a9191f1eea32de0417179c4731ba/ui/arc/notification/arc_notification_content_view.h
[modify] https://crrev.com/033d4edf16c6a9191f1eea32de0417179c4731ba/ui/arc/notification/arc_notification_content_view_unittest.cc
[modify] https://crrev.com/033d4edf16c6a9191f1eea32de0417179c4731ba/ui/arc/notification/arc_notification_item.h
[modify] https://crrev.com/033d4edf16c6a9191f1eea32de0417179c4731ba/ui/arc/notification/arc_notification_item_impl.cc
[modify] https://crrev.com/033d4edf16c6a9191f1eea32de0417179c4731ba/ui/arc/notification/arc_notification_item_impl.h
[modify] https://crrev.com/033d4edf16c6a9191f1eea32de0417179c4731ba/ui/arc/notification/arc_notification_view.cc
[modify] https://crrev.com/033d4edf16c6a9191f1eea32de0417179c4731ba/ui/arc/notification/arc_notification_view.h
[modify] https://crrev.com/033d4edf16c6a9191f1eea32de0417179c4731ba/ui/message_center/views/message_center_view.cc
[modify] https://crrev.com/033d4edf16c6a9191f1eea32de0417179c4731ba/ui/message_center/views/message_list_view.cc
[modify] https://crrev.com/033d4edf16c6a9191f1eea32de0417179c4731ba/ui/message_center/views/message_popup_collection.cc
[modify] https://crrev.com/033d4edf16c6a9191f1eea32de0417179c4731ba/ui/message_center/views/message_view.cc
[modify] https://crrev.com/033d4edf16c6a9191f1eea32de0417179c4731ba/ui/message_center/views/message_view.h
[modify] https://crrev.com/033d4edf16c6a9191f1eea32de0417179c4731ba/ui/message_center/views/notification_view.cc
[modify] https://crrev.com/033d4edf16c6a9191f1eea32de0417179c4731ba/ui/message_center/views/notification_view.h
[modify] https://crrev.com/033d4edf16c6a9191f1eea32de0417179c4731ba/ui/message_center/views/notification_view_md.cc
[modify] https://crrev.com/033d4edf16c6a9191f1eea32de0417179c4731ba/ui/message_center/views/notification_view_unittest.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Aug 31 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/454e807bff7067ad3cc1d4c99e3280787d0518ba

commit 454e807bff7067ad3cc1d4c99e3280787d0518ba
Author: Mike Wittman <wittman@chromium.org>
Date: Thu Aug 31 17:54:16 2017

Revert "Create the flag to override the pinned state in MessageView"

This reverts commit 033d4edf16c6a9191f1eea32de0417179c4731ba.

Reason for revert: breaks compile

FAILED: obj/ui/message_center/message_center/notification_view_md.obj
ninja -t msvc -e environment.x64 --
"c:\b\c\win_toolchain\vs_files\f53e4598951162bad6330f7a167486c7ae5db1e5\vc\bin\amd64/cl.exe"
/nologo /showIncludes
@obj/ui/message_center/message_center/notification_view_md.obj.rsp /c
../../ui/message_center/views/notification_view_md.cc
/Foobj/ui/message_center/message_center/notification_view_md.obj
/Fd"obj/ui/message_center/message_center_cc.pdb"
../../ui/message_center/views/notification_view_md.cc(567): error C2039:
'UpdateControlButtonsVisibilityWithNotification': is not a member of
'message_center::NotificationViewMD'

Original change's description:
> Create the flag to override the pinned state in MessageView
>
> We should ignore the pinned state on popup notification. This CL adds
a flag to override the state.
>
> This CL also removes the duplicated "pinned" flag in
ArcNotificationItem, since we already have the flag in MessageView.
>
> This change affects only for Chrome OS, since the pinned state is
efficient only on Chrome OS.
>
> TBR=yoshiki@chromium.org
>
> (cherry picked from commit a701b1b2c9b63981ad0a56bc793b9175cb33ba1f)
>
> Bug:  755891 
> Change-Id: I51c626a29f4e8f8bf9a292d0c63b8afa86b95907
> Reviewed-on: https://chromium-review.googlesource.com/609393
> Commit-Queue: Yoshiki Iguchi <yoshiki@chromium.org>
> Reviewed-by: Yuichiro Hanada <yhanada@chromium.org>
> Cr-Original-Commit-Position: refs/heads/master@{#495154}
> Reviewed-on: https://chromium-review.googlesource.com/645409
> Reviewed-by: Yoshiki Iguchi <yoshiki@chromium.org>
> Cr-Commit-Position: refs/branch-heads/3163@{#1030}
> Cr-Branched-From:
ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}

TBR=yoshiki@chromium.org,yhanada@chromium.org

No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  755891 , 760909
Change-Id: Ieaa62f20754de4206b07db2cd38214164181e640
Reviewed-on: https://chromium-review.googlesource.com/646573
Reviewed-by: Mike Wittman <wittman@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#1035}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/454e807bff7067ad3cc1d4c99e3280787d0518ba/ui/arc/notification/arc_notification_content_view.cc
[modify] https://crrev.com/454e807bff7067ad3cc1d4c99e3280787d0518ba/ui/arc/notification/arc_notification_content_view.h
[modify] https://crrev.com/454e807bff7067ad3cc1d4c99e3280787d0518ba/ui/arc/notification/arc_notification_content_view_unittest.cc
[modify] https://crrev.com/454e807bff7067ad3cc1d4c99e3280787d0518ba/ui/arc/notification/arc_notification_item.h
[modify] https://crrev.com/454e807bff7067ad3cc1d4c99e3280787d0518ba/ui/arc/notification/arc_notification_item_impl.cc
[modify] https://crrev.com/454e807bff7067ad3cc1d4c99e3280787d0518ba/ui/arc/notification/arc_notification_item_impl.h
[modify] https://crrev.com/454e807bff7067ad3cc1d4c99e3280787d0518ba/ui/arc/notification/arc_notification_view.cc
[modify] https://crrev.com/454e807bff7067ad3cc1d4c99e3280787d0518ba/ui/arc/notification/arc_notification_view.h
[modify] https://crrev.com/454e807bff7067ad3cc1d4c99e3280787d0518ba/ui/message_center/views/message_center_view.cc
[modify] https://crrev.com/454e807bff7067ad3cc1d4c99e3280787d0518ba/ui/message_center/views/message_list_view.cc
[modify] https://crrev.com/454e807bff7067ad3cc1d4c99e3280787d0518ba/ui/message_center/views/message_popup_collection.cc
[modify] https://crrev.com/454e807bff7067ad3cc1d4c99e3280787d0518ba/ui/message_center/views/message_view.cc
[modify] https://crrev.com/454e807bff7067ad3cc1d4c99e3280787d0518ba/ui/message_center/views/message_view.h
[modify] https://crrev.com/454e807bff7067ad3cc1d4c99e3280787d0518ba/ui/message_center/views/notification_view.cc
[modify] https://crrev.com/454e807bff7067ad3cc1d4c99e3280787d0518ba/ui/message_center/views/notification_view.h
[modify] https://crrev.com/454e807bff7067ad3cc1d4c99e3280787d0518ba/ui/message_center/views/notification_view_md.cc
[modify] https://crrev.com/454e807bff7067ad3cc1d4c99e3280787d0518ba/ui/message_center/views/notification_view_unittest.cc

Labels: -M-61 -merge-merged-3163 M-62
Let me change the milestone since this is not worth to merge to M61 at this time.
Status: Fixed (was: Started)

Sign in to add a comment