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

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

This is added behind --apps-keep-chrome-alive.

When Chrome is quit, a notification is shown to let the user know that
Chrome will continue running.

The browser session should be shut down like on Windows, but that is
not yet implemented. Similarly, quitting all the apps should quit
Chrome. These will be added in followup CLs.

BUG=333429
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=265889

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266731

[jackhou1, 2014-04-01 03:19:23]

This should implement enough to let people try out the UI and reach a final
decision on the strings and the behavior of "Don't show this again".

[tapted, 2014-04-01 08:09:44]

looks really good overall

https://codereview.chromium.org/220373003/diff/1/apps/pref_names.h
File apps/pref_names.h (right):

https://codereview.chromium.org/220373003/diff/1/apps/pref_names.h#newcode14
apps/pref_names.h:14: extern const char kNotifyWhenAppsKeepChromeAlive[];
I don't think the pref belongs in apps/ -- it's a very chromey thing, and
src/apps is more for when apps stand alone from chrome. (maybe check with
benwells)

there's register prefs in browser_shutdown.h and background_mode_manager.h ..
but none of those feel quite right.

I think you can just add static member function like
QuitWithAppsController::RegisterPrefs and put constants in
chrome/common/pref_names.cc

https://codereview.chromium.org/220373003/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/220373003/diff/1/chrome/browser/about_flags.c...
chrome/browser/about_flags.cc:1417: "apps-keep-chrome-alive",
nit: new flags should be added to the end of kExperiments (see comment on line
429)

https://codereview.chromium.org/220373003/diff/1/chrome/browser/app_controlle...
File chrome/browser/app_controller_mac.h (right):

https://codereview.chromium.org/220373003/diff/1/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.h:31: class QuitWithAppsController;
nit: move to before line 28? I think there should be a line after too.

https://codereview.chromium.org/220373003/diff/1/chrome/browser/ui/cocoa/apps...
File chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc (right):

https://codereview.chromium.org/220373003/diff/1/chrome/browser/ui/cocoa/apps...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:35:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
nit: there's a shiny new DCHECK_CURRENTLY_ON macro

https://codereview.chromium.org/220373003/diff/1/chrome/browser/ui/cocoa/apps...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:47:
GURL("chrome://quit-with-apps"),
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).

https://codereview.chromium.org/220373003/diff/1/chrome/browser/ui/cocoa/apps...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:55:
base::UTF8ToUTF16(kQuitWithApps),
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.

https://codereview.chromium.org/220373003/diff/1/chrome/browser/ui/cocoa/apps...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:95:
(*it)->GetBaseWindow()->Close();
are we sure this is safe? (e.g. did you have any insight on
http://crbug.com/356362)

https://codereview.chromium.org/220373003/diff/1/chrome/browser/ui/cocoa/apps...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:99: // Don't show
this again.
nit: maybe (in anon namespace)

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

then you can ditch the comments :)

https://codereview.chromium.org/220373003/diff/1/chrome/browser/ui/cocoa/apps...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:112:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
DCHECK_CURRENTLY_ON

https://codereview.chromium.org/220373003/diff/1/chrome/browser/ui/cocoa/apps...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:131: if
(!chrome::BrowserIterator().done() &&
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).

https://codereview.chromium.org/220373003/diff/1/chrome/browser/ui/cocoa/apps...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:132:
(suppress_for_session_ || suppress_always))
nit: curlies for multi-line if condition

https://codereview.chromium.org/220373003/diff/1/chrome/browser/ui/cocoa/apps...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:140: if
(!profiles.size())
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

https://codereview.chromium.org/220373003/diff/1/chrome/browser/ui/cocoa/apps...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:143:
g_browser_process->notification_ui_manager()->CancelById(id());
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)

https://codereview.chromium.org/220373003/diff/1/chrome/browser/ui/cocoa/apps...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:147: // Always
return false, "Quit All Apps" will cause Chrome to quit.
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.

https://codereview.chromium.org/220373003/diff/1/chrome/browser/ui/cocoa/apps...
File chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.h (right):

https://codereview.chromium.org/220373003/diff/1/chrome/browser/ui/cocoa/apps...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.h:8: #include
"chrome/browser/notifications/notification.h"
nit: #include scoped_ptr

some would say you should include base/macros.h too (since they were moved from
basictypes.h), but that trend hasn't really started yet.

