Implement PasswordStore::GetLoginsForSameOrganizationName.

Implement PasswordStore::GetLoginsForSameOrganizationName.

The method returns all stored credentials with SCHEME_HTTP that have an origin
whose organization-identifying name -- that is, the first domain name label
below the effective TLD -- matches that of a given origin.

For example, the organization-identifying name of "https://foo.example.org" is
`example`, and logins will be returned for "http://bar.example.co.uk", but not
for "http://notexample.com" or "https://example.foo.com".

BUG=720599
Review-Url: https://codereview.chromium.org/2899083004
Cr-Commit-Position: refs/heads/master@{#475322}
Committed: https://chromium.googlesource.com/chromium/src/+/05971b1368780700908f226db1ff99837463abf1

[engedy, 2017-05-24 12:09:03]

Description was changed from

==========
Implement PasswordStore::GetLoginsForSameOrganizationName.
.
It returns all stored credentials with SCHEME_HTTP that have an origin whose
organization-identifying name -- that is, the first domain name label below the
effective TLD -- matches that of |signon_realm|.

For example, the organization-identifying name of "https://foo.example.org" is
`example`, and logins will be returned for "http://bar.example.co.uk", but not
for "http://notexample.com" or "https://example.foo.com".

BUG=720599
==========

to

==========
Implement PasswordStore::GetLoginsForSameOrganizationName.

The method returns all stored credentials with SCHEME_HTTP that have an origin
whose organization-identifying name -- that is, the first domain name label
below the effective TLD -- matches that of |signon_realm|.

For example, the organization-identifying name of "https://foo.example.org" is
`example`, and logins will be returned for "http://bar.example.co.uk", but not
for "http://notexample.com" or "https://example.foo.com".

BUG=720599
==========

[engedy, 2017-05-24 12:09:03]

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

[engedy, 2017-05-24 12:56:33]

Vasilii, please take a look.

Less than 100 lines of production code changed, most of the CL are the tests.

[engedy, 2017-05-24 13:01:15]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 13:01:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 13:19:26]

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

[commit-bot: I haz the power, 2017-05-24 13:19:27]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[engedy, 2017-05-24 13:33:55]

Looks like I'm hitting https://gcc.gnu.org/bugzilla/show_bug.cgi?id=49132 on
Android. Wouldn't it be nice if used something newer than a 3-year old version
of gcc. :-(

[engedy, 2017-05-24 13:59:44]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[engedy, 2017-05-24 13:59:55]

... and same on Windows. Should be fixed now.

[commit-bot: I haz the power, 2017-05-24 13:59:59]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 14:16:24]

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

[commit-bot: I haz the power, 2017-05-24 14:16:25]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[engedy, 2017-05-24 14:55:16]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 14:55:50]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 15:42:45]

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

[commit-bot: I haz the power, 2017-05-24 15:42:45]

Dry run: This issue passed the CQ dry run.

[engedy, 2017-05-24 16:05:26]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 16:05:59]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 16:50:26]

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

[commit-bot: I haz the power, 2017-05-24 16:50:27]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[engedy, 2017-05-24 17:38:46]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 17:39:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vasilii, 2017-05-24 17:48:39]

