Make TrayDetailsView a ViewClickListener and ButtonListener

Make TrayDetailsView a ViewClickListener and ButtonListener

Some initial changes in preparation for implementing
the material design version of the detailed views
in the Ash system menu:

* Make TrayDetailsView extend ViewClickListener
  and ButtonListener instead of having each of
  TrayDetailsView's subclasses extend these.
  The aim is to place common code in TrayDetailsView.

* Rename the TrayDetailsView "footer row" to be
  the "title row"; for material design this row
  appears at the top of the detailed view, not
  the bottom.

* For material design (--ash-md=experimental), show
  the title row at the top of the detailed view
  and do not instantiate or show any non-MD
  buttons in this row. Further improvements to
  the appearance and functionality of the title
  rows to be handled in subsequent CLs.

BUG=642136
TEST=ash_unittests

Committed: https://crrev.com/bd26b635c2b1e5ad8287a4f87d87f5ce2c697267
Cr-Commit-Position: refs/heads/master@{#416134}

[tdanderson, 2016-08-30 16:27:28]

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

[commit-bot: I haz the power, 2016-08-30 16:27:42]

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

[tdanderson, 2016-08-30 16:36:21]

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

[commit-bot: I haz the power, 2016-08-30 16:36:33]

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

[tdanderson, 2016-08-30 16:36:46]

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

[tdanderson, 2016-08-30 16:36:47]

Valery, can you please take a look?

[commit-bot: I haz the power, 2016-08-30 17:19:30]

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

[commit-bot: I haz the power, 2016-08-30 17:19:31]

Dry run: Exceeded global retry quota

[varkha, 2016-08-31 20:28:14]

That's a nice start. One thing that is visually a bit confusing is that the
title row has the same highlight as the currently hot-tracked row and so it
looks as if both are hot-tracked.

https://codereview.chromium.org/2289223002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/bluetooth/tray_bluetooth.cc (right):

https://codereview.chromium.org/2289223002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:146: const bool md =
MaterialDesignController::IsSystemTrayMenuMaterial();
nit: s/md/material for consistency and I thought we frown upon abbreviations?

https://codereview.chromium.org/2289223002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:149: if (!md)
What do you think about replacing this with "if (throbber_)"? Just as easy to
remove in future and concentrates use of MDC where the controls are created.

https://codereview.chromium.org/2289223002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:153: if (!md)
ditto.

https://codereview.chromium.org/2289223002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:163: if
(!MaterialDesignController::IsSystemTrayMenuMaterial())
ditto.

https://codereview.chromium.org/2289223002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/ime_menu/ime_list_view.cc (right):

https://codereview.chromium.org/2289223002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/ime_menu/ime_list_view.cc:139: std::map<views::View*,
std::string>::const_iterator ime_find;
Merge those two lines? Here and below for prop_find.
I would also s/ime_find/ime and s/prop_find/property.

https://codereview.chromium.org/2289223002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/ime_menu/ime_list_view.cc:145: GetWidget()->Close();
nit: This could be taken out of {} and done at the end of this method body if
you add and else and an early return in line 153.

https://codereview.chromium.org/2289223002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/ime_menu/ime_list_view.h (right):

https://codereview.chromium.org/2289223002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/ime_menu/ime_list_view.h:9: #include
"ash/common/system/tray/tray_details_view.h"
Thanks for removing unused headers.

https://codereview.chromium.org/2289223002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/network/network_state_list_detailed_view.h
(right):

https://codereview.chromium.org/2289223002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/network/network_state_list_detailed_view.h:60: void
OnViewClicked(views::View* sender) override;
Where is this overridden from?

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

https://codereview.chromium.org/2289223002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_details_view.cc:86: 
nit: no empty line.

[tdanderson, 2016-08-31 22:22:37]

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

[commit-bot: I haz the power, 2016-08-31 22:23:23]

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

[tdanderson, 2016-08-31 22:24:15]

Valery, please take another look. Regarding your comment about the appearance of
the top row, that will be addressed in a subsequent CL (this CL is only the
first piece).

https://codereview.chromium.org/2289223002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/bluetooth/tray_bluetooth.cc (right):

https://codereview.chromium.org/2289223002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:146: const bool md =
MaterialDesignController::IsSystemTrayMenuMaterial();
On 2016/08/31 20:28:13, varkha wrote:
> nit: s/md/material for consistency and I thought we frown upon abbreviations?

Removed.

https://codereview.chromium.org/2289223002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:149: if (!md)
On 2016/08/31 20:28:14, varkha wrote:
> What do you think about replacing this with "if (throbber_)"? Just as easy to
> remove in future and concentrates use of MDC where the controls are created.

Good idea, changed.

https://codereview.chromium.org/2289223002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:153: if (!md)
On 2016/08/31 20:28:14, varkha wrote:
> ditto.

Done.

https://codereview.chromium.org/2289223002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:163: if
(!MaterialDesignController::IsSystemTrayMenuMaterial())
On 2016/08/31 20:28:14, varkha wrote:
> ditto.

Done.

https://codereview.chromium.org/2289223002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/ime_menu/ime_list_view.cc (right):

https://codereview.chromium.org/2289223002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/ime_menu/ime_list_view.cc:139: std::map<views::View*,
std::string>::const_iterator ime_find;
On 2016/08/31 20:28:14, varkha wrote:
> Merge those two lines? Here and below for prop_find.
> I would also s/ime_find/ime and s/prop_find/property.

Done.

https://codereview.chromium.org/2289223002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/ime_menu/ime_list_view.cc:145: GetWidget()->Close();
On 2016/08/31 20:28:14, varkha wrote:
> nit: This could be taken out of {} and done at the end of this method body if
> you add and else and an early return in line 153.

Done, but in a slightly different manner than suggested.

https://codereview.chromium.org/2289223002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/network/network_state_list_detailed_view.h
(right):

https://codereview.chromium.org/2289223002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/network/network_state_list_detailed_view.h:60: void
OnViewClicked(views::View* sender) override;
On 2016/08/31 20:28:14, varkha wrote:
> Where is this overridden from?

Thanks for catching. It turns out that ui::NetworkListDelegate and
ViewClickListener both have pure virtual functions OnViewClicked(views::View*
sender). In the next patch set I've renamed the former to avoid confusion.

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

https://codereview.chromium.org/2289223002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_details_view.cc:86: 
On 2016/08/31 20:28:14, varkha wrote:
> nit: no empty line.

Done.

[commit-bot: I haz the power, 2016-08-31 23:06:30]

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

[commit-bot: I haz the power, 2016-08-31 23:06:31]

Dry run: This issue passed the CQ dry run.

[varkha, 2016-08-31 23:18:13]

Code looks good, just one question.

https://codereview.chromium.org/2289223002/diff/40001/ash/common/system/tray/...
File ash/common/system/tray/tray_details_view.cc (right):

https://codereview.chromium.org/2289223002/diff/40001/ash/common/system/tray/...
ash/common/system/tray/tray_details_view.cc:90: ViewClickListener* listener) {
Is there a case when |listener| is not |this|? Do you need the extra parameter?

[tdanderson, 2016-09-01 14:38:40]

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

[commit-bot: I haz the power, 2016-09-01 14:38:55]

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

[tdanderson, 2016-09-01 14:39:11]

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

[tdanderson, 2016-09-01 14:39:12]

Valery, please take another look.

Oshima, can you please look at the one-line change in ui/ ?

https://codereview.chromium.org/2289223002/diff/40001/ash/common/system/tray/...
File ash/common/system/tray/tray_details_view.cc (right):

https://codereview.chromium.org/2289223002/diff/40001/ash/common/system/tray/...
ash/common/system/tray/tray_details_view.cc:90: ViewClickListener* listener) {
On 2016/08/31 23:18:12, varkha wrote:
> Is there a case when |listener| is not |this|? Do you need the extra
parameter?

No, this has been removed.

[varkha, 2016-09-01 14:45:21]

lgtm

[commit-bot: I haz the power, 2016-09-01 15:18:35]

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

[commit-bot: I haz the power, 2016-09-01 15:18:36]

Dry run: This issue passed the CQ dry run.

[oshima, 2016-09-01 18:00:58]

one q

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

https://codereview.chromium.org/2289223002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/tray_details_view.h:44: views::View* scroll_content()
const { return scroll_content_; }
ash uses the pattern where const method should return const internal object.
Does this have to be const (or can return value be const)?

[tdanderson, 2016-09-01 19:48:00]

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

https://codereview.chromium.org/2289223002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/tray_details_view.h:44: views::View* scroll_content()
const { return scroll_content_; }
On 2016/09/01 18:00:58, oshima wrote:
> ash uses the pattern where const method should return const internal object.
> Does this have to be const (or can return value be const)?

Sorry, I don't understand your question - It seems OK as-is (an accessor should
be prevented from mutating any members). Can you please clarify?

