Not Remove Non-Closable Arc Popup When Close Button is Pressed

Not Remove Non-Closable Arc Popup When Close Button is Pressed

* Fixes the issue: crbug.com/689220
  - Adds ArcNotificationDelegate::Close method to handle the closing event
  - Not calls ArcCustomNotificationItem::Close() when the close button is pressed
* Refines the code
  - Uses the same method MessageView::OnCloseButtonPressed() both from custom and normal notification

BUG=689220
Review-Url: https://codereview.chromium.org/2668583005
Cr-Commit-Position: refs/heads/master@{#454814}
Committed: https://chromium.googlesource.com/chromium/src/+/9739ee62037118f47be3dab9701164c428b02e5b

[yoshiki, 2017-02-01 15:10:13]

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

[commit-bot: I haz the power, 2017-02-01 15:10:29]

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

[yoshiki, 2017-02-01 15:20:36]

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

[commit-bot: I haz the power, 2017-02-01 15:20:43]

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-02-01 15:35:05]

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

[commit-bot: I haz the power, 2017-02-01 15:35:06]

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

[yoshiki, 2017-02-01 16:01:21]

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

[commit-bot: I haz the power, 2017-02-01 16:01: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-02-01 16:42:52]

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

[commit-bot: I haz the power, 2017-02-01 16:42:53]

Dry run: This issue passed the CQ dry run.

[yoshiki, 2017-02-06 22:02:40]

Patchset #1 (id:1) has been deleted

[yoshiki, 2017-02-06 22:02:47]

Patchset #1 (id:20001) has been deleted

[yoshiki, 2017-02-06 22:20:11]

Description was changed from

==========
wip

.

BUG=
==========

to

==========
Not Remove Non-Closable Arc Popup When Close Button is Pressed

* Fixes the issue: crbug.com/689220
  - Adds ArcNotificationDelegate::Close method to handle the closing event
  - Not calls ArcCustomNotificationItem::Close() when the close button is
pressed
* Refine the code
  - Calling the same method MessageView::OnCloseButtonPressed() both from custom
and normal notification.

BUG=689220
==========

[yoshiki, 2017-02-06 22:20:11]

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

[yoshiki, 2017-02-06 22:21:01]

Description was changed from

==========
Not Remove Non-Closable Arc Popup When Close Button is Pressed

* Fixes the issue: crbug.com/689220
  - Adds ArcNotificationDelegate::Close method to handle the closing event
  - Not calls ArcCustomNotificationItem::Close() when the close button is
pressed
* Refine the code
  - Calling the same method MessageView::OnCloseButtonPressed() both from custom
and normal notification.

BUG=689220
==========

to

==========
Not Remove Non-Closable Arc Popup When Close Button is Pressed

* Fixes the issue: crbug.com/689220
  - Adds ArcNotificationDelegate::Close method to handle the closing event
  - Not calls ArcCustomNotificationItem::Close() when the close button is
pressed
* Refine the code
  - Uses the same method MessageView::OnCloseButtonPressed() both from custom
and normal notification

BUG=689220
==========

[yoshiki, 2017-02-06 22:22:22]

Justin, PTAL. Thanks.

https://codereview.chromium.org/2668583005/diff/40001/ui/arc/notification/arc...
File ui/arc/notification/arc_custom_notification_item.cc (left):

https://codereview.chromium.org/2668583005/diff/40001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_item.cc:111: // Needs to manually
remove notification from MessageCenter because
This comment was wrong. We didn't need to call "Close(true)" if we handled
ArcNotificationDelegate::Close() correctly.

[dewittj, 2017-02-07 00:03:12]

https://codereview.chromium.org/2668583005/diff/40001/ui/arc/notification/arc...
File ui/arc/notification/arc_custom_notification_view.cc (right):

https://codereview.chromium.org/2668583005/diff/40001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view.cc:509:
parent_->OnCloseButtonPressed();
any reason you didn't just use views::View::parent()?  If it's about casting,
I'd like to see a DCHECK(parent_ == parent()) so that any hierarchy mistakes are
apparent.

[yoshiki, 2017-02-22 08:53:19]

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

[commit-bot: I haz the power, 2017-02-22 08:54:26]

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

[yoshiki, 2017-02-22 08:55:23]

Justin, Sorry for late, PTAL.

