Add memory pressure dispatching callback to MemoryCoordinator

Add memory pressure dispatching callback to MemoryCoordinator

The callback will be used to route memory pressure notifications to
MemoryCoordinator instead of MemoryPressureListener. When MemoryCoordinator
receives memory pressure notifications, it translates them to memory state
events. Specifically, "moderate" -> "throttled" and "critical" -> "suspended".
This translation is a tentative workaround. When the priority tracker is
implemented we will replace the logic.

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

[bashi, 2016-07-14 08:37:43]

PTAL after [1]?

This depends on two CLs[1,2] so it might be too early to ask reviews but I'd
like to reduce turn-around time of review :)

[1] https://codereview.chromium.org/2144163002/
[2] https://codereview.chromium.org/2139363006/

[bashi, 2016-07-14 08:38:18]

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

[bashi, 2016-07-14 08:38:18]

(forgot to add reviewers...)

[chrisha, 2016-07-14 20:42:14]

lgtm!

[haraken, 2016-07-15 02:35:50]

https://codereview.chromium.org/2150783003/diff/1/components/memory_coordinat...
File components/memory_coordinator/browser/memory_coordinator.cc (right):

https://codereview.chromium.org/2150783003/diff/1/components/memory_coordinat...
components/memory_coordinator/browser/memory_coordinator.cc:70:
mojom::MemoryState::SUSPENDED);

Hmm, I'm not sure if this translation makes sense. Do we really want to actually
suspend the tab when receiving a critical signal?

[bashi, 2016-07-15 02:45:29]

https://codereview.chromium.org/2150783003/diff/1/components/memory_coordinat...
File components/memory_coordinator/browser/memory_coordinator.cc (right):

https://codereview.chromium.org/2150783003/diff/1/components/memory_coordinat...
components/memory_coordinator/browser/memory_coordinator.cc:70:
mojom::MemoryState::SUSPENDED);
On 2016/07/15 02:35:50, haraken wrote:
> 
> Hmm, I'm not sure if this translation makes sense. Do we really want to
actually
> suspend the tab when receiving a critical signal?

I discussed this in the design doc and if I remember correctly we discussed it
offline as well.
https://docs.google.com/document/d/1rGeUzhfzIKwmBzUcqUEwxEpLQQRq3PulRuHwFBlVI...

IIUC our conclusion is that we don't want to add tentative MC messages which
just means critical and moderate. This means that we need to translate them into
MC states. If you don't like the approach we need to add other MC states, which
I didn't expect you want to (but maybe I'm wrong).

[bashi, 2016-07-15 02:53:48]

On 2016/07/15 02:45:29, bashi1 wrote:
>
https://codereview.chromium.org/2150783003/diff/1/components/memory_coordinat...
> File components/memory_coordinator/browser/memory_coordinator.cc (right):
> 
>
https://codereview.chromium.org/2150783003/diff/1/components/memory_coordinat...
> components/memory_coordinator/browser/memory_coordinator.cc:70:
> mojom::MemoryState::SUSPENDED);
> On 2016/07/15 02:35:50, haraken wrote:
> > 
> > Hmm, I'm not sure if this translation makes sense. Do we really want to
> actually
> > suspend the tab when receiving a critical signal?
> 
> I discussed this in the design doc and if I remember correctly we discussed it
> offline as well.
>
https://docs.google.com/document/d/1rGeUzhfzIKwmBzUcqUEwxEpLQQRq3PulRuHwFBlVI...
> 
> IIUC our conclusion is that we don't want to add tentative MC messages which
> just means critical and moderate. This means that we need to translate them
into
> MC states. If you don't like the approach we need to add other MC states,
which
> I didn't expect you want to (but maybe I'm wrong).

...And I'd appreciate it if you give this kind of feedback when you review the
design doc:) I think one of the main purposes of a design doc is to discuss
"what we want to do" like this.

[haraken, 2016-07-15 04:32:12]

