Migrate components/data_reduction_proxy to Mojo interfaces.

Migrate components/data_reduction_proxy to Mojo interfaces.

This CL also adds typemapping for net::HostPortPair.

BUG=577685
TEST=components_unittests --gtest_filter=DataReductionProxyHostImplTest*

[leonhsl(Using Gerrit), 2016-06-13 09:50:34]

leon.han@intel.com changed reviewers:
+ agl@chromium.org, amistry@chromium.org, megjablon@chromium.org

[leonhsl(Using Gerrit), 2016-06-13 09:50:35]

Hi, would you PTAL at this? If overall OK, I'll add gyp and unittest codes
later. Thanks. 

amistry@: Mojo related overall

megjablon@: OWNER review for data_reduction_proxy

agl@: OWNER review for net

[agl, 2016-06-13 15:36:10]

Rubber stamp LGTM for net/ if Mojo reviewers agree that this is correct.

[leonhsl(Using Gerrit), 2016-06-14 09:50:37]

Description was changed from

==========
Migrate components/data_reduction_proxy to Mojo interfaces.

This CL also adds typemapping for net::HostPortPair.

BUG=577685
==========

to

==========
Migrate components/data_reduction_proxy to Mojo interfaces.

This CL also adds typemapping for net::HostPortPair.

BUG=577685
TEST=components_unittests --gtest_filter=DataReductionProxyHostImplTest*
==========

[leonhsl(Using Gerrit), 2016-06-14 09:58:06]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-14 09:58:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2063683002/40001

[leonhsl(Using Gerrit), 2016-06-14 10:16:47]

leon.han@intel.com changed reviewers:
+ sammc@chromium.org

[leonhsl(Using Gerrit), 2016-06-14 10:16:48]

+sammc@

Hi, Anand, Sam, would you PTAL at this CL and my inline question? Thanks.

