Create MemoryMonitor and Windows implementation.

Create MemoryMonitor and Windows implementation.

This defines the interface for the memory monitoring component of the MemoryCoordinator, and provides an implementation on Windows.

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

[chrisha, 2016-08-11 14:33:02]

chrisha@chromium.org changed reviewers:
+ bashi@chromium.org, georgesak@chromium.org, haraken@chromium.org

[chrisha, 2016-08-11 14:33:02]

haraken/bashi: FYI.
georgesak: PTAL?

[chrisha, 2016-08-11 15:43:59]

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

[commit-bot: I haz the power, 2016-08-11 15:44:18]

Dry run: 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-08-11 15:50:07]

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

[commit-bot: I haz the power, 2016-08-11 15:50:08]

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

[chrisha, 2016-08-11 16:55:58]

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

[commit-bot: I haz the power, 2016-08-11 16:56:22]

Dry run: 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-08-11 20:12:52]

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

[commit-bot: I haz the power, 2016-08-11 20:12:53]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[bashi, 2016-08-11 22:03:12]

https://codereview.chromium.org/2236983002/diff/40001/components/memory_coord...
File components/memory_coordinator/browser/BUILD.gn (right):

https://codereview.chromium.org/2236983002/diff/40001/components/memory_coord...
components/memory_coordinator/browser/BUILD.gn:10: "memory_monitor_win.cc",
Should we have "if (is_win) { ... }" block?

https://codereview.chromium.org/2236983002/diff/40001/components/memory_coord...
File components/memory_coordinator/browser/memory_monitor.h (right):

https://codereview.chromium.org/2236983002/diff/40001/components/memory_coord...
components/memory_coordinator/browser/memory_monitor.h:26: virtual int
GetFreeMemoryUntilCriticalMB() = 0;
(just a question)
Do we have specific plans for implementing this method on other platforms,
especially on Android?

https://codereview.chromium.org/2236983002/diff/40001/components/memory_coord...
File components/memory_coordinator/browser/memory_monitor_win.cc (right):

https://codereview.chromium.org/2236983002/diff/40001/components/memory_coord...
components/memory_coordinator/browser/memory_monitor_win.cc:91: //
Implementation of factory function defined in memory_monitor.h.
Put this in a separate file (memory_monitor.cc) ?

[haraken, 2016-08-12 06:53:15]

Looks good on my side.

https://codereview.chromium.org/2236983002/diff/40001/components/memory_coord...
File components/memory_coordinator/browser/memory_monitor_win.cc (right):

https://codereview.chromium.org/2236983002/diff/40001/components/memory_coord...
components/memory_coordinator/browser/memory_monitor_win.cc:45: // This is the
the target free memory used for systems with < ~2GB of physical

the the

https://codereview.chromium.org/2236983002/diff/40001/components/memory_coord...
File components/memory_coordinator/browser/memory_monitor_win.h (right):

https://codereview.chromium.org/2236983002/diff/40001/components/memory_coord...
components/memory_coordinator/browser/memory_monitor_win.h:46: static bool
IsLargeMemory(MemoryMonitorWinDelegate* delegate);

Add a "ForTesting" suffic for testing methods.

https://codereview.chromium.org/2236983002/diff/40001/components/memory_coord...
components/memory_coordinator/browser/memory_monitor_win.h:49: static int
GetTargetFreeMB(MemoryMonitorWinDelegate* delegate);

Ditto.

[chrisha, 2016-08-12 20:13:43]

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

[chrisha, 2016-08-12 20:14:00]

Thanks for the comments. Another quick look?

https://codereview.chromium.org/2236983002/diff/40001/components/memory_coord...
File components/memory_coordinator/browser/BUILD.gn (right):

https://codereview.chromium.org/2236983002/diff/40001/components/memory_coord...
components/memory_coordinator/browser/BUILD.gn:10: "memory_monitor_win.cc",
On 2016/08/11 22:03:12, bashi1 (slow til 16 Aug) wrote:
> Should we have "if (is_win) { ... }" block?

No need, as _win (and _linux, _chromeos, _mac, etc) files are automatically
added to only platform specific builds. This is the standard way it appears to
be done in GN.

https://cs.chromium.org/search/?q=file:BUILD.gn+_win.cc&sq=package:chromium&t...

https://codereview.chromium.org/2236983002/diff/40001/components/memory_coord...
File components/memory_coordinator/browser/memory_monitor.h (right):

https://codereview.chromium.org/2236983002/diff/40001/components/memory_coord...
components/memory_coordinator/browser/memory_monitor.h:26: virtual int
GetFreeMemoryUntilCriticalMB() = 0;
On 2016/08/11 22:03:12, bashi1 (slow til 16 Aug) wrote:
> (just a question)
> Do we have specific plans for implementing this method on other platforms,
> especially on Android?

Yes. I'm starting with desktop because that's where I'm most comfortable, but
the plan is to do something similar on Android.

https://codereview.chromium.org/2236983002/diff/40001/components/memory_coord...
File components/memory_coordinator/browser/memory_monitor_win.cc (right):