[oshima, 2016-09-01 20:23:17]

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

https://codereview.chromium.org/2289223002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/tray_details_view.h:44: views::View* scroll_content()
const { return scroll_content_; }
On 2016/09/01 19:48:00, tdanderson wrote:
> On 2016/09/01 18:00:58, oshima wrote:
> > ash uses the pattern where const method should return const internal object.
> > Does this have to be const (or can return value be const)?
> 
> Sorry, I don't understand your question - It seems OK as-is (an accessor
should
> be prevented from mutating any members). Can you please clarify?

const accessor shouldn't return non const internal object. Consider the
following:

class B {
   void modify() {
     value_ = 100;
   }
   int value_;
}

class A {
  void ModifyStateB() {
     b_->modify();
  }
  B* b() const {};

  B* b_;
};
  

method(const A* a) {
   a->ModifyStateB(); // won't compile.

   a->b()->modify();  // shouldn't work, but works.
}

[oshima, 2016-09-01 21:53:59]

lgtm

[tdanderson, 2016-09-01 21:56:36]

The CQ bit was checked by tdanderson@chromium.org

[tdanderson, 2016-09-01 21:56:37]

The patchset sent to the CQ was uploaded after l-g-t-m from varkha@chromium.org
Link to the patchset: https://codereview.chromium.org/2289223002/#ps80001
(title: "const accessors")