https://codereview.chromium.org/2668583005/diff/40001/ui/arc/notification/arc...
File ui/arc/notification/arc_custom_notification_view.cc (right):

https://codereview.chromium.org/2668583005/diff/40001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view.cc:509:
parent_->OnCloseButtonPressed();
On 2017/02/07 00:03:11, dewittj (OOO 2.20-2.24) wrote:
> any reason you didn't just use views::View::parent()?  If it's about casting,
> I'd like to see a DCHECK(parent_ == parent()) so that any hierarchy mistakes
are
> apparent.

Thanks. I changed the code with using parent(). PTAL again?

[commit-bot: I haz the power, 2017-02-22 10:18:10]

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

[commit-bot: I haz the power, 2017-02-22 10:18:11]

Dry run: This issue passed the CQ dry run.

[yoshiki, 2017-02-27 14:52:01]

Description was changed from

==========
Not Remove Non-Closable Arc Popup When Close Button is Pressed

* Fixes the issue: crbug.com/689220
  - Adds ArcNotificationDelegate::Close method to handle the closing event
  - Not calls ArcCustomNotificationItem::Close() when the close button is
pressed
* Refine the code
  - Uses the same method MessageView::OnCloseButtonPressed() both from custom
and normal notification

BUG=689220
==========

to

==========
Not Remove Non-Closable Arc Popup When Close Button is Pressed

* Fixes the issue: crbug.com/689220
  - Adds ArcNotificationDelegate::Close method to handle the closing event
  - Not calls ArcCustomNotificationItem::Close() when the close button is
pressed
* Refines the code
  - Uses the same method MessageView::OnCloseButtonPressed() both from custom
and normal notification

BUG=689220
==========

[dewittj, 2017-02-28 21:38:48]

https://codereview.chromium.org/2668583005/diff/60001/ui/arc/notification/arc...
File ui/arc/notification/arc_custom_notification_view.cc (right):