On 2016/07/15 02:53:48, bashi1 wrote:
> On 2016/07/15 02:45:29, bashi1 wrote:
> >
>
https://codereview.chromium.org/2150783003/diff/1/components/memory_coordinat...
> > File components/memory_coordinator/browser/memory_coordinator.cc (right):
> > 
> >
>
https://codereview.chromium.org/2150783003/diff/1/components/memory_coordinat...
> > components/memory_coordinator/browser/memory_coordinator.cc:70:
> > mojom::MemoryState::SUSPENDED);
> > On 2016/07/15 02:35:50, haraken wrote:
> > > 
> > > Hmm, I'm not sure if this translation makes sense. Do we really want to
> > actually
> > > suspend the tab when receiving a critical signal?
> > 
> > I discussed this in the design doc and if I remember correctly we discussed
it
> > offline as well.
> >
>
https://docs.google.com/document/d/1rGeUzhfzIKwmBzUcqUEwxEpLQQRq3PulRuHwFBlVI...
> > 
> > IIUC our conclusion is that we don't want to add tentative MC messages which
> > just means critical and moderate. This means that we need to translate them
> into
> > MC states. If you don't like the approach we need to add other MC states,
> which
> > I didn't expect you want to (but maybe I'm wrong).
> 
> ...And I'd appreciate it if you give this kind of feedback when you review the
> design doc:) I think one of the main purposes of a design doc is to discuss
> "what we want to do" like this.

Yeah, sorry. I was slightly changing my mind while discussing the risk of the
purge + suspend. We're now discussing a way to minimize # of tabs that get
suspended.

LGTM to experiment with this CL. But given the risk of purge + suspend, I'm not
sure if we can ship the CRITICAL => SUSPEND mapping to a Finch experiment.

[bashi, 2016-07-15 05:05:41]

On 2016/07/15 04:32:12, haraken wrote:
> On 2016/07/15 02:53:48, bashi1 wrote:
> > On 2016/07/15 02:45:29, bashi1 wrote:
> > >
> >
>
https://codereview.chromium.org/2150783003/diff/1/components/memory_coordinat...
> > > File components/memory_coordinator/browser/memory_coordinator.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2150783003/diff/1/components/memory_coordinat...
> > > components/memory_coordinator/browser/memory_coordinator.cc:70:
> > > mojom::MemoryState::SUSPENDED);
> > > On 2016/07/15 02:35:50, haraken wrote:
> > > > 
> > > > Hmm, I'm not sure if this translation makes sense. Do we really want to
> > > actually
> > > > suspend the tab when receiving a critical signal?
> > > 
> > > I discussed this in the design doc and if I remember correctly we
discussed
> it
> > > offline as well.
> > >
> >
>
https://docs.google.com/document/d/1rGeUzhfzIKwmBzUcqUEwxEpLQQRq3PulRuHwFBlVI...
> > > 
> > > IIUC our conclusion is that we don't want to add tentative MC messages
which
> > > just means critical and moderate. This means that we need to translate
them
> > into
> > > MC states. If you don't like the approach we need to add other MC states,
> > which
> > > I didn't expect you want to (but maybe I'm wrong).
> > 
> > ...And I'd appreciate it if you give this kind of feedback when you review
the
> > design doc:) I think one of the main purposes of a design doc is to discuss
> > "what we want to do" like this.
> 
> Yeah, sorry. I was slightly changing my mind while discussing the risk of the
> purge + suspend. We're now discussing a way to minimize # of tabs that get
> suspended.
> 
> LGTM to experiment with this CL. But given the risk of purge + suspend, I'm
not
> sure if we can ship the CRITICAL => SUSPEND mapping to a Finch experiment.

To be clear, SUSPENDED wouldn't mean actual suspension in V0. It will be more
like "purge memory as much as possible". I agree that this translation is
confusing, but the reason I'm doing that is that I thought you didn't want to
add a tentative state or method to MC (as you commented on the first draft of
MCv0[1]). I'm happy to add a tentative state/method if it seems correct. WDYT?