https://codereview.chromium.org/2063683002/diff/40001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2063683002/diff/40001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:994:
host->GetServiceRegistry()->AddService(
Instead of here I think it's better to put the AddService call into
ChromeContentBrowserClient::RegisterRenderProcessMojoServices(service_registry),
but there we can't get the target RPH, which is necessary to get the attached
DataReductionProxyChromeSettings instance.
So how about we add one more parameter(the RPH) to the original function
declaration, just as:
ContentBrowserClient::RegisterRenderProcessMojoServices(service_registry,
render_process_host)? This way will enable bind some RPH-specific information.
WDYT?

[commit-bot: I haz the power, 2016-06-14 10:42:19]

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

[commit-bot: I haz the power, 2016-06-14 10:42:20]

Dry run: This issue passed the CQ dry run.

[Anand Mistry (off Chromium), 2016-06-15 08:09:40]

Just responding to the question. Haven't had a chance to review yet.

https://codereview.chromium.org/2063683002/diff/40001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2063683002/diff/40001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:994:
host->GetServiceRegistry()->AddService(
On 2016/06/14 10:16:48, leonhsl wrote:
> Instead of here I think it's better to put the AddService call into
>
ChromeContentBrowserClient::RegisterRenderProcessMojoServices(service_registry),
> but there we can't get the target RPH, which is necessary to get the attached
> DataReductionProxyChromeSettings instance.
> So how about we add one more parameter(the RPH) to the original function
> declaration, just as:
> ContentBrowserClient::RegisterRenderProcessMojoServices(service_registry,
> render_process_host)? This way will enable bind some RPH-specific information.
> WDYT?

I think that's reasonable given that the RenderFrameHost equivalent provides the
host. I suggest doing it as a separate CL since it's orthogonal to this one and
likely to be reviewed faster.

[leonhsl(Using Gerrit), 2016-06-16 07:01:36]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-16 07:01:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2063683002/60001

[commit-bot: I haz the power, 2016-06-16 08:07:42]

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

[commit-bot: I haz the power, 2016-06-16 08:07:45]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-06-16 08:28:30]

Uploaded ps#4.

https://codereview.chromium.org/2063683002/diff/40001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2063683002/diff/40001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:994:
host->GetServiceRegistry()->AddService(
On 2016/06/15 08:09:40, Anand Mistry wrote:
> On 2016/06/14 10:16:48, leonhsl wrote:
> > Instead of here I think it's better to put the AddService call into
> >
>
ChromeContentBrowserClient::RegisterRenderProcessMojoServices(service_registry),
> > but there we can't get the target RPH, which is necessary to get the
attached
> > DataReductionProxyChromeSettings instance.
> > So how about we add one more parameter(the RPH) to the original function
> > declaration, just as:
> > ContentBrowserClient::RegisterRenderProcessMojoServices(service_registry,
> > render_process_host)? This way will enable bind some RPH-specific
information.
> > WDYT?
> 
> I think that's reasonable given that the RenderFrameHost equivalent provides
the
> host. I suggest doing it as a separate CL since it's orthogonal to this one
and
> likely to be reviewed faster.

Done.
A separate CL https://codereview.chromium.org/2060943005/ has been landed and
for this CL moved AddService call into
ChromeContentBrowserClient::RegisterRenderProcessMojoServices(). Thanks!

[Anand Mistry (off Chromium), 2016-06-16 09:04:04]

https://codereview.chromium.org/2063683002/diff/60001/components/data_reducti...
File
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.h
(right):

https://codereview.chromium.org/2063683002/diff/60001/components/data_reducti...
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.h:37:
// Must outlive |this|. May be null.
How can you guarantee this? The legacy IPC achieved this because the message
filter is attached to the RenderProcessHost and will be destroyed when the RPH
is gone. I'm assuming the RPH outlives the Profile. However, this class's
lifecycle is disconnected from the RPH and instead governed by the life of the
message pipe.

[leonhsl(Using Gerrit), 2016-06-17 08:43:57]

https://codereview.chromium.org/2063683002/diff/60001/components/data_reducti...
File
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.h
(right):

https://codereview.chromium.org/2063683002/diff/60001/components/data_reducti...
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.h:37:
// Must outlive |this|. May be null.
On 2016/06/16 09:04:04, Anand Mistry wrote:
> How can you guarantee this? The legacy IPC achieved this because the message
> filter is attached to the RenderProcessHost and will be destroyed when the RPH
> is gone. I'm assuming the RPH outlives the Profile. However, this class's
> lifecycle is disconnected from the RPH and instead governed by the life of the
> message pipe.

Yeah it is, the lifecycle is the difference with before. Thanks a lot for
pointing out this. I investigated around and I think we may have 2 options now:
1, 
Just for current use case, client side(PageLoadHistograms::Dump() on renderer
thread) executes "Connect-->Sync call-->Disconnect" all at once, message pipe
lifetime is just this timeslot, and the same to  DataReductionProxyHostImpl,
while within this timeslot the browser side RPH should just be keeping
alive(because renderer thread is running one task). This means we're still
achieving "RPH outlives DataReductionProxyHostImpl", so should have no problem..
 Maybe I'm saying something stupid,, but just my understanding now ;-) 
2,
Let DataReductionProxyHostImpl be a RPH observer so that it can connect
lifecycle with RPH. Once got RPH destroyed notification, post a task to IO
thread to delete itself.
 
WDYT? Thanks. If option1 is nonsense, I'll try option2.

[Sam McNally, 2016-06-20 04:57:26]

https://codereview.chromium.org/2063683002/diff/60001/components/data_reducti...
File
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.cc
(right):

https://codereview.chromium.org/2063683002/diff/60001/components/data_reducti...
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.cc:28:
config_ = settings->Config();
DataReductionProxySettings can only be used on the UI thread.

https://codereview.chromium.org/2063683002/diff/60001/components/data_reducti...
File
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.h
(right):

https://codereview.chromium.org/2063683002/diff/60001/components/data_reducti...
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.h:37:
// Must outlive |this|. May be null.
On 2016/06/17 08:43:57, leonhsl wrote:
> On 2016/06/16 09:04:04, Anand Mistry wrote:
> > How can you guarantee this? The legacy IPC achieved this because the message
> > filter is attached to the RenderProcessHost and will be destroyed when the
RPH
> > is gone. I'm assuming the RPH outlives the Profile. However, this class's
> > lifecycle is disconnected from the RPH and instead governed by the life of
the
> > message pipe.
> 
> Yeah it is, the lifecycle is the difference with before. Thanks a lot for
> pointing out this. I investigated around and I think we may have 2 options
now:
> 1, 
> Just for current use case, client side(PageLoadHistograms::Dump() on renderer
> thread) executes "Connect-->Sync call-->Disconnect" all at once, message pipe
> lifetime is just this timeslot, and the same to  DataReductionProxyHostImpl,
> while within this timeslot the browser side RPH should just be keeping
> alive(because renderer thread is running one task). This means we're still
> achieving "RPH outlives DataReductionProxyHostImpl", so should have no
problem..
>  Maybe I'm saying something stupid,, but just my understanding now ;-) 
> 2,
> Let DataReductionProxyHostImpl be a RPH observer so that it can connect
> lifecycle with RPH. Once got RPH destroyed notification, post a task to IO
> thread to delete itself.
>  
> WDYT? Thanks. If option1 is nonsense, I'll try option2.

The lifetime in the renderer doesn't directly correspond to lifetime in the
browser. IsDataReductionProxy() won't be called synchronously when the pipe is
bound and neither will the disconnect.

On the other hand, using a RenderProcessHostObserver would be messy at
destruction time if the StrongBinding still lives on the IO thread as the UI and
IO threads could both decide to delete this at the same time.

[leonhsl(Using Gerrit), 2016-06-20 10:51:51]

https://codereview.chromium.org/2063683002/diff/60001/components/data_reducti...
File
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.cc
(right):

https://codereview.chromium.org/2063683002/diff/60001/components/data_reducti...
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.cc:28:
config_ = settings->Config();
On 2016/06/20 04:57:26, Sam McNally wrote:
> DataReductionProxySettings can only be used on the UI thread.

Oh yes, we can pass settings->Config() instead of settings into the constructor.

https://codereview.chromium.org/2063683002/diff/60001/components/data_reducti...
File
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.h
(right):

https://codereview.chromium.org/2063683002/diff/60001/components/data_reducti...
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.h:37:
// Must outlive |this|. May be null.
On 2016/06/20 04:57:26, Sam McNally wrote:
> On 2016/06/17 08:43:57, leonhsl wrote:
> > On 2016/06/16 09:04:04, Anand Mistry wrote:
> > > How can you guarantee this? The legacy IPC achieved this because the
message
> > > filter is attached to the RenderProcessHost and will be destroyed when the
> RPH
> > > is gone. I'm assuming the RPH outlives the Profile. However, this class's
> > > lifecycle is disconnected from the RPH and instead governed by the life of
> the
> > > message pipe.
> > 
> > Yeah it is, the lifecycle is the difference with before. Thanks a lot for
> > pointing out this. I investigated around and I think we may have 2 options
> now:
> > 1, 
> > Just for current use case, client side(PageLoadHistograms::Dump() on
renderer
> > thread) executes "Connect-->Sync call-->Disconnect" all at once, message
pipe
> > lifetime is just this timeslot, and the same to  DataReductionProxyHostImpl,
> > while within this timeslot the browser side RPH should just be keeping
> > alive(because renderer thread is running one task). This means we're still
> > achieving "RPH outlives DataReductionProxyHostImpl", so should have no
> problem..
> >  Maybe I'm saying something stupid,, but just my understanding now ;-) 
> > 2,
> > Let DataReductionProxyHostImpl be a RPH observer so that it can connect
> > lifecycle with RPH. Once got RPH destroyed notification, post a task to IO
> > thread to delete itself.
> >  
> > WDYT? Thanks. If option1 is nonsense, I'll try option2.
> 
> The lifetime in the renderer doesn't directly correspond to lifetime in the
> browser. IsDataReductionProxy() won't be called synchronously when the pipe is
> bound and neither will the disconnect.
> 
> On the other hand, using a RenderProcessHostObserver would be messy at
> destruction time if the StrongBinding still lives on the IO thread as the UI
and
> IO threads could both decide to delete this at the same time.

Option1, I think the renderer thread will block on calling
IsDataReductionProxy() until pipe bound and IsDataReductionProxy() execution
returned in browser side. Within this timeslot, browser side RPH should be alive
and DataReductionProxyConfig should be guaranteed available.

Option2, yes it would be messy and hard to solve the race. Let me think more..

[Anand Mistry (off Chromium), 2016-06-20 11:18:17]

On 2016/06/20 10:51:51, leonhsl wrote:
>
https://codereview.chromium.org/2063683002/diff/60001/components/data_reducti...
> File
>
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.cc
> (right):
> 
>
https://codereview.chromium.org/2063683002/diff/60001/components/data_reducti...
>
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.cc:28:
> config_ = settings->Config();
> On 2016/06/20 04:57:26, Sam McNally wrote:
> > DataReductionProxySettings can only be used on the UI thread.
> 
> Oh yes, we can pass settings->Config() instead of settings into the
constructor.
> 
>
https://codereview.chromium.org/2063683002/diff/60001/components/data_reducti...
> File
>
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.h
> (right):
> 
>
https://codereview.chromium.org/2063683002/diff/60001/components/data_reducti...
>
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.h:37:
> // Must outlive |this|. May be null.
> On 2016/06/20 04:57:26, Sam McNally wrote:
> > On 2016/06/17 08:43:57, leonhsl wrote:
> > > On 2016/06/16 09:04:04, Anand Mistry wrote:
> > > > How can you guarantee this? The legacy IPC achieved this because the
> message
> > > > filter is attached to the RenderProcessHost and will be destroyed when
the
> > RPH
> > > > is gone. I'm assuming the RPH outlives the Profile. However, this
class's
> > > > lifecycle is disconnected from the RPH and instead governed by the life
of
> > the
> > > > message pipe.
> > > 
> > > Yeah it is, the lifecycle is the difference with before. Thanks a lot for
> > > pointing out this. I investigated around and I think we may have 2 options
> > now:
> > > 1, 
> > > Just for current use case, client side(PageLoadHistograms::Dump() on
> renderer
> > > thread) executes "Connect-->Sync call-->Disconnect" all at once, message
> pipe
> > > lifetime is just this timeslot, and the same to 
DataReductionProxyHostImpl,
> > > while within this timeslot the browser side RPH should just be keeping
> > > alive(because renderer thread is running one task). This means we're still
> > > achieving "RPH outlives DataReductionProxyHostImpl", so should have no
> > problem..
> > >  Maybe I'm saying something stupid,, but just my understanding now ;-) 
> > > 2,
> > > Let DataReductionProxyHostImpl be a RPH observer so that it can connect
> > > lifecycle with RPH. Once got RPH destroyed notification, post a task to IO
> > > thread to delete itself.
> > >  
> > > WDYT? Thanks. If option1 is nonsense, I'll try option2.
> > 
> > The lifetime in the renderer doesn't directly correspond to lifetime in the
> > browser. IsDataReductionProxy() won't be called synchronously when the pipe
is
> > bound and neither will the disconnect.
> > 
> > On the other hand, using a RenderProcessHostObserver would be messy at
> > destruction time if the StrongBinding still lives on the IO thread as the UI
> and
> > IO threads could both decide to delete this at the same time.
> 
> Option1, I think the renderer thread will block on calling
> IsDataReductionProxy() until pipe bound and IsDataReductionProxy() execution
> returned in browser side. Within this timeslot, browser side RPH should be
alive
> and DataReductionProxyConfig should be guaranteed available.

What you say only applies to the renderer process. If the profile in the browser
is shut down right after the renderer sends that IPC, you may end up in a
situation where the browser may receive and start processing that IPC after the
profile and RPH has been deleted. This is because the binding lifecycle isn't
bound in any way to the RPH. And receipt of the IPC does NOT imply the renderer
is still alive.

> 
> Option2, yes it would be messy and hard to solve the race. Let me think more..

[leonhsl(Using Gerrit), 2016-06-20 14:26:57]

On 2016/06/20 11:18:17, Anand Mistry wrote:
> On 2016/06/20 10:51:51, leonhsl wrote:
> >
>
https://codereview.chromium.org/2063683002/diff/60001/components/data_reducti...
> > File
> >
>
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/2063683002/diff/60001/components/data_reducti...
> >
>
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.cc:28:
> > config_ = settings->Config();
> > On 2016/06/20 04:57:26, Sam McNally wrote:
> > > DataReductionProxySettings can only be used on the UI thread.
> > 
> > Oh yes, we can pass settings->Config() instead of settings into the
> constructor.
> > 
> >
>
https://codereview.chromium.org/2063683002/diff/60001/components/data_reducti...
> > File
> >
>
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.h
> > (right):
> > 
> >
>
https://codereview.chromium.org/2063683002/diff/60001/components/data_reducti...
> >
>
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.h:37:
> > // Must outlive |this|. May be null.
> > On 2016/06/20 04:57:26, Sam McNally wrote:
> > > On 2016/06/17 08:43:57, leonhsl wrote:
> > > > On 2016/06/16 09:04:04, Anand Mistry wrote:
> > > > > How can you guarantee this? The legacy IPC achieved this because the
> > message
> > > > > filter is attached to the RenderProcessHost and will be destroyed when
> the
> > > RPH
> > > > > is gone. I'm assuming the RPH outlives the Profile. However, this
> class's
> > > > > lifecycle is disconnected from the RPH and instead governed by the
life
> of
> > > the
> > > > > message pipe.
> > > > 
> > > > Yeah it is, the lifecycle is the difference with before. Thanks a lot
for
> > > > pointing out this. I investigated around and I think we may have 2
options
> > > now:
> > > > 1, 
> > > > Just for current use case, client side(PageLoadHistograms::Dump() on
> > renderer
> > > > thread) executes "Connect-->Sync call-->Disconnect" all at once, message
> > pipe
> > > > lifetime is just this timeslot, and the same to 
> DataReductionProxyHostImpl,
> > > > while within this timeslot the browser side RPH should just be keeping
> > > > alive(because renderer thread is running one task). This means we're
still
> > > > achieving "RPH outlives DataReductionProxyHostImpl", so should have no
> > > problem..
> > > >  Maybe I'm saying something stupid,, but just my understanding now ;-) 
> > > > 2,
> > > > Let DataReductionProxyHostImpl be a RPH observer so that it can connect
> > > > lifecycle with RPH. Once got RPH destroyed notification, post a task to
IO
> > > > thread to delete itself.
> > > >  
> > > > WDYT? Thanks. If option1 is nonsense, I'll try option2.
> > > 
> > > The lifetime in the renderer doesn't directly correspond to lifetime in
the
> > > browser. IsDataReductionProxy() won't be called synchronously when the
pipe
> is
> > > bound and neither will the disconnect.
> > > 
> > > On the other hand, using a RenderProcessHostObserver would be messy at
> > > destruction time if the StrongBinding still lives on the IO thread as the
UI
> > and
> > > IO threads could both decide to delete this at the same time.
> > 
> > Option1, I think the renderer thread will block on calling
> > IsDataReductionProxy() until pipe bound and IsDataReductionProxy() execution
> > returned in browser side. Within this timeslot, browser side RPH should be
> alive
> > and DataReductionProxyConfig should be guaranteed available.
> 
> What you say only applies to the renderer process. If the profile in the
browser
> is shut down right after the renderer sends that IPC, you may end up in a
> situation where the browser may receive and start processing that IPC after
the
> profile and RPH has been deleted. This is because the binding lifecycle isn't
> bound in any way to the RPH. And receipt of the IPC does NOT imply the
renderer
> is still alive.
> 

If that scenario happened, as RPH has already been deleted, so RPH's service
registry has also disappeared, that means DataReductionProxyHost interface
connection itself can't be established. This result is similar with legacy IPC,
RPH disappeared, so did browser message filter, then received
IsDataReductionProxy IPC will just be ignored.

> > 
> > Option2, yes it would be messy and hard to solve the race. Let me think
more..

[leonhsl(Using Gerrit), 2016-06-22 02:52:58]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-22 02:53:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2063683002/80001

[commit-bot: I haz the power, 2016-06-22 03:00:30]

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

[commit-bot: I haz the power, 2016-06-22 03:00:31]

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

[Anand Mistry (off Chromium), 2016-06-22 05:49:26]

On 2016/06/20 14:26:57, leonhsl wrote:
> On 2016/06/20 11:18:17, Anand Mistry wrote:
> > On 2016/06/20 10:51:51, leonhsl wrote:
> > >
> >
>
https://codereview.chromium.org/2063683002/diff/60001/components/data_reducti...
> > > File
> > >
> >
>
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.cc
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2063683002/diff/60001/components/data_reducti...
> > >
> >
>
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.cc:28:
> > > config_ = settings->Config();
> > > On 2016/06/20 04:57:26, Sam McNally wrote:
> > > > DataReductionProxySettings can only be used on the UI thread.
> > > 
> > > Oh yes, we can pass settings->Config() instead of settings into the
> > constructor.
> > > 
> > >
> >
>
https://codereview.chromium.org/2063683002/diff/60001/components/data_reducti...
> > > File
> > >
> >
>
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.h
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2063683002/diff/60001/components/data_reducti...
> > >
> >
>
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.h:37:
> > > // Must outlive |this|. May be null.
> > > On 2016/06/20 04:57:26, Sam McNally wrote:
> > > > On 2016/06/17 08:43:57, leonhsl wrote:
> > > > > On 2016/06/16 09:04:04, Anand Mistry wrote:
> > > > > > How can you guarantee this? The legacy IPC achieved this because the
> > > message
> > > > > > filter is attached to the RenderProcessHost and will be destroyed
when
> > the
> > > > RPH
> > > > > > is gone. I'm assuming the RPH outlives the Profile. However, this
> > class's
> > > > > > lifecycle is disconnected from the RPH and instead governed by the
> life
> > of
> > > > the
> > > > > > message pipe.
> > > > > 
> > > > > Yeah it is, the lifecycle is the difference with before. Thanks a lot
> for
> > > > > pointing out this. I investigated around and I think we may have 2
> options
> > > > now:
> > > > > 1, 
> > > > > Just for current use case, client side(PageLoadHistograms::Dump() on
> > > renderer
> > > > > thread) executes "Connect-->Sync call-->Disconnect" all at once,
message
> > > pipe
> > > > > lifetime is just this timeslot, and the same to 
> > DataReductionProxyHostImpl,
> > > > > while within this timeslot the browser side RPH should just be keeping
> > > > > alive(because renderer thread is running one task). This means we're
> still
> > > > > achieving "RPH outlives DataReductionProxyHostImpl", so should have no
> > > > problem..
> > > > >  Maybe I'm saying something stupid,, but just my understanding now ;-)

> > > > > 2,
> > > > > Let DataReductionProxyHostImpl be a RPH observer so that it can
connect
> > > > > lifecycle with RPH. Once got RPH destroyed notification, post a task
to
> IO
> > > > > thread to delete itself.
> > > > >  
> > > > > WDYT? Thanks. If option1 is nonsense, I'll try option2.
> > > > 
> > > > The lifetime in the renderer doesn't directly correspond to lifetime in
> the
> > > > browser. IsDataReductionProxy() won't be called synchronously when the
> pipe
> > is
> > > > bound and neither will the disconnect.
> > > > 
> > > > On the other hand, using a RenderProcessHostObserver would be messy at
> > > > destruction time if the StrongBinding still lives on the IO thread as
the
> UI
> > > and
> > > > IO threads could both decide to delete this at the same time.
> > > 
> > > Option1, I think the renderer thread will block on calling
> > > IsDataReductionProxy() until pipe bound and IsDataReductionProxy()
execution
> > > returned in browser side. Within this timeslot, browser side RPH should be
> > alive
> > > and DataReductionProxyConfig should be guaranteed available.
> > 
> > What you say only applies to the renderer process. If the profile in the
> browser
> > is shut down right after the renderer sends that IPC, you may end up in a
> > situation where the browser may receive and start processing that IPC after
> the
> > profile and RPH has been deleted. This is because the binding lifecycle
isn't
> > bound in any way to the RPH. And receipt of the IPC does NOT imply the
> renderer
> > is still alive.
> > 
> 
> If that scenario happened, as RPH has already been deleted, so RPH's service
> registry has also disappeared, that means DataReductionProxyHost interface
> connection itself can't be established. This result is similar with legacy
IPC,
> RPH disappeared, so did browser message filter, then received
> IsDataReductionProxy IPC will just be ignored.

Nope. The RPH can be deleted after the impl has been created, but before a
method is called. This is good argument that StrongBinding<> is not appropriate
here because the lifecycle isn't disconnected from the RPH (or profile).
StrongBinding<> only works for things where the lifecycle of the impl is
decoupled from anything else.

> 
> > > 
> > > Option2, yes it would be messy and hard to solve the race. Let me think
> more..

[leonhsl(Using Gerrit), 2016-06-22 07:38:06]

On 2016/06/22 05:49:26, Anand Mistry wrote:
> On 2016/06/20 14:26:57, leonhsl wrote:
> > On 2016/06/20 11:18:17, Anand Mistry wrote:
> > > On 2016/06/20 10:51:51, leonhsl wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2063683002/diff/60001/components/data_reducti...
> > > > File
> > > >
> > >
> >
>
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.cc
> > > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2063683002/diff/60001/components/data_reducti...
> > > >
> > >
> >
>
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.cc:28:
> > > > config_ = settings->Config();
> > > > On 2016/06/20 04:57:26, Sam McNally wrote:
> > > > > DataReductionProxySettings can only be used on the UI thread.
> > > > 
> > > > Oh yes, we can pass settings->Config() instead of settings into the
> > > constructor.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2063683002/diff/60001/components/data_reducti...
> > > > File
> > > >
> > >
> >
>
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.h
> > > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2063683002/diff/60001/components/data_reducti...
> > > >
> > >
> >
>
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.h:37:
> > > > // Must outlive |this|. May be null.
> > > > On 2016/06/20 04:57:26, Sam McNally wrote:
> > > > > On 2016/06/17 08:43:57, leonhsl wrote:
> > > > > > On 2016/06/16 09:04:04, Anand Mistry wrote:
> > > > > > > How can you guarantee this? The legacy IPC achieved this because
the
> > > > message
> > > > > > > filter is attached to the RenderProcessHost and will be destroyed
> when
> > > the
> > > > > RPH
> > > > > > > is gone. I'm assuming the RPH outlives the Profile. However, this
> > > class's
> > > > > > > lifecycle is disconnected from the RPH and instead governed by the
> > life
> > > of
> > > > > the
> > > > > > > message pipe.
> > > > > > 
> > > > > > Yeah it is, the lifecycle is the difference with before. Thanks a
lot
> > for
> > > > > > pointing out this. I investigated around and I think we may have 2
> > options
> > > > > now:
> > > > > > 1, 
> > > > > > Just for current use case, client side(PageLoadHistograms::Dump() on
> > > > renderer
> > > > > > thread) executes "Connect-->Sync call-->Disconnect" all at once,
> message
> > > > pipe
> > > > > > lifetime is just this timeslot, and the same to 
> > > DataReductionProxyHostImpl,
> > > > > > while within this timeslot the browser side RPH should just be
keeping
> > > > > > alive(because renderer thread is running one task). This means we're
> > still
> > > > > > achieving "RPH outlives DataReductionProxyHostImpl", so should have
no
> > > > > problem..
> > > > > >  Maybe I'm saying something stupid,, but just my understanding now
;-)
> 
> > > > > > 2,
> > > > > > Let DataReductionProxyHostImpl be a RPH observer so that it can
> connect
> > > > > > lifecycle with RPH. Once got RPH destroyed notification, post a task
> to
> > IO
> > > > > > thread to delete itself.
> > > > > >  
> > > > > > WDYT? Thanks. If option1 is nonsense, I'll try option2.
> > > > > 
> > > > > The lifetime in the renderer doesn't directly correspond to lifetime
in
> > the
> > > > > browser. IsDataReductionProxy() won't be called synchronously when the
> > pipe
> > > is
> > > > > bound and neither will the disconnect.
> > > > > 
> > > > > On the other hand, using a RenderProcessHostObserver would be messy at
> > > > > destruction time if the StrongBinding still lives on the IO thread as
> the
> > UI
> > > > and
> > > > > IO threads could both decide to delete this at the same time.
> > > > 
> > > > Option1, I think the renderer thread will block on calling
> > > > IsDataReductionProxy() until pipe bound and IsDataReductionProxy()
> execution
> > > > returned in browser side. Within this timeslot, browser side RPH should
be
> > > alive
> > > > and DataReductionProxyConfig should be guaranteed available.
> > > 
> > > What you say only applies to the renderer process. If the profile in the
> > browser
> > > is shut down right after the renderer sends that IPC, you may end up in a
> > > situation where the browser may receive and start processing that IPC
after
> > the
> > > profile and RPH has been deleted. This is because the binding lifecycle
> isn't
> > > bound in any way to the RPH. And receipt of the IPC does NOT imply the
> > renderer
> > > is still alive.
> > > 
> > 
> > If that scenario happened, as RPH has already been deleted, so RPH's service
> > registry has also disappeared, that means DataReductionProxyHost interface
> > connection itself can't be established. This result is similar with legacy
> IPC,
> > RPH disappeared, so did browser message filter, then received
> > IsDataReductionProxy IPC will just be ignored.
> 
> Nope. The RPH can be deleted after the impl has been created, but before a
> method is called. This is good argument that StrongBinding<> is not
appropriate
> here because the lifecycle isn't disconnected from the RPH (or profile).
> StrongBinding<> only works for things where the lifecycle of the impl is
> decoupled from anything else.
> 

Ok this scenario is actually possible.. Thank you~
And I've got an idea which I think finally should have no problem anymore:
1, 
Let DataReductionProxyHostImpl be a RPH observer, once got RPH destroy
notification, post a task to IO thread to delete self. This would ensure
DataReductionProxyConfig always outlives our DataReductionProxyHostImpl, just
like the case of RPH message filter before. Because DataReductionProxyConfig is
also deleted on IO thread by the task posted from Profile destruction, but as
Profile destruction is later than RPH destruction, DataReductionProxyConfig
destruction will also be later than our DataReductionProxyHostImpl.
2,
Just use Binding<> instead of StrongBinding<>, create DataReductionProxyHostImpl
instance when we register the interface, keep its 1:1 relationship with RPH.

WDYT?

> > 
> > > > 
> > > > Option2, yes it would be messy and hard to solve the race. Let me think
> > more..

[leonhsl(Using Gerrit), 2016-06-22 10:22:22]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-22 10:22:45]

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

