Add a row in the network tray to inform users to turn Bluetooth on to enable Tether.

ash/system/tray/tray_details_view.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef ASH_SYSTEM_TRAY_TRAY_DETAILS_VIEW_H_
 #define ASH_SYSTEM_TRAY_TRAY_DETAILS_VIEW_H_
 
 #include <memory>
 
 #include "ash/ash_export.h"
+#include "ash/system/tray/actionable_view.h"
 #include "ash/system/tray/tray_constants.h"
 #include "ash/system/tray/view_click_listener.h"
 #include "base/macros.h"
 #include "base/timer/timer.h"
 #include "ui/views/controls/button/button.h"
+#include "ui/views/controls/button/label_button.h"
 #include "ui/views/view.h"
 
 namespace gfx {
 struct VectorIcon;
 }  // namespace gfx
 
 namespace views {
 class BoxLayout;
 class CustomButton;
 class Label;
@@ skipping 21 matching lines @@
   // ViewClickListener:
   // Don't override this --- override HandleViewClicked.
   void OnViewClicked(views::View* sender) final;
 
   // views::ButtonListener:
   // Don't override this --- override HandleButtonPressed.
   void ButtonPressed(views::Button* sender, const ui::Event& event) final;
 
   SystemTrayItem* owner() { return owner_; }
 
- protected:
+  class InfoLabelDelegate;

[Kyle Horimoto, 2017/07/10 18:37:08]

We should still keep InfoLabel protected. There shouldn't be any need for other
classes to be aware of the implementation details of TrayDetailsView.

To rectify this:
(1) Keep InfoLabelDelegate public, but change its name (since the caller
shouldn't have to be aware of what an InfoLabel is). How about
TrayDetailClickHandler?
(2) Currently, OnLabelClicked() refers to a label as well. How about
OnTrayDetailClicked()?
(3) You pass an InfoLabel as a parameter to the delegate function, but the
client shouldn't need to be aware of the implementation of the label. Instead,
just pass the message ID of the item that was clicked. In your NetworkList
implementation, the only thing you use the InfoLabel for is to fetch its message
ID, so that should be sufficient.

[James Cook, 2017/07/11 23:14:15]

I chatted with Leslie over IM. I suggest a different approach, which is more
similar to some of our other system tray code.

tray_details_view.cc is pretty long (>500 lines) and this CL makes it longer. I
would prefer to pull InfoLabel out into its own file. I would call it
TrayInfoLabel or similar and document that it is related to TrayDetailsView.
Since it would become public it would be easier to add tests for it.

I would do the delegate like:

class TrayInfoLabel : public <stuff> {
 public:
  class Delegate {
    ...stuff...
  }
  TrayInfoLabel(Delegate* delegate, ...);
}

[lesliewatkins, 2017/07/12 21:49:50]

Done.

+
   // A view containing only a label, which is to be inserted as a non-targetable
   // row within a system menu detailed view (e.g., the "Scanning for devices..."
   // message that can appear at the top of the Bluetooth detailed view).

[Kyle Horimoto, 2017/07/10 18:37:08]

nit: Now that this is clickable, add some description to the class regarding the
click handler delegate.

[lesliewatkins, 2017/07/12 21:49:50]

Done.

-  class InfoLabel : public View {
+  class InfoLabel : public ActionableView {
    public:
-    explicit InfoLabel(int message_id);
-    ~InfoLabel() override;

[Kyle Horimoto, 2017/07/10 18:37:08]

Don't remove the destructor.

+    explicit InfoLabel(int message_id, InfoLabelDelegate* delegate);

[James Cook, 2017/07/10 18:43:03]

no explicit

nit: Usually the delegate* is the first parameter.

Document that delegate can be null and what happens in that case.

Class still needs virtual destructor

[lesliewatkins, 2017/07/12 21:49:50]

Done.

+    void Update(int message_id);

[Kyle Horimoto, 2017/07/10 18:37:08]

nit: Previously, SetMessage() was pretty obvious in what it did (due to its
name), so a description wasn't necessary. Now, Update() is a bit more vaguer, so
please add a description.

[lesliewatkins, 2017/07/12 21:49:50]

Done.

+    const int& message_id();

[Kyle Horimoto, 2017/07/10 18:37:08]

You can get rid of this function after my explanation earlier.

[James Cook, 2017/07/10 18:43:03]

just return the int, there's no perf win for returning a reference to it. Also,
you can inline the code in the header. And the method should be const.

[lesliewatkins, 2017/07/12 21:49:50]

Acknowledged.

[lesliewatkins, 2017/07/12 21:49:50]

Done.

 
-    void SetMessage(int message_id);
+    // ActionableView:
+    bool PerformAction(const ui::Event& event) override;
+    void GetAccessibleNodeData(ui::AXNodeData* node_data) override;
 
    private:
+    bool IsClickable();
+
     views::Label* const label_;
+    int message_id_;
+
+    InfoLabelDelegate* delegate_;
 
     DISALLOW_COPY_AND_ASSIGN(InfoLabel);
   };
 
+  // A delegate for handling actions when an InfoLabel is clicked.
+  class InfoLabelDelegate {

[James Cook, 2017/07/10 18:43:03]

optional: You could also declare this as an inner class of InfoLabel.

[lesliewatkins, 2017/07/12 21:49:50]

Acknowledged.

+   public:
+    virtual void OnLabelClicked(InfoLabel* label) = 0;
+  };

[James Cook, 2017/07/10 18:43:03]

needs virtual destructor

[lesliewatkins, 2017/07/12 21:49:50]

Done.

+
+ protected:
   // views::View:
   void Layout() override;
   int GetHeightForWidth(int width) const override;
 
   // Exposes the layout manager of this view to give control to subclasses.
   views::BoxLayout* box_layout() { return box_layout_; }
 
   // Creates the row containing the back button and title. For material design
   // this appears at the top of the view, for non-material design it appears
   // at the bottom.
@@ skipping 95 matching lines @@
 
   // Used to delay the transition to the default view.
   base::OneShotTimer transition_delay_timer_;
 
   DISALLOW_COPY_AND_ASSIGN(TrayDetailsView);
 };
 
 }  // namespace ash
 
 #endif  // ASH_SYSTEM_TRAY_TRAY_DETAILS_VIEW_H_