rAc: try really hard not to ellipsize addresses

rAc: try really hard not to ellipsize addresses

Change rAc suggestion text to return two string values, one which is preferred (but may not fit horizontally) and one which is a fallback (which has a better chance to fit horizontally).

BUG=264973
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=217076

[Ilya Sherman, 2013-08-06 08:00:23]

LGTM % nits, but please also ask someone else to take a look at the fiddly Views
code, as I don't feel quite comfortable reviewing the nitty gritty details
there.

https://codereview.chromium.org/21724002/diff/34001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_types.h (right):

https://codereview.chromium.org/21724002/diff/34001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_types.h:200: bool show;
nit:  Perhaps "should_be_shown"?  This currently sounds more like a command,
i.e. method, rather than a property.

https://codereview.chromium.org/21724002/diff/34001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/21724002/diff/34001/chrome/browser/ui/views/a...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:156: int w = 0;
nit: Please spell out "height" and "width" (throughout).

[Evan Stade, 2013-08-06 19:00:37]

+sky for views bits.

https://codereview.chromium.org/21724002/diff/34001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_types.h (right):

https://codereview.chromium.org/21724002/diff/34001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_types.h:200: bool show;
On 2013/08/06 08:00:23, Ilya Sherman (Away Aug. 9-25) wrote:
> nit:  Perhaps "should_be_shown"?  This currently sounds more like a command,
> i.e. method, rather than a property.

will change to |visible|.

https://codereview.chromium.org/21724002/diff/34001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/21724002/diff/34001/chrome/browser/ui/views/a...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:156: int w = 0;
On 2013/08/06 08:00:23, Ilya Sherman (Away Aug. 9-25) wrote:
> nit: Please spell out "height" and "width" (throughout).

I'll change it here, but the views function in view.h is defined as:

virtual int GetHeightForWidth(int w);

so I'd rather stick to that naming convention in the implementation of that
function.

[Evan Stade, 2013-08-06 19:34:53]

https://codereview.chromium.org/21724002/diff/34001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/21724002/diff/34001/chrome/browser/ui/views/a...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:156: int w = 0;
On 2013/08/06 19:00:37, Evan Stade wrote:
> On 2013/08/06 08:00:23, Ilya Sherman (Away Aug. 9-25) wrote:
> > nit: Please spell out "height" and "width" (throughout).
> 
> I'll change it here, but the views function in view.h is defined as:
> 
> virtual int GetHeightForWidth(int w);
> 
> so I'd rather stick to that naming convention in the implementation of that
> function.

scratch that, all updated.

[sky, 2013-08-06 21:28:10]