https://codereview.chromium.org/220373003/diff/1/chrome/browser/ui/cocoa/apps...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.h:16: static const
char kQuitWithApps[];
nit: maybe 'kQuitWithAppsNotificationID'

also I think style guide says these come before the constructor

https://codereview.chromium.org/220373003/diff/1/chrome/browser/ui/cocoa/apps...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.h:36: bool
suppress_for_session_;
nit: maybe a comment for this - i had to look up a meaning

https://codereview.chromium.org/220373003/diff/1/chrome/chrome_browser_ui.gypi
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/220373003/diff/1/chrome/chrome_browser_ui.gyp...
chrome/chrome_browser_ui.gypi:537:
'browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc',
nit: sort

[jackhou1, 2014-04-02 06:38:37]

https://codereview.chromium.org/220373003/diff/1/apps/pref_names.h
File apps/pref_names.h (right):

https://codereview.chromium.org/220373003/diff/1/apps/pref_names.h#newcode14
apps/pref_names.h:14: extern const char kNotifyWhenAppsKeepChromeAlive[];
On 2014/04/01 08:09:45, tapted wrote:
> I don't think the pref belongs in apps/ -- it's a very chromey thing, and
> src/apps is more for when apps stand alone from chrome. (maybe check with
> benwells)
> 
> there's register prefs in browser_shutdown.h and background_mode_manager.h ..
> but none of those feel quite right.
> 
> I think you can just add static member function like
> QuitWithAppsController::RegisterPrefs and put constants in
> chrome/common/pref_names.cc
> 

Done.

https://codereview.chromium.org/220373003/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/220373003/diff/1/chrome/browser/about_flags.c...
chrome/browser/about_flags.cc:1417: "apps-keep-chrome-alive",
On 2014/04/01 08:09:45, tapted wrote:
> nit: new flags should be added to the end of kExperiments (see comment on line
> 429)

Done.

https://codereview.chromium.org/220373003/diff/1/chrome/browser/app_controlle...
File chrome/browser/app_controller_mac.h (right):

https://codereview.chromium.org/220373003/diff/1/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.h:31: class QuitWithAppsController;
On 2014/04/01 08:09:45, tapted wrote:
> nit: move to before line 28? I think there should be a line after too.

Done.

https://codereview.chromium.org/220373003/diff/1/chrome/browser/ui/cocoa/apps...
File chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc (right):

https://codereview.chromium.org/220373003/diff/1/chrome/browser/ui/cocoa/apps...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:35:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
On 2014/04/01 08:09:45, tapted wrote:
> nit: there's a shiny new DCHECK_CURRENTLY_ON macro

Done.

https://codereview.chromium.org/220373003/diff/1/chrome/browser/ui/cocoa/apps...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:47:
GURL("chrome://quit-with-apps"),
On 2014/04/01 08:09:45, tapted wrote:
> 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).

Done.

https://codereview.chromium.org/220373003/diff/1/chrome/browser/ui/cocoa/apps...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:55:
base::UTF8ToUTF16(kQuitWithApps),
On 2014/04/01 08:09:45, tapted wrote:
> 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. 

Done.

https://codereview.chromium.org/220373003/diff/1/chrome/browser/ui/cocoa/apps...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:95:
(*it)->GetBaseWindow()->Close();
On 2014/04/01 08:09:45, tapted wrote:
> are we sure this is safe? (e.g. did you have any insight on
> http://crbug.com/356362)

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.

https://codereview.chromium.org/220373003/diff/1/chrome/browser/ui/cocoa/apps...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:99: // Don't show
this again.
On 2014/04/01 08:09:45, tapted wrote:
> nit: maybe (in anon namespace)
> 
> const int kQuitAllAppsButtonIndex = 0;
> const int kDontShowAgainButtonIndex = 1;
> 
> then you can ditch the comments :)

Done.

https://codereview.chromium.org/220373003/diff/1/chrome/browser/ui/cocoa/apps...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:112:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
On 2014/04/01 08:09:45, tapted wrote:
> DCHECK_CURRENTLY_ON

Done.

https://codereview.chromium.org/220373003/diff/1/chrome/browser/ui/cocoa/apps...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:131: if
(!chrome::BrowserIterator().done() &&
On 2014/04/01 08:09:45, tapted wrote:
> 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).

Done.

