Decoupling visibility states from webcontent, adding visibility management in ResourceManager

Decoupling visibility states from webcontent, adding visibility management in ResourceManager

It works as desired. An abstract class to handle overview representations of unloaded WebContents has still to be added, but since the CL is already quite big as is, I split this up into a follow up CL.

BUG=408826, 408834
TEST=AppActivityTest.*, ResourceManager.* and visual

Committed: https://crrev.com/2d350ffed31ecb2fa67d131585e50e1df2ca2d09
Cr-Commit-Position: refs/heads/master@{#293414}

[Mr4D (OOO till 08-26), 2014-09-03 19:45:39]

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

[Mr4D (OOO till 08-26), 2014-09-03 19:45:49]

Please have a look!

[oshima, 2014-09-03 23:15:00]

before going to details on other file, I have one key question on the change
under contents/

https://codereview.chromium.org/536013002/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/536013002/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura.cc:1606: // ResourceManager
and not the window visibility.
have you consult this approach (if def) with content owner?

I have a few concerns here.
1) This probably will break several tests for contents. If we disable them, we
lose coverage.
2) This also can mean things may regress only on athena without being caught by
bots.

I believe that we want to decouple window visibility and web contents
visibility. Is it possible to decouple that way instead?

[oshima, 2014-09-03 23:15:01]

before going to details on other file, I have one key question on the change
under contents/

[oshima, 2014-09-04 16:39:23]

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

https://codereview.chromium.org/536013002/diff/20001/athena/activity/activity...
athena/activity/activity.cc:14: delete activity;
Maybe we should just have

ActivityManager::DeleteActivity instead ?

And just out of my curiosity, what kind of issue you had?
(I can guess, but want to know exact issue/crash you had)

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

https://codereview.chromium.org/536013002/diff/20001/athena/content/app_activ...
athena/content/app_activity.cc:67: }
nuke {}

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

https://codereview.chromium.org/536013002/diff/20001/athena/content/app_activ...
athena/content/app_activity_registry.cc:104: AppActivity*
AppActivityRegistry::GetMruActivity() {
MruActivity may not be app activity right?

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

https://codereview.chromium.org/536013002/diff/20001/athena/content/app_activ...
athena/content/app_activity_registry.h:62: Activity* unloaded_activity_proxy()
const { return unloaded_activity_proxy_; }
remove const

(a method that returns non const internal object should be non const.)

https://codereview.chromium.org/536013002/diff/20001/athena/resource_manager/...
File athena/resource_manager/public/resource_manager.h (right):

https://codereview.chromium.org/536013002/diff/20001/athena/resource_manager/...
athena/resource_manager/public/resource_manager.h:34: // Disable the resource
manager temporarily if |pause| is set. This can be
"Disable" may be misleading. How about just say "Pauses" or "Suspends"?

https://codereview.chromium.org/536013002/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/536013002/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura.cc:1600:
UpdateWebContentsVisibility(visible);
If we use window_->IsVisible() here, instead of
visible, and can we control visibility by changing the window_'s visibility?

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

Addressed! Please have another look!

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

https://codereview.chromium.org/536013002/diff/20001/athena/activity/activity...
athena/activity/activity.cc:14: delete activity;
During destruction a pure virtual call occured inside
ResourceManagerImpl::OnActivityEnding while accessing the Activity to remove a
window observer. This got called by the ActivityManager as part of the
unregistering at which time the object is apparently already halfway gone.

Renamed to Activity::Delete as discussed.

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

https://codereview.chromium.org/536013002/diff/20001/athena/content/app_activ...
athena/content/app_activity.cc:67: }
On 2014/09/04 16:39:23, oshima wrote:
> nuke {}

Done.

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