Looks like the bots don't like you today :)

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
File components/password_manager/core/browser/login_database.cc (right):

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
components/password_manager/core/browser/login_database.cc:1156: const char*
kRegexpSchemeAndSubdomains = "^https?://([\\w+%-]+\\.)*";
I think the convention is to use static constexpr char[]
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/static...

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
components/password_manager/core/browser/login_database.cc:1160:
kRegexpDotAndEffectiveTLD;
Is this logic better than "LIKE %.organization_name.%"?

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
components/password_manager/core/browser/login_database.cc:1173:
GetOrganizationIdentifyingName(GURL(form->signon_realm));
candidate_signon_realm

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
File components/password_manager/core/browser/login_database_unittest.cc
(right):

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
components/password_manager/core/browser/login_database_unittest.cc:822: false},
actually this should be true. Or we don't care about federated credentials
because they are not filled?

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
components/password_manager/core/browser/login_database_unittest.cc:840:
EXPECT_EQ(test_case.expected_matches_itself ? 1u : 0u,
Doesn't test_case.expected_matches_itself just work?

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
components/password_manager/core/browser/login_database_unittest.cc:913:
{"https://example.com/", "http://www.foo;bar.com", false},
Also seems useful
 {"https://example.com/", "example", false},

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
File components/password_manager/core/browser/password_store.h (right):

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
components/password_manager/core/browser/password_store.h:196: virtual void
GetLoginsForSameOrganizationName(
Why virtual?

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
components/password_manager/core/browser/password_store.h:395:
FillLoginsForSameOrganizationName(const std::string signon_realm) = 0;
const std::string&?

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

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
components/password_manager/core/browser/password_store_unittest.cc:64:
"https:/example.appspot.com/origin";
https://

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
components/password_manager/core/browser/password_store_unittest.cc:890:
base::WrapUnique(new LoginDatabase(test_login_db_file_path()))));
I think we prefer MakeUnique

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
components/password_manager/core/browser/password_store_unittest.cc:899:
base::RunLoop().RunUntilIdle();
What's the point?

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
components/password_manager/core/browser/password_store_unittest.cc:902:
static_assert(4u <= arraysize(kTestCredentials), "Too few test forms.");
What about specifying 2 arrays in the beginning of the test (expected +
unexpected)? Thus, you don't need magic numbers here.

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
components/password_manager/core/browser/password_store_unittest.cc:914:
store->ShutdownOnUIThread();
I think it's overly optimistic to expect password store to return something
after ShutdownOnUIThread() is called. Thus, I'd put RunUntilIdle() before
ShutdownOnUIThread().
Regarding RunUntilIdle() after ShutdownOnUIThread() I don't have a strong
feeling. I think the test will pass without it too but it doesn't harm.

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
File components/password_manager/core/browser/psl_matching_helper_unittest.cc
(right):

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
components/password_manager/core/browser/psl_matching_helper_unittest.cc:398:
bool expect_invalid_url;
Why are you testing GURL::is_valid() here?

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
components/password_manager/core/browser/psl_matching_helper_unittest.cc:417:
{"https://www.sz\xc3\xb3t\xc3\xa1r.appspot.com", "xn--sztr-7na0i"},
What is happening here?

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
components/password_manager/core/browser/psl_matching_helper_unittest.cc:468: if
(chr == '.' || isalnum(chr))
Wouldn't the test pass without this?

[engedy, 2017-05-24 19:19:56]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 19:20:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[engedy, 2017-05-24 19:26:00]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[engedy, 2017-05-24 19:26:08]

Patchset #8 (id:140001) has been deleted

[commit-bot: I haz the power, 2017-05-24 19:26:19]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[engedy, 2017-05-24 19:27:10]

engedy@chromium.org changed reviewers:
+ kolos@chromium.org

[engedy, 2017-05-24 19:27:13]

Thanks for the thorough review! Comments addressed % 4 questions below.

@Maxim, there is also a question for you below.

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
File components/password_manager/core/browser/login_database.cc (right):

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
components/password_manager/core/browser/login_database.cc:1156: const char*
kRegexpSchemeAndSubdomains = "^https?://([\\w+%-]+\\.)*";
On 2017/05/24 17:48:38, vasilii wrote:
> I think the convention is to use static constexpr char[]
>
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/static...

Done.

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
components/password_manager/core/browser/login_database.cc:1160:
kRegexpDotAndEffectiveTLD;
On 2017/05/24 17:48:38, vasilii wrote:
> Is this logic better than "LIKE %.organization_name.%"?

The pattern cannot have a '.' at the beginning because "http://example.com" must
also match the query string "example". Without the dot, "http://notexample.com"
and the like would also match "example", and would have to be filtered it out
below. Given that we will call this method for every observed form, I'd try to
be relatively efficient here.

The other disadvantage of the LIKE expression is that Android and federated
logins, where `example` is in the APK name or the |federation_url|,
respectively, will also match the LIKE condition.

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
components/password_manager/core/browser/login_database.cc:1173:
GetOrganizationIdentifyingName(GURL(form->signon_realm));
On 2017/05/24 17:48:38, vasilii wrote:
> candidate_signon_realm