https://codereview.chromium.org/2236983002/diff/40001/components/memory_coord...
components/memory_coordinator/browser/memory_monitor_win.cc:45: // This is the
the target free memory used for systems with < ~2GB of physical
On 2016/08/12 06:53:15, haraken wrote:
> 
> the the

Done.

https://codereview.chromium.org/2236983002/diff/40001/components/memory_coord...
components/memory_coordinator/browser/memory_monitor_win.cc:91: //
Implementation of factory function defined in memory_monitor.h.
On 2016/08/11 22:03:12, bashi1 (slow til 16 Aug) wrote:
> Put this in a separate file (memory_monitor.cc) ?

This is a platform dependent factory function. Moving the implementations to a
cross-platform .cc will require a lot of ugly precompiler definitions. I think
it's cleaner this way. YMMV.

https://codereview.chromium.org/2236983002/diff/40001/components/memory_coord...
File components/memory_coordinator/browser/memory_monitor_win.h (right):

https://codereview.chromium.org/2236983002/diff/40001/components/memory_coord...
components/memory_coordinator/browser/memory_monitor_win.h:46: static bool
IsLargeMemory(MemoryMonitorWinDelegate* delegate);
On 2016/08/12 06:53:15, haraken wrote:
> 
> Add a "ForTesting" suffic for testing methods.

These are exposed for testing, but they're not "test only" functions (ie, they
are used in production logic). Reworded the comment.

https://codereview.chromium.org/2236983002/diff/40001/components/memory_coord...
components/memory_coordinator/browser/memory_monitor_win.h:49: static int
GetTargetFreeMB(MemoryMonitorWinDelegate* delegate);
On 2016/08/12 06:53:15, haraken wrote:
> 
> Ditto.

Ditto.

[commit-bot: I haz the power, 2016-08-12 20:14:13]

Dry run: 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-08-12 20:38:10]

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

[commit-bot: I haz the power, 2016-08-12 20:38:10]

Dry run: Exceeded global retry quota

[bashi, 2016-08-15 23:29:41]

lgtm on my side.

https://codereview.chromium.org/2236983002/diff/40001/components/memory_coord...
File components/memory_coordinator/browser/BUILD.gn (right):

https://codereview.chromium.org/2236983002/diff/40001/components/memory_coord...
components/memory_coordinator/browser/BUILD.gn:10: "memory_monitor_win.cc",
On 2016/08/12 20:14:00, chrisha (slow) wrote:
> On 2016/08/11 22:03:12, bashi1 (slow til 16 Aug) wrote:
> > Should we have "if (is_win) { ... }" block?
> 
> No need, as _win (and _linux, _chromeos, _mac, etc) files are automatically
> added to only platform specific builds. This is the standard way it appears to
> be done in GN.
> 
>
https://cs.chromium.org/search/?q=file:BUILD.gn+_win.cc&sq=package:chromium&t...

I didn't know that. Thanks for the explanation.

https://codereview.chromium.org/2236983002/diff/60001/components/memory_coord...
File components/memory_coordinator/browser/memory_monitor_win.cc (right):

https://codereview.chromium.org/2236983002/diff/60001/components/memory_coord...
components/memory_coordinator/browser/memory_monitor_win.cc:20: class
MemoryMonitorWinDelegateImpl : public MemoryMonitorWinDelegate {
(Just comment)
Maybe this delegate can be platform independent? When we add other platform
supports, we may want to use this delegation.

[chrisha, 2016-08-17 18:52:36]

https://codereview.chromium.org/2236983002/diff/60001/components/memory_coord...
File components/memory_coordinator/browser/memory_monitor_win.cc (right):

https://codereview.chromium.org/2236983002/diff/60001/components/memory_coord...
components/memory_coordinator/browser/memory_monitor_win.cc:20: class
MemoryMonitorWinDelegateImpl : public MemoryMonitorWinDelegate {
On 2016/08/15 23:29:40, bashi1 (slow til 16 Aug) wrote:
> (Just comment)
> Maybe this delegate can be platform independent? When we add other platform
> supports, we may want to use this delegation.

Yeah, it might want to be. I'll leave it Win only for now, and move it if needed
on another platform.

[chrisha, 2016-08-17 18:52:43]

The CQ bit was checked by chrisha@chromium.org

[commit-bot: I haz the power, 2016-08-17 18:53:10]

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-08-17 20:12:53]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-17 20:21:17]

Description was changed from

==========
Create MemoryMonitor and Windows implementation.

This defines the interface for the memory monitoring component of the
MemoryCoordinator, and provides an implementation on Windows.

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

to

==========
Create MemoryMonitor and Windows implementation.

This defines the interface for the memory monitoring component of the
MemoryCoordinator, and provides an implementation on Windows.

BUG=617492

Committed: https://crrev.com/1be61ae09f6df6838443c5e260923917cb4be87c
Cr-Commit-Position: refs/heads/master@{#412627}
==========

[commit-bot: I haz the power, 2016-08-17 20:21:19]

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