Let ActivityManager delete the remaining activities on shutdown.

Let ActivityManager delete the remaining activities on shutdown.

BUG=None
R=oshima@chromium.org
TEST=None

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

[oshima, 2014-06-04 05:05:12]

+sadrul,

I think the design is that an activity can delete itself, and what's missing was
to delete activities in dtor?

[oshima, 2014-06-04 05:05:52]

oops, +sadrul again

[Jun Mukai, 2014-06-04 05:09:15]

On 2014/06/04 05:05:12, oshima wrote:
> +sadrul,
> 
> I think the design is that an activity can delete itself, and what's missing
was
> to delete activities in dtor?

My point is, no one deletes the activities. Right now, a WebActivity is created
and then just ignored.

If each activity should remove itself from the manager in dtor, then it should
happen in Activity class, not WebActivity class. And anyways ActivityManager
should remove its activities at some point (I think it's in Shutdown())

[sadrul, 2014-06-04 05:17:40]

On 2014/06/04 05:09:15, Jun Mukai wrote:
> On 2014/06/04 05:05:12, oshima wrote:
> > +sadrul,
> > 
> > I think the design is that an activity can delete itself, and what's missing
> was
> > to delete activities in dtor?
> 
> My point is, no one deletes the activities. Right now, a WebActivity is
created
> and then just ignored.
> 
> If each activity should remove itself from the manager in dtor, then it should
> happen in Activity class, not WebActivity class. And anyways ActivityManager
> should remove its activities at some point (I think it's in Shutdown())

We do not currently have a way to actually close/remove an activity. This is by
design.

I expect that the lifetime of the activity will be managed by something other
than the ActivityManager. It could be the activity itself (e.g. window.close()
from JS), or it could be some UI element (e.g. UX decides to add a close button,
or going 'back'), or there could be an ActivityLifetimeManager (which may or may
not be related to an ActivityResourceManager) etc. It will possibly be a
combination of these.

I do not necessarily see anything wrong with this patch, but it may be a good
idea to postpone this change until we have a better understanding of how/when we
close an activity.

https://codereview.chromium.org/310243003/diff/1/athena/activity/activity_man...
File athena/activity/activity_manager_impl.cc (right):

https://codereview.chromium.org/310243003/diff/1/athena/activity/activity_man...
athena/activity/activity_manager_impl.cc:51: manager->AddActivity(activity);
Oops. Looks like I made a copy-pasta error here. Mind fixing?

https://codereview.chromium.org/310243003/diff/1/athena/activity/activity_man...
athena/activity/activity_manager_impl.cc:62: std::vector<Activity*> activities_;
Use ScopedVector<> instead?

[oshima, 2014-06-04 16:56:05]

How about something like this?

* ActivityManager owns Activity. What this means is that it simply keep track of
objects and make sure it wont leak, and provide
  a way to list all activities.

* Activity adds/remove itself to/from ActivityManager. Pros: provides standard
way to manage activity instance.
  This way, you can just delete activity and it will be gone from the activity
manager.
  ** we may want to do this outside of ctor/dtor as doing so have several cons,
but it's ok for now.

* If there are live activities when deleting ActivityMananger, activity manager
delete them all.

* The lifetime of activity (when to close it) will be determined by external
entity. It can be closed by user via some UI
 (close button/shotcut), or a policy object ActivityLifetimeManager (a.k.a
strategy pattern).
  It simply decide when to die (or create), but it does not manage objects.


On 2014/06/04 05:17:40, sadrul wrote:
> On 2014/06/04 05:09:15, Jun Mukai wrote:
> > On 2014/06/04 05:05:12, oshima wrote:
> > > +sadrul,
> > > 
> > > I think the design is that an activity can delete itself, and what's
missing
> > was
> > > to delete activities in dtor?
> > 
> > My point is, no one deletes the activities. Right now, a WebActivity is
> created
> > and then just ignored.
> > 
> > If each activity should remove itself from the manager in dtor, then it
should
> > happen in Activity class, not WebActivity class. And anyways ActivityManager
> > should remove its activities at some point (I think it's in Shutdown())
> 
> We do not currently have a way to actually close/remove an activity. This is
by
> design.
> 
> I expect that the lifetime of the activity will be managed by something other
> than the ActivityManager. It could be the activity itself (e.g. window.close()
> from JS), or it could be some UI element (e.g. UX decides to add a close
button,
> or going 'back'), or there could be an ActivityLifetimeManager (which may or
may
> not be related to an ActivityResourceManager) etc. It will possibly be a
> combination of these.
> 
> I do not necessarily see anything wrong with this patch, but it may be a good
> idea to postpone this change until we have a better understanding of how/when
we
> close an activity.
> 
>
https://codereview.chromium.org/310243003/diff/1/athena/activity/activity_man...
> File athena/activity/activity_manager_impl.cc (right):
> 
>
https://codereview.chromium.org/310243003/diff/1/athena/activity/activity_man...
> athena/activity/activity_manager_impl.cc:51: manager->AddActivity(activity);
> Oops. Looks like I made a copy-pasta error here. Mind fixing?
> 
>
https://codereview.chromium.org/310243003/diff/1/athena/activity/activity_man...
> athena/activity/activity_manager_impl.cc:62: std::vector<Activity*>
activities_;
> Use ScopedVector<> instead?

[Jun Mukai, 2014-06-04 20:03:29]

Changed the code based on oshima's suggestion.
- Activity invokes RemoveActivity on its destructor
- ActivityManager cleans up the remaining activity on Shutdown

PTAL

https://codereview.chromium.org/310243003/diff/1/athena/activity/activity_man...
File athena/activity/activity_manager_impl.cc (right):

https://codereview.chromium.org/310243003/diff/1/athena/activity/activity_man...
athena/activity/activity_manager_impl.cc:51: manager->AddActivity(activity);
On 2014/06/04 05:17:40, sadrul wrote:
> Oops. Looks like I made a copy-pasta error here. Mind fixing?

fixed by another CL :)

https://codereview.chromium.org/310243003/diff/1/athena/activity/activity_man...
athena/activity/activity_manager_impl.cc:62: std::vector<Activity*> activities_;
On 2014/06/04 05:17:40, sadrul wrote:
> Use ScopedVector<> instead?

Unfortunately, ScopedVector doesn't fit to the usage of the (new) design,
activities_.erase() will remove the Activity instance which calls
RemoveActivity().

[sadrul, 2014-06-04 20:34:04]

~ActivityManagerImpl() deletes the set of tasks, which triggers back to
ActivityManagerImpl::RemoveActivity() through Activity::~Activity(). This also
requires the ActivityViewManager to remain alive until the ActivityManager is
shut-down. We are doing a lot of work during shutdown. This all seems somewhat
unnecessary, and a little hairy.

https://codereview.chromium.org/310243003/diff/20001/athena/activity/activity...
File athena/activity/activity_manager_impl.cc (right):

https://codereview.chromium.org/310243003/diff/20001/athena/activity/activity...
athena/activity/activity_manager_impl.cc:30: delete activities_.front();
STLDeleteContainerPointers instead?

https://codereview.chromium.org/310243003/diff/20001/athena/activity/activity...
athena/activity/activity_manager_impl.cc:88: ActivityViewManager::Shutdown();
This seems unfortunate.

[Jun Mukai, 2014-06-04 21:11:14]

https://codereview.chromium.org/310243003/diff/20001/athena/activity/activity...
File athena/activity/activity_manager_impl.cc (right):

https://codereview.chromium.org/310243003/diff/20001/athena/activity/activity...
athena/activity/activity_manager_impl.cc:30: delete activities_.front();
On 2014/06/04 20:34:05, sadrul wrote:
> STLDeleteContainerPointers instead?

RemoveActivity changes the contents of |activities_| and
STLDeleteContainerPointers is going mad about that.

https://codereview.chromium.org/310243003/diff/20001/athena/activity/activity...
athena/activity/activity_manager_impl.cc:88: ActivityViewManager::Shutdown();
On 2014/06/04 20:34:05, sadrul wrote:
> This seems unfortunate.

Because ActivityManager depends on ActivityViewManager, I think this order is
more natural.
Changed the order in Create() for the consistency.

[Jun Mukai, 2014-06-04 21:12:16]

changed the title a bit to reflect the actual contents of the CL.

[oshima, 2014-06-04 21:16:47]

On 2014/06/04 20:34:04, sadrul wrote:
> ~ActivityManagerImpl() deletes the set of tasks, which triggers back to
> ActivityManagerImpl::RemoveActivity() through Activity::~Activity(). 

I think we should have explicit shutdown process rather than deleting them in
dtor,
(same in Activity, where activity in the ActivityManager may be already in
dtor),
but I think we can address them a bit later.


On 2014/06/04 21:11:14, Jun Mukai wrote:
>
https://codereview.chromium.org/310243003/diff/20001/athena/activity/activity...
> athena/activity/activity_manager_impl.cc:88: ActivityViewManager::Shutdown();
> On 2014/06/04 20:34:05, sadrul wrote:
> > This seems unfortunate.
> 
> Because ActivityManager depends on ActivityViewManager, I think this order is
> more natural.

I agree with mukai (or the current state of the code, although we may want to
reverse the dependency at some point.
view->model is more natual than model->view IMO.

[oshima, 2014-06-04 21:18:50]

lgtm

I'm sure we need to change this again, but let's move on.

[Jun Mukai, 2014-06-05 21:52:59]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2014-06-05 21:55:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mukai@chromium.org/310243003/60001

[commit-bot: I haz the power, 2014-06-06 05:19:15]

Change committed as 275337