Replace prepareToSuspend() with onMemoryStateChange(SUSPENDED)

Replace prepareToSuspend() with onMemoryStateChange(SUSPENDED)

To simplify the API.

BUG=617492
Committed: https://crrev.com/fa8801f66d8d2bba2cd293c01862fcacda898516
Cr-Commit-Position: refs/heads/master@{#428955}

[bashi, 2016-11-01 05:06:48]

The CQ bit was checked by bashi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 05:07:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bashi, 2016-11-01 05:07:28]

bashi@chromium.org changed reviewers:
+ haraken@chromium.org, tasak@google.com

[bashi, 2016-11-01 05:07:29]

Does this make sense to you guys?

[haraken, 2016-11-01 05:13:15]

I agree that we need this change, but would it make more sense to rewrite
prepareToSuspend() with onMemoryStateChange(SUSPENDED) (and make the purge +
suspend call onMemoryStateChange(SUSPENDED))?

In the future, I think all memory handling should be written in
onMemoryStateChange().

[bashi, 2016-11-01 05:32:23]

On 2016/11/01 05:13:15, haraken wrote:
> I agree that we need this change, but would it make more sense to rewrite
> prepareToSuspend() with onMemoryStateChange(SUSPENDED) (and make the purge +
> suspend call onMemoryStateChange(SUSPENDED))?
> 
> In the future, I think all memory handling should be written in
> onMemoryStateChange().

I see. I'll do that.

(That said I'm not a big fan of creating onMemoryStateChange() everywhere. I
feel it's over-abstracted and just messing up the code.)

[bashi, 2016-11-01 05:44:35]

Description was changed from

==========
Call prepareToSuspend() in onMemoryStateChange()

Blink's memory coordinator has prepareToSuspend(). It is exposed
for purge+suspend experiment[1], but we should also call it
in onMemoryStateChange()

[1] https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4

BUG=617492
==========

to

==========
Replace prepareToSuspend() with onMemoryStateChange(SUSPENDED)

To simplify the API.

BUG=617492
==========

[bashi, 2016-11-01 05:45:42]

The CQ bit was checked by bashi@chromium.org to run a CQ dry run

[bashi, 2016-11-01 05:45:46]

On 2016/11/01 05:32:23, bashi1 (slow) wrote:
> On 2016/11/01 05:13:15, haraken wrote:
> > I agree that we need this change, but would it make more sense to rewrite
> > prepareToSuspend() with onMemoryStateChange(SUSPENDED) (and make the purge +
> > suspend call onMemoryStateChange(SUSPENDED))?
> > 
> > In the future, I think all memory handling should be written in
> > onMemoryStateChange().
> 
> I see. I'll do that.
> 
> (That said I'm not a big fan of creating onMemoryStateChange() everywhere. I
> feel it's over-abstracted and just messing up the code.)

Done.

[commit-bot: I haz the power, 2016-11-01 05:45:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[haraken, 2016-11-01 06:21:29]

LGTM

https://codereview.chromium.org/2463283002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/MemoryCoordinator.cpp (right):

https://codereview.chromium.org/2463283002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/MemoryCoordinator.cpp:56: } else {

Why do we need the 'else'?

https://codereview.chromium.org/2463283002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebMemoryCoordinator.h (left):

https://codereview.chromium.org/2463283002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebMemoryCoordinator.h:24:
BLINK_PLATFORM_EXPORT static void prepareToSuspend();

Just to confirm: tasak@ has not yet landed a CL to use this API on the content
side, right?

[bashi, 2016-11-01 06:31:06]

https://codereview.chromium.org/2463283002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/MemoryCoordinator.cpp (right):

https://codereview.chromium.org/2463283002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/MemoryCoordinator.cpp:56: } else {
On 2016/11/01 06:21:29, haraken wrote:
> 
> Why do we need the 'else'?

Just avoid calling decommitFreeableMemory() twice. prepareToSuspend() called
decommitFreeableMemory(). It's gone anyway :)

https://codereview.chromium.org/2463283002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebMemoryCoordinator.h (left):

https://codereview.chromium.org/2463283002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebMemoryCoordinator.h:24:
BLINK_PLATFORM_EXPORT static void prepareToSuspend();
On 2016/11/01 06:21:29, haraken wrote:
> 
> Just to confirm: tasak@ has not yet landed a CL to use this API on the content
> side, right?

IIUC he won't call it. onMemoryStateChange() will be called via
MemoryCoordinatorClientRegistry::Notify().

[bashi, 2016-11-01 06:31:14]

The CQ bit was unchecked by bashi@chromium.org

[bashi, 2016-11-01 06:31:16]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2016-11-01 06:31:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-01 07:21:54]

Description was changed from

==========
Replace prepareToSuspend() with onMemoryStateChange(SUSPENDED)

To simplify the API.

BUG=617492
==========

to

==========
Replace prepareToSuspend() with onMemoryStateChange(SUSPENDED)

To simplify the API.

BUG=617492
==========

[commit-bot: I haz the power, 2016-11-01 07:21:55]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-01 07:24:22]

Description was changed from

==========
Replace prepareToSuspend() with onMemoryStateChange(SUSPENDED)

To simplify the API.

BUG=617492
==========

to

==========
Replace prepareToSuspend() with onMemoryStateChange(SUSPENDED)

To simplify the API.

BUG=617492

Committed: https://crrev.com/fa8801f66d8d2bba2cd293c01862fcacda898516
Cr-Commit-Position: refs/heads/master@{#428955}
==========

[commit-bot: I haz the power, 2016-11-01 07:24:23]

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