athena: Add the concept of Activity and related managers.

athena: Add the concept of Activity and related managers.

An Activity can be a WebActivity, a SampleActivity etc. Each Activity has an
ActivityViewModel. The ActivityViewModel describes the UI for the Activity.

The ActivityManager is there to manage the list of activities. It is the source
of all the activities that various other activity-managers would use.

The ActivityViewManager is notified of activity updates from the ActivityManager,
and it creates the appropriate UI elements (views::Widget, etc.) to show the UI
for the activity to the user.

BUG=376357
R=oshima@chromium.org, skuhne@chromium.org
TBR=ben@chromium.org

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

[chromium-reviews, 2014-05-27 17:10:18]

As this affects navigation and resource management I'd like to have skuhne@'s
input on this.

I'm in meeting hell today. I'll review as soon as I can.
On Mon May 26 2014 at 4:00:33 PM, <sadrul@chromium.org> wrote:

> Reviewers: Ben Goodger (Google), oshima, Albert Bodenhamer,
>
> Description:
> athena: Add the concept of Task and TaskManager.
>
> A Task can be a WebTask, a SampleTask etc. Each Task has a native window
> (i.e.  aura window), and provides necessary information about how it should
> be presented to the user. The TaskManager is responsible for using that
> information to create the appropriate UI elements and adding it to the
> window manager.
>
> BUG=376357
>
> Please review this at https://codereview.chromium.org/302683002/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files (+412, -4 lines):
>    M athena/athena.gyp
>    M athena/main/DEPS
>    M athena/main/athena_main.cc
>    M athena/main/athena_main.gyp
>    M athena/main/placeholder.h
>    M athena/main/placeholder.cc
>    A athena/main/web_task.h
>    A athena/main/web_task.cc
>    A + athena/task/DEPS
>    A + athena/task/public/DEPS
>    A athena/task/public/task.h
>    A athena/task/public/task_manager.h
>    A athena/task/task_manager_impl.cc
>    M athena/ui/DEPS
>    M athena/ui/athena_ui_main.cc
>    M athena/ui/athena_ui_main.gyp
>    A athena/ui/sample_task.h
>    A athena/ui/sample_task.cc
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sadrul, 2014-05-27 19:04:17]

+skuhne@

[Mr4D (OOO till 08-26), 2014-05-27 20:12:41]

A few comments about the naming for clarification.

https://codereview.chromium.org/302683002/diff/110001/athena/task/public/task.h
File athena/task/public/task.h (right):

https://codereview.chromium.org/302683002/diff/110001/athena/task/public/task...
athena/task/public/task.h:19: 
Could you please add a comment towards what a Task actually is?

https://codereview.chromium.org/302683002/diff/110001/athena/task/public/task...
athena/task/public/task.h:31: virtual aura::Window* GetNativeWindow() = 0;
I might be blended by the name "Task" and "TaskManager" here. Is this supposed
to be an high level accessor for a V1/V2/NaCl app / browser object?

If so - we should add here some calls to unify their resource handling - like a
query mechanism about how much of what is used and / or setting the state of
such a task. (I can elaborate more on this if this is what it is).

If I am wrong and this is meant to be an "UI representation"
of a user's task - like: send an email - (and it has nothing to do with an OS
task) could we use a term like "Activity" instead of "Task"?

https://codereview.chromium.org/302683002/diff/110001/athena/task/public/task...
File athena/task/public/task_manager.h (right):

https://codereview.chromium.org/302683002/diff/110001/athena/task/public/task...
athena/task/public/task_manager.h:14: // Manages a set of tasks.
As stated above - could you please explain further what this exactly is?

[sadrul, 2014-05-27 20:30:45]

https://codereview.chromium.org/302683002/diff/110001/athena/task/public/task.h
File athena/task/public/task.h (right):

https://codereview.chromium.org/302683002/diff/110001/athena/task/public/task...
athena/task/public/task.h:31: virtual aura::Window* GetNativeWindow() = 0;
On 2014/05/27 20:12:42, Mr4D wrote:
> I might be blended by the name "Task" and "TaskManager" here. Is this supposed
> to be an high level accessor for a V1/V2/NaCl app / browser object?

