Include a page title in the Physical Web omnibox overflow item

components/omnibox/browser/physical_web_provider.cc

 // Copyright (c) 2016 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.
 
 #include "base/memory/ptr_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "components/omnibox/browser/autocomplete_provider_client.h"
 #include "components/omnibox/browser/autocomplete_provider_listener.h"
 #include "components/omnibox/browser/history_url_provider.h"
@@ skipping 18 matching lines @@
 // suggestion will be the overflow item which navigates to the WebUI when
 // tapped.
 static const size_t kPhysicalWebMaxMatches = 1;
 
 // Relevance score of the first Physical Web URL autocomplete match. This score
 // is intended to be between ClipboardURLProvider and ZeroSuggestProvider.
 // Subsequent matches should decrease in relevance to preserve the ordering
 // 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
 PhysicalWebProvider* PhysicalWebProvider::Create(
     AutocompleteProviderClient* client,
     HistoryURLProvider* history_url_provider) {
   return new PhysicalWebProvider(client, history_url_provider);
 }
 
 void PhysicalWebProvider::Start(const AutocompleteInput& input,
@@ skipping 66 matching lines @@
 
     // Add match items with decreasing relevance to preserve the ordering in
     // the metadata list.
     int relevance = kPhysicalWebUrlBaseRelevance - used_slots;
 
     // Append an overflow item if creating a match for each metadata item would
     // exceed the match limit.
     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;
     }
 
     GURL url(url_string);
     base::string16 title = base::UTF8ToUTF16(title_string);
 
     AutocompleteMatch match(this, relevance, false,
         AutocompleteMatchType::PHYSICAL_WEB);
     match.destination_url = url;
 
@@ skipping 12 matching lines @@
     match.description = AutocompleteMatch::SanitizeString(title);
     match.description_class.push_back(
         ACMatchClassification(0, ACMatchClassification::NONE));
 
     matches_.push_back(match);
     ++used_slots;
   }
 }
 
 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);
 
   AutocompleteMatch match(this, relevance, false,
       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));
 
   matches_.push_back(match);
 }