[Credential Management] Smart lock save Credentials bubble should not always pop up.

[Credential Management] Smart lock save Credentials bubble should not always pop up.

Bubble shouldn't be popped up if user has blacklisted the site or
password management is disabled for the page.

BUG=450583
Committed: https://crrev.com/90ada142d5ce68a21ee0dca95b6a8ca872a35fe4
Cr-Commit-Position: refs/heads/master@{#324200}

[melandory, 2015-03-25 15:33:50]

Patchset #1 (id:1) has been deleted

[melandory, 2015-03-25 15:37:30]

Patchset #1 (id:20001) has been deleted

[melandory, 2015-03-26 09:59:40]

Patchset #1 (id:40001) has been deleted

[melandory, 2015-03-26 11:49:50]

Patchset #1 (id:60001) has been deleted

[melandory, 2015-03-26 11:49:59]

Patchset #1 (id:80001) has been deleted

[melandory, 2015-03-26 14:25:14]

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

[melandory, 2015-03-26 14:25:14]

Hi Vaclav,

review this CL, please.

Thanks!

[vabr (Chromium), 2015-03-26 14:51:59]

Thanks, Tanja.

This looks good, however, I believe we should simplify this further (please see
below).

Thanks!
Vaclav

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

https://codereview.chromium.org/1031153002/diff/100001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.cc:112: bool
ChromePasswordManagerClient::IsEnabledForCurrentPage() const {
Thinking about the two very similarly named methods:
IsEnabledForCurrentPage
and
IsPasswordManagerEnabledForCurrentPage

I suggest to get rid of IsEnabledForCurrentPage completely, and replace it
inline with the 2 checks it consists of. This will avoid misunderstandings and
make the code more readable.

It would be great to add logging similar to that on lines 116-122 in
DidLastPageLoadEncounterSSLErrors -- just get rid of the
STRING_ENABLED_FOR_CURRENT_PAGE_METHOD message completely, and put the remaining
two in the respective methods instead.

https://codereview.chromium.org/1031153002/diff/100001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.cc:116:
scoped_ptr<password_manager::BrowserSavePasswordProgressLogger> logger;
Because the logger is only used inside the block of code which is sure to create
it, please get rid of the scoped_ptr, and create the logger directly inside the
if-block:

if (IsLoggingActive()) {
  password_manager::BrowserSavePasswordProgressLogger logger(this);
  logger.LogMessage(...);
  ...
}

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

https://codereview.chromium.org/1031153002/diff/100001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:295:
TEST_F(ChromePasswordManagerClientTest, IsEnabledForCurrentPage) {
If you get rid of IsEnabledForCurrentPage, then of course feel free to delete
this test (i.e., there is no need to replace it by testing the components of
IsEnabledForCurrentPage).

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

https://codereview.chromium.org/1031153002/diff/100001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:197:
logger->LogBoolean(Logger::STRING_SSL_ERRORS_PRESENT,
If you add this message inside DidLastPageLoadEncounterSSLErrors, replace the
lines 197-198 with these two:

// This method logs the SSL status as a side-effect.
client_->DidLastPageLoadEncounterSSLErrors()

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

https://codereview.chromium.org/1031153002/diff/100001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:719:
EXPECT_CALL(client_, IsEnabledForCurrentPage()).WillRepeatedly(Return(false));
If you get rid of IsEnabledForCurrentPage, then please replace the tests
SavingDisabledIfManagerDisabled and AutofillingDisabledIfManagerDisabled with
two copies of each, always one copy having  DidLastPageLoadEncounterSSLErrors
returning true, and the other copy having IsPasswordManagerEnabledForCurrentPage
returning false.

[melandory, 2015-03-30 07:08:36]

On 2015/03/26 14:51:59, vabr (Chromium) wrote:
> Thanks, Tanja.
> 
> This looks good, however, I believe we should simplify this further (please
see
> below).
> 
> Thanks!
> Vaclav
> 
>
https://codereview.chromium.org/1031153002/diff/100001/chrome/browser/passwor...
> File chrome/browser/password_manager/chrome_password_manager_client.cc
(right):
> 
>
https://codereview.chromium.org/1031153002/diff/100001/chrome/browser/passwor...
> chrome/browser/password_manager/chrome_password_manager_client.cc:112: bool
> ChromePasswordManagerClient::IsEnabledForCurrentPage() const {
> Thinking about the two very similarly named methods:
> IsEnabledForCurrentPage
> and
> IsPasswordManagerEnabledForCurrentPage
> 
> I suggest to get rid of IsEnabledForCurrentPage completely, and replace it
> inline with the 2 checks it consists of. This will avoid misunderstandings and
> make the code more readable.
> 
> It would be great to add logging similar to that on lines 116-122 in
> DidLastPageLoadEncounterSSLErrors -- just get rid of the
> STRING_ENABLED_FOR_CURRENT_PAGE_METHOD message completely, and put the
remaining
> two in the respective methods instead.
> 
>
https://codereview.chromium.org/1031153002/diff/100001/chrome/browser/passwor...
> chrome/browser/password_manager/chrome_password_manager_client.cc:116:
> scoped_ptr<password_manager::BrowserSavePasswordProgressLogger> logger;
> Because the logger is only used inside the block of code which is sure to
create
> it, please get rid of the scoped_ptr, and create the logger directly inside
the
> if-block:
> 
> if (IsLoggingActive()) {
>   password_manager::BrowserSavePasswordProgressLogger logger(this);
>   logger.LogMessage(...);
>   ...
> }
> 
>
https://codereview.chromium.org/1031153002/diff/100001/chrome/browser/passwor...
> File
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/1031153002/diff/100001/chrome/browser/passwor...
>
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:295:
> TEST_F(ChromePasswordManagerClientTest, IsEnabledForCurrentPage) {
> If you get rid of IsEnabledForCurrentPage, then of course feel free to delete
> this test (i.e., there is no need to replace it by testing the components of
> IsEnabledForCurrentPage).
> 
>
https://codereview.chromium.org/1031153002/diff/100001/components/password_ma...
> File components/password_manager/core/browser/password_manager.cc (right):
> 
>
https://codereview.chromium.org/1031153002/diff/100001/components/password_ma...
> components/password_manager/core/browser/password_manager.cc:197:
> logger->LogBoolean(Logger::STRING_SSL_ERRORS_PRESENT,
> If you add this message inside DidLastPageLoadEncounterSSLErrors, replace the
> lines 197-198 with these two:
> 
> // This method logs the SSL status as a side-effect.
> client_->DidLastPageLoadEncounterSSLErrors()
> 
>
https://codereview.chromium.org/1031153002/diff/100001/components/password_ma...
> File components/password_manager/core/browser/password_manager_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/1031153002/diff/100001/components/password_ma...
> components/password_manager/core/browser/password_manager_unittest.cc:719:
> EXPECT_CALL(client_, IsEnabledForCurrentPage()).WillRepeatedly(Return(false));
> If you get rid of IsEnabledForCurrentPage, then please replace the tests
> SavingDisabledIfManagerDisabled and AutofillingDisabledIfManagerDisabled with
> two copies of each, always one copy having  DidLastPageLoadEncounterSSLErrors
> returning true, and the other copy having
IsPasswordManagerEnabledForCurrentPage
> returning false.

I end up re-factoring tests, so now this CL depends on
https://codereview.chromium.org/1044643002/

[melandory, 2015-03-30 09:46:23]

Patchset #4 (id:160001) has been deleted

[melandory, 2015-03-30 09:46:30]

Patchset #4 (id:180001) has been deleted

[melandory, 2015-03-30 09:50:17]

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

https://codereview.chromium.org/1031153002/diff/100001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.cc:112: bool
ChromePasswordManagerClient::IsEnabledForCurrentPage() const {
On 2015/03/26 14:51:58, vabr (Chromium) wrote:
> Thinking about the two very similarly named methods:
> IsEnabledForCurrentPage
> and
> IsPasswordManagerEnabledForCurrentPage
> 
> I suggest to get rid of IsEnabledForCurrentPage completely, and replace it
> inline with the 2 checks it consists of. This will avoid misunderstandings and
> make the code more readable.
> 
> It would be great to add logging similar to that on lines 116-122 in
> DidLastPageLoadEncounterSSLErrors -- just get rid of the
> STRING_ENABLED_FOR_CURRENT_PAGE_METHOD message completely, and put the
remaining
> two in the respective methods instead.

I was also puzzled with this naming collision, but was unable to come up with
good solution, what you're proposing I like a lot ;)

https://codereview.chromium.org/1031153002/diff/100001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.cc:116:
scoped_ptr<password_manager::BrowserSavePasswordProgressLogger> logger;
On 2015/03/26 14:51:59, vabr (Chromium) wrote:
> Because the logger is only used inside the block of code which is sure to
create
> it, please get rid of the scoped_ptr, and create the logger directly inside
the
> if-block:
> 
> if (IsLoggingActive()) {
>   password_manager::BrowserSavePasswordProgressLogger logger(this);
>   logger.LogMessage(...);
>   ...
> }

Done.

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

https://codereview.chromium.org/1031153002/diff/100001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:295:
TEST_F(ChromePasswordManagerClientTest, IsEnabledForCurrentPage) {
On 2015/03/26 14:51:59, vabr (Chromium) wrote:
> If you get rid of IsEnabledForCurrentPage, then of course feel free to delete
> this test (i.e., there is no need to replace it by testing the components of
> IsEnabledForCurrentPage).

Done.

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

https://codereview.chromium.org/1031153002/diff/100001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:197:
logger->LogBoolean(Logger::STRING_SSL_ERRORS_PRESENT,
On 2015/03/26 14:51:59, vabr (Chromium) wrote:
> If you add this message inside DidLastPageLoadEncounterSSLErrors, replace the
> lines 197-198 with these two:
> 
> // This method logs the SSL status as a side-effect.
> client_->DidLastPageLoadEncounterSSLErrors()

Done.

[vabr (Chromium), 2015-03-30 10:58:43]

Thanks, Tanja!
The changes look good, but I would like to propose one more method move (see
below). Let me know what do you think.

Thanks,
Vaclav

https://codereview.chromium.org/1031153002/diff/200001/chrome/browser/passwor...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/1031153002/diff/200001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.cc:117: bool
is_enabled;
Please initialise to false (which seems to be a safe value).

https://codereview.chromium.org/1031153002/diff/200001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.cc:134:
logger.LogBoolean(Logger::STRING_CLIENT_CHECK_PRESENT, is_enabled);
nit: The Logger's enum name seems a bit outdated. Would you mind changing it to,
say,
STRING_ENABLED_FOR_CURRENT_PAGE
?

https://codereview.chromium.org/1031153002/diff/200001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.cc:354: bool
ssl_errors;
Please initialise to true (which seems to be a safe value).

https://codereview.chromium.org/1031153002/diff/200001/components/password_ma...
File
components/password_manager/content/browser/credential_manager_dispatcher.cc
(right):

https://codereview.chromium.org/1031153002/diff/200001/components/password_ma...
components/password_manager/content/browser/credential_manager_dispatcher.cc:307:
bool CredentialManagerDispatcher::IsSavingEnabledForCurrentPage() const {
Currently, this duplicates PasswordManager::IsSavingEnabledForCurrentPage. I
believe it makes sense to move this method up to the client as well, and call it
on the client from both PasswordManager and CredentialManagerDispatcher. What do
you think?

https://codereview.chromium.org/1031153002/diff/200001/components/password_ma...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/1031153002/diff/200001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:191:
scoped_ptr<password_manager::BrowserSavePasswordProgressLogger> logger;
You can drop password_manager::, because this code is in this namespace already.

https://codereview.chromium.org/1031153002/diff/200001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:197: // This method
logs the SSL status as a side-effect.
Actually, you can drop lines 197-198 completely. I did not notice before, that
DidLastPageLoadEncounterSSLErrors() will get called (and the status logged)
because of line 189.
Sorry for the noise!

https://codereview.chromium.org/1031153002/diff/200001/components/password_ma...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/1031153002/diff/200001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:740:
TEST_F(PasswordManagerTest, SavingDisabledIfManagerDisabledWithSSLErrors) {
Two more kinds of tests for both saving and autofilling could be made:

* functionality disabled if there are SSL errors, but the manager itself is
enabled,
* functionality enabled if there are no SSL errors and the manager is enabled.

[vabr (Chromium), 2015-03-30 10:59:53]

Adding one forgotten comment.

https://codereview.chromium.org/1031153002/diff/200001/components/password_ma...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/1031153002/diff/200001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:147:
saving_passwords_enabled_.Init(prefs::kPasswordManagerSavingEnabled,
For the proposed move of the method to the client, I believe it also makes sense
to move saving_passwords_enabled_ and ReportMetrics to there.

[melandory, 2015-03-30 16:17:57]

On 2015/03/30 10:59:53, vabr (Chromium) wrote:
> Adding one forgotten comment.
> 
>
https://codereview.chromium.org/1031153002/diff/200001/components/password_ma...
> File components/password_manager/core/browser/password_manager.cc (right):
> 
>
https://codereview.chromium.org/1031153002/diff/200001/components/password_ma...
> components/password_manager/core/browser/password_manager.cc:147:
> saving_passwords_enabled_.Init(prefs::kPasswordManagerSavingEnabled,
> For the proposed move of the method to the client, I believe it also makes
sense
> to move saving_passwords_enabled_ and ReportMetrics to there.

And another re-factoring was inspired by this CL:
https://codereview.chromium.org/1042813003/

[melandory, 2015-03-31 12:22:54]

On 2015/03/30 16:17:57, melandory wrote:
> On 2015/03/30 10:59:53, vabr (Chromium) wrote:
> > Adding one forgotten comment.
> > 
> >
>
https://codereview.chromium.org/1031153002/diff/200001/components/password_ma...
> > File components/password_manager/core/browser/password_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/1031153002/diff/200001/components/password_ma...
> > components/password_manager/core/browser/password_manager.cc:147:
> > saving_passwords_enabled_.Init(prefs::kPasswordManagerSavingEnabled,
> > For the proposed move of the method to the client, I believe it also makes
> sense
> > to move saving_passwords_enabled_ and ReportMetrics to there.
> 
> And another re-factoring was inspired by this CL:
> https://codereview.chromium.org/1042813003/

https://codereview.chromium.org/1043163002/

[melandory, 2015-04-07 09:18:54]

Patchset #5 (id:220001) has been deleted

[melandory, 2015-04-07 12:50:29]

Patchset #5 (id:240001) has been deleted

[melandory, 2015-04-07 12:50:37]

Patchset #5 (id:260001) has been deleted

[melandory, 2015-04-07 13:14:13]

Ready for review.


Thanks!

https://codereview.chromium.org/1031153002/diff/200001/chrome/browser/passwor...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/1031153002/diff/200001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.cc:117: bool
is_enabled;
On 2015/03/30 10:58:42, vabr (Chromium) wrote:
> Please initialise to false (which seems to be a safe value).

Done.

https://codereview.chromium.org/1031153002/diff/200001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.cc:134:
logger.LogBoolean(Logger::STRING_CLIENT_CHECK_PRESENT, is_enabled);
On 2015/03/30 10:58:42, vabr (Chromium) wrote:
> nit: The Logger's enum name seems a bit outdated. Would you mind changing it
to,
> say,
> STRING_ENABLED_FOR_CURRENT_PAGE
> ?

Done.

https://codereview.chromium.org/1031153002/diff/200001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.cc:354: bool
ssl_errors;
On 2015/03/30 10:58:42, vabr (Chromium) wrote:
> Please initialise to true (which seems to be a safe value).

Done.

https://codereview.chromium.org/1031153002/diff/200001/components/password_ma...
File
components/password_manager/content/browser/credential_manager_dispatcher.cc
(right):

https://codereview.chromium.org/1031153002/diff/200001/components/password_ma...
components/password_manager/content/browser/credential_manager_dispatcher.cc:307:
bool CredentialManagerDispatcher::IsSavingEnabledForCurrentPage() const {
On 2015/03/30 10:58:42, vabr (Chromium) wrote:
> Currently, this duplicates PasswordManager::IsSavingEnabledForCurrentPage. I
> believe it makes sense to move this method up to the client as well, and call
it
> on the client from both PasswordManager and CredentialManagerDispatcher. What
do
> you think?

Done.

https://codereview.chromium.org/1031153002/diff/200001/components/password_ma...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/1031153002/diff/200001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:147:
saving_passwords_enabled_.Init(prefs::kPasswordManagerSavingEnabled,
On 2015/03/30 10:59:53, vabr (Chromium) wrote:
> For the proposed move of the method to the client, I believe it also makes
sense
> to move saving_passwords_enabled_ and ReportMetrics to there.

Done.

https://codereview.chromium.org/1031153002/diff/200001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:191:
scoped_ptr<password_manager::BrowserSavePasswordProgressLogger> logger;
On 2015/03/30 10:58:43, vabr (Chromium) wrote:
> You can drop password_manager::, because this code is in this namespace
already.

Done.

https://codereview.chromium.org/1031153002/diff/200001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:197: // This method
logs the SSL status as a side-effect.
On 2015/03/30 10:58:43, vabr (Chromium) wrote:
> Actually, you can drop lines 197-198 completely. I did not notice before, that
> DidLastPageLoadEncounterSSLErrors() will get called (and the status logged)
> because of line 189.
> Sorry for the noise!

Done.

[vabr (Chromium), 2015-04-07 13:53:15]

LGTM with some comments.

Thanks!
Vaclav

https://codereview.chromium.org/1031153002/diff/290001/chrome/browser/passwor...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/1031153002/diff/290001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.cc:61: // This
routine is called when PasswordManagersClient is constructed.
typo: Managers -> Manager

https://codereview.chromium.org/1031153002/diff/290001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.cc:71:
DCHECK(initial_thread_id == base::PlatformThread::CurrentId());
nit: Use
DCHECK_EQ(base::PlatformThread::CurrentId(), initial_thread_id);
instead.
(Note that the order also matters, the first one is the expected value.)

https://codereview.chromium.org/1031153002/diff/290001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.cc:90: typedef
autofill::SavePasswordProgressLogger Logger;
I believe this should go above the namespace. Ideally, please put it just after
the using-block on lines 54-56, so that all type aliases are on one place.

https://codereview.chromium.org/1031153002/diff/290001/chrome/browser/passwor...
File chrome/browser/password_manager/chrome_password_manager_client_unittest.cc
(right):

https://codereview.chromium.org/1031153002/diff/290001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:47:
class MockChromePasswordManagerClient : public ChromePasswordManagerClient {
Could you please add the following comment?
// TODO(vabr): Get rid of the mocked client in the client's own test, see
http://crbug.com/474577.

(Of course, this has nothing to do with your change. Also, I know that I just
LGTMed the creation of the mock last week -- the problem is not with using
GMock, it was the same problem already when the Test...Client was overriding the
real methods.)

https://codereview.chromium.org/1031153002/diff/290001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:442:
// Functionality disabled if there are SSL errors and the manager itself is
nit: Please separate individual blocks of the test (those starting with the //
Functionality... comment) with a blank line.

https://codereview.chromium.org/1031153002/diff/290001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:448:
// is
nit: Please join the lines 448 and 449.

https://codereview.chromium.org/1031153002/diff/290001/components/autofill/co...
File components/autofill/core/common/save_password_progress_logger.cc (right):

https://codereview.chromium.org/1031153002/diff/290001/components/autofill/co...
components/autofill/core/common/save_password_progress_logger.cc:206: return
"IsPasswordManagerEnabledForCurrentPage";
typo: Manager -> Management ?

[melandory, 2015-04-07 14:24:56]

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

[melandory, 2015-04-07 14:24:57]

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

[commit-bot: I haz the power, 2015-04-07 14:25:09]

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

[melandory, 2015-04-07 14:28:52]

https://codereview.chromium.org/1031153002/diff/290001/chrome/browser/passwor...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/1031153002/diff/290001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.cc:61: // This
routine is called when PasswordManagersClient is constructed.
On 2015/04/07 13:53:14, vabr (Chromium) wrote:
> typo: Managers -> Manager

Done.

https://codereview.chromium.org/1031153002/diff/290001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.cc:71:
DCHECK(initial_thread_id == base::PlatformThread::CurrentId());
On 2015/04/07 13:53:14, vabr (Chromium) wrote:
> nit: Use
> DCHECK_EQ(base::PlatformThread::CurrentId(), initial_thread_id);
> instead.
> (Note that the order also matters, the first one is the expected value.)

Done.

https://codereview.chromium.org/1031153002/diff/290001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.cc:90: typedef
autofill::SavePasswordProgressLogger Logger;
On 2015/04/07 13:53:14, vabr (Chromium) wrote:
> I believe this should go above the namespace. Ideally, please put it just
after
> the using-block on lines 54-56, so that all type aliases are on one place.

Done.

https://codereview.chromium.org/1031153002/diff/290001/chrome/browser/passwor...
File chrome/browser/password_manager/chrome_password_manager_client_unittest.cc
(right):

https://codereview.chromium.org/1031153002/diff/290001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:47:
class MockChromePasswordManagerClient : public ChromePasswordManagerClient {
On 2015/04/07 13:53:15, vabr (Chromium) wrote:
> Could you please add the following comment?
> // TODO(vabr): Get rid of the mocked client in the client's own test, see
> http://crbug.com/474577.
> 
> (Of course, this has nothing to do with your change. Also, I know that I just
> LGTMed the creation of the mock last week -- the problem is not with using
> GMock, it was the same problem already when the Test...Client was overriding
the
> real methods.)

Yeah, I totally understand that. I tried not to use
MockChromePasswordManagerClient when I was writing tests for
IsSavingEnabledForCurrent page, but failed to create environment where I can
simulate DidLastPageLoadEncounterSSLErrors.

https://codereview.chromium.org/1031153002/diff/290001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:442:
// Functionality disabled if there are SSL errors and the manager itself is
On 2015/04/07 13:53:15, vabr (Chromium) wrote:
> nit: Please separate individual blocks of the test (those starting with the //
> Functionality... comment) with a blank line.

Done.

https://codereview.chromium.org/1031153002/diff/290001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:448:
// is
On 2015/04/07 13:53:15, vabr (Chromium) wrote:
> nit: Please join the lines 448 and 449.

Done.

https://codereview.chromium.org/1031153002/diff/290001/components/autofill/co...
File components/autofill/core/common/save_password_progress_logger.cc (right):

https://codereview.chromium.org/1031153002/diff/290001/components/autofill/co...
components/autofill/core/common/save_password_progress_logger.cc:206: return
"IsPasswordManagerEnabledForCurrentPage";
On 2015/04/07 13:53:15, vabr (Chromium) wrote:
> typo: Manager -> Management ?

Done.

[commit-bot: I haz the power, 2015-04-07 15:04:26]

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

[commit-bot: I haz the power, 2015-04-07 15:04:26]

This issue passed the CQ dry run.

[vabr (Chromium), 2015-04-07 15:14:45]

LGTM, Thanks!

[melandory, 2015-04-08 09:19:14]

The CQ bit was checked by melandory@chromium.org

[commit-bot: I haz the power, 2015-04-08 09:19:48]

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

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

Committed patchset #7 (id:310001)

[commit-bot: I haz the power, 2015-04-08 09:22:12]

Patchset 7 (id:??) landed as
https://crrev.com/90ada142d5ce68a21ee0dca95b6a8ca872a35fe4
Cr-Commit-Position: refs/heads/master@{#324200}