Something like that. This is essentially supposed to be the UI representation of
a V1/V2/NaCl app / browser object. Perhaps 'TaskUI' explains it better. See the
implementation of WebTask, for example, which provides the UI for athena for a
content::WebContents.

> 
> If so - we should add here some calls to unify their resource handling - like
a
> query mechanism about how much of what is used and / or setting the state of
> such a task. (I can elaborate more on this if this is what it is).
> 
> If I am wrong and this is meant to be an "UI representation"
> of a user's task - like: send an email - (and it has nothing to do with an OS
> task) could we use a term like "Activity" instead of "Task"?

[chromium-reviews, 2014-05-27 22:01:11]

It sounds like there's a lot to discuss here. Does it make sense to have
sadrul, oshima, and skuhne jump on VC and sort it out?

On Tue May 27 2014 at 1:30:47 PM, <sadrul@chromium.org> wrote:

>
> https://codereview.chromium.org/302683002/diff/110001/
> athena/task/public/task.h
> File athena/task/public/task.h (right):
>
> https://codereview.chromium.org/302683002/diff/110001/
> athena/task/public/task.h#newcode31
>
athena/task/public/task.h:31<https://codereview.chromium.org/302683002/diff/110001/athena/task/public/task.h#newcode31athena/task/public/task.h:31>:
> virtual aura::Window* GetNativeWindow() =
> 0;
> On 2014/05/27 20:12:42, Mr4D wrote:
> > I might be blended by the name "Task" and "TaskManager" here. Is this
> supposed
> > to be an high level accessor for a V1/V2/NaCl app / browser object?
>
> Something like that. This is essentially supposed to be the UI
> representation of a V1/V2/NaCl app / browser object. Perhaps 'TaskUI'
> explains it better. See the implementation of WebTask, for example,
> which provides the UI for athena for a content::WebContents.
>
>
> > If so - we should add here some calls to unify their resource handling
> - like a
> > query mechanism about how much of what is used and / or setting the
> state of
> > such a task. (I can elaborate more on this if this is what it is).
>
> > If I am wrong and this is meant to be an "UI representation"
> > of a user's task - like: send an email - (and it has nothing to do
> with an OS
> > task) could we use a term like "Activity" instead of "Task"?
>
> https://codereview.chromium.org/302683002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[oshima, 2014-05-27 22:13:45]

On 2014/05/27 22:01:11, chromium-reviews_chromium.org wrote:
> It sounds like there's a lot to discuss here. Does it make sense to have
> sadrul, oshima, and skuhne jump on VC and sort it out?
> 

We just did :), and sadrul will write up a simple doc for the design he has in
his mind.

> On Tue May 27 2014 at 1:30:47 PM, <mailto:sadrul@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.org/302683002/diff/110001/
> > athena/task/public/task.h
> > File athena/task/public/task.h (right):
> >
> > https://codereview.chromium.org/302683002/diff/110001/
> > athena/task/public/task.h#newcode31
> >
>
athena/task/public/task.h:31<https://codereview.chromium.org/302683002/diff/110001/athena/task/public/task.h#newcode31athena/task/public/task.h:31>:
> > virtual aura::Window* GetNativeWindow() =
> > 0;
> > On 2014/05/27 20:12:42, Mr4D wrote:
> > > I might be blended by the name "Task" and "TaskManager" here. Is this
> > supposed
> > > to be an high level accessor for a V1/V2/NaCl app / browser object?
> >
> > Something like that. This is essentially supposed to be the UI
> > representation of a V1/V2/NaCl app / browser object. Perhaps 'TaskUI'
> > explains it better. See the implementation of WebTask, for example,
> > which provides the UI for athena for a content::WebContents.
> >
> >
> > > If so - we should add here some calls to unify their resource handling
> > - like a
> > > query mechanism about how much of what is used and / or setting the
> > state of
> > > such a task. (I can elaborate more on this if this is what it is).
> >
> > > If I am wrong and this is meant to be an "UI representation"
> > > of a user's task - like: send an email - (and it has nothing to do
> > with an OS
> > > task) could we use a term like "Activity" instead of "Task"?
> >
> > https://codereview.chromium.org/302683002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2014-05-27 22:17:37]

Perfect! Thanks.

On Tue May 27 2014 at 3:13:47 PM, <oshima@chromium.org> wrote:

