autocomplete: add ability to get full address (hidden behind a flag).

autocomplete: add ability to get full address (hidden behind a flag).

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

[Evan Stade, 2014-05-05 20:50:36]

+isherman for autofill changes
+rouslan for libaddressinput changes

[please use gerrit instead, 2014-05-05 20:55:49]

libaddressinput lgtm.

[Ilya Sherman, 2014-05-06 04:15:58]

Sorry, didn't get to this today; will try to tomorrow.

[Ilya Sherman, 2014-05-07 01:19:19]

https://codereview.chromium.org/261013010/diff/90001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_dialog_common.cc (right):

https://codereview.chromium.org/261013010/diff/90001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_common.cc:39: if (server_type ==
ADDRESS_HOME_STREET_ADDRESS ||
This line seems redundant with line 44.  Also, what happens now if the page asks
for line1 or line2?

https://codereview.chromium.org/261013010/diff/90001/components/autofill/core...
File components/autofill/core/browser/address_i18n.cc (right):

https://codereview.chromium.org/261013010/diff/90001/components/autofill/core...
components/autofill/core/browser/address_i18n.cc:23:
get_info.Run(AutofillType(HTML_TYPE_COUNTRY_CODE, HTML_MODE_SHIPPING)));
nit: Why SHIPPING as opposed to NONE?

https://codereview.chromium.org/261013010/diff/90001/components/autofill/core...
components/autofill/core/browser/address_i18n.cc:24: DCHECK_EQ(2U,
address_data->country_code.size());
What if the country code is unspecified?

https://codereview.chromium.org/261013010/diff/90001/components/autofill/core...
components/autofill/core/browser/address_i18n.cc:37: '\n',
Not all addresses are joined with newlines, right?  Are you certain that this
matches what libaddressinput expects?

https://codereview.chromium.org/261013010/diff/90001/components/autofill/core...
File components/autofill/core/browser/address_i18n.h (right):

https://codereview.chromium.org/261013010/diff/90001/components/autofill/core...
components/autofill/core/browser/address_i18n.h:8: #include "base/callback.h"
nit: Would callback_forward suffice?

https://codereview.chromium.org/261013010/diff/90001/components/autofill/core...
components/autofill/core/browser/address_i18n.h:26: scoped_ptr<
::i18n::addressinput::AddressData> CreateAddressData(
nit: Is the space between "<" and "::" necessary?

https://codereview.chromium.org/261013010/diff/90001/components/autofill/core...
File components/autofill/core/browser/autofill_profile.cc (right):

https://codereview.chromium.org/261013010/diff/90001/components/autofill/core...
components/autofill/core/browser/autofill_profile.cc:33: 
nit: Spurious newline.

https://codereview.chromium.org/261013010/diff/90001/components/autofill/core...
components/autofill/core/browser/autofill_profile.cc:228: base::string16
GetInfoHelper(const AutofillProfile* profile,
nit: Why not const-ref?

https://codereview.chromium.org/261013010/diff/90001/components/autofill/core...
File components/autofill/core/browser/autofill_profile_unittest.cc (right):

https://codereview.chromium.org/261013010/diff/90001/components/autofill/core...
components/autofill/core/browser/autofill_profile_unittest.cc:873: }
Please add a test for getting the full address from an empty profile and another
test for a partially empty profile.

[Evan Stade, 2014-05-09 22:14:58]

PTAL, made some substantial updates (including some in libaddressinput which
need re-review from you Rouslan)

https://codereview.chromium.org/261013010/diff/90001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_dialog_common.cc (right):

https://codereview.chromium.org/261013010/diff/90001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_common.cc:39: if (server_type ==
ADDRESS_HOME_STREET_ADDRESS ||
On 2014/05/07 01:19:19, Ilya Sherman wrote:
> This line seems redundant with line 44.  Also, what happens now if the page
asks
> for line1 or line2?

woopsies, this is the result of a bad merge (I started writing this patch before
updating the dialog to accept many address lines).

https://codereview.chromium.org/261013010/diff/90001/components/autofill/core...
File components/autofill/core/browser/address_i18n.cc (right):

https://codereview.chromium.org/261013010/diff/90001/components/autofill/core...
components/autofill/core/browser/address_i18n.cc:23:
get_info.Run(AutofillType(HTML_TYPE_COUNTRY_CODE, HTML_MODE_SHIPPING)));
On 2014/05/07 01:19:19, Ilya Sherman wrote:
> nit: Why SHIPPING as opposed to NONE?

Done.

https://codereview.chromium.org/261013010/diff/90001/components/autofill/core...
components/autofill/core/browser/address_i18n.cc:24: DCHECK_EQ(2U,
address_data->country_code.size());
On 2014/05/07 01:19:19, Ilya Sherman wrote:
> What if the country code is unspecified?

