Don't compile save_password_infobar_delegate.cc everywhere but Mac and Android

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 f7060ba64fdb09f646ec0401bed1afa3f686d9d6..50c0e32d79335ab594ed74c6a7800dc7d5f58990 100644
--- a/chrome/browser/password_manager/chrome_password_manager_client.cc
+++ b/chrome/browser/password_manager/chrome_password_manager_client.cc
@@ -12,7 +12,6 @@
 #include "base/prefs/pref_service.h"
 #include "chrome/browser/browsing_data/browsing_data_helper.h"
 #include "chrome/browser/password_manager/password_store_factory.h"
-#include "chrome/browser/password_manager/save_password_infobar_delegate.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/browser/sync/profile_sync_service.h"
@@ -49,6 +48,10 @@
 #include "net/base/url_util.h"
 #include "third_party/re2/re2/re2.h"
 
+#if defined(OS_MACOSX) || defined(OS_ANDROID)
+#include "chrome/browser/password_manager/save_password_infobar_delegate.h"
+#endif
+
 #if defined(OS_ANDROID)
 #include "chrome/browser/android/tab_android.h"
 #include "chrome/browser/password_manager/generated_password_saved_infobar_delegate_android.h"
@@ -216,6 +219,7 @@ bool ChromePasswordManagerClient::PromptUserToSaveOrUpdatePassword(
       manage_passwords_ui_controller->OnPasswordSubmitted(form_to_save.Pass());
     }
   } else {
+#if defined(OS_MACOSX) || defined(OS_ANDROID)
     if (form_to_save->IsBlacklisted())
       return false;
     std::string uma_histogram_suffix(
@@ -224,6 +228,9 @@ bool ChromePasswordManagerClient::PromptUserToSaveOrUpdatePassword(
                 form_to_save->pending_credentials().signon_realm, GetPrefs())));
     SavePasswordInfoBarDelegate::Create(
         web_contents(), form_to_save.Pass(), uma_histogram_suffix, type);
+#else
+    LOG(FATAL) << "New bubble UI should be used everywhere but Mac and Android";

[Peter Kasting, 2015/09/08 08:20:00]

Avoid LOG statements if possible; prefer to do nothing, or if you really need
to, NOTREACHED().

[ki.stfu, 2015/09/08 20:39:54]

I worry about the case if IsTheHotNewBubbleUIEnabled() returns false on Mac or
Android. Can I do as follows?
```
  const bool is_the_hot_new_bubble_ui_enabled = IsTheHotNewBubbleUIEnabled();
#if !defined(OS_MACOSX) && !defined(OS_ANDROID)
  DCHECK(is_the_hot_new_bubble_ui_enabled) << "New bubble UI should be used"
      << " everywhere but Mac and Android platforms";
#endif

  if (is_the_hot_new_bubble_ui_enabled) {
```

[Peter Kasting, 2015/09/08 20:50:33]

If you're trying to not compile SavePasswordInfoBarDelegate, you're going to
need to #if out the second part of the conditional as you're currently doing
anyway, or the code won't compile.  What I was suggesting was that you use
NOTREACHED() in place of LOG(FATAL).  We try to avoid logging except in specific
scenarios where we need to be able to collect and see the log data; it basically
means "this shouldn't happen, but it does, and we're debugging why". 
NOTREACHED, OTOH, means that the codepath is never reached.  That's the case
here.

+#endif
   }
   return true;
 }
@@ -509,9 +516,16 @@ bool ChromePasswordManagerClient::IsTheHotNewBubbleUIEnabled() {
 #if !defined(USE_AURA) && !defined(OS_MACOSX)
   return false;
 #endif

[vabr (Chromium), 2015/09/08 12:52:49]

The current changes in this method are not ideal, because
1. There is no reason to offer the command-line flags for the bubble if we don't
want to see it (bonus points if you exclude its definition on Aura platforms as
well).
2. The corresponding field trial is expired (sorry, I should have mentioned it
to you, you have no way to check that), so no need to worry about that.

Also, the negations in the ifdefs are getting hard to read. So I suggest the
following structure of the code:

#if defined(OS_MACOSX) || defined(OS_ANDROID)
  ... (the current checks of flags and field trials)
#elif defined(USE_AURA)
  return true;
#else
  return false;
#endif

[ki.stfu, 2015/09/08 20:39:54]

I worry about the last case:
```
#else
  return false;
#endif
```

FMPOV, we should return "true" there, because "false" is only suitable for Mac
and Android platforms.

[vabr (Chromium), 2015/09/09 09:18:20]

Fair enough. This file only compiles on Android, Mac, Win, Linux and CrOS. The
last 3 always define USE_AURA, and the rest is covered by the #if branch.
So dropping the "return false" is OK.

+
   base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
-  if (command_line->HasSwitch(switches::kDisableSavePasswordBubble))
+  if (command_line->HasSwitch(switches::kDisableSavePasswordBubble)) {
+#if !defined(OS_MACOSX) && !defined(OS_ANDROID)
+    LOG(ERROR) << "--" << switches::kDisableSavePasswordBubble

[Peter Kasting, 2015/09/08 08:20:00]

Same comment.

[ki.stfu, 2015/09/08 20:39:54]

Done.

+        << " is supported on Mac and Android only";
+#else
     return false;
+#endif
+  }
 
   if (command_line->HasSwitch(switches::kEnableSavePasswordBubble))
     return true;