> On 2014/05/27 22:01:11, chromium-reviews_chromium.org wrote:
> > It sounds like there's a lot to discuss here. Does it make sense to have
> > sadrul, oshima, and skuhne jump on VC and sort it out?
>
>
> We just did :), and sadrul will write up a simple doc for the design he has
> in
> his mind.
>
> > On Tue May 27 2014 at 1:30:47 PM, <mailto:sadrul@chromium.org> wrote:
>
> > >
> > > https://codereview.chromium.org/302683002/diff/110001/
> > > athena/task/public/task.h
> > > File athena/task/public/task.h (right):
> > >
> > > https://codereview.chromium.org/302683002/diff/110001/
> > > athena/task/public/task.h#newcode31
> > >
>
> athena/task/public/task.h:31<https://codereview.chromium.
> org/302683002/diff/110001/athena/task/public/task.h#
> newcode31athena/task/public/task.h:31>:
> > > virtual aura::Window* GetNativeWindow() =
> > > 0;
> > > On 2014/05/27 20:12:42, Mr4D wrote:
> > > > I might be blended by the name "Task" and "TaskManager" here. Is this
> > > supposed
> > > > to be an high level accessor for a V1/V2/NaCl app / browser object?
> > >
> > > Something like that. This is essentially supposed to be the UI
> > > representation of a V1/V2/NaCl app / browser object. Perhaps 'TaskUI'
> > > explains it better. See the implementation of WebTask, for example,
> > > which provides the UI for athena for a content::WebContents.
> > >
> > >
> > > > If so - we should add here some calls to unify their resource
> handling
> > > - like a
> > > > query mechanism about how much of what is used and / or setting the
> > > state of
> > > > such a task. (I can elaborate more on this if this is what it is).
> > >
> > > > If I am wrong and this is meant to be an "UI representation"
> > > > of a user's task - like: send an email - (and it has nothing to do
> > > with an OS
> > > > task) could we use a term like "Activity" instead of "Task"?
> > >
> > > https://codereview.chromium.org/302683002/
> > >
>
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>
> https://codereview.chromium.org/302683002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sadrul, 2014-05-30 18:20:32]

Hi! I have updated the patch and the CL description based on the feedback on the
doc. This is now ready for formal review. PTAL.

[oshima, 2014-05-30 20:55:59]

https://codereview.chromium.org/302683002/diff/150001/athena/activity/activit...
File athena/activity/activity_manager_impl.cc (right):

https://codereview.chromium.org/302683002/diff/150001/athena/activity/activit...
athena/activity/activity_manager_impl.cc:55: static ActivityManagerImpl*
g_instance;
remove static and g_. (I'd like to use g_ only for global scoped variable while
this is file scoped)

https://codereview.chromium.org/302683002/diff/150001/athena/activity/activit...
athena/activity/activity_manager_impl.cc:62: g_instance = new
ActivityManagerImpl();
can you assign/reset in ctor/dtor like other manager classes?

https://codereview.chromium.org/302683002/diff/150001/athena/activity/activit...
File athena/activity/activity_view_manager_impl.cc (right):

https://codereview.chromium.org/302683002/diff/150001/athena/activity/activit...
athena/activity/activity_view_manager_impl.cc:34: 
Do you think using WebView is better option? It seems to be able to handle
things like fullscreen / accessibility etc.

[sadrul, 2014-05-30 23:45:36]

https://codereview.chromium.org/302683002/diff/150001/athena/activity/activit...
File athena/activity/activity_manager_impl.cc (right):

https://codereview.chromium.org/302683002/diff/150001/athena/activity/activit...
athena/activity/activity_manager_impl.cc:55: static ActivityManagerImpl*
g_instance;
On 2014/05/30 20:55:59, oshima wrote:
> remove static and g_. (I'd like to use g_ only for global scoped variable
while
> this is file scoped)

Done.

https://codereview.chromium.org/302683002/diff/150001/athena/activity/activit...
athena/activity/activity_manager_impl.cc:62: g_instance = new
ActivityManagerImpl();
On 2014/05/30 20:55:59, oshima wrote:
> can you assign/reset in ctor/dtor like other manager classes?

Done.

https://codereview.chromium.org/302683002/diff/150001/athena/activity/activit...
File athena/activity/activity_view_manager_impl.cc (right):

