Remove non-MD code from TrayBackgroundView

Remove non-MD code from TrayBackgroundView

BUG=687806
TEST=none

Review-Url: https://codereview.chromium.org/2723843002
Cr-Commit-Position: refs/heads/master@{#455957}
Committed: https://chromium.googlesource.com/chromium/src/+/8d97710b15a51c4e2cec80be55d97bc722f84fb1

[mohsen, 2017-02-28 23:28:50]

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

[commit-bot: I haz the power, 2017-02-28 23:29:31]

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 00:38:58]

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

[commit-bot: I haz the power, 2017-03-01 00:38:59]

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

[mohsen, 2017-03-01 01:26:58]

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

[commit-bot: I haz the power, 2017-03-01 01:27:30]

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 02:45:58]

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

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

Dry run: This issue passed the CQ dry run.

[mohsen, 2017-03-01 02:55:28]

mohsen@chromium.org changed reviewers:
+ tdanderson@chromium.org

[mohsen, 2017-03-01 02:55:29]

Please take a look...

[Evan Stade, 2017-03-01 04:12:20]

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

[Evan Stade, 2017-03-01 04:12:21]

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

https://codereview.chromium.org/2723843002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_background_view.cc:350: background_ = new
TrayBackground(this);
Do you need this function?

[mohsen, 2017-03-01 04:40:41]

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

https://codereview.chromium.org/2723843002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_background_view.cc:350: background_ = new
TrayBackground(this);
On 2017/03/01 at 04:12:21, Evan Stade wrote:
> Do you need this function?

It is not called from LogoutButtonTray. So, I think not all subclasses of
TrayBackgroundView need the background.

[Evan Stade, 2017-03-01 07:20:21]

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

https://codereview.chromium.org/2723843002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_background_view.cc:350: background_ = new
TrayBackground(this);
On 2017/03/01 04:40:41, mohsen wrote:
> On 2017/03/01 at 04:12:21, Evan Stade wrote:
> > Do you need this function?
> 
> It is not called from LogoutButtonTray. So, I think not all subclasses of
> TrayBackgroundView need the background.

Good point, but I still think we should remove it (set by default) and just have
the logout button clear the background on tray_container_. It was only something
you could dynamically set because of the activat-ability.

[mohsen, 2017-03-01 17:48:05]

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

https://codereview.chromium.org/2723843002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_background_view.cc:350: background_ = new
TrayBackground(this);
On 2017/03/01 at 07:20:21, Evan Stade wrote:
> On 2017/03/01 04:40:41, mohsen wrote:
> > On 2017/03/01 at 04:12:21, Evan Stade wrote:
> > > Do you need this function?
> > 
> > It is not called from LogoutButtonTray. So, I think not all subclasses of
> > TrayBackgroundView need the background.
> 
> Good point, but I still think we should remove it (set by default) and just
have the logout button clear the background on tray_container_. It was only
something you could dynamically set because of the activat-ability.

I think logout button would still need a similar method to be able to clear
|background_|. How about adding a flag to TrayBackgroundView c'tor?

[Evan Stade, 2017-03-01 18:04:14]

On 2017/03/01 17:48:05, mohsen wrote:
>
https://codereview.chromium.org/2723843002/diff/20001/ash/common/system/tray/...
> File ash/common/system/tray/tray_background_view.cc (right):
> 
>
https://codereview.chromium.org/2723843002/diff/20001/ash/common/system/tray/...
> ash/common/system/tray/tray_background_view.cc:350: background_ = new
> TrayBackground(this);
> On 2017/03/01 at 07:20:21, Evan Stade wrote:
> > On 2017/03/01 04:40:41, mohsen wrote:
> > > On 2017/03/01 at 04:12:21, Evan Stade wrote:
> > > > Do you need this function?
> > > 
> > > It is not called from LogoutButtonTray. So, I think not all subclasses of
> > > TrayBackgroundView need the background.
> > 
> > Good point, but I still think we should remove it (set by default) and just
> have the logout button clear the background on tray_container_. It was only
> something you could dynamically set because of the activat-ability.
> 
> I think logout button would still need a similar method to be able to clear
> |background_|.

Yea, I guess background_ is an issue here. But at least only one(?) subview has
to call it.

> How about adding a flag to TrayBackgroundView c'tor?

that's ok with me. I'd slightly prefer a function that clears it after
construction so the normal case (background on) doesn't have to worry about it,
but either way works.

[mohsen, 2017-03-01 19:25:19]