[commit-bot: I haz the power, 2016-09-01 21:57:05]

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, 2016-09-02 00:03:22]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-02 00:08:06]

Description was changed from

==========
Make TrayDetailsView a ViewClickListener and ButtonListener

Some initial changes in preparation for implementing
the material design version of the detailed views
in the Ash system menu:

* Make TrayDetailsView extend ViewClickListener
  and ButtonListener instead of having each of
  TrayDetailsView's subclasses extend these.
  The aim is to place common code in TrayDetailsView.

* Rename the TrayDetailsView "footer row" to be
  the "title row"; for material design this row
  appears at the top of the detailed view, not
  the bottom.

* For material design (--ash-md=experimental), show
  the title row at the top of the detailed view
  and do not instantiate or show any non-MD
  buttons in this row. Further improvements to
  the appearance and functionality of the title
  rows to be handled in subsequent CLs.

BUG=642136
TEST=ash_unittests
==========

to

==========
Make TrayDetailsView a ViewClickListener and ButtonListener

Some initial changes in preparation for implementing
the material design version of the detailed views
in the Ash system menu:

* Make TrayDetailsView extend ViewClickListener
  and ButtonListener instead of having each of
  TrayDetailsView's subclasses extend these.
  The aim is to place common code in TrayDetailsView.

* Rename the TrayDetailsView "footer row" to be
  the "title row"; for material design this row
  appears at the top of the detailed view, not
  the bottom.

* For material design (--ash-md=experimental), show
  the title row at the top of the detailed view
  and do not instantiate or show any non-MD
  buttons in this row. Further improvements to
  the appearance and functionality of the title
  rows to be handled in subsequent CLs.

BUG=642136
TEST=ash_unittests

Committed: https://crrev.com/bd26b635c2b1e5ad8287a4f87d87f5ce2c697267
Cr-Commit-Position: refs/heads/master@{#416134}
==========

[commit-bot: I haz the power, 2016-09-02 00:08:07]

Patchset 5 (id:??) landed as
https://crrev.com/bd26b635c2b1e5ad8287a4f87d87f5ce2c697267
Cr-Commit-Position: refs/heads/master@{#416134}

[findit-for-me, 2016-09-02 03:26:09]

FYI: Findit try jobs (rerunning failed compile or tests) identified this CL
at revision 416134 as the culprit for failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

[magjed_chromium, 2016-09-02 09:29:39]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2300353002/ by magjed@chromium.org.

The reason for reverting is: This CL breaks
TrayDetailsViewTest.HoverHighlightViewTouchFeedback on:
Linux ChromiumOS Tests (dbg)(1)
and
Linux Chromium OS ASan LSan Tests (1

https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2...
[ RUN      ] TrayDetailsViewTest.HoverHighlightViewTouchFeedback
Xlib:  extension "RANDR" missing on display ":9".
=================================================================
==5332==ERROR: AddressSanitizer: heap-use-after-free on address 0x616000324580
at pc 0x0000007871d6 bp 0x7ffe4d468250 sp 0x7ffe4d468248
READ of size 1 at 0x616000324580 thread T0
    #0 0x7871d5 in hover ash/common/system/tray/hover_highlight_view.h:89:31
    #1 0x7871d5 in
ash::test::TrayDetailsViewTest_HoverHighlightViewTouchFeedback_Test::TestBody()
ash/common/system/tray/tray_details_view_unittest.cc:205
    #2 0x24fe1bb in HandleExceptionsInMethodIfSupported<testing::Test, void>
testing/gtest/src/gtest.cc:2458:12
    #3 0x24fe1bb in testing::Test::Run() testing/gtest/src/gtest.cc:2474
    #4 0x250024b in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2656:11
    #5 0x2501006 in testing::TestCase::Run() testing/gtest/src/gtest.cc:2774:28
    #6 0x2515006 in testing::internal::UnitTestImpl::RunAllTests()
testing/gtest/src/gtest.cc:4647:43
    #7 0x2514667 in
HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>
testing/gtest/src/gtest.cc:2458:12
    #8 0x2514667 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:4255
    #9 0x1b28b8b in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2237:46
    #10 0x1b28b8b in base::TestSuite::Run() base/test/test_suite.cc:246
    #11 0x1b2c5b4 in Run base/callback.h:388:12
    #12 0x1b2c5b4 in base::(anonymous
namespace)::LaunchUnitTestsInternal(base::Callback<int (),
(base::internal::CopyMode)1> const&, int, int, bool, base::Callback<void (),
(base::internal::CopyMode)1> const&)
base/test/launcher/unit_test_launcher.cc:206
    #13 0x1b2c26e in base::LaunchUnitTests(int, char**, base::Callback<int (),
(base::internal::CopyMode)1> const&)
base/test/launcher/unit_test_launcher.cc:445:10
    #14 0xcab0de in main ash/test/ash_unittests.cc:14:10
    #15 0x7fe83864e7ec in __libc_start_main
/build/eglibc-oqps9y/eglibc-2.15/csu/libc-start.c:226.