Fix blacklisting for desktop infobars.

Fix blacklisting for desktop infobars.

Also, please write in the CL description:
* Blacklisting was broken for desktop infobars after CL  https://codereview.chromium.org/967193002
* Source of problem was in reversed action in if/else clause.

BUG=472006
R=mkwst@chromium.org

Committed: https://crrev.com/14177dfa384a66bf361c73b34b34648e11bf4946
Cr-Commit-Position: refs/heads/master@{#323630}

[Mike West, 2015-03-31 07:27:35]

Please add a test. :)

[Mike West, 2015-04-02 05:58:14]

On 2015/03/31 at 07:27:35, Mike West wrote:
> Please add a test. :)

Ping?

[melandory, 2015-04-02 07:54:43]

On 2015/04/02 05:58:14, Mike West wrote:
> On 2015/03/31 at 07:27:35, Mike West wrote:
> > Please add a test. :)
> 
> Ping?

Will finish today, sorry.

[Mike West, 2015-04-02 12:36:28]

On 2015/04/02 at 07:54:43, melandory wrote:
> On 2015/04/02 05:58:14, Mike West wrote:
> > On 2015/03/31 at 07:27:35, Mike West wrote:
> > > Please add a test. :)
> > 
> > Ping?
> 
> Will finish today, sorry.

OWNERs LGTM. Please find someone in the office to sanity check.

Also, I think you'll need to update the GN file as well as the gypi, right?

[vabr (Chromium), 2015-04-02 13:32:16]

vabr@chromium.org changed reviewers:
+ vabr@chromium.org

[vabr (Chromium), 2015-04-02 13:32:17]

Hi Tanja,
LGTM with some comments.
Also, please write in the CL description:
* What the problem was (link to https://codereview.chromium.org/967193002 might
be helpful, but is not necessary).
* How this is fixed.

(I know it is not very complex, but makes it easy to understand the CL without
reading the diff, in case of need.)

Thanks!
Vaclav

https://codereview.chromium.org/1045173003/diff/40001/chrome/browser/password...
File chrome/browser/password_manager/save_password_infobar_delegate.h (right):

https://codereview.chromium.org/1045173003/diff/40001/chrome/browser/password...
chrome/browser/password_manager/save_password_infobar_delegate.h:43: #if
defined(UNIT_TEST)
As an alternative -- what about making the SavePasswordInfoBarDelegate
constructor protected, with a comment that it is for tests, and creating a
sub-class of SavePasswordInfoBarDelegate in your unittest with the sole purpose
to expose the constructor through it?

I would prefer that to adding stuff and #ifdefs to the production header.

https://codereview.chromium.org/1045173003/diff/40001/chrome/browser/password...
chrome/browser/password_manager/save_password_infobar_delegate.h:46: const
std::string& uma_histogram_suffix,
I don't think you need this argument, just hard-code a std::string().

https://codereview.chromium.org/1045173003/diff/40001/chrome/browser/password...
File chrome/browser/password_manager/save_password_infobar_delegate_unittest.cc
(right):

https://codereview.chromium.org/1045173003/diff/40001/chrome/browser/password...
chrome/browser/password_manager/save_password_infobar_delegate_unittest.cc:20:
autofill::PasswordForm form)
const autofill::PasswordForm& ?

https://codereview.chromium.org/1045173003/diff/40001/chrome/browser/password...
chrome/browser/password_manager/save_password_infobar_delegate_unittest.cc:30:
private:
Did you mean to add DISALLOW_COPY_AND_ASSIGN in the private section?
If not, please remove the section completely.

https://codereview.chromium.org/1045173003/diff/40001/chrome/browser/password...
chrome/browser/password_manager/save_password_infobar_delegate_unittest.cc:41:
autofill::PasswordForm test_form() { return test_form_; }
Please make this return a const ref instead of the value.

