Additions to Activities to allow resource management

Additions to Activities to allow resource management

This is by no means finished. There are plenty of things to do - but I send it out now so that you can have a look at it as it is to see what my plans are.

Note: The activities management functions are not yet called. So there should be no impact on anything running yet. 

Please have a look! If you generally agree I will try to get it in before I go on vacation, otherwise it will have to wait until I get back. I might even work tomorrow instead of taking off as planned if needed.

BUG=388085
TEST=not yet done
TBR=rsesek

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

[Mr4D (OOO till 08-26), 2014-06-26 18:24:04]

Not really ready yet, but can make it into a patch before I disappear. Note that
this has no real impact yet since the management functions are not there yet to
call the new functionality.

[Jun Mukai, 2014-06-26 19:48:01]

https://codereview.chromium.org/358003002/diff/1/athena/activity/public/activ...
File athena/activity/public/activity.h (right):

https://codereview.chromium.org/358003002/diff/1/athena/activity/public/activ...
athena/activity/public/activity.h:35: // The activity is not visible, but it is
active e.g. by streaming audio.
Got confused by the comment a bit. So ACTIVE doesn't mean the active-ness in
ui/wm context (meaning topmost / keyboard event handling) but something
different? Can we use another term here? I don't have concrete alternatives
though...

https://codereview.chromium.org/358003002/diff/1/athena/activity/public/activ...
athena/activity/public/activity.h:41: ACTIVITY_STATE_DEEP_SLEEP_1,
sleep_1 and sleep_2 doesn't sound good... :(

https://codereview.chromium.org/358003002/diff/1/athena/content/web_activity.cc
File athena/content/web_activity.cc (right):

https://codereview.chromium.org/358003002/diff/1/athena/content/web_activity....
athena/content/web_activity.cc:236: web_view_->EvictContent();
Can we just discard web_view_ itself instead of implementing EvictContent?

[Mr4D (OOO till 08-26), 2014-06-26 20:25:39]

A few comments before I start changing.

https://codereview.chromium.org/358003002/diff/1/athena/activity/public/activ...
File athena/activity/public/activity.h (right):

https://codereview.chromium.org/358003002/diff/1/athena/activity/public/activ...
athena/activity/public/activity.h:35: // The activity is not visible, but it is
active e.g. by streaming audio.
Sure!

Note: There are 2 "Active" levels: 1=Visible (means: activity is full on)
2=(Audio)Active (means that the activity is actively doing something and it
should kept in a running state (releasing gfx resources though)

So - what about ACTIVITY_STATE_BACKGROUND_ACTIVE?

https://codereview.chromium.org/358003002/diff/1/athena/activity/public/activ...
athena/activity/public/activity.h:41: ACTIVITY_STATE_DEEP_SLEEP_1,
Right. I was hoping to nail this down in this weeks EngReview meeting - but that
did not take place. For the time being I might simply remove "_1" and the entire
second state.

https://codereview.chromium.org/358003002/diff/1/athena/content/web_activity.cc
File athena/content/web_activity.cc (right):

https://codereview.chromium.org/358003002/diff/1/athena/content/web_activity....
athena/content/web_activity.cc:236: web_view_->EvictContent();
Well... WebView is used by another instance (the one who calls
"GetContentsView"). As such I kept it alive.

Note: I was actually even considering to use it to show the content for
navigation.

Last but not least - this follows the way the TabModel uses it.

That all said - sure, we can of course do it that way. I did not complete my
investigation of how to do this with applications and there we might have to do
something like that (since the ShellWindow has a reference to the WebView which
we possibly cannot simply break) .

But my main concern is to agree on the general SW structure - the "how it is
done in the end" might still change.

[Jun Mukai, 2014-06-26 21:45:26]

https://codereview.chromium.org/358003002/diff/1/athena/content/web_activity.cc
File athena/content/web_activity.cc (right):

https://codereview.chromium.org/358003002/diff/1/athena/content/web_activity....
athena/content/web_activity.cc:236: web_view_->EvictContent();
On 2014/06/26 20:25:39, Mr4D wrote:
> Well... WebView is used by another instance (the one who calls
> "GetContentsView"). As such I kept it alive.
> 
> Note: I was actually even considering to use it to show the content for
> navigation.
> 
> Last but not least - this follows the way the TabModel uses it.
> 
> That all said - sure, we can of course do it that way. I did not complete my
> investigation of how to do this with applications and there we might have to
do
> something like that (since the ShellWindow has a reference to the WebView
which
> we possibly cannot simply break) .
> 
> But my main concern is to agree on the general SW structure - the "how it is
> done in the end" might still change.

Okay, doing the same approach as TabModel sounds promising. I also want oshima
to review this though.

[oshima, 2014-06-26 22:46:25]

I'm currently interrupted by high dpi work but will take a look as soon as
possible. I also think we should have VC to discuss a bit more details tomorrow
as there is a overlap with window management (not necessarily mean bad, but we
just need to discuss the direction and plan).

[oshima, 2014-07-11 18:02:59]

https://codereview.chromium.org/358003002/diff/40001/athena/activity/public/a...
File athena/activity/public/activity.h (right):

https://codereview.chromium.org/358003002/diff/40001/athena/activity/public/a...
athena/activity/public/activity.h:45: };
As per discussion, I'd recommend to start with hour states,

