Make HttpNetworkSession a client of memory coordinator

Make HttpNetworkSession a client of memory coordinator

When the memory coordinator is enabled, HttpNetworkSession becomes a
client of the memory coordinator instead of installing
MemoryPressureListener.

In the implmentation of this CL, both MemoryPressureListener and
MemoryCoordinatorClient are installed regardless of the flag, but as
default MemoryCoordinator is not enabled and when MemoryCoordinator is
enabled by a flag, we plan to suppress MemoryPressureMonitor features.

Design Doc: https://docs.google.com/document/d/1a69mMr7jI7qK0OfKNlrZ350xiXizVMCCe8orGX7K8Uo/edit?ts=57d7968b#

BUG=639700
Committed: https://crrev.com/8156e7c5f7b94c717ccc4a97eb1dc09f8e6723a6
Cr-Commit-Position: refs/heads/master@{#421761}

[hajimehoshi, 2016-09-23 10:03:23]

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

[commit-bot: I haz the power, 2016-09-23 10:03:50]

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

[hajimehoshi, 2016-09-23 10:04:54]

hajimehoshi@chromium.org changed reviewers:
+ bashi@chromium.org, chrisha@chromium.org, rsleevi@chromium.org,
tasak@chromium.org

[hajimehoshi, 2016-09-23 10:04:54]

PTAL

[commit-bot: I haz the power, 2016-09-23 10:42:29]

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

[commit-bot: I haz the power, 2016-09-23 10:42:30]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Ryan Sleevi, 2016-09-23 14:50:19]

TL;DR: I'm sorry that I've written more feedback than this CL, but I'm not
comfortable with this CL landing, but for reasons that can all hopefully be
easily resolved.

The pattern being promoted here:
base::MemoryCoordinatorClientRegistry::GetInstance()->Register(this)
leaves me with a sense of unease and discomfort - that is, a feeling that it's
not right and will cause trouble. 