Done.

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
File components/password_manager/core/browser/login_database_unittest.cc
(right):

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
components/password_manager/core/browser/login_database_unittest.cc:822: false},
On 2017/05/24 17:48:38, vasilii wrote:
> actually this should be true. Or we don't care about federated credentials
> because they are not filled?

That was my thinking, but I can be convinced otherwise. Alternatively, we could
record them somewhere else.

@Maxim, what do you think?

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
components/password_manager/core/browser/login_database_unittest.cc:840:
EXPECT_EQ(test_case.expected_matches_itself ? 1u : 0u,
On 2017/05/24 17:48:38, vasilii wrote:
> Doesn't test_case.expected_matches_itself just work?

It happens to work, but I think it's less clear to the reader. What can they
expect from the EXPECT_EQ macro? Does it perform normal integral promotion, or
will it convert the size() to bool? (In the latter case possibly hiding errors
where there are 2 elements, not one.)

What do you think?

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
components/password_manager/core/browser/login_database_unittest.cc:913:
{"https://example.com/", "http://www.foo;bar.com", false},
On 2017/05/24 17:48:38, vasilii wrote:
> Also seems useful
>  {"https://example.com/", "example", false},

Done.

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
File components/password_manager/core/browser/password_store.h (right):

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
components/password_manager/core/browser/password_store.h:196: virtual void
GetLoginsForSameOrganizationName(
On 2017/05/24 17:48:38, vasilii wrote:
> Why virtual?

Uhm, no reason. Removed.

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
components/password_manager/core/browser/password_store.h:395:
FillLoginsForSameOrganizationName(const std::string signon_realm) = 0;
On 2017/05/24 17:48:38, vasilii wrote:
> const std::string&?

Good catch. Fixed. All 13 occurrences.

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

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
components/password_manager/core/browser/password_store_unittest.cc:64:
"https:/example.appspot.com/origin";
On 2017/05/24 17:48:38, vasilii wrote:
> https://

Done.

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
components/password_manager/core/browser/password_store_unittest.cc:890:
base::WrapUnique(new LoginDatabase(test_login_db_file_path()))));
On 2017/05/24 17:48:38, vasilii wrote:
> I think we prefer MakeUnique

Yep. Done.

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
components/password_manager/core/browser/password_store_unittest.cc:899:
base::RunLoop().RunUntilIdle();
On 2017/05/24 17:48:38, vasilii wrote:
> What's the point?

I just followed suit with the other tests, but now that you mention it, it does
seem useless. Removed.

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
components/password_manager/core/browser/password_store_unittest.cc:902:
static_assert(4u <= arraysize(kTestCredentials), "Too few test forms.");
On 2017/05/24 17:48:38, vasilii wrote:
> What about specifying 2 arrays in the beginning of the test (expected +
> unexpected)? Thus, you don't need magic numbers here.

Good idea. Looks much better!

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
components/password_manager/core/browser/password_store_unittest.cc:914:
store->ShutdownOnUIThread();
On 2017/05/24 17:48:38, vasilii wrote:
> I think it's overly optimistic to expect password store to return something
> after ShutdownOnUIThread() is called. Thus, I'd put RunUntilIdle() before
> ShutdownOnUIThread().
> Regarding RunUntilIdle() after ShutdownOnUIThread() I don't have a strong
> feeling. I think the test will pass without it too but it doesn't harm.

Done.

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
File components/password_manager/core/browser/psl_matching_helper_unittest.cc
(right):

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
components/password_manager/core/browser/psl_matching_helper_unittest.cc:398:
bool expect_invalid_url;
On 2017/05/24 17:48:38, vasilii wrote:
> Why are you testing GURL::is_valid() here?

The intention was to convince readers that some of the below URLs are indeed
valid, but on second thought, this is already obvious from the fact that their
org-name is non-empty. Removed.

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
components/password_manager/core/browser/psl_matching_helper_unittest.cc:417:
{"https://www.sz\xc3\xb3t\xc3\xa1r.appspot.com", "xn--sztr-7na0i"},
On 2017/05/24 17:48:38, vasilii wrote:
> What is happening here?

Ah yes, added a comment to clarify. I had been reading `url_canon_unittest.cc`
before writing this, so the ugliness of this line didn't really register
anymore. :-)

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
components/password_manager/core/browser/psl_matching_helper_unittest.cc:468: if
(chr == '.' || isalnum(chr))
On 2017/05/24 17:48:39, vasilii wrote:
> Wouldn't the test pass without this?

