Fix IE password import

Functions that decoded ie7 structures to extract per-URL username and password assumed there will only be one such pair for a URL. Changed to work with URLs for which there are several sets of credentials stored.

BUG=305241
TEST=IE7PasswordTest.*

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

[mpawlowski, 2013-10-08 14:05:56]

git cl upload complained about missing OWNERS for these files, you were
suggested though.

[Lei Zhang, 2013-10-08 17:49:46]

https://codereview.chromium.org/26465006/diff/1/chrome/browser/password_manag...
File chrome/browser/password_manager/password_store_win.cc (right):

https://codereview.chromium.org/26465006/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_store_win.cc:98: void
PasswordStoreWin::DBHandler::GetIE7Results(
Just return a std::vector<PasswordForm*>, but write this method so compilers can
do RVO. http://msdn.microsoft.com/en-us/library/ms364057(VS.80).aspx

https://codereview.chromium.org/26465006/diff/1/components/webdata/encryptor/...
File components/webdata/encryptor/ie7_password.cc (right):

https://codereview.chromium.org/26465006/diff/1/components/webdata/encryptor/...
components/webdata/encryptor/ie7_password.cc:78: for (int i = 0; i <
entry_count/2; ++i) {
nit: entry_count / 2

https://codereview.chromium.org/26465006/diff/1/components/webdata/encryptor/...
components/webdata/encryptor/ie7_password.cc:79: const uint8* ptr =
&data.front();
You can move this out of the for-loop since it has the same value for all
iterations of the loop.

https://codereview.chromium.org/26465006/diff/1/components/webdata/encryptor/...
File components/webdata/encryptor/ie7_password.h (right):

https://codereview.chromium.org/26465006/diff/1/components/webdata/encryptor/...
components/webdata/encryptor/ie7_password.h:31: DecryptedCredentials();
nit: we generally don't bother with ctor/dtors for structs. e.g. IE7PasswordInfo
above on line 16.

https://codereview.chromium.org/26465006/diff/1/components/webdata/encryptor/...
components/webdata/encryptor/ie7_password.h:34: std::wstring username;
There exists a presubmit check to prevent std::wstring use. However, we know in
this case it's ok. You'll have to write it this way to pacify the presubmit
check:

// presubmit: allow wstring
std:wstring username;

https://codereview.chromium.org/26465006/diff/1/components/webdata/encryptor/...
components/webdata/encryptor/ie7_password.h:38: // Parses a data structure to
find passwords and usernames.
Can you mention the format of |data| and mention |credentials| is where the
results go?

https://codereview.chromium.org/26465006/diff/1/components/webdata/encryptor/...
File components/webdata/encryptor/ie7_password_unittest_win.cc (right):

https://codereview.chromium.org/26465006/diff/1/components/webdata/encryptor/...
components/webdata/encryptor/ie7_password_unittest_win.cc:11: #include
"base/strings/string16.h"
no longer used, remove?

https://codereview.chromium.org/26465006/diff/1/components/webdata/encryptor/...
components/webdata/encryptor/ie7_password_unittest_win.cc:64: unsigned char
data[] =
nit - This should be: const unsigned char kData[], ditto in the original test.

https://codereview.chromium.org/26465006/diff/1/components/webdata/encryptor/...
components/webdata/encryptor/ie7_password_unittest_win.cc:77: 
nit: only 1 blank line, ditto in the above test.

[mpawlowski, 2013-10-09 07:42:30]

new patch set uploaded

[Lei Zhang, 2013-10-09 07:59:09]

https://codereview.chromium.org/26465006/diff/7001/components/webdata/encrypt...
File components/webdata/encryptor/ie7_password.cc (right):

https://codereview.chromium.org/26465006/diff/7001/components/webdata/encrypt...
components/webdata/encryptor/ie7_password.cc:73: const uint8* offset_to_data =
&data[0] +
Ok, you got rid of |ptr|, but isn't |offset_to_data| the same in every loop
iteration as well?

[Lei Zhang, 2013-10-09 08:13:38]

Otherwise LGTM!

[commit-bot: I haz the power, 2013-10-09 08:26:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpawlowski@opera.com/26465006/17001

[commit-bot: I haz the power, 2013-10-09 09:20:13]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[mpawlowski, 2013-10-09 09:22:06]

On 2013/10/09 09:20:13, I haz the power (commit-bot) wrote:
> Retried try job too often on chromium_presubmit for step(s) presubmit
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

What does this error mean? Do I have to get rid of all wstrings after all? Or
just try again?

[Lei Zhang, 2013-10-09 19:55:15]

On 2013/10/09 09:22:06, mpawlowski wrote:
> On 2013/10/09 09:20:13, I haz the power (commit-bot) wrote:
> > Retried try job too often on chromium_presubmit for step(s) presubmit
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
> 
> What does this error mean? Do I have to get rid of all wstrings after all? Or
> just try again?

Ya, that was the thing I wanted you to avoid with the // presubmit: allow
wstring, but missed a couple cases. You'll need:

bool DecryptPasswords(const std::wstring& url /* presubmit: allow wstring */,

in ie7_password.{cc,h}

[Ilya Sherman, 2013-10-09 20:46:08]

On 2013/10/09 19:55:15, Lei Zhang wrote:
> On 2013/10/09 09:22:06, mpawlowski wrote:
> > On 2013/10/09 09:20:13, I haz the power (commit-bot) wrote:
> > > Retried try job too often on chromium_presubmit for step(s) presubmit
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
> > 
> > What does this error mean? Do I have to get rid of all wstrings after all?
Or
> > just try again?
> 
> Ya, that was the thing I wanted you to avoid with the // presubmit: allow
> wstring, but missed a couple cases. You'll need:
> 
> bool DecryptPasswords(const std::wstring& url /* presubmit: allow wstring */,
> 
> in ie7_password.{cc,h}

Alternately, we could rename the files to ie7_password_win* (i.e. add a "_win"
suffix to the filenames), and then the presubmit script wouldn't complain, and I
bet the .gyp file would get a bit simpler as well.

[Lei Zhang, 2013-10-09 21:05:28]

On 2013/10/09 20:46:08, Ilya Sherman wrote:
> On 2013/10/09 19:55:15, Lei Zhang wrote:
> > On 2013/10/09 09:22:06, mpawlowski wrote:
> > > On 2013/10/09 09:20:13, I haz the power (commit-bot) wrote:
> > > > Retried try job too often on chromium_presubmit for step(s) presubmit
> > > >
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
> > > 
> > > What does this error mean? Do I have to get rid of all wstrings after all?
> Or
> > > just try again?
> > 
> > Ya, that was the thing I wanted you to avoid with the // presubmit: allow
> > wstring, but missed a couple cases. You'll need:
> > 
> > bool DecryptPasswords(const std::wstring& url /* presubmit: allow wstring
*/,
> > 
> > in ie7_password.{cc,h}
> 
> Alternately, we could rename the files to ie7_password_win* (i.e. add a "_win"
> suffix to the filenames), and then the presubmit script wouldn't complain, and
I
> bet the .gyp file would get a bit simpler as well.

Ah, did not know that. Thanks.

Maciej, you can do the "presubmit: allow" or the rename. It's up to you.

[mpawlowski, 2013-10-10 07:57:28]

I'll try renaming it to _win, wanna reap the extra bonus from simplifying the
gyp.

[mpawlowski, 2013-10-10 12:47:13]

Renamed ie7_password to end with _win

[Lei Zhang, 2013-10-10 16:21:42]

lgtm++

[commit-bot: I haz the power, 2013-10-10 16:21:52]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[commit-bot: I haz the power, 2013-10-10 22:12:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpawlowski@opera.com/26465006/30001

[commit-bot: I haz the power, 2013-10-10 22:34:43]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Lei Zhang, 2013-10-10 22:37:49]

+joi

[Jói, 2013-10-11 10:26:33]

LGTM

On Thu, Oct 10, 2013 at 10:37 PM,  <thestig@chromium.org> wrote:
> +joi
>
> https://codereview.chromium.org/26465006/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2013-10-11 10:54:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpawlowski@opera.com/26465006/30001

[commit-bot: I haz the power, 2013-10-11 13:55:22]

Change committed as 228180