https://codereview.chromium.org/220373003/diff/1/chrome/browser/ui/cocoa/apps...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:132:
(suppress_for_session_ || suppress_always))
On 2014/04/01 08:09:45, tapted wrote:
> nit: curlies for multi-line if condition

Done.

https://codereview.chromium.org/220373003/diff/1/chrome/browser/ui/cocoa/apps...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:140: if
(!profiles.size())
On 2014/04/01 08:09:45, tapted wrote:
> 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

Done.

https://codereview.chromium.org/220373003/diff/1/chrome/browser/ui/cocoa/apps...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:143:
g_browser_process->notification_ui_manager()->CancelById(id());
On 2014/04/01 08:09:45, tapted wrote:
> 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)

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

https://codereview.chromium.org/220373003/diff/1/chrome/browser/ui/cocoa/apps...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:147: // Always
return false, "Quit All Apps" will cause Chrome to quit.
On 2014/04/01 08:09:45, tapted wrote:
> 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.

Done.

https://codereview.chromium.org/220373003/diff/1/chrome/browser/ui/cocoa/apps...
File chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.h (right):

https://codereview.chromium.org/220373003/diff/1/chrome/browser/ui/cocoa/apps...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.h:8: #include
"chrome/browser/notifications/notification.h"
On 2014/04/01 08:09:45, tapted wrote:
> nit: #include scoped_ptr
> 
> some would say you should include base/macros.h too (since they were moved
from
> basictypes.h), but that trend hasn't really started yet.

Done.

https://codereview.chromium.org/220373003/diff/1/chrome/browser/ui/cocoa/apps...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.h:16: static const
char kQuitWithApps[];
On 2014/04/01 08:09:45, tapted wrote:
> nit: maybe 'kQuitWithAppsNotificationID'
> 
> also I think style guide says these come before the constructor

Done.

https://codereview.chromium.org/220373003/diff/1/chrome/browser/ui/cocoa/apps...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.h:36: bool
suppress_for_session_;
On 2014/04/01 08:09:45, tapted wrote:
> nit: maybe a comment for this - i had to look up a meaning

Done.

https://codereview.chromium.org/220373003/diff/1/chrome/chrome_browser_ui.gypi
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/220373003/diff/1/chrome/chrome_browser_ui.gyp...
chrome/chrome_browser_ui.gypi:537:
'browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc',
On 2014/04/01 08:09:45, tapted wrote:
> nit: sort

Done.

[tapted, 2014-04-03 01:45:37]

https://codereview.chromium.org/220373003/diff/1/chrome/browser/ui/cocoa/apps...
File chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc (right):

