Update affiliated web credentials when Android credentials are updated.

Update affiliated web credentials when Android credentials are updated.

BUG=476526
Committed: https://crrev.com/07307dd962b110bd2d90521059b4939e6dc7491f
Cr-Commit-Position: refs/heads/master@{#326019}

[engedy, 2015-04-17 17:33:21]

Patchset #1 (id:1) has been deleted

[engedy, 2015-04-17 17:34:07]

engedy@chromium.org changed reviewers:
+ mkwst@chromium.org, vasilii@chromium.org

[engedy, 2015-04-17 17:39:32]

@Mike, @Vasilii: Could you please take a preliminary look?

What's left to do is:
  * Detailed tests for helper methods.
  * Make the whole thing controllable by an experiment.
  * Potentially implement reverse direction of updates so the current direction
becomes more reliable too.

[Mike West, 2015-04-20 08:17:29]

I need some help understanding the big PasswordStoreTest. Ping me when you have
a few minutes. :)

https://codereview.chromium.org/1066543004/diff/20001/components/password_man...
File components/password_manager/core/browser/affiliated_match_helper.cc
(right):

https://codereview.chromium.org/1066543004/diff/20001/components/password_man...
components/password_manager/core/browser/affiliated_match_helper.cc:80: return;
If you really don't expect this to happen, and it's really bad if it does, why
not CHECK?

https://codereview.chromium.org/1066543004/diff/20001/components/password_man...
components/password_manager/core/browser/affiliated_match_helper.cc:156:
affiliated_realms.push_back(affiliated_facet.canonical_spec() + "/");
This seems like a strange difference between facets and realms. :/

https://codereview.chromium.org/1066543004/diff/20001/components/password_man...
File components/password_manager/core/browser/password_store_sync.h (right):

