Restart running apps when chrome restarts.

Restart running apps when chrome restarts.

This adds the event chrome.app.runtime.onRestarted, which apps can use to
restore their state after a restart.


BUG=138676
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156659

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=161827

[benwells, 2012-08-23 06:54:53]

Quick comments from someone unwise in the way of session restore - would it make
sense to encapsulate this in a new PackagedAppRestore class?

https://chromiumcodereview.appspot.com/10875027/diff/1/chrome/browser/session...
File chrome/browser/sessions/session_restore.cc (right):

https://chromiumcodereview.appspot.com/10875027/diff/1/chrome/browser/session...
chrome/browser/sessions/session_restore.cc:743:
extensions::AppEventRouter::DispatchOnRestartEvent(profile_, extension);
Indenting of this block and empty line at top.

[koz (OOO until 15th September), 2012-08-23 07:27:18]

Changed reviewer to sky, who I think knows about session restore stuff.

[koz (OOO until 15th September), 2012-08-23 07:30:12]

https://chromiumcodereview.appspot.com/10875027/diff/1/chrome/browser/session...
File chrome/browser/sessions/session_restore.cc (right):

https://chromiumcodereview.appspot.com/10875027/diff/1/chrome/browser/session...
chrome/browser/sessions/session_restore.cc:743:
extensions::AppEventRouter::DispatchOnRestartEvent(profile_, extension);
On 2012/08/23 06:54:53, benwells wrote:
> Indenting of this block and empty line at top.

Done.

[marja, 2012-08-23 07:31:18]

Drive-by:

https://chromiumcodereview.appspot.com/10875027/diff/1/chrome/browser/session...
File chrome/browser/sessions/session_restore.cc (right):