https://codereview.chromium.org/220373003/diff/1/chrome/browser/ui/cocoa/apps...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:95:
(*it)->GetBaseWindow()->Close();
On 2014/04/02 06:38:38, jackhou1 wrote:
> On 2014/04/01 08:09:45, tapted wrote:
> > are we sure this is safe? (e.g. did you have any insight on
> > http://crbug.com/356362)
> 
> 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.

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

https://codereview.chromium.org/220373003/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc (right):

https://codereview.chromium.org/220373003/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:105: while
(windows.size())
(once moved): !windows.empty() -- AppWindowList is *actually* a list, so .size()
is O(n).. at least until C++11 is properly implemented (trivia: gcc implemented
it but had to revert due to ABI incompatability
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49561)

https://codereview.chromium.org/220373003/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:148: if
(!profile_manager)
nit: this can maybe be a DCHECK too (for same reasons as !profiles.empty below)

https://codereview.chromium.org/220373003/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:164: void
QuitWithAppsController::RegisterPrefs(PrefRegistrySimple* registry) {
nit: // static

[jackhou1, 2014-04-03 03:07:06]

https://codereview.chromium.org/220373003/diff/1/chrome/browser/ui/cocoa/apps...
File chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc (right):

https://codereview.chromium.org/220373003/diff/1/chrome/browser/ui/cocoa/apps...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:95:
(*it)->GetBaseWindow()->Close();
On 2014/04/03 01:45:38, tapted wrote:
> On 2014/04/02 06:38:38, jackhou1 wrote:
> > On 2014/04/01 08:09:45, tapted wrote:
> > > are we sure this is safe? (e.g. did you have any insight on
> > > http://crbug.com/356362)
> > 
> > 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.
> 
> 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).

Done.

https://codereview.chromium.org/220373003/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc (right):

https://codereview.chromium.org/220373003/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:105: while
(windows.size())
On 2014/04/03 01:45:38, tapted wrote:
> (once moved): !windows.empty() -- AppWindowList is *actually* a list, so
.size()
> is O(n).. at least until C++11 is properly implemented (trivia: gcc
implemented
> it but had to revert due to ABI incompatability
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49561)

Done.

https://codereview.chromium.org/220373003/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:148: if
(!profile_manager)
On 2014/04/03 01:45:38, tapted wrote:
> nit: this can maybe be a DCHECK too (for same reasons as !profiles.empty
below)

Done.

https://codereview.chromium.org/220373003/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:164: void
QuitWithAppsController::RegisterPrefs(PrefRegistrySimple* registry) {
On 2014/04/03 01:45:38, tapted wrote:
> nit: // static

Done.

[tapted, 2014-04-03 03:49:15]

lgtm with a nit

https://codereview.chromium.org/220373003/diff/40001/apps/app_window_registry.cc
File apps/app_window_registry.cc (right):

https://codereview.chromium.org/220373003/diff/40001/apps/app_window_registry...
apps/app_window_registry.cc:245: while (!app_windows.empty())
nit: I would just use !registry->app_windows_.empty() (and
registry->app_windows_.front() below) - I don't think there are any style rules
against accessing private members like this from a static function.

[I'm still scared by that const-reference ;) ]

[jackhou1, 2014-04-03 04:00:51]

https://codereview.chromium.org/220373003/diff/40001/apps/app_window_registry.cc
File apps/app_window_registry.cc (right):

https://codereview.chromium.org/220373003/diff/40001/apps/app_window_registry...
apps/app_window_registry.cc:245: while (!app_windows.empty())
On 2014/04/03 03:49:15, tapted wrote:
> nit: I would just use !registry->app_windows_.empty() (and
> registry->app_windows_.front() below) - I don't think there are any style
rules
> against accessing private members like this from a static function.
> 
> [I'm still scared by that const-reference ;) ]

Done.

[jackhou1, 2014-04-03 04:02:43]

thakis, please review for OWNERS:

chrome/browser/app_controller_mac.h
chrome/browser/app_controller_mac.mm
chrome/browser/prefs/browser_prefs.cc

[jackhou1, 2014-04-08 23:54:05]

Ping.

[Nico, 2014-04-22 22:32:02]

Looks like the UI has been discussed in great detail on the bug, so I won't
comment on that. Code-wise, this is good work, lgtm.

https://codereview.chromium.org/220373003/diff/60001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc (right):

https://codereview.chromium.org/220373003/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:32: namespace {
const has implicit internal linkage, no need for namespace

https://codereview.chromium.org/220373003/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:80: LOG(ERROR) <<
"Error displaying notification while quitting with apps open.";
Is this a useful log message? Try to minimize logspam.

https://codereview.chromium.org/220373003/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:105: return NULL;
Huh, is this allowed?

https://codereview.chromium.org/220373003/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:143: // Delete any
existing notification to ensure this one is shown.
This only cancels previous versions of the same notification, it doesn't
override pending notifications, right? If I come back from vacation and calednar
has queued up 20 notifications while I was away, the notification from this will
be at the end of the queue?

(Since the function returns false, I think that's fine, just checking.)

https://codereview.chromium.org/220373003/diff/60001/chrome/browser/ui/cocoa/...
File
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac_interactive_uitest.cc
(right):

https://codereview.chromium.org/220373003/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac_interactive_uitest.cc:59:
IN_PROC_BROWSER_TEST_F(QuitWithAppsControllerInteractiveTest, QuitBehavior) {
<3

[jackhou1, 2014-04-24 04:29:21]

https://codereview.chromium.org/220373003/diff/60001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc (right):

https://codereview.chromium.org/220373003/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:32: namespace {
On 2014/04/22 22:32:02, Nico wrote:
> const has implicit internal linkage, no need for namespace

Done.

https://codereview.chromium.org/220373003/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:80: LOG(ERROR) <<
"Error displaying notification while quitting with apps open.";
On 2014/04/22 22:32:02, Nico wrote:
> Is this a useful log message? Try to minimize logspam.

Done.

https://codereview.chromium.org/220373003/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:105: return NULL;
On 2014/04/22 22:32:02, Nico wrote:
> Huh, is this allowed?

Yeah, the comment in c/b/notifications/notification_delegate.h says it can be
NULL. Updated the signature since it's changed in ToT.

https://codereview.chromium.org/220373003/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac.cc:143: // Delete any
existing notification to ensure this one is shown.
On 2014/04/22 22:32:02, Nico wrote:
> This only cancels previous versions of the same notification, it doesn't
> override pending notifications, right? If I come back from vacation and
calednar
> has queued up 20 notifications while I was away, the notification from this
will
> be at the end of the queue?
> 
> (Since the function returns false, I think that's fine, just checking.)

Yup that's correct.

[jackhou1, 2014-04-24 04:29:30]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2014-04-24 05:02:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jackhou@chromium.org/220373003/80001

[commit-bot: I haz the power, 2014-04-24 08:44:37]

Change committed as 265889

[Alexander Potapenko, 2014-04-24 11:52:42]

On 2014/04/24 08:44:37, I haz the power (commit-bot) wrote:
> Change committed as 265889

This CL has broken interactive_ui_tests under Mac ASan
(http://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%20Tests%20%28...):

=================================================================
==2343==ERROR: AddressSanitizer: heap-use-after-free on address 0x17a12f80 at pc
0x7a097 bp 0xbff98118 sp 0xbff98114
READ of size 4 at 0x17a12f80 thread T0
    #0 0x7a096 in extensions::PlatformAppBrowserTest::GetAppWindowCount() (in
interactive_ui_tests) + 310
    #1 0x203079 in
QuitWithAppsControllerInteractiveTest_QuitBehavior_Test::RunTestOnMainThread()
(in interactive_ui_tests) + 16137
    #2 0x206052 in non-virtual thunk to
QuitWithAppsControllerInteractiveTest_QuitBehavior_Test::RunTestOnMainThread()
(in interactive_ui_tests) + 82
    #3 0x28f9633 in InProcessBrowserTest::RunTestOnMainThreadLoop() (in
interactive_ui_tests) + 995
    #4 0xeb6b882 in base::internal::Invoker<1,
base::internal::BindState<base::internal::RunnableAdapter<void
(content::BrowserTestBase::*)()>, void (content::BrowserTestBase*), void
()(content::BrowserTestBase*)>, void
()(content::BrowserTestBase*)>::Run(base::internal::BindStateBase*) (in
interactive_ui_tests) + 146
    #5 0x1530aa6 in ChromeBrowserMainParts::PreMainMessageLoopRunImpl() (in
interactive_ui_tests) + 16214
    #6 0x152c603 in ChromeBrowserMainParts::PreMainMessageLoopRun() (in
interactive_ui_tests) + 275
    #7 0x53cea98 in content::BrowserMainLoop::PreMainMessageLoopRun() (in
interactive_ui_tests) + 488
    #8 0x53d562f in base::internal::Invoker<1,
base::internal::BindState<base::internal::RunnableAdapter<int
(content::BrowserMainLoop::*)()>, int (content::BrowserMainLoop*), void
(base::internal::UnretainedWrapper<content::BrowserMainLoop>)>, int
()(content::BrowserMainLoop*)>::Run(base::internal::BindStateBase*) (in
interactive_ui_tests) + 143
    #9 0x5c1d7ee in content::StartupTaskRunner::RunAllTasksNow() (in
interactive_ui_tests) + 318
    #10 0x53ca070 in content::BrowserMainLoop::CreateStartupTasks() (in
interactive_ui_tests) + 2096
    #11 0x53d63a4 in
content::BrowserMainRunnerImpl::Initialize(content::MainFunctionParams const&)
(in interactive_ui_tests) + 1204
    #12 0x53c118b in content::BrowserMain(content::MainFunctionParams const&)
(in interactive_ui_tests) + 443
    #13 0xeb65a78 in content::RunNamedProcessTypeMain(std::string const&,
content::MainFunctionParams const&, content::ContentMainDelegate*) (in
interactive_ui_tests) + 392
    #14 0xeb6785b in content::ContentMainRunnerImpl::Run() (in
interactive_ui_tests) + 539
    #15 0xeb65393 in content::ContentMain(content::ContentMainParams const&) (in