https://codereview.chromium.org/1066543004/diff/20001/components/password_man...
components/password_manager/core/browser/password_store_sync.h:39: virtual
PasswordStoreChangeList RemoveLoginSync(
Nit: Next time, you could split this out into a separate CL to clean up the
diff.

https://codereview.chromium.org/1066543004/diff/20001/components/password_man...
File components/password_manager/core/browser/password_store_unittest.cc
(right):

https://codereview.chromium.org/1066543004/diff/20001/components/password_man...
components/password_manager/core/browser/password_store_unittest.cc:94: //
|GetAffiliatedWebRealms|, and will cause the result callback supplied to
Nit: s/GetAffiliatedWebRealms/expected_android_form/ ?

https://codereview.chromium.org/1066543004/diff/20001/components/password_man...
components/password_manager/core/browser/password_store_unittest.cc:316:
kTestPSLMatchingWebRealm,
Nit: You could have done this cleanup in a separate CL as well.

https://codereview.chromium.org/1066543004/diff/20001/components/password_man...
components/password_manager/core/browser/password_store_unittest.cc:343:
store->SetAffiliatedMatchHelper(make_scoped_ptr(mock_helper));
What's the impact of moving this down?

https://codereview.chromium.org/1066543004/diff/20001/components/password_man...
components/password_manager/core/browser/password_store_unittest.cc:439:
store->SetAffiliatedMatchHelper(make_scoped_ptr(mock_helper));
Ditto?

https://codereview.chromium.org/1066543004/diff/20001/components/password_man...
components/password_manager/core/browser/password_store_unittest.cc:510: //
Although the password is different, it should be updated regardless.
The password is the same as [1]? Or do you mean different from [0]?

https://codereview.chromium.org/1066543004/diff/20001/components/password_man...
components/password_manager/core/browser/password_store_unittest.cc:586: // As
the test interacts with the (Update|Add)LoginSync methods, only the
"Chrome Sync" and "Synchronous" are a bit confusing here. :/

https://codereview.chromium.org/1066543004/diff/20001/components/password_man...
components/password_manager/core/browser/password_store_unittest.cc:586: // As
the test interacts with the (Update|Add)LoginSync methods, only the
"Chrome Sync" and "Synchronous" are a bit confusing here. :/

[engedy, 2015-04-20 09:30:11]

https://codereview.chromium.org/1066543004/diff/20001/components/password_man...
File components/password_manager/core/browser/affiliated_match_helper.cc
(right):

https://codereview.chromium.org/1066543004/diff/20001/components/password_man...
components/password_manager/core/browser/affiliated_match_helper.cc:80: return;
On 2015/04/20 08:17:28, Mike West wrote:
> If you really don't expect this to happen, and it's really bad if it does, why
> not CHECK?

My reasoning was that there is no risk in continuing here. We are not in an
undefined/ill-defined state that we did not expect and in which all bets are
off; there is no security issue here. But perhaps I can change this to be more
in line with GetAffiliatedAndroidRealms(). WDYT?

https://codereview.chromium.org/1066543004/diff/20001/components/password_man...
components/password_manager/core/browser/affiliated_match_helper.cc:156:
affiliated_realms.push_back(affiliated_facet.canonical_spec() + "/");
On 2015/04/20 08:17:28, Mike West wrote:
> This seems like a strange difference between facets and realms. :/

Acknowledged.

https://codereview.chromium.org/1066543004/diff/20001/components/password_man...
File components/password_manager/core/browser/password_store_sync.h (right):

https://codereview.chromium.org/1066543004/diff/20001/components/password_man...
components/password_manager/core/browser/password_store_sync.h:39: virtual
PasswordStoreChangeList RemoveLoginSync(
On 2015/04/20 08:17:28, Mike West wrote:
> Nit: Next time, you could split this out into a separate CL to clean up the
> diff.

Agreed, I just wanted to minimize the number of CLs.

https://codereview.chromium.org/1066543004/diff/20001/components/password_man...
File components/password_manager/core/browser/password_store_unittest.cc
(right):

https://codereview.chromium.org/1066543004/diff/20001/components/password_man...
components/password_manager/core/browser/password_store_unittest.cc:94: //
|GetAffiliatedWebRealms|, and will cause the result callback supplied to
On 2015/04/20 08:17:28, Mike West wrote:
> Nit: s/GetAffiliatedWebRealms/expected_android_form/ ?

Done.

https://codereview.chromium.org/1066543004/diff/20001/components/password_man...
components/password_manager/core/browser/password_store_unittest.cc:316:
kTestPSLMatchingWebRealm,
On 2015/04/20 08:17:28, Mike West wrote:
> Nit: You could have done this cleanup in a separate CL as well.

Agreed, but I would like to minimize the number of CLs this time.

https://codereview.chromium.org/1066543004/diff/20001/components/password_man...
components/password_manager/core/browser/password_store_unittest.cc:343:
store->SetAffiliatedMatchHelper(make_scoped_ptr(mock_helper));
On 2015/04/20 08:17:28, Mike West wrote:
> What's the impact of moving this down?

See below.

https://codereview.chromium.org/1066543004/diff/20001/components/password_man...
components/password_manager/core/browser/password_store_unittest.cc:439:
store->SetAffiliatedMatchHelper(make_scoped_ptr(mock_helper));
On 2015/04/20 08:17:28, Mike West wrote:
> Ditto?

This was to prevent the presently implemented updates taking place when setting
up the test data, but now I have realized that this is not going to be an issue
here.

https://codereview.chromium.org/1066543004/diff/20001/components/password_man...
components/password_manager/core/browser/password_store_unittest.cc:510: //
Although the password is different, it should be updated regardless.
On 2015/04/20 08:17:28, Mike West wrote:
> The password is the same as [1]? Or do you mean different from [0]?

Oh yes, the above should have been kTestOldPassword. Fixed and clarified in
comment.

https://codereview.chromium.org/1066543004/diff/20001/components/password_man...
components/password_manager/core/browser/password_store_unittest.cc:586: // As
the test interacts with the (Update|Add)LoginSync methods, only the
On 2015/04/20 08:17:28, Mike West wrote:
> "Chrome Sync" and "Synchronous" are a bit confusing here. :/

Dup.

https://codereview.chromium.org/1066543004/diff/20001/components/password_man...
components/password_manager/core/browser/password_store_unittest.cc:586: // As
the test interacts with the (Update|Add)LoginSync methods, only the
On 2015/04/20 08:17:28, Mike West wrote:
> "Chrome Sync" and "Synchronous" are a bit confusing here. :/

Actually, both interpretations are correct: they are both the synchronous
versions of these methods, and the ones used by Sync. :-)

Jokes aside, happy to rename, though. Do you have any good suggestions?

[Mike West, 2015-04-20 10:54:57]

Looks good so far. Ping again when you've got the other bits finished that you'd
like to cram into this patch. :)