https://codereview.chromium.org/1045173003/diff/40001/chrome/browser/password...
chrome/browser/password_manager/save_password_infobar_delegate_unittest.cc:71:
password_form_manager_.reset(
Instead of keeping this as a member variable, please create a helper method,
which will return the manager:
scoped_ptr<MockPasswordFormManager> CreateMockFormManager()

Then, add an explicit scoped_ptr<PasswordFormManager> argument to
SaveInfoBarDelegateTest::CreateDelegate() to make it clear, which form manager
it uses.

The current state hides the fact that after the first call to CreateDelegate,
the member-variable scoped_ptr<MockPasswordFormManager> becomes null.

https://codereview.chromium.org/1045173003/diff/40001/components/password_man...
File components/password_manager/core/browser/password_form_manager.h (right):

https://codereview.chromium.org/1045173003/diff/40001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:141: virtual
void PermanentlyBlacklist();
Please add a TODO at the PasswordFormManager class definition and file a bug for
separating the PasswordFormManager API interface and the implementation in two
classes.
The reason is to make mocking easier (no need to deal with the cumbersome
constructor). This will be some work, and there is no reason to mix that with
this CL, but we should not forget about it.
Please mark the bug with Cr-UI-Browser-Passwords and Hotlist-GoodFirstBug
labels. Thanks!

[melandory, 2015-04-02 15:48:48]

Patchset #4 (id:60001) has been deleted

[melandory, 2015-04-02 15:48:53]

Patchset #4 (id:60001) has been deleted

[melandory, 2015-04-02 15:49:07]

Patchset #4 (id:60001) has been deleted

[melandory, 2015-04-02 15:49:17]

https://codereview.chromium.org/1045173003/diff/40001/chrome/browser/password...
File chrome/browser/password_manager/save_password_infobar_delegate.h (right):

https://codereview.chromium.org/1045173003/diff/40001/chrome/browser/password...
chrome/browser/password_manager/save_password_infobar_delegate.h:43: #if
defined(UNIT_TEST)
On 2015/04/02 13:32:16, vabr (Chromium) wrote:
> As an alternative -- what about making the SavePasswordInfoBarDelegate
> constructor protected, with a comment that it is for tests, and creating a
> sub-class of SavePasswordInfoBarDelegate in your unittest with the sole
purpose
> to expose the constructor through it?
> 
> I would prefer that to adding stuff and #ifdefs to the production header.

I'll add TODO.

https://codereview.chromium.org/1045173003/diff/40001/chrome/browser/password...
chrome/browser/password_manager/save_password_infobar_delegate.h:46: const
std::string& uma_histogram_suffix,
On 2015/04/02 13:32:16, vabr (Chromium) wrote:
> I don't think you need this argument, just hard-code a std::string().

Done.

https://codereview.chromium.org/1045173003/diff/40001/chrome/browser/password...
File chrome/browser/password_manager/save_password_infobar_delegate_unittest.cc
(right):

https://codereview.chromium.org/1045173003/diff/40001/chrome/browser/password...
chrome/browser/password_manager/save_password_infobar_delegate_unittest.cc:20:
autofill::PasswordForm form)
On 2015/04/02 13:32:17, vabr (Chromium) wrote:
> const autofill::PasswordForm& ?

Done.

https://codereview.chromium.org/1045173003/diff/40001/chrome/browser/password...
chrome/browser/password_manager/save_password_infobar_delegate_unittest.cc:30:
private:
On 2015/04/02 13:32:17, vabr (Chromium) wrote:
> Did you mean to add DISALLOW_COPY_AND_ASSIGN in the private section?
> If not, please remove the section completely.

Done.

https://codereview.chromium.org/1045173003/diff/40001/chrome/browser/password...
chrome/browser/password_manager/save_password_infobar_delegate_unittest.cc:41:
autofill::PasswordForm test_form() { return test_form_; }
On 2015/04/02 13:32:16, vabr (Chromium) wrote:
> Please make this return a const ref instead of the value.

Done.