How about this now?

[engedy, 2017-05-24 21:37:17]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 21:38:11]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 22:16:36]

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

[commit-bot: I haz the power, 2017-05-24 22:16:38]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[engedy, 2017-05-26 09:37:26]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 09:37:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-26 10:32:22]

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

[commit-bot: I haz the power, 2017-05-26 10:32:23]

Dry run: This issue passed the CQ dry run.

[vasilii, 2017-05-26 10:53:36]

I will have another full pass once addressed.

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
File components/password_manager/core/browser/login_database_unittest.cc
(right):

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
components/password_manager/core/browser/login_database_unittest.cc:822: false},
On 2017/05/24 19:27:12, engedy wrote:
> On 2017/05/24 17:48:38, vasilii wrote:
> > actually this should be true. Or we don't care about federated credentials
> > because they are not filled?
> 
> That was my thinking, but I can be convinced otherwise. Alternatively, we
could
> record them somewhere else.
> 
> @Maxim, what do you think?

I agree with your thinking

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
components/password_manager/core/browser/login_database_unittest.cc:840:
EXPECT_EQ(test_case.expected_matches_itself ? 1u : 0u,
On 2017/05/24 19:27:12, engedy wrote:
> On 2017/05/24 17:48:38, vasilii wrote:
> > Doesn't test_case.expected_matches_itself just work?
> 
> It happens to work, but I think it's less clear to the reader. What can they
> expect from the EXPECT_EQ macro? Does it perform normal integral promotion, or
> will it convert the size() to bool? (In the latter case possibly hiding errors
> where there are 2 elements, not one.)
> 
> What do you think?

If EXPECT_EQ goes crazy with converting the second parameter to bool then your
check will pass as well ;-)
Up to you. Assuming EXPECT_EQ just does "if (expected == actual)" we are safe.

https://codereview.chromium.org/2899083004/diff/210001/components/password_ma...
File components/password_manager/core/browser/login_database.cc (right):

https://codereview.chromium.org/2899083004/diff/210001/components/password_ma...
components/password_manager/core/browser/login_database.cc:1173:
GetOrganizationIdentifyingName(GURL(candidate_signon_realm));
candidate_signon_realm is already GURL.

https://codereview.chromium.org/2899083004/diff/210001/components/password_ma...
File components/password_manager/core/browser/psl_matching_helper_unittest.cc
(right):

https://codereview.chromium.org/2899083004/diff/210001/components/password_ma...
components/password_manager/core/browser/psl_matching_helper_unittest.cc:480:
base::StringPrintf("http://a%cb.hu/", base::ToLowerASCII((char)chr));
static_cast<>, no C-cast per style guide.

[engedy, 2017-05-26 10:58:41]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[engedy, 2017-05-26 10:58:43]

Please take a final look.

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
File components/password_manager/core/browser/login_database_unittest.cc
(right):

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
components/password_manager/core/browser/login_database_unittest.cc:822: false},
On 2017/05/26 10:53:36, vasilii wrote:
> On 2017/05/24 19:27:12, engedy wrote:
> > On 2017/05/24 17:48:38, vasilii wrote:
> > > actually this should be true. Or we don't care about federated credentials
> > > because they are not filled?
> > 
> > That was my thinking, but I can be convinced otherwise. Alternatively, we
> could
> > record them somewhere else.
> > 
> > @Maxim, what do you think?
> 
> I agree with your thinking

Acknowledged.

https://codereview.chromium.org/2899083004/diff/60001/components/password_man...
components/password_manager/core/browser/login_database_unittest.cc:840:
EXPECT_EQ(test_case.expected_matches_itself ? 1u : 0u,
On 2017/05/26 10:53:36, vasilii wrote:
> On 2017/05/24 19:27:12, engedy wrote:
> > On 2017/05/24 17:48:38, vasilii wrote:
> > > Doesn't test_case.expected_matches_itself just work?
> > 
> > It happens to work, but I think it's less clear to the reader. What can they
> > expect from the EXPECT_EQ macro? Does it perform normal integral promotion,
or
> > will it convert the size() to bool? (In the latter case possibly hiding
errors
> > where there are 2 elements, not one.)
> > 
> > What do you think?
> 
> If EXPECT_EQ goes crazy with converting the second parameter to bool then your
> check will pass as well ;-)
> Up to you. Assuming EXPECT_EQ just does "if (expected == actual)" we are safe.