[commit-bot: I haz the power, 2016-06-22 10:38:10]

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

[commit-bot: I haz the power, 2016-06-22 10:38:12]

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

[leonhsl(Using Gerrit), 2016-06-23 02:58:25]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-23 02:58:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2063683002/140001

[commit-bot: I haz the power, 2016-06-23 03:11:22]

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

[commit-bot: I haz the power, 2016-06-23 03:11:23]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[leonhsl(Using Gerrit), 2016-06-23 03:54:01]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-23 03:54:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2063683002/160001

[commit-bot: I haz the power, 2016-06-23 06:59:51]

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

[commit-bot: I haz the power, 2016-06-23 06:59:52]

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

[leonhsl(Using Gerrit), 2016-06-23 07:16:37]

Would you PTAnL at ps#9? Thanks.

Now DataReductionProxyHostImpl is a RPH observer and ties its  lifecycle with
RPH instead of the message pipe bound.

https://codereview.chromium.org/2063683002/diff/60001/components/data_reducti...
File
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.cc
(right):

https://codereview.chromium.org/2063683002/diff/60001/components/data_reducti...
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.cc:28:
config_ = settings->Config();
On 2016/06/20 10:51:51, leonhsl wrote:
> On 2016/06/20 04:57:26, Sam McNally wrote:
> > DataReductionProxySettings can only be used on the UI thread.
> 
> Oh yes, we can pass settings->Config() instead of settings into the
constructor.

