Forward handling of low-mem message to the content renderer client. Add APIs to handle this case.

Forward handling of low-mem message to the content renderer client. Add APIs to handle this case.
Found from Coverity CID=16390

R=jam@chromium.org
BUG=
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=95896

[jam, 2011-06-21 22:59:39]

lgtm, thanks

http://codereview.chromium.org/7222023/diff/1/chrome/renderer/chrome_content_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/7222023/diff/1/chrome/renderer/chrome_content_...
chrome/renderer/chrome_content_renderer_client.cc:586: if (spellcheck_.get()) {
nit: this file doesn't use brace brackets for one line statements, so please
follow the convention (also below)

http://codereview.chromium.org/7222023/diff/1/chrome/renderer/spellchecker/sp...
File chrome/renderer/spellchecker/spellcheck.cc (right):

http://codereview.chromium.org/7222023/diff/1/chrome/renderer/spellchecker/sp...
chrome/renderer/spellchecker/spellcheck.cc:42: if (renderer_client_) {
ditto

[jam, 2011-06-21 23:02:21]

btw, no need for empty

BUG=
TEST=

lines.  only need to add one if you have a bug id or specific test instructions
to the test team.

[Greg Billock, 2011-06-22 15:58:35]

Thanks. Is there a way to trigger this lowmem condition artificially to test and
make sure it works?

http://codereview.chromium.org/7222023/diff/1/chrome/renderer/chrome_content_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/7222023/diff/1/chrome/renderer/chrome_content_...
chrome/renderer/chrome_content_renderer_client.cc:586: if (spellcheck_.get()) {
On 2011/06/21 22:59:39, John Abd-El-Malek wrote:
> nit: this file doesn't use brace brackets for one line statements, so please
> follow the convention (also below)

Done.

http://codereview.chromium.org/7222023/diff/1/chrome/renderer/spellchecker/sp...
File chrome/renderer/spellchecker/spellcheck.cc (right):

http://codereview.chromium.org/7222023/diff/1/chrome/renderer/spellchecker/sp...
chrome/renderer/spellchecker/spellcheck.cc:42: if (renderer_client_) {
On 2011/06/21 22:59:39, John Abd-El-Malek wrote:
> ditto

Done.

[jam, 2011-06-23 18:36:53]

(still lgtm)

On 2011/06/22 15:58:35, Greg Billock wrote:
> Thanks. Is there a way to trigger this lowmem condition artificially to test
and
> make sure it works?

I'm not familiar with that. you can try sending the IPC manually in the browser
process?
> 
>
http://codereview.chromium.org/7222023/diff/1/chrome/renderer/chrome_content_...
> File chrome/renderer/chrome_content_renderer_client.cc (right):
> 
>
http://codereview.chromium.org/7222023/diff/1/chrome/renderer/chrome_content_...
> chrome/renderer/chrome_content_renderer_client.cc:586: if (spellcheck_.get())
{
> On 2011/06/21 22:59:39, John Abd-El-Malek wrote:
> > nit: this file doesn't use brace brackets for one line statements, so please
> > follow the convention (also below)
> 
> Done.
> 
>
http://codereview.chromium.org/7222023/diff/1/chrome/renderer/spellchecker/sp...
> File chrome/renderer/spellchecker/spellcheck.cc (right):
> 
>
http://codereview.chromium.org/7222023/diff/1/chrome/renderer/spellchecker/sp...
> chrome/renderer/spellchecker/spellcheck.cc:42: if (renderer_client_) {
> On 2011/06/21 22:59:39, John Abd-El-Malek wrote:
> > ditto
> 
> Done.

[Greg Billock, 2011-07-21 00:17:32]

I was testing this out. It turns out the ChromeRenderProcessObserver was
swallowing the message, so the code never got executed anyway. I was talking
with Al Patrick about what to do. We decided to forward to the
NotificationService. (There's already a message type for this for the IO
thread.) I think that's better, if a bit more invasive than violating the IPC
semantics by returning false from the observers.

I left in some test code right now (in browser.cc and a couple LOGs) I'll take
those out before submitting. Have a look and tell me if there's any flaws in
this plan.

On 2011/06/23 18:36:53, John Abd-El-Malek wrote:
> (still lgtm)
> 
> On 2011/06/22 15:58:35, Greg Billock wrote:
> > Thanks. Is there a way to trigger this lowmem condition artificially to test
> and
> > make sure it works?
> 
> I'm not familiar with that. you can try sending the IPC manually in the
browser
> process?
> > 
> >
>
http://codereview.chromium.org/7222023/diff/1/chrome/renderer/chrome_content_...
> > File chrome/renderer/chrome_content_renderer_client.cc (right):
> > 
> >
>
http://codereview.chromium.org/7222023/diff/1/chrome/renderer/chrome_content_...
> > chrome/renderer/chrome_content_renderer_client.cc:586: if
(spellcheck_.get())
> {
> > On 2011/06/21 22:59:39, John Abd-El-Malek wrote:
> > > nit: this file doesn't use brace brackets for one line statements, so
please
> > > follow the convention (also below)
> > 
> > Done.
> > 
> >
>
http://codereview.chromium.org/7222023/diff/1/chrome/renderer/spellchecker/sp...
> > File chrome/renderer/spellchecker/spellcheck.cc (right):
> > 
> >
>
http://codereview.chromium.org/7222023/diff/1/chrome/renderer/spellchecker/sp...
> > chrome/renderer/spellchecker/spellcheck.cc:42: if (renderer_client_) {
> > On 2011/06/21 22:59:39, John Abd-El-Malek wrote:
> > > ditto
> > 
> > Done.

[jam, 2011-07-21 06:43:16]

On 2011/07/21 00:17:32, Greg Billock wrote:
> I was testing this out. It turns out the ChromeRenderProcessObserver was
> swallowing the message, so the code never got executed anyway. I was talking
> with Al Patrick about what to do. We decided to forward to the
> NotificationService. (There's already a message type for this for the IO
> thread.)

We avoid using NotificationService in renderer process. The notification system
is overused in chrome, and allows random parts of the code to call each other
indirectly. We especially wouldn't want to reuse a notification in different
processes.

> I think that's better, if a bit more invasive than violating the IPC
> semantics by returning false from the observers.

I'm not sure what you mean by violating the IPC semantics. It's valid for a
class to filter messages and not swallow them.

> 
> I left in some test code right now (in browser.cc and a couple LOGs) I'll take
> those out before submitting. Have a look and tell me if there's any flaws in
> this plan.
> 
> On 2011/06/23 18:36:53, John Abd-El-Malek wrote:
> > (still lgtm)
> > 
> > On 2011/06/22 15:58:35, Greg Billock wrote:
> > > Thanks. Is there a way to trigger this lowmem condition artificially to
test
> > and
> > > make sure it works?
> > 
> > I'm not familiar with that. you can try sending the IPC manually in the
> browser
> > process?
> > > 
> > >
> >
>
http://codereview.chromium.org/7222023/diff/1/chrome/renderer/chrome_content_...
> > > File chrome/renderer/chrome_content_renderer_client.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/7222023/diff/1/chrome/renderer/chrome_content_...
> > > chrome/renderer/chrome_content_renderer_client.cc:586: if
> (spellcheck_.get())
> > {
> > > On 2011/06/21 22:59:39, John Abd-El-Malek wrote:
> > > > nit: this file doesn't use brace brackets for one line statements, so
> please
> > > > follow the convention (also below)
> > > 
> > > Done.
> > > 
> > >
> >
>
http://codereview.chromium.org/7222023/diff/1/chrome/renderer/spellchecker/sp...
> > > File chrome/renderer/spellchecker/spellcheck.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/7222023/diff/1/chrome/renderer/spellchecker/sp...
> > > chrome/renderer/spellchecker/spellcheck.cc:42: if (renderer_client_) {
> > > On 2011/06/21 22:59:39, John Abd-El-Malek wrote:
> > > > ditto
> > > 
> > > Done.

[jam, 2011-07-21 16:35:27]

http://codereview.chromium.org/7222023/diff/13001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7222023/diff/13001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1938: 
nit: extra line

http://codereview.chromium.org/7222023/diff/13001/content/renderer/render_thr...
File content/renderer/render_thread.cc (right):

http://codereview.chromium.org/7222023/diff/13001/content/renderer/render_thr...
content/renderer/render_thread.cc:24: #include "chrome/common/render_messages.h"
also, code in content can't include code in chrome,so you'll get a deps failure
for this. this is sent from chrome code, so it shouldn't be dispatched in
content.

[Greg Billock, 2011-07-21 17:26:48]

On Wed, Jul 20, 2011 at 11:43 PM, <jam@chromium.org> wrote:

> On 2011/07/21 00:17:32, Greg Billock wrote:
>
>> I was testing this out. It turns out the ChromeRenderProcessObserver was
>> swallowing the message, so the code never got executed anyway. I was
>> talking
>> with Al Patrick about what to do. We decided to forward to the
>> NotificationService. (There's already a message type for this for the IO
>> thread.)
>>
>
> We avoid using NotificationService in renderer process. The notification
> system
> is overused in chrome, and allows random parts of the code to call each
> other
> indirectly. We especially wouldn't want to reuse a notification in
> different
> processes.
>
>
drat. it seemed nice.


>
>  I think that's better, if a bit more invasive than violating the IPC
>> semantics by returning false from the observers.
>>
>
> I'm not sure what you mean by violating the IPC semantics. It's valid for a
> class to filter messages and not swallow them.
>
>
This isn't filtering -- it is responding to them and saying it didn't. Is
there a facility in IPC::Message or the rest of the IPC system to say that a
message is broadcast, and expects these semantics? During my testing, I left
'return false' on and the message just gets redelivered. So it's pretty
clear that this is unexpected and erroneous behavior not well tolerated by
the machinery. It could definitely be abused this way, but terminating the
message in RenderThread. Do you think that's better than using
NotificationService, though?


>
>
>  I left in some test code right now (in browser.cc and a couple LOGs) I'll
>> take
>> those out before submitting. Have a look and tell me if there's any flaws
>> in
>> this plan.
>>
>
>  On 2011/06/23 18:36:53, John Abd-El-Malek wrote:
>> > (still lgtm)
>> >
>> > On 2011/06/22 15:58:35, Greg Billock wrote:
>> > > Thanks. Is there a way to trigger this lowmem condition artificially
>> to
>>
> test
>
>> > and
>> > > make sure it works?
>> >
>> > I'm not familiar with that. you can try sending the IPC manually in the
>> browser
>> > process?
>> > >
>> > >
>> >
>>
>
> http://codereview.chromium.**org/7222023/diff/1/chrome/**
>
renderer/chrome_content_**renderer_client.cc<http://codereview.chromium.org/7222023/diff/1/chrome/renderer/chrome_content_renderer_client.cc>
>
>> > > File chrome/renderer/chrome_**content_renderer_client.cc (right):
>> > >
>> > >
>> >
>>
>
> http://codereview.chromium.**org/7222023/diff/1/chrome/**
>
renderer/chrome_content_**renderer_client.cc#newcode586<http://codereview.chromium.org/7222023/diff/1/chrome/renderer/chrome_content_renderer_client.cc#newcode586>
>
>> > > chrome/renderer/chrome_**content_renderer_client.cc:**586: if
>> (spellcheck_.get())
>> > {
>> > > On 2011/06/21 22:59:39, John Abd-El-Malek wrote:
>> > > > nit: this file doesn't use brace brackets for one line statements,
>> so
>> please
>> > > > follow the convention (also below)
>> > >
>> > > Done.
>> > >
>> > >
>> >
>>
>
> http://codereview.chromium.**org/7222023/diff/1/chrome/**
>
renderer/spellchecker/**spellcheck.cc<http://codereview.chromium.org/7222023/diff/1/chrome/renderer/spellchecker/spellcheck.cc>
>
>> > > File chrome/renderer/spellchecker/**spellcheck.cc (right):
>> > >
>> > >
>> >
>>
>
> http://codereview.chromium.**org/7222023/diff/1/chrome/**
>
renderer/spellchecker/**spellcheck.cc#newcode42<http://codereview.chromium.org/7222023/diff/1/chrome/renderer/spellchecker/spellcheck.cc#newcode42>
>
>> > > chrome/renderer/spellchecker/**spellcheck.cc:42: if
>> (renderer_client_) {
>> > > On 2011/06/21 22:59:39, John Abd-El-Malek wrote:
>> > > > ditto
>> > >
>> > > Done.
>>
>
>
>
>
http://codereview.chromium.**org/7222023/<http://codereview.chromium.org/7222...
>

[Greg Billock, 2011-07-21 19:10:01]

OK, I've updated this. John and I talked about this on IM. The RenderThread is
bad to terminate this message, since it is in content/. We need to terminate it
in chrome/renderer/. As we discussed, I made the ChromeRenderProcessObserver
call into the ChromeRenderClient, which works. It is a bit ugly, since now it
needs a back reference, which it didn't. Since the ChromeRenderClient creates
the CRPO class, it'd be nice to have it delegate that way, but the whole point
is so the render client class doesn't do message sinking, so I did it the way we
talked. Tell me what you think.

http://codereview.chromium.org/7222023/diff/13001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7222023/diff/13001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1938: 
On 2011/07/21 16:35:27, John Abd-El-Malek wrote:
> nit: extra line

Yeah, I'm going to revert this file. This was just if you cared to see the test
setup.

http://codereview.chromium.org/7222023/diff/13001/content/renderer/render_thr...
File content/renderer/render_thread.cc (right):

http://codereview.chromium.org/7222023/diff/13001/content/renderer/render_thr...
content/renderer/render_thread.cc:24: #include "chrome/common/render_messages.h"
Is there a lint or presubmit to run that detects this? It compiles ok, but
presumably there's another tool that will holler.

On 2011/07/21 16:35:27, John Abd-El-Malek wrote:
> also, code in content can't include code in chrome,so you'll get a deps
failure
> for this. this is sent from chrome code, so it shouldn't be dispatched in
> content.

[jam, 2011-07-22 16:31:50]

lgtm with nits

http://codereview.chromium.org/7222023/diff/13001/content/renderer/render_thr...
File content/renderer/render_thread.cc (right):

http://codereview.chromium.org/7222023/diff/13001/content/renderer/render_thr...
content/renderer/render_thread.cc:24: #include "chrome/common/render_messages.h"
On 2011/07/21 19:10:01, Greg Billock wrote:
> Is there a lint or presubmit to run that detects this? It compiles ok, but
> presumably there's another tool that will holler.
> 
> On 2011/07/21 16:35:27, John Abd-El-Malek wrote:
> > also, code in content can't include code in chrome,so you'll get a deps
> failure
> > for this. this is sent from chrome code, so it shouldn't be dispatched in
> > content.
> 

there's tools\checkdeps\checkdeps.py. We don't run it on presubmit because it's
slow, but it runs on the trybots

http://codereview.chromium.org/7222023/diff/17001/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/7222023/diff/17001/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:633: LOG(INFO) << "Resetting
spellcheck in renderer client";
nit: i believe people try to use VLOG now so that it's easier to filter only the
messages they want to see

http://codereview.chromium.org/7222023/diff/17001/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.h (right):

http://codereview.chromium.org/7222023/diff/17001/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.h:77: // Called in low-memory
conditions to dump the memory used by the spellchecker
nit: why add two methods? i.e. why not just have the caller call ResetSpellCheck
directly?

http://codereview.chromium.org/7222023/diff/17001/chrome/renderer/chrome_rend...
File chrome/renderer/chrome_render_process_observer.cc (right):

http://codereview.chromium.org/7222023/diff/17001/chrome/renderer/chrome_rend...
chrome/renderer/chrome_render_process_observer.cc:488: LOG(INFO) << "CRPO
purging memory!";
ditto

http://codereview.chromium.org/7222023/diff/17001/chrome/renderer/chrome_rend...
chrome/renderer/chrome_render_process_observer.cc:516: if (client_)
nit: can client_ ever be NULL? if not then no need for this

[Greg Billock, 2011-07-22 17:02:07]

http://codereview.chromium.org/7222023/diff/13001/content/renderer/render_thr...
File content/renderer/render_thread.cc (right):

http://codereview.chromium.org/7222023/diff/13001/content/renderer/render_thr...
content/renderer/render_thread.cc:24: #include "chrome/common/render_messages.h"
cool. I did see the check_deps fail on the trybots.

On 2011/07/22 16:31:50, John Abd-El-Malek wrote:
> On 2011/07/21 19:10:01, Greg Billock wrote:
> > Is there a lint or presubmit to run that detects this? It compiles ok, but
> > presumably there's another tool that will holler.
> > 
> > On 2011/07/21 16:35:27, John Abd-El-Malek wrote:
> > > also, code in content can't include code in chrome,so you'll get a deps
> > failure
> > > for this. this is sent from chrome code, so it shouldn't be dispatched in
> > > content.
> > 
> 
> there's tools\checkdeps\checkdeps.py. We don't run it on presubmit because
it's
> slow, but it runs on the trybots

http://codereview.chromium.org/7222023/diff/17001/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/7222023/diff/17001/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:633: LOG(INFO) << "Resetting
spellcheck in renderer client";
Yeah, I'm going to strip out all this test code. A VLOG might be good, though.
I'll leave one of those here.

On 2011/07/22 16:31:50, John Abd-El-Malek wrote:
> nit: i believe people try to use VLOG now so that it's easier to filter only
the
> messages they want to see

http://codereview.chromium.org/7222023/diff/17001/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.h (right):

http://codereview.chromium.org/7222023/diff/17001/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.h:77: // Called in low-memory
conditions to dump the memory used by the spellchecker
On 2011/07/22 16:31:50, John Abd-El-Malek wrote:
> nit: why add two methods? i.e. why not just have the caller call
ResetSpellCheck
> directly?

Good point. I liked having the caller call OnPurgeMemory so it's more
self-documenting when this happens and the call sequence, but there's no reason
for an extra signature here. Removed ResetSpellCheck and just did it in
OnPurgeMemory.

http://codereview.chromium.org/7222023/diff/17001/chrome/renderer/chrome_rend...
File chrome/renderer/chrome_render_process_observer.cc (right):

http://codereview.chromium.org/7222023/diff/17001/chrome/renderer/chrome_rend...
chrome/renderer/chrome_render_process_observer.cc:488: LOG(INFO) << "CRPO
purging memory!";
removed

On 2011/07/22 16:31:50, John Abd-El-Malek wrote:
> ditto

http://codereview.chromium.org/7222023/diff/17001/chrome/renderer/chrome_rend...
chrome/renderer/chrome_render_process_observer.cc:516: if (client_)
On 2011/07/22 16:31:50, John Abd-El-Malek wrote:
> nit: can client_ ever be NULL? if not then no need for this

For unit tests and such it can, so I thought this would be best.

[Greg Billock, 2011-07-28 19:56:55]

Anything else on this? I'm planning to submit. trybots look ok wrt this
change (there are some other errors, but they look unrelated)

On Fri, Jul 22, 2011 at 10:02 AM, <gbillock@chromium.org> wrote:

>
> http://codereview.chromium.**org/7222023/diff/13001/**
>
content/renderer/render_**thread.cc<http://codereview.chromium.org/7222023/diff/13001/content/renderer/render_thread.cc>
> File content/renderer/render_**thread.cc (right):
>
> http://codereview.chromium.**org/7222023/diff/13001/**
>
content/renderer/render_**thread.cc#newcode24<http://codereview.chromium.org/7222023/diff/13001/content/renderer/render_thread.cc#newcode24>
> content/renderer/render_**thread.cc:24: #include
> "chrome/common/render_**messages.h"
> cool. I did see the check_deps fail on the trybots.
>
>
> On 2011/07/22 16:31:50, John Abd-El-Malek wrote:
>
>> On 2011/07/21 19:10:01, Greg Billock wrote:
>> > Is there a lint or presubmit to run that detects this? It compiles
>>
> ok, but
>
>> > presumably there's another tool that will holler.
>> >
>> > On 2011/07/21 16:35:27, John Abd-El-Malek wrote:
>> > > also, code in content can't include code in chrome,so you'll get a
>>
> deps
>
>> > failure
>> > > for this. this is sent from chrome code, so it shouldn't be
>>
> dispatched in
>
>> > > content.
>> >
>>
>
>  there's tools\checkdeps\checkdeps.py. We don't run it on presubmit
>>
> because it's
>
>> slow, but it runs on the trybots
>>
>
> http://codereview.chromium.**org/7222023/diff/17001/chrome/**
>
renderer/chrome_content_**renderer_client.cc<http://codereview.chromium.org/7222023/diff/17001/chrome/renderer/chrome_content_renderer_client.cc>
> File chrome/renderer/chrome_**content_renderer_client.cc (right):
>
> http://codereview.chromium.**org/7222023/diff/17001/chrome/**
>
renderer/chrome_content_**renderer_client.cc#newcode633<http://codereview.chromium.org/7222023/diff/17001/chrome/renderer/chrome_content_renderer_client.cc#newcode633>
> chrome/renderer/chrome_**content_renderer_client.cc:**633: LOG(INFO) <<
> "Resetting spellcheck in renderer client";
> Yeah, I'm going to strip out all this test code. A VLOG might be good,
> though. I'll leave one of those here.
>
>
> On 2011/07/22 16:31:50, John Abd-El-Malek wrote:
>
>> nit: i believe people try to use VLOG now so that it's easier to
>>
> filter only the
>
>> messages they want to see
>>
>
> http://codereview.chromium.**org/7222023/diff/17001/chrome/**
>
renderer/chrome_content_**renderer_client.h<http://codereview.chromium.org/7222023/diff/17001/chrome/renderer/chrome_content_renderer_client.h>
> File chrome/renderer/chrome_**content_renderer_client.h (right):
>
> http://codereview.chromium.**org/7222023/diff/17001/chrome/**
>
renderer/chrome_content_**renderer_client.h#newcode77<http://codereview.chromium.org/7222023/diff/17001/chrome/renderer/chrome_content_renderer_client.h#newcode77>
> chrome/renderer/chrome_**content_renderer_client.h:77: // Called in
> low-memory conditions to dump the memory used by the spellchecker
> On 2011/07/22 16:31:50, John Abd-El-Malek wrote:
>
>> nit: why add two methods? i.e. why not just have the caller call
>>
> ResetSpellCheck
>
>> directly?
>>
>
> Good point. I liked having the caller call OnPurgeMemory so it's more
> self-documenting when this happens and the call sequence, but there's no
> reason for an extra signature here. Removed ResetSpellCheck and just did
> it in OnPurgeMemory.
>
>
> http://codereview.chromium.**org/7222023/diff/17001/chrome/**
>
renderer/chrome_render_**process_observer.cc<http://codereview.chromium.org/7222023/diff/17001/chrome/renderer/chrome_render_process_observer.cc>
> File chrome/renderer/chrome_render_**process_observer.cc (right):
>
> http://codereview.chromium.**org/7222023/diff/17001/chrome/**
>
renderer/chrome_render_**process_observer.cc#newcode488<http://codereview.chromium.org/7222023/diff/17001/chrome/renderer/chrome_render_process_observer.cc#newcode488>
> chrome/renderer/chrome_render_**process_observer.cc:488: LOG(INFO) <<
> "CRPO purging memory!";
> removed
>
>
> On 2011/07/22 16:31:50, John Abd-El-Malek wrote:
>
>> ditto
>>
>
> http://codereview.chromium.**org/7222023/diff/17001/chrome/**
>
renderer/chrome_render_**process_observer.cc#newcode516<http://codereview.chromium.org/7222023/diff/17001/chrome/renderer/chrome_render_process_observer.cc#newcode516>
> chrome/renderer/chrome_render_**process_observer.cc:516: if (client_)
> On 2011/07/22 16:31:50, John Abd-El-Malek wrote:
>
>> nit: can client_ ever be NULL? if not then no need for this
>>
>
> For unit tests and such it can, so I thought this would be best.
>
>
>
http://codereview.chromium.**org/7222023/<http://codereview.chromium.org/7222...
>

[commit-bot: I haz the power, 2011-08-08 22:45:33]

Change committed as 95896