Prevent Chrome from quitting when apps are open. (Mac)

chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.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/ui/cocoa/apps/quit_with_apps_controller_mac.h"
+
+#include "apps/app_window.h"
+#include "apps/app_window_registry.h"
+#include "apps/pref_names.h"
+#include "apps/ui/native_app_window.h"
+#include "base/command_line.h"
+#include "base/i18n/number_formatting.h"
+#include "base/prefs/pref_service.h"
+#include "base/strings/sys_string_conversions.h"
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/notifications/notification.h"
+#include "chrome/browser/notifications/notification_ui_manager.h"
+#include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/ui/browser_iterator.h"
+#include "chrome/common/chrome_switches.h"
+#include "extensions/common/extension.h"
+#include "grit/chromium_strings.h"
+#include "grit/generated_resources.h"
+#include "grit/google_chrome_strings.h"
+#include "grit/theme_resources.h"
+#include "ui/base/l10n/l10n_util.h"
+#include "ui/base/l10n/l10n_util_mac.h"
+#include "ui/base/resource/resource_bundle.h"
+
+const char QuitWithAppsController::kQuitWithApps[] = "quit-with-apps";
+
+QuitWithAppsController::QuitWithAppsController()
+    : suppress_for_session_(false) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));

[tapted, 2014/04/01 08:09:45]

nit: there's a shiny new DCHECK_CURRENTLY_ON macro

[jackhou1, 2014/04/02 06:38:38]

Done.

+
+  message_center::ButtonInfo quit_apps_button_info(
+      l10n_util::GetStringUTF16(IDS_QUIT_WITH_APPS_QUIT_LABEL));
+  message_center::ButtonInfo suppression_button_info(
+      l10n_util::GetStringUTF16(IDS_QUIT_WITH_APPS_SUPPRESSION_LABEL));
+  message_center::RichNotificationData rich_notification_data;
+  rich_notification_data.buttons.push_back(quit_apps_button_info);
+  rich_notification_data.buttons.push_back(suppression_button_info);
+
+  notification_.reset(new Notification(
+      message_center::NOTIFICATION_TYPE_SIMPLE,
+      GURL("chrome://quit-with-apps"),

[tapted, 2014/04/01 08:09:45]

nit: poking around other notifications, it seems common to oull this constant
out to an an anonymous namespace maybe something like kNotificationOriginUrl or
kQuitWithAppsOriginUrl  (I think it helps explain the arg for notifications
n00bs like me too).

[jackhou1, 2014/04/02 06:38:38]

Done.

+      l10n_util::GetStringUTF16(IDS_QUIT_WITH_APPS_TITLE),
+      l10n_util::GetStringUTF16(IDS_QUIT_WITH_APPS_EXPLANATION),
+      ui::ResourceBundle::GetSharedInstance().GetImageNamed(
+          IDR_APP_DEFAULT_ICON),
+      blink::WebTextDirectionDefault,
+      message_center::NotifierId(message_center::NotifierId::SYSTEM_COMPONENT,
+                                 kQuitWithApps),
+      base::UTF8ToUTF16(kQuitWithApps),

[tapted, 2014/04/01 08:09:45]

should this be localized?

actually this and the next argument are especially hard to grok. Maybe

// There is only ever one notification to replace, so use the same replace_id
each time.
base::string16 replace_id = base::UTF8ToUTF16(id());

// words
base::string16 display_source = ..

I actually can't tell from headers what display_source is used for but other
NotificaitonDelegates do localize it. `IDS_FLAGS_APPS_KEEP_CHROME_ALIVE_NAME`
seems fitting (but maybe hacky).

If it's innocuous, (but stil relevant) maybe TODO(..): Localize the
display_source. 

[jackhou1, 2014/04/02 06:38:38]

Done.

+      base::UTF8ToUTF16(id()),
+      rich_notification_data,
+      this));
+}
+
+QuitWithAppsController::~QuitWithAppsController() {}
+
+void QuitWithAppsController::Display() {}
+
+void QuitWithAppsController::Error() {
+  LOG(ERROR) << "Error displaying notification while quitting with apps open.";
+}
+
+void QuitWithAppsController::Close(bool by_user) {
+  if (by_user)
+    suppress_for_session_ = true;
+}
+
+void QuitWithAppsController::Click() {
+  g_browser_process->notification_ui_manager()->CancelById(id());
+}
+
+void QuitWithAppsController::ButtonClick(int button_index) {
+  typedef apps::AppWindowRegistry::AppWindowList AppWindowList;
+
+  g_browser_process->notification_ui_manager()->CancelById(id());
+  if (button_index == 0) {
+    // Quit all apps by closing all windows.
+    ProfileManager* profile_manager = g_browser_process->profile_manager();
+    if (!profile_manager)
+      return;
+
+    std::vector<Profile*> profiles(profile_manager->GetLoadedProfiles());
+    for (size_t i = 0; i < profiles.size(); ++i) {
+      AppWindowList windows =
+          apps::AppWindowRegistry::Get(profiles[i])->app_windows();
+      for (AppWindowList::const_iterator it = windows.begin();
+           it != windows.end();
+           ++it) {
+        (*it)->GetBaseWindow()->Close();

[tapted, 2014/04/01 08:09:45]

are we sure this is safe? (e.g. did you have any insight on
http://crbug.com/356362)

[jackhou1, 2014/04/02 06:38:38]

I don't have a fix/proof, but this code can be written to avoid it.

The current code actually copies the AppWindowList. I changed it to a while
loop. Not sure which is better. Alternatively we could add a method to
AppWindowRegistry.

[tapted, 2014/04/03 01:45:38]

Yeah, I think a method in a method in AppWindowRegistry would be a lot better.
This is a little mind-bending -- calling methods on items in a const-vector in a
way that modifies the vector gives me a shiver (ideally the items in the vector
would be const as well).

Also, since temporaries bind to const-references, it's a bit fragile (e.g. if
someone changes ::app_windows() to return a non-reference).

[jackhou1, 2014/04/03 03:07:07]

Done.

+      }
+    }
+  } else if (button_index == 1) {
+    // Don't show this again.

[tapted, 2014/04/01 08:09:45]

nit: maybe (in anon namespace)

const int kQuitAllAppsButtonIndex = 0;
const int kDontShowAgainButtonIndex = 1;

then you can ditch the comments :)

[jackhou1, 2014/04/02 06:38:38]

Done.

+    g_browser_process->local_state()->SetBoolean(
+        apps::prefs::kNotifyWhenAppsKeepChromeAlive, false);
+  }
+}
+
+content::RenderViewHost* QuitWithAppsController::GetRenderViewHost() const {
+  return NULL;
+}
+
+std::string QuitWithAppsController::id() const { return kQuitWithApps; }
+
+bool QuitWithAppsController::ShouldQuit() {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));

