A visibile to invisible state change will also release memory and should therefore wait as an unloa…

A visibile to invisible state change will also release memory and should therefore wait as an unload does

A visibility change will also release memory and should therefore also stall an immediate unload.

Updated comments and cleaned up the ResourceManager function.

BUG=414918
TEST=unittest

Committed: https://crrev.com/e07573d4bf70bbb0d5181864cce6f265b06625fb
Cr-Commit-Position: refs/heads/master@{#296118}

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

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

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

Please have a look!

[oshima, 2014-09-22 16:22:51]

lgtm with nits

https://codereview.chromium.org/588823002/diff/1/athena/resource_manager/reso...
File athena/resource_manager/resource_manager_impl.cc (right):

https://codereview.chromium.org/588823002/diff/1/athena/resource_manager/reso...
athena/resource_manager/resource_manager_impl.cc:97: void
ResourcesGotReleased();
OnResourceReleased is more common.

https://codereview.chromium.org/588823002/diff/1/athena/resource_manager/reso...
athena/resource_manager/resource_manager_impl.cc:101: void
MemoryPressureIncreased();
ditto. OnMemoryPressureIncreased

https://codereview.chromium.org/588823002/diff/1/athena/resource_manager/reso...
athena/resource_manager/resource_manager_impl.cc:294: void
ResourceManagerImpl::CheckVisibilityStates() {
how about UpdateVisibilityStates?

https://codereview.chromium.org/588823002/diff/1/athena/resource_manager/reso...
athena/resource_manager/resource_manager_impl.cc:301: max_activities = 1 +
(in_splitview_mode_ ? 1 : 0);
in_spit_view_mode ? 2 : 1

https://codereview.chromium.org/588823002/diff/1/athena/resource_manager/reso...
athena/resource_manager/resource_manager_impl.cc:454:
next_resource_management_time_ = base::Time::Now();
Note that Now() may not always increase (that is, may go back), so the next
AllowedToUnloadActivty still may return false.

It's rare so this shouldn't be an issue, but just wanted to point out.

[Mr4D (OOO till 08-26), 2014-09-23 00:28:56]

Addressed! Thanks for the review!

https://codereview.chromium.org/588823002/diff/1/athena/resource_manager/reso...
File athena/resource_manager/resource_manager_impl.cc (right):

https://codereview.chromium.org/588823002/diff/1/athena/resource_manager/reso...
athena/resource_manager/resource_manager_impl.cc:97: void
ResourcesGotReleased();
On 2014/09/22 16:22:51, oshima wrote:
> OnResourceReleased is more common.

Done.

[Even though I have to say that the On.. notion feels more like an external
event and not like an internal function]

https://codereview.chromium.org/588823002/diff/1/athena/resource_manager/reso...
athena/resource_manager/resource_manager_impl.cc:101: void
MemoryPressureIncreased();
Done. And ditto.

https://codereview.chromium.org/588823002/diff/1/athena/resource_manager/reso...
athena/resource_manager/resource_manager_impl.cc:294: void
ResourceManagerImpl::CheckVisibilityStates() {
On 2014/09/22 16:22:50, oshima wrote:
> how about UpdateVisibilityStates?

Done.

https://codereview.chromium.org/588823002/diff/1/athena/resource_manager/reso...
athena/resource_manager/resource_manager_impl.cc:301: max_activities = 1 +
(in_splitview_mode_ ? 1 : 0);
On 2014/09/22 16:22:51, oshima wrote:
> in_spit_view_mode ? 2 : 1

Done.

https://codereview.chromium.org/588823002/diff/1/athena/resource_manager/reso...
athena/resource_manager/resource_manager_impl.cc:454:
next_resource_management_time_ = base::Time::Now();
Which would be ok! All which is required is that ResourcesGotReleased() is
setting it to a later time. So no problem!

[Mr4D (OOO till 08-26), 2014-09-23 00:29:00]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-09-23 00:29:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/588823002/20001

[oshima, 2014-09-23 00:42:36]

s lgtm

https://codereview.chromium.org/588823002/diff/1/athena/resource_manager/reso...
File athena/resource_manager/resource_manager_impl.cc (right):

https://codereview.chromium.org/588823002/diff/1/athena/resource_manager/reso...
athena/resource_manager/resource_manager_impl.cc:454:
next_resource_management_time_ = base::Time::Now();
On 2014/09/23 00:28:56, Mr4D wrote:
> Which would be ok! All which is required is that ResourcesGotReleased() is
> setting it to a later time. So no problem!

Just in case you might have misunderstood my comment. Now() in
AllowedToUnloadActivity may return earlier time than Now() here, so the next
call may not execute immediately. (It's extremely rare so this won't be an
issue, as I said)

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

Committed patchset #2 (id:20001) as df5725ed0f826f9d3ad5fcc10bca695f36e03ed6

[commit-bot: I haz the power, 2014-09-23 01:09:37]

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