[CrOS PhoneHub] Use a vector BrowserTabsMetadata instead of optionals

* Remove the two optionals BrowserTabMetadata from BrowserTabsModel.
* Adds sorted vector of BrowserTabMetadata to BrowserTabsModel, from
  most recently visited to least recently visited browser tabs.
* Modifies chrome://multidevice-internals to accommodate.

https://screenshot.googleplex.com/ASLmNrodybKchY6
https://screenshot.googleplex.com/5CyAkSuWD5wXM9g

Bug: 1106937
Change-Id: I5bc898b12db304491cec8091c51a2bf7b1c2ad24
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2415537
Commit-Queue: Regan Hsu <hsuregan@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#808108}

chrome/browser/resources/chromeos/multidevice_internals/browser_tabs_metadata_form.html

@@ -4,6 +4,14 @@
     flex: 1 0 100%;
   }
 
+  :host(:not([is-valid])) {
+    background-color: LightGray;
+  }
+
+  :host([is-valid]) {
+    background-color: LightGreen;
+   }
+
   #faviconSelector {
     align-items: normal;
   }
@@ -11,12 +19,12 @@
 
 <div class="column">
   <cr-input value="{{url_}}" id="urlInput" auto-validate required label="URL: "
-      error-message="This browser tab will be a nullopt if no url input">
+      error-message="This row is invalid">
   </cr-input>
 </div>
 <div class="column">
   <cr-input value="{{title_}}" label="Title:" auto-validate required
-      error-message="This browser tab will be a nullopt if no title input">
+      error-message="This row is invalid">
   </cr-input>
 </div>
 <div class="column">

chrome/browser/resources/chromeos/multidevice_internals/browser_tabs_metadata_form.js