Done.

[leonhsl(Using Gerrit), 2016-06-23 07:37:39]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-23 07:37:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2063683002/160001

[leonhsl(Using Gerrit), 2016-06-23 09:37:06]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-23 09:37:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2063683002/180001

[commit-bot: I haz the power, 2016-06-23 10:35:08]

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

[commit-bot: I haz the power, 2016-06-23 10:35:09]

Dry run: This issue passed the CQ dry run.

[Anand Mistry (off Chromium), 2016-06-24 07:12:28]

https://codereview.chromium.org/2063683002/diff/180001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2063683002/diff/180001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:2723:
DataReductionProxyChromeSettingsFactory::GetForBrowserContext(profile);
I'm personally of the opinion that as much as possible, we should be lazily
creating impls. However, others have differing views. I'll ask sam to comment on
this.

https://codereview.chromium.org/2063683002/diff/180001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:2730: // The
data_reduction_proxy_host is a render process host observer, so here we
I'm convinced this is technically correct. However, the reason why is subtle
which makes me not like this approach. Basically, you're relying on PostTask
ordering between the deletion in the RPH destruction observer and the bind in
ServiceRegistryImpl. You're guaranteed that the service registry post will
always happen before the destruction (because RPH destruction kills the service
registry), and therefore any call to DRPHI::BindRequest will always happen
before the DRPHI is deleted.

Another approach is to always have the DataReductionProxyHostImpl live and
service requests on the UI thread. That way, you can delete when the RPH
observer fires and avoid any lifetime complexity. To service the
IsDataReductionProxy() method, you'll need to post to the IO thread and post
back to the UI thread to respond, but you can use a WeakPtr to ensure lifetime
and this would make lifetime reasoning much simpler.

https://codereview.chromium.org/2063683002/diff/180001/chrome/common/chrome_c...
File chrome/common/chrome_content_client.cc (left):

