Modify TrayItemMore to use enabled status

Modify TrayItemMore to use enabled status

With this patch, if a TrayItemMore object is set to be disabled, it will
not show the more arrow and ripple animations. Therefore, |show_more_|
field is not needed anymore and is removed.

Also, non-MD code path is removed from TrayItemMore.

BUG=687689, 687817
Review-Url: https://codereview.chromium.org/2699443004
Cr-Commit-Position: refs/heads/master@{#451917}
Committed: https://chromium.googlesource.com/chromium/src/+/469c3b662e9bb4bfc28853a166420458079cf19d

[mohsen, 2017-02-15 05:34:19]

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

[commit-bot: I haz the power, 2017-02-15 05:35: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-02-15 05:40:08]

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

[commit-bot: I haz the power, 2017-02-15 05:40:08]

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

[mohsen, 2017-02-16 04:17:19]

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

[commit-bot: I haz the power, 2017-02-16 04:18:01]

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

[mohsen, 2017-02-16 04:19:40]

Description was changed from

==========
Modify TrayItemMore to honor enabled status

With this patch, if a TrayItemMore object is set to be disabled, it will
not show the more arrow and ripple animations. Therefore, |show_more_|
field is not needed anymore and is removed.

BUG=687689
==========

to

==========
Modify TrayItemMore to use enabled status

With this patch, if a TrayItemMore object is set to be disabled, it will
not show the more arrow and ripple animations. Therefore, |show_more_|
field is not needed anymore and is removed.

BUG=687689
==========

[commit-bot: I haz the power, 2017-02-16 06:17:55]

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

[commit-bot: I haz the power, 2017-02-16 06:17:56]

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

[mohsen, 2017-02-16 18:46:03]

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

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

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-16 18:53:40]

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

[commit-bot: I haz the power, 2017-02-16 18:53:41]

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_asan_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_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mohsen, 2017-02-16 22:05:16]

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

[commit-bot: I haz the power, 2017-02-16 22:07:11]

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-17 00:24:06]

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

[commit-bot: I haz the power, 2017-02-17 00:24:06]

Dry run: This issue passed the CQ dry run.

[mohsen, 2017-02-17 03:33:18]

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

[mohsen, 2017-02-17 03:34:31]

Description was changed from

==========
Modify TrayItemMore to use enabled status

With this patch, if a TrayItemMore object is set to be disabled, it will
not show the more arrow and ripple animations. Therefore, |show_more_|
field is not needed anymore and is removed.

BUG=687689
==========

to

==========
Modify TrayItemMore to use enabled status

With this patch, if a TrayItemMore object is set to be disabled, it will
not show the more arrow and ripple animations. Therefore, |show_more_|
field is not needed anymore and is removed.

Also, non-MD code path is removed from TrayItemMore.

BUG=687689,687817
==========

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

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

[mohsen, 2017-02-17 03:37:21]

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

[mohsen, 2017-02-17 03:37:22]

Please take a look...

[commit-bot: I haz the power, 2017-02-17 05:24:27]

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

[commit-bot: I haz the power, 2017-02-17 05:24:28]

Dry run: This issue passed the CQ dry run.

[tdanderson, 2017-02-17 22:29:47]

Looks great! Just a few questions below:

https://codereview.chromium.org/2699443004/diff/100001/ash/common/system/tray...
File ash/common/system/tray/tray_item_more.cc (right):

https://codereview.chromium.org/2699443004/diff/100001/ash/common/system/tray...
ash/common/system/tray/tray_item_more.cc:73: style->SetupLabel(label_);
Don't we also want to be theming |icon_| based on the enabled/disabled state?

https://codereview.chromium.org/2699443004/diff/100001/ash/common/system/tray...
ash/common/system/tray/tray_item_more.cc:89:
tri_view_->SetContainerVisible(TriView::Container::END, enabled());
If the enabled state were to ever change from disabled->enabled (though I don't
think that will currently ever happen), it seems that you'd want to be calling
UpdateStyle() otherwise the color theming will be incorrect. Is there any harm
in just calling UpdateStyle() at the end of this function?

https://codereview.chromium.org/2699443004/diff/100001/ash/common/system/tray...
File ash/common/system/tray/tray_item_more.h (right):

https://codereview.chromium.org/2699443004/diff/100001/ash/common/system/tray...
ash/common/system/tray/tray_item_more.h:41: // However, if the view is disabled
will force disabled ColorStyle.
Consider shortening to "Returns a style that will be applied to the elements in
the UpdateStyle() method if |this| is enabled; otherwise we force |this| to use
ColorStyle::DISABLED." or similar

[mohsen, 2017-02-21 20:59:40]

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

[commit-bot: I haz the power, 2017-02-21 21:00:55]

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

[mohsen, 2017-02-21 21:04:56]