https://chromiumcodereview.appspot.com/10875027/diff/1/chrome/browser/session...
chrome/browser/sessions/session_restore.cc:738: for
(std::set<std::string>::const_iterator it = apps->begin();
Is there a specific reason why this is done first, and not after creating the
windows?

https://chromiumcodereview.appspot.com/10875027/diff/1/chrome/browser/session...
File chrome/browser/sessions/session_service.cc (right):

https://chromiumcodereview.appspot.com/10875027/diff/1/chrome/browser/session...
chrome/browser/sessions/session_service.cc:22: #include
"chrome/browser/extensions/extension_host.h"
The #include order is wrong.

https://chromiumcodereview.appspot.com/10875027/diff/1/chrome/browser/session...
chrome/browser/sessions/session_service.cc:1498: void
SessionService::ScheduleReset() {
You'd need some code to recreate the app started / stopped information about the
apps here.

You can try this out by setting kWritesPerReset above to a low value, so you'll
force recreating the session restore data repeatedly. Afaics, in this version,
the commands (kCommandAppStarted and kCommandAppStopped) you've added will get
lost after a reset.

https://chromiumcodereview.appspot.com/10875027/diff/1/chrome/browser/session...
File chrome/browser/sessions/session_service.h (right):

https://chromiumcodereview.appspot.com/10875027/diff/1/chrome/browser/session...
chrome/browser/sessions/session_service.h:489:
std::set<extensions::ExtensionHost*> active_extensions_;
Could you add a comment explaining what this is for? Or is this needed at all?
The .cc file seems to insert into this but never delete or read...

[koz (OOO until 15th September), 2012-08-23 07:59:04]

Thanks marja!

https://chromiumcodereview.appspot.com/10875027/diff/1/chrome/browser/session...
File chrome/browser/sessions/session_restore.cc (right):

https://chromiumcodereview.appspot.com/10875027/diff/1/chrome/browser/session...
chrome/browser/sessions/session_restore.cc:738: for
(std::set<std::string>::const_iterator it = apps->begin();
On 2012/08/23 07:31:18, marja wrote:
> Is there a specific reason why this is done first, and not after creating the
> windows?

That's a good question. In fact, this doesn't need to go inside this function at
all. I've moved this code out into ProcessSessionApps().

https://chromiumcodereview.appspot.com/10875027/diff/1/chrome/browser/session...
File chrome/browser/sessions/session_service.cc (right):

https://chromiumcodereview.appspot.com/10875027/diff/1/chrome/browser/session...
chrome/browser/sessions/session_service.cc:22: #include
"chrome/browser/extensions/extension_host.h"
On 2012/08/23 07:31:18, marja wrote:
> The #include order is wrong.

Done.

https://chromiumcodereview.appspot.com/10875027/diff/1/chrome/browser/session...
chrome/browser/sessions/session_service.cc:1498: void
SessionService::ScheduleReset() {
On 2012/08/23 07:31:18, marja wrote:
> You'd need some code to recreate the app started / stopped information about
the
> apps here.
> 
> You can try this out by setting kWritesPerReset above to a low value, so
you'll
> force recreating the session restore data repeatedly. Afaics, in this version,
> the commands (kCommandAppStarted and kCommandAppStopped) you've added will get
> lost after a reset.

Ah, I see. Yes, you're right. I've added a BuildCommandsFromRunningApps() which
is analogous to BuildCommandsFromBrowsers().

https://chromiumcodereview.appspot.com/10875027/diff/1/chrome/browser/session...
File chrome/browser/sessions/session_service.h (right):

https://chromiumcodereview.appspot.com/10875027/diff/1/chrome/browser/session...
chrome/browser/sessions/session_service.h:489:
std::set<extensions::ExtensionHost*> active_extensions_;
On 2012/08/23 07:31:18, marja wrote:
> Could you add a comment explaining what this is for? Or is this needed at all?
> The .cc file seems to insert into this but never delete or read...

This was dead code, but after your comments RE resetting I've repurposed this
and added a comment.

[sky, 2012-08-23 16:23:52]

I'm not sure this is going to work correctly. In particular when you close the
last browser with apps open then create a new browser we trigger restore. In
such a situation we wouldn't want to restore the apps. It may be that we need to
persist the set of open apps else where (perhaps similar to how we track pinned
tabs).

https://chromiumcodereview.appspot.com/10875027/diff/8001/chrome/browser/sess...
File chrome/browser/sessions/session_restore.cc (right):

https://chromiumcodereview.appspot.com/10875027/diff/8001/chrome/browser/sess...
chrome/browser/sessions/session_restore.cc:1056: std::set<std::string> apps_;
newline between 1056/1057 and document what apps_ is.

https://chromiumcodereview.appspot.com/10875027/diff/8001/chrome/browser/sess...
File chrome/browser/sessions/session_service.cc (right):

https://chromiumcodereview.appspot.com/10875027/diff/8001/chrome/browser/sess...
chrome/browser/sessions/session_service.cc:1333: if
(!command_pickle->ReadString(&iter, &extension_id)) {
no {} here and 1344

https://chromiumcodereview.appspot.com/10875027/diff/8001/chrome/browser/sess...
File chrome/browser/sessions/session_service.h (right):

https://chromiumcodereview.appspot.com/10875027/diff/8001/chrome/browser/sess...
chrome/browser/sessions/session_service.h:35: class ExtensionHost;
don't indent.

https://chromiumcodereview.appspot.com/10875027/diff/8001/chrome/browser/sess...
chrome/browser/sessions/session_service.h:185: std::set<std::string>*)>
Can set the set be by reference?

https://chromiumcodereview.appspot.com/10875027/diff/8001/chrome/browser/sess...
chrome/browser/sessions/session_service.h:275: // Converts the commands into
SessionWindows. On return any valid
Update description.

https://chromiumcodereview.appspot.com/10875027/diff/8001/chrome/browser/sess...
chrome/browser/sessions/session_service.h:495:
std::set<extensions::ExtensionHost*> running_apps_;
I think this should be ordered. Otherwise when you restart the apps won't start
in the order they were launched.

[koz (OOO until 15th September), 2012-08-24 02:08:35]

On 2012/08/23 16:23:52, sky wrote:
> I'm not sure this is going to work correctly. In particular when you close the
> last browser with apps open then create a new browser we trigger restore. In
> such a situation we wouldn't want to restore the apps. It may be that we need
to
> persist the set of open apps else where (perhaps similar to how we track
pinned
> tabs).

I wasn't aware of this behaviour. Could you clarify how one would trigger that?
I tried closing the last browser window with an app running and created a new
browser window by running the chrome binary again, but it didn't seem to trigger
a restore.

[koz (OOO until 15th September), 2012-08-24 02:08:57]

https://chromiumcodereview.appspot.com/10875027/diff/8001/chrome/browser/sess...
File chrome/browser/sessions/session_restore.cc (right):

https://chromiumcodereview.appspot.com/10875027/diff/8001/chrome/browser/sess...
chrome/browser/sessions/session_restore.cc:1056: std::set<std::string> apps_;
On 2012/08/23 16:23:52, sky wrote:
> newline between 1056/1057 and document what apps_ is.

Done.

https://chromiumcodereview.appspot.com/10875027/diff/8001/chrome/browser/sess...
File chrome/browser/sessions/session_service.cc (right):

https://chromiumcodereview.appspot.com/10875027/diff/8001/chrome/browser/sess...
chrome/browser/sessions/session_service.cc:1333: if
(!command_pickle->ReadString(&iter, &extension_id)) {
On 2012/08/23 16:23:52, sky wrote:
> no {} here and 1344

Done.

https://chromiumcodereview.appspot.com/10875027/diff/8001/chrome/browser/sess...
File chrome/browser/sessions/session_service.h (right):

https://chromiumcodereview.appspot.com/10875027/diff/8001/chrome/browser/sess...
chrome/browser/sessions/session_service.h:35: class ExtensionHost;
On 2012/08/23 16:23:52, sky wrote:
> don't indent.

Done.

https://chromiumcodereview.appspot.com/10875027/diff/8001/chrome/browser/sess...
chrome/browser/sessions/session_service.h:185: std::set<std::string>*)>
On 2012/08/23 16:23:52, sky wrote:
> Can set the set be by reference?

Done.

https://chromiumcodereview.appspot.com/10875027/diff/8001/chrome/browser/sess...
chrome/browser/sessions/session_service.h:275: // Converts the commands into
SessionWindows. On return any valid
On 2012/08/23 16:23:52, sky wrote:
> Update description.

Done.

https://chromiumcodereview.appspot.com/10875027/diff/8001/chrome/browser/sess...
chrome/browser/sessions/session_service.h:495:
std::set<extensions::ExtensionHost*> running_apps_;
On 2012/08/23 16:23:52, sky wrote:
> I think this should be ordered. Otherwise when you restart the apps won't
start
> in the order they were launched.

Due to the asynchronous nature in which apps run I don't think it matters which
order they are started in. That is, we restart the apps by dispatching an
onRestarted() event to them, which will be delivered asynchronously.

[sky, 2012-08-24 17:16:58]

On Thu, Aug 23, 2012 at 7:08 PM,  <koz@chromium.org> wrote:
> On 2012/08/23 16:23:52, sky wrote:
>>
>> I'm not sure this is going to work correctly. In particular when you close
>> the
>> last browser with apps open then create a new browser we trigger restore.
>> In
>> such a situation we wouldn't want to restore the apps. It may be that we
>> need
>
> to
>>
>> persist the set of open apps else where (perhaps similar to how we track
>
> pinned
>>
>> tabs).
>
>
> I wasn't aware of this behaviour. Could you clarify how one would trigger
> that?
> I tried closing the last browser window with an app running and created a
> new
> browser window by running the chrome binary again, but it didn't seem to
> trigger
> a restore.
>
> https://chromiumcodereview.appspot.com/10875027/

Maybe its only appsv1 that triggers this behavior, although I'm
surprised appsv2 doesn't trigger it. Here's a way you can see it:

. turn on session restore.
. open a couple of tabs in a non-incognito window.
. create an incognito window.
. close the non-incognito window.
. hit control-n

  -Scott

[sky, 2012-08-24 17:17:24]

On Thu, Aug 23, 2012 at 7:08 PM,  <koz@chromium.org> wrote:
>
>
https://chromiumcodereview.appspot.com/10875027/diff/8001/chrome/browser/sess...
> File chrome/browser/sessions/session_restore.cc (right):
>
>
https://chromiumcodereview.appspot.com/10875027/diff/8001/chrome/browser/sess...
> chrome/browser/sessions/session_restore.cc:1056: std::set<std::string>
> apps_;
> On 2012/08/23 16:23:52, sky wrote:
>>
>> newline between 1056/1057 and document what apps_ is.
>
>
> Done.
>
>
>
https://chromiumcodereview.appspot.com/10875027/diff/8001/chrome/browser/sess...
> File chrome/browser/sessions/session_service.cc (right):
>
>
https://chromiumcodereview.appspot.com/10875027/diff/8001/chrome/browser/sess...
> chrome/browser/sessions/session_service.cc:1333: if
> (!command_pickle->ReadString(&iter, &extension_id)) {
> On 2012/08/23 16:23:52, sky wrote:
>>
>> no {} here and 1344
>
>
> Done.
>
>
>
https://chromiumcodereview.appspot.com/10875027/diff/8001/chrome/browser/sess...
> File chrome/browser/sessions/session_service.h (right):
>
>
https://chromiumcodereview.appspot.com/10875027/diff/8001/chrome/browser/sess...
> chrome/browser/sessions/session_service.h:35: class ExtensionHost;
> On 2012/08/23 16:23:52, sky wrote:
>>
>> don't indent.
>
>
> Done.
>
>
>
https://chromiumcodereview.appspot.com/10875027/diff/8001/chrome/browser/sess...
> chrome/browser/sessions/session_service.h:185: std::set<std::string>*)>
> On 2012/08/23 16:23:52, sky wrote:
>>
>> Can set the set be by reference?
>
>
> Done.
>
>
>
https://chromiumcodereview.appspot.com/10875027/diff/8001/chrome/browser/sess...
> chrome/browser/sessions/session_service.h:275: // Converts the commands
> into SessionWindows. On return any valid
> On 2012/08/23 16:23:52, sky wrote:
>>
>> Update description.
>
>
> Done.
>
>
>
https://chromiumcodereview.appspot.com/10875027/diff/8001/chrome/browser/sess...
> chrome/browser/sessions/session_service.h:495:
> std::set<extensions::ExtensionHost*> running_apps_;
> On 2012/08/23 16:23:52, sky wrote:
>>
>> I think this should be ordered. Otherwise when you restart the apps
>
> won't start
>>
>> in the order they were launched.
>
>
> Due to the asynchronous nature in which apps run I don't think it
> matters which order they are started in. That is, we restart the apps by
> dispatching an onRestarted() event to them, which will be delivered
> asynchronously.
>
> https://chromiumcodereview.appspot.com/10875027/

Does this mean the order of the items in the windows taskbar and
chromeos launcher is random? That sounds bad.

  -Scott

[koz (OOO until 15th September), 2012-08-27 08:43:41]

On 2012/08/24 17:16:58, sky wrote:
> On Thu, Aug 23, 2012 at 7:08 PM,  <mailto:koz@chromium.org> wrote:
> > On 2012/08/23 16:23:52, sky wrote:
> >>
> >> I'm not sure this is going to work correctly. In particular when you close
> >> the
> >> last browser with apps open then create a new browser we trigger restore.
> >> In
> >> such a situation we wouldn't want to restore the apps. It may be that we
> >> need
> >
> > to
> >>
> >> persist the set of open apps else where (perhaps similar to how we track
> >
> > pinned
> >>
> >> tabs).
> >
> >
> > I wasn't aware of this behaviour. Could you clarify how one would trigger
> > that?
> > I tried closing the last browser window with an app running and created a
> > new
> > browser window by running the chrome binary again, but it didn't seem to
> > trigger
> > a restore.
> >
> > https://chromiumcodereview.appspot.com/10875027/
> 
> Maybe its only appsv1 that triggers this behavior, although I'm
> surprised appsv2 doesn't trigger it. Here's a way you can see it:
> 
> . turn on session restore.
> . open a couple of tabs in a non-incognito window.
> . create an incognito window.
> . close the non-incognito window.
> . hit control-n
> 
>   -Scott

Ah, I see it now. I didn't realise session restore was a setting. Thanks for the
instructions.

Yep, I'll try splitting out this logic into a separate AppRestoreService.

[koz (OOO until 15th September), 2012-08-27 08:46:10]

On 2012/08/24 17:17:24, sky wrote:
> On Thu, Aug 23, 2012 at 7:08 PM,  <mailto:koz@chromium.org> wrote:
> >
> >
>
https://chromiumcodereview.appspot.com/10875027/diff/8001/chrome/browser/sess...
> > File chrome/browser/sessions/session_restore.cc (right):
> >
> >
>
https://chromiumcodereview.appspot.com/10875027/diff/8001/chrome/browser/sess...
> > chrome/browser/sessions/session_restore.cc:1056: std::set<std::string>
> > apps_;
> > On 2012/08/23 16:23:52, sky wrote:
> >>
> >> newline between 1056/1057 and document what apps_ is.
> >
> >
> > Done.
> >
> >
> >
>
https://chromiumcodereview.appspot.com/10875027/diff/8001/chrome/browser/sess...
> > File chrome/browser/sessions/session_service.cc (right):
> >
> >
>
https://chromiumcodereview.appspot.com/10875027/diff/8001/chrome/browser/sess...
> > chrome/browser/sessions/session_service.cc:1333: if
> > (!command_pickle->ReadString(&iter, &extension_id)) {
> > On 2012/08/23 16:23:52, sky wrote:
> >>
> >> no {} here and 1344
> >
> >
> > Done.
> >
> >
> >
>
https://chromiumcodereview.appspot.com/10875027/diff/8001/chrome/browser/sess...
> > File chrome/browser/sessions/session_service.h (right):
> >
> >
>
https://chromiumcodereview.appspot.com/10875027/diff/8001/chrome/browser/sess...
> > chrome/browser/sessions/session_service.h:35: class ExtensionHost;
> > On 2012/08/23 16:23:52, sky wrote:
> >>
> >> don't indent.
> >
> >
> > Done.
> >
> >
> >
>
https://chromiumcodereview.appspot.com/10875027/diff/8001/chrome/browser/sess...
> > chrome/browser/sessions/session_service.h:185: std::set<std::string>*)>
> > On 2012/08/23 16:23:52, sky wrote:
> >>
> >> Can set the set be by reference?
> >
> >
> > Done.
> >
> >
> >
>
https://chromiumcodereview.appspot.com/10875027/diff/8001/chrome/browser/sess...
> > chrome/browser/sessions/session_service.h:275: // Converts the commands
> > into SessionWindows. On return any valid
> > On 2012/08/23 16:23:52, sky wrote:
> >>
> >> Update description.
> >
> >
> > Done.
> >
> >
> >
>
https://chromiumcodereview.appspot.com/10875027/diff/8001/chrome/browser/sess...
> > chrome/browser/sessions/session_service.h:495:
> > std::set<extensions::ExtensionHost*> running_apps_;
> > On 2012/08/23 16:23:52, sky wrote:
> >>
> >> I think this should be ordered. Otherwise when you restart the apps
> >
> > won't start
> >>
> >> in the order they were launched.
> >
> >
> > Due to the asynchronous nature in which apps run I don't think it
> > matters which order they are started in. That is, we restart the apps by
> > dispatching an onRestarted() event to them, which will be delivered
> > asynchronously.
> >
> > https://chromiumcodereview.appspot.com/10875027/
> 
> Does this mean the order of the items in the windows taskbar and
> chromeos launcher is random? That sounds bad.
> 
>   -Scott

Yeah, that does. I can make the new restore service remember the order and
hopefully it won't be too slow to open the apps synchronously.

[koz (OOO until 15th September), 2012-08-28 05:47:29]

I've made a new class AppRestoreService which maintains the list of running apps
separately to SessionService, and I've added a TODO to make the apps launch
order be stable.

[sky, 2012-08-28 16:56:12]

https://chromiumcodereview.appspot.com/10875027/diff/13001/chrome/browser/ses...
File chrome/browser/sessions/app_restore_service.h (right):

https://chromiumcodereview.appspot.com/10875027/diff/13001/chrome/browser/ses...
chrome/browser/sessions/app_restore_service.h:24: class AppRestoreService :
public BaseSessionService,
The list of apps is going to be smallish, I think stashing them in prefs will
make everything a lot simpler.

[Mihai Parparita -not on Chrome, 2012-08-28 17:47:11]

On Tue, Aug 28, 2012 at 9:56 AM, <sky@chromium.org> wrote:

> The list of apps is going to be smallish, I think stashing them in prefs
> will make everything a lot simpler.
>

Another option is to use extensions::StateStore to store a per-app
isRunning key.

Mihai

[koz (OOO until 15th September), 2012-08-29 09:27:14]

I opted to store the list in preferences, just so we don't need to iterate over
all the extensions to find out which ones should be restarted.

[koz (OOO until 15th September), 2012-09-02 23:34:46]

On 2012/08/29 09:27:14, koz wrote:
> I opted to store the list in preferences, just so we don't need to iterate
over
> all the extensions to find out which ones should be restarted.

Friendly ping.

[Mihai Parparita -not on Chrome, 2012-09-04 04:14:48]

https://chromiumcodereview.appspot.com/10875027/diff/18018/chrome/browser/ext...
File chrome/browser/extensions/api/app_runtime/app_runtime_api.cc (right):

https://chromiumcodereview.appspot.com/10875027/diff/18018/chrome/browser/ext...
chrome/browser/extensions/api/app_runtime/app_runtime_api.cc:36: void
AppEventRouter::DispatchOnRestartEvent(
Nit: should be called DispatchOnRestartedEvent.

https://chromiumcodereview.appspot.com/10875027/diff/18018/chrome/browser/ses...
File chrome/browser/sessions/app_restore_service.h (right):

https://chromiumcodereview.appspot.com/10875027/diff/18018/chrome/browser/ses...
chrome/browser/sessions/app_restore_service.h:23: // Tracks what apps need to be
restarted when the browser restarts.
Does it make sense for this to be in chrome/browser/sessions? It doesn't seem to
be using any session-related code. chrome/browser/extensions may be more
appropriate.

https://chromiumcodereview.appspot.com/10875027/diff/18018/chrome/browser/ses...
chrome/browser/sessions/app_restore_service.h:35: void
RestartApps(std::list<std::string>& apps);
This appears to be unused/undefined.

https://chromiumcodereview.appspot.com/10875027/diff/18018/chrome/browser/ses...
chrome/browser/sessions/app_restore_service.h:38: virtual void Observe(int type,
Needs OVERRIDE (also, it could be private).

https://chromiumcodereview.appspot.com/10875027/diff/18018/chrome/browser/ses...
File chrome/browser/sessions/base_session_service.h (right):

https://chromiumcodereview.appspot.com/10875027/diff/18018/chrome/browser/ses...
chrome/browser/sessions/base_session_service.h:41: APP_RESTORE,
I don't think you need this enum value (or the changes for it in
session_backend.cc) now that AppRestoreService is a separate standalone class.

https://chromiumcodereview.appspot.com/10875027/diff/18018/chrome/browser/ui/...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://chromiumcodereview.appspot.com/10875027/diff/18018/chrome/browser/ui/...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:603: LOG(ERROR) <<
"here";
Remove these.

[koz (OOO until 15th September), 2012-09-04 07:10:20]

https://chromiumcodereview.appspot.com/10875027/diff/18018/chrome/browser/ext...
File chrome/browser/extensions/api/app_runtime/app_runtime_api.cc (right):

https://chromiumcodereview.appspot.com/10875027/diff/18018/chrome/browser/ext...
chrome/browser/extensions/api/app_runtime/app_runtime_api.cc:36: void
AppEventRouter::DispatchOnRestartEvent(
On 2012/09/04 04:14:48, Mihai Parparita wrote:
> Nit: should be called DispatchOnRestartedEvent.

Done.

https://chromiumcodereview.appspot.com/10875027/diff/18018/chrome/browser/ses...
File chrome/browser/sessions/app_restore_service.h (right):

https://chromiumcodereview.appspot.com/10875027/diff/18018/chrome/browser/ses...
chrome/browser/sessions/app_restore_service.h:23: // Tracks what apps need to be
restarted when the browser restarts.
On 2012/09/04 04:14:48, Mihai Parparita wrote:
> Does it make sense for this to be in chrome/browser/sessions? It doesn't seem
to
> be using any session-related code. chrome/browser/extensions may be more
> appropriate.

Done.

https://chromiumcodereview.appspot.com/10875027/diff/18018/chrome/browser/ses...
chrome/browser/sessions/app_restore_service.h:35: void
RestartApps(std::list<std::string>& apps);
On 2012/09/04 04:14:48, Mihai Parparita wrote:
> This appears to be unused/undefined.

Done.

https://chromiumcodereview.appspot.com/10875027/diff/18018/chrome/browser/ses...
chrome/browser/sessions/app_restore_service.h:38: virtual void Observe(int type,
On 2012/09/04 04:14:48, Mihai Parparita wrote:
> Needs OVERRIDE (also, it could be private).

Done.

https://chromiumcodereview.appspot.com/10875027/diff/18018/chrome/browser/ses...
File chrome/browser/sessions/base_session_service.h (right):

https://chromiumcodereview.appspot.com/10875027/diff/18018/chrome/browser/ses...
chrome/browser/sessions/base_session_service.h:41: APP_RESTORE,
On 2012/09/04 04:14:48, Mihai Parparita wrote:
> I don't think you need this enum value (or the changes for it in
> session_backend.cc) now that AppRestoreService is a separate standalone class.

Done.

https://chromiumcodereview.appspot.com/10875027/diff/18018/chrome/browser/ui/...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://chromiumcodereview.appspot.com/10875027/diff/18018/chrome/browser/ui/...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:603: LOG(ERROR) <<
"here";
On 2012/09/04 04:14:48, Mihai Parparita wrote:
> Remove these.

Done.

[sky, 2012-09-04 15:37:42]

LGTM

[Mihai Parparita -not on Chrome, 2012-09-04 17:47:14]

https://chromiumcodereview.appspot.com/10875027/diff/23017/chrome/browser/ext...
File chrome/browser/extensions/app_restore_service.cc (right):

https://chromiumcodereview.appspot.com/10875027/diff/23017/chrome/browser/ext...
chrome/browser/extensions/app_restore_service.cc:36:
profile_->GetPrefs()->GetUserPrefValue(prefs::kAppsRunningAtShutdown);
Is there a reason why this pref isn't stored via extensions::ExtensionPrefs? It
might make the most sense as a per-extension "was running" pref, so that it can
be annotated with additional data in the future (e.g. the list of open windows).

https://chromiumcodereview.appspot.com/10875027/diff/23017/chrome/browser/ext...
File chrome/browser/extensions/app_restore_service.h (right):

https://chromiumcodereview.appspot.com/10875027/diff/23017/chrome/browser/ext...
chrome/browser/extensions/app_restore_service.h:19: namespace extensions {
The whole class should be in the extensions namespace.

https://chromiumcodereview.appspot.com/10875027/diff/23017/chrome/common/exte...
File chrome/common/extensions/api/app_runtime.idl (right):

https://chromiumcodereview.appspot.com/10875027/diff/23017/chrome/common/exte...
chrome/common/extensions/api/app_runtime.idl:33: static void onLaunched(optional
LaunchData launchData);
Mind adding docstrings for both of these events?

[koz (OOO until 15th September), 2012-09-05 06:21:33]

Cheers, Mihai.

https://chromiumcodereview.appspot.com/10875027/diff/23017/chrome/browser/ext...
File chrome/browser/extensions/app_restore_service.cc (right):

https://chromiumcodereview.appspot.com/10875027/diff/23017/chrome/browser/ext...
chrome/browser/extensions/app_restore_service.cc:36:
profile_->GetPrefs()->GetUserPrefValue(prefs::kAppsRunningAtShutdown);
On 2012/09/04 17:47:14, Mihai Parparita wrote:
> Is there a reason why this pref isn't stored via extensions::ExtensionPrefs?
It
> might make the most sense as a per-extension "was running" pref, so that it
can
> be annotated with additional data in the future (e.g. the list of open
windows).

Done.

https://chromiumcodereview.appspot.com/10875027/diff/23017/chrome/browser/ext...
File chrome/browser/extensions/app_restore_service.h (right):

https://chromiumcodereview.appspot.com/10875027/diff/23017/chrome/browser/ext...
chrome/browser/extensions/app_restore_service.h:19: namespace extensions {
On 2012/09/04 17:47:14, Mihai Parparita wrote:
> The whole class should be in the extensions namespace.

Done.

https://chromiumcodereview.appspot.com/10875027/diff/23017/chrome/common/exte...
File chrome/common/extensions/api/app_runtime.idl (right):

https://chromiumcodereview.appspot.com/10875027/diff/23017/chrome/common/exte...
chrome/common/extensions/api/app_runtime.idl:33: static void onLaunched(optional
LaunchData launchData);
On 2012/09/04 17:47:14, Mihai Parparita wrote:
> Mind adding docstrings for both of these events?

Done.

[Mihai Parparita -not on Chrome, 2012-09-05 20:54:06]

It should be possible to test these, now that we have the PRE_ mechanism for
doing browser tests across Chrome restarts (see http://crrev.com/150096).

https://chromiumcodereview.appspot.com/10875027/diff/21002/chrome/browser/ext...
File chrome/browser/extensions/app_restore_service.cc (right):

https://chromiumcodereview.appspot.com/10875027/diff/21002/chrome/browser/ext...
chrome/browser/extensions/app_restore_service.cc:38:
ExtensionPrefs::ExtensionIds extension_ids(
You can also get the list of apps from the extension service
(ExtensionPrefs::GetExtensionsFrom is meant to be used before ExtensionService
is initialize, which isn't the case here).

https://chromiumcodereview.appspot.com/10875027/diff/21002/chrome/browser/ext...
chrome/browser/extensions/app_restore_service.cc:61: extensions::ExtensionHost*
host =
Now that you're in the extensions namespace you can drop the prefix here (and a
few other places in the file).

https://chromiumcodereview.appspot.com/10875027/diff/21002/chrome/browser/ext...
File chrome/browser/extensions/app_restore_service.h (right):

https://chromiumcodereview.appspot.com/10875027/diff/21002/chrome/browser/ext...
chrome/browser/extensions/app_restore_service.h:12: #include <list>
list and vector are unused.

https://chromiumcodereview.appspot.com/10875027/diff/21002/chrome/browser/ext...
chrome/browser/extensions/app_restore_service.h:16: class PrefService;
Ditto for the PrefService and ExtensionHost forward declarations.

https://chromiumcodereview.appspot.com/10875027/diff/21002/chrome/browser/pre...
File chrome/browser/prefs/browser_prefs.cc (right):

https://chromiumcodereview.appspot.com/10875027/diff/21002/chrome/browser/pre...
chrome/browser/prefs/browser_prefs.cc:20: #include
"chrome/browser/extensions/app_restore_service.h"
This change can be reverted.

https://chromiumcodereview.appspot.com/10875027/diff/21002/chrome/common/pref...
File chrome/common/pref_names.cc (left):

https://chromiumcodereview.appspot.com/10875027/diff/21002/chrome/common/pref...
chrome/common/pref_names.cc:497: // An integer pref to initially mute volume if
1. This pref is ignored if
Ditto.

https://chromiumcodereview.appspot.com/10875027/diff/21002/chrome/common/pref...
File chrome/common/pref_names.h (left):

https://chromiumcodereview.appspot.com/10875027/diff/21002/chrome/common/pref...
chrome/common/pref_names.h:270: extern const char kSecondaryDisplays[];
I don't think you mean to make these changes.

[koz (OOO until 15th September), 2012-09-06 07:50:43]

I would have uploaded a better test than the one here, but when I simulate a
restart in the test (by using PRE_ and changing the return value of
StartupBrowserCreator::WasRestarted()) the extension appears to crash with an
error message from the bowels of WebKit. I think it may not be worth the effort
tracking that down.

https://chromiumcodereview.appspot.com/10875027/diff/21002/chrome/browser/ext...
File chrome/browser/extensions/app_restore_service.cc (right):

https://chromiumcodereview.appspot.com/10875027/diff/21002/chrome/browser/ext...
chrome/browser/extensions/app_restore_service.cc:38:
ExtensionPrefs::ExtensionIds extension_ids(
On 2012/09/05 20:54:06, Mihai Parparita wrote:
> You can also get the list of apps from the extension service
> (ExtensionPrefs::GetExtensionsFrom is meant to be used before ExtensionService
> is initialize, which isn't the case here).

Done.

https://chromiumcodereview.appspot.com/10875027/diff/21002/chrome/browser/ext...
chrome/browser/extensions/app_restore_service.cc:61: extensions::ExtensionHost*
host =
On 2012/09/05 20:54:06, Mihai Parparita wrote:
> Now that you're in the extensions namespace you can drop the prefix here (and
a
> few other places in the file).

Done.

https://chromiumcodereview.appspot.com/10875027/diff/21002/chrome/browser/ext...
File chrome/browser/extensions/app_restore_service.h (right):

https://chromiumcodereview.appspot.com/10875027/diff/21002/chrome/browser/ext...
chrome/browser/extensions/app_restore_service.h:12: #include <list>
On 2012/09/05 20:54:06, Mihai Parparita wrote:
> list and vector are unused.

Done.

https://chromiumcodereview.appspot.com/10875027/diff/21002/chrome/browser/ext...
chrome/browser/extensions/app_restore_service.h:16: class PrefService;
On 2012/09/05 20:54:06, Mihai Parparita wrote:
> Ditto for the PrefService and ExtensionHost forward declarations.

Done.

https://chromiumcodereview.appspot.com/10875027/diff/21002/chrome/browser/pre...
File chrome/browser/prefs/browser_prefs.cc (right):

https://chromiumcodereview.appspot.com/10875027/diff/21002/chrome/browser/pre...
chrome/browser/prefs/browser_prefs.cc:20: #include
"chrome/browser/extensions/app_restore_service.h"
On 2012/09/05 20:54:06, Mihai Parparita wrote:
> This change can be reverted.

Done.

https://chromiumcodereview.appspot.com/10875027/diff/21002/chrome/common/pref...
File chrome/common/pref_names.cc (left):

https://chromiumcodereview.appspot.com/10875027/diff/21002/chrome/common/pref...
chrome/common/pref_names.cc:497: // An integer pref to initially mute volume if
1. This pref is ignored if
On 2012/09/05 20:54:06, Mihai Parparita wrote:
> Ditto.

Done.

https://chromiumcodereview.appspot.com/10875027/diff/21002/chrome/common/pref...
File chrome/common/pref_names.h (left):

https://chromiumcodereview.appspot.com/10875027/diff/21002/chrome/common/pref...
chrome/common/pref_names.h:270: extern const char kSecondaryDisplays[];
On 2012/09/05 20:54:06, Mihai Parparita wrote:
> I don't think you mean to make these changes.

Done.

[Mihai Parparita -not on Chrome, 2012-09-06 23:07:09]

LGTM

What kind of crash were you getting in the test?

https://chromiumcodereview.appspot.com/10875027/diff/32002/chrome/browser/ext...
File chrome/browser/extensions/app_restore_service.cc (right):

https://chromiumcodereview.appspot.com/10875027/diff/32002/chrome/browser/ext...
chrome/browser/extensions/app_restore_service.cc:47:
RestoreApp(extension->id());
Given that RestoreApp then looks up the Extension instance by ID, you might as
well just pass it in.

https://chromiumcodereview.appspot.com/10875027/diff/32002/chrome/browser/ext...
chrome/browser/extensions/app_restore_service.cc:49:
extension_prefs->SetExtensionRunning(extension->id(), false);
Not sure I understand the need for this. It seems like the extension is already
marked as not running.

https://chromiumcodereview.appspot.com/10875027/diff/32002/chrome/browser/ext...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10875027/diff/32002/chrome/browser/ext...
chrome/browser/extensions/platform_app_browsertest.cc:586:
ASSERT_TRUE(extension_prefs->IsExtensionRunning(extension->id()));
To make this test more comprehensive, you could then shut down the app, manually
set the isRunning pref to true, call RestoreApps, and check that an onRestarted
event gets dispatched to it.

[koz (OOO until 15th September), 2012-09-10 04:51:57]

The exception looked like this:

ASSERTION FAILED: m_key != PTHREAD_KEYS_MAX
../../third_party/WebKit/Source/WTF/wtf/ThreadIdentifierDataPthreads.cpp(65) :
static WTF::ThreadIdentifier WTF::ThreadIdentifierData::identifier()
1   0x4ba6c5f
2   0x4b7ebd7
3   0x1ba87ff
...

https://chromiumcodereview.appspot.com/10875027/diff/32002/chrome/browser/ext...
File chrome/browser/extensions/app_restore_service.cc (right):

https://chromiumcodereview.appspot.com/10875027/diff/32002/chrome/browser/ext...
chrome/browser/extensions/app_restore_service.cc:47:
RestoreApp(extension->id());
On 2012/09/06 23:07:09, Mihai Parparita wrote:
> Given that RestoreApp then looks up the Extension instance by ID, you might as
> well just pass it in.

Done.

https://chromiumcodereview.appspot.com/10875027/diff/32002/chrome/browser/ext...
chrome/browser/extensions/app_restore_service.cc:49:
extension_prefs->SetExtensionRunning(extension->id(), false);
On 2012/09/06 23:07:09, Mihai Parparita wrote:
> Not sure I understand the need for this. It seems like the extension is
already
> marked as not running.

Oh, yeah. Silly mistake.

https://chromiumcodereview.appspot.com/10875027/diff/32002/chrome/browser/ext...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10875027/diff/32002/chrome/browser/ext...
chrome/browser/extensions/platform_app_browsertest.cc:586:
ASSERT_TRUE(extension_prefs->IsExtensionRunning(extension->id()));
On 2012/09/06 23:07:09, Mihai Parparita wrote:
> To make this test more comprehensive, you could then shut down the app,
manually
> set the isRunning pref to true, call RestoreApps, and check that an
onRestarted
> event gets dispatched to it.

Done.

[commit-bot: I haz the power, 2012-09-10 04:53:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/10875027/34011

[commit-bot: I haz the power, 2012-09-10 05:15:28]

Try job failure for 10875027-34011 (retry) on mac for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

[commit-bot: I haz the power, 2012-09-10 06:22:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/10875027/32022

[commit-bot: I haz the power, 2012-09-10 08:17:50]

Try job failure for 10875027-32022 (retry) on linux_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-09-13 20:44:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/10875027/30002

[commit-bot: I haz the power, 2012-09-13 22:38:28]

Change committed as 156659

[koz (OOO until 15th September), 2012-09-19 04:10:38]

Scott,

As discussed I've moved the call to RestoreApps() further down in
ChromeBrowserMain, after PostBrowserStart(). I couldn't find any initialisation
code for apps that starts at browser startup where this code would more suitably
go.

[commit-bot: I haz the power, 2012-10-15 03:34:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/10875027/44001

[commit-bot: I haz the power, 2012-10-15 05:47:56]

Change committed as 161827