https://codereview.chromium.org/2063683002/diff/180001/chrome/common/chrome_c...
chrome/common/chrome_content_client.cc:605: bool
ChromeContentClient::CanSendWhileSwappedOut(const IPC::Message* message) {
I honestly don't understand what's supposed to happen with swapped-out frames.
Someone that understand the effect of this should really take a look.

https://codereview.chromium.org/2063683002/diff/180001/chrome/common/chrome_c...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/2063683002/diff/180001/chrome/common/chrome_c...
chrome/common/chrome_content_client.cc:16: #include "base/memory/ptr_util.h"
Is this needed? I'm guessing the messages file brought this in transitively and
now you need it. Is that right?

https://codereview.chromium.org/2063683002/diff/180001/components/data_reduct...
File
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.h
(right):

https://codereview.chromium.org/2063683002/diff/180001/components/data_reduct...
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.h:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
Ahem! Unless you've got a time machine I can borrow :P

https://codereview.chromium.org/2063683002/diff/180001/components/data_reduct...
File
components/data_reduction_proxy/content/common/data_reduction_proxy_messages.h
(left):

https://codereview.chromium.org/2063683002/diff/180001/components/data_reduct...
components/data_reduction_proxy/content/common/data_reduction_proxy_messages.h:10:
#define IPC_MESSAGE_START DataReductionProxyStart
Also remove this from the main messages file.

[Sam McNally, 2016-06-24 07:24:48]

https://codereview.chromium.org/2063683002/diff/180001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2063683002/diff/180001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:2723:
DataReductionProxyChromeSettingsFactory::GetForBrowserContext(profile);
On 2016/06/24 07:12:28, Anand Mistry wrote:
> I'm personally of the opinion that as much as possible, we should be lazily
> creating impls. However, others have differing views. I'll ask sam to comment
on
> this.

While it would be nicer to create them lazily, it's simpler to create them ahead
of time. If we create something to abstract away this complexity, then I think
we can recommend lazy creation.

https://codereview.chromium.org/2063683002/diff/180001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:2730: // The
data_reduction_proxy_host is a render process host observer, so here we
On 2016/06/24 07:12:28, Anand Mistry wrote:
> I'm convinced this is technically correct. However, the reason why is subtle
> which makes me not like this approach. Basically, you're relying on PostTask
> ordering between the deletion in the RPH destruction observer and the bind in
> ServiceRegistryImpl. You're guaranteed that the service registry post will
> always happen before the destruction (because RPH destruction kills the
service
> registry), and therefore any call to DRPHI::BindRequest will always happen
> before the DRPHI is deleted.
> 
> Another approach is to always have the DataReductionProxyHostImpl live and
> service requests on the UI thread. That way, you can delete when the RPH
> observer fires and avoid any lifetime complexity. To service the
> IsDataReductionProxy() method, you'll need to post to the IO thread and post
> back to the UI thread to respond, but you can use a WeakPtr to ensure lifetime
> and this would make lifetime reasoning much simpler.

This feels like a lot of unnecessary thread bouncing, and may not work for all
MessageFilter conversions. I would prefer we find a way to do this nicely for
interface implementations that don't live on the UI thread. That said, exposing
this complexity in each converted MessageFilter does sound like an invitation
for confusion and bugs further down the road.

[leonhsl(Using Gerrit), 2016-06-24 09:37:44]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-24 09:37:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2063683002/220001

[leonhsl(Using Gerrit), 2016-06-24 09:59:21]

Hi, Anand, Sam, Thanks a lot for comments! Uploaded ps#11 to address comments
and ps#12 to do rebase, PTAnL, Thanks.

https://codereview.chromium.org/2063683002/diff/180001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2063683002/diff/180001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:2730: // The
data_reduction_proxy_host is a render process host observer, so here we
On 2016/06/24 07:24:48, Sam McNally wrote:
> On 2016/06/24 07:12:28, Anand Mistry wrote:
> > I'm convinced this is technically correct. However, the reason why is subtle
> > which makes me not like this approach. Basically, you're relying on PostTask
> > ordering between the deletion in the RPH destruction observer and the bind
in
> > ServiceRegistryImpl. You're guaranteed that the service registry post will
> > always happen before the destruction (because RPH destruction kills the
> service
> > registry), and therefore any call to DRPHI::BindRequest will always happen
> > before the DRPHI is deleted.
> > 
> > Another approach is to always have the DataReductionProxyHostImpl live and
> > service requests on the UI thread. That way, you can delete when the RPH
> > observer fires and avoid any lifetime complexity. To service the
> > IsDataReductionProxy() method, you'll need to post to the IO thread and post
> > back to the UI thread to respond, but you can use a WeakPtr to ensure
lifetime
> > and this would make lifetime reasoning much simpler.
> 
> This feels like a lot of unnecessary thread bouncing, and may not work for all
> MessageFilter conversions. I would prefer we find a way to do this nicely for
> interface implementations that don't live on the UI thread. That said,
exposing
> this complexity in each converted MessageFilter does sound like an invitation
> for confusion and bugs further down the road.

En,, if let DataReductionProxyHostImpl live on UI thread, this maybe cause UI
thread get blocked sometimes when IO thread is busy. As IsDataReductionProxy()
is a sync call.

https://codereview.chromium.org/2063683002/diff/180001/chrome/common/chrome_c...
File chrome/common/chrome_content_client.cc (left):

https://codereview.chromium.org/2063683002/diff/180001/chrome/common/chrome_c...
chrome/common/chrome_content_client.cc:605: bool
ChromeContentClient::CanSendWhileSwappedOut(const IPC::Message* message) {
On 2016/06/24 07:12:28, Anand Mistry wrote:
> I honestly don't understand what's supposed to happen with swapped-out frames.
> Someone that understand the effect of this should really take a look.

From my understanding, under swapped-out status RenderWidget::Send() can send
only some explicitly announced IPCs, here is just the annunciation for
DataReductionProxyViewHostMsg_IsDataReductionProxy. After switched to Mojo, the
restriction itself does not exist anymore, so I think we need to do nothing for
this.
Anyway, I'll try to find someone familiar with this to take a look.

https://codereview.chromium.org/2063683002/diff/180001/chrome/common/chrome_c...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/2063683002/diff/180001/chrome/common/chrome_c...
chrome/common/chrome_content_client.cc:16: #include "base/memory/ptr_util.h"
On 2016/06/24 07:12:28, Anand Mistry wrote:
> Is this needed? I'm guessing the messages file brought this in transitively
and
> now you need it. Is that right?

Actually it's not my codes need it ;-)
https://codereview.chromium.org/2049783002 started to use base::WrapUnique but
did not include this header file explicitly, it implicitly uses via the message
file I delete here in this CL. To let all trybots happy I have to add this line
here. I did a rebase on ps#12, which will show the usage of base::WrapUnique
clearly.

Although, I've created a separate CL https://codereview.chromium.org/2095473003/
to add this explicitly.

https://codereview.chromium.org/2063683002/diff/180001/components/data_reduct...
File
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.h
(right):

https://codereview.chromium.org/2063683002/diff/180001/components/data_reduct...
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.h:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
On 2016/06/24 07:12:28, Anand Mistry wrote:
> Ahem! Unless you've got a time machine I can borrow :P

Ehh,, Done! Thank you ;-)

https://codereview.chromium.org/2063683002/diff/180001/components/data_reduct...
File
components/data_reduction_proxy/content/common/data_reduction_proxy_messages.h
(left):

https://codereview.chromium.org/2063683002/diff/180001/components/data_reduct...
components/data_reduction_proxy/content/common/data_reduction_proxy_messages.h:10:
#define IPC_MESSAGE_START DataReductionProxyStart
On 2016/06/24 07:12:28, Anand Mistry wrote:
> Also remove this from the main messages file.

Done.

[commit-bot: I haz the power, 2016-06-24 11:20:11]

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

[commit-bot: I haz the power, 2016-06-24 11:20:13]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-06-24 14:46:11]

https://codereview.chromium.org/2063683002/diff/180001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2063683002/diff/180001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:2730: // The
data_reduction_proxy_host is a render process host observer, so here we
On 2016/06/24 09:59:21, leonhsl wrote:
> On 2016/06/24 07:24:48, Sam McNally wrote:
> > On 2016/06/24 07:12:28, Anand Mistry wrote:
> > > I'm convinced this is technically correct. However, the reason why is
subtle
> > > which makes me not like this approach. Basically, you're relying on
PostTask
> > > ordering between the deletion in the RPH destruction observer and the bind
> in
> > > ServiceRegistryImpl. You're guaranteed that the service registry post will
> > > always happen before the destruction (because RPH destruction kills the
> > service
> > > registry), and therefore any call to DRPHI::BindRequest will always happen
> > > before the DRPHI is deleted.
> > > 
> > > Another approach is to always have the DataReductionProxyHostImpl live and
> > > service requests on the UI thread. That way, you can delete when the RPH
> > > observer fires and avoid any lifetime complexity. To service the
> > > IsDataReductionProxy() method, you'll need to post to the IO thread and
post
> > > back to the UI thread to respond, but you can use a WeakPtr to ensure
> lifetime
> > > and this would make lifetime reasoning much simpler.
> > 
> > This feels like a lot of unnecessary thread bouncing, and may not work for
all
> > MessageFilter conversions. I would prefer we find a way to do this nicely
for
> > interface implementations that don't live on the UI thread. That said,
> exposing
> > this complexity in each converted MessageFilter does sound like an
invitation
> > for confusion and bugs further down the road.
> 
> En,, if let DataReductionProxyHostImpl live on UI thread, this maybe cause UI
> thread get blocked sometimes when IO thread is busy. As IsDataReductionProxy()
> is a sync call.

Sorry for my misunderstanding, you could just ignore my nonsense words.. I'll
try to consider about the nicer way mentioned by Sam.

[leonhsl(Using Gerrit), 2016-06-24 15:33:46]

leon.han@intel.com changed reviewers:
+ tbansal@chromium.org

[leonhsl(Using Gerrit), 2016-06-24 15:33:47]

Hi, tbansal@, megjablon@, in addition to the following inline question, I want
to confirm another one:
As for 1 render process host, will there be multiple render frames sending the
IsDataReductionProxy IPC to it? Thanks in advance.

https://codereview.chromium.org/2063683002/diff/180001/chrome/common/chrome_c...
File chrome/common/chrome_content_client.cc (left):