https://codereview.chromium.org/2699443004/diff/100001/ash/common/system/tray...
File ash/common/system/tray/tray_item_more.cc (right):

https://codereview.chromium.org/2699443004/diff/100001/ash/common/system/tray...
ash/common/system/tray/tray_item_more.cc:73: style->SetupLabel(label_);
On 2017/02/17 at 22:29:47, tdanderson wrote:
> Don't we also want to be theming |icon_| based on the enabled/disabled state?

Theming of the |icon_| happens in each subclass since currently we can't
generically change the icon color here. Subclasses already use the style
returned by CreateStyle() which now takes enabled state into account to update
the icon (an exception is network row).

https://codereview.chromium.org/2699443004/diff/100001/ash/common/system/tray...
ash/common/system/tray/tray_item_more.cc:89:
tri_view_->SetContainerVisible(TriView::Container::END, enabled());
On 2017/02/17 at 22:29:47, tdanderson wrote:
> If the enabled state were to ever change from disabled->enabled (though I
don't think that will currently ever happen), it seems that you'd want to be
calling UpdateStyle() otherwise the color theming will be incorrect. Is there
any harm in just calling UpdateStyle() at the end of this function?

Right. Done.

https://codereview.chromium.org/2699443004/diff/100001/ash/common/system/tray...
File ash/common/system/tray/tray_item_more.h (right):

https://codereview.chromium.org/2699443004/diff/100001/ash/common/system/tray...
ash/common/system/tray/tray_item_more.h:41: // However, if the view is disabled
will force disabled ColorStyle.
On 2017/02/17 at 22:29:47, tdanderson wrote:
> Consider shortening to "Returns a style that will be applied to the elements
in the UpdateStyle() method if |this| is enabled; otherwise we force |this| to
use ColorStyle::DISABLED." or similar

Done.

[commit-bot: I haz the power, 2017-02-21 21:08:51]

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

[commit-bot: I haz the power, 2017-02-21 21:08:52]

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

[mohsen, 2017-02-21 21:28:11]

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

[commit-bot: I haz the power, 2017-02-21 21:29:47]

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

[tdanderson, 2017-02-21 21:30:41]

LGTM

https://codereview.chromium.org/2699443004/diff/100001/ash/common/system/tray...
File ash/common/system/tray/tray_item_more.cc (right):

https://codereview.chromium.org/2699443004/diff/100001/ash/common/system/tray...
ash/common/system/tray/tray_item_more.cc:73: style->SetupLabel(label_);
On 2017/02/21 21:04:56, mohsen wrote:
> On 2017/02/17 at 22:29:47, tdanderson wrote:
> > Don't we also want to be theming |icon_| based on the enabled/disabled
state?
> 
> Theming of the |icon_| happens in each subclass since currently we can't
> generically change the icon color here. Subclasses already use the style
> returned by CreateStyle() which now takes enabled state into account to update
> the icon (an exception is network row).

Acknowledged. Might be worth a quick chat with Sebastien to see if he's happy
with that or if he has any further theming suggestions (no need to block landing
this CL, though.)

[commit-bot: I haz the power, 2017-02-21 23:35:56]

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

[commit-bot: I haz the power, 2017-02-21 23:35:57]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[mohsen, 2017-02-22 00:03:51]

The CQ bit was checked by mohsen@chromium.org

[commit-bot: I haz the power, 2017-02-22 00:05:17]

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

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

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

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[mohsen, 2017-02-22 02:09:59]

The CQ bit was checked by mohsen@chromium.org

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

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-22 08:05:01]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1487729399158910,
"parent_rev": "a6345fb8100733f36a40b8578c38ec3980919780", "commit_rev":
"469c3b662e9bb4bfc28853a166420458079cf19d"}

[commit-bot: I haz the power, 2017-02-22 08:05:38]

Description was changed from

==========
Modify TrayItemMore to use enabled status

With this patch, if a TrayItemMore object is set to be disabled, it will
not show the more arrow and ripple animations. Therefore, |show_more_|
field is not needed anymore and is removed.

Also, non-MD code path is removed from TrayItemMore.

BUG=687689,687817
==========

to

==========
Modify TrayItemMore to use enabled status

With this patch, if a TrayItemMore object is set to be disabled, it will
not show the more arrow and ripple animations. Therefore, |show_more_|
field is not needed anymore and is removed.

Also, non-MD code path is removed from TrayItemMore.

BUG=687689,687817

Review-Url: https://codereview.chromium.org/2699443004
Cr-Commit-Position: refs/heads/master@{#451917}
Committed:
https://chromium.googlesource.com/chromium/src/+/469c3b662e9bb4bfc28853a16642...
==========

[commit-bot: I haz the power, 2017-02-22 08:05:40]

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