https://codereview.chromium.org/1066543004/diff/20001/components/password_man...
File components/password_manager/core/browser/affiliated_match_helper.cc
(right):

https://codereview.chromium.org/1066543004/diff/20001/components/password_man...
components/password_manager/core/browser/affiliated_match_helper.cc:80: return;
On 2015/04/20 09:30:10, engedy wrote:
> On 2015/04/20 08:17:28, Mike West wrote:
> > If you really don't expect this to happen, and it's really bad if it does,
why
> > not CHECK?
> 
> My reasoning was that there is no risk in continuing here. We are not in an
> undefined/ill-defined state that we did not expect and in which all bets are
> off; there is no security issue here. But perhaps I can change this to be more
> in line with GetAffiliatedAndroidRealms(). WDYT?

I guess my point was
https://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC...

```
NOTREACHED();
return;
```

is explicitly forbidden. Just CHECK or DCHECK. :)

https://codereview.chromium.org/1066543004/diff/20001/components/password_man...
File components/password_manager/core/browser/password_store_unittest.cc
(right):

https://codereview.chromium.org/1066543004/diff/20001/components/password_man...
components/password_manager/core/browser/password_store_unittest.cc:586: // As
the test interacts with the (Update|Add)LoginSync methods, only the
On 2015/04/20 09:30:11, engedy wrote:
> Actually, both interpretations are correct: they are both the synchronous
> versions of these methods, and the ones used by Sync. :-)
> 
> Jokes aside, happy to rename, though. Do you have any good suggestions?

Not really. If you come up with anything, you can rename in a followup CL.

[engedy, 2015-04-20 12:46:21]

I have added the missing unit test, and made the whole thing contingent on a
variations parameter.

https://codereview.chromium.org/1066543004/diff/20001/components/password_man...
File components/password_manager/core/browser/affiliated_match_helper.cc
(right):

https://codereview.chromium.org/1066543004/diff/20001/components/password_man...
components/password_manager/core/browser/affiliated_match_helper.cc:80: return;
On 2015/04/20 10:54:57, Mike West wrote:
> On 2015/04/20 09:30:10, engedy wrote:
> > On 2015/04/20 08:17:28, Mike West wrote:
> > > If you really don't expect this to happen, and it's really bad if it does,
> why
> > > not CHECK?
> > 
> > My reasoning was that there is no risk in continuing here. We are not in an
> > undefined/ill-defined state that we did not expect and in which all bets are
> > off; there is no security issue here. But perhaps I can change this to be
more
> > in line with GetAffiliatedAndroidRealms(). WDYT?
> 
> I guess my point was
>
https://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC...
> 
> ```
> NOTREACHED();
> return;
> ```
> 
> is explicitly forbidden. Just CHECK or DCHECK. :)

I would argue that the situation here is similar to the exceptional case when
there are security implications, but I am just as happy as refactoring. PTAL.

[vasilii, 2015-04-20 13:04:08]

Did you consider moving the logic out of PasswordStore? You just need to observe
when Sync pushes something.

https://codereview.chromium.org/1066543004/diff/40001/components/password_man...
File components/password_manager/core/browser/password_store.cc (right):

https://codereview.chromium.org/1066543004/diff/40001/components/password_man...
components/password_manager/core/browser/password_store.cc:446:
NotifyLoginsChanged(UpdateLoginImpl(*web_login));
You can call NotifyLoginsChanged just once instead of for every credential.