https://codereview.chromium.org/2063683002/diff/180001/chrome/common/chrome_c...
chrome/common/chrome_content_client.cc:605: bool
ChromeContentClient::CanSendWhileSwappedOut(const IPC::Message* message) {
On 2016/06/24 09:59:21, leonhsl wrote:
> On 2016/06/24 07:12:28, Anand Mistry wrote:
> > I honestly don't understand what's supposed to happen with swapped-out
frames.
> > Someone that understand the effect of this should really take a look.
> 
> From my understanding, under swapped-out status RenderWidget::Send() can send
> only some explicitly announced IPCs, here is just the annunciation for
> DataReductionProxyViewHostMsg_IsDataReductionProxy. After switched to Mojo,
the
> restriction itself does not exist anymore, so I think we need to do nothing
for
> this.
> Anyway, I'll try to find someone familiar with this to take a look.

Hi, tbansal@, megjablon@, I found that you have touched these codes before,
would you please help to take a look? Thanks.

[tbansal1, 2016-06-24 15:56:50]

On 2016/06/24 15:33:47, leonhsl wrote:
> Hi, tbansal@, megjablon@, in addition to the following inline question, I want
> to confirm another one:
> As for 1 render process host, will there be multiple render frames sending the
> IsDataReductionProxy IPC to it? Thanks in advance.
The message is send once per page load.

If the two render frames are part of same process (e.g., example.com
loaded in two different tabs may share render process), then the
render process host will get 2 IPCs.

> 
>
https://codereview.chromium.org/2063683002/diff/180001/chrome/common/chrome_c...
> File chrome/common/chrome_content_client.cc (left):
> 
>
https://codereview.chromium.org/2063683002/diff/180001/chrome/common/chrome_c...
> chrome/common/chrome_content_client.cc:605: bool
> ChromeContentClient::CanSendWhileSwappedOut(const IPC::Message* message) {
> On 2016/06/24 09:59:21, leonhsl wrote:
> > On 2016/06/24 07:12:28, Anand Mistry wrote:
> > > I honestly don't understand what's supposed to happen with swapped-out
> frames.
> > > Someone that understand the effect of this should really take a look.
> > 
> > From my understanding, under swapped-out status RenderWidget::Send() can
send
> > only some explicitly announced IPCs, here is just the annunciation for
> > DataReductionProxyViewHostMsg_IsDataReductionProxy. After switched to Mojo,
> the
> > restriction itself does not exist anymore, so I think we need to do nothing
> for
> > this.
> > Anyway, I'll try to find someone familiar with this to take a look.
> 
> Hi, tbansal@, megjablon@, I found that you have touched these codes before,
> would you please help to take a look? Thanks.

[tbansal1, 2016-06-24 15:57:08]

https://codereview.chromium.org/2063683002/diff/180001/chrome/common/chrome_c...
File chrome/common/chrome_content_client.cc (left):

https://codereview.chromium.org/2063683002/diff/180001/chrome/common/chrome_c...
chrome/common/chrome_content_client.cc:605: bool
ChromeContentClient::CanSendWhileSwappedOut(const IPC::Message* message) {
On 2016/06/24 15:33:47, leonhsl wrote:
> On 2016/06/24 09:59:21, leonhsl wrote:
> > On 2016/06/24 07:12:28, Anand Mistry wrote:
> > > I honestly don't understand what's supposed to happen with swapped-out
> frames.
> > > Someone that understand the effect of this should really take a look.
> > 
> > From my understanding, under swapped-out status RenderWidget::Send() can
send
> > only some explicitly announced IPCs, here is just the annunciation for
> > DataReductionProxyViewHostMsg_IsDataReductionProxy. After switched to Mojo,
> the
> > restriction itself does not exist anymore, so I think we need to do nothing
> for
> > this.
> > Anyway, I'll try to find someone familiar with this to take a look.
> 
> Hi, tbansal@, megjablon@, I found that you have touched these codes before,
> would you please help to take a look? Thanks.

leonhsi@, you are right. By default the IPCs are blocked for swapped out frames.
Here, we are trying to record page load metrics (e.g., page load time) if the
page used the data reduction proxy.

So, the renderer queries the browser if the page used the data reduction proxy.
This code punches a hole in the IPC mechanism to allow this message to go
through even after the swap out.

I am not familiar with Mojo, but if the IPC restriction (messages are blocked
after renderer is swapped out) is no longer there, then this code can be
removed.

[leonhsl(Using Gerrit), 2016-06-24 16:08:20]

On 2016/06/24 15:57:08, tbansal1 wrote:
>
https://codereview.chromium.org/2063683002/diff/180001/chrome/common/chrome_c...
> File chrome/common/chrome_content_client.cc (left):
> 
>
https://codereview.chromium.org/2063683002/diff/180001/chrome/common/chrome_c...
> chrome/common/chrome_content_client.cc:605: bool
> ChromeContentClient::CanSendWhileSwappedOut(const IPC::Message* message) {
> On 2016/06/24 15:33:47, leonhsl wrote:
> > On 2016/06/24 09:59:21, leonhsl wrote:
> > > On 2016/06/24 07:12:28, Anand Mistry wrote:
> > > > I honestly don't understand what's supposed to happen with swapped-out
> > frames.
> > > > Someone that understand the effect of this should really take a look.
> > > 
> > > From my understanding, under swapped-out status RenderWidget::Send() can
> send
> > > only some explicitly announced IPCs, here is just the annunciation for
> > > DataReductionProxyViewHostMsg_IsDataReductionProxy. After switched to
Mojo,
> > the
> > > restriction itself does not exist anymore, so I think we need to do
nothing
> > for
> > > this.
> > > Anyway, I'll try to find someone familiar with this to take a look.
> > 
> > Hi, tbansal@, megjablon@, I found that you have touched these codes before,
> > would you please help to take a look? Thanks.
> 
> leonhsi@, you are right. By default the IPCs are blocked for swapped out
frames.
> Here, we are trying to record page load metrics (e.g., page load time) if the
> page used the data reduction proxy.
> 
> So, the renderer queries the browser if the page used the data reduction
proxy.
> This code punches a hole in the IPC mechanism to allow this message to go
> through even after the swap out.
> 
> I am not familiar with Mojo, but if the IPC restriction (messages are blocked
> after renderer is swapped out) is no longer there, then this code can be
> removed.

Got it. Thanks a lot for quick response!

[leonhsl(Using Gerrit), 2016-06-24 16:20:59]

On 2016/06/24 15:56:50, tbansal1 wrote:
> On 2016/06/24 15:33:47, leonhsl wrote:
> > Hi, tbansal@, megjablon@, in addition to the following inline question, I
want
> > to confirm another one:
> > As for 1 render process host, will there be multiple render frames sending
the
> > IsDataReductionProxy IPC to it? Thanks in advance.
> The message is send once per page load.
> 
> If the two render frames are part of same process (e.g., http://example.com
> loaded in two different tabs may share render process), then the
> render process host will get 2 IPCs.
> 

OK I see, then I need to enable our DataReductionProxyHostImpl serve multiple
clients. Thanks!
And, sorry for another question not related with this CL ;-) Can multiple render
frames share 1 render frame host?

[tbansal1, 2016-06-24 18:38:40]

+creis to answer your last question

[Charlie Reis, 2016-06-24 23:00:20]

On 2016/06/24 16:20:59, leonhsl wrote:
> On 2016/06/24 15:56:50, tbansal1 wrote:
> > On 2016/06/24 15:33:47, leonhsl wrote:
> > > Hi, tbansal@, megjablon@, in addition to the following inline question, I
> want
> > > to confirm another one:
> > > As for 1 render process host, will there be multiple render frames sending
> the
> > > IsDataReductionProxy IPC to it? Thanks in advance.
> > The message is send once per page load.
> > 
> > If the two render frames are part of same process (e.g., http://example.com
> > loaded in two different tabs may share render process), then the
> > render process host will get 2 IPCs.
> > 
> 
> OK I see, then I need to enable our DataReductionProxyHostImpl serve multiple
> clients. Thanks!
> And, sorry for another question not related with this CL ;-) Can multiple
render
> frames share 1 render frame host?

No, each RenderFrame has its own RenderFrameHost (which share a routing ID).  If
multiple RenderFrames in the same process, then their respective
RenderFrameHosts will have the same RenderProcessHost.

Hope that helps!

[leonhsl(Using Gerrit), 2016-06-25 00:26:39]

On 2016/06/24 23:00:20, Charlie Reis wrote:
> On 2016/06/24 16:20:59, leonhsl wrote:
> > On 2016/06/24 15:56:50, tbansal1 wrote:
> > > On 2016/06/24 15:33:47, leonhsl wrote:
> > > > Hi, tbansal@, megjablon@, in addition to the following inline question,
I
> > want
> > > > to confirm another one:
> > > > As for 1 render process host, will there be multiple render frames
sending
> > the
> > > > IsDataReductionProxy IPC to it? Thanks in advance.
> > > The message is send once per page load.
> > > 
> > > If the two render frames are part of same process (e.g.,
http://example.com
> > > loaded in two different tabs may share render process), then the
> > > render process host will get 2 IPCs.
> > > 
> > 
> > OK I see, then I need to enable our DataReductionProxyHostImpl serve
multiple
> > clients. Thanks!
> > And, sorry for another question not related with this CL ;-) Can multiple
> render
> > frames share 1 render frame host?
> 
> No, each RenderFrame has its own RenderFrameHost (which share a routing ID). 
If
> multiple RenderFrames in the same process, then their respective
> RenderFrameHosts will have the same RenderProcessHost.
> 
> Hope that helps!

Understood and thank you~

[leonhsl(Using Gerrit), 2016-06-25 07:20:56]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-25 07:21:13]

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

[commit-bot: I haz the power, 2016-06-25 08:22:10]

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

[commit-bot: I haz the power, 2016-06-25 08:22:12]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-06-29 09:42:30]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

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

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-06-29 11:10:01]

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

[commit-bot: I haz the power, 2016-06-29 11:10:03]

Dry run: This issue passed the CQ dry run.

[Anand Mistry (off Chromium), 2016-06-30 12:51:45]

Pretty good looking at the Mojo side. Just one lifecycle thing.

https://codereview.chromium.org/2063683002/diff/260001/components/data_reduct...
File
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.cc
(right):

https://codereview.chromium.org/2063683002/diff/260001/components/data_reduct...
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.cc:42:
void DataReductionProxyHostImpl::RenderProcessHostDestroyed(
RenderProcessHosts can be reused. And every time, a new set of Mojo services is
registered. This means you'll end up with zombie DataReductionProxyHostImpls
until the RPH is finally destroyed. I think you want RenderProcessHostExited
instead which runs when the renderer process exits.

[leonhsl(Using Gerrit), 2016-07-01 01:26:28]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-01 01:26:46]

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-01 02:11:19]

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

[commit-bot: I haz the power, 2016-07-01 02:11:21]

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_...)

[leonhsl(Using Gerrit), 2016-07-01 02:25:57]

Uploaded ps#15 to address comments, PTAnL, Thanks~

https://codereview.chromium.org/2063683002/diff/260001/components/data_reduct...
File
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.cc
(right):

https://codereview.chromium.org/2063683002/diff/260001/components/data_reduct...
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.cc:42:
void DataReductionProxyHostImpl::RenderProcessHostDestroyed(
On 2016/06/30 12:51:45, Anand Mistry wrote:
> RenderProcessHosts can be reused. And every time, a new set of Mojo services
is
> registered. This means you'll end up with zombie DataReductionProxyHostImpls
> until the RPH is finally destroyed. I think you want RenderProcessHostExited
> instead which runs when the renderer process exits.

Oh, yeah, I see now.. Thanks a lot for pointing out this! I really learned a lot
from this CL! Done.

[leonhsl(Using Gerrit), 2016-07-01 02:59:27]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-01 02:59:50]

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-01 03:34:43]

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

[commit-bot: I haz the power, 2016-07-01 03:34:45]

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_...)

[Anand Mistry (off Chromium), 2016-07-01 05:57:26]

lgtm from a mojo perspective