https://codereview.chromium.org/2668583005/diff/60001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view.cc:492:
CHECK_EQ(message_center::CustomNotificationView::kViewClassName,
Unclear to me whether this should be CHECK or DCHECK.  I guess check would
prevent possible security bugs if this were ever wrong..  Do we have unit tests
that cause OnBlur and ButtonPressed to be called on ArcCustomNotificationViews? 
With these CHECKs in place we need them so that crashes are detected before they
reach a stable channel.

https://codereview.chromium.org/2668583005/diff/60001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view.cc:501:
CHECK_EQ(message_center::CustomNotificationView::kViewClassName,
this doesn't seem to be relevant here?

[yoshiki, 2017-03-01 17:11:32]

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

[commit-bot: I haz the power, 2017-03-01 17:11:53]

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-03-01 17:59:20]

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

[commit-bot: I haz the power, 2017-03-01 17:59:20]

Dry run: This issue passed the CQ dry run.

[yoshiki, 2017-03-02 03:33:35]

Justin, PTAL. Thanks.

https://codereview.chromium.org/2668583005/diff/60001/ui/arc/notification/arc...
File ui/arc/notification/arc_custom_notification_view.cc (right):

https://codereview.chromium.org/2668583005/diff/60001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view.cc:492:
CHECK_EQ(message_center::CustomNotificationView::kViewClassName,
On 2017/02/28 21:38:48, dewittj wrote:
> Unclear to me whether this should be CHECK or DCHECK.  I guess check would
> prevent possible security bugs if this were ever wrong..  Do we have unit
tests
> that cause OnBlur and ButtonPressed to be called on
ArcCustomNotificationViews? 
> With these CHECKs in place we need them so that crashes are detected before
they
> reach a stable channel.

I'm adding a test, which checks CHECK for ButtonPressed, in a separated patch.
And I'll add tests for OnFocus and OnBlue. 
https://codereview.chromium.org/2723143002/

Can I submit this patch without tests?

https://codereview.chromium.org/2668583005/diff/60001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view.cc:501:
CHECK_EQ(message_center::CustomNotificationView::kViewClassName,
On 2017/02/28 21:38:48, dewittj wrote:
> this doesn't seem to be relevant here?

Good catch. This should be in OnFocus() instead of here.

[yoshiki, 2017-03-02 16:58:55]

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

[commit-bot: I haz the power, 2017-03-02 16:59:16]

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-03-02 17:44:57]

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

[commit-bot: I haz the power, 2017-03-02 17:44:57]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-03-02 20:39:50]

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

[xiyuan, 2017-03-02 20:39:51]

drive-by

https://codereview.chromium.org/2668583005/diff/100001/ui/arc/notification/ar...
File ui/arc/notification/arc_custom_notification_view.h (right):

https://codereview.chromium.org/2668583005/diff/100001/ui/arc/notification/ar...
ui/arc/notification/arc_custom_notification_view.h:38:
ArcCustomNotificationView(ArcCustomNotificationItem* item);
"explicit" is required for single arg constructor per style guide.

https://google.github.io/styleguide/cppguide.html#Implicit_Conversions

https://codereview.chromium.org/2668583005/diff/100001/ui/arc/notification/ar...
ui/arc/notification/arc_custom_notification_view.h:90:
ArcCustomNotificationItem* item_;
nit: Why do we need to change this? It would be useful if the constructor is
changed later and forget to initialize this.

[yoshiki, 2017-03-03 01:08:47]

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

[commit-bot: I haz the power, 2017-03-03 01:09:20]

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

[yoshiki, 2017-03-03 01:20:04]

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

[commit-bot: I haz the power, 2017-03-03 01:20:43]

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

[yoshiki, 2017-03-03 01:21:34]

Patchset #6 (id:140001) has been deleted

[yoshiki, 2017-03-03 01:22:04]

https://codereview.chromium.org/2668583005/diff/100001/ui/arc/notification/ar...
File ui/arc/notification/arc_custom_notification_view.h (right):

https://codereview.chromium.org/2668583005/diff/100001/ui/arc/notification/ar...
ui/arc/notification/arc_custom_notification_view.h:38:
ArcCustomNotificationView(ArcCustomNotificationItem* item);
On 2017/03/02 20:39:51, xiyuan wrote:
> "explicit" is required for single arg constructor per style guide.
> 
> https://google.github.io/styleguide/cppguide.html#Implicit_Conversions

Done.

https://codereview.chromium.org/2668583005/diff/100001/ui/arc/notification/ar...
ui/arc/notification/arc_custom_notification_view.h:90:
ArcCustomNotificationItem* item_;
On 2017/03/02 20:39:51, xiyuan wrote:
> nit: Why do we need to change this? It would be useful if the constructor is
> changed later and forget to initialize this.

Oops, you're right. Reverted.

[commit-bot: I haz the power, 2017-03-03 01:22:42]

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...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[commit-bot: I haz the power, 2017-03-03 02:17:15]

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

[commit-bot: I haz the power, 2017-03-03 02:17:17]

Dry run: This issue passed the CQ dry run.

[dewittj, 2017-03-03 17:44:45]

lgtm

Thanks for writing the tests in another cl.

[xiyuan, 2017-03-03 17:56:03]

lgtm

[yoshiki, 2017-03-06 01:43:49]

The CQ bit was checked by yoshiki@chromium.org

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

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-03-06 02:27:52]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488764629158950,
"parent_rev": "037f99733dfc7a5465e0ac7dc39f3711dc4090d7", "commit_rev":
"9739ee62037118f47be3dab9701164c428b02e5b"}

[commit-bot: I haz the power, 2017-03-06 02:28:50]

Description was changed from

==========
Not Remove Non-Closable Arc Popup When Close Button is Pressed

* Fixes the issue: crbug.com/689220
  - Adds ArcNotificationDelegate::Close method to handle the closing event
  - Not calls ArcCustomNotificationItem::Close() when the close button is
pressed
* Refines the code
  - Uses the same method MessageView::OnCloseButtonPressed() both from custom
and normal notification

BUG=689220
==========

to

==========
Not Remove Non-Closable Arc Popup When Close Button is Pressed

* Fixes the issue: crbug.com/689220
  - Adds ArcNotificationDelegate::Close method to handle the closing event
  - Not calls ArcCustomNotificationItem::Close() when the close button is
pressed
* Refines the code
  - Uses the same method MessageView::OnCloseButtonPressed() both from custom
and normal notification

BUG=689220

Review-Url: https://codereview.chromium.org/2668583005
Cr-Commit-Position: refs/heads/master@{#454814}
Committed:
https://chromium.googlesource.com/chromium/src/+/9739ee62037118f47be3dab97011...
==========

[commit-bot: I haz the power, 2017-03-06 02:28:51]

Committed patchset #5 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/9739ee62037118f47be3dab97011...