Rename OS-specific MemoryPressureMonitor implementations.

Rename OS-specific MemoryPressureMonitor implementations.

This is cleanup from https://codereview.chromium.org/1122863005/

BUG=472772
Committed: https://crrev.com/dc837f5038e15031c679cf30163b657bab27affa
Cr-Commit-Position: refs/heads/master@{#330032}

[chrisha, 2015-05-13 22:12:11]

chrisha@chromium.org changed reviewers:
+ grt@chromium.org, thakis@chromium.org

[chrisha, 2015-05-13 22:12:11]

PTAL?

[Nico, 2015-05-13 22:14:46]

https://codereview.chromium.org/1124163003/diff/1/base/chromeos/memory_pressu...
File base/chromeos/memory_pressure_monitor.h (right):

https://codereview.chromium.org/1124163003/diff/1/base/chromeos/memory_pressu...
base/chromeos/memory_pressure_monitor.h:29: class BASE_EXPORT
MemoryPressureMonitor : public MemoryPressureMonitor {
"class BASE_EXPORT MemoryPressureMonitor : public MemoryPressureMonitor"

?

[chrisha, 2015-05-13 23:04:25]

Silly me, I don't have Mac or ChromeOS Dev machines so couldn't test
compiles locally. Search and replace ftw! I'll re-ping tomorrow once the
trybots are coming back green.

On Wed, May 13, 2015, 18:14  <thakis@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1124163003/diff/1/base/chromeos/memory_pressu...
> File base/chromeos/memory_pressure_monitor.h (right):
>
>
>
https://codereview.chromium.org/1124163003/diff/1/base/chromeos/memory_pressu...
> base/chromeos/memory_pressure_monitor.h:29
>
<https://codereview.chromium.org/1124163003/diff/1/base/chromeos/memory_pressu...>:
> class BASE_EXPORT
> MemoryPressureMonitor : public MemoryPressureMonitor {
> "class BASE_EXPORT MemoryPressureMonitor : public MemoryPressureMonitor"
>
> ?
>
> https://codereview.chromium.org/1124163003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[grt (UTC plus 2), 2015-05-14 14:07:41]

https://codereview.chromium.org/1124163003/diff/60001/base/chromeos/memory_pr...
File base/chromeos/memory_pressure_monitor_unittest.cc (right):

https://codereview.chromium.org/1124163003/diff/60001/base/chromeos/memory_pr...
base/chromeos/memory_pressure_monitor_unittest.cc:48:
MemoryPressureMonitor::THRESHOLD_DEFAULT),
is "MemoryPressureMonitor::" needed?

https://codereview.chromium.org/1124163003/diff/60001/base/mac/memory_pressur...
File base/mac/memory_pressure_monitor.h (right):

https://codereview.chromium.org/1124163003/diff/60001/base/mac/memory_pressur...
base/mac/memory_pressure_monitor.h:5: #ifndef
BASE_MAC_MEMORY_MEMORY_PRESSURE_MONITOR_H_
is there an echo echo in here here? :-)

https://codereview.chromium.org/1124163003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/memory/oom_priority_manager.cc (right):

https://codereview.chromium.org/1124163003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/memory/oom_priority_manager.cc:515:
base::chromeos::MemoryPressureMonitor* monitor =
since you're editing the world, what do you think about putting a public static
Get() method on base::chromeos::MemoryPressureMonitor that does the casting so
that client code like this doesn't need to do it?

https://codereview.chromium.org/1124163003/diff/60001/chromeos/chromeos_switc...
File chromeos/chromeos_switches.cc (right):

https://codereview.chromium.org/1124163003/diff/60001/chromeos/chromeos_switc...
chromeos/chromeos_switches.cc:356: if
(!base::CommandLine::ForCurrentProcess()->HasSwitch(
nit: consider adding
  using MemoryPressureMonitor = base::chromeos::MemoryPressureMonitor;
above this line, and getting rid of all of the base::chromeos:: prefixes below.

[chrisha, 2015-05-14 15:27:18]

Okay... addressed grt@'s comments, and the trybots are now green. Another look?

https://codereview.chromium.org/1124163003/diff/60001/base/chromeos/memory_pr...
File base/chromeos/memory_pressure_monitor_unittest.cc (right):

https://codereview.chromium.org/1124163003/diff/60001/base/chromeos/memory_pr...
base/chromeos/memory_pressure_monitor_unittest.cc:48:
MemoryPressureMonitor::THRESHOLD_DEFAULT),
On 2015/05/14 14:07:40, grt wrote:
> is "MemoryPressureMonitor::" needed?

Done.

https://codereview.chromium.org/1124163003/diff/60001/base/mac/memory_pressur...
File base/mac/memory_pressure_monitor.h (right):

https://codereview.chromium.org/1124163003/diff/60001/base/mac/memory_pressur...
base/mac/memory_pressure_monitor.h:5: #ifndef
BASE_MAC_MEMORY_MEMORY_PRESSURE_MONITOR_H_
On 2015/05/14 14:07:40, grt wrote:
> is there an echo echo in here here? :-)

Done.

https://codereview.chromium.org/1124163003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/memory/oom_priority_manager.cc (right):

https://codereview.chromium.org/1124163003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/memory/oom_priority_manager.cc:515:
base::chromeos::MemoryPressureMonitor* monitor =
On 2015/05/14 14:07:40, grt wrote:
> since you're editing the world, what do you think about putting a public
static
> Get() method on base::chromeos::MemoryPressureMonitor that does the casting so
> that client code like this doesn't need to do it?

base::MemoryPressureMonitor::Get returns a pointer to the singleton
base::MemoryPressureMonitor. Because of the current derivation layout, this is
actually one of a base::win::MPM, base::chromeos::MPM or a base::mac::MPM.
Making the base class aware of the derived-types is a bit of an anti-pattern.

This is also the *only* the only place where the cast happens, and it does so
because it wants access to a function that is platform implementation specific.

My next cleanup in this code will be to actually rearrange things to a single
class with platform-specific implementations, and no derivation. This will
obviate the need for casting entirely. (This cleanup will hopefully happen once
I convince the memory-pressure owners that this is a good idea.)

So, leave it as is for now?

https://codereview.chromium.org/1124163003/diff/60001/chromeos/chromeos_switc...
File chromeos/chromeos_switches.cc (right):

https://codereview.chromium.org/1124163003/diff/60001/chromeos/chromeos_switc...
chromeos/chromeos_switches.cc:356: if
(!base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2015/05/14 14:07:40, grt wrote:
> nit: consider adding
>   using MemoryPressureMonitor = base::chromeos::MemoryPressureMonitor;
> above this line, and getting rid of all of the base::chromeos:: prefixes
below.

Done.

[grt (UTC plus 2), 2015-05-14 15:30:26]

https://codereview.chromium.org/1124163003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/memory/oom_priority_manager.cc (right):

https://codereview.chromium.org/1124163003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/memory/oom_priority_manager.cc:515:
base::chromeos::MemoryPressureMonitor* monitor =
On 2015/05/14 15:27:17, chrisha wrote:
> On 2015/05/14 14:07:40, grt wrote:
> > since you're editing the world, what do you think about putting a public
> static
> > Get() method on base::chromeos::MemoryPressureMonitor that does the casting
so
> > that client code like this doesn't need to do it?
> 
> base::MemoryPressureMonitor::Get returns a pointer to the singleton
> base::MemoryPressureMonitor. Because of the current derivation layout, this is
> actually one of a base::win::MPM, base::chromeos::MPM or a base::mac::MPM.
> Making the base class aware of the derived-types is a bit of an anti-pattern.

I was suggesting adding a public static Get() method to base::chromeos::MP that
does the exact call and cast that you find here. It amounts to the same thing
that's here now, but more friendly.

[grt (UTC plus 2), 2015-05-14 15:40:22]

https://codereview.chromium.org/1124163003/diff/80001/base/chromeos/memory_pr...
File base/chromeos/memory_pressure_monitor_unittest.cc (right):

https://codereview.chromium.org/1124163003/diff/80001/base/chromeos/memory_pr...
base/chromeos/memory_pressure_monitor_unittest.cc:47:
MemoryPressureMonitor(THRESHOLD_DEFAULT),
Now that this is less ugly, I see that it doesn't follow the style guid
(https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Constructor_...).
Please change it to:
  TestMemoryPressureMonitor()
      : MemoryPressureMonitor(THRESHOLD_DEFAULT),
        memory_in_percent_override_(0) {
    ...
  }

https://codereview.chromium.org/1124163003/diff/80001/chromeos/chromeos_switc...
File chromeos/chromeos_switches.cc (right):

https://codereview.chromium.org/1124163003/diff/80001/chromeos/chromeos_switc...
chromeos/chromeos_switches.cc:354: using MemoryPressureMonitor =
base::chromeos::MemoryPressureMonitor;
I think it's best to keep aliases like this either within the scope of a
function to limit their influence (i.e., move down to line 357) or put them at
the top of the .cc file (just after the #includes) so that it's clear that they
apply to the entire .cc file (rather than only what happens to lie below this
one line). Please pick one. In this case, I would lean toward moving it into the
function (yeah, this means the return value must be fully qualified).

[chrisha, 2015-05-14 19:46:47]

chrisha@chromium.org changed reviewers:
+ avi@chromium.org

[chrisha, 2015-05-14 19:53:15]

Alright, another round of nits addressed. Another look?

+avi for content/browser

https://codereview.chromium.org/1124163003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/memory/oom_priority_manager.cc (right):

https://codereview.chromium.org/1124163003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/memory/oom_priority_manager.cc:515:
base::chromeos::MemoryPressureMonitor* monitor =
On 2015/05/14 15:30:26, grt wrote:
> On 2015/05/14 15:27:17, chrisha wrote:
> > On 2015/05/14 14:07:40, grt wrote:
> > > since you're editing the world, what do you think about putting a public
> > static
> > > Get() method on base::chromeos::MemoryPressureMonitor that does the
casting
> so
> > > that client code like this doesn't need to do it?
> > 
> > base::MemoryPressureMonitor::Get returns a pointer to the singleton
> > base::MemoryPressureMonitor. Because of the current derivation layout, this
is
> > actually one of a base::win::MPM, base::chromeos::MPM or a base::mac::MPM.
> > Making the base class aware of the derived-types is a bit of an
anti-pattern.
> 
> I was suggesting adding a public static Get() method to base::chromeos::MP
that
> does the exact call and cast that you find here. It amounts to the same thing
> that's here now, but more friendly.

Well you know, my skull is pretty thick :)

Done.

https://codereview.chromium.org/1124163003/diff/80001/base/chromeos/memory_pr...
File base/chromeos/memory_pressure_monitor_unittest.cc (right):

https://codereview.chromium.org/1124163003/diff/80001/base/chromeos/memory_pr...
base/chromeos/memory_pressure_monitor_unittest.cc:47:
MemoryPressureMonitor(THRESHOLD_DEFAULT),
On 2015/05/14 15:40:21, grt wrote:
> Now that this is less ugly, I see that it doesn't follow the style guid
>
(https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Constructor_...).
> Please change it to:
>   TestMemoryPressureMonitor()
>       : MemoryPressureMonitor(THRESHOLD_DEFAULT),
>         memory_in_percent_override_(0) {
>     ...
>   }

Done.

https://codereview.chromium.org/1124163003/diff/80001/chromeos/chromeos_switc...
File chromeos/chromeos_switches.cc (right):

https://codereview.chromium.org/1124163003/diff/80001/chromeos/chromeos_switc...
chromeos/chromeos_switches.cc:354: using MemoryPressureMonitor =
base::chromeos::MemoryPressureMonitor;
On 2015/05/14 15:40:21, grt wrote:
> I think it's best to keep aliases like this either within the scope of a
> function to limit their influence (i.e., move down to line 357) or put them at
> the top of the .cc file (just after the #includes) so that it's clear that
they
> apply to the entire .cc file (rather than only what happens to lie below this
> one line). Please pick one. In this case, I would lean toward moving it into
the
> function (yeah, this means the return value must be fully qualified).

Done.

[grt (UTC plus 2), 2015-05-14 19:56:09]

thanks for taking care of the other cleanups i asked for. lgtm.

[Nico, 2015-05-14 20:03:37]

https://codereview.chromium.org/1124163003/diff/120001/base/chromeos/memory_p...
File base/chromeos/memory_pressure_monitor.h (right):

https://codereview.chromium.org/1124163003/diff/120001/base/chromeos/memory_p...
base/chromeos/memory_pressure_monitor.h:29: class BASE_EXPORT
MemoryPressureMonitor : public base::MemoryPressureMonitor {
Having the same class name in slightly different namespaces is super ugly imho.
This is as far as I know not one of the patterns we usually use.

Usual patterns are:
* Have a single class base::MemoryPressureMonitor, spread its implementation
over various files (xplat stuff in mpm.cc, then per-platform stuff in mpm_mac.cc
etc), e.g. launch.cc
* Have an interface class with per-os subclasses, give the interface a Create()
method that opaquely returns an instance of the subclass (e.g.
CreateWebContentsView() and WebContentsView + subclasses)
* A cross-platform function that then instantiated completely distinct types
behind the scenes (e.g. chrome::ShowTaskManager())

[chrisha, 2015-05-14 20:07:35]

On 2015/05/14 20:03:37, Nico wrote:
>
https://codereview.chromium.org/1124163003/diff/120001/base/chromeos/memory_p...
> File base/chromeos/memory_pressure_monitor.h (right):
> 
>
https://codereview.chromium.org/1124163003/diff/120001/base/chromeos/memory_p...
> base/chromeos/memory_pressure_monitor.h:29: class BASE_EXPORT
> MemoryPressureMonitor : public base::MemoryPressureMonitor {
> Having the same class name in slightly different namespaces is super ugly
imho.
> This is as far as I know not one of the patterns we usually use.
> 
> Usual patterns are:
> * Have a single class base::MemoryPressureMonitor, spread its implementation
> over various files (xplat stuff in mpm.cc, then per-platform stuff in
mpm_mac.cc
> etc), e.g. launch.cc
> * Have an interface class with per-os subclasses, give the interface a
Create()
> method that opaquely returns an instance of the subclass (e.g.
> CreateWebContentsView() and WebContentsView + subclasses)
> * A cross-platform function that then instantiated completely distinct types
> behind the scenes (e.g. chrome::ShowTaskManager())

And I'd opt for the first approach you've suggested, as I think it makes the
most sense here. This CL was just an attempt to make the existing world
consistent, as requested in https://codereview.chromium.org/1122863005/.

I think this should also be paired with a single static factory function that
abstracts away OS-dependent things. However, that should live in Chrome
somewhere, and not base, as it's dependent on the command-line, etc. My plan is
to all of this in a follow-on cleanup, once the memory-pressure owners are on
board.

[Nico, 2015-05-14 20:10:06]

On 2015/05/14 20:07:35, chrisha wrote:
> On 2015/05/14 20:03:37, Nico wrote:
> >
>
https://codereview.chromium.org/1124163003/diff/120001/base/chromeos/memory_p...
> > File base/chromeos/memory_pressure_monitor.h (right):
> > 
> >
>
https://codereview.chromium.org/1124163003/diff/120001/base/chromeos/memory_p...
> > base/chromeos/memory_pressure_monitor.h:29: class BASE_EXPORT
> > MemoryPressureMonitor : public base::MemoryPressureMonitor {
> > Having the same class name in slightly different namespaces is super ugly
> imho.
> > This is as far as I know not one of the patterns we usually use.
> > 
> > Usual patterns are:
> > * Have a single class base::MemoryPressureMonitor, spread its implementation
> > over various files (xplat stuff in mpm.cc, then per-platform stuff in
> mpm_mac.cc
> > etc), e.g. launch.cc
> > * Have an interface class with per-os subclasses, give the interface a
> Create()
> > method that opaquely returns an instance of the subclass (e.g.
> > CreateWebContentsView() and WebContentsView + subclasses)
> > * A cross-platform function that then instantiated completely distinct types
> > behind the scenes (e.g. chrome::ShowTaskManager())
> 
> And I'd opt for the first approach you've suggested, as I think it makes the
> most sense here. This CL was just an attempt to make the existing world
> consistent, as requested in https://codereview.chromium.org/1122863005/.
> 
> I think this should also be paired with a single static factory function that
> abstracts away OS-dependent things. However, that should live in Chrome
> somewhere, and not base, as it's dependent on the command-line, etc. My plan
is
> to all of this in a follow-on cleanup, once the memory-pressure owners are on
> board.

lgtm as an intermediate step. The other memory pressure owners (besides
yourself) are probably skuhne and shrike?

[Avi (use Gerrit), 2015-05-14 20:52:26]

lgtm

Just a rename? stampity stamp

[chrisha, 2015-05-14 21:26:08]

On 2015/05/14 20:10:06, Nico wrote:
> On 2015/05/14 20:07:35, chrisha wrote:
> > On 2015/05/14 20:03:37, Nico wrote:
> > >
> >
>
https://codereview.chromium.org/1124163003/diff/120001/base/chromeos/memory_p...
> > > File base/chromeos/memory_pressure_monitor.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1124163003/diff/120001/base/chromeos/memory_p...
> > > base/chromeos/memory_pressure_monitor.h:29: class BASE_EXPORT
> > > MemoryPressureMonitor : public base::MemoryPressureMonitor {
> > > Having the same class name in slightly different namespaces is super ugly
> > imho.
> > > This is as far as I know not one of the patterns we usually use.
> > > 
> > > Usual patterns are:
> > > * Have a single class base::MemoryPressureMonitor, spread its
implementation
> > > over various files (xplat stuff in mpm.cc, then per-platform stuff in
> > mpm_mac.cc
> > > etc), e.g. launch.cc
> > > * Have an interface class with per-os subclasses, give the interface a
> > Create()
> > > method that opaquely returns an instance of the subclass (e.g.
> > > CreateWebContentsView() and WebContentsView + subclasses)
> > > * A cross-platform function that then instantiated completely distinct
types
> > > behind the scenes (e.g. chrome::ShowTaskManager())
> > 
> > And I'd opt for the first approach you've suggested, as I think it makes the
> > most sense here. This CL was just an attempt to make the existing world
> > consistent, as requested in https://codereview.chromium.org/1122863005/.
> > 
> > I think this should also be paired with a single static factory function
that
> > abstracts away OS-dependent things. However, that should live in Chrome
> > somewhere, and not base, as it's dependent on the command-line, etc. My plan
> is
> > to all of this in a follow-on cleanup, once the memory-pressure owners are
on
> > board.
> 
> lgtm as an intermediate step. The other memory pressure owners (besides
> yourself) are probably skuhne and shrike?

Yes, those are the owners (mostly skuhne@). We have an ongoing discussion about
refactoring.

[chrisha, 2015-05-15 03:04:39]

The CQ bit was checked by chrisha@chromium.org

[commit-bot: I haz the power, 2015-05-15 03:05:05]

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

[commit-bot: I haz the power, 2015-05-15 03:09:15]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-05-15 03:10:02]

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

[tfarina, 2015-05-20 00:38:54]

https://codereview.chromium.org/1124163003/diff/120001/base/mac/memory_pressu...
File base/mac/memory_pressure_monitor.cc (right):

https://codereview.chromium.org/1124163003/diff/120001/base/mac/memory_pressu...
base/mac/memory_pressure_monitor.cc:5: #include
"base/mac/memory_pressure_monitor.h"
why did you do that?

I'm seeing this now:

/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/libtool:
warning same member name (base.memory_pressure_monitor.o) in output file used
for input files: obj/base/memory/base.memory_pressure_monitor.o and:
obj/base/mac/base.memory_pressure_monitor.o (due to use of basename, truncation,
blank padding or duplicate input files)

[chrisha, 2015-05-20 00:53:36]

It was a cleanup request from another CL, referenced in the description. I
wasn't aware of it causing any problems. There are other intended cleanups
that should get rid of this in the near future. If this is benign we can
wait for that, otherwise we can revert. Thoughts?

On Tue, May 19, 2015, 20:38  <tfarina@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1124163003/diff/120001/base/mac/memory_pressu...
> File base/mac/memory_pressure_monitor.cc (right):
>
>
>
https://codereview.chromium.org/1124163003/diff/120001/base/mac/memory_pressu...
> base/mac/memory_pressure_monitor.cc:5
>
<https://codereview.chromium.org/1124163003/diff/120001/base/mac/memory_pressu...>:
> #include
> "base/mac/memory_pressure_monitor.h"
> why did you do that?
>
> I'm seeing this now:
>
>
>
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/libtool:
> warning same member name (base.memory_pressure_monitor.o) in output file
> used for input files: obj/base/memory/base.memory_pressure_monitor.o
> and: obj/base/mac/base.memory_pressure_monitor.o (due to use of
> basename, truncation, blank padding or duplicate input files)
>
> https://codereview.chromium.org/1124163003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2015-05-21 19:37:41]

Gyp used to check for this, looks like someone removed it:
https://codereview.chromium.org/670063006/

But yeah, that's one of the reasons why we use the _win etc suffices. Maybe just
add that, like I suggested in the first place :-/