ACTIVITY_UNLOADED,
ACTIVITY_PERSISTENT,
ACTIVITY_BACKGROUND_LOW_PRIORITY,
ACTIVITY_NORMAL

and use the state as a transition command. We can add more states as we make
progress.

https://codereview.chromium.org/358003002/diff/40001/athena/activity/public/a...
File athena/activity/public/activity_view_model.h (right):

https://codereview.chromium.org/358003002/diff/40001/athena/activity/public/a...
athena/activity/public/activity_view_model.h:45: virtual void
CreateOverviewModeImage() = 0;
Since the actual behavior is undefined, how about "PrepareOverviewModeImage()" ?

https://codereview.chromium.org/358003002/diff/40001/athena/content/app_activ...
File athena/content/app_activity.h (right):

https://codereview.chromium.org/358003002/diff/40001/athena/content/app_activ...
athena/content/app_activity.h:12: //#include "ui/gfx/image/image_skia.h"
ui/gfx is fine

https://codereview.chromium.org/358003002/diff/40001/athena/content/web_activ...
File athena/content/web_activity.cc (right):

https://codereview.chromium.org/358003002/diff/40001/athena/content/web_activ...
athena/content/web_activity.cc:168: bool IsContentEvicted() { return
!evicted_web_contents_.get(); }
did you mean "!!" ?

const

https://codereview.chromium.org/358003002/diff/40001/athena/content/web_activ...
athena/content/web_activity.cc:191: 
remove extra new line

[Mr4D (OOO till 08-26), 2014-07-11 22:38:09]

There are still a lot of TODO's, but here is the patch with the addressed
portions.

https://codereview.chromium.org/358003002/diff/40001/athena/activity/public/a...
File athena/activity/public/activity.h (right):

https://codereview.chromium.org/358003002/diff/40001/athena/activity/public/a...
athena/activity/public/activity.h:45: };
Changed as discussed

https://codereview.chromium.org/358003002/diff/40001/athena/activity/public/a...
File athena/activity/public/activity_view_model.h (right):

https://codereview.chromium.org/358003002/diff/40001/athena/activity/public/a...
athena/activity/public/activity_view_model.h:45: virtual void
CreateOverviewModeImage() = 0;
In the question of "Prepare" vs. "create" I would always pick "create" since
"prepare" suggests that you need to do another call to actually really do it. I
would prefer to keep the name...

https://codereview.chromium.org/358003002/diff/40001/athena/content/app_activ...
File athena/content/app_activity.h (right):

https://codereview.chromium.org/358003002/diff/40001/athena/content/app_activ...
athena/content/app_activity.h:12: //#include "ui/gfx/image/image_skia.h"
Okay. Try again - it didn't work 2 weeks ago. ;)

https://codereview.chromium.org/358003002/diff/40001/athena/content/web_activ...
File athena/content/web_activity.cc (right):

https://codereview.chromium.org/358003002/diff/40001/athena/content/web_activ...
athena/content/web_activity.cc:168: bool IsContentEvicted() { return
!evicted_web_contents_.get(); }
In deed. Done!

https://codereview.chromium.org/358003002/diff/40001/athena/content/web_activ...
athena/content/web_activity.cc:191: 
On 2014/07/11 18:02:59, oshima wrote:
> remove extra new line