[tapted, 2014/04/01 08:09:45]

DCHECK_CURRENTLY_ON

[jackhou1, 2014/04/02 06:38:38]

Done.

+
+  if (!CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kAppsKeepChromeAlive)) {
+    g_browser_process->local_state()->ClearPref(
+        apps::prefs::kNotifyWhenAppsKeepChromeAlive);
+    return true;
+  }
+
+  // Quit immediately if there are no windows or the confirmation has been
+  // suppressed.
+  if (!apps::AppWindowRegistry::IsAppWindowRegisteredInAnyProfile(0))
+    return true;
+
+  // If there are browser windows, and this notification has been suppressed for
+  // this session or permanently, then just return false to prevent Chrome from
+  // quitting.
+  bool suppress_always = !g_browser_process->local_state()->GetBoolean(
+      apps::prefs::kNotifyWhenAppsKeepChromeAlive);
+  if (!chrome::BrowserIterator().done() &&

[tapted, 2014/04/01 08:09:45]

Does this mean choosing 'Quit' from the Chrome dock icon with no browser windows
open (but apps open) will always show the notification? (I think that's good,
but subtle - maybe worth a comment).

[jackhou1, 2014/04/02 06:38:38]

Done.

+      (suppress_for_session_ || suppress_always))

[tapted, 2014/04/01 08:09:45]

nit: curlies for multi-line if condition

[jackhou1, 2014/04/02 06:38:38]

Done.

+    return false;
+
+  ProfileManager* profile_manager = g_browser_process->profile_manager();
+  if (!profile_manager)
+    return true;
+
+  std::vector<Profile*> profiles(profile_manager->GetLoadedProfiles());
+  if (!profiles.size())

[tapted, 2014/04/01 08:09:45]

nit: if (profiles.empty()). But also shouldn't this and the check above never
happen, because apps::AppWindowRegistry::IsAppWindowRegisteredInAnyProfile is
guaranteed to return false?

Maybe DCHECK instead

[jackhou1, 2014/04/02 06:38:38]

Done.

+    return true;
+
+  g_browser_process->notification_ui_manager()->CancelById(id());

[tapted, 2014/04/01 08:09:45]

what happens without this line? (e.g. is it necessary, or is there better UX by
cancelling first, since the 'Add' codepaths seem to have logic for replacing
too)

[jackhou1, 2014/04/02 06:38:38]

Yeah, Add() will replace an existing one, which might not show it.

+  g_browser_process->notification_ui_manager()->Add(*notification_,
+                                                    profiles[0]);
+
+  // Always return false, "Quit All Apps" will cause Chrome to quit.

[tapted, 2014/04/01 08:09:45]

this makes it sound like returning false will cause QuitWithApps[Controller] to
quit Chrome _now_. But the opposite is true. maybe say, ~ the notification UI
can be used to quit chrome.

[jackhou1, 2014/04/02 06:38:38]

Done.

+  return false;
+}