[leonhsl(Using Gerrit), 2016-07-01 06:54:27]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-01 06:54:36]

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-01 07:44:44]

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

[commit-bot: I haz the power, 2016-07-01 07:44:47]

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_...)

[leonhsl(Using Gerrit), 2016-07-01 08:15:43]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-01 08:15:48]

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-01 08:57:42]

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

[commit-bot: I haz the power, 2016-07-01 08:57:44]

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_...)

[leonhsl(Using Gerrit), 2016-07-01 09:20:50]

leon.han@intel.com changed reviewers:
+ dcheng@chromium.org, jochen@chromium.org

[leonhsl(Using Gerrit), 2016-07-01 09:20:51]

Although trybots are still red, I checked and suppose it should not be because
of changes of this CL. 

PTAL, Thanks.

tbansal@, megjablon@ : OWNER review for data_reduction_proxy.

dcheng@: Security review for mojom, ipc.

jochen@: OWNER review for chrome/, components/ except data_reduction_proxy.

[jochen (gone - plz use gerrit), 2016-07-01 14:17:41]

lgtm

[leonhsl(Using Gerrit), 2016-07-02 11:03:28]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-02 11:03:38]

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-02 11:45:33]

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

[commit-bot: I haz the power, 2016-07-02 11:45:35]

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_...)

[leonhsl(Using Gerrit), 2016-07-04 01:45:18]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-04 01:45:27]

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-04 02:30:50]

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

[commit-bot: I haz the power, 2016-07-04 02:30:52]

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_...)

[leonhsl(Using Gerrit), 2016-07-05 02:18:30]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-05 02:18:47]

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-05 03:02:01]

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

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

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

[dcheng, 2016-07-08 05:50:53]

https://codereview.chromium.org/2063683002/diff/320001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2063683002/diff/320001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:2784:
Profile::FromBrowserContext(render_process_host->GetBrowserContext());
I think a BrowserContext here should be sufficient.

(If it turns out other things need a Profile later, that's easy enough to
change)

https://codereview.chromium.org/2063683002/diff/320001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:2792: render_process_host);
Lines 2789-2791 should probably just be inlined into the host ctor: it's an
implementation detail of the host.

https://codereview.chromium.org/2063683002/diff/320001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:2798:
BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO));
I'm a bit sad about how the ownership here has become very loosely coupled: we
have to bind an unretained pointer into the callback, and then add a comment
(quite far away from the actual class and easy to become outdated) that it
self-deletes due to being a RenderProcessHostObserver.

As a strawman... how bad would it be:
- Make this a base::Owned() pointer.
- Have the InterfaceRegistry clear registered callbacks on the task runner the
interface factory was registered with

https://codereview.chromium.org/2063683002/diff/320001/chrome/renderer/page_l...
File chrome/renderer/page_load_histograms.cc (right):

https://codereview.chromium.org/2063683002/diff/320001/chrome/renderer/page_l...
chrome/renderer/page_load_histograms.cc:895: bool handled =
data_reduction_proxy_host->IsDataReductionProxy(
How expensive is it to create / tear down a message pipe everytime we query this
state? It definitely seems less efficient than legacy IPC, since we'd just route
one message there.

https://codereview.chromium.org/2063683002/diff/320001/components/data_reduct...
File components/data_reduction_proxy/content/common/OWNERS (right):

https://codereview.chromium.org/2063683002/diff/320001/components/data_reduct...
components/data_reduction_proxy/content/common/OWNERS:4: per-file
*_messages.cc=file://ipc/SECURITY_OWNERS
Nit: I think you can delete lines 3-4.

[leonhsl(Using Gerrit), 2016-07-11 01:50:09]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-11 01:50: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-11 03:08:23]

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

[commit-bot: I haz the power, 2016-07-11 03:08:25]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-07-11 11:13:22]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-11 11:13:41]

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-11 11:28:52]

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

[commit-bot: I haz the power, 2016-07-11 11:28:53]

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

[leonhsl(Using Gerrit), 2016-07-11 13:09:35]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-11 13:09:45]

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-11 14:07:11]

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

[commit-bot: I haz the power, 2016-07-11 14:07:13]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-07-11 14:36:05]

leon.han@intel.com changed reviewers:
+ rockot@chromium.org

[leonhsl(Using Gerrit), 2016-07-11 14:36:07]

Thanks Daniel a lot for review and add Ken to help take a look at such case.

https://codereview.chromium.org/2063683002/diff/320001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2063683002/diff/320001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:2784:
Profile::FromBrowserContext(render_process_host->GetBrowserContext());
On 2016/07/08 05:50:53, dcheng wrote:
> I think a BrowserContext here should be sufficient.
> 
> (If it turns out other things need a Profile later, that's easy enough to
> change)

Done and thanks!

https://codereview.chromium.org/2063683002/diff/320001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:2792: render_process_host);
On 2016/07/08 05:50:53, dcheng wrote:
> Lines 2789-2791 should probably just be inlined into the host ctor: it's an
> implementation detail of the host.

Done.

https://codereview.chromium.org/2063683002/diff/320001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:2798:
BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO));
On 2016/07/08 05:50:53, dcheng wrote:
> I'm a bit sad about how the ownership here has become very loosely coupled: we
> have to bind an unretained pointer into the callback, and then add a comment
> (quite far away from the actual class and easy to become outdated) that it
> self-deletes due to being a RenderProcessHostObserver.
> 
> As a strawman... how bad would it be:
> - Make this a base::Owned() pointer.
> - Have the InterfaceRegistry clear registered callbacks on the task runner the
> interface factory was registered with

I'm also investigating around,, but I think Ken would have some insights about
this concrete sample from mojo overall point of view. Thanks~

https://codereview.chromium.org/2063683002/diff/320001/chrome/renderer/page_l...
File chrome/renderer/page_load_histograms.cc (right):

https://codereview.chromium.org/2063683002/diff/320001/chrome/renderer/page_l...
chrome/renderer/page_load_histograms.cc:895: bool handled =
data_reduction_proxy_host->IsDataReductionProxy(
On 2016/07/08 05:50:53, dcheng wrote:
> How expensive is it to create / tear down a message pipe everytime we query
this
> state? It definitely seems less efficient than legacy IPC, since we'd just
route
> one message there.

From my understanding this call will just happen once for each main render
frame, and as I remember that a mojo message pipe should be cheap to create and
destroy, so I made this choice. I referenced some other mojofication works which
are also using mojo message pipes as if it's free ;-)

https://codereview.chromium.org/2063683002/diff/320001/components/data_reduct...
File components/data_reduction_proxy/content/common/OWNERS (right):

https://codereview.chromium.org/2063683002/diff/320001/components/data_reduct...
components/data_reduction_proxy/content/common/OWNERS:4: per-file
*_messages.cc=file://ipc/SECURITY_OWNERS
On 2016/07/08 05:50:53, dcheng wrote:
> Nit: I think you can delete lines 3-4.

Done.

[megjablon, 2016-07-13 00:18:49]

Description was changed from

==========
Migrate components/data_reduction_proxy to Mojo interfaces.

This CL also adds typemapping for net::HostPortPair.

BUG=577685
TEST=components_unittests --gtest_filter=DataReductionProxyHostImplTest*
==========

to

==========
Migrate components/data_reduction_proxy to Mojo interfaces.

This CL also adds typemapping for net::HostPortPair.

BUG=577685
TEST=components_unittests --gtest_filter=DataReductionProxyHostImplTest*
==========

[megjablon, 2016-07-13 00:18:49]

megjablon@chromium.org changed reviewers:
- megjablon@chromium.org

[leonhsl(Using Gerrit), 2016-07-13 10:20:17]

Ping tbansal1@, would you please take a OWNER review? Thanks.

[tbansal1, 2016-07-13 17:35:18]

Few nits.

https://codereview.chromium.org/2063683002/diff/380001/components/data_reduct...
File components/data_reduction_proxy/content/browser/BUILD.gn (right):

https://codereview.chromium.org/2063683002/diff/380001/components/data_reduct...
components/data_reduction_proxy/content/browser/BUILD.gn:42:
"//mojo/public/cpp/bindings",
Should not this be near line 22 above?

https://codereview.chromium.org/2063683002/diff/380001/components/data_reduct...
File
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.cc
(right):

https://codereview.chromium.org/2063683002/diff/380001/components/data_reduct...
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.cc:37:
if (!config_) {
Add DCHECK_CURRENTLY_ON(content::BrowserThread::IO);

https://codereview.chromium.org/2063683002/diff/380001/components/data_reduct...
File
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.h
(right):

https://codereview.chromium.org/2063683002/diff/380001/components/data_reduct...
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.h:25:
// |config| may be null.
Did you mean |settings|?

https://codereview.chromium.org/2063683002/diff/380001/components/data_reduct...
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.h:27:
content::RenderProcessHost* host);
Forward declare RenderProcessHost.

[leonhsl(Using Gerrit), 2016-07-14 03:25:37]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-14 03:25:51]

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-14 04:12:30]

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

[commit-bot: I haz the power, 2016-07-14 04:12:32]

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_...)

[leonhsl(Using Gerrit), 2016-07-14 07:48:39]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-14 07:48:51]

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-14 09:30:42]

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

[commit-bot: I haz the power, 2016-07-14 09:30:43]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-07-14 14:54:53]

Uploaded ps#21 and ps#22 to address comments, PTAnL, Thanks.

https://codereview.chromium.org/2063683002/diff/380001/components/data_reduct...
File components/data_reduction_proxy/content/browser/BUILD.gn (right):