Done.

[oshima, 2014-07-11 23:45:45]

can you also update sample_activity? you can build it with the target
athena_shell

https://codereview.chromium.org/358003002/diff/60001/athena/activity/public/a...
File athena/activity/public/activity.h (right):

https://codereview.chromium.org/358003002/diff/60001/athena/activity/public/a...
athena/activity/public/activity.h:34: ACTIVITY_VISIBLE,
I thought you wanted to name it ACTIVE?

https://codereview.chromium.org/358003002/diff/60001/athena/content/DEPS
File athena/content/DEPS (right):

https://codereview.chromium.org/358003002/diff/60001/athena/content/DEPS#newc...
athena/content/DEPS:10: "+ui/gfx/image",
you may just add ui/gfx

https://codereview.chromium.org/358003002/diff/60001/athena/content/app_activ...
File athena/content/app_activity.cc (right):

https://codereview.chromium.org/358003002/diff/60001/athena/content/app_activ...
athena/content/app_activity.cc:39: // If this does not work, the eviction needs
to take place at a higher level.
indent. please remove the commented out code. you can store it in your private
branch, or upload to crrev to keep it.

[Mr4D (OOO till 08-26), 2014-07-12 01:38:38]

Done! Please have another look!

https://codereview.chromium.org/358003002/diff/60001/athena/activity/public/a...
File athena/activity/public/activity.h (right):

https://codereview.chromium.org/358003002/diff/60001/athena/activity/public/a...
athena/activity/public/activity.h:34: ACTIVITY_VISIBLE,
Right - but somehow INVISIBLE and ACTIVE together sounds odd - or not? If you
want ACTIVE you get active :)

https://codereview.chromium.org/358003002/diff/60001/athena/content/DEPS
File athena/content/DEPS (right):

https://codereview.chromium.org/358003002/diff/60001/athena/content/DEPS#newc...
athena/content/DEPS:10: "+ui/gfx/image",
On 2014/07/11 23:45:44, oshima wrote:
> you may just add ui/gfx

Done.

https://codereview.chromium.org/358003002/diff/60001/athena/content/app_activ...
File athena/content/app_activity.cc (right):

https://codereview.chromium.org/358003002/diff/60001/athena/content/app_activ...
athena/content/app_activity.cc:39: // If this does not work, the eviction needs
to take place at a higher level.
On 2014/07/11 23:45:44, oshima wrote:
> indent. please remove the commented out code. you can store it in your private
> branch, or upload to crrev to keep it.

Done.

[oshima, 2014-07-12 01:43:24]

lgtm

thanks. we can revisit the name again if necessary.

https://codereview.chromium.org/358003002/diff/70001/athena/test/sample_activ...
File athena/test/sample_activity.cc (right):

https://codereview.chromium.org/358003002/diff/70001/athena/test/sample_activ...
athena/test/sample_activity.cc:74: return overview_mode_image_;
i guess we'll never create an image for this? If so,
you can just return gfx::ImageSkia()

you can remove member variable and also remove include in the header.

[Mr4D (OOO till 08-26), 2014-07-12 01:46:33]

Thanks!

https://codereview.chromium.org/358003002/diff/70001/athena/test/sample_activ...
File athena/test/sample_activity.cc (right):

https://codereview.chromium.org/358003002/diff/70001/athena/test/sample_activ...
athena/test/sample_activity.cc:74: return overview_mode_image_;
On 2014/07/12 01:43:23, oshima wrote:
> i guess we'll never create an image for this? If so,
> you can just return gfx::ImageSkia()
> 
> you can remove member variable and also remove include in the header.

Done.

[Mr4D (OOO till 08-26), 2014-07-12 01:46:40]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-07-12 01:47:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/skuhne@chromium.org/358003002/90001

[commit-bot: I haz the power, 2014-07-12 03:37:27]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-07-12 03:40:13]

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

[commit-bot: I haz the power, 2014-07-12 03:40:14]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[Mr4D (OOO till 08-26), 2014-07-12 21:52:26]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-07-12 21:53:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/skuhne@chromium.org/358003002/90001

[commit-bot: I haz the power, 2014-07-12 23:01:30]

Change committed as 282841