Include a page title in the Physical Web omnibox overflow item

components/omnibox/browser/physical_web_provider.cc

Index: components/omnibox/browser/physical_web_provider.cc
diff --git a/components/omnibox/browser/physical_web_provider.cc b/components/omnibox/browser/physical_web_provider.cc
index caac80699f9eb5b5d6fc59fc55ad2c0eea390ffb..e2db27946c5b028cf9b3cc39959078888a15cfd5 100644
--- a/components/omnibox/browser/physical_web_provider.cc
+++ b/components/omnibox/browser/physical_web_provider.cc
@@ -36,6 +36,10 @@ static const size_t kPhysicalWebMaxMatches = 1;
 // in the metadata list.
 static const int kPhysicalWebUrlBaseRelevance = 700;
 
+// The maximum length of the page title in the overflow item. Longer titles will

[Mark P, 2016/09/08 18:49:50]

nit: consider:
"page title in the overflow item"
->
"page title's part of the overflow item's description"

[mattreynolds, 2016/09/09 21:24:54]

Done.

+// be truncated to this length.
+static const size_t kMaxTitleLength = 15;

[Mark P, 2016/09/08 18:49:49]

Where did you get this number from?  Calculations of the length of the rest of
the overflow item contents (as translated into a set of common languages)
combined with typical phone screen sizes?

(I don't expect that much, but I would like to make sure you thought about how
this number works internationally and across devices.)

[mattreynolds, 2016/09/09 21:24:54]

I found the longest string of "m"s that would fit on a simulated iPhone SE in
portrait.  I didn't try other languages.

[Mark P, 2016/09/13 23:34:42]

Is this the number of "m"s that would fit, or the number of "m" that would fit
once you add in the rest of the overflow item's text ("and 1 more web page" for
example)?

[mattreynolds, 2016/09/14 19:04:41]

It's the number of "m"s that would fit once the rest of the text is added.

+
 }
 
 // static
@@ -122,7 +126,7 @@ void PhysicalWebProvider::ConstructMatches(base::ListValue* metadata_list) {
     const size_t remaining_slots = kPhysicalWebMaxMatches - used_slots;
     const size_t remaining_metadata = metadata_count - i;
     if ((remaining_slots == 1) && (remaining_metadata > remaining_slots)) {
-      AppendOverflowItem(remaining_metadata, relevance);
+      AppendOverflowItem(remaining_metadata, relevance, title_string);
       return;
     }
 
@@ -155,7 +159,8 @@ void PhysicalWebProvider::ConstructMatches(base::ListValue* metadata_list) {
 }
 
 void PhysicalWebProvider::AppendOverflowItem(int additional_url_count,
-                                             int relevance) {
+                                             int relevance,
+                                             const std::string& title) {
   std::string url_string = "chrome://physical-web";
   GURL url(url_string);
 
@@ -163,19 +168,27 @@ void PhysicalWebProvider::AppendOverflowItem(int additional_url_count,
       AutocompleteMatchType::PHYSICAL_WEB_OVERFLOW);
   match.destination_url = url;
 
-  // Don't omit the chrome:// scheme when displaying the WebUI URL.
-  match.contents = url_formatter::FormatUrl(url,
-      url_formatter::kFormatUrlOmitNothing, net::UnescapeRule::SPACES,
-      nullptr, nullptr, nullptr);
+  std::string contents_title;
+  if (title.length() > kMaxTitleLength) {

[Mark P, 2016/09/08 18:49:49]

This is slightly wrong if indeed as you say above "Longer titles will be
truncated to this length."  After all, you're adding three characters to the
title, so using this comparison (without taking that into account) means that
titles could be up to two characters longer than kMaxTitleLength.

[mattreynolds, 2016/09/09 21:24:54]

I'm now using gfx::TruncateString which ensures the returned string's length is
<= kMaxTitleLength, including the ellipsis character.

+    contents_title = title.substr(0, kMaxTitleLength) + "... ";

[Mark P, 2016/09/08 18:49:49]

You probably should be using one of the standard constants here, perhaps
gfx::kEllipsisUTF16.

Then you probably want to append the " " unconditionally.

[mattreynolds, 2016/09/09 21:24:54]

I switched to using gfx::TruncateString, which automatically appends the
ellipsis .

+  } else {
+    contents_title = title + " ";
+  }

[Mark P, 2016/09/08 18:49:49]

Please also consider what happens with non-latin characters.
For example, can titles be in a RTL language?
Also, there's probably call AutocompleteMatch::SanitizeString() on the title
somewhere in this code path too (or in some common place so you don't call it
twice on the same string).

[Mark P, 2016/09/08 18:49:49]

Please consider what happens with empty titles.

[mattreynolds, 2016/09/09 21:24:54]

I'm not sure what the best practices are for RTL in this situation, but
currently it displays:

and 2 more web pages ...[insert RTL page title]

This looks wrong to my LTR-accustomed eyes but seems to be consistent with the
behavior in the omnibox when composing queries with mixed LTR and RTL.

I also moved the SanitizeString call up to avoid needing to call it twice.

[mattreynolds, 2016/09/09 21:24:54]

I added an alternate string to use when the title is empty.

[Mark P, 2016/09/13 23:34:42]

Good enough for now I think.  You may want to bring this up when as part of the
launch review (internationalization section) for the full rollout.

[mattreynolds, 2016/09/14 19:04:41]

Acknowledged.

+  base::string16 contents_suffix = l10n_util::GetPluralStringFUTF16(
+      IDS_PHYSICAL_WEB_OVERFLOW, additional_url_count - 1);
+  base::string16 contents_string = base::UTF8ToUTF16(contents_title) +
+      contents_suffix;
+
+  match.contents = contents_string;
   match.contents_class.push_back(
-      ACMatchClassification(0, ACMatchClassification::URL));
+      ACMatchClassification(0, ACMatchClassification::DIM));
 
   match.fill_into_edit =
       AutocompleteInput::FormattedStringWithEquivalentMeaning(
           url, match.contents, client_->GetSchemeClassifier());
 
-  match.description = l10n_util::GetPluralStringFUTF16(
-      IDS_PHYSICAL_WEB_OVERFLOW, additional_url_count);
+  match.description = l10n_util::GetStringUTF16(
+      IDS_PHYSICAL_WEB_OVERFLOW_DESCRIPTION);
   match.description_class.push_back(
       ACMatchClassification(0, ACMatchClassification::NONE));