[Password Manager] Add the scheme to sort keys for chrome://settings/passwords

[Password Manager] Add the scheme to sort keys for chrome://settings/passwords

The scheme was not a part of sort key. Therefore, we merged http and https credentials (if they have the same origin, username and password). See the bug for details.

BUG=658981
Committed: https://crrev.com/2064c3247e03ad89688194f7bd4fa79479363644
Cr-Commit-Position: refs/heads/master@{#432189}

[kolos1, 2016-11-15 13:02:05]

Description was changed from

==========
o

BUG=
==========

to

==========
[Password Manager] Add the scheme to sort keys for chrome://settings/passwords

The scheme was not a part of sort key. Therefore, we merged http and https
credentials (if they have the same origin, username and password). See the bug
for details.

BUG=658981
==========

[kolos1, 2016-11-15 13:05:28]

Patchset #2 (id:20001) has been deleted

[kolos1, 2016-11-15 13:05:32]

Patchset #1 (id:1) has been deleted

[kolos1, 2016-11-15 13:05:45]

Patchset #1 (id:40001) has been deleted

[kolos1, 2016-11-15 13:08:06]

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

[kolos1, 2016-11-15 13:08:07]

Hi Vasilii,

Please review this CL.

Regards,
Maxim

[vasilii, 2016-11-15 13:18:11]

https://codereview.chromium.org/2500393002/diff/60001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/password_manager_presenter.cc (right):

https://codereview.chromium.org/2500393002/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/password_manager_presenter.cc:97: if
(!form.federation_origin.unique())
Why don't you just use Origin::Serialize()?

https://codereview.chromium.org/2500393002/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/password_manager_presenter.cc:100: key = key +
kSortKeyPartsSeparator + kSortKeyNoFederationSymbol;
key+= is faster and easier to read.

[kolos1, 2016-11-15 13:55:52]

Patchset #2 (id:80001) has been deleted

[kolos1, 2016-11-15 13:57:38]

https://codereview.chromium.org/2500393002/diff/60001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/password_manager_presenter.cc (right):

https://codereview.chromium.org/2500393002/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/password_manager_presenter.cc:97: if
(!form.federation_origin.unique())
Serialize() returns "null" if there is no federation. "null" will be compared
with other federations. 

But I would like to separate the entries with federations and the entries w/o
federations. The special symbol like "-" does that. The entries w/o federations
appear earlier.

https://codereview.chromium.org/2500393002/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/password_manager_presenter.cc:100: key = key +
kSortKeyPartsSeparator + kSortKeyNoFederationSymbol;
On 2016/11/15 13:18:10, vasilii wrote:
> key+= is faster and easier to read. 
In this particular case, we cannot use +=, because the compiler will calculate
the sum of two characters and add it to |key|. But we want to add the
concatenation of these characters.

[vasilii, 2016-11-15 14:14:55]

https://codereview.chromium.org/2500393002/diff/60001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/password_manager_presenter.cc (right):

https://codereview.chromium.org/2500393002/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/password_manager_presenter.cc:97: if
(!form.federation_origin.unique())
On 2016/11/15 13:57:38, kolos1 wrote:
> Serialize() returns "null" if there is no federation. "null" will be compared
> with other federations. 
> 
> But I would like to separate the entries with federations and the entries w/o
> federations. The special symbol like "-" does that. The entries w/o
federations
> appear earlier.

If it's important then it should go to a comment on kSortKeyNoFederationSymbol.

https://codereview.chromium.org/2500393002/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/password_manager_presenter.cc:100: key = key +
kSortKeyPartsSeparator + kSortKeyNoFederationSymbol;
On 2016/11/15 13:57:38, kolos1 wrote:
> On 2016/11/15 13:18:10, vasilii wrote:
> > key+= is faster and easier to read. 
> In this particular case, we cannot use +=, because the compiler will calculate
> the sum of two characters and add it to |key|. But we want to add the
> concatenation of these characters. 

key += kSortKeyPartsSeparator;
if ()
  key += form.federation_origin.host();
else 
  key += kSortKeyNoFederationSymbol;

[vasilii, 2016-11-15 14:24:44]

https://groups.google.com/a/chromium.org/forum/?utm_source=digest&utm_medium=...

explains why using strings carefully is important.

[kolos1, 2016-11-15 14:42:37]

Patchset #3 (id:120001) has been deleted

[kolos1, 2016-11-15 14:42:51]

https://codereview.chromium.org/2500393002/diff/60001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/password_manager_presenter.cc (right):

https://codereview.chromium.org/2500393002/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/password_manager_presenter.cc:97: if
(!form.federation_origin.unique())
On 2016/11/15 14:14:54, vasilii wrote:
> On 2016/11/15 13:57:38, kolos1 wrote:
> > Serialize() returns "null" if there is no federation. "null" will be
compared
> > with other federations. 
> > 
> > But I would like to separate the entries with federations and the entries
w/o
> > federations. The special symbol like "-" does that. The entries w/o
> federations
> > appear earlier.
> 
> If it's important then it should go to a comment on
kSortKeyNoFederationSymbol.

Done.

https://codereview.chromium.org/2500393002/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/password_manager_presenter.cc:100: key = key +
kSortKeyPartsSeparator + kSortKeyNoFederationSymbol;
On 2016/11/15 14:14:55, vasilii wrote:
> On 2016/11/15 13:57:38, kolos1 wrote:
> > On 2016/11/15 13:18:10, vasilii wrote:
> > > key+= is faster and easier to read. 
> > In this particular case, we cannot use +=, because the compiler will
calculate
> > the sum of two characters and add it to |key|. But we want to add the
> > concatenation of these characters. 
> 
> key += kSortKeyPartsSeparator;
> if ()
>   key += form.federation_origin.host();
> else 
>   key += kSortKeyNoFederationSymbol;

Done.

[vasilii, 2016-11-15 14:50:06]

lgtm

[kolos1, 2016-11-15 14:55:53]

The CQ bit was checked by kolos@chromium.org

[commit-bot: I haz the power, 2016-11-15 14:56:15]

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

[kolos1, 2016-11-15 14:56:53]

The CQ bit was unchecked by kolos@chromium.org

[kolos1, 2016-11-15 15:03:09]

The CQ bit was checked by kolos@chromium.org

[commit-bot: I haz the power, 2016-11-15 15:03:32]

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, 2016-11-15 16:08:09]