On 2017/03/01 at 18:04:14, estade wrote:
> On 2017/03/01 17:48:05, mohsen wrote:
> >
https://codereview.chromium.org/2723843002/diff/20001/ash/common/system/tray/...
> > File ash/common/system/tray/tray_background_view.cc (right):
> > 
> >
https://codereview.chromium.org/2723843002/diff/20001/ash/common/system/tray/...
> > ash/common/system/tray/tray_background_view.cc:350: background_ = new
> > TrayBackground(this);
> > On 2017/03/01 at 07:20:21, Evan Stade wrote:
> > > On 2017/03/01 04:40:41, mohsen wrote:
> > > > On 2017/03/01 at 04:12:21, Evan Stade wrote:
> > > > > Do you need this function?
> > > > 
> > > > It is not called from LogoutButtonTray. So, I think not all subclasses
of
> > > > TrayBackgroundView need the background.
> > > 
> > > Good point, but I still think we should remove it (set by default) and
just
> > have the logout button clear the background on tray_container_. It was only
> > something you could dynamically set because of the activat-ability.
> > 
> > I think logout button would still need a similar method to be able to clear
> > |background_|.
> 
> Yea, I guess background_ is an issue here. But at least only one(?) subview
has to call it.
> 
> > How about adding a flag to TrayBackgroundView c'tor?
> 
> that's ok with me. I'd slightly prefer a function that clears it after
construction so the normal case (background on) doesn't have to worry about it,
but either way works.

I added a flag to the c'tor (I prefer not to create the background and then
delete it immediately). Looks good?

[Evan Stade, 2017-03-02 02:49:51]

https://codereview.chromium.org/2723843002/diff/40001/ash/common/system/tray/...
File ash/common/system/tray/tray_background_view.h (right):

https://codereview.chromium.org/2723843002/diff/40001/ash/common/system/tray/...
ash/common/system/tray/tray_background_view.h:63: TrayBackgroundView(WmShelf*
wm_shelf, bool no_background);
I would expect this to be inverted, i.e. bool draws_background

That said, does LogoutButton need to be a TrayBackgroundView? It doesn't have a
background or a separator. What functionality is it actually relying on?

[mohsen, 2017-03-02 04:07:09]

https://codereview.chromium.org/2723843002/diff/40001/ash/common/system/tray/...
File ash/common/system/tray/tray_background_view.h (right):

https://codereview.chromium.org/2723843002/diff/40001/ash/common/system/tray/...
ash/common/system/tray/tray_background_view.h:63: TrayBackgroundView(WmShelf*
wm_shelf, bool no_background);
On 2017/03/02 at 02:49:51, Evan Stade wrote:
> I would expect this to be inverted, i.e. bool draws_background
Done.
 
> That said, does LogoutButton need to be a TrayBackgroundView? It doesn't have
a background or a separator. What functionality is it actually relying on?

Hmm, good question. It doesn't even use clickability of ActionableView since it
hosts its own clickable button. It apparently only uses TrayBackgroundView's
TrayContainer layout. Probably we can take TrayContainer out of
TrayBackgroundView and make logout button a regular View that uses
TrayContainer? Added a TODO.

[Evan Stade, 2017-03-03 01:30:46]

lgtm

https://codereview.chromium.org/2723843002/diff/60001/ash/common/system/tray/...
File ash/common/system/tray/tray_background_view.h (right):

https://codereview.chromium.org/2723843002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/tray_background_view.h:63: TrayBackgroundView(WmShelf*
wm_shelf, bool draws_background);
nit: can you also add a TODO here to remove this param if/when it's not needed?

[mohsen, 2017-03-03 04:39:37]

tdanderson@: Can you take a look...

https://codereview.chromium.org/2723843002/diff/60001/ash/common/system/tray/...
File ash/common/system/tray/tray_background_view.h (right):

https://codereview.chromium.org/2723843002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/tray_background_view.h:63: TrayBackgroundView(WmShelf*
wm_shelf, bool draws_background);
On 2017/03/03 at 01:30:46, Evan Stade wrote:
> nit: can you also add a TODO here to remove this param if/when it's not
needed?

Done.

[mohsen, 2017-03-03 07:08:08]

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

[commit-bot: I haz the power, 2017-03-03 07:08:20]

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-03 07:10:25]

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

[commit-bot: I haz the power, 2017-03-03 07:10:28]

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

[mohsen, 2017-03-03 07:19:08]

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

[commit-bot: I haz the power, 2017-03-03 07:19:20]

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-03 09:22:20]

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

[commit-bot: I haz the power, 2017-03-03 09:22:21]

Dry run: This issue passed the CQ dry run.

[tdanderson, 2017-03-07 22:19:07]

LGTM, and apologies for the review delay.

[mohsen, 2017-03-08 04:24:42]

The CQ bit was checked by mohsen@chromium.org

