[rac] Format an address for display.

[rac] Format an address for display.

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

[please use gerrit instead, 2014-01-13 21:48:53]

Evan: PTAL.

[Evan Stade, 2014-01-13 22:04:39]

https://codereview.chromium.org/131223004/diff/100001/third_party/libaddressi...
File
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_data.h
(right):

https://codereview.chromium.org/131223004/diff/100001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_data.h:48:
FORMAT_TWO_LINES,
what is this going to be used for?

I think the API should simply be

a) vector of strings (corresponds to rows in the full format)
b) a separator string (to put in between strings from the vector in the compact
format)

https://codereview.chromium.org/131223004/diff/100001/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/address_data.cc (right):

https://codereview.chromium.org/131223004/diff/100001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/address_data.cc:236:
std::stringstream ss;
nit: use std::string and concatenation

[please use gerrit instead, 2014-01-14 01:17:46]

Evan: PTAL Patch Set 2.

https://codereview.chromium.org/131223004/diff/100001/third_party/libaddressi...
File
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_data.h
(right):

https://codereview.chromium.org/131223004/diff/100001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_data.h:48:
FORMAT_TWO_LINES,
On 2014/01/13 22:04:39, Evan Stade wrote:
> what is this going to be used for?
> 
> I think the API should simply be
> 
> a) vector of strings (corresponds to rows in the full format)
> b) a separator string (to put in between strings from the vector in the
compact
> format)

Done.

New API:
a) AddressData::BuildDisplayLines()
b) GetCompactAddressLinesSeparator()

https://codereview.chromium.org/131223004/diff/100001/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/address_data.cc (right):

https://codereview.chromium.org/131223004/diff/100001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/address_data.cc:236:
std::stringstream ss;
On 2014/01/13 22:04:39, Evan Stade wrote:
> nit: use std::string and concatenation

Done.

[Evan Stade, 2014-01-14 18:05:22]

will review more later

https://codereview.chromium.org/131223004/diff/270002/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/address_data.cc (right):

https://codereview.chromium.org/131223004/diff/270002/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/address_data.cc:41: const
FormatElement& element = rule.GetFormat()[i][j];
rule.GetFormat()[i] is used enough to get a local variable imo (like |element|
here)