I've tried to read through the code related to MemoryCoordinatorClientRegistry,
and I can't find much (or any) documentation in the headers about the threading
guarantees (and find much of the classes under-documented for something that
lives in //base).

For example, //base/memory/memory_coordinator_client_registry.h doesn't document
any threading guarantees about how clients are invoked. However, those threading
guarantees are 'obvious' because the implementation detail (namely, clients())
has leaked into the public interface. Exposing an ObserverListThreadSafe
publicly makes me very uncomfortable - I believe it's a dangerous coding
pattern, much like exposing a WeakPtr/WeakPtrFactory publicly, because it means
you can no longer make guarantees about the invariants of when or how an object
is called. This makes it difficult, in this CL, to evaluate whether there are
threading or lifetime risks when you use the API like this.

Similarly, ///base/memory/memory_coordinator_client.h doesn't document any
threading or timing guarantees. For example, is OnMemoryStateChange potentially
going to flap? Are there guarantees of quiescence or frequency of calls? I don't
know, and so it's difficult to evaluate, in this CL, whether there are any
performance sensitivities this implementation needs to be aware of.

I'm also uncomfortable because it seems there's no //base memory coordinator
(and perhaps that's coming), but it makes it hard to actually look at the
implementation to see how all the pieces fit together. The conclusions from the
doc make me think that this is not something that's really been evaluated in the
context of //base, but instead in //components. Would the design-docs review
mailing list have helped in calling out this somewhat surprising inconsistency
of layering?

I appreciate that this is a small CL, and subjectively, should be very easy to
review. However, I hope the review feedback above is positive enough and
explains why I'm not comfortable with this landing. As a reviewer, I want to do
everything I can to make sure the code not just compiles, but fits the problem
at hand and doesn't have any potential red flags/gotchas, or that we've
addressed them. At present, I don't feel there's sufficient documentation to
support this.

I read through the design doc. However, the doc suggests that it's unclear
whether or not the browser will have a 'suspended' state, but it also states
that purging MPLs will be mapped to a suspended MCC. So it's unclear, as a
reviewer, how to evaluate with the given information about whether or not this
will result in 'correct' behaviour.

[xunjieli, 2016-09-23 15:51:22]

Description was changed from

==========
Make HttpNetworkSession a client of memory coordinator

When the memory coordinator is enabled, HttpNetworkSession becomes a
client of the memory coordinator instead of installing
MemoryPressureListener.

In the implmentation of this CL, both MemoryPressureListener and
MemoryCoordinatorClient are installed regardless of the flag, but as
default MemoryCoordinator is not enabled and when MemoryCoordinator is
enabled by a flag, we plan to suppress MemoryPressureMonitor features.

Design Doc:
https://docs.google.com/document/d/1a69mMr7jI7qK0OfKNlrZ350xiXizVMCCe8orGX7K8...

BUG=639700
==========

to

==========
Make HttpNetworkSession a client of memory coordinator

When the memory coordinator is enabled, HttpNetworkSession becomes a
client of the memory coordinator instead of installing
MemoryPressureListener.

In the implmentation of this CL, both MemoryPressureListener and
MemoryCoordinatorClient are installed regardless of the flag, but as
default MemoryCoordinator is not enabled and when MemoryCoordinator is
enabled by a flag, we plan to suppress MemoryPressureMonitor features.

Design Doc:
https://docs.google.com/document/d/1a69mMr7jI7qK0OfKNlrZ350xiXizVMCCe8orGX7K8...

BUG=639700
==========

[haraken, 2016-09-26 00:00:12]

On 2016/09/23 14:50:19, Ryan Sleevi (slow) wrote:
> TL;DR: I'm sorry that I've written more feedback than this CL, but I'm not
> comfortable with this CL landing, but for reasons that can all hopefully be
> easily resolved.
> 
> The pattern being promoted here:
> base::MemoryCoordinatorClientRegistry::GetInstance()->Register(this)
> leaves me with a sense of unease and discomfort - that is, a feeling that it's
> not right and will cause trouble. 
> 
> I've tried to read through the code related to
MemoryCoordinatorClientRegistry,
> and I can't find much (or any) documentation in the headers about the
threading
> guarantees (and find much of the classes under-documented for something that
> lives in //base).
> 
> For example, //base/memory/memory_coordinator_client_registry.h doesn't
document
> any threading guarantees about how clients are invoked. However, those
threading
> guarantees are 'obvious' because the implementation detail (namely, clients())
> has leaked into the public interface. Exposing an ObserverListThreadSafe
> publicly makes me very uncomfortable - I believe it's a dangerous coding
> pattern, much like exposing a WeakPtr/WeakPtrFactory publicly, because it
means
> you can no longer make guarantees about the invariants of when or how an
object
> is called. This makes it difficult, in this CL, to evaluate whether there are
> threading or lifetime risks when you use the API like this.
> 
> Similarly, ///base/memory/memory_coordinator_client.h doesn't document any
> threading or timing guarantees. For example, is OnMemoryStateChange
potentially
> going to flap? Are there guarantees of quiescence or frequency of calls? I
don't
> know, and so it's difficult to evaluate, in this CL, whether there are any
> performance sensitivities this implementation needs to be aware of.
> 
> I'm also uncomfortable because it seems there's no //base memory coordinator
> (and perhaps that's coming), but it makes it hard to actually look at the
> implementation to see how all the pieces fit together. The conclusions from
the
> doc make me think that this is not something that's really been evaluated in
the
> context of //base, but instead in //components. Would the design-docs review
> mailing list have helped in calling out this somewhat surprising inconsistency
> of layering?
> 
> I appreciate that this is a small CL, and subjectively, should be very easy to
> review. However, I hope the review feedback above is positive enough and
> explains why I'm not comfortable with this landing. As a reviewer, I want to
do
> everything I can to make sure the code not just compiles, but fits the problem
> at hand and doesn't have any potential red flags/gotchas, or that we've
> addressed them. At present, I don't feel there's sufficient documentation to
> support this.
> 
> I read through the design doc. However, the doc suggests that it's unclear
> whether or not the browser will have a 'suspended' state, but it also states
> that purging MPLs will be mapped to a suspended MCC. So it's unclear, as a
> reviewer, how to evaluate with the given information about whether or not this
> will result in 'correct' behaviour.

bashi@: Would you address the concerns about documentation of MemoryCoordinator?

Regarding where MemoryCoordinator should be placed, we had a lot of discussions
with owners and have decided to go with that way
(https://codereview.chromium.org/2311723002/).

[bashi, 2016-09-26 02:08:10]

Ryan,
Thank you for the detailed feedback. I tried to address your comments as much as
possible in https://codereview.chromium.org/2363353002. Does that make sense to
you? Since memory coordinator is under development and not fully working yet,
some questions you asked may not be explicitly answered. However, I hope it
describes what memory coordinator would look like.

[chrisha, 2016-09-26 14:29:38]

On 2016/09/23 14:50:19, Ryan Sleevi (slow) wrote:
> TL;DR: I'm sorry that I've written more feedback than this CL, but I'm not
> comfortable with this CL landing, but for reasons that can all hopefully be
> easily resolved.
> 
> The pattern being promoted here:
> base::MemoryCoordinatorClientRegistry::GetInstance()->Register(this)
> leaves me with a sense of unease and discomfort - that is, a feeling that it's
> not right and will cause trouble. 
> 
> I've tried to read through the code related to
MemoryCoordinatorClientRegistry,
> and I can't find much (or any) documentation in the headers about the
threading
> guarantees (and find much of the classes under-documented for something that
> lives in //base).
> 
> For example, //base/memory/memory_coordinator_client_registry.h doesn't
document
> any threading guarantees about how clients are invoked. However, those
threading
> guarantees are 'obvious' because the implementation detail (namely, clients())
> has leaked into the public interface. Exposing an ObserverListThreadSafe
> publicly makes me very uncomfortable - I believe it's a dangerous coding
> pattern, much like exposing a WeakPtr/WeakPtrFactory publicly, because it
means
> you can no longer make guarantees about the invariants of when or how an
object
> is called. This makes it difficult, in this CL, to evaluate whether there are
> threading or lifetime risks when you use the API like this.
> 
> Similarly, ///base/memory/memory_coordinator_client.h doesn't document any
> threading or timing guarantees. For example, is OnMemoryStateChange
potentially
> going to flap? Are there guarantees of quiescence or frequency of calls? I
don't
> know, and so it's difficult to evaluate, in this CL, whether there are any
> performance sensitivities this implementation needs to be aware of.
> 
> I'm also uncomfortable because it seems there's no //base memory coordinator
> (and perhaps that's coming), but it makes it hard to actually look at the
> implementation to see how all the pieces fit together. The conclusions from
the
> doc make me think that this is not something that's really been evaluated in
the
> context of //base, but instead in //components. Would the design-docs review
> mailing list have helped in calling out this somewhat surprising inconsistency
> of layering?
> 
> I appreciate that this is a small CL, and subjectively, should be very easy to
> review. However, I hope the review feedback above is positive enough and
> explains why I'm not comfortable with this landing. As a reviewer, I want to
do
> everything I can to make sure the code not just compiles, but fits the problem
> at hand and doesn't have any potential red flags/gotchas, or that we've
> addressed them. At present, I don't feel there's sufficient documentation to
> support this.
> 
> I read through the design doc. However, the doc suggests that it's unclear
> whether or not the browser will have a 'suspended' state, but it also states
> that purging MPLs will be mapped to a suspended MCC. So it's unclear, as a
> reviewer, how to evaluate with the given information about whether or not this
> will result in 'correct' behaviour.

Note that MC is effectively a drop-in replacement for MPL, but stateful rather
than stateless. Same threading guarantees and rate-limiting, and clearer
guidance on what each "level" means. Agreed that the documentation should be
beefed up to make this more clear.

[Ryan Sleevi, 2016-09-26 15:15:59]

https://codereview.chromium.org/2359723006/diff/1/net/http/http_network_sessi...
File net/http/http_network_session.cc (right):

https://codereview.chromium.org/2359723006/diff/1/net/http/http_network_sessi...
net/http/http_network_session.cc:413: // have such limitation so far though.
I often link to
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis...
for comments like this

// TODO(hajimehoshi): When the state changes, adjust the sizes of the caches to
reduce the limits. HttpNetworkSession doesn't have the ability to limit at
present.

(Assuming I understood the comment properly)

https://codereview.chromium.org/2359723006/diff/1/net/http/http_network_sessi...
net/http/http_network_session.cc:419: // have such value, let's change the
argument accroding to the value.
Typo: according

But I'm also uncertain what this comment is trying to say. Is it trying to
document how //components/memory_coordinator is implemented? If so, it feels
like a layering violation - nothing will update this comment appropriately.

If there is a broader discussion of what to do when throttled, perhaps filing a
bug to attach to and explain sufficient context about what is going on makes
more sense than the comment here.

https://codereview.chromium.org/2359723006/diff/1/net/http/http_network_sessi...
net/http/http_network_session.cc:423: // Note that SUSPENDED never occurs in the
main browser process so far.
//net shouldn't talk about things like "main browser process" - that's a
layering violation (//net is used in more than Chrome, but also shouldn't know
about how //chrome or //components are implemented)

case base::MemoryState::SUSPENDED:
  // Note: Not supported at present. Fall through.
case base::MemoryState::UNKNOWN:
  NOTREACHED();
  break;

https://codereview.chromium.org/2359723006/diff/1/net/http/http_network_sessi...
File net/http/http_network_session.h (right):

https://codereview.chromium.org/2359723006/diff/1/net/http/http_network_sessi...
net/http/http_network_session.h:66: public base::MemoryCoordinatorClient {
Just from a design perspective, base::MemoryPressureListener seems like a safer
design pattern/paradigm.

That is, it automatically includes the de-registration aspect (when it goes out
of scope) and it avoids the need for public inheritance (by virtue of invoking
the callback).

The interface pattern is useful if we anticipate there being multiple methods a
memory coordinator client can have invoked, but if we just expect one (a state
transition), the callback approach seems to offer much better design decoupling.

We could easily imagine this being done by something like

class HttpNetworkSession {
 private:
  class Helper;

  void OnMemoryStateChange(base::MemoryState state);

  std::unique_ptr<Helper> helper_;
};

in .cc
class HttpNetworkSession::Helper {
 public:
  explicit Helper(const base::Callback<base::MemoryState>& callback) :
cb_(callback) {
   ClientRegistry->AddObserver(this);
  }
  ~Helper() {
   ClientRegistry->RemoveObserver(this);
  }

  void OnMemoryStateChange(base::MemoryState state) {
   cb_.Run(state);
  }
};



I mention this because I think having a bunch of classes inherit from
base::MemoryCoordinatorClient as part of their public interfaces probably isn't
what we want/desire long term, because that exposes the implementation detail
publicly, versus the MemoryPressureListener 'implementation detail' approach.

[hajimehoshi, 2016-09-27 11:39:29]

Thank you! (we're discussing interfaces vs callbacks and not conclude yet).

https://codereview.chromium.org/2359723006/diff/1/net/http/http_network_sessi...
File net/http/http_network_session.cc (right):

https://codereview.chromium.org/2359723006/diff/1/net/http/http_network_sessi...
net/http/http_network_session.cc:413: // have such limitation so far though.
On 2016/09/26 15:15:59, Ryan Sleevi (slow) wrote:
> I often link to
>
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis...
> for comments like this
> 
> // TODO(hajimehoshi): When the state changes, adjust the sizes of the caches
to
> reduce the limits. HttpNetworkSession doesn't have the ability to limit at
> present.
> 
> (Assuming I understood the comment properly)

Done.

https://codereview.chromium.org/2359723006/diff/1/net/http/http_network_sessi...
net/http/http_network_session.cc:413: // have such limitation so far though.
On 2016/09/26 15:15:59, Ryan Sleevi (slow) wrote:
> I often link to
>
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis...
> for comments like this
> 
> // TODO(hajimehoshi): When the state changes, adjust the sizes of the caches
to
> reduce the limits. HttpNetworkSession doesn't have the ability to limit at
> present.
> 
> (Assuming I understood the comment properly)

Done.

https://codereview.chromium.org/2359723006/diff/1/net/http/http_network_sessi...
net/http/http_network_session.cc:419: // have such value, let's change the
argument accroding to the value.
On 2016/09/26 15:15:59, Ryan Sleevi (slow) wrote:
> Typo: according
> 
> But I'm also uncertain what this comment is trying to say. Is it trying to
> document how //components/memory_coordinator is implemented? If so, it feels
> like a layering violation - nothing will update this comment appropriately.
> 
> If there is a broader discussion of what to do when throttled, perhaps filing
a
> bug to attach to and explain sufficient context about what is going on makes
> more sense than the comment here.

Done (removed). IIRC we don't have any plan to add this yet.

https://codereview.chromium.org/2359723006/diff/1/net/http/http_network_sessi...
net/http/http_network_session.cc:423: // Note that SUSPENDED never occurs in the
main browser process so far.
On 2016/09/26 15:15:59, Ryan Sleevi (slow) wrote:
> //net shouldn't talk about things like "main browser process" - that's a
> layering violation (//net is used in more than Chrome, but also shouldn't know
> about how //chrome or //components are implemented)
> 
> case base::MemoryState::SUSPENDED:
>   // Note: Not supported at present. Fall through.
> case base::MemoryState::UNKNOWN:
>   NOTREACHED();
>   break;

Done.

[hajimehoshi, 2016-09-28 06:07:04]

hajimehoshi@chromium.org changed reviewers:
+ tasak@google.com
- tasak@chromium.org

[hajimehoshi, 2016-09-28 07:41:35]

hajimehoshi@chromium.org changed reviewers:
+ haraken@chromium.org

[hajimehoshi, 2016-09-28 07:41:35]

+haraken

[haraken, 2016-09-28 08:56:04]

Currently we only have only one API, so using callback function would make more
sense than using interface.

However, the plan is to add a couple of more APIs to MemoryCoordinatorClient.
For example, we're planning to add APIs to let the client report the size of
current memory, the estimated size of memory the client can purge, the estimated
size of memory the client can the throttle etc (see the design doc here:
https://docs.google.com/document/d/1dkUXXmpJk7xBUeQM-olBpTHJ2MXamDgY_kjNrl9JX...).
In that world, I think using interface would make more sense.

Does it makes sense?

[Ryan Sleevi, 2016-09-29 01:52:48]

On 2016/09/28 08:56:04, haraken wrote:
> Currently we only have only one API, so using callback function would make
more
> sense than using interface.
> 
> However, the plan is to add a couple of more APIs to MemoryCoordinatorClient.
> For example, we're planning to add APIs to let the client report the size of
> current memory, the estimated size of memory the client can purge, the
estimated
> size of memory the client can the throttle etc (see the design doc here:
>
https://docs.google.com/document/d/1dkUXXmpJk7xBUeQM-olBpTHJ2MXamDgY_kjNrl9JX...).
> In that world, I think using interface would make more sense.
> 
> Does it makes sense?

Yes and no. I mean, the first seems more relevant to the parallel investigation
in memory usage - that is, it does not make sense to me to have two ways in
which clients can report memory usage (MemoryCoordinatorClient and whatever the
other thing memory-team is doing). I'm unclear of the value of memory purging
estimation and throttling, particularly for //net, and would rather see how the
design gets born out.

That said, even if we do decide that the interface is the right approach, it
does seem desirable to ensure that more memory-safe, RAII patterns for
observation are preferred over manual AddObserver/RemoveObserver calls which can
easily become desynchronized (example: 30 minutes ago I was just looking at a CL
that rectified as "We implemented this interface but forgot to AddObserver it")

I realize these are somewhat 'meta' comments, but they were highlighted from
reviewing this CL.

[Ryan Sleevi, 2016-09-29 02:02:09]

Despite my nits, I'm going to LGTM, but if and only if one thing gets fixed
first, and that's this :)

MemoryCoordinatorClient's destructor should be protected-virtual, not public,
otherwise this introduces an unsafe pattern into memory management of this
class, by potentially resulting in an early or double deletion.

(I'd like to see the AddObserver/RemoveObserver scoped as RAII, but there are
several ways that could be done, and it doesn't seem fair to block this CL on
it, even though I disagree with the current pattern for the long-term)

[Ryan Sleevi, 2016-09-29 02:04:47]

On 2016/09/29 02:02:09, Ryan Sleevi (slow) wrote:
> MemoryCoordinatorClient's destructor should be protected-virtual, not public,
> otherwise this introduces an unsafe pattern into memory management of this
> class, by potentially resulting in an early or double deletion.

I just realized this might have been ambiguous/unclear, so restated:

A public destructor on a pure-virtual interface is a signal that any consumer of
that interface is free to deallocate the structure (e.g. by putting it in a
std::unique_ptr). That's clearly not required of the API contract of
MemoryCoordinatorClient, because if it was, it would not be safe to make this
class derive from it - because HttpNetworkSession is owned (effectively) by the
URLRequestContext.

By making the destructor protected-virtual, you are signalling that the API
contract of the MemoryCoordinatorClient makes no requirements on the
implementations; they might be ref-counted, they might be stack-allocated, they
might be heap allocated, they might be global singletons. I believe that's
consistent with your desired contract, but from this CL alone, that's not
obvious, hence the 'conditional' LG :)

[bashi, 2016-09-29 02:21:53]

On 2016/09/29 02:04:47, Ryan Sleevi (slow) wrote:
> On 2016/09/29 02:02:09, Ryan Sleevi (slow) wrote:
> > MemoryCoordinatorClient's destructor should be protected-virtual, not
public,
> > otherwise this introduces an unsafe pattern into memory management of this
> > class, by potentially resulting in an early or double deletion.
> 
> I just realized this might have been ambiguous/unclear, so restated:
> 
> A public destructor on a pure-virtual interface is a signal that any consumer
of
> that interface is free to deallocate the structure (e.g. by putting it in a
> std::unique_ptr). That's clearly not required of the API contract of
> MemoryCoordinatorClient, because if it was, it would not be safe to make this
> class derive from it - because HttpNetworkSession is owned (effectively) by
the
> URLRequestContext.
> 
> By making the destructor protected-virtual, you are signalling that the API
> contract of the MemoryCoordinatorClient makes no requirements on the
> implementations; they might be ref-counted, they might be stack-allocated,
they
> might be heap allocated, they might be global singletons. I believe that's
> consistent with your desired contract, but from this CL alone, that's not
> obvious, hence the 'conditional' LG :)

Thanks for detailed explanation :) I'll make the destructor of
MemoryCoordinatorClient protected.

[hajimehoshi, 2016-09-29 05:18:41]

The CQ bit was checked by hajimehoshi@chromium.org

[commit-bot: I haz the power, 2016-09-29 05:19:03]

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

[hajimehoshi, 2016-09-29 05:20:02]

On 2016/09/29 02:21:53, bashi1 wrote:
> On 2016/09/29 02:04:47, Ryan Sleevi (slow) wrote:
> > On 2016/09/29 02:02:09, Ryan Sleevi (slow) wrote:
> > > MemoryCoordinatorClient's destructor should be protected-virtual, not
> public,
> > > otherwise this introduces an unsafe pattern into memory management of this
> > > class, by potentially resulting in an early or double deletion.
> > 
> > I just realized this might have been ambiguous/unclear, so restated:
> > 
> > A public destructor on a pure-virtual interface is a signal that any
consumer
> of
> > that interface is free to deallocate the structure (e.g. by putting it in a
> > std::unique_ptr). That's clearly not required of the API contract of
> > MemoryCoordinatorClient, because if it was, it would not be safe to make
this
> > class derive from it - because HttpNetworkSession is owned (effectively) by
> the
> > URLRequestContext.
> > 
> > By making the destructor protected-virtual, you are signalling that the API
> > contract of the MemoryCoordinatorClient makes no requirements on the
> > implementations; they might be ref-counted, they might be stack-allocated,
> they
> > might be heap allocated, they might be global singletons. I believe that's
> > consistent with your desired contract, but from this CL alone, that's not
> > obvious, hence the 'conditional' LG :)
> 
> Thanks for detailed explanation :) I'll make the destructor of
> MemoryCoordinatorClient protected.

Thank you! As the destructor is now protected virtual
(https://codereview.chromium.org/2374003003/), I'll commit this.

[commit-bot: I haz the power, 2016-09-29 06:19:03]

Description was changed from

==========
Make HttpNetworkSession a client of memory coordinator

When the memory coordinator is enabled, HttpNetworkSession becomes a
client of the memory coordinator instead of installing
MemoryPressureListener.

In the implmentation of this CL, both MemoryPressureListener and
MemoryCoordinatorClient are installed regardless of the flag, but as
default MemoryCoordinator is not enabled and when MemoryCoordinator is
enabled by a flag, we plan to suppress MemoryPressureMonitor features.

Design Doc:
https://docs.google.com/document/d/1a69mMr7jI7qK0OfKNlrZ350xiXizVMCCe8orGX7K8...

BUG=639700
==========

to

==========
Make HttpNetworkSession a client of memory coordinator

When the memory coordinator is enabled, HttpNetworkSession becomes a
client of the memory coordinator instead of installing
MemoryPressureListener.

In the implmentation of this CL, both MemoryPressureListener and
MemoryCoordinatorClient are installed regardless of the flag, but as
default MemoryCoordinator is not enabled and when MemoryCoordinator is
enabled by a flag, we plan to suppress MemoryPressureMonitor features.

Design Doc:
https://docs.google.com/document/d/1a69mMr7jI7qK0OfKNlrZ350xiXizVMCCe8orGX7K8...

BUG=639700
==========

[commit-bot: I haz the power, 2016-09-29 06:19:04]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-29 06:21:09]

Description was changed from

==========
Make HttpNetworkSession a client of memory coordinator

When the memory coordinator is enabled, HttpNetworkSession becomes a
client of the memory coordinator instead of installing
MemoryPressureListener.

In the implmentation of this CL, both MemoryPressureListener and
MemoryCoordinatorClient are installed regardless of the flag, but as
default MemoryCoordinator is not enabled and when MemoryCoordinator is
enabled by a flag, we plan to suppress MemoryPressureMonitor features.

Design Doc:
https://docs.google.com/document/d/1a69mMr7jI7qK0OfKNlrZ350xiXizVMCCe8orGX7K8...

BUG=639700
==========

to

==========
Make HttpNetworkSession a client of memory coordinator

When the memory coordinator is enabled, HttpNetworkSession becomes a
client of the memory coordinator instead of installing
MemoryPressureListener.

In the implmentation of this CL, both MemoryPressureListener and
MemoryCoordinatorClient are installed regardless of the flag, but as
default MemoryCoordinator is not enabled and when MemoryCoordinator is
enabled by a flag, we plan to suppress MemoryPressureMonitor features.

Design Doc:
https://docs.google.com/document/d/1a69mMr7jI7qK0OfKNlrZ350xiXizVMCCe8orGX7K8...

BUG=639700

Committed: https://crrev.com/8156e7c5f7b94c717ccc4a97eb1dc09f8e6723a6
Cr-Commit-Position: refs/heads/master@{#421761}
==========

[commit-bot: I haz the power, 2016-09-29 06:21:10]

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