interactive_ui_tests) + 195
    #16 0xeb6a997 in content::BrowserTestBase::SetUp() (in interactive_ui_tests)
+ 1895
    #17 0x28f6ae3 in InProcessBrowserTest::SetUp() (in interactive_ui_tests) +
771
    #18 0x11d53e in non-virtual thunk to ExtensionBrowserTest::SetUp() (in
interactive_ui_tests) + 398
...

0x17a12f80 is located 128 bytes inside of 332-byte region
[0x17a12f00,0x17a1304c)
freed by thread T0 here:
    #0 0x14a0856b in wrap__ZdlPv (in libclang_rt.asan_osx_dynamic.dylib) + 91
    #1 0xb83edc7 in -[BrowserWindowController .cxx_destruct] (in
interactive_ui_tests) + 1415
    #2 0x94f0d416 in object_cxxDestructFromClass(objc_object*, objc_class*) (in
libobjc.A.dylib) + 91
    #3 0x94f04ab9 in objc_destructInstance (in libobjc.A.dylib) + 39
    #4 0xbfc43ea in (anonymous namespace)::ZombieDealloc(objc_object*,
objc_selector*) (in interactive_ui_tests) + 266
    #5 0x943eeab0 in -[NSResponder dealloc] (in AppKit) + 155
    #6 0x9431cb87 in -[NSWindowController dealloc] (in AppKit) + 737
    #7 0xb832d98 in -[BrowserWindowController dealloc] (in interactive_ui_tests)