added a testcase

https://codereview.chromium.org/261013010/diff/90001/components/autofill/core...
components/autofill/core/browser/address_i18n.cc:37: '\n',
On 2014/05/07 01:19:19, Ilya Sherman wrote:
> Not all addresses are joined with newlines, right?  Are you certain that this
> matches what libaddressinput expects?

Yes, all addresses are joined with newlines in the multi-line case. It's only in
the case where we're scrunching things together onto one line that addresses
have different separators in different regions. But the GetInfo callback should
be returning the street address in multi-line format.

https://codereview.chromium.org/261013010/diff/90001/components/autofill/core...
File components/autofill/core/browser/address_i18n.h (right):

https://codereview.chromium.org/261013010/diff/90001/components/autofill/core...
components/autofill/core/browser/address_i18n.h:8: #include "base/callback.h"
On 2014/05/07 01:19:19, Ilya Sherman wrote:
> nit: Would callback_forward suffice?

Done.

https://codereview.chromium.org/261013010/diff/90001/components/autofill/core...
components/autofill/core/browser/address_i18n.h:26: scoped_ptr<
::i18n::addressinput::AddressData> CreateAddressData(
On 2014/05/07 01:19:19, Ilya Sherman wrote:
> nit: Is the space between "<" and "::" necessary?

yes for GCC

https://codereview.chromium.org/261013010/diff/90001/components/autofill/core...
File components/autofill/core/browser/autofill_profile.cc (right):

https://codereview.chromium.org/261013010/diff/90001/components/autofill/core...
components/autofill/core/browser/autofill_profile.cc:33: 
On 2014/05/07 01:19:19, Ilya Sherman wrote:
> nit: Spurious newline.

Done.

https://codereview.chromium.org/261013010/diff/90001/components/autofill/core...
components/autofill/core/browser/autofill_profile.cc:228: base::string16
GetInfoHelper(const AutofillProfile* profile,
On 2014/05/07 01:19:19, Ilya Sherman wrote:
> nit: Why not const-ref?

Done.

[please use gerrit instead, 2014-05-09 22:26:49]

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

https://codereview.chromium.org/261013010/diff/150001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/address_data.cc:133:
std::vector<::i18n::addressinput::AddressField> required_fields =
Place add a space between < and ::.

https://codereview.chromium.org/261013010/diff/150001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/address_data.cc:136: if
(GetFieldValue(required_fields[i]).empty())
Please check for STREET_ADDRESS here to avoid the assert(false) in
GetMemberForField().

[Evan Stade, 2014-05-09 22:49:08]

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

https://codereview.chromium.org/261013010/diff/150001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/address_data.cc:133:
std::vector<::i18n::addressinput::AddressField> required_fields =
On 2014/05/09 22:26:50, Rouslan Solomakhin wrote:
> Place add a space between < and ::.

Done.

https://codereview.chromium.org/261013010/diff/150001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/address_data.cc:136: if
(GetFieldValue(required_fields[i]).empty())
On 2014/05/09 22:26:50, Rouslan Solomakhin wrote:
> Please check for STREET_ADDRESS here to avoid the assert(false) in
> GetMemberForField().

Done.

[please use gerrit instead, 2014-05-09 23:05:49]

Looks like the patch did not get uploaded.

[Evan Stade, 2014-05-09 23:07:58]

On 2014/05/09 23:05:49, Rouslan Solomakhin wrote:
> Looks like the patch did not get uploaded.

voila

[please use gerrit instead, 2014-05-09 23:38:54]

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

https://codereview.chromium.org/261013010/diff/170001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/address_data.cc:137:
(address_lines.empty() || address_lines[0].empty())) {
Please avoid hitting GetFieldValue(STREET_ADDRESS) call when street address
lines are not empty.

[Ilya Sherman, 2014-05-13 04:07:04]

LGTM, thanks.

[please use gerrit instead, 2014-05-13 05:07:45]

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

https://codereview.chromium.org/261013010/diff/190001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/address_data.cc:138: return false;
Thanks, lgtm.

[Evan Stade, 2014-05-14 02:58:50]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-05-14 02:59:56]

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

[commit-bot: I haz the power, 2014-05-14 07:46:32]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-14 07:49:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-14 07:49:05]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[Evan Stade, 2014-05-14 23:06:16]

Rouslan, I had to make some more changes to libaddressinput so that
libaddressinput_util wouldn't depend on libaddressinput_strings (mostly for the
sake of iOS + android). Had to move some logic from Rule to elsewhere. PTAL.

[please use gerrit instead, 2014-05-15 04:29:39]

libaddressinput lgtm

[Evan Stade, 2014-05-15 17:14:33]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-05-15 17:15:57]

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

[commit-bot: I haz the power, 2014-05-15 23:13:31]

Change committed as 270842