[1]
https://docs.google.com/document/d/1Ibja1-0odRBEojGxwiHu7WseAGq41pNLY1610NF6m...

[haraken, 2016-07-15 05:19:32]

On 2016/07/15 05:05:41, bashi1 wrote:
> On 2016/07/15 04:32:12, haraken wrote:
> > On 2016/07/15 02:53:48, bashi1 wrote:
> > > On 2016/07/15 02:45:29, bashi1 wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2150783003/diff/1/components/memory_coordinat...
> > > > File components/memory_coordinator/browser/memory_coordinator.cc
(right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2150783003/diff/1/components/memory_coordinat...
> > > > components/memory_coordinator/browser/memory_coordinator.cc:70:
> > > > mojom::MemoryState::SUSPENDED);
> > > > On 2016/07/15 02:35:50, haraken wrote:
> > > > > 
> > > > > Hmm, I'm not sure if this translation makes sense. Do we really want
to
> > > > actually
> > > > > suspend the tab when receiving a critical signal?
> > > > 
> > > > I discussed this in the design doc and if I remember correctly we
> discussed
> > it
> > > > offline as well.
> > > >
> > >
> >
>
https://docs.google.com/document/d/1rGeUzhfzIKwmBzUcqUEwxEpLQQRq3PulRuHwFBlVI...
> > > > 
> > > > IIUC our conclusion is that we don't want to add tentative MC messages
> which
> > > > just means critical and moderate. This means that we need to translate
> them
> > > into
> > > > MC states. If you don't like the approach we need to add other MC
states,
> > > which
> > > > I didn't expect you want to (but maybe I'm wrong).
> > > 
> > > ...And I'd appreciate it if you give this kind of feedback when you review
> the
> > > design doc:) I think one of the main purposes of a design doc is to
discuss
> > > "what we want to do" like this.
> > 
> > Yeah, sorry. I was slightly changing my mind while discussing the risk of
the
> > purge + suspend. We're now discussing a way to minimize # of tabs that get
> > suspended.
> > 
> > LGTM to experiment with this CL. But given the risk of purge + suspend, I'm
> not
> > sure if we can ship the CRITICAL => SUSPEND mapping to a Finch experiment.
> 
> To be clear, SUSPENDED wouldn't mean actual suspension in V0. It will be more
> like "purge memory as much as possible".

Makes sense.

> I agree that this translation is
> confusing, but the reason I'm doing that is that I thought you didn't want to
> add a tentative state or method to MC (as you commented on the first draft of
> MCv0[1]). I'm happy to add a tentative state/method if it seems correct. WDYT?
> 
> [1]
>
https://docs.google.com/document/d/1Ibja1-0odRBEojGxwiHu7WseAGq41pNLY1610NF6m...

Yeah. Honestly speaking, I'm not sure what we should do in the transitive
period. Maybe it would make more sense to just map both MODERATE and CRITICAL to
THROTTLE, but it might not be enough to reduce as much memory as we do today. Or
maybe we won't need to ship the transitive MC to a Finch experiment if
PriorityTracker is available in the near future. So, I'm fine with landing this
CL and iterating.

[bashi, 2016-07-15 05:34:43]

