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

chrome/browser/password_manager/chrome_password_manager_client.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/password_manager/chrome_password_manager_client.h"
 
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/memory/singleton.h"
 #include "base/metrics/histogram.h"
@@ skipping 91 matching lines @@
     service->UnregisterClient(this);
 }
 
 bool ChromePasswordManagerClient::IsAutomaticPasswordSavingEnabled() const {
   return base::CommandLine::ForCurrentProcess()->HasSwitch(
              password_manager::switches::kEnableAutomaticPasswordSaving) &&
          chrome::VersionInfo::GetChannel() ==
              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());
   content::NavigationEntry* entry =
       web_contents()->GetController().GetLastCommittedEntry();
   if (!entry) {
     // TODO(gcasto): Determine if fix for crbug.com/388246 is relevant here.
     return true;
   }
 
@@ skipping 431 matching lines @@
       autofill_sync_state_ = DISALLOW_SYNC_CREDENTIALS;
   } else {
     // Allow by default.
     autofill_sync_state_ = ALLOW_SYNC_CREDENTIALS;
   }
 }
 
 const GURL& ChromePasswordManagerClient::GetMainFrameURL() const {
   return web_contents()->GetVisibleURL();
 }