Yeah, right. Shortened this.

https://codereview.chromium.org/2899083004/diff/210001/components/password_ma...
File components/password_manager/core/browser/login_database.cc (right):

https://codereview.chromium.org/2899083004/diff/210001/components/password_ma...
components/password_manager/core/browser/login_database.cc:1173:
GetOrganizationIdentifyingName(GURL(candidate_signon_realm));
On 2017/05/26 10:53:36, vasilii wrote:
> candidate_signon_realm is already GURL.

Done.

https://codereview.chromium.org/2899083004/diff/210001/components/password_ma...
File components/password_manager/core/browser/psl_matching_helper_unittest.cc
(right):

https://codereview.chromium.org/2899083004/diff/210001/components/password_ma...
components/password_manager/core/browser/psl_matching_helper_unittest.cc:480:
base::StringPrintf("http://a%cb.hu/", base::ToLowerASCII((char)chr));
On 2017/05/26 10:53:36, vasilii wrote:
> static_cast<>, no C-cast per style guide.

Done.

[commit-bot: I haz the power, 2017-05-26 10:58:48]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[engedy, 2017-05-26 11:15:28]

Description was changed from

==========
Implement PasswordStore::GetLoginsForSameOrganizationName.

The method returns all stored credentials with SCHEME_HTTP that have an origin
whose organization-identifying name -- that is, the first domain name label
below the effective TLD -- matches that of |signon_realm|.

For example, the organization-identifying name of "https://foo.example.org" is
`example`, and logins will be returned for "http://bar.example.co.uk", but not
for "http://notexample.com" or "https://example.foo.com".

BUG=720599
==========

to

==========
Implement PasswordStore::GetLoginsForSameOrganizationName.

The method returns all stored credentials with SCHEME_HTTP that have an origin
whose organization-identifying name -- that is, the first domain name label
below the effective TLD -- matches that of a given origin.

For example, the organization-identifying name of "https://foo.example.org" is
`example`, and logins will be returned for "http://bar.example.co.uk", but not
for "http://notexample.com" or "https://example.foo.com".

BUG=720599
==========

[commit-bot: I haz the power, 2017-05-26 11:38:55]

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

[commit-bot: I haz the power, 2017-05-26 11:38:56]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[engedy, 2017-05-26 12:00:02]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 12:00:19]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vasilii, 2017-05-26 12:47:55]

lgtm

https://codereview.chromium.org/2899083004/diff/250001/components/password_ma...
File components/password_manager/core/browser/login_database_unittest.cc
(right):

https://codereview.chromium.org/2899083004/diff/250001/components/password_ma...
components/password_manager/core/browser/login_database_unittest.cc:795: const
struct {
constexpr

https://codereview.chromium.org/2899083004/diff/250001/components/password_ma...
File components/password_manager/core/browser/password_store_unittest.cc
(right):

https://codereview.chromium.org/2899083004/diff/250001/components/password_ma...
components/password_manager/core/browser/password_store_unittest.cc:63: const
char kTestSameOrganizationNameWebOrigin[] =
Optionally: convert everything to constexpr.

https://codereview.chromium.org/2899083004/diff/250001/components/password_ma...
components/password_manager/core/browser/password_store_unittest.cc:858: static
const PasswordFormData kSameOrganizationCredentials[] = {
constexpr here and below

https://codereview.chromium.org/2899083004/diff/250001/components/password_ma...
File components/password_manager/core/browser/psl_matching_helper.cc (right):

https://codereview.chromium.org/2899083004/diff/250001/components/password_ma...
components/password_manager/core/browser/psl_matching_helper.cc:135: }
Optional: you can omit {}

https://codereview.chromium.org/2899083004/diff/250001/components/password_ma...
components/password_manager/core/browser/psl_matching_helper.cc:140: 0,
organization_and_registrar.size() - registrar_length - 1);
I suppose that "1" is for ".". Are we sure that it's always encoded like '.' and
not %2E? I assume 'yes' but it's worth checking