@@ -20,7 +20,15 @@ Polymer({
     browserTabMetadata: {
       type: Object,
       notify: true,
-      computed: 'getMetadata_(url_, title_, lastAccessedTimeStamp_, favicon_)',
+      computed: 'getMetadata_(isValid, url_, title_, lastAccessedTimeStamp_, ' +
+          'favicon_)',
+    },
+
+    /** True if the fields are all filled out. */
+    isValid: {
+      type: Boolean,
+      computed: 'getIsValid_(url_, title_, lastAccessedTimeStamp_, favicon_)',
+      reflectToAttribute: true,
     },
 
     /** @private */
@@ -80,12 +88,22 @@ Polymer({
     this.favicon_ = this.faviconList_[select.selectedIndex];
   },
 
+  /**
+   * @return{boolean}
+   * @private
+   */
+  getIsValid_() {
+    return !!this.url_ && !!this.title_ && this.lastAccessedTimeStamp_ > -1 &&
+        this.favicon_ !== ImageType.NONE;
+  },
+
   /**
    * @return{BrowserTabsMetadataModel}
    * @private
    */
   getMetadata_() {
     return {
+      isValid: this.isValid,
       url: this.url_,
       title: this.title_,
       lastAccessedTimeStamp: this.lastAccessedTimeStamp_,

chrome/browser/resources/chromeos/multidevice_internals/browser_tabs_model_form.html

 <style include="cr-shared-style shared-style">
   :host {
     display: flex;
-    height: 250px;
+    height: 500px;
   }
 
   #fieldColumn {
     flex: 2;
   }
+
+  .updates {
+    font-size: 15px;
+  }
 </style>
 
 <div class="column">
+  <div class="emphasize updates" hidden="[[!isTabSyncEnabled_]]">
+    [[nValidTabs_]] valid metadatas
+  </div>
   <cr-button on-click="setFakeBrowserTabModel_" class="internals-button">
     <span class="emphasize">Change Browser Tabs Status</span>
   </cr-button>
   <div class="label">
-    <span class="emphasize">Note:</span> Click the button above to propagate all
-    browser tab status values on the right hand side to the fake phonehub
-    manager. Note that if a field is empty, then that corresponding tab will
-    be nullopt.
+    <span class="emphasize">Note:</span> Click the button above to propagate
+    <span class="emphasize">VALID (green)</span> browser tab status values on
+    the right hand side to the fake phonehub manager. Note that if a field is
+    empty or favicon is set to NONE, then that corresponding tab will not be
+    sent, and the background will be grey.
   </div>
 </div>
 <div class="column" id="fieldColumn">
@@ -39,6 +47,18 @@
           browser-tab-metadata="{{browserTabTwoMetadata_}}">
       </browser-tabs-metadata-form>
     </div>
+    <div class="cr-row">
+      <div class="column">3: </div>
+      <browser-tabs-metadata-form
+          browser-tab-metadata="{{browserTabThreeMetadata_}}">
+      </browser-tabs-metadata-form>
+    </div>
+    <div class="cr-row">
+      <div class="column">4: </div>
+      <browser-tabs-metadata-form
+          browser-tab-metadata="{{browserTabFourMetadata_}}">
+      </browser-tabs-metadata-form>
+    </div>
   </template>
 </div>
 

chrome/browser/resources/chromeos/multidevice_internals/browser_tabs_model_form.js

@@ -33,6 +33,25 @@ Polymer({
     browserTabTwoMetadata_: {
       type: Object,
     },
+
+    /** @private{BrowserTabsMetadataModel} */
+    browserTabThreeMetadata_: {
+      type: Object,
+    },
+
+    /** @private{BrowserTabsMetadataModel} */
+    browserTabFourMetadata_: {
+      type: Object,
+    },
+
+    /** @type{number} */
+    nValidTabs_: {
+      type: Number,
+      computed:
+          'computeNValidTabs_(isTabSyncEnabled_, browserTabOneMetadata_, ' +
+          'browserTabTwoMetadata_, browserTabThreeMetadata_, ' +
+          'browserTabFourMetadata_)',
+    },
   },
 
   /** @private{?MultidevicePhoneHubBrowserProxy}*/
@@ -43,14 +62,51 @@ Polymer({
     this.browserProxy_ = MultidevicePhoneHubBrowserProxy.getInstance();
   },
 
+  /**
+   * @return {Array<!BrowserTabsMetadataModel>}
+   * @private
+   */
+  getAllBrowserTabMetadatas_() {
+    return [
+      this.browserTabOneMetadata_, this.browserTabTwoMetadata_,
+      this.browserTabThreeMetadata_, this.browserTabFourMetadata_
+    ];
+  },
+
+  /**
+   * @return {number}
+   * @private
+   */
+  computeNValidTabs_() {
+    if (!this.isTabSyncEnabled_) {
+      return 0;
+    }
+
+    return this.getAllBrowserTabMetadatas_().reduce((acc, metadata) => {
+      return acc + (!!metadata && metadata.isValid);
+    }, 0);
+  },
+
   /** @private */
   setFakeBrowserTabModel_() {
+    if (!this.isTabSyncEnabled_) {
+      const syncDisabledBrowserTabsModel = {
+        isTabSyncEnabled: false,
+        browserTabOneMetadata: null,
+        browserTabTwoMetadata: null,
+        browserTabThreeMetadata: null,
+        browserTabFourMetadata: null,
+      };
+      this.browserProxy_.setBrowserTabs(syncDisabledBrowserTabsModel);
+      return;
+    }
+
     const browserTabsModel = {
       isTabSyncEnabled: this.isTabSyncEnabled_,
-      browserTabOneMetadata:
-          this.isTabSyncEnabled_ ? this.browserTabOneMetadata_ : null,
-      browserTabTwoMetadata:
-          this.isTabSyncEnabled_ ? this.browserTabTwoMetadata_ : null,
+      browserTabOneMetadata: this.browserTabOneMetadata_,
+      browserTabTwoMetadata: this.browserTabTwoMetadata_,
+      browserTabThreeMetadata: this.browserTabThreeMetadata_,
+      browserTabFourMetadata: this.browserTabFourMetadata_,
     };
     this.browserProxy_.setBrowserTabs(browserTabsModel);
   },

chrome/browser/resources/chromeos/multidevice_internals/types.js

@@ -129,6 +129,7 @@ export let PhoneStatusModel;
 
 /**
  * @typedef {{
+ *   isValid: boolean,
  *   url: string,
  *   title: string,
  *   lastAccessedTimeStamp: number,
@@ -142,6 +143,8 @@ export let BrowserTabsMetadataModel;
  *   isTabSyncEnabled: boolean,
  *   browserTabOneMetadata: ?BrowserTabsMetadataModel,
  *   browserTabTwoMetadata: ?BrowserTabsMetadataModel,
+ *   browserTabThreeMetadata: ?BrowserTabsMetadataModel,
+ *   browserTabFourMetadata: ?BrowserTabsMetadataModel,
  * }}
  */
 export let BrowserTabsModel;

chrome/browser/ui/webui/chromeos/multidevice_internals/multidevice_internals_phone_hub_handler.cc

@@ -71,38 +71,46 @@ phonehub::Notification::AppMetadata DictToAppMetadata(
                                              icon);
 }
 
-base::Optional<phonehub::BrowserTabsModel::BrowserTabMetadata>
-DictToBrowserTabMetadataModel(
-    const base::DictionaryValue* browser_tab_metadata) {
+void TryAddingMetadata(
+    const std::string& key,
+    const base::DictionaryValue* browser_tab_status_dict,
+    std::vector<phonehub::BrowserTabsModel::BrowserTabMetadata>& metadatas) {
+  const base::DictionaryValue* browser_tab_metadata = nullptr;
+
+  if (!browser_tab_status_dict->GetDictionary(key, &browser_tab_metadata))
+    return;
+
   std::string url;
-  if (!browser_tab_metadata->GetString("url", &url) || url.empty()) {
-    return base::nullopt;
-  }
+  if (!browser_tab_metadata->GetString("url", &url) || url.empty())
+    return;
 
   base::string16 title;
-  if (!browser_tab_metadata->GetString("title", &title) || title.empty()) {
-    return base::nullopt;
-  }
+  if (!browser_tab_metadata->GetString("title", &title) || title.empty())
+    return;
 
   // JavaScript time stamps don't fit in int.
   double last_accessed_timestamp;
   if (!browser_tab_metadata->GetDouble("lastAccessedTimeStamp",
                                        &last_accessed_timestamp)) {
-    return base::nullopt;
+    return;
   }
 
   int favicon_image_type_as_int;
   if (!browser_tab_metadata->GetInteger("favicon",
-                                        &favicon_image_type_as_int)) {
-    return base::nullopt;
+                                        &favicon_image_type_as_int) ||
+      !favicon_image_type_as_int) {
+    return;
   }
 
   auto favicon_image_type = static_cast<ImageType>(favicon_image_type_as_int);
   gfx::Image favicon =
       gfx::Image::CreateFrom1xBitmap(ImageTypeToBitmap(favicon_image_type));
-  return phonehub::BrowserTabsModel::BrowserTabMetadata(
+
+  auto metadata = phonehub::BrowserTabsModel::BrowserTabMetadata(
       GURL(url), title, base::Time::FromJsTime(last_accessed_timestamp),
       favicon);
+
+  metadatas.push_back(metadata);
 }
 
 }  // namespace
@@ -300,34 +308,28 @@ void MultidevicePhoneHubHandler::HandleSetBrowserTabs(
     return;
   }
 
-  base::Optional<phonehub::BrowserTabsModel::BrowserTabMetadata> metadata_one;
-  const base::DictionaryValue* browser_tab_one_metadata = nullptr;
-  if (browser_tab_status_dict->GetDictionary("browserTabOneMetadata",
-                                             &browser_tab_one_metadata)) {
-    metadata_one = DictToBrowserTabMetadataModel(browser_tab_one_metadata);
-  }
-
-  base::Optional<phonehub::BrowserTabsModel::BrowserTabMetadata> metadata_two;
-  const base::DictionaryValue* browser_tab_two_metadata = nullptr;
-  if (browser_tab_status_dict->GetDictionary("browserTabTwoMetadata",
-                                             &browser_tab_two_metadata)) {
-    metadata_two = DictToBrowserTabMetadataModel(browser_tab_two_metadata);
-  }
-
-  // TODO(hsuregan): Add metadata_three and metadata_four.
-  std::vector<base::Optional<phonehub::BrowserTabsModel::BrowserTabMetadata>>
-      metadatas{{metadata_one, metadata_two}};
-  std::sort(metadatas.begin(), metadatas.end());
+  std::vector<phonehub::BrowserTabsModel::BrowserTabMetadata> metadatas;
+  TryAddingMetadata("browserTabOneMetadata", browser_tab_status_dict,
+                    metadatas);
+  TryAddingMetadata("browserTabTwoMetadata", browser_tab_status_dict,
+                    metadatas);
+  TryAddingMetadata("browserTabThreeMetadata", browser_tab_status_dict,
+                    metadatas);
+  TryAddingMetadata("browserTabFourMetadata", browser_tab_status_dict,
+                    metadatas);
 
   fake_phone_hub_manager_->mutable_phone_model()->SetBrowserTabsModel(
-      phonehub::BrowserTabsModel(is_tab_sync_enabled, metadatas[1],
-                                 metadatas[0]));
+      phonehub::BrowserTabsModel(is_tab_sync_enabled, metadatas));
 
-  if (metadatas[1].has_value())
-    PA_LOG(VERBOSE) << "Set most recent browser tab to" << *metadatas[1];
+  auto browser_tabs_model =
+      fake_phone_hub_manager_->mutable_phone_model()->browser_tabs_model();
+  CHECK(browser_tabs_model.has_value());
 
-  if (metadatas[0].has_value())
-    PA_LOG(VERBOSE) << "Set second most recent browser tab to" << *metadatas[0];
+  // Log the most recently visited browser tab (at index 0) last.
+  for (int i = metadatas.size() - 1; i > -1; --i) {
+    PA_LOG(VERBOSE) << "Set most recent browser tab number " << i
+                    << " to: " << browser_tabs_model->most_recent_tabs()[i];
+  }
 }
 
 void MultidevicePhoneHubHandler::HandleSetNotification(

chromeos/components/phonehub/browser_tabs_model.cc

@@ -9,6 +9,8 @@
 namespace chromeos {
 namespace phonehub {
 
+const size_t BrowserTabsModel::kMaxMostRecentTabs = 4;
+
 BrowserTabsModel::BrowserTabMetadata::BrowserTabMetadata(
     GURL url,
     const base::string16& title,
@@ -36,23 +38,31 @@ bool BrowserTabsModel::BrowserTabMetadata::operator!=(
 
 bool BrowserTabsModel::BrowserTabMetadata::operator<(
     const BrowserTabMetadata& other) const {
-  return std::tie(last_accessed_timestamp) <
+  // More recently visited tabs should come before earlier visited tabs.
+  return std::tie(last_accessed_timestamp) >
          std::tie(other.last_accessed_timestamp);
 }
 
 BrowserTabsModel::BrowserTabsModel(
     bool is_tab_sync_enabled,
-    const base::Optional<BrowserTabMetadata>& most_recent_tab,
-    const base::Optional<BrowserTabMetadata>& second_most_recent_tab)
+    const std::vector<BrowserTabMetadata>& most_recent_tabs)
     : is_tab_sync_enabled_(is_tab_sync_enabled),
-      most_recent_tab_(most_recent_tab),
-      second_most_recent_tab_(second_most_recent_tab) {
-  if (!is_tab_sync_enabled_ &&
-      (most_recent_tab_.has_value() || second_most_recent_tab_.has_value())) {
+      most_recent_tabs_(most_recent_tabs) {
+  if (!is_tab_sync_enabled_ && !most_recent_tabs_.empty()) {
     PA_LOG(WARNING) << "Tab sync is not enabled, but tab metadata was "
                     << "provided; clearing metadata.";
-    most_recent_tab_.reset();
-    second_most_recent_tab_.reset();
+    most_recent_tabs_.clear();
+    return;
+  }
+
+  std::sort(most_recent_tabs_.begin(), most_recent_tabs_.end());
+  if (most_recent_tabs_.size() > kMaxMostRecentTabs) {
+    PA_LOG(WARNING) << "More than the max number of browser tab metadatas were "
+                       "provided; truncating least recently visited browser "
+                       "tabs' metadatas.";
+    most_recent_tabs_.erase(most_recent_tabs_.begin() + kMaxMostRecentTabs,
+                            most_recent_tabs_.end());
+    return;
   }
 }
 
@@ -62,8 +72,7 @@ BrowserTabsModel::~BrowserTabsModel() = default;
 
 bool BrowserTabsModel::operator==(const BrowserTabsModel& other) const {
   return is_tab_sync_enabled_ == other.is_tab_sync_enabled_ &&
-         most_recent_tab_ == other.most_recent_tab_ &&
-         second_most_recent_tab_ == other.second_most_recent_tab_;
+         most_recent_tabs_ == other.most_recent_tabs_;
 }
 
 bool BrowserTabsModel::operator!=(const BrowserTabsModel& other) const {

chromeos/components/phonehub/browser_tabs_model.h

@@ -19,6 +19,8 @@ namespace phonehub {
 // Contains metadata about browser tabs that are open on the user's phone.
 class BrowserTabsModel {
  public:
+  static const size_t kMaxMostRecentTabs;
+
   struct BrowserTabMetadata {
     BrowserTabMetadata(GURL url,
                        const base::string16& title,
@@ -37,14 +39,11 @@ class BrowserTabsModel {
   };
 
   // |is_tab_sync_enabled| indicates whether the Chrome OS device is currently
-  // syncing tab metadata. If that parameter is false, the optional tab
-  // parameters should be null. If it is true, one or both of the parameters may
-  // still be null if the user does not have browser tabs open on their phone.
+  // syncing tab metadata. If that parameter is false, |most_recent_tabs_|
+  // should be empty. If it is true, |most_recent_tabs_| can contain up to four.
   BrowserTabsModel(
       bool is_tab_sync_enabled,
-      const base::Optional<BrowserTabMetadata>& most_recent_tab = base::nullopt,
-      const base::Optional<BrowserTabMetadata>& second_most_recent_tab =
-          base::nullopt);
+      const std::vector<BrowserTabMetadata>& most_recent_tabs = {});
   BrowserTabsModel(const BrowserTabsModel& other);
   ~BrowserTabsModel();
 
@@ -52,17 +51,16 @@ class BrowserTabsModel {
   bool operator!=(const BrowserTabsModel& other) const;
 
   bool is_tab_sync_enabled() const { return is_tab_sync_enabled_; }
-  const base::Optional<BrowserTabMetadata>& most_recent_tab() const {
-    return most_recent_tab_;
-  }
-  const base::Optional<BrowserTabMetadata>& second_most_recent_tab() const {
-    return second_most_recent_tab_;
+
+  const std::vector<BrowserTabMetadata>& most_recent_tabs() const {
+    return most_recent_tabs_;
   }
 
  private:
   bool is_tab_sync_enabled_;
-  base::Optional<BrowserTabMetadata> most_recent_tab_;
-  base::Optional<BrowserTabMetadata> second_most_recent_tab_;
+
+  // Sorted from most recently visited to least recently visited.
+  std::vector<BrowserTabMetadata> most_recent_tabs_;
 };
 
 std::ostream& operator<<(

chromeos/components/phonehub/browser_tabs_model_unittest.cc

@@ -11,17 +11,28 @@ namespace chromeos {
 namespace phonehub {
 
 TEST(BrowserTabsModelTest, Initialization) {
-  BrowserTabsModel success(/*is_tab_sync_enabled=*/true,
-                           CreateFakeBrowserTabMetadata());
+  std::vector<BrowserTabsModel::BrowserTabMetadata> most_recent_tabs(
+      {CreateFakeBrowserTabMetadata()});
+
+  BrowserTabsModel success(/*is_tab_sync_enabled=*/true, most_recent_tabs);
   EXPECT_TRUE(success.is_tab_sync_enabled());
-  EXPECT_EQ(CreateFakeBrowserTabMetadata(), *success.most_recent_tab());
-  EXPECT_FALSE(success.second_most_recent_tab().has_value());
+  EXPECT_EQ(most_recent_tabs, success.most_recent_tabs());
 
   // If tab metadata is provided by tab sync is disabled, the data should be
   // cleared.
   BrowserTabsModel invalid_metadata(/*is_tab_sync_enabled=*/false,
-                                    CreateFakeBrowserTabMetadata());
-  EXPECT_FALSE(invalid_metadata.most_recent_tab().has_value());
+                                    most_recent_tabs);
+  EXPECT_TRUE(invalid_metadata.most_recent_tabs().empty());
+
+  // If the number of metadata is provided exceeds |kMaxMostRecentTabs|, the
+  // metadata vector will be truncated to be of size |kMaxMostRecentTabs|.
+  const int exceed_max_offset = BrowserTabsModel::kMaxMostRecentTabs + 1;
+  std::vector<BrowserTabsModel::BrowserTabMetadata> most_recent_tabs_exceeded(
+      exceed_max_offset, CreateFakeBrowserTabMetadata());
+  BrowserTabsModel truncated_metadata(/*is_tab_sync_enabled=*/true,
+                                      most_recent_tabs_exceeded);
+  EXPECT_EQ(BrowserTabsModel::kMaxMostRecentTabs,
+            truncated_metadata.most_recent_tabs().size());
 }
 
 }  // namespace phonehub

chromeos/components/phonehub/phone_model_test_util.cc

@@ -33,26 +33,35 @@ const PhoneStatusModel& CreateFakePhoneStatusModel() {
 
 const char kFakeBrowserTabUrl1[] = "https://www.example.com/tab1";
 const char kFakeBrowserTabName1[] = "Tab 1";
+const base::Time kFakeBrowserTabLastAccessedTimestamp1 =
+    base::Time::FromDoubleT(4);
+
 const char kFakeBrowserTabUrl2[] = "https://www.example.com/tab2";
 const char kFakeBrowserTabName2[] = "Tab 2";
+const base::Time kFakeBrowserTabLastAccessedTimestamp2 =
+    base::Time::FromDoubleT(3);
 
 const BrowserTabsModel::BrowserTabMetadata& CreateFakeBrowserTabMetadata() {
   static const base::NoDestructor<BrowserTabsModel::BrowserTabMetadata>
-      fake_browser_tab_metadata{GURL(kFakeBrowserTabUrl1),
-                                base::UTF8ToUTF16(kFakeBrowserTabName1),
-                                base::Time(), gfx::Image()};
+      fake_browser_tab_metadata{
+          GURL(kFakeBrowserTabUrl1), base::UTF8ToUTF16(kFakeBrowserTabName1),
+          kFakeBrowserTabLastAccessedTimestamp1, gfx::Image()};
   return *fake_browser_tab_metadata;
 }
 
 const BrowserTabsModel& CreateFakeBrowserTabsModel() {
   static const base::NoDestructor<BrowserTabsModel::BrowserTabMetadata>
-      second_browser_tab_metadata{GURL(kFakeBrowserTabUrl2),
-                                  base::UTF8ToUTF16(kFakeBrowserTabName2),
-                                  base::Time(), gfx::Image()};
+      second_browser_tab_metadata{
+          GURL(kFakeBrowserTabUrl2), base::UTF8ToUTF16(kFakeBrowserTabName2),
+          kFakeBrowserTabLastAccessedTimestamp2, gfx::Image()};
+
+  static const base::NoDestructor<
+      std::vector<BrowserTabsModel::BrowserTabMetadata>>
+      most_recent_tabs(
+          {CreateFakeBrowserTabMetadata(), *second_browser_tab_metadata});
 
   static const base::NoDestructor<BrowserTabsModel> fake_browser_tabs_model{
-      /*is_tab_sync_enabled=*/true, CreateFakeBrowserTabMetadata(),
-      *second_browser_tab_metadata};
+      /*is_tab_sync_enabled=*/true, *most_recent_tabs};
 
   return *fake_browser_tabs_model;
 }

chromeos/components/phonehub/phone_model_test_util.h

@@ -25,8 +25,10 @@ const PhoneStatusModel& CreateFakePhoneStatusModel();
 // Fake data for browser tabs.
 extern const char kFakeBrowserTabUrl1[];
 extern const char kFakeBrowserTabName1[];
+extern const base::Time kFakeBrowserTabLastAccessedTimestamp1;
 extern const char kFakeBrowserTabUrl2[];
 extern const char kFakeBrowserTabName2[];
+extern const base::Time kFakeBrowserTabLastAccessedTimestamp2;
 
 // Creates fake browser tab data for use in tests.
 const BrowserTabsModel::BrowserTabMetadata& CreateFakeBrowserTabMetadata();