[mohsen, 2017-03-08 04:24:42]

The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org,
tdanderson@chromium.org
Link to the patchset: https://codereview.chromium.org/2723843002/#ps120001
(title: "Rebased")

[commit-bot: I haz the power, 2017-03-08 04:24:54]

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

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

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

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_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  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_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mohsen, 2017-03-08 04:27:54]

The CQ bit was checked by mohsen@chromium.org

[commit-bot: I haz the power, 2017-03-08 04:28:08]

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-08 04:29:13]

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

[commit-bot: I haz the power, 2017-03-08 04:29:15]

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_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  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_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[mohsen, 2017-03-08 05:31:10]

The CQ bit was checked by mohsen@chromium.org

[commit-bot: I haz the power, 2017-03-08 05:31:26]

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-08 05:38:45]

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

[commit-bot: I haz the power, 2017-03-08 05:38:47]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[mohsen, 2017-03-08 05:47:51]

mohsen@chromium.org changed reviewers:
+ bruthig@chromium.org

[mohsen, 2017-03-08 05:47:52]

bruthig@: Can you take a look at changes in ui/views/animaiton/?

[bruthig, 2017-03-09 23:08:40]

lgtm

[mohsen, 2017-03-09 23:46:50]

The CQ bit was checked by mohsen@chromium.org

[commit-bot: I haz the power, 2017-03-09 23:47:35]

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-10 01:01:21]

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

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

Failed to apply patch for
ash/common/system/web_notification/web_notification_tray.cc:
While running git apply --index -p1;
  error: patch failed:
ash/common/system/web_notification/web_notification_tray.cc:297
  error: ash/common/system/web_notification/web_notification_tray.cc: patch does
not apply

Patch:       ash/common/system/web_notification/web_notification_tray.cc
Index: ash/common/system/web_notification/web_notification_tray.cc
diff --git a/ash/common/system/web_notification/web_notification_tray.cc
b/ash/common/system/web_notification/web_notification_tray.cc
index
69401ce4ae27d55e7d5d935b679ec3ad26b9c3bb..e090a7c520da7ec47e00b2db08163596c129f81f
100644
--- a/ash/common/system/web_notification/web_notification_tray.cc
+++ b/ash/common/system/web_notification/web_notification_tray.cc
@@ -285,7 +285,7 @@ class WebNotificationLabel : public WebNotificationItem {
 WebNotificationTray::WebNotificationTray(WmShelf* shelf,
                                          WmWindow* status_area_window,
                                          SystemTray* system_tray)
-    : TrayBackgroundView(shelf),
+    : TrayBackgroundView(shelf, true),
       status_area_window_(status_area_window),
       system_tray_(system_tray),
       show_message_center_on_unlock_(false),
@@ -297,14 +297,12 @@ WebNotificationTray::WebNotificationTray(WmShelf* shelf,
 
   if (MaterialDesignController::IsShelfMaterial()) {
     SetInkDropMode(InkDropMode::ON);
-    SetContentsBackground(false);
     gfx::ImageSkia bell_image =
         CreateVectorIcon(kShelfNotificationsIcon, kShelfIconColor);
     const gfx::Size bell_icon_size = kTrayItemInnerIconSize;
     bell_icon_.reset(new WebNotificationImage(
         bell_image, bell_icon_size, animation_container_.get(), this));
   } else {
-    SetContentsBackground(true);
     gfx::ImageSkia bell_image =
         CreateVectorIcon(gfx::VectorIconId::NOTIFICATIONS, kNoUnreadIconSize,
                          kWebNotificationColorNoUnread);

[mohsen, 2017-03-10 01:16:48]

The CQ bit was checked by mohsen@chromium.org

[mohsen, 2017-03-10 01:16:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdanderson@chromium.org, estade@chromium.org, bruthig@chromium.org
Link to the patchset: https://codereview.chromium.org/2723843002/#ps140001
(title: "Rebased")

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

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-10 02:04:34]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1489108608534800,
"parent_rev": "d8ee83729c29c767b7465dc23940b83a8a88fb89", "commit_rev":
"8d97710b15a51c4e2cec80be55d97bc722f84fb1"}

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

Description was changed from

==========
Remove non-MD code from TrayBackgroundView

BUG=687806
TEST=none
==========

to

==========
Remove non-MD code from TrayBackgroundView

BUG=687806
TEST=none

Review-Url: https://codereview.chromium.org/2723843002
Cr-Commit-Position: refs/heads/master@{#455957}
Committed:
https://chromium.googlesource.com/chromium/src/+/8d97710b15a51c4e2cec80be55d9...
==========

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

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/8d97710b15a51c4e2cec80be55d9...