https://codereview.chromium.org/2899083004/diff/250001/components/password_ma...
File components/password_manager/core/browser/psl_matching_helper_unittest.cc
(right):

https://codereview.chromium.org/2899083004/diff/250001/components/password_ma...
components/password_manager/core/browser/psl_matching_helper_unittest.cc:396:
const struct {
constexpr

https://codereview.chromium.org/2899083004/diff/250001/components/password_ma...
components/password_manager/core/browser/psl_matching_helper_unittest.cc:422: //
    szotar = sz\xc3\xb3t\xc3\xa1r (UTF-8) = xn--sztr-7na0i (IDN)
I hope it's not a swear word ;-)

https://codereview.chromium.org/2899083004/diff/250001/components/password_ma...
components/password_manager/core/browser/psl_matching_helper_unittest.cc:470: //
canonicalized URL.
Would be cool to explain why we care about it and what the relation to
GetOrganizationIdentifyingName is.

https://codereview.chromium.org/2899083004/diff/250001/components/password_ma...
components/password_manager/core/browser/psl_matching_helper_unittest.cc:473:
char kExpectedNonAlphaNumChars[] = {'+', '-', '_', '.'};
constexpr?

[commit-bot: I haz the power, 2017-05-26 12:50:08]

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

[commit-bot: I haz the power, 2017-05-26 12:50:09]

Dry run: This issue passed the CQ dry run.

[kolos1, 2017-05-26 13:06:53]

LGTM. Thanks Balazs!

https://codereview.chromium.org/2899083004/diff/250001/components/password_ma...
File components/password_manager/core/browser/login_database.cc (right):

https://codereview.chromium.org/2899083004/diff/250001/components/password_ma...
components/password_manager/core/browser/login_database.cc:1156: static
constexpr char kRESchemeOrSubdomains[] = "^https?://([\\w+%-]+\\.)*";
kRESchemeAndSubdomains?

[engedy, 2017-05-29 08:40:32]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-29 08:40:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[engedy, 2017-05-29 08:41:10]

Thank you both for the thorough review!

https://codereview.chromium.org/2899083004/diff/250001/components/password_ma...
File components/password_manager/core/browser/login_database.cc (right):

https://codereview.chromium.org/2899083004/diff/250001/components/password_ma...
components/password_manager/core/browser/login_database.cc:1156: static
constexpr char kRESchemeOrSubdomains[] = "^https?://([\\w+%-]+\\.)*";
On 2017/05/26 13:06:53, kolos1 wrote:
> kRESchemeAndSubdomains?

Right, I changed the semantics and forgot to rename this. Done.

https://codereview.chromium.org/2899083004/diff/250001/components/password_ma...
File components/password_manager/core/browser/login_database_unittest.cc
(right):

https://codereview.chromium.org/2899083004/diff/250001/components/password_ma...
components/password_manager/core/browser/login_database_unittest.cc:795: const
struct {
On 2017/05/26 12:47:55, vasilii wrote:
> constexpr

Done.

https://codereview.chromium.org/2899083004/diff/250001/components/password_ma...
File components/password_manager/core/browser/password_store_unittest.cc
(right):

https://codereview.chromium.org/2899083004/diff/250001/components/password_ma...
components/password_manager/core/browser/password_store_unittest.cc:63: const
char kTestSameOrganizationNameWebOrigin[] =
On 2017/05/26 12:47:55, vasilii wrote:
> Optionally: convert everything to constexpr.

Done.

https://codereview.chromium.org/2899083004/diff/250001/components/password_ma...
components/password_manager/core/browser/password_store_unittest.cc:858: static
const PasswordFormData kSameOrganizationCredentials[] = {
On 2017/05/26 12:47:55, vasilii wrote:
> constexpr here and below

Done.

https://codereview.chromium.org/2899083004/diff/250001/components/password_ma...
File components/password_manager/core/browser/psl_matching_helper.cc (right):

https://codereview.chromium.org/2899083004/diff/250001/components/password_ma...
components/password_manager/core/browser/psl_matching_helper.cc:135: }
On 2017/05/26 12:47:55, vasilii wrote:
> Optional: you can omit {}

Let me keep these, I like them a lot.

https://codereview.chromium.org/2899083004/diff/250001/components/password_ma...
components/password_manager/core/browser/psl_matching_helper.cc:140: 0,
organization_and_registrar.size() - registrar_length - 1);
On 2017/05/26 12:47:55, vasilii wrote:
> I suppose that "1" is for ".". Are we sure that it's always encoded like '.'
and
> not %2E? I assume 'yes' but it's worth checking

Yes, '.' is one of the characters that will always be percent-decoded during URL
canonicalization. PSLMatchingUtilsTest.GetOrganizationIdentifyingName has a test
case to enforce this.

https://codereview.chromium.org/2899083004/diff/250001/components/password_ma...
File components/password_manager/core/browser/psl_matching_helper_unittest.cc
(right):

https://codereview.chromium.org/2899083004/diff/250001/components/password_ma...
components/password_manager/core/browser/psl_matching_helper_unittest.cc:396:
const struct {
On 2017/05/26 12:47:55, vasilii wrote:
> constexpr

Done.

https://codereview.chromium.org/2899083004/diff/250001/components/password_ma...
components/password_manager/core/browser/psl_matching_helper_unittest.cc:422: //
    szotar = sz\xc3\xb3t\xc3\xa1r (UTF-8) = xn--sztr-7na0i (IDN)
On 2017/05/26 12:47:55, vasilii wrote:
> I hope it's not a swear word ;-)

It means `dictionary` in Hungarian. Didn't do extensive research to ensure it's
not a swear word in other languages... :-)

https://codereview.chromium.org/2899083004/diff/250001/components/password_ma...
components/password_manager/core/browser/psl_matching_helper_unittest.cc:470: //
canonicalized URL.
On 2017/05/26 12:47:55, vasilii wrote:
> Would be cool to explain why we care about it and what the relation to
> GetOrganizationIdentifyingName is.

Done.

https://codereview.chromium.org/2899083004/diff/250001/components/password_ma...
components/password_manager/core/browser/psl_matching_helper_unittest.cc:473:
char kExpectedNonAlphaNumChars[] = {'+', '-', '_', '.'};
On 2017/05/26 12:47:55, vasilii wrote:
> constexpr?

Done.

[engedy, 2017-05-29 08:47:49]

The CQ bit was checked by engedy@chromium.org

[engedy, 2017-05-29 08:47:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
vasilii@chromium.org, kolos@chromium.org
Link to the patchset: https://codereview.chromium.org/2899083004/#ps290001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-05-29 08:48:10]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-29 09:39:03]

CQ is committing da patch.

Bot data: {"patchset_id": 290001, "attempt_start_ts": 1496047669805780,
"parent_rev": "81467d8e08d523865daed682046dff8b60c0d5c8", "commit_rev":
"05971b1368780700908f226db1ff99837463abf1"}

[commit-bot: I haz the power, 2017-05-29 09:39:16]

Description was changed from

==========
Implement PasswordStore::GetLoginsForSameOrganizationName.

The method returns all stored credentials with SCHEME_HTTP that have an origin
whose organization-identifying name -- that is, the first domain name label
below the effective TLD -- matches that of a given origin.

For example, the organization-identifying name of "https://foo.example.org" is
`example`, and logins will be returned for "http://bar.example.co.uk", but not
for "http://notexample.com" or "https://example.foo.com".

BUG=720599
==========

to

==========
Implement PasswordStore::GetLoginsForSameOrganizationName.

The method returns all stored credentials with SCHEME_HTTP that have an origin
whose organization-identifying name -- that is, the first domain name label
below the effective TLD -- matches that of a given origin.

For example, the organization-identifying name of "https://foo.example.org" is
`example`, and logins will be returned for "http://bar.example.co.uk", but not
for "http://notexample.com" or "https://example.foo.com".

BUG=720599

Review-Url: https://codereview.chromium.org/2899083004
Cr-Commit-Position: refs/heads/master@{#475322}
Committed:
https://chromium.googlesource.com/chromium/src/+/05971b1368780700908f226db1ff...
==========

[commit-bot: I haz the power, 2017-05-29 09:39:18]

Committed patchset #15 (id:290001) as
https://chromium.googlesource.com/chromium/src/+/05971b1368780700908f226db1ff...