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

chrome/browser/password_manager/chrome_password_manager_client.cc

Index: chrome/browser/password_manager/chrome_password_manager_client.cc
diff --git a/chrome/browser/password_manager/chrome_password_manager_client.cc b/chrome/browser/password_manager/chrome_password_manager_client.cc
index 79d900d4e7f96a379dd75971fc2328c16213d068..52335fa06f8cf7ba84824898452cc95c3b4ab29e 100644
--- a/chrome/browser/password_manager/chrome_password_manager_client.cc
+++ b/chrome/browser/password_manager/chrome_password_manager_client.cc
@@ -109,6 +109,21 @@ bool ChromePasswordManagerClient::IsAutomaticPasswordSavingEnabled() const {
              chrome::VersionInfo::CHANNEL_UNKNOWN;
 }
 
+bool ChromePasswordManagerClient::IsEnabledForCurrentPage() const {

[vabr (Chromium), 2015/03/26 14:51:58]

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.

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

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 ;)

+  bool ssl_errors = DidLastPageLoadEncounterSSLErrors();
+  bool client_check = IsPasswordManagerEnabledForCurrentPage();
+
+  scoped_ptr<password_manager::BrowserSavePasswordProgressLogger> logger;

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

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(...);
  ...
}

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

Done.

+  if (IsLoggingActive()) {
+    logger.reset(new password_manager::BrowserSavePasswordProgressLogger(this));
+    logger->LogMessage(Logger::STRING_ENABLED_FOR_CURRENT_PAGE_METHOD);
+    logger->LogBoolean(Logger::STRING_SSL_ERRORS_PRESENT, ssl_errors);
+    logger->LogBoolean(Logger::STRING_CLIENT_CHECK_PRESENT, client_check);
+  }
+
+  return !ssl_errors && client_check;
+}
+
 bool ChromePasswordManagerClient::IsPasswordManagerEnabledForCurrentPage()
     const {
   DCHECK(web_contents());