https://codereview.chromium.org/1066543004/diff/40001/components/password_man...
File components/password_manager/core/browser/password_store_unittest.cc
(right):

https://codereview.chromium.org/1066543004/diff/40001/components/password_man...
components/password_manager/core/browser/password_store_unittest.cc:604: for
(bool test_remove_and_add_login : kTestRemoveAndAddLogin) {
Why not making this a test parameter?

[engedy, 2015-04-20 13:50:43]

Yes, I initially wanted to implement the logic in AffiliatedMatchHelper.

However, the showstopper there was the need to check the old password value (to
see if it was changed), which I could no longer do in the observers, as the
value had already been overwritten at that point.

If you have ideas about alternatives, please do let me know.

https://codereview.chromium.org/1066543004/diff/40001/components/password_man...
File components/password_manager/core/browser/password_store.cc (right):

https://codereview.chromium.org/1066543004/diff/40001/components/password_man...
components/password_manager/core/browser/password_store.cc:446:
NotifyLoginsChanged(UpdateLoginImpl(*web_login));
On 2015/04/20 13:04:08, vasilii wrote:
> You can call NotifyLoginsChanged just once instead of for every credential.

Done.

https://codereview.chromium.org/1066543004/diff/40001/components/password_man...
File components/password_manager/core/browser/password_store_unittest.cc
(right):

https://codereview.chromium.org/1066543004/diff/40001/components/password_man...
components/password_manager/core/browser/password_store_unittest.cc:604: for
(bool test_remove_and_add_login : kTestRemoveAndAddLogin) {
On 2015/04/20 13:04:08, vasilii wrote:
> Why not making this a test parameter?

The parameter only makes sense for this test, not the entire suite of tests. As
far as I understand, I would need to define a separate test fixture for this
then. I would rather not do that.

[vasilii, 2015-04-20 15:02:07]

https://codereview.chromium.org/1066543004/diff/100001/components/password_ma...
File components/password_manager/core/browser/affiliation_utils.cc (right):

https://codereview.chromium.org/1066543004/diff/100001/components/password_ma...
components/password_manager/core/browser/affiliation_utils.cc:310: const
std::string update_enabled = variations::GetVariationParamValue(
Move the line down where you use it.

https://codereview.chromium.org/1066543004/diff/100001/components/password_ma...
File components/password_manager/core/browser/password_store.cc (right):

https://codereview.chromium.org/1066543004/diff/100001/components/password_ma...
components/password_manager/core/browser/password_store.cc:341: for (auto
it_candidate = candidates.begin(); it_candidate != candidates.end();
Why not the new style loop?

https://codereview.chromium.org/1066543004/diff/100001/components/password_ma...
File components/password_manager/core/browser/password_store.h (right):

https://codereview.chromium.org/1066543004/diff/100001/components/password_ma...
components/password_manager/core/browser/password_store.h:351: bool
is_propagating_password_changes_to_web_enabled_;
Can't you just set |affiliated_match_helper_| to NULL if it's not enabled?

[engedy, 2015-04-20 15:14:05]

https://codereview.chromium.org/1066543004/diff/100001/components/password_ma...
File components/password_manager/core/browser/affiliation_utils.cc (right):

https://codereview.chromium.org/1066543004/diff/100001/components/password_ma...
components/password_manager/core/browser/affiliation_utils.cc:310: const
std::string update_enabled = variations::GetVariationParamValue(
On 2015/04/20 15:02:07, vasilii wrote:
> Move the line down where you use it.

I have added a comment to clarify that this has side-effects, so it must be
called before the checks below.

https://codereview.chromium.org/1066543004/diff/100001/components/password_ma...
File components/password_manager/core/browser/password_store.cc (right):

https://codereview.chromium.org/1066543004/diff/100001/components/password_ma...
components/password_manager/core/browser/password_store.cc:341: for (auto
it_candidate = candidates.begin(); it_candidate != candidates.end();
On 2015/04/20 15:02:07, vasilii wrote:
> Why not the new style loop?

I need the iterator to be able to weak_erase the element in line 351. But
perhaps I am overlooking something and this can be done without the iterator?

https://codereview.chromium.org/1066543004/diff/100001/components/password_ma...
File components/password_manager/core/browser/password_store.h (right):

https://codereview.chromium.org/1066543004/diff/100001/components/password_ma...
components/password_manager/core/browser/password_store.h:351: bool
is_propagating_password_changes_to_web_enabled_;
On 2015/04/20 15:02:07, vasilii wrote:
> Can't you just set |affiliated_match_helper_| to NULL if it's not enabled?

We need this to be toggled separately from the main feature. Later on, we might
want to have the affiliation-based matching, but not the propagation. I have
added a comment to clarify, and updated helper function naming.

[vasilii, 2015-04-20 16:25:19]

https://codereview.chromium.org/1066543004/diff/100001/components/password_ma...
File components/password_manager/core/browser/password_store.cc (right):

https://codereview.chromium.org/1066543004/diff/100001/components/password_ma...
components/password_manager/core/browser/password_store.cc:341: for (auto
it_candidate = candidates.begin(); it_candidate != candidates.end();
On 2015/04/20 15:14:05, engedy wrote:
> On 2015/04/20 15:02:07, vasilii wrote:
> > Why not the new style loop?
> 
> I need the iterator to be able to weak_erase the element in line 351. But
> perhaps I am overlooking something and this can be done without the iterator?

Assign NULL to it instead of weak_erase?

https://codereview.chromium.org/1066543004/diff/140001/components/password_ma...
File components/password_manager/core/browser/affiliation_utils.cc (right):

https://codereview.chromium.org/1066543004/diff/140001/components/password_ma...
components/password_manager/core/browser/affiliation_utils.cc:311: // UMA
reports the correct group.
Can you elaborate?

https://codereview.chromium.org/1066543004/diff/140001/components/password_ma...
File components/password_manager/core/browser/password_store.cc (right):

https://codereview.chromium.org/1066543004/diff/140001/components/password_ma...
components/password_manager/core/browser/password_store.cc:241:
scoped_ptr<autofill::PasswordForm> old_form(GetLoginImpl(form));
You don't need to execute this if
is_propagating_password_changes_to_web_credentials_enabled_ is false

[engedy, 2015-04-20 18:31:59]

https://codereview.chromium.org/1066543004/diff/100001/components/password_ma...
File components/password_manager/core/browser/password_store.cc (right):

https://codereview.chromium.org/1066543004/diff/100001/components/password_ma...
components/password_manager/core/browser/password_store.cc:341: for (auto
it_candidate = candidates.begin(); it_candidate != candidates.end();
On 2015/04/20 16:25:19, vasilii wrote:
> On 2015/04/20 15:14:05, engedy wrote:
> > On 2015/04/20 15:02:07, vasilii wrote:
> > > Why not the new style loop?
> > 
> > I need the iterator to be able to weak_erase the element in line 351. But
> > perhaps I am overlooking something and this can be done without the
iterator?
> 
> Assign NULL to it instead of weak_erase?

Oh, yes, right. Done.

https://codereview.chromium.org/1066543004/diff/140001/components/password_ma...
File components/password_manager/core/browser/affiliation_utils.cc (right):

https://codereview.chromium.org/1066543004/diff/140001/components/password_ma...
components/password_manager/core/browser/affiliation_utils.cc:311: // UMA
reports the correct group.
On 2015/04/20 16:25:19, vasilii wrote:
> Can you elaborate?

Done.

https://codereview.chromium.org/1066543004/diff/140001/components/password_ma...
File components/password_manager/core/browser/password_store.cc (right):

https://codereview.chromium.org/1066543004/diff/140001/components/password_ma...
components/password_manager/core/browser/password_store.cc:241:
scoped_ptr<autofill::PasswordForm> old_form(GetLoginImpl(form));
On 2015/04/20 16:25:19, vasilii wrote:
> You don't need to execute this if
> is_propagating_password_changes_to_web_credentials_enabled_ is false

Correct, but |propagating_password_changes_to_web_credentials_enabled_| should
be used only from the UI thread. I do not expect any race conditions, but it is
somewhat hard to argue why such a solution would be correct.

Also, it should not matter all that much performance-wise, as this code path is
exercised only for Android credentials, and
|is_propagating_password_changes_to_web_credentials_enabled_| will almost always
be true -- it is used as a kill-switch in case we need to disable this
functionality.

[Mike West, 2015-04-21 08:55:21]

LGTM, thanks for making the changes we discussed. I'm happy with this, but
please wait until Vasilii's feeling good about it before landing.

[vasilii, 2015-04-21 09:20:19]

lgtm

https://codereview.chromium.org/1066543004/diff/180001/components/password_ma...
File components/password_manager/core/browser/affiliation_utils_unittest.cc
(right):

https://codereview.chromium.org/1066543004/diff/180001/components/password_ma...
components/password_manager/core/browser/affiliation_utils_unittest.cc:20: const
char kFieldTrialName[] = "AffiliationBasedMatching";
Alphabet order.

https://codereview.chromium.org/1066543004/diff/180001/components/password_ma...
File components/password_manager/core/browser/password_store.h (right):

https://codereview.chromium.org/1066543004/diff/180001/components/password_ma...
components/password_manager/core/browser/password_store.h:314:
scoped_ptr<autofill::PasswordForm> GetLoginImpl(
For these functions mention the thread they are executed on.

[engedy, 2015-04-21 09:42:25]

Mike, Vasilii, thank you both for reviewing this!

https://codereview.chromium.org/1066543004/diff/180001/components/password_ma...
File components/password_manager/core/browser/affiliation_utils_unittest.cc
(right):

https://codereview.chromium.org/1066543004/diff/180001/components/password_ma...
components/password_manager/core/browser/affiliation_utils_unittest.cc:20: const
char kFieldTrialName[] = "AffiliationBasedMatching";
On 2015/04/21 09:20:18, vasilii wrote:
> Alphabet order.

Done.

https://codereview.chromium.org/1066543004/diff/180001/components/password_ma...
File components/password_manager/core/browser/password_store.h (right):

https://codereview.chromium.org/1066543004/diff/180001/components/password_ma...
components/password_manager/core/browser/password_store.h:314:
scoped_ptr<autofill::PasswordForm> GetLoginImpl(
On 2015/04/21 09:20:18, vasilii wrote:
> For these functions mention the thread they are executed on.

Done.

[engedy, 2015-04-21 11:11:30]

The CQ bit was checked by engedy@chromium.org

[engedy, 2015-04-21 11:11:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
vasilii@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1066543004/#ps200001
(title: "Addressed comments to Patch Set 9 from vasilii@.")

[commit-bot: I haz the power, 2015-04-21 11:12:00]

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

[commit-bot: I haz the power, 2015-04-21 11:20:10]

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

[commit-bot: I haz the power, 2015-04-21 11:20:11]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[engedy, 2015-04-21 11:28:09]

The CQ bit was checked by engedy@chromium.org

[engedy, 2015-04-21 11:28:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
vasilii@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1066543004/#ps220001
(title: "Use ThreadTaskRunnerHandle instead of now-banned MessageLoopProxy.")

[commit-bot: I haz the power, 2015-04-21 11:28:24]

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

[commit-bot: I haz the power, 2015-04-21 11:36:02]

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

[commit-bot: I haz the power, 2015-04-21 11:36:03]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[engedy, 2015-04-21 12:02:31]

The CQ bit was checked by engedy@chromium.org

[engedy, 2015-04-21 12:02:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
vasilii@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1066543004/#ps240001
(title: "Fix the correct MessageLoopProxy occurrence this time.")

[commit-bot: I haz the power, 2015-04-21 12:02:50]

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

[commit-bot: I haz the power, 2015-04-21 12:59:41]

Committed patchset #12 (id:240001)

[commit-bot: I haz the power, 2015-04-21 13:00:42]

Patchset 12 (id:??) landed as
https://crrev.com/07307dd962b110bd2d90521059b4939e6dc7491f
Cr-Commit-Position: refs/heads/master@{#326019}