Add unittests of ArcCustomNotificationView

Add unittests of ArcCustomNotificationView

Major changes:
- Merge ArcCustomNotificationView into ArcNotificationView
  Since non-custom version is no logner used by itself.
- Split exo::NotificationSurface into implementation and interface
- Split arc::ArcNotificationView into implementation and interface
- Add unittests of ArcNotificationView
- Separate ArcNotificationDelegate into a separated file

This patch contains some cleanup, removing unnecessary code and moving code. But the logic except tests is not changed at all.

BUG=697359
TEST=newly added unittests run without error

[yoshiki, 2017-03-01 16:36:52]

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

[commit-bot: I haz the power, 2017-03-01 16:37:16]

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-01 16:37:52]

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

[commit-bot: I haz the power, 2017-03-01 16:38:19]

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 16:43:22]

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

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

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

[yoshiki, 2017-03-01 16:46:13]

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

[commit-bot: I haz the power, 2017-03-01 16:46:25]

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 16:50:52]

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

[commit-bot: I haz the power, 2017-03-01 16:50:53]

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

[yoshiki, 2017-03-02 02:57:17]

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

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

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 03:06:33]

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

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

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  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-03-02 03:22:18]

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

[yoshiki, 2017-03-02 03:22:30]

Patchset #1 (id:1) has been deleted

[yoshiki, 2017-03-02 03:22:39]

Patchset #1 (id:20001) has been deleted

[yoshiki, 2017-03-02 03:22:45]

Patchset #1 (id:40001) has been deleted

[yoshiki, 2017-03-02 03:22:51]

Patchset #1 (id:60001) has been deleted

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

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-02 03:26:03]

Description was changed from

==========
wip

.


.


.


.


.


.


.


.


.

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

to

==========
Add unittests of ArcNotificationView

Major changes:
- Merge ArcCustomNotificationView into ArcNotificationView
  Since non-custom version is no logner used.
- Split exo::NotificationSurface into implementation and interface
- Split arc::ArcNotificationView into implementation and interface
- Add unittests of ArcNotificationView


.


.


.


.


.


.


.


.

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

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

Description was changed from

==========
Add unittests of ArcNotificationView

Major changes:
- Merge ArcCustomNotificationView into ArcNotificationView
  Since non-custom version is no logner used.
- Split exo::NotificationSurface into implementation and interface
- Split arc::ArcNotificationView into implementation and interface
- Add unittests of ArcNotificationView


.


.


.


.


.


.


.


.

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

to

==========
Add unittests of ArcNotificationView

Major changes:
- Merge ArcCustomNotificationView into ArcNotificationView
  Since non-custom version is no logner used.
- Split exo::NotificationSurface into implementation and interface
- Split arc::ArcNotificationView into implementation and interface
- Add unittests of ArcNotificationView
- Separate ArcNotificationDelegate into a separated file

This patch contains some cleanup, removing unnecessary code and moving code. But
the logic except tests is not changed at all.

BUG=697359
TEST=newly added unittests run without error
==========

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

yoshiki@chromium.org changed reviewers:
+ hidehiko@chromium.org, reveman@chromium.org

[yoshiki, 2017-03-02 03:37:10]

Description was changed from

==========
Add unittests of ArcNotificationView

Major changes:
- Merge ArcCustomNotificationView into ArcNotificationView
  Since non-custom version is no logner used.
- Split exo::NotificationSurface into implementation and interface
- Split arc::ArcNotificationView into implementation and interface
- Add unittests of ArcNotificationView
- Separate ArcNotificationDelegate into a separated file

This patch contains some cleanup, removing unnecessary code and moving code. But
the logic except tests is not changed at all.

BUG=697359
TEST=newly added unittests run without error
==========

to

==========
Add unittests of ArcNotificationView

Major changes:
- Merge ArcCustomNotificationView into ArcNotificationView
  Since non-custom version is no logner used by itself.
- Split exo::NotificationSurface into implementation and interface
- Split arc::ArcNotificationView into implementation and interface
- Add unittests of ArcNotificationView
- Separate ArcNotificationDelegate into a separated file