https://codereview.chromium.org/536013002/diff/20001/athena/content/app_activ...
athena/content/app_activity_registry.cc:104: AppActivity*
AppActivityRegistry::GetMruActivity() {
Well - this is the MRU for *this* app (if there are more). So this is correct -
when looking at it in perspective of this app (which is the reason why it is a
class member of this class). Makes sense?

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

https://codereview.chromium.org/536013002/diff/20001/athena/content/app_activ...
athena/content/app_activity_registry.h:62: Activity* unloaded_activity_proxy()
const { return unloaded_activity_proxy_; }
On 2014/09/04 16:39:23, oshima wrote:
> remove const
> 
> (a method that returns non const internal object should be non const.)

Done.

https://codereview.chromium.org/536013002/diff/20001/athena/resource_manager/...
File athena/resource_manager/public/resource_manager.h (right):

https://codereview.chromium.org/536013002/diff/20001/athena/resource_manager/...
athena/resource_manager/public/resource_manager.h:34: // Disable the resource
manager temporarily if |pause| is set. This can be
On 2014/09/04 16:39:23, oshima wrote:
> "Disable" may be misleading. How about just say "Pauses" or "Suspends"?

Done.

https://codereview.chromium.org/536013002/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/536013002/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura.cc:1600:
UpdateWebContentsVisibility(visible);
Very complicated at this place. 
a. Window_->IsVisible() looks at this window and the chain of parents and their
layers to see if it is visible or not.
b. There are situations (e.g. bubbles in Windows) where turning the
WebContentsView invisible once, it will immediately destroy all its buffers.
When being made visible again, the content does not know its size and the
resulting layer is far too small and garbage is shown until content is loaded.
So in that case for example it should not get hidden - even though it would
follow this. And there are many more special cases like that.
c. I discussed with sky and he said that it would be wrong to control the
resources through the visibility and instead we should listen to the renderer
for resource deallocation messages and trigger from that the preview generation.
Not quite sure how much work that might be.

Anyways - need to do this differently and I will remove this and the logic which
I had put in place to control these states - at least for the most part.

https://codereview.chromium.org/536013002/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura.cc:1606: // ResourceManager
and not the window visibility.
We don't do this. I have to come up with a different method to address this
(ensuring that we create an overview image before the layers get released by the
renderer, ..).

[oshima, 2014-09-04 22:11:01]

lgtm with nits.

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

https://codereview.chromium.org/536013002/diff/20001/athena/content/app_activ...
athena/content/app_activity_registry.h:77: // Get the most recently used
activity.
how about

// Gets most recently used AppAcitivty that belongs to the same application.

?

https://codereview.chromium.org/536013002/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/536013002/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura.cc:1600:
UpdateWebContentsVisibility(visible);
On 2014/09/04 19:10:57, Mr4D wrote:
> Very complicated at this place. 
> a. Window_->IsVisible() looks at this window and the chain of parents and
their
> layers to see if it is visible or not.

Yes, I believe that's what we want in UpdateWebContentsVisibility, and
consistent with
OnWnidowParentChanged.

> b. There are situations (e.g. bubbles in Windows) where turning the
> WebContentsView invisible once, it will immediately destroy all its buffers.
> When being made visible again, the content does not know its size and the
> resulting layer is far too small and garbage is shown until content is loaded.
> So in that case for example it should not get hidden - even though it would
> follow this. And there are many more special cases like that.

The current code DOES hide in the scenario you mentioned. The difference that
above change
will make is: when the web content is hidden and parent window (such as top
level) is shown.

 Before: WasShown gets called. 
 After: WasShown won't be called.

> c. I discussed with sky and he said that it would be wrong to control the
> resources through the visibility and instead we should listen to the renderer
> for resource deallocation messages and trigger from that the preview
generation.
> Not quite sure how much work that might be.

I think we just need both signals to manage resources (as we want to do
aggressive resource management).

> 
> Anyways - need to do this differently and I will remove this and the logic
which
> I had put in place to control these states - at least for the most part.

Sure. Let's leave it for the (hopefully near) future.

https://codereview.chromium.org/536013002/diff/40001/athena/activity/activity.cc
File athena/activity/activity.cc (left):

https://codereview.chromium.org/536013002/diff/40001/athena/activity/activity...
athena/activity/activity.cc:12: ActivityManager::Get()->RemoveActivity(this);
optional: maybe we should DCHECK to make sure that ActivityManager doesn't have
this activity?

[Mr4D (OOO till 08-26), 2014-09-04 22:36:52]

Many thanks for your review!

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

https://codereview.chromium.org/536013002/diff/20001/athena/content/app_activ...
athena/content/app_activity_registry.h:77: // Get the most recently used
activity.
On 2014/09/04 22:11:01, oshima wrote:
> how about
> 
> // Gets most recently used AppAcitivty that belongs to the same application.
> 
> ?

Done.

https://codereview.chromium.org/536013002/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/536013002/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_view_aura.cc:1600:
UpdateWebContentsVisibility(visible);
On 2014/09/04 22:11:01, oshima wrote:
> On 2014/09/04 19:10:57, Mr4D wrote:
> > Very complicated at this place. 
> > a. Window_->IsVisible() looks at this window and the chain of parents and
> their
> > layers to see if it is visible or not.
> 
> Yes, I believe that's what we want in UpdateWebContentsVisibility, and
> consistent with
> OnWnidowParentChanged.
> 
> > b. There are situations (e.g. bubbles in Windows) where turning the
> > WebContentsView invisible once, it will immediately destroy all its buffers.
> > When being made visible again, the content does not know its size and the
> > resulting layer is far too small and garbage is shown until content is
loaded.
> > So in that case for example it should not get hidden - even though it would
> > follow this. And there are many more special cases like that.
> 
> The current code DOES hide in the scenario you mentioned. The difference that
> above change
> will make is: when the web content is hidden and parent window (such as top
> level) is shown.
> 
>  Before: WasShown gets called. 
>  After: WasShown won't be called.
> 
> > c. I discussed with sky and he said that it would be wrong to control the
> > resources through the visibility and instead we should listen to the
renderer
> > for resource deallocation messages and trigger from that the preview
> generation.
> > Not quite sure how much work that might be.
> 
> I think we just need both signals to manage resources (as we want to do
> aggressive resource management).
> 
> > 
> > Anyways - need to do this differently and I will remove this and the logic
> which
> > I had put in place to control these states - at least for the most part.
> 
> Sure. Let's leave it for the (hopefully near) future.

Acknowledged.

https://codereview.chromium.org/536013002/diff/40001/athena/activity/activity.cc
File athena/activity/activity.cc (left):

https://codereview.chromium.org/536013002/diff/40001/athena/activity/activity...
athena/activity/activity.cc:12: ActivityManager::Get()->RemoveActivity(this);
Ack. There was no need for an ActivityManager::IsActivityRegistered() (or
similar) yet. As such I won't do this yet.

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

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-09-04 22:38:38]

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

[commit-bot: I haz the power, 2014-09-05 03:06:48]

Committed patchset #4 (id:60001) as 02fe17192b69d313e1abfb38268df5aa42ef8cb1

[commit-bot: I haz the power, 2014-09-10 03:36:03]

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