https://codereview.chromium.org/131223004/diff/270002/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/address_data.cc:56: for (size_t k =
0; k < address_lines.size(); ++k) {
I don't understand why street address line 2 might ever be on the same line as
street address line 1

https://codereview.chromium.org/131223004/diff/270002/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/address_data.cc:83: const
std::string& AddressData::GetField(AddressField field) const {
nit: should be called GetFieldValue

https://codereview.chromium.org/131223004/diff/270002/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/rule.cc (right):

https://codereview.chromium.org/131223004/diff/270002/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/rule.cc:94:
elements->back().push_back(FormatElement(data_parts[0]));
don't really understand what's going on here, and why you can't write this all
as one loop. Comments would be nice.

https://codereview.chromium.org/131223004/diff/270002/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/rule.h (right):

https://codereview.chromium.org/131223004/diff/270002/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/rule.h:30: enum Type {
imo, you don't need this type. Just key off the emptiness of |literal|.

https://codereview.chromium.org/131223004/diff/270002/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/rule.h:32: LITERAL
docs

[please use gerrit instead, 2014-01-14 23:21:11]

Evan: PTAL Patch Set 3.

https://codereview.chromium.org/131223004/diff/270002/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/address_data.cc (right):

https://codereview.chromium.org/131223004/diff/270002/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/address_data.cc:41: const
FormatElement& element = rule.GetFormat()[i][j];
On 2014/01/14 18:05:22, Evan Stade wrote:
> rule.GetFormat()[i] is used enough to get a local variable imo (like |element|
> here)

Done.

https://codereview.chromium.org/131223004/diff/270002/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/address_data.cc:56: for (size_t k =
0; k < address_lines.size(); ++k) {
On 2014/01/14 18:05:22, Evan Stade wrote:
> I don't understand why street address line 2 might ever be on the same line as
> street address line 1

Take the https://i18napis.appspot.com/ssl-address/data/CI format for example:

"%N%n%O%n%X %A %C %X"

This parses into:

{
    {RECIPIENT},
    {ORGANIZATION},
    {SORTING-CODE, " ", STREET-ADDRESS, " ", LOCALITY, " ", SORTING-CODE}
}

https://codereview.chromium.org/131223004/diff/270002/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/address_data.cc:83: const
std::string& AddressData::GetField(AddressField field) const {
On 2014/01/14 18:05:22, Evan Stade wrote:
> nit: should be called GetFieldValue

Done.

https://codereview.chromium.org/131223004/diff/270002/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/rule.cc (right):

https://codereview.chromium.org/131223004/diff/270002/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/rule.cc:94:
elements->back().push_back(FormatElement(data_parts[0]));
On 2014/01/14 18:05:22, Evan Stade wrote:
> don't really understand what's going on here, and why you can't write this all
> as one loop. Comments would be nice.

Added a comment.

https://codereview.chromium.org/131223004/diff/270002/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/rule.h (right):

https://codereview.chromium.org/131223004/diff/270002/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/rule.h:30: enum Type {
On 2014/01/14 18:05:22, Evan Stade wrote:
> imo, you don't need this type. Just key off the emptiness of |literal|.

Done.

https://codereview.chromium.org/131223004/diff/270002/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/rule.h:32: LITERAL
On 2014/01/14 18:05:22, Evan Stade wrote:
> docs

Done.

[Evan Stade, 2014-01-14 23:45:36]

https://codereview.chromium.org/131223004/diff/270002/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/address_data.cc (right):

https://codereview.chromium.org/131223004/diff/270002/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/address_data.cc:56: for (size_t k =
0; k < address_lines.size(); ++k) {
On 2014/01/14 23:21:12, Rouslan Solomakhin wrote:
> On 2014/01/14 18:05:22, Evan Stade wrote:
> > I don't understand why street address line 2 might ever be on the same line
as
> > street address line 1
> 
> Take the https://i18napis.appspot.com/ssl-address/data/CI format for example:
> 
> "%N%n%O%n%X %A %C %X"
> 
> This parses into:
> 
> {
>     {RECIPIENT},
>     {ORGANIZATION},
>     {SORTING-CODE, " ", STREET-ADDRESS, " ", LOCALITY, " ", SORTING-CODE}
> }

I think that would imply there's only one street address line, so you don't need
to loop here. I do think you can combine the size() == 1 case and this loop.

https://codereview.chromium.org/131223004/diff/430001/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/rule.cc (right):

https://codereview.chromium.org/131223004/diff/430001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/rule.cc:86:
std::vector<std::vector<FormatElement> >* elements) {
nit: s/elements/lines

https://codereview.chromium.org/131223004/diff/430001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/rule.cc:101: 
^H

https://codereview.chromium.org/131223004/diff/430001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/rule.cc:104: // the token is a
literal.
thanks for the explanation. To make the code more self-documenting, you could
use some local variables:

const char control_character = data_parts[i][0];
std::string literal = data_parts[i].substr(1);

[please use gerrit instead, 2014-01-15 00:05:47]

Evan: PTAL Patch Set 4.

https://codereview.chromium.org/131223004/diff/270002/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/address_data.cc (right):

https://codereview.chromium.org/131223004/diff/270002/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/address_data.cc:56: for (size_t k =
0; k < address_lines.size(); ++k) {
On 2014/01/14 23:45:36, Evan Stade wrote:
> On 2014/01/14 23:21:12, Rouslan Solomakhin wrote:
> > On 2014/01/14 18:05:22, Evan Stade wrote:
> > > I don't understand why street address line 2 might ever be on the same
line
> as
> > > street address line 1
> > 
> > Take the https://i18napis.appspot.com/ssl-address/data/CI format for
example:
> > 
> > "%N%n%O%n%X %A %C %X"
> > 
> > This parses into:
> > 
> > {
> >     {RECIPIENT},
> >     {ORGANIZATION},
> >     {SORTING-CODE, " ", STREET-ADDRESS, " ", LOCALITY, " ", SORTING-CODE}
> > }
> 
> I think that would imply there's only one street address line, so you don't
need
> to loop here. I do think you can combine the size() == 1 case and this loop.

Combined the cases and simplified the code. If Chrome provides only one street
address line for Ivory Coast, everything will look good.

https://codereview.chromium.org/131223004/diff/430001/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/rule.cc (right):

https://codereview.chromium.org/131223004/diff/430001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/rule.cc:86:
std::vector<std::vector<FormatElement> >* elements) {
On 2014/01/14 23:45:36, Evan Stade wrote:
> nit: s/elements/lines

Done.

https://codereview.chromium.org/131223004/diff/430001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/rule.cc:101: 
On 2014/01/14 23:45:36, Evan Stade wrote:
> ^H

Done.

https://codereview.chromium.org/131223004/diff/430001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/rule.cc:104: // the token is a
literal.
On 2014/01/14 23:45:36, Evan Stade wrote:
> thanks for the explanation. To make the code more self-documenting, you could
> use some local variables:
> 
> const char control_character = data_parts[i][0];
> std::string literal = data_parts[i].substr(1);

Done.

[Evan Stade, 2014-01-16 00:37:40]

lgtm

https://codereview.chromium.org/131223004/diff/560001/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/address_data.cc (right):

https://codereview.chromium.org/131223004/diff/560001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/address_data.cc:30: void
AddressData::BuildDisplayLines(std::vector<std::string>* lines) const {
nit: call this FormatForDisplay?

https://codereview.chromium.org/131223004/diff/560001/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/address_ui.cc (right):

https://codereview.chromium.org/131223004/diff/560001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/address_ui.cc:81:
std::set<AddressField> fields;
need a comment that this is for avoiding showing an input for the same thing
twice (also an example of when the same field appears on the address format
twice)

https://codereview.chromium.org/131223004/diff/560001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/address_ui.cc:86: int num_fields =
0;
num_fields_this_row

https://codereview.chromium.org/131223004/diff/560001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/address_ui.cc:101:
fields.find(element_it->field) != fields.end()) {
store field in a local var?

https://codereview.chromium.org/131223004/diff/560001/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/rule.cc (right):

https://codereview.chromium.org/131223004/diff/560001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/rule.cc:104: const char
control_character = format_parts[i][0];
should maybe:

if (format_parts[i].empty())
  continue;

https://codereview.chromium.org/131223004/diff/560001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/rule.cc:114: std::string literal =
format_parts[i].substr(1);
std::string literal = format_parts[i].substr(1);
if (!literal.empty())
  lines->back().push_back(FormatElement(literal));

[please use gerrit instead, 2014-01-16 01:22:04]

https://codereview.chromium.org/131223004/diff/560001/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/address_data.cc (right):

https://codereview.chromium.org/131223004/diff/560001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/address_data.cc:30: void
AddressData::BuildDisplayLines(std::vector<std::string>* lines) const {
On 2014/01/16 00:37:40, Evan Stade wrote:
> nit: call this FormatForDisplay?

Done.

https://codereview.chromium.org/131223004/diff/560001/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/address_ui.cc (right):

https://codereview.chromium.org/131223004/diff/560001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/address_ui.cc:81:
std::set<AddressField> fields;
On 2014/01/16 00:37:40, Evan Stade wrote:
> need a comment that this is for avoiding showing an input for the same thing
> twice (also an example of when the same field appears on the address format
> twice)

Done.

https://codereview.chromium.org/131223004/diff/560001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/address_ui.cc:86: int num_fields =
0;
On 2014/01/16 00:37:40, Evan Stade wrote:
> num_fields_this_row

Done.

https://codereview.chromium.org/131223004/diff/560001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/address_ui.cc:101:
fields.find(element_it->field) != fields.end()) {
On 2014/01/16 00:37:40, Evan Stade wrote:
> store field in a local var?

Done.

https://codereview.chromium.org/131223004/diff/560001/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/rule.cc (right):

https://codereview.chromium.org/131223004/diff/560001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/rule.cc:104: const char
control_character = format_parts[i][0];
On 2014/01/16 00:37:40, Evan Stade wrote:
> should maybe:
> 
> if (format_parts[i].empty())
>   continue;

Done.

https://codereview.chromium.org/131223004/diff/560001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/rule.cc:114: std::string literal =
format_parts[i].substr(1);
On 2014/01/16 00:37:40, Evan Stade wrote:
> std::string literal = format_parts[i].substr(1);
> if (!literal.empty())
>   lines->back().push_back(FormatElement(literal));

Done.

[commit-bot: I haz the power, 2014-01-16 01:27:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rouslan@chromium.org/131223004/690001

[commit-bot: I haz the power, 2014-01-16 04:18:02]

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

[commit-bot: I haz the power, 2014-01-16 17:14:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rouslan@chromium.org/131223004/690001

[commit-bot: I haz the power, 2014-01-16 21:14:45]

Failed to apply patch for
third_party/libaddressinput/chromium/cpp/src/address_ui.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file third_party/libaddressinput/chromium/cpp/src/address_ui.cc
  Hunk #1 succeeded at 17 with fuzz 2 (offset -1 lines).
  Hunk #2 FAILED at 78.
  Hunk #3 succeeded at 98 (offset -1 lines).
  1 out of 3 hunks FAILED -- saving rejects to file
third_party/libaddressinput/chromium/cpp/src/address_ui.cc.rej

Patch:       third_party/libaddressinput/chromium/cpp/src/address_ui.cc
Index: third_party/libaddressinput/chromium/cpp/src/address_ui.cc
diff --git a/third_party/libaddressinput/chromium/cpp/src/address_ui.cc
b/third_party/libaddressinput/chromium/cpp/src/address_ui.cc
index
cf57543bb743e2e9012f72e2bad16732dd799083..f6f35d07981101d769ab96016c2dd2aa632f8f67
100644
--- a/third_party/libaddressinput/chromium/cpp/src/address_ui.cc
+++ b/third_party/libaddressinput/chromium/cpp/src/address_ui.cc
@@ -18,6 +18,8 @@
 #include <libaddressinput/address_ui_component.h>
 #include <libaddressinput/localization.h>
 
+#include <algorithm>
+#include <set>
 #include <string>
 #include <vector>
 
@@ -76,20 +78,43 @@ std::vector<AddressUiComponent> BuildComponents(
     return result;
   }
 
-  for (std::vector<std::vector<AddressField> >::const_iterator
-       line_it = rule.GetFormat().begin();
+  // For avoiding showing an input field twice, when the field is displayed
+  // twice on an envelope.
+  std::set<AddressField> fields;
+
+  for (std::vector<std::vector<FormatElement> >::const_iterator
+           line_it = rule.GetFormat().begin();
        line_it != rule.GetFormat().end();
        ++line_it) {
-    for (std::vector<AddressField>::const_iterator field_it = line_it->begin();
-         field_it != line_it->end(); ++field_it) {
+    int num_fields_this_row = 0;
+    for (std::vector<FormatElement>::const_iterator element_it =
+             line_it->begin();
+         element_it != line_it->end();
+         ++element_it) {
+      if (element_it->IsField()) {
+        ++num_fields_this_row;
+      }
+    }
+
+    for (std::vector<FormatElement>::const_iterator element_it =
+             line_it->begin();
+         element_it != line_it->end();
+         ++element_it) {
+      AddressField field = element_it->field;
+      if (!element_it->IsField() || fields.find(field) != fields.end()) {
+        continue;
+      }
+      fields.insert(field);
+
       AddressUiComponent component;
       component.length_hint =
-          line_it->size() == 1 ? AddressUiComponent::HINT_LONG
-                               : AddressUiComponent::HINT_SHORT;
-      component.field = *field_it;
+          num_fields_this_row == 1 ? AddressUiComponent::HINT_LONG
+                                   : AddressUiComponent::HINT_SHORT;
+      component.field = field;
       component.name = localization.GetString(
-          GetMessageIdForField(*field_it, rule.GetAdminAreaNameMessageId(),
-                              rule.GetPostalCodeNameMessageId()));
+          GetMessageIdForField(field,
+                               rule.GetAdminAreaNameMessageId(),
+                               rule.GetPostalCodeNameMessageId()));
       result.push_back(component);
     }
   }
@@ -97,5 +122,31 @@ std::vector<AddressUiComponent> BuildComponents(
   return result;
 }
 
+const std::string& GetCompactAddressLinesSeparator(
+    const std::string& language_code,
+    const std::string& country_code) {
+  Rule rule;
+  rule.CopyFrom(Rule::GetDefault());
+  if (!rule.ParseSerializedRule(
+          RegionDataConstants::GetRegionData(country_code))) {
+    return RegionDataConstants::GetLanguageCompactLineSeparator(language_code);
+  }
+
+  std::vector<std::string>::const_iterator lang_it =
+      std::find(rule.GetLanguages().begin(),
+                rule.GetLanguages().end(),
+                language_code);
+  if (lang_it != rule.GetLanguages().end()) {
+    return RegionDataConstants::GetLanguageCompactLineSeparator(*lang_it);
+  }
+
+  if (!rule.GetLanguage().empty()) {
+    return RegionDataConstants::GetLanguageCompactLineSeparator(
+        rule.GetLanguage());
+  }
+
+  return RegionDataConstants::GetCountryCompactLineSeparator(country_code);
+}
+
 }  // namespace addressinput
 }  // namespace i18n

[commit-bot: I haz the power, 2014-01-16 21:27:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rouslan@chromium.org/131223004/980001

[commit-bot: I haz the power, 2014-01-17 00:14:39]

Change committed as 245389