https://codereview.chromium.org/21724002/diff/76001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/21724002/diff/76001/chrome/browser/ui/views/a...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:1051: int
available_width, int* resulting_height) {
nit: when you wrap one param per line.

https://codereview.chromium.org/21724002/diff/76001/chrome/browser/ui/views/a...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:1051: int
available_width, int* resulting_height) {
How many times if this called with varying widths that you need to cache it?

https://codereview.chromium.org/21724002/diff/76001/chrome/browser/ui/views/a...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:1077: std::pair<bool,
int> values = calculated_heights_[available_width];
const std::pair&

https://codereview.chromium.org/21724002/diff/76001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/autofill/autofill_dialog_views.h (right):

https://codereview.chromium.org/21724002/diff/76001/chrome/browser/ui/views/a...
chrome/browser/ui/views/autofill/autofill_dialog_views.h:396: bool
CanUseVerticallyCompactText(int available_width,
Please add a description.

[Evan Stade, 2013-08-06 23:15:34]

https://chromiumcodereview.appspot.com/21724002/diff/76001/chrome/browser/ui/...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://chromiumcodereview.appspot.com/21724002/diff/76001/chrome/browser/ui/...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:1051: int
available_width, int* resulting_height) {
On 2013/08/06 21:28:10, sky wrote:
> How many times if this called with varying widths that you need to cache it?

It's mostly called with the same width over and over as a result of
GetHeightForWidth during layout calculations.

And the direct answer to that question is "too many times". A few hundred times
just for showing the dialog. There seem to be performance issues that need to be
solved at a higher level, but I think this is still a worthwhile optimization.

https://chromiumcodereview.appspot.com/21724002/diff/76001/chrome/browser/ui/...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:1051: int
available_width, int* resulting_height) {
On 2013/08/06 21:28:10, sky wrote:
> nit: when you wrap one param per line.

Done.

https://chromiumcodereview.appspot.com/21724002/diff/76001/chrome/browser/ui/...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:1077: std::pair<bool,
int> values = calculated_heights_[available_width];
On 2013/08/06 21:28:10, sky wrote:
> const std::pair&

Done.

https://chromiumcodereview.appspot.com/21724002/diff/76001/chrome/browser/ui/...
File chrome/browser/ui/views/autofill/autofill_dialog_views.h (right):

https://chromiumcodereview.appspot.com/21724002/diff/76001/chrome/browser/ui/...
chrome/browser/ui/views/autofill/autofill_dialog_views.h:396: bool
CanUseVerticallyCompactText(int available_width,
On 2013/08/06 21:28:10, sky wrote:
> Please add a description.

Done.

[sky, 2013-08-06 23:51:33]

Please file a bug against calling GetPreferredSize so many times. Something
sounds very wrong.

LGTM

[Evan Stade, 2013-08-07 18:48:15]

On 2013/08/06 23:51:33, sky (OOO back on the 19th) wrote:
> Please file a bug against calling GetPreferredSize so many times. Something
> sounds very wrong.
> 
> LGTM

filed as crbug.com/269674

[commit-bot: I haz the power, 2013-08-07 18:48:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/21724002/90001

[commit-bot: I haz the power, 2013-08-07 18:48:47]

Failed to apply patch for chrome/browser/ui/autofill/data_model_wrapper.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/ui/autofill/data_model_wrapper.cc
  Hunk #1 FAILED at 24.
  Hunk #2 FAILED at 55.
  Hunk #3 FAILED at 76.
  Hunk #4 FAILED at 92.
  Hunk #5 FAILED at 113.
  Hunk #6 FAILED at 142.
  Hunk #7 succeeded at 160 (offset 6 lines).
  Hunk #8 FAILED at 171.
  Hunk #9 FAILED at 192.
  Hunk #10 FAILED at 203.
  Hunk #11 FAILED at 227.
  Hunk #12 succeeded at 244 (offset 6 lines).
  Hunk #13 FAILED at 259.
  11 out of 13 hunks FAILED -- saving rejects to file
chrome/browser/ui/autofill/data_model_wrapper.cc.rej

Patch:       chrome/browser/ui/autofill/data_model_wrapper.cc
Index: chrome/browser/ui/autofill/data_model_wrapper.cc
diff --git a/chrome/browser/ui/autofill/data_model_wrapper.cc
b/chrome/browser/ui/autofill/data_model_wrapper.cc
index
b1a1f766b2f7bda8568795490fac6ed14a19bc39..d921b4aed204b27945ab4c70d6ab0dae577c2751
100644
--- a/chrome/browser/ui/autofill/data_model_wrapper.cc
+++ b/chrome/browser/ui/autofill/data_model_wrapper.cc
@@ -24,17 +24,25 @@ namespace autofill {
 
 DataModelWrapper::~DataModelWrapper() {}
 
-string16 DataModelWrapper::GetDisplayText() {
-  string16 comma = ASCIIToUTF16(", ");
-  string16 label = GetInfo(NAME_FULL) + comma + GetInfo(ADDRESS_HOME_LINE1);
-  string16 address2 = GetInfo(ADDRESS_HOME_LINE2);
-  if (!address2.empty())
-    label += comma + address2;
-  label += ASCIIToUTF16("\n") +
-      GetInfo(ADDRESS_HOME_CITY) + comma +
-      GetInfo(ADDRESS_HOME_STATE) + ASCIIToUTF16(" ") +
-      GetInfo(ADDRESS_HOME_ZIP);
-  return label;
+gfx::Image DataModelWrapper::GetIcon() {
+  return gfx::Image();
+}
+
+void DataModelWrapper::FillInputs(DetailInputs* inputs) {
+  for (size_t i = 0; i < inputs->size(); ++i) {
+    (*inputs)[i].initial_value = GetInfo((*inputs)[i].type);
+  }
+}
+
+bool DataModelWrapper::GetDisplayText(
+    base::string16* vertically_compact,
+    base::string16* horizontally_compact) {
+  base::string16 comma = ASCIIToUTF16(", ");
+  base::string16 newline = ASCIIToUTF16("\n");
+
+  *vertically_compact = GetAddressDisplayText(comma);
+  *horizontally_compact = GetAddressDisplayText(newline);
+  return true;
 }
 
 bool DataModelWrapper::FillFormStructure(
@@ -55,20 +63,31 @@ bool DataModelWrapper::FillFormStructure(
   return filled_something;
 }
 
-void DataModelWrapper::FillInputs(DetailInputs* inputs) {
-  for (size_t i = 0; i < inputs->size(); ++i) {
-    (*inputs)[i].initial_value = GetInfo((*inputs)[i].type);
-  }
-}
+DataModelWrapper::DataModelWrapper() {}
 
 void DataModelWrapper::FillFormField(AutofillField* field) const {
   field->value = GetInfo(field->type());
 }
 
-DataModelWrapper::DataModelWrapper() {}
+base::string16 DataModelWrapper::GetAddressDisplayText(
+    const base::string16& separator) {
+  base::string16 address = GetInfo(NAME_FULL) + separator +
+      GetInfo(ADDRESS_HOME_LINE1);
+  base::string16 address2 = GetInfo(ADDRESS_HOME_LINE2);
+  if (!address2.empty())
+    address += separator + address2;
 
-gfx::Image DataModelWrapper::GetIcon() {
-  return gfx::Image();
+  base::string16 comma = ASCIIToUTF16(", ");
+  base::string16 newline = ASCIIToUTF16("\n");
+  address += separator +
+      GetInfo(ADDRESS_HOME_CITY) + comma +
+      GetInfo(ADDRESS_HOME_STATE) + ASCIIToUTF16(" ") +
+      GetInfo(ADDRESS_HOME_ZIP);
+
+  // TODO(estade): email?
+  address += newline + GetInfo(PHONE_HOME_WHOLE_NUMBER);
+
+  return address;
 }
 
 // EmptyDataModelWrapper
@@ -76,8 +95,8 @@ gfx::Image DataModelWrapper::GetIcon() {
 EmptyDataModelWrapper::EmptyDataModelWrapper() {}
 EmptyDataModelWrapper::~EmptyDataModelWrapper() {}
 
-string16 EmptyDataModelWrapper::GetInfo(AutofillFieldType type) const {
-  return string16();
+base::string16 EmptyDataModelWrapper::GetInfo(AutofillFieldType type) const {
+  return base::string16();
 }
 
 void EmptyDataModelWrapper::FillFormField(AutofillField* field) const {}
@@ -92,7 +111,7 @@ AutofillDataModelWrapper::AutofillDataModelWrapper(
 
 AutofillDataModelWrapper::~AutofillDataModelWrapper() {}
 
-string16 AutofillDataModelWrapper::GetInfo(AutofillFieldType type) const {
+base::string16 AutofillDataModelWrapper::GetInfo(AutofillFieldType type) const
{
   return data_model_->GetInfo(type, g_browser_process->GetApplicationLocale());
 }
 
@@ -113,7 +132,7 @@ AutofillProfileWrapper::~AutofillProfileWrapper() {}
 void AutofillProfileWrapper::FillInputs(DetailInputs* inputs) {
   const std::string app_locale = g_browser_process->GetApplicationLocale();
   for (size_t j = 0; j < inputs->size(); ++j) {
-    std::vector<string16> values;
+    std::vector<base::string16> values;
     profile_->GetMultiInfo((*inputs)[j].type, app_locale, &values);
     (*inputs)[j].initial_value = values[variant()];
   }
@@ -142,7 +161,8 @@ AutofillCreditCardWrapper::AutofillCreditCardWrapper(const
CreditCard* card)
 
 AutofillCreditCardWrapper::~AutofillCreditCardWrapper() {}
 
-string16 AutofillCreditCardWrapper::GetInfo(AutofillFieldType type) const {
+base::string16 AutofillCreditCardWrapper::GetInfo(
+    AutofillFieldType type) const {
   if (type == CREDIT_CARD_EXP_MONTH)
     return MonthComboboxModel::FormatMonth(card_->expiration_month());
 
@@ -154,11 +174,14 @@ gfx::Image AutofillCreditCardWrapper::GetIcon() {
   return rb.GetImageNamed(CreditCard::IconResourceId(card_->type()));
 }
 
-string16 AutofillCreditCardWrapper::GetDisplayText() {
+bool AutofillCreditCardWrapper::GetDisplayText(
+    base::string16* vertically_compact,
+    base::string16* horizontally_compact) {
   if (!card_->IsValid())
-    return string16();
+    return false;
 
-  return card_->TypeAndLastFourDigits();
+  *vertically_compact = *horizontally_compact = card_->TypeAndLastFourDigits();
+  return true;
 }
 
 // WalletAddressWrapper
@@ -168,17 +191,20 @@ WalletAddressWrapper::WalletAddressWrapper(
 
 WalletAddressWrapper::~WalletAddressWrapper() {}
 
-string16 WalletAddressWrapper::GetInfo(AutofillFieldType type) const {
+base::string16 WalletAddressWrapper::GetInfo(AutofillFieldType type) const {
   return address_->GetInfo(type, g_browser_process->GetApplicationLocale());
 }
 
-string16 WalletAddressWrapper::GetDisplayText() {
+bool WalletAddressWrapper::GetDisplayText(
+    base::string16* vertically_compact,
+    base::string16* horizontally_compact) {
   if (!address_->is_complete_address() ||
       GetInfo(PHONE_HOME_WHOLE_NUMBER).empty()) {
-    return string16();
+    return false;
   }
 
-  return DataModelWrapper::GetDisplayText();
+  return DataModelWrapper::GetDisplayText(vertically_compact,
+                                          horizontally_compact);
 }
 
 // WalletInstrumentWrapper
@@ -189,7 +215,7 @@ WalletInstrumentWrapper::WalletInstrumentWrapper(
 
 WalletInstrumentWrapper::~WalletInstrumentWrapper() {}
 
-string16 WalletInstrumentWrapper::GetInfo(AutofillFieldType type) const {
+base::string16 WalletInstrumentWrapper::GetInfo(AutofillFieldType type) const {
   if (type == CREDIT_CARD_EXP_MONTH)
     return MonthComboboxModel::FormatMonth(instrument_->expiration_month());
 
@@ -200,18 +226,23 @@ gfx::Image WalletInstrumentWrapper::GetIcon() {
   return instrument_->CardIcon();
 }
 
-string16 WalletInstrumentWrapper::GetDisplayText() {
+bool WalletInstrumentWrapper::GetDisplayText(
+    base::string16* vertically_compact,
+    base::string16* horizontally_compact) {
   // TODO(dbeam): handle other instrument statuses? http://crbug.com/233048
   if (instrument_->status() == wallet::WalletItems::MaskedInstrument::EXPIRED
||
       !instrument_->address().is_complete_address() ||
       GetInfo(PHONE_HOME_WHOLE_NUMBER).empty()) {
-    return string16();
+    return false;
   }
 
+  DataModelWrapper::GetDisplayText(vertically_compact, horizontally_compact);
   // TODO(estade): descriptive_name() is user-provided. Should we use it or
   // just type + last 4 digits?
-  string16 line1 = instrument_->descriptive_name();
-  return line1 + ASCIIToUTF16("\n") + DataModelWrapper::GetDisplayText();
+  base::string16 line1 = instrument_->descriptive_name() + ASCIIToUTF16("\n");
+  *vertically_compact = line1 + *vertically_compact;
+  *horizontally_compact = line1 + *horizontally_compact;
+  return true;
 }
 
 // FullWalletBillingWrapper
@@ -224,7 +255,7 @@ FullWalletBillingWrapper::FullWalletBillingWrapper(
 
 FullWalletBillingWrapper::~FullWalletBillingWrapper() {}
 
-string16 FullWalletBillingWrapper::GetInfo(AutofillFieldType type) const {
+base::string16 FullWalletBillingWrapper::GetInfo(AutofillFieldType type) const
{
   if (type == CREDIT_CARD_EXP_MONTH)
     return MonthComboboxModel::FormatMonth(full_wallet_->expiration_month());
 
@@ -235,12 +266,15 @@ string16
FullWalletBillingWrapper::GetInfo(AutofillFieldType type) const {
       type, g_browser_process->GetApplicationLocale());
 }
 
-string16 FullWalletBillingWrapper::GetDisplayText() {
+bool FullWalletBillingWrapper::GetDisplayText(
+    base::string16* vertically_compact,
+    base::string16* horizontally_compact) {
   // TODO(dbeam): handle other required actions? http://crbug.com/163508
   if (full_wallet_->HasRequiredAction(wallet::UPDATE_EXPIRATION_DATE))
-    return string16();
+    return false;
 
-  return DataModelWrapper::GetDisplayText();
+  return DataModelWrapper::GetDisplayText(vertically_compact,
+                                          horizontally_compact);
 }
 
 // FullWalletShippingWrapper
@@ -253,7 +287,8 @@ FullWalletShippingWrapper::FullWalletShippingWrapper(
 
 FullWalletShippingWrapper::~FullWalletShippingWrapper() {}
 
-string16 FullWalletShippingWrapper::GetInfo(AutofillFieldType type) const {
+base::string16 FullWalletShippingWrapper::GetInfo(
+    AutofillFieldType type) const {
   return full_wallet_->shipping_address()->GetInfo(
       type, g_browser_process->GetApplicationLocale());
 }

[commit-bot: I haz the power, 2013-08-07 22:20:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/21724002/97001

[commit-bot: I haz the power, 2013-08-07 22:47:13]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-08-08 00:41:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/21724002/120001

[commit-bot: I haz the power, 2013-08-08 19:39:47]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-08-08 22:52:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/21724002/90002

[commit-bot: I haz the power, 2013-08-09 00:47:53]

Retried try job too often on linux_chromeos for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-08-09 16:14:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/21724002/90002

[commit-bot: I haz the power, 2013-08-09 18:20:45]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-08-09 23:23:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/21724002/172001

[commit-bot: I haz the power, 2013-08-10 00:04:09]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2013-08-12 17:52:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/21724002/183001

[commit-bot: I haz the power, 2013-08-12 21:20:38]

Change committed as 217076