+ 712
    #8 0x94f1b8fe in -[NSObject release] (in libobjc.A.dylib) + 46
    #9 0x9449655d in -[NSWindowController release] (in AppKit) + 152
    #10 0x9779221f in __delayedPerformCleanup (in Foundation) + 51
    #11 0x95bf767f in CFRunLoopTimerInvalidate (in CoreFoundation) + 623
    #12 0x95bf6ddc in __CFRunLoopDoTimer (in CoreFoundation) + 764
    #13 0x95bdbbb1 in __CFRunLoopRun (in CoreFoundation) + 1841
    #14 0x95bdb019 in CFRunLoopRunSpecific (in CoreFoundation) + 377
    #15 0x95bdae8a in CFRunLoopRunInMode (in CoreFoundation) + 122
    #16 0x906fff59 in RunCurrentEventLoopInMode (in HIToolbox) + 241
    #17 0x906ffcc8 in ReceiveNextEventCommon (in HIToolbox) + 373
    #18 0x906ffb43 in BlockUntilNextEventMatchingListInMode (in HIToolbox) + 87
    #19 0x943f9939 in _DPSNextEvent (in AppKit) + 723
    #20 0x943f916b in -[NSApplication
nextEventMatchingMask:untilDate:inMode:dequeue:] (in AppKit) + 118
    #21 0x943ef5cb in -[NSApplication run] (in AppKit) + 854
    #22 0x2911c9a in
base::MessagePumpNSApplication::DoRun(base::MessagePump::Delegate*) (in
interactive_ui_tests) + 1146
    #23 0x2910070 in
base::MessagePumpCFRunLoopBase::Run(base::MessagePump::Delegate*) (in
interactive_ui_tests) + 480
...
previously allocated by thread T0 here:
    #0 0x14a07fab in wrap__Znwm (in libclang_rt.asan_osx_dynamic.dylib) + 91
    #1 0xbb32e26 in StartupBrowserCreatorImpl::OpenTabsInBrowser(Browser*, bool,
std::vector<StartupTab, std::allocator<StartupTab> > const&,
chrome::HostDesktopType) (in interactive_ui_tests) + 550
    #2 0xbb2f266 in
StartupBrowserCreatorImpl::ProcessSpecifiedURLs(std::vector<GURL,
std::allocator<GURL> > const&, chrome::HostDesktopType) (in
interactive_ui_tests) + 1702
    #3 0xbb2e7bc in
StartupBrowserCreatorImpl::ProcessStartupURLs(std::vector<GURL,
std::allocator<GURL> > const&, chrome::HostDesktopType) (in
interactive_ui_tests) + 1756
    #4 0xbb2c7d0 in StartupBrowserCreatorImpl::ProcessLaunchURLs(bool,
std::vector<GURL, std::allocator<GURL> > const&, chrome::HostDesktopType) (in
interactive_ui_tests) + 832
    #5 0xbb2ab7a in StartupBrowserCreatorImpl::Launch(Profile*,
