Adding overview / layer framework to Activities so that unloaded / sleeping activities can be shown…

Adding overview / layer framework to Activities so that unloaded / sleeping activities can be shown with as little resources as possible

The new |ContentProxy| object is a placeholder for now which only creates a single color view. It can use the |web_view| to generate the image preview in the next step and will work for WebContents as well as App's.

So more work is necessary, this patch is mainly to get the overview mode working again when running out of memory (and avoiding that it gets too big).

Adding sky for the DEPS inclusion.

BUG=408837
TEST=unittests and visual
TBR=sky@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/73a3f091451b84bb89b288935d938eaa38d39918

[Mr4D (OOO till 08-26), 2014-09-09 23:37:10]

skuhne@chromium.org changed reviewers:
+ mukai@chromium.org, pkotwicz@chromium.org

[Mr4D (OOO till 08-26), 2014-09-09 23:37:19]

Please have a look!

[Jun Mukai, 2014-09-10 00:38:53]

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

https://codereview.chromium.org/548633005/diff/1/athena/content/app_activity....
athena/content/app_activity.cc:196: content_proxy_.reset(NULL);
you can omit NULL, i.e. reset();

Also, you can remove the check of line 195.

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

https://codereview.chromium.org/548633005/diff/1/athena/content/content_proxy...
athena/content/content_proxy.cc:125:
client_view->RemoveChildView(proxy_content_.get());
views::View instance is owned by its parent.  Therefore, after this line,
|proxy_content_| will hold the pointer to the removed view.

You may want to use pointer instead of scoped_ptr.

https://codereview.chromium.org/548633005/diff/1/athena/content/content_proxy.h
File athena/content/content_proxy.h (right):

https://codereview.chromium.org/548633005/diff/1/athena/content/content_proxy...
athena/content/content_proxy.h:63: protected:
This protected section does not make sense to me.
friend class declaration allows access of all protected/private methods
regardless of where it's declared.

[pkotwicz, 2014-09-10 01:43:13]

https://codereview.chromium.org/548633005/diff/1/athena/content/content_proxy.h
File athena/content/content_proxy.h (right):