https://codereview.chromium.org/1045173003/diff/40001/chrome/browser/password...
chrome/browser/password_manager/save_password_infobar_delegate_unittest.cc:71:
password_form_manager_.reset(
On 2015/04/02 13:32:17, vabr (Chromium) wrote:
> Instead of keeping this as a member variable, please create a helper method,
> which will return the manager:
> scoped_ptr<MockPasswordFormManager> CreateMockFormManager()
> 
> Then, add an explicit scoped_ptr<PasswordFormManager> argument to
> SaveInfoBarDelegateTest::CreateDelegate() to make it clear, which form manager
> it uses.
> 
> The current state hides the fact that after the first call to CreateDelegate,
> the member-variable scoped_ptr<MockPasswordFormManager> becomes null.

I've created helper function which returns raw pointer with explanation who owns
it.

https://codereview.chromium.org/1045173003/diff/40001/components/password_man...
File components/password_manager/core/browser/password_form_manager.h (right):

https://codereview.chromium.org/1045173003/diff/40001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:141: virtual
void PermanentlyBlacklist();
On 2015/04/02 13:32:17, vabr (Chromium) wrote:
> Please add a TODO at the PasswordFormManager class definition and file a bug
for
> separating the PasswordFormManager API interface and the implementation in two
> classes.
> The reason is to make mocking easier (no need to deal with the cumbersome
> constructor). This will be some work, and there is no reason to mix that with
> this CL, but we should not forget about it.
> Please mark the bug with Cr-UI-Browser-Passwords and Hotlist-GoodFirstBug
> labels. Thanks!

Done.

[melandory, 2015-04-02 15:51:50]

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

[melandory, 2015-04-02 15:51:50]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
vabr@chromium.org
Link to the patchset: https://codereview.chromium.org/1045173003/#ps80001
(title: " ")

[commit-bot: I haz the power, 2015-04-02 15:53:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1045173003/80001

[commit-bot: I haz the power, 2015-04-02 16:02:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1045173003/80001

[commit-bot: I haz the power, 2015-04-02 16:17:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1045173003/80001

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

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1045173003/80001

[commit-bot: I haz the power, 2015-04-02 16:47:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1045173003/80001

[commit-bot: I haz the power, 2015-04-02 16:53:55]

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

[commit-bot: I haz the power, 2015-04-02 16:53:56]

This issue passed the CQ dry run.

[melandory, 2015-04-02 20:39:23]

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

[melandory, 2015-04-02 20:39:23]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1045173003/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-04-02 20:42:25]

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

[commit-bot: I haz the power, 2015-04-02 21:06:16]

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

[commit-bot: I haz the power, 2015-04-02 21:06:17]

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

[melandory, 2015-04-02 21:24:01]

The CQ bit was checked by melandory@chromium.org

[commit-bot: I haz the power, 2015-04-02 21:25:29]

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

[commit-bot: I haz the power, 2015-04-03 05:28:51]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-04-03 20:32:33]

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

[vabr (Chromium), 2015-04-07 07:51:37]

Hi Tanja,

I have 2 post-landing comments. Please consider them for a follow-up CL.

Thanks!
Vaclav

https://codereview.chromium.org/1045173003/diff/100001/chrome/browser/passwor...
File chrome/browser/password_manager/save_password_infobar_delegate_unittest.cc
(right):

https://codereview.chromium.org/1045173003/diff/100001/chrome/browser/passwor...
chrome/browser/password_manager/save_password_infobar_delegate_unittest.cc:103:
// SavePasswordInfoBarDelegate::Create takes ownership of
Why not just use scoped_ptr? Raw pointers are fragile, and require long comments
for documenting, which quickly get out of date. scoped_ptr works automatically,
and documents precisely and always correctly, who owns what.

Also, instead of duplicating this comment on lines 116-117, it should be at the
declaration of CreateDelegate (but I really suggest using scoped_ptr instead).

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

https://codereview.chromium.org/1045173003/diff/100001/components/password_ma...
components/password_manager/core/browser/password_manager.h:40: //
implementation in two classes crbug/473184.
nit: Please don't leave out http:// and .com, so that the link is clickable and
usable in CodeSearch on public networks.
http://crbug.com/473184