Use/save language code for autofill profiles.

Use/save language code for autofill profiles.

For saved autofill profiles, use the associated language code when retrieving
address ui components. For new autofill profiles, save the best language code
tag returned from libaddressinput's BuildUiComponents.

BUG=454034
Committed: https://crrev.com/5c9c165ecc87231b47ef3f93f63a563c7e966039
Cr-Commit-Position: refs/heads/master@{#314355}

[Theresa, 2015-02-02 19:56:33]

twellington@chromium.org changed reviewers:
+ newt@chromium.org, rouslan@chromium.org

[please use gerrit instead, 2015-02-02 20:26:38]

LGTM

Thank you for the quick fix.

https://codereview.chromium.org/882123003/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java
(right):

https://codereview.chromium.org/882123003/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:123:
sCurrentBestLanguageCode =
Minor nit: Is it OK for a static method to have a side-effect?

https://codereview.chromium.org/882123003/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java
(right):

https://codereview.chromium.org/882123003/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:54:
private boolean mShouldPreserveLanguageCodeString;
Nit: I think a name like "mIsCreatingNewProfile" would make this variable more
clear. For example:

    if (mIsCreatingNewProfile) {
        mLanguageCodeString =
AutofillProfileBridge.getCurrentBestLanguageCode();
    }

(Note that mIsCreatingNewProfile == !mShouldPreserveLanguageCodeString.)

[newt (away), 2015-02-02 22:06:10]

https://codereview.chromium.org/882123003/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java
(right):

https://codereview.chromium.org/882123003/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:63:
private static String sCurrentBestLanguageCode;
I'd make this non-static, and also make getAddressUiComponents() and
getCurrentBestLanguageCode() non-static.

You really should avoid statics as much as possible. They create unexpected
dependencies between otherwise non-interacting pieces of code. For example:
imagine that a user is running Chrome in multi-instance mode on a Samsung tablet
with the device language set to German. In instance A of Chrome, the user is
editing an existing autofill profile for a Japanese address; in instance B of
Chrome, the user is creating a new autofill profile. The return value of
getCurrentBestLanguageCode() in instance B of Chrome depends on the timing of
actions in instance A of Chrome. Not good.

https://codereview.chromium.org/882123003/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:123:
sCurrentBestLanguageCode =
On 2015/02/02 20:26:38, Rouslan Solomakhin wrote:
> Minor nit: Is it OK for a static method to have a side-effect?

Nooooooo

[Theresa, 2015-02-02 22:28:26]

On 2015/02/02 22:06:10, newt wrote:
>
https://codereview.chromium.org/882123003/diff/1/chrome/android/java/src/org/...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java
> (right):
> 
>
https://codereview.chromium.org/882123003/diff/1/chrome/android/java/src/org/...
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:63:
> private static String sCurrentBestLanguageCode;
> I'd make this non-static, and also make getAddressUiComponents() and
> getCurrentBestLanguageCode() non-static.
> 
> You really should avoid statics as much as possible. They create unexpected
> dependencies between otherwise non-interacting pieces of code. For example:
> imagine that a user is running Chrome in multi-instance mode on a Samsung
tablet
> with the device language set to German. In instance A of Chrome, the user is
> editing an existing autofill profile for a Japanese address; in instance B of
> Chrome, the user is creating a new autofill profile. The return value of
> getCurrentBestLanguageCode() in instance B of Chrome depends on the timing of
> actions in instance A of Chrome. Not good.
> 
>
https://codereview.chromium.org/882123003/diff/1/chrome/android/java/src/org/...
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:123:
> sCurrentBestLanguageCode =
> On 2015/02/02 20:26:38, Rouslan Solomakhin wrote:
> > Minor nit: Is it OK for a static method to have a side-effect?
> 
> Nooooooo

I'll make getAddressUiComponents and getCurrentBestLanguage non-static

[Theresa, 2015-02-02 22:30:02]

On 2015/02/02 20:26:38, Rouslan Solomakhin wrote:
> LGTM
> 
> Thank you for the quick fix.
> 
>
https://codereview.chromium.org/882123003/diff/1/chrome/android/java/src/org/...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java
> (right):
> 
>
https://codereview.chromium.org/882123003/diff/1/chrome/android/java/src/org/...
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:123:
> sCurrentBestLanguageCode =
> Minor nit: Is it OK for a static method to have a side-effect?
> 
>
https://codereview.chromium.org/882123003/diff/1/chrome/android/java/src/org/...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java
> (right):
> 
>
https://codereview.chromium.org/882123003/diff/1/chrome/android/java/src/org/...
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileEditor.java:54:
> private boolean mShouldPreserveLanguageCodeString;
> Nit: I think a name like "mIsCreatingNewProfile" would make this variable more
> clear. For example:
> 
>     if (mIsCreatingNewProfile) {
>         mLanguageCodeString =
> AutofillProfileBridge.getCurrentBestLanguageCode();
>     }
> 
> (Note that mIsCreatingNewProfile == !mShouldPreserveLanguageCodeString.)

The mShouldPreserveLanguageCodeString is set to false if the user changes the
country on a saved profile. Maybe something like
mUseSavedProfileLanguageCodeString would be more clear?

[newt (away), 2015-02-02 23:09:30]

> The mShouldPreserveLanguageCodeString is set to false if the user changes the
> country on a saved profile. Maybe something like
> mUseSavedProfileLanguageCodeString would be more clear?

If you use that name, I'd suggest simplifying the name
"mUseSavedProfileLanguage"

[please use gerrit instead, 2015-02-02 23:10:10]

On 2015/02/02 23:09:30, newt wrote:
> > The mShouldPreserveLanguageCodeString is set to false if the user changes
the
> > country on a saved profile. Maybe something like
> > mUseSavedProfileLanguageCodeString would be more clear?
> 
> If you use that name, I'd suggest simplifying the name
> "mUseSavedProfileLanguage"

SGTM

[Theresa, 2015-02-02 23:53:31]

PTAL

[please use gerrit instead, 2015-02-03 00:11:41]

Still LGTM

[newt (away), 2015-02-03 00:13:19]

lgtm. Thanks for the improvement :)

[Theresa, 2015-02-03 00:14:48]

The CQ bit was checked by twellington@chromium.org

[commit-bot: I haz the power, 2015-02-03 00:16:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/882123003/20001

[commit-bot: I haz the power, 2015-02-03 02:17:31]

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

[commit-bot: I haz the power, 2015-02-03 02:17:32]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium.win (None)

[Theresa, 2015-02-03 02:19:56]

The CQ bit was checked by twellington@chromium.org

[commit-bot: I haz the power, 2015-02-03 02:20:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/882123003/20001

[Theresa, 2015-02-03 02:50:32]

The CQ bit was unchecked by twellington@chromium.org

[Theresa, 2015-02-03 17:20:53]

The CQ bit was checked by twellington@chromium.org

[commit-bot: I haz the power, 2015-02-03 17:21:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/882123003/20001

[commit-bot: I haz the power, 2015-02-03 17:22:23]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-02-03 17:23:21]

Patchset 2 (id:??) landed as
https://crrev.com/5c9c165ecc87231b47ef3f93f63a563c7e966039
Cr-Commit-Position: refs/heads/master@{#314355}