std::vector<GURL, std::allocator<GURL> > const&, bool, chrome::HostDesktopType)
(in interactive_ui_tests) + 3306
    #6 0xbb22c80 in StartupBrowserCreator::LaunchBrowser(base::CommandLine
const&, Profile*, base::FilePath const&, chrome::startup::IsProcessStartup,
chrome::startup::IsFirstRun, int*) (in interactive_ui_tests) + 1040
    #7 0xbb26b93 in StartupBrowserCreator::ProcessCmdLineImpl(base::CommandLine
const&, base::FilePath const&, bool, Profile*, std::vector<Profile*,
std::allocator<Profile*> > const&, int*, StartupBrowserCreator*) (in
interactive_ui_tests) + 8259
    #8 0x153053f in ChromeBrowserMainParts::PreMainMessageLoopRunImpl() (in
interactive_ui_tests) + 14831
    #9 0x152c603 in ChromeBrowserMainParts::PreMainMessageLoopRun() (in
interactive_ui_tests) + 275
    #10 0x53cea98 in content::BrowserMainLoop::PreMainMessageLoopRun() (in
interactive_ui_tests) + 488
    #11 0x53d562f in base::internal::Invoker<1,
base::internal::BindState<base::internal::RunnableAdapter<int
(content::BrowserMainLoop::*)()>, int (content::BrowserMainLoop*), void
(base::internal::UnretainedWrapper<content::BrowserMainLoop>)>, int
()(content::BrowserMainLoop*)>::Run(base::internal::BindStateBase*) (in
interactive_ui_tests) + 143
...

[jackhou1, 2014-04-28 01:49:26]

Thanks for handling this.

The problem is that PlatformAppBrowserTest::GetAppWindowCount() calls
InProcessBrowserTest::browser(), which returns an invalidated pointer if the
browser has been closed.

Changed this to use AppWindowRegistry::IsAppWindowRegisteredInAnyProfile()
instead.

[jackhou1, 2014-04-28 01:49:44]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2014-04-28 01:50:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jackhou@chromium.org/220373003/100001

[commit-bot: I haz the power, 2014-04-28 01:50:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-28 01:50:43]

Failed to apply patch for chrome/browser/about_flags.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/about_flags.cc
  Hunk #1 FAILED at 1796.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/about_flags.cc.rej

Patch:       chrome/browser/about_flags.cc
Index: chrome/browser/about_flags.cc
diff --git a/chrome/browser/about_flags.cc b/chrome/browser/about_flags.cc
index
3554ed797c42238cc923ab5147b31aef1680e117..3b2f6881532ce37ebf033601ce054ca9214f5c75
100644
--- a/chrome/browser/about_flags.cc
+++ b/chrome/browser/about_flags.cc
@@ -1796,6 +1796,15 @@ const Experiment kExperiments[] = {
     SINGLE_VALUE_TYPE(chromeos::switches::kEnableFileManagerMTP)
   },
 #endif
+#if defined(OS_MACOSX)
+  {
+    "apps-keep-chrome-alive",
+    IDS_FLAGS_APPS_KEEP_CHROME_ALIVE_NAME,
+    IDS_FLAGS_APPS_KEEP_CHROME_ALIVE_DESCRIPTION,
+    kOsMac,
+    SINGLE_VALUE_TYPE(switches::kAppsKeepChromeAlive)
+  }
+#endif
 };
 
 const Experiment* experiments = kExperiments;

[tapted, 2014-04-28 01:51:12]

cool - lgtm

[jackhou1, 2014-04-28 03:08:45]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2014-04-28 03:09:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jackhou@chromium.org/220373003/120001

[commit-bot: I haz the power, 2014-04-28 03:15:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-28 03:15:42]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[jackhou1, 2014-04-28 05:20:37]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2014-04-28 05:21:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jackhou@chromium.org/220373003/120001

[commit-bot: I haz the power, 2014-04-28 06:01:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-28 06:01:17]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[jackhou1, 2014-04-28 07:07:43]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2014-04-28 07:09:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jackhou@chromium.org/220373003/120001

[commit-bot: I haz the power, 2014-04-28 07:42:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-28 07:42:32]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[jackhou1, 2014-04-29 01:22:07]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2014-04-29 01:22:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jackhou@chromium.org/220373003/120001

[commit-bot: I haz the power, 2014-04-29 01:51:46]

Change committed as 266731