[SendTabToSelf] Polishing code

Fix nits to make code clearer.

Bug: 980277
Change-Id: I08c6f04fba7e78482f01fbd5ed5f6a0b1eb09a4d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1706556Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Tina Wang <tinazwang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#680108}

chrome/browser/renderer_context_menu/render_view_context_menu.cc

@@ -1256,19 +1256,18 @@ void RenderViewContextMenu::AppendLinkItems() {
       menu_model_.AddSeparator(ui::NORMAL_SEPARATOR);
       if (send_tab_to_self::GetValidDeviceCount(GetBrowser()->profile()) == 1) {
 #if defined(OS_MACOSX)
-        menu_model_.AddItem(
-            IDC_CONTENT_LINK_SEND_TAB_TO_SELF_SINGLE_TARGET,
-            l10n_util::GetStringFUTF16(
-                IDS_LINK_MENU_SEND_TAB_TO_SELF_SINGLE_TARGET,
-                base::UTF8ToUTF16(send_tab_to_self::GetSingleTargetDeviceName(
-                    GetBrowser()->profile()))));
+        menu_model_.AddItem(IDC_CONTENT_LINK_SEND_TAB_TO_SELF_SINGLE_TARGET,
+                            l10n_util::GetStringFUTF16(
+                                IDS_LINK_MENU_SEND_TAB_TO_SELF_SINGLE_TARGET,
+                                send_tab_to_self::GetSingleTargetDeviceName(
+                                    GetBrowser()->profile())));
 #else
         menu_model_.AddItemWithIcon(
             IDC_CONTENT_LINK_SEND_TAB_TO_SELF_SINGLE_TARGET,
             l10n_util::GetStringFUTF16(
                 IDS_LINK_MENU_SEND_TAB_TO_SELF_SINGLE_TARGET,
-                base::UTF8ToUTF16(send_tab_to_self::GetSingleTargetDeviceName(
-                    GetBrowser()->profile()))),
+                send_tab_to_self::GetSingleTargetDeviceName(
+                    GetBrowser()->profile())),
             *send_tab_to_self::GetImageSkia());
 #endif
         send_tab_to_self::RecordSendTabToSelfClickResult(
@@ -1491,19 +1490,18 @@ void RenderViewContextMenu::AppendPageItems() {
     menu_model_.AddSeparator(ui::NORMAL_SEPARATOR);
     if (send_tab_to_self::GetValidDeviceCount(GetBrowser()->profile()) == 1) {
 #if defined(OS_MACOSX)
-      menu_model_.AddItem(
-          IDC_SEND_TAB_TO_SELF_SINGLE_TARGET,
-          l10n_util::GetStringFUTF16(
-              IDS_CONTEXT_MENU_SEND_TAB_TO_SELF_SINGLE_TARGET,
-              base::UTF8ToUTF16(send_tab_to_self::GetSingleTargetDeviceName(
-                  GetBrowser()->profile()))));
+      menu_model_.AddItem(IDC_SEND_TAB_TO_SELF_SINGLE_TARGET,
+                          l10n_util::GetStringFUTF16(
+                              IDS_CONTEXT_MENU_SEND_TAB_TO_SELF_SINGLE_TARGET,
+                              send_tab_to_self::GetSingleTargetDeviceName(
+                                  GetBrowser()->profile())));
 #else
       menu_model_.AddItemWithIcon(
           IDC_SEND_TAB_TO_SELF_SINGLE_TARGET,
           l10n_util::GetStringFUTF16(
               IDS_CONTEXT_MENU_SEND_TAB_TO_SELF_SINGLE_TARGET,
-              base::UTF8ToUTF16(send_tab_to_self::GetSingleTargetDeviceName(
-                  GetBrowser()->profile()))),
+              send_tab_to_self::GetSingleTargetDeviceName(
+                  GetBrowser()->profile())),
           *send_tab_to_self::GetImageSkia());
 #endif
       send_tab_to_self::RecordSendTabToSelfClickResult(

chrome/browser/renderer_context_menu/render_view_context_menu.h

@@ -71,8 +71,6 @@ class RenderViewContextMenu : public RenderViewContextMenuBase {
                                        bool is_checked);
 
   // Range of command IDs to use for the items in the send tab to self submenu.
-  static const int kMinSendTabToSelfSubMenuCommandId =
-      send_tab_to_self::SendTabToSelfSubMenuModel::kMinCommandId;
   static const int kMaxSendTabToSelfSubMenuCommandId =
       send_tab_to_self::SendTabToSelfSubMenuModel::kMaxCommandId;
 

chrome/browser/send_tab_to_self/send_tab_to_self_desktop_util.cc

@@ -64,8 +64,8 @@ void CreateNewEntry(content::WebContents* tab,
 
 void ShareToSingleTarget(content::WebContents* tab, const GURL& link_url) {
   Profile* profile = Profile::FromBrowserContext(tab->GetBrowserContext());
-  DCHECK(GetValidDeviceCount(profile) == 1);
-  std::vector<TargetDeviceInfo> devices =
+  DCHECK_EQ(GetValidDeviceCount(profile), 1u);
+  const std::vector<TargetDeviceInfo>& devices =
       SendTabToSelfSyncServiceFactory::GetForProfile(profile)
           ->GetSendTabToSelfModel()
           ->GetTargetDeviceInfoSortedList();
@@ -94,24 +94,25 @@ void RecordSendTabToSelfDeviceCount(const std::string& entry_point,
                               device_count);
 }
 
-int GetValidDeviceCount(Profile* profile) {
+size_t GetValidDeviceCount(Profile* profile) {
   SendTabToSelfSyncService* service =
       SendTabToSelfSyncServiceFactory::GetForProfile(profile);
   DCHECK(service);
   SendTabToSelfModel* model = service->GetSendTabToSelfModel();
   DCHECK(model);
-  std::vector<TargetDeviceInfo> devices =
+  const std::vector<TargetDeviceInfo>& devices =
       model->GetTargetDeviceInfoSortedList();
   return devices.size();
 }
 
-std::string GetSingleTargetDeviceName(Profile* profile) {
-  DCHECK(GetValidDeviceCount(profile) == 1);
-  return SendTabToSelfSyncServiceFactory::GetForProfile(profile)
-      ->GetSendTabToSelfModel()
-      ->GetTargetDeviceInfoSortedList()
-      .begin()
-      ->device_name;
+base::string16 GetSingleTargetDeviceName(Profile* profile) {
+  DCHECK_EQ(GetValidDeviceCount(profile), 1u);
+  return base::UTF8ToUTF16(
+      SendTabToSelfSyncServiceFactory::GetForProfile(profile)
+          ->GetSendTabToSelfModel()
+          ->GetTargetDeviceInfoSortedList()
+          .begin()
+          ->device_name);
 }
 
 }  // namespace send_tab_to_self

chrome/browser/send_tab_to_self/send_tab_to_self_desktop_util.h

@@ -6,6 +6,8 @@
 #define CHROME_BROWSER_SEND_TAB_TO_SELF_SEND_TAB_TO_SELF_DESKTOP_UTIL_H_
 
 #include <string>
+
+#include "base/strings/string16.h"
 #include "url/gurl.h"
 
 class GURL;
@@ -63,11 +65,11 @@ void RecordSendTabToSelfDeviceCount(const std::string& entry_point,
                                     const int& device_count);
 
 // Gets the count of valid device number.
-int GetValidDeviceCount(Profile* profile);
+size_t GetValidDeviceCount(Profile* profile);
 
 // Gets the name of the single valid device. Will be called when
 // GetValidDeviceCount() == 1.
-std::string GetSingleTargetDeviceName(Profile* profile);
+base::string16 GetSingleTargetDeviceName(Profile* profile);
 
 }  // namespace send_tab_to_self
 

chrome/browser/ui/send_tab_to_self/send_tab_to_self_bubble_controller.cc

@@ -62,8 +62,8 @@ base::string16 SendTabToSelfBubbleController::GetWindowTitle() const {
   return l10n_util::GetStringUTF16(IDS_CONTEXT_MENU_SEND_TAB_TO_SELF);
 }
 
-std::vector<TargetDeviceInfo> SendTabToSelfBubbleController::GetValidDevices()
-    const {
+const std::vector<TargetDeviceInfo>&
+SendTabToSelfBubbleController::GetValidDevices() const {
   return valid_devices_;
 }
 
@@ -91,7 +91,7 @@ SendTabToSelfBubbleController::SendTabToSelfBubbleController() = default;
 SendTabToSelfBubbleController::SendTabToSelfBubbleController(
     content::WebContents* web_contents)
     : web_contents_(web_contents) {
-  this->FetchDeviceInfo();
+  FetchDeviceInfo();
 }
 
 void SendTabToSelfBubbleController::FetchDeviceInfo() {

chrome/browser/ui/send_tab_to_self/send_tab_to_self_bubble_controller.h

@@ -40,7 +40,7 @@ class SendTabToSelfBubbleController
   // Returns the title of send tab to self bubble.
   base::string16 GetWindowTitle() const;
   // Returns the valid devices info map.
-  std::vector<TargetDeviceInfo> GetValidDevices() const;
+  const std::vector<TargetDeviceInfo>& GetValidDevices() const;
   // Returns current profile.
   Profile* GetProfile() const;
 

chrome/browser/ui/send_tab_to_self/send_tab_to_self_bubble_view.h

@@ -11,6 +11,8 @@ namespace send_tab_to_self {
 // This object is responsible for its own lifetime.
 class SendTabToSelfBubbleView {
  public:
+  virtual ~SendTabToSelfBubbleView() = default;
+
   // Called to close the bubble and prevent future callbacks into the
   // controller.
   virtual void Hide() = 0;

chrome/browser/ui/send_tab_to_self/send_tab_to_self_sub_menu_model.cc

@@ -41,7 +41,7 @@ static_assert(
 
 // Returns true if the command id identifies a non-link contextual menu item.
 bool IsShareTabCommandId(int command_id) {
-  return (command_id >= kShareTabCommandId && command_id < kShareLinkCommandId);
+  return command_id >= kShareTabCommandId && command_id < kShareLinkCommandId;
 }
 
 // Returns true if the command id identifies a link contextual menu item.
@@ -53,7 +53,8 @@ bool IsShareLinkCommandId(int command_id) {
 int CommandIdToVectorIndex(int command_id) {
   if (IsShareTabCommandId(command_id)) {
     return command_id - kShareTabCommandId;
-  } else if (IsShareLinkCommandId(command_id)) {
+  }
+  if (IsShareLinkCommandId(command_id)) {
     return command_id - kShareLinkCommandId;
   }
   return -1;
@@ -85,6 +86,11 @@ struct SendTabToSelfSubMenuModel::ValidDeviceItem {
   std::string cache_guid;
 };
 
+SendTabToSelfSubMenuModel::SendTabToSelfSubMenuModel(
+    content::WebContents* tab,
+    SendTabToSelfMenuType menu_type)
+    : SendTabToSelfSubMenuModel(tab, menu_type, GURL()) {}
+
 SendTabToSelfSubMenuModel::SendTabToSelfSubMenuModel(
     content::WebContents* tab,
     SendTabToSelfMenuType menu_type,
@@ -155,14 +161,8 @@ void SendTabToSelfSubMenuModel::BuildDeviceItem(const std::string& device_name,
                                                 const std::string& cache_guid,
                                                 int index) {
   ValidDeviceItem item(device_name, cache_guid);
-  int command_id;
-  if (menu_type_ == kLink) {
-    // Generates command ids for sharing a link.
-    command_id = index + kShareLinkCommandId;
-  } else {
-    // Generates command ids for sharing a tab.
-    command_id = index + kShareTabCommandId;
-  }
+  int command_id =
+      (menu_type_ == kTab) ? kShareTabCommandId : kShareLinkCommandId + index;
   InsertItemAt(index, command_id, base::UTF8ToUTF16(device_name));
   valid_device_items_.push_back(item);
 }

chrome/browser/ui/send_tab_to_self/send_tab_to_self_sub_menu_model.h

@@ -32,9 +32,11 @@ class SendTabToSelfSubMenuModel : public ui::SimpleMenuModel,
 
   struct ValidDeviceItem;
 
+  SendTabToSelfSubMenuModel(content::WebContents* tab,
+                            SendTabToSelfMenuType menu_type);
   SendTabToSelfSubMenuModel(content::WebContents* tab,
                             SendTabToSelfMenuType menu_type,
-                            const GURL& link_url = GURL());
+                            const GURL& link_url);
   ~SendTabToSelfSubMenuModel() override;
 
   // Overridden from ui::SimpleMenuModel::Delegate:

chrome/browser/ui/tabs/tab_menu_model.cc

@@ -6,7 +6,6 @@
 
 #include "base/command_line.h"
 #include "base/metrics/user_metrics.h"
-#include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/send_tab_to_self/send_tab_to_self_desktop_util.h"
@@ -94,16 +93,15 @@ void TabMenuModel::Build(TabStripModel* tab_strip, int index) {
       AddItem(TabStripModel::CommandSendTabToSelfSingleTarget,
               l10n_util::GetStringFUTF16(
                   IDS_CONTEXT_MENU_SEND_TAB_TO_SELF_SINGLE_TARGET,
-                  base::UTF8ToUTF16(send_tab_to_self::GetSingleTargetDeviceName(
-                      tab_strip->profile()))));
+                  send_tab_to_self::GetSingleTargetDeviceName(
+                      tab_strip->profile())));
 #else
-      AddItemWithIcon(
-          TabStripModel::CommandSendTabToSelfSingleTarget,
-          l10n_util::GetStringFUTF16(
-              IDS_CONTEXT_MENU_SEND_TAB_TO_SELF_SINGLE_TARGET,
-              base::UTF8ToUTF16(send_tab_to_self::GetSingleTargetDeviceName(
-                  tab_strip->profile()))),
-          *send_tab_to_self::GetImageSkia());
+      AddItemWithIcon(TabStripModel::CommandSendTabToSelfSingleTarget,
+                      l10n_util::GetStringFUTF16(
+                          IDS_CONTEXT_MENU_SEND_TAB_TO_SELF_SINGLE_TARGET,
+                          (send_tab_to_self::GetSingleTargetDeviceName(
+                              tab_strip->profile()))),
+                      *send_tab_to_self::GetImageSkia());
 #endif
       send_tab_to_self::RecordSendTabToSelfClickResult(
           send_tab_to_self::kTabMenu,

chrome/browser/ui/views/location_bar/location_bar_view.cc

@@ -1041,10 +1041,7 @@ inline bool LocationBarView::UpdateSendTabToSelfIcon() {
   PageActionIconView* icon =
       this->omnibox_page_action_icon_container_view()->GetPageActionIconView(
           PageActionIconType::kSendTabToSelf);
-  if (icon) {
-    return icon->Update();
-  }
-  return false;
+  return icon && icon->Update();
 }
 
 void LocationBarView::SaveStateToContents(WebContents* contents) {

chrome/browser/ui/views/omnibox/omnibox_view_views.cc

@@ -12,7 +12,6 @@
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_util.h"
-#include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/command_updater.h"
@@ -1773,8 +1772,8 @@ void OmniboxViewViews::UpdateContextMenu(ui::SimpleMenuModel* menu_contents) {
           index, IDC_SEND_TAB_TO_SELF_SINGLE_TARGET,
           l10n_util::GetStringFUTF16(
               IDS_CONTEXT_MENU_SEND_TAB_TO_SELF_SINGLE_TARGET,
-              base::UTF8ToUTF16(send_tab_to_self::GetSingleTargetDeviceName(
-                  location_bar_view_->profile()))));
+              send_tab_to_self::GetSingleTargetDeviceName(
+                  location_bar_view_->profile())));
       send_tab_to_self::RecordSendTabToSelfClickResult(
           send_tab_to_self::kOmniboxMenu,
           SendTabToSelfClickResult::kShowDeviceList);

chrome/browser/ui/views/send_tab_to_self/send_tab_to_self_bubble_device_button.cc

@@ -6,6 +6,7 @@
 
 #include <string>
 
+#include "base/strings/string16.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "chrome/app/vector_icons/vector_icons.h"

chrome/browser/ui/views/send_tab_to_self/send_tab_to_self_bubble_device_button.h

@@ -24,8 +24,8 @@ class SendTabToSelfBubbleDeviceButton : public HoverButton {
                                   int button_tag);
   ~SendTabToSelfBubbleDeviceButton() override;
 
-  const std::string device_name() const { return device_name_; }
-  const std::string device_guid() const { return device_guid_; }
+  const std::string& device_name() const { return device_name_; }
+  const std::string& device_guid() const { return device_guid_; }
   sync_pb::SyncEnums::DeviceType device_type() const { return device_type_; }
 
  private:

chrome/browser/ui/views/send_tab_to_self/send_tab_to_self_bubble_view_impl.cc

@@ -25,6 +25,17 @@
 
 namespace send_tab_to_self {
 
+namespace {
+
+// The valid device button height.
+constexpr int kDeviceButtonHeight = 56;
+// Maximum number of buttons that are shown without scroll. If the device
+// number is larger than kMaximumButtons, the bubble content will be
+// scrollable.
+constexpr int kMaximumButtons = 5;
+
+}  // namespace
+
 SendTabToSelfBubbleViewImpl::SendTabToSelfBubbleViewImpl(
     views::View* anchor_view,
     const gfx::Point& anchor_point,
@@ -71,10 +82,7 @@ bool SendTabToSelfBubbleViewImpl::Close() {
 
 void SendTabToSelfBubbleViewImpl::ButtonPressed(views::Button* sender,
                                                 const ui::Event& event) {
-  base::PostTaskWithTraits(
-      FROM_HERE, {content::BrowserThread::UI},
-      base::BindOnce(&SendTabToSelfBubbleViewImpl::DevicePressed,
-                     weak_factory_.GetWeakPtr(), sender->tag()));
+  DevicePressed(sender->tag());
 }
 
 gfx::Size SendTabToSelfBubbleViewImpl::CalculatePreferredSize() const {
@@ -129,7 +137,7 @@ void SendTabToSelfBubbleViewImpl::CreateScrollView() {
 }
 
 void SendTabToSelfBubbleViewImpl::PopulateScrollView(
-    const std::vector<TargetDeviceInfo> devices) {
+    const std::vector<TargetDeviceInfo>& devices) {
   device_buttons_.clear();
   auto device_list_view = std::make_unique<views::View>();
   device_list_view->SetLayoutManager(std::make_unique<views::BoxLayout>(
@@ -152,8 +160,7 @@ void SendTabToSelfBubbleViewImpl::DevicePressed(size_t index) {
   if (!controller_) {
     return;
   }
-  SendTabToSelfBubbleDeviceButton* device_button =
-      device_buttons_.at(index).get();
+  SendTabToSelfBubbleDeviceButton* device_button = device_buttons_[index].get();
   controller_->OnDeviceSelected(device_button->device_name(),
                                 device_button->device_guid());
   Hide();

chrome/browser/ui/views/send_tab_to_self/send_tab_to_self_bubble_view_impl.h

@@ -5,7 +5,9 @@
 #ifndef CHROME_BROWSER_UI_VIEWS_SEND_TAB_TO_SELF_SEND_TAB_TO_SELF_BUBBLE_VIEW_IMPL_H_
 #define CHROME_BROWSER_UI_VIEWS_SEND_TAB_TO_SELF_SEND_TAB_TO_SELF_BUBBLE_VIEW_IMPL_H_
 
+#include <map>
 #include <memory>
+#include <string>
 #include <vector>
 
 #include "base/memory/weak_ptr.h"
@@ -21,7 +23,7 @@ class Canvas;
 
 namespace content {
 class WebContents;
-}
+}  // namespace content
 
 namespace send_tab_to_self {
 
@@ -35,13 +37,6 @@ class SendTabToSelfBubbleViewImpl : public SendTabToSelfBubbleView,
                                     public views::ButtonListener,
                                     public LocationBarBubbleDelegateView {
  public:
-  // The valid device button height.
-  static constexpr int kDeviceButtonHeight = 56;
-  // Maximum number of buttons that are shown without scroll. If the device
-  // number is larger than kMaximumButtons, the bubble content will be
-  // scrollable.
-  static constexpr int kMaximumButtons = 5;
-
   // Bubble will be anchored to |anchor_view|.
   SendTabToSelfBubbleViewImpl(views::View* anchor_view,
                               const gfx::Point& anchor_point,
@@ -88,7 +83,7 @@ class SendTabToSelfBubbleViewImpl : public SendTabToSelfBubbleView,
   void CreateScrollView();
 
   // Populates the scroll view containing valid devices.
-  void PopulateScrollView(const std::vector<TargetDeviceInfo> devices);
+  void PopulateScrollView(const std::vector<TargetDeviceInfo>& devices);
 
   // Handles the action when a target device has been pressed.
   void DevicePressed(size_t index);