[Password Manager] Fix to recognise failed login attempt for sites where content server pushes new …

components/password_manager/core/browser/password_manager.cc

Index: components/password_manager/core/browser/password_manager.cc
diff --git a/components/password_manager/core/browser/password_manager.cc b/components/password_manager/core/browser/password_manager.cc
index e05dab3878ac5999bb8748d898a43b44507c6458..fd9b860902b258f78755266bb81b7c043987321b 100644
--- a/components/password_manager/core/browser/password_manager.cc
+++ b/components/password_manager/core/browser/password_manager.cc
@@ -69,6 +69,22 @@ bool ShouldDropSyncCredential() {
   return group_name != "Disabled";
 }
 
+bool EqualsIgnoreScheme(const GURL& src, const GURL& dst) {

[vabr (Chromium), 2014/08/26 09:42:59]

The function name is unclear, and the roles of the arguments are the same,
nothing asymmetric like source and destination (that would indicate that src is
being overwritten by dst).

So I suggest renaming to:
bool URLsEqualUpToScheme(const GURL& a, const GURL& b)
or similar.

[Pritam Nikam, 2014/08/26 12:41:33]

Done.

+  return (src.GetContent() == dst.GetContent());
+}
+
+bool EqualsLoginFormAction(const GURL& src, const GURL& dst) {

[vabr (Chromium), 2014/08/26 09:42:59]

nit: The function is actually oblivious to the fact that the URLs are actions of
some forms. Also the roles of the arguments are the same, nothing asymmetric
like source and destination (that would indicate that src is being overwritten
by dst).

So I suggest renaming to
bool URLsEqualUpToHttpHttpsSubstitution(const GURL& a, const GURL& b)
or similar.

[Pritam Nikam, 2014/08/26 12:41:33]

Done.

+  bool is_same_form = (src == dst);

[vabr (Chromium), 2014/08/26 09:42:59]

I suggest an explicit early exit instead:

if (src == dst)
  return true;

The advantage is that you don't have to test |is_same_form| below, which reduces
the clutter in the if-conditions. Also, it's clear on first sight, that what
goes below is only for unequal URLs.

[Pritam Nikam, 2014/08/26 12:41:33]

Done.

+  // If action URLs are not matching, there is a possiblility that the

[vabr (Chromium), 2014/08/26 09:42:59]

nit: This comment id long enough to harm its own usefulness, and on top of it
incorrect (it suggests, that schemes are ignored even outside of the HTTP-HTTPS
substitution).

Please consider replacing with a short mention of the bug number which contains
a use-case for this exception, e.g.:

  // The first-time and retry login forms action URLs sometimes differ in
switching from HTTP to HTTPS, see http://crbug.com/400769.

[Pritam Nikam, 2014/08/26 12:41:33]

Done.

+  // content-server would have pushed a separate login page. We can extend
+  // heuristics by comparing action URLs by ignoring their schemes
+  // especially for URLs having HTTP or HTTPS schemes.
+  if (!is_same_form && src.SchemeIsHTTPOrHTTPS() && dst.SchemeIsHTTPOrHTTPS())

[vabr (Chromium), 2014/08/26 09:42:59]

If you replace |is_same_form| with the early return above, I suggest to rewrite
the rest as follows:

if (src.SchemeIsHTTPOrHTTPS() && dst.SchemeIsHTTPOrHTTPS())
  return EqualsIgnoreScheme(src, dst);

return false;

That would result in three return statements overall, corresponding to the three
major cases in the code's logic: equal URLs, URLs equal up to HTTP<->HTTPS
substitution, and everything else, improving the code's readability.

[Pritam Nikam, 2014/08/26 12:41:33]

Done.

+    is_same_form = EqualsIgnoreScheme(src, dst);
+
+  return is_same_form;
+}
+
 }  // namespace
 
 const char PasswordManager::kOtherPossibleUsernamesExperiment[] =
@@ -437,12 +453,16 @@ void PasswordManager::OnPasswordFormsRendered(
   // If we see the login form again, then the login failed.
   if (did_stop_loading) {
     for (size_t i = 0; i < all_visible_forms_.size(); ++i) {
-      // TODO(vabr): The similarity check is just action equality for now. If it
-      // becomes more complex, it may make sense to consider modifying and using
-      // PasswordFormManager::DoesManage for it.
+      // TODO(vabr): The similarity check is just action equality for now.
+      // Moreover, for cases where action URLs are not matching, extends

[vabr (Chromium), 2014/08/26 09:42:59]

The comment is out of date again.
My suggestion is to actually keep the very original comment (before this CL),
and just insert "up to HTTP<->HTTPS substitution" after "action equality".

[Pritam Nikam, 2014/08/26 12:41:34]

Done.

+      // heuristics by ignoring their schemes as well as cases especially for
+      // URLs having HTTP or HTTPS schemes.
+      // If it becomes more complex, it may make sense to consider modifying and
+      // using PasswordFormManager::DoesManage for it.
       if (all_visible_forms_[i].action.is_valid() &&
-          provisional_save_manager_->pending_credentials().action ==
-              all_visible_forms_[i].action) {
+          EqualsLoginFormAction(
+              provisional_save_manager_->pending_credentials().action,
+              all_visible_forms_[i].action)) {
         if (logger) {
           logger->LogPasswordForm(Logger::STRING_PASSWORD_FORM_REAPPEARED,
                                   visible_forms[i]);