https://codereview.chromium.org/548633005/diff/1/athena/content/content_proxy...
athena/content/content_proxy.h:38: class ContentProxy {
I wonder whether ContentProxy can inherit from views::View
It would contain two views, a views::WebView which would be passed in through
the ContentProxy constructor and a "proxy" view.

scoped_ptr<ContentProxy> ContentProxy::Clone() would return a new ContentProxy
with a NULL views::WebView

[pkotwicz, 2014-09-10 14:30:21]

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

https://codereview.chromium.org/548633005/diff/1/athena/content/content_proxy...
athena/content/content_proxy.cc:111:
proxy_content_->SetFillsBoundsOpaquely(true);
Are there any cases where |proxy_content_| is painted to a ui::Layer? (i.e.
View::SetPaintToLayer() is called?)
If there are not any, View::SetFillsBoundsOpaquely() is a no-op

https://codereview.chromium.org/548633005/diff/1/athena/content/content_proxy.h
File athena/content/content_proxy.h (right):

https://codereview.chromium.org/548633005/diff/1/athena/content/content_proxy...
athena/content/content_proxy.h:38: class ContentProxy {
Clarification: Ideally, WebActivity::GetContentsView() will return a
ContentProxy, probably more aptly renamed to athena::ContentView
This should allow you to remove ContentView::window_

It might be possible to replace ContentProxy::Reparent() with
scoped_ptr<ContentView> ContentView::Clone()

[Mr4D (OOO till 08-26), 2014-09-10 15:55:46]

Addressed (several items).

Note: I have found one issue where creating the AppProxyWindow will change the
window order and will pop the window in front, covering the selected window. I
will address that separately.

Please have another look!

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

https://codereview.chromium.org/548633005/diff/1/athena/content/app_activity....
athena/content/app_activity.cc:196: content_proxy_.reset(NULL);
On 2014/09/10 00:38:53, Jun Mukai wrote:
> you can omit NULL, i.e. reset();
> 
> Also, you can remove the check of line 195.

Done.

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

https://codereview.chromium.org/548633005/diff/1/athena/content/content_proxy...
athena/content/content_proxy.cc:111:
proxy_content_->SetFillsBoundsOpaquely(true);
Removed.

https://codereview.chromium.org/548633005/diff/1/athena/content/content_proxy...
athena/content/content_proxy.cc:125:
client_view->RemoveChildView(proxy_content_.get());
Right - but the view does not get deleted by this command. The idea being that
the Show / hide can be called multiple times and the view could even be passed
to someone else if we want to paint it in another window. To ensure its proper
deletion I have put it into a scoped_ptr.

Makes sense?

https://codereview.chromium.org/548633005/diff/1/athena/content/content_proxy.h
File athena/content/content_proxy.h (right):

https://codereview.chromium.org/548633005/diff/1/athena/content/content_proxy...
athena/content/content_proxy.h:38: class ContentProxy {
I see what you mean - and yes, I experimented with that yesterday before sending
out the CL. However, after thinking about it I removed it again since it is not
clear yet how the presentation in Overview mode will be done. It could be that
we only hand out the image and this view will be removed. It could also be that
we create a layer and pass around the layer to be shown in a window. Or we might
stick with the view. Furthermore I remember vaguely that there was a problem
with the view being passed back through GetContentsView - but I cannot remember
exactly what it was.

I even added a function to ActivityViewModel to retrieve this object so that the
Overview mode could use this to present this content by use of a passed layer /
image / view / window or what ever.

But again - this is a temporary solution to avoid showing garbage in overview
mode which needs to be refined to show something useful there. Once we are at
that point we can re-consider changing this.

I hope that makes sense?

https://codereview.chromium.org/548633005/diff/1/athena/content/content_proxy...
athena/content/content_proxy.h:63: protected:
Removed. The reason being that these functions should only ever be used by
AppActivity and WebActivity. Adding comment instead.

[pkotwicz, 2014-09-10 18:58:54]

pkotwicz@chromium.org changed reviewers:
+ sadrul@chromium.org

[pkotwicz, 2014-09-10 18:58:55]

Adding sadrul@ as a reviewer. I think he is a more appropriate reviewer than I

[pkotwicz, 2014-09-10 19:00:15]

For completeness, after discussing with skuhne@, on ToT there is no more garbage
in overview mode. It got fixed since the CL got written

[Mr4D (OOO till 08-26), 2014-09-10 19:27:50]

For completeness, I also said that this patch has several fixes in it and that
most of it will remain as it is which makes me still recommend bringing it in
rather sooner then later.

[Jun Mukai, 2014-09-10 21:04:57]

I will also delegate to sadrul for this. The code looks sane to me though.

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

https://codereview.chromium.org/548633005/diff/1/athena/content/content_proxy...
athena/content/content_proxy.cc:125:
client_view->RemoveChildView(proxy_content_.get());
On 2014/09/10 15:55:45, Mr4D wrote:
> Right - but the view does not get deleted by this command. The idea being that
> the Show / hide can be called multiple times and the view could even be passed
> to someone else if we want to paint it in another window. To ensure its proper
> deletion I have put it into a scoped_ptr.
> 
> Makes sense?

Ah, I see, I was wrong (I believed RemoveChildView implicitly removes the view).

[sadrul, 2014-09-11 19:37:55]

https://codereview.chromium.org/548633005/diff/20001/athena/content/app_activ...
File athena/content/app_activity_registry.cc (right):

https://codereview.chromium.org/548633005/diff/20001/athena/content/app_activ...
athena/content/app_activity_registry.cc:89:
base::ThreadTaskRunnerHandle::Get()->PostNonNestableTask(
Why non-nestable?

https://codereview.chromium.org/548633005/diff/20001/athena/content/content_p...
File athena/content/content_proxy.cc (right):

https://codereview.chromium.org/548633005/diff/20001/athena/content/content_p...
athena/content/content_proxy.cc:111: proxy_content_.reset(new views::View());
You should set_owned_by_client() it.

https://codereview.chromium.org/548633005/diff/20001/athena/content/content_p...
athena/content/content_proxy.cc:118: views::View* client_view =
widget->client_view();
null check widget?

https://codereview.chromium.org/548633005/diff/20001/athena/content/content_p...
athena/content/content_proxy.cc:120:
client_view->AddChildViewAt(proxy_content_.get(), 0);
The View at index 0 is at the back of all its siblings.

[Mr4D (OOO till 08-26), 2014-09-11 21:52:42]

Rebased, fixed resulting problems and addressed comments.

Please have another look!

https://codereview.chromium.org/548633005/diff/20001/athena/content/app_activ...
File athena/content/app_activity_registry.cc (right):

https://codereview.chromium.org/548633005/diff/20001/athena/content/app_activ...
athena/content/app_activity_registry.cc:89:
base::ThreadTaskRunnerHandle::Get()->PostNonNestableTask(
Mainly because it got suggested to me. Reading through the comments PostTask
would also do the trick though, so I have replaced it.

https://codereview.chromium.org/548633005/diff/20001/athena/content/content_p...
File athena/content/content_proxy.cc (right):

https://codereview.chromium.org/548633005/diff/20001/athena/content/content_p...
athena/content/content_proxy.cc:111: proxy_content_.reset(new views::View());
On 2014/09/11 19:37:54, sadrul wrote:
> You should set_owned_by_client() it.

Done.

https://codereview.chromium.org/548633005/diff/20001/athena/content/content_p...
athena/content/content_proxy.cc:118: views::View* client_view =
widget->client_view();
On 2014/09/11 19:37:54, sadrul wrote:
> null check widget?

Done.

https://codereview.chromium.org/548633005/diff/20001/athena/content/content_p...
athena/content/content_proxy.cc:120:
client_view->AddChildViewAt(proxy_content_.get(), 0);
On 2014/09/11 19:37:54, sadrul wrote:
> The View at index 0 is at the back of all its siblings.

Done.

[sadrul, 2014-09-12 16:30:15]

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

https://codereview.chromium.org/548633005/diff/40001/athena/content/app_activ...
athena/content/app_activity.h:71: // Showing a content proxy instead of the real
content to save resoruces.
*resources

https://codereview.chromium.org/548633005/diff/40001/athena/content/app_activ...
File athena/content/app_activity_proxy.cc (right):

https://codereview.chromium.org/548633005/diff/40001/athena/content/app_activ...
athena/content/app_activity_proxy.cc:71:
wm::ActivateWindow(window_list_provider->GetWindowList().back());
Why does calling StackWindowBehindTo() change activation of the windows? This
sounds like a bug somewhere that we should fix first.

https://codereview.chromium.org/548633005/diff/40001/athena/content/app_activ...
File athena/content/app_activity_registry.cc (right):

https://codereview.chromium.org/548633005/diff/40001/athena/content/app_activ...
athena/content/app_activity_registry.cc:87: // while we are in an observerloop.
I am not sure what this means, but if you mean while iterating over
ObserverList, then that should be fine (i.e. you can add/remove observers from
the ObserverList you are iterating over).

https://codereview.chromium.org/548633005/diff/40001/athena/content/app_activ...
File athena/content/app_activity_unittest.cc (right):

https://codereview.chromium.org/548633005/diff/40001/athena/content/app_activ...
athena/content/app_activity_unittest.cc:369:
app_activity1->app_activity_registry()->Unload();
Looks like AppActivity::SetCurrentState() takes care of calling Unload() on the
app_activity_registry() ... so remove these explicit calls to Unload()?

https://codereview.chromium.org/548633005/diff/40001/athena/content/content_p...
File athena/content/content_proxy.cc (right):

https://codereview.chromium.org/548633005/diff/40001/athena/content/content_p...
athena/content/content_proxy.cc:123: client_view->child_count() - 1);
This is going to add the view just behind the front-most view. Just use
AddChildView()

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

https://codereview.chromium.org/548633005/diff/40001/athena/content/web_activ...
athena/content/web_activity.cc:413: if (content_proxy_.get())
You can just do if (content_proxy_) or if (!content_proxy_)

https://codereview.chromium.org/548633005/diff/40001/athena/resource_manager/...
File athena/resource_manager/delegate/resource_manager_delegate.cc (right):

https://codereview.chromium.org/548633005/diff/40001/athena/resource_manager/...
athena/resource_manager/delegate/resource_manager_delegate.cc:28: return 99;
?

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

Please have another look!

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

https://codereview.chromium.org/548633005/diff/40001/athena/content/app_activ...
athena/content/app_activity.h:71: // Showing a content proxy instead of the real
content to save resoruces.
On 2014/09/12 16:30:14, sadrul wrote:
> *resources

Done.

https://codereview.chromium.org/548633005/diff/40001/athena/content/app_activ...
File athena/content/app_activity_proxy.cc (right):

https://codereview.chromium.org/548633005/diff/40001/athena/content/app_activ...
athena/content/app_activity_proxy.cc:71:
wm::ActivateWindow(window_list_provider->GetWindowList().back());
It's not that. When the resource manager unloads an item, all activities get
unloaded and the windows get deleted. Then this proxy get created. By creating
the proxy window (and the associated AthenaFrame, a show will get called which
then changes the activation order. But as the TODO states - this should be
addressed at a different place.

https://codereview.chromium.org/548633005/diff/40001/athena/content/app_activ...
File athena/content/app_activity_registry.cc (right):

https://codereview.chromium.org/548633005/diff/40001/athena/content/app_activ...
athena/content/app_activity_registry.cc:87: // while we are in an observerloop.
Updated the comment.

But I do not remember exactly how I did get into this situation. (I think it was
a special case where the last item got erased) I will have another look into
this and let you know, but first lets land this patch (there is already far too
much addressed which is far from ideal).

https://codereview.chromium.org/548633005/diff/40001/athena/content/app_activ...
File athena/content/app_activity_unittest.cc (right):

https://codereview.chromium.org/548633005/diff/40001/athena/content/app_activ...
athena/content/app_activity_unittest.cc:369:
app_activity1->app_activity_registry()->Unload();
On 2014/09/12 16:30:14, sadrul wrote:
> Looks like AppActivity::SetCurrentState() takes care of calling Unload() on
the
> app_activity_registry() ... so remove these explicit calls to Unload()?

Done.

https://codereview.chromium.org/548633005/diff/40001/athena/content/content_p...
File athena/content/content_proxy.cc (right):

https://codereview.chromium.org/548633005/diff/40001/athena/content/content_p...
athena/content/content_proxy.cc:123: client_view->child_count() - 1);
On 2014/09/12 16:30:14, sadrul wrote:
> This is going to add the view just behind the front-most view. Just use
> AddChildView()

Done.

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

https://codereview.chromium.org/548633005/diff/40001/athena/content/web_activ...
athena/content/web_activity.cc:413: if (content_proxy_.get())
On 2014/09/12 16:30:14, sadrul wrote:
> You can just do if (content_proxy_) or if (!content_proxy_)

Done.

https://codereview.chromium.org/548633005/diff/40001/athena/resource_manager/...
File athena/resource_manager/delegate/resource_manager_delegate.cc (right):

https://codereview.chromium.org/548633005/diff/40001/athena/resource_manager/...
athena/resource_manager/delegate/resource_manager_delegate.cc:28: return 99;
Good one. :) Done.

[sadrul, 2014-09-15 14:13:37]

sadrul@chromium.org changed reviewers:
+ oshima@chromium.org

[sadrul, 2014-09-15 14:13:37]

lgtm

+oshima@ for ideas on fixing the issue in AppActivityProxy::Init()

[Mr4D (OOO till 08-26), 2014-09-15 14:56:32]

Thanks for reviewing!

I will land this patch and address the the activity change in another patch.

[Mr4D (OOO till 08-26), 2014-09-15 14:56:36]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-09-15 14:57:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/548633005/60001

[commit-bot: I haz the power, 2014-09-15 15:07:18]

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

[commit-bot: I haz the power, 2014-09-15 15:07:19]

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

[Mr4D (OOO till 08-26), 2014-09-15 15:12:28]

Never mind then - apparently 

    athena/activity/public/activity_view_model.h
    athena/athena.gyp
    athena/test/sample_activity.cc
    athena/test/sample_activity.h

need another lgtm. Jun/Oshima Please have a look!

[Jun Mukai, 2014-09-15 16:17:55]

lgtm

[Mr4D (OOO till 08-26), 2014-09-15 16:23:44]

The CQ bit was checked by skuhne@chromium.org

[Mr4D (OOO till 08-26), 2014-09-15 16:23:48]

The CQ bit was unchecked by skuhne@chromium.org

[Mr4D (OOO till 08-26), 2014-09-15 16:24:23]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-09-15 16:28:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/548633005/60001

[oshima, 2014-09-15 16:37:11]

https://codereview.chromium.org/548633005/diff/40001/athena/content/app_activ...
File athena/content/app_activity_proxy.cc (right):

https://codereview.chromium.org/548633005/diff/40001/athena/content/app_activ...
athena/content/app_activity_proxy.cc:71:
wm::ActivateWindow(window_list_provider->GetWindowList().back());
On 2014/09/12 20:11:26, Mr4D wrote:
> It's not that. When the resource manager unloads an item, all activities get
> unloaded and the windows get deleted. Then this proxy get created. By creating
> the proxy window (and the associated AthenaFrame, a show will get called which
> then changes the activation order. But as the TODO states - this should be
> addressed at a different place.

Widget::ShowInactive?

[commit-bot: I haz the power, 2014-09-15 16:50:11]

Patchset 4 (id:??) landed as
https://crrev.com/60014850f1c3d283bb68cc77c880e46abaea13c0
Cr-Commit-Position: refs/heads/master@{#294837}

[commit-bot: I haz the power, 2014-09-15 16:50:32]

Committed patchset #4 (id:60001) as 73a3f091451b84bb89b288935d938eaa38d39918