This patch contains some cleanup, removing unnecessary code and moving code. But
the logic except tests is not changed at all.

BUG=697359
TEST=newly added unittests run without error
==========

[yoshiki, 2017-03-02 03:39:18]

Hidehiko: ui/arc/*
David: components/exo/*

PTAL?

[commit-bot: I haz the power, 2017-03-02 04:23:19]

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

[commit-bot: I haz the power, 2017-03-02 04:23:20]

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_...)

[hidehiko, 2017-03-02 15:38:17]

My initial scan.

The bot failure looks real one, as ui arc test is failing. Could you look into
it?

https://codereview.chromium.org/2723143002/diff/80001/components/exo/notifica...
File components/exo/notification_surface.h (right):

https://codereview.chromium.org/2723143002/diff/80001/components/exo/notifica...
components/exo/notification_surface.h:12: #include
"components/exo/surface_delegate.h"
Unused?

https://codereview.chromium.org/2723143002/diff/80001/components/exo/notifica...
components/exo/notification_surface.h:13: #include
"components/exo/surface_observer.h"
Ditto.

https://codereview.chromium.org/2723143002/diff/80001/components/exo/notifica...
components/exo/notification_surface.h:29: virtual ~NotificationSurface() {}
How about; s/{}/= default;/

https://codereview.chromium.org/2723143002/diff/80001/components/exo/notifica...
components/exo/notification_surface.h:31: virtual gfx::Size GetSize() const = 0;
Because this is now interface, could you document for each function?

https://codereview.chromium.org/2723143002/diff/80001/components/exo/notifica...
components/exo/notification_surface.h:35: virtual aura::Window* window() = 0;
Optional: These are now virtual function so it can be non-trivial getter. So,
how about GetWindow() and GetNotificationId() ?

https://codereview.chromium.org/2723143002/diff/80001/components/exo/notifica...
File components/exo/notification_surface_impl.cc (right):

https://codereview.chromium.org/2723143002/diff/80001/components/exo/notifica...
components/exo/notification_surface_impl.cc:105: void
NotificationSurfaceImpl::AttachWindow(views::NativeViewHost* nvh) {
Could you avoid non-trivial abbreviation?

https://codereview.chromium.org/2723143002/diff/80001/ui/arc/notification/arc...
File ui/arc/notification/arc_custom_notification_view.h (right):

https://codereview.chromium.org/2723143002/diff/80001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view.h:38: static const char
kViewClassName[];
Could you add brief comment what this is for?

https://codereview.chromium.org/2723143002/diff/80001/ui/arc/notification/arc...
File ui/arc/notification/arc_custom_notification_view_unittest.cc (right):

https://codereview.chromium.org/2723143002/diff/80001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view_unittest.cc:24: class
MockNotificationSurface : public exo::NotificationSurface {
Optional: Mock implies gmock in Chromium, IIUC.
How about Fake?

https://codereview.chromium.org/2723143002/diff/80001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view_unittest.cc:49: class
MockArcNotificationItem : public ArcNotificationItem {
Moving to anonymous namespace, too?

Also how about s/Mock/Fake/ here, too?

https://codereview.chromium.org/2723143002/diff/80001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view_unittest.cc:55:
mojom::ArcNotificationDataPtr data) override {}
s/{}/= default;/

https://codereview.chromium.org/2723143002/diff/80001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view_unittest.cc:97:
TestMessageCenterController() {}
s/{}/= default;/

https://codereview.chromium.org/2723143002/diff/80001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view_unittest.cc:148: ~DummyEvent()
override {}
s/{}/= default;/
Or, you may be able to drop it simply.

https://codereview.chromium.org/2723143002/diff/80001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view_unittest.cc:153:
ArcCustomNotificationViewTest() {}
ditto.

https://codereview.chromium.org/2723143002/diff/80001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view_unittest.cc:157: std::string
notification_id("notification id");
Optional: in Chromium, std::string foo = "string"; is more popular?

https://codereview.chromium.org/2723143002/diff/80001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view_unittest.cc:159:
views::ViewsTestBase::SetUp();
Could you do this at beginning of SetUp()?

https://codereview.chromium.org/2723143002/diff/80001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view_unittest.cc:160:
notification_item_.reset(new MockArcNotificationItem(notification_id));
Optional: I recommend to use notification_item_ = base::MakeUnique<...>(...).
Ditto for below.

https://codereview.chromium.org/2723143002/diff/80001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view_unittest.cc:181: views::Widget*
widget = new views::Widget();
Looks leak?

https://codereview.chromium.org/2723143002/diff/80001/ui/arc/notification/arc...
File ui/arc/notification/arc_notification_item.h (right):

https://codereview.chromium.org/2723143002/diff/80001/ui/arc/notification/arc...
ui/arc/notification/arc_notification_item.h:29: virtual ~ArcNotificationItem()
{}
ditto.

https://codereview.chromium.org/2723143002/diff/80001/ui/arc/notification/arc...
ui/arc/notification/arc_notification_item.h:31: virtual void
UpdateWithArcNotificationData(
As you change this to interface, could you document for each function, and avoid
using accessor_name?

https://codereview.chromium.org/2723143002/diff/80001/ui/arc/notification/arc...
File ui/arc/notification/arc_notification_item_impl.cc (right):

https://codereview.chromium.org/2723143002/diff/80001/ui/arc/notification/arc...
ui/arc/notification/arc_notification_item_impl.cc:64: 
Here, some code is dropped. Any background?

https://codereview.chromium.org/2723143002/diff/80001/ui/arc/notification/arc...
File ui/arc/notification/arc_notification_manager.cc (right):

https://codereview.chromium.org/2723143002/diff/80001/ui/arc/notification/arc...
ui/arc/notification/arc_notification_manager.cc:70: ArcNotificationItem* item =
new ArcNotificationItemImpl(
nit: maybe auto item = base::MakeUnique<>() then you just can move it at L73?

[yoshiki, 2017-04-26 08:59:58]

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

[commit-bot: I haz the power, 2017-04-26 09:00:09]

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-04-26 09:03:10]

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

[commit-bot: I haz the power, 2017-04-26 09:03:11]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[yoshiki, 2017-04-27 01:57:36]

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

[commit-bot: I haz the power, 2017-04-27 01:58: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-04-27 02:03:56]

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

[commit-bot: I haz the power, 2017-04-27 02:03:57]

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...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_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_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[yoshiki, 2017-04-27 07:48:54]

yoshiki@chromium.org changed reviewers:
- hidehiko@chromium.org, reveman@chromium.org

[yoshiki, 2017-05-01 05:24:11]

Thank you for reviewing. The M58 work is getting finished, so I want to resume
this work.

I just think this CL is large, so let me split this into some smaller CLs.
Thanks.

[yoshiki, 2017-05-10 00:33:16]

Description was changed from

==========
Add unittests of ArcNotificationView

Major changes:
- Merge ArcCustomNotificationView into ArcNotificationView
  Since non-custom version is no logner used by itself.
- Split exo::NotificationSurface into implementation and interface
- Split arc::ArcNotificationView into implementation and interface
- Add unittests of ArcNotificationView
- Separate ArcNotificationDelegate into a separated file

This patch contains some cleanup, removing unnecessary code and moving code. But
the logic except tests is not changed at all.

BUG=697359
TEST=newly added unittests run without error
==========

to

==========
Add unittests of ArcCustomNotificationView

Major changes:
- Merge ArcCustomNotificationView into ArcNotificationView
  Since non-custom version is no logner used by itself.
- Split exo::NotificationSurface into implementation and interface
- Split arc::ArcNotificationView into implementation and interface
- Add unittests of ArcNotificationView
- Separate ArcNotificationDelegate into a separated file

This patch contains some cleanup, removing unnecessary code and moving code. But
the logic except tests is not changed at all.

BUG=697359
TEST=newly added unittests run without error
==========