https://codereview.chromium.org/302683002/diff/150001/athena/activity/activit...
athena/activity/activity_view_manager_impl.cc:34: 
On 2014/05/30 20:55:59, oshima wrote:
> Do you think using WebView is better option? It seems to be able to handle
> things like fullscreen / accessibility etc.

It may make sense to use WebView for WebActivity, but I don't think it would
make sense for non-web activities. So I am using a NativeViewHost that makes it
possible for the activity implementation to decide what to use.

[oshima, 2014-05-31 00:58:00]

lgtm with nits

https://codereview.chromium.org/302683002/diff/150001/athena/activity/activit...
File athena/activity/activity_view_manager_impl.cc (right):

https://codereview.chromium.org/302683002/diff/150001/athena/activity/activit...
athena/activity/activity_view_manager_impl.cc:34: 
On 2014/05/30 23:45:36, sadrul wrote:
> On 2014/05/30 20:55:59, oshima wrote:
> > Do you think using WebView is better option? It seems to be able to handle
> > things like fullscreen / accessibility etc.
> 
> It may make sense to use WebView for WebActivity, but I don't think it would
> make sense for non-web activities. So I am using a NativeViewHost that makes
it
> possible for the activity implementation to decide what to use.

I understand the motivation and am ok for now. I believe WebView has its own
NativeViewHost so we probably need some refactoring later.

https://codereview.chromium.org/302683002/diff/160022/athena/activity/activit...
File athena/activity/activity_manager_impl.cc (right):

https://codereview.chromium.org/302683002/diff/160022/athena/activity/activit...
athena/activity/activity_manager_impl.cc:17: static ActivityManager* instance;
You can nuke static. It's deprecated in C++03, and it's back in C++11 but
anonymous namespace has internal linkage. I'd also initialize to NULL but you
may omit if you want.

https://codereview.chromium.org/302683002/diff/160022/athena/activity/activit...
File athena/activity/activity_view_manager_impl.cc (right):

https://codereview.chromium.org/302683002/diff/160022/athena/activity/activit...
athena/activity/activity_view_manager_impl.cc:51: params.parent =
ScreenManager::Get()->GetContext();
this should be context

https://codereview.chromium.org/302683002/diff/160022/athena/activity/activit...
athena/activity/activity_view_manager_impl.cc:107: static ActivityViewManager*
instance;
ditto

https://codereview.chromium.org/302683002/diff/160022/athena/screen/backgroun...
File athena/screen/background_controller.cc (right):

https://codereview.chromium.org/302683002/diff/160022/athena/screen/backgroun...
athena/screen/background_controller.cc:28: if (!image_.isNull()) {
StartAthena sets the default background, so you don't need this.

[Mr4D (OOO till 08-26), 2014-05-31 01:21:21]

Looked over it, lgtm. Thanks for changing the names!

[sadrul, 2014-05-31 04:23:45]

https://codereview.chromium.org/302683002/diff/160022/athena/activity/activit...
File athena/activity/activity_manager_impl.cc (right):

https://codereview.chromium.org/302683002/diff/160022/athena/activity/activit...
athena/activity/activity_manager_impl.cc:17: static ActivityManager* instance;
On 2014/05/31 00:58:01, oshima wrote:
> You can nuke static. It's deprecated in C++03, and it's back in C++11 but
> anonymous namespace has internal linkage. I'd also initialize to NULL but you
> may omit if you want.

Done.

https://codereview.chromium.org/302683002/diff/160022/athena/activity/activit...
File athena/activity/activity_view_manager_impl.cc (right):

https://codereview.chromium.org/302683002/diff/160022/athena/activity/activit...
athena/activity/activity_view_manager_impl.cc:51: params.parent =
ScreenManager::Get()->GetContext();
On 2014/05/31 00:58:01, oshima wrote:
> this should be context

Done.

https://codereview.chromium.org/302683002/diff/160022/athena/activity/activit...
athena/activity/activity_view_manager_impl.cc:107: static ActivityViewManager*
instance;
On 2014/05/31 00:58:01, oshima wrote:
> ditto

Done.

[sadrul, 2014-05-31 04:28:57]

Committed patchset #8 manually as r274015 (presubmit successful).