Description was changed from

==========
[Password Manager] Add the scheme to sort keys for chrome://settings/passwords

The scheme was not a part of sort key. Therefore, we merged http and https
credentials (if they have the same origin, username and password). See the bug
for details.

BUG=658981
==========

to

==========
[Password Manager] Add the scheme to sort keys for chrome://settings/passwords

The scheme was not a part of sort key. Therefore, we merged http and https
credentials (if they have the same origin, username and password). See the bug
for details.

BUG=658981
==========

[commit-bot: I haz the power, 2016-11-15 16:08:10]

Committed patchset #3 (id:140001)

[commit-bot: I haz the power, 2016-11-15 16:11:06]

Description was changed from

==========
[Password Manager] Add the scheme to sort keys for chrome://settings/passwords

The scheme was not a part of sort key. Therefore, we merged http and https
credentials (if they have the same origin, username and password). See the bug
for details.

BUG=658981
==========

to

==========
[Password Manager] Add the scheme to sort keys for chrome://settings/passwords

The scheme was not a part of sort key. Therefore, we merged http and https
credentials (if they have the same origin, username and password). See the bug
for details.

BUG=658981

Committed: https://crrev.com/2064c3247e03ad89688194f7bd4fa79479363644
Cr-Commit-Position: refs/heads/master@{#432189}
==========

[commit-bot: I haz the power, 2016-11-15 16:11:07]

Patchset 3 (id:??) landed as
https://crrev.com/2064c3247e03ad89688194f7bd4fa79479363644
Cr-Commit-Position: refs/heads/master@{#432189}