https://codereview.chromium.org/2063683002/diff/380001/components/data_reduct...
components/data_reduction_proxy/content/browser/BUILD.gn:42:
"//mojo/public/cpp/bindings",
On 2016/07/13 17:35:18, tbansal1 wrote:
> Should not this be near line 22 above?

Ah this should be unnecessary. Removed and Thanks!

https://codereview.chromium.org/2063683002/diff/380001/components/data_reduct...
File
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.cc
(right):

https://codereview.chromium.org/2063683002/diff/380001/components/data_reduct...
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.cc:37:
if (!config_) {
On 2016/07/13 17:35:18, tbansal1 wrote:
> Add DCHECK_CURRENTLY_ON(content::BrowserThread::IO);

Tried to add in ps#21 but found DataReductionProxyHostImpl unit test failed,
because unit test code is not running on BrowserThread::IO.
As mojo is thread hostile, all mojo calls must run on the same thread on which
the interface impl was bound, so I think as we have ensured BindRequest()
running on BrowserThread::IO, here we can omit this check?

https://codereview.chromium.org/2063683002/diff/380001/components/data_reduct...
File
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.h
(right):

https://codereview.chromium.org/2063683002/diff/380001/components/data_reduct...
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.h:25:
// |config| may be null.
On 2016/07/13 17:35:18, tbansal1 wrote:
> Did you mean |settings|?

Done.

https://codereview.chromium.org/2063683002/diff/380001/components/data_reduct...
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.h:27:
content::RenderProcessHost* host);
On 2016/07/13 17:35:18, tbansal1 wrote:
> Forward declare RenderProcessHost.

Done.

[tbansal1, 2016-07-14 17:02:03]

components/d_r_p lgtm. Thanks for doing this.

[leonhsl(Using Gerrit), 2016-07-20 03:24:23]

Ping Daniel, thanks. 
I think chromium-mojo@ discussions about interface impl lifecycle have already
come out some conclusions.

[dcheng, 2016-07-20 10:31:46]

On 2016/07/20 03:24:23, leonhsl wrote:
> Ping Daniel, thanks. 
> I think chromium-mojo@ discussions about interface impl lifecycle have already
> come out some conclusions.

I thought tibell@ was working on something where we wouldn't need to use
base::Unretained pointers so much? I still see that being used in this CL.

[leonhsl(Using Gerrit), 2016-07-21 09:15:54]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-21 09:16:15]

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 10:31:02]

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

[commit-bot: I haz the power, 2016-07-21 10:31:04]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-07-21 11:57:53]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-21 11:58:19]

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

[leonhsl(Using Gerrit), 2016-07-21 11:59:15]

On 2016/07/20 10:31:46, dcheng wrote:
> On 2016/07/20 03:24:23, leonhsl wrote:
> > Ping Daniel, thanks. 
> > I think chromium-mojo@ discussions about interface impl lifecycle have
already
> > come out some conclusions.
> 
> I thought tibell@ was working on something where we wouldn't need to use
> base::Unretained pointers so much? I still see that being used in this CL.

Oh yeah.. Uploaded ps#24 to use tibell's solution, PTAnL, Thanks.

[commit-bot: I haz the power, 2016-07-21 12:48:33]

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

[commit-bot: I haz the power, 2016-07-21 12:48:34]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-07-22 02:59:47]

On 2016/07/21 11:59:15, leonhsl wrote:
> On 2016/07/20 10:31:46, dcheng wrote:
> > On 2016/07/20 03:24:23, leonhsl wrote:
> > > Ping Daniel, thanks. 
> > > I think chromium-mojo@ discussions about interface impl lifecycle have
> already
> > > come out some conclusions.
> > 
> > I thought tibell@ was working on something where we wouldn't need to use
> > base::Unretained pointers so much? I still see that being used in this CL.
> 
> Oh yeah.. Uploaded ps#24 to use tibell's solution, PTAnL, Thanks.

I'm afraid I have to revert ps#24 because this solution does not fit the case of
this CL. Our DataReductionProxyHostImpl's lifetime should be bound to the
lifetime of each render process, but not the RPHI itself. We should still make
DataReductionProxyHostImpl as RenderProcessHostObserver to observe exit
notification of each render process via RenderProcessHostExited.

[jam, 2016-07-22 05:24:13]

drive-by: please let's hold off on adding more usage of AddOwnedInterface until
we reach consensus on that abstraction.

[leonhsl(Using Gerrit), 2016-07-22 05:34:12]

On 2016/07/22 05:24:13, jam wrote:
> drive-by: please let's hold off on adding more usage of AddOwnedInterface
until
> we reach consensus on that abstraction.

Yeah I realized that AddOwnedInterface does not fit the case of this CL and will
revert ps#24. I'll pay attention on the chromium-dev@ discussions.
Well for such case, I do think what ps#23 does(RPH observer) is 'sort of thing
that can be established, learned, and understood with relative ease' and
acceptable currently.

[leonhsl(Using Gerrit), 2016-08-11 06:49:43]

Patchset #24 (id:460001) has been deleted

[leonhsl(Using Gerrit), 2016-08-11 11:22:59]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-11 11:23:13]

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

[leonhsl(Using Gerrit), 2016-08-11 11:36:23]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-11 11:36:40]

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

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

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

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

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-08-12 02:42:12]

leon.han@intel.com changed reviewers:
+ jam@chromium.org

[leonhsl(Using Gerrit), 2016-08-12 02:42:14]

Hi, Daniel, would you PTAnL at ps#25? Thanks.

Per jam@'s suggestion in chromium-mojo@ discussion thread, now we use
StrongBinding to bind the lifecycle of DataReductionProxyHostImpl with the
message pipe, and use weak ptr to guard the access to DataReductionProxyConfig
instance inside DataReductionProxyHostImpl.

Also, +jam@ to help confirm the new solution. Thanks.

[leonhsl(Using Gerrit), 2016-08-19 03:07:46]

Ping Daniel and John, thanks.

[dcheng, 2016-08-26 19:16:20]

(assuming the owners of data_reduction_proxy are OK with exposing the weakptr to
the config externally... it seems a bit funny to do that, but I don't have any
better ideas)

https://codereview.chromium.org/2063683002/diff/500001/chrome/renderer/page_l...
File chrome/renderer/page_load_histograms.cc (right):

https://codereview.chromium.org/2063683002/diff/500001/chrome/renderer/page_l...
chrome/renderer/page_load_histograms.cc:844: bool handled =
data_reduction_proxy_host->IsDataReductionProxy(
What happens to this request if the other side is never bound? Wouldn't it hang?

https://codereview.chromium.org/2063683002/diff/500001/components/data_reduct...
File
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.cc
(right):

https://codereview.chromium.org/2063683002/diff/500001/components/data_reduct...
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.cc:19:
new data_reduction_proxy::DataReductionProxyHostImpl(config,
Does it make sense to actually call Create if |config| is null? I'm not sure why
we AddInterface in that case, as we'll never bind anything to the interface
request anyway.

[tbansal1, 2016-08-26 20:56:34]

When I last looked at the CL, it was not exposing the WeakPtr from DRP. So, now
I need to look at it again. leonhsl: Can you please hold on this CL for a while.

[leonhsl(Using Gerrit), 2016-08-29 03:36:33]

On 2016/08/26 20:56:34, tbansal1 wrote:
> When I last looked at the CL, it was not exposing the WeakPtr from DRP. So,
now
> I need to look at it again. leonhsl: Can you please hold on this CL for a
while.

Understood. I'll address Daniel's comments firstly and let's figure out a proper
pattern making all happy ;-)

[leonhsl(Using Gerrit), 2016-08-29 06:12:06]

https://codereview.chromium.org/2063683002/diff/500001/chrome/renderer/page_l...
File chrome/renderer/page_load_histograms.cc (right):

https://codereview.chromium.org/2063683002/diff/500001/chrome/renderer/page_l...
chrome/renderer/page_load_histograms.cc:844: bool handled =
data_reduction_proxy_host->IsDataReductionProxy(
On 2016/08/26 19:16:20, dcheng wrote:
> What happens to this request if the other side is never bound? Wouldn't it
hang?

Normally the other side will either bind or reject(close) the request. Closing
request means breaking the message pipe connection, which will wake up the
waiting sync call to return with no any value set to
|ata_reduction_proxy_was_used|.
Will hang here ONLY IF the other side holds the request deliberately but does
not bind it, which would never happen for this CL.

https://codereview.chromium.org/2063683002/diff/500001/components/data_reduct...
File
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.cc
(right):

https://codereview.chromium.org/2063683002/diff/500001/components/data_reduct...
components/data_reduction_proxy/content/browser/data_reduction_proxy_host_impl.cc:19:
new data_reduction_proxy::DataReductionProxyHostImpl(config,
On 2016/08/26 19:16:20, dcheng wrote:
> Does it make sense to actually call Create if |config| is null? I'm not sure
why
> we AddInterface in that case, as we'll never bind anything to the interface
> request anyway.

If we do not AddInterface, ERROR log "Failed to locate a binder for interface:
XXX" will be dumped by
https://cs.chromium.org/chromium/src/services/shell/public/cpp/lib/interface_...
when the request arrived. I assume we do not want this.

[leonhsl(Using Gerrit), 2016-09-13 02:56:41]

As data_reduction_proxy IPCs have all been deprecated by
https://codereview.chromium.org/2314163003/, no need to do the mojo conversion
further, so close this CL now.

Thanks all for the help to me here ;-)