On 2016/07/15 05:19:32, haraken wrote:
> On 2016/07/15 05:05:41, bashi1 wrote:
> > On 2016/07/15 04:32:12, haraken wrote:
> > > On 2016/07/15 02:53:48, bashi1 wrote:
> > > > On 2016/07/15 02:45:29, bashi1 wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2150783003/diff/1/components/memory_coordinat...
> > > > > File components/memory_coordinator/browser/memory_coordinator.cc
> (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2150783003/diff/1/components/memory_coordinat...
> > > > > components/memory_coordinator/browser/memory_coordinator.cc:70:
> > > > > mojom::MemoryState::SUSPENDED);
> > > > > On 2016/07/15 02:35:50, haraken wrote:
> > > > > > 
> > > > > > Hmm, I'm not sure if this translation makes sense. Do we really want
> to
> > > > > actually
> > > > > > suspend the tab when receiving a critical signal?
> > > > > 
> > > > > I discussed this in the design doc and if I remember correctly we
> > discussed
> > > it
> > > > > offline as well.
> > > > >
> > > >
> > >
> >
>
https://docs.google.com/document/d/1rGeUzhfzIKwmBzUcqUEwxEpLQQRq3PulRuHwFBlVI...
> > > > > 
> > > > > IIUC our conclusion is that we don't want to add tentative MC messages
> > which
> > > > > just means critical and moderate. This means that we need to translate
> > them
> > > > into
> > > > > MC states. If you don't like the approach we need to add other MC
> states,
> > > > which
> > > > > I didn't expect you want to (but maybe I'm wrong).
> > > > 
> > > > ...And I'd appreciate it if you give this kind of feedback when you
review
> > the
> > > > design doc:) I think one of the main purposes of a design doc is to
> discuss
> > > > "what we want to do" like this.
> > > 
> > > Yeah, sorry. I was slightly changing my mind while discussing the risk of
> the
> > > purge + suspend. We're now discussing a way to minimize # of tabs that get
> > > suspended.
> > > 
> > > LGTM to experiment with this CL. But given the risk of purge + suspend,
I'm
> > not
> > > sure if we can ship the CRITICAL => SUSPEND mapping to a Finch experiment.
> > 
> > To be clear, SUSPENDED wouldn't mean actual suspension in V0. It will be
more
> > like "purge memory as much as possible".
> 
> Makes sense.
> 
> > I agree that this translation is
> > confusing, but the reason I'm doing that is that I thought you didn't want
to
> > add a tentative state or method to MC (as you commented on the first draft
of
> > MCv0[1]). I'm happy to add a tentative state/method if it seems correct.
WDYT?
> > 
> > [1]
> >
>
https://docs.google.com/document/d/1Ibja1-0odRBEojGxwiHu7WseAGq41pNLY1610NF6m...
> 
> Yeah. Honestly speaking, I'm not sure what we should do in the transitive
> period. Maybe it would make more sense to just map both MODERATE and CRITICAL
to
> THROTTLE, but it might not be enough to reduce as much memory as we do today.
Or
> maybe we won't need to ship the transitive MC to a Finch experiment if
> PriorityTracker is available in the near future. So, I'm fine with landing
this
> CL and iterating.

Thanks. Let's keep iteration :) I'll land this after all required CLs are
landed.

BTW, I'm a bit surprised what was in your mind. By "ship MCv0 to finch
experiment" you meant that we'll run an experiment with a configuration which
replaces MemoryPressureListener with MemoryCoordinatorClient. Based on the
assumption I wrote "Success criteria" section in the design document. Maybe I'm
misunderstanding :(

[bashi, 2016-07-21 01:07:43]

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

[commit-bot: I haz the power, 2016-07-21 01:08:10]

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-07-21 01:43:53]

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

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

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

[bashi, 2016-07-21 02:54:55]

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

[commit-bot: I haz the power, 2016-07-21 02:55: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-07-21 03:23:22]

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

[commit-bot: I haz the power, 2016-07-21 03:23:23]

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

[bashi, 2016-07-21 04:35:23]

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

[commit-bot: I haz the power, 2016-07-21 04:35:30]

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-07-21 05:20:08]

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

[commit-bot: I haz the power, 2016-07-21 05:20:10]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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-07-21 08:27:45]

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

[commit-bot: I haz the power, 2016-07-21 08:28:00]

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-07-21 08:57:39]

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

[commit-bot: I haz the power, 2016-07-21 08:57:40]

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

[bashi, 2016-07-21 23:18:58]

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

[commit-bot: I haz the power, 2016-07-21 23:19:49]

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-07-21 23:52:48]

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

[commit-bot: I haz the power, 2016-07-21 23:52:49]

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

[bashi, 2016-07-22 01:00:22]

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

[commit-bot: I haz the power, 2016-07-22 01:00:57]

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

[bashi, 2016-07-22 01:50:19]

bashi@chromium.org changed reviewers:
+ caitkp@chromium.org, dcheng@chromium.org

[bashi, 2016-07-22 01:50:20]

caitkp@, could you review components/BUILD.gn?
dcheng@, could you review *.mojom?

[commit-bot: I haz the power, 2016-07-22 02:25:25]

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

[commit-bot: I haz the power, 2016-07-22 02:25:26]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-07-22 04:49:03]

https://codereview.chromium.org/2150783003/diff/80001/components/memory_coord...
File components/memory_coordinator/browser/memory_coordinator.cc (right):

https://codereview.chromium.org/2150783003/diff/80001/components/memory_coord...
components/memory_coordinator/browser/memory_coordinator.cc:37:
base::Unretained(this))) {}
Can you explain why this is safe? I only see this getting used by the tests, so
maybe the tests should just bind the callback? Or just friend the test so it can
bind the callback itself?

[bashi, 2016-07-22 04:59:44]

Thanks for review!

https://codereview.chromium.org/2150783003/diff/80001/components/memory_coord...
File components/memory_coordinator/browser/memory_coordinator.cc (right):

https://codereview.chromium.org/2150783003/diff/80001/components/memory_coord...
components/memory_coordinator/browser/memory_coordinator.cc:37:
base::Unretained(this))) {}
On 2016/07/22 04:49:03, dcheng wrote:
> Can you explain why this is safe? I only see this getting used by the tests,
so
> maybe the tests should just bind the callback? Or just friend the test so it
can
> bind the callback itself?

Is your concern the lifetime of MemoryCoordinator? (exposing callback without
retaining?) It is owned by BrowserMainLoop so I thought it's OK but it may not
be OK (I'm not sure).

I have a follow-up CL which uses the callback so I exposed it.
https://codereview.chromium.org/2172893003

[dcheng, 2016-07-22 05:02:15]

On 2016/07/22 04:59:44, bashi1 wrote:
> Thanks for review!
> 
>
https://codereview.chromium.org/2150783003/diff/80001/components/memory_coord...
> File components/memory_coordinator/browser/memory_coordinator.cc (right):
> 
>
https://codereview.chromium.org/2150783003/diff/80001/components/memory_coord...
> components/memory_coordinator/browser/memory_coordinator.cc:37:
> base::Unretained(this))) {}
> On 2016/07/22 04:49:03, dcheng wrote:
> > Can you explain why this is safe? I only see this getting used by the tests,
> so
> > maybe the tests should just bind the callback? Or just friend the test so it
> can
> > bind the callback itself?
> 
> Is your concern the lifetime of MemoryCoordinator? (exposing callback without
> retaining?) It is owned by BrowserMainLoop so I thought it's OK but it may not
> be OK (I'm not sure).
> 
> I have a follow-up CL which uses the callback so I exposed it.
> https://codereview.chromium.org/2172893003

My concern is that it's not clear when this will be safe: because it's exposed
in a getter, anyone can get a reference to the callback and keep it live, but
the stored object pointer is unretained.

[bashi, 2016-07-22 05:50:38]

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

[commit-bot: I haz the power, 2016-07-22 05:51:02]

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

[bashi, 2016-07-22 05:52:18]

On 2016/07/22 05:02:15, dcheng wrote:
> On 2016/07/22 04:59:44, bashi1 wrote:
> > Thanks for review!
> > 
> >
>
https://codereview.chromium.org/2150783003/diff/80001/components/memory_coord...
> > File components/memory_coordinator/browser/memory_coordinator.cc (right):
> > 
> >
>
https://codereview.chromium.org/2150783003/diff/80001/components/memory_coord...
> > components/memory_coordinator/browser/memory_coordinator.cc:37:
> > base::Unretained(this))) {}
> > On 2016/07/22 04:49:03, dcheng wrote:
> > > Can you explain why this is safe? I only see this getting used by the
tests,
> > so
> > > maybe the tests should just bind the callback? Or just friend the test so
it
> > can
> > > bind the callback itself?
> > 
> > Is your concern the lifetime of MemoryCoordinator? (exposing callback
without
> > retaining?) It is owned by BrowserMainLoop so I thought it's OK but it may
not
> > be OK (I'm not sure).
> > 
> > I have a follow-up CL which uses the callback so I exposed it.
> > https://codereview.chromium.org/2172893003
> 
> My concern is that it's not clear when this will be safe: because it's exposed
> in a getter, anyone can get a reference to the callback and keep it live, but
> the stored object pointer is unretained.

I didn't aware of that. Updated the CL not to expose the callback.

[dcheng, 2016-07-22 06:04:34]

The mojom changes LGTM, but the //base interface seems a little weird. We can
iterate on that in a followup CL though.

https://codereview.chromium.org/2150783003/diff/100001/components/memory_coor...
File components/memory_coordinator/browser/memory_coordinator.cc (right):

https://codereview.chromium.org/2150783003/diff/100001/components/memory_coor...
components/memory_coordinator/browser/memory_coordinator.cc:50:
base::Bind(&base::MemoryPressureListener::NotifyMemoryPressure));
What does the //base version of this do?

It seems a little unusual that a class outside of //base has to reinstall the
original listener hook. It seems harder to use correctly this way.

[commit-bot: I haz the power, 2016-07-22 06:47:16]

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

[commit-bot: I haz the power, 2016-07-22 06:47:17]

Dry run: This issue passed the CQ dry run.

[bashi, 2016-07-22 07:10:12]

https://codereview.chromium.org/2150783003/diff/100001/components/memory_coor...
File components/memory_coordinator/browser/memory_coordinator.cc (right):

https://codereview.chromium.org/2150783003/diff/100001/components/memory_coor...
components/memory_coordinator/browser/memory_coordinator.cc:50:
base::Bind(&base::MemoryPressureListener::NotifyMemoryPressure));
On 2016/07/22 06:04:34, dcheng wrote:
> What does the //base version of this do?
> 
> It seems a little unusual that a class outside of //base has to reinstall the
> original listener hook. It seems harder to use correctly this way.

You are right. Thinking this a bit more, I feel that it might be better to have
a MemoryPressureListener in this class instead of hijacking the original hook.
Let me try to land this first. I'll revisit this later.

[bashi, 2016-07-25 00:30:12]

bashi@chromium.org changed reviewers:
+ blundell@chromium.org
- caitkp@chromium.org

[bashi, 2016-07-25 00:30:13]

-caitkp@, +blundell@ as caitkp seems busy.

blundell@, could you review components/BUILD.gn ?

[blundell, 2016-07-25 05:31:38]

lgtm

[bashi, 2016-07-25 09:18:06]

The CQ bit was checked by bashi@chromium.org

[bashi, 2016-07-25 09:18:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, chrisha@chromium.org, dcheng@chromium.org,
blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/2150783003/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2016-07-25 09:18:19]

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-07-25 10:22:25]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-07-25 10:24:58]

Description was changed from

==========
Add memory pressure dispatching callback to MemoryCoordinator

The callback will be used to route memory pressure notifications to
MemoryCoordinator instead of MemoryPressureListener. When MemoryCoordinator
receives memory pressure notifications, it translates them to memory state
events. Specifically, "moderate" -> "throttled" and "critical" -> "suspended".
This translation is a tentative workaround. When the priority tracker is
implemented we will replace the logic.

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

to

==========
Add memory pressure dispatching callback to MemoryCoordinator

The callback will be used to route memory pressure notifications to
MemoryCoordinator instead of MemoryPressureListener. When MemoryCoordinator
receives memory pressure notifications, it translates them to memory state
events. Specifically, "moderate" -> "throttled" and "critical" -> "suspended".
This translation is a tentative workaround. When the priority tracker is
implemented we will replace the logic.

BUG=617492

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

[commit-bot: I haz the power, 2016-07-25 10:24:59]

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