Adding DRP specfic UMA for FirstContentfulPaint

Adding DataReductionProxy (DRP) specfic UMA for FirstContentfulPaint.
This consumes the PageLoad.Timing2 code path to get data specific to DRP with regards to having DRP on and being in the lofi experiment. Right now, it only considers
NavigationToFirstContentfulPaint.

This introduces:
DataReductionProxyData, which is stored on URLRequest;
NavigationData, a cloneable interface used in content/ to communicate information to NavigationHandle;
ChromeNavigationData, an inheritor of NavigationData that owns a DataReductionProxyData and is stored in NavigationHandleImpl (UI thread) and URLRequest (IO thread, in UserData).

BUG=578321
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/fe32816d121c9c779c56d52e63b26d2746ed9987
Cr-Commit-Position: refs/heads/master@{#393087}

[RyanSturm, 2016-02-22 19:27:34]

Description was changed from

==========
Adding DRP specfic UMA for FirstContentfulPaint
This consumes the PageLoad.Timing2 code path to get data
specific to DRP with regards to having DRP on and being in the
lofi experiment. Right now, it only considers
NavigationToFirstContentfulPaint.

BUG=578321
==========

to

==========
Adding DRP specfic UMA for FirstContentfulPaint
This consumes the PageLoad.Timing2 code path to get data
specific to DRP with regards to having DRP on and being in the
lofi experiment. Right now, it only considers
NavigationToFirstContentfulPaint.

BUG=578321
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[RyanSturm, 2016-02-24 21:55:47]

ryansturm@chromium.org changed reviewers:
+ bengr@chromium.org

[bengr, 2016-02-26 22:30:00]

Please provide a more detailed synopsis of your change in the CL description.

[bengr, 2016-02-26 22:53:58]

https://codereview.chromium.org/1721813002/diff/1/chrome/browser/page_load_me...
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/1721813002/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:242:
bool in_lofi_enabled_group =
I'd get rid of these temporaries.

https://codereview.chromium.org/1721813002/diff/1/chrome/browser/page_load_me...
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/1721813002/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc:376:
// Navigate again to force logging
Are these metrics only recorded on a subsequent navigation? Won't we miss a lot
of navigations, e.g., every single navigation session? I guess there's nothing
to be done about that in this CL.

Also, add a period to the comment.

https://codereview.chromium.org/1721813002/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc:388:
SetSimulateFetchViaDataReductionProxy(true);
I suggest you follow the whitespace pattern in the RapporQuickPageLoad test
above.

https://codereview.chromium.org/1721813002/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc:392:
// Navigate again to force logging
logging -> logging.

https://codereview.chromium.org/1721813002/diff/1/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.h
(right):

https://codereview.chromium.org/1721813002/diff/1/chrome/browser/renderer_hos...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.h:95:
content::ResourceContext* resource_context) override;
You should probably forward declare this.

https://codereview.chromium.org/1721813002/diff/1/components/page_load_metric...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1721813002/diff/1/components/page_load_metric...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:376:
finished_nav->set_is_using_lofi(navigation_handle->IsUsingLofi());
Is there an existing pattern to set the PageLoadTracker? This seems ad hoc.

https://codereview.chromium.org/1721813002/diff/1/components/page_load_metric...
File components/page_load_metrics/browser/metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/1721813002/diff/1/components/page_load_metric...
components/page_load_metrics/browser/metrics_web_contents_observer.h:169: bool
is_using_lofi_;
Add comments.

https://codereview.chromium.org/1721813002/diff/1/components/page_load_metric...
File components/page_load_metrics/browser/page_load_metrics_observer.h (right):

https://codereview.chromium.org/1721813002/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_observer.h:58: bool
was_fetched_via_data_reduction_proxy);
That's a mouthful. How about used_data_reduction_proxy? If you make the change,
do so everywhere.

https://codereview.chromium.org/1721813002/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_observer.h:87: bool
is_using_lofi;
Add comments to describe these.

https://codereview.chromium.org/1721813002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/1721813002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.h:281: bool is_using_lofi_;
Add comments.

https://codereview.chromium.org/1721813002/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/1721813002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_loader.cc:109: if (resource_context &&
host_delegate)
Use curly braces.

https://codereview.chromium.org/1721813002/diff/1/content/common/frame_messag...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1721813002/diff/1/content/common/frame_messag...
content/common/frame_messages.h:294: // True if Lofi was on for this navigation
Add a period here and below.

https://codereview.chromium.org/1721813002/diff/1/content/public/browser/navi...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/1721813002/diff/1/content/public/browser/navi...
content/public/browser/navigation_handle.h:170: // Whether the navigation
requested a LoFi version of the resource or not
All comments should be complete sentences. At the very least, end with a period.

https://codereview.chromium.org/1721813002/diff/1/content/public/browser/navi...
content/public/browser/navigation_handle.h:171: virtual bool IsUsingLofi() = 0;
This should probably be a const method.

https://codereview.chromium.org/1721813002/diff/1/content/public/browser/navi...
content/public/browser/navigation_handle.h:173: // Whether this navigation
fetched it's resource through DataReductionProxy
Wrong "its."

https://codereview.chromium.org/1721813002/diff/1/content/public/browser/navi...
content/public/browser/navigation_handle.h:174: virtual bool
WasFetchedViaDataReductionProxy() = 0;
Also const.

https://codereview.chromium.org/1721813002/diff/1/content/public/common/resou...
File content/public/common/resource_response_info.h (right):

https://codereview.chromium.org/1721813002/diff/1/content/public/common/resou...
content/public/common/resource_response_info.h:140: // Whether the request went
through the data reduction proxy or not
Period.

https://codereview.chromium.org/1721813002/diff/1/content/test/test_render_fr...
File content/test/test_render_frame_host.h (right):

https://codereview.chromium.org/1721813002/diff/1/content/test/test_render_fr...
content/test/test_render_frame_host.h:117: void
set_simulate_fetch_via_data_reduction_proxy(bool enabled) {
Add a comment.

[RyanSturm, 2016-03-01 19:36:04]

https://codereview.chromium.org/1721813002/diff/1/chrome/browser/page_load_me...
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/1721813002/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:242:
bool in_lofi_enabled_group =
On 2016/02/26 22:53:57, bengr wrote:
> I'd get rid of these temporaries.

Done.

https://codereview.chromium.org/1721813002/diff/1/chrome/browser/page_load_me...
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/1721813002/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc:376:
// Navigate again to force logging
On 2016/02/26 22:53:57, bengr wrote:
> Are these metrics only recorded on a subsequent navigation? Won't we miss a
lot
> of navigations, e.g., every single navigation session? I guess there's nothing
> to be done about that in this CL.
> 
> Also, add a period to the comment.

Like you said, there's nothing to be done about it in this CL.

https://codereview.chromium.org/1721813002/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc:388:
SetSimulateFetchViaDataReductionProxy(true);
On 2016/02/26 22:53:57, bengr wrote:
> I suggest you follow the whitespace pattern in the RapporQuickPageLoad test
> above.

Done.

https://codereview.chromium.org/1721813002/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc:392:
// Navigate again to force logging
On 2016/02/26 22:53:57, bengr wrote:
> logging -> logging.

Done.

https://codereview.chromium.org/1721813002/diff/1/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.h
(right):

https://codereview.chromium.org/1721813002/diff/1/chrome/browser/renderer_hos...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.h:95:
content::ResourceContext* resource_context) override;
On 2016/02/26 22:53:57, bengr wrote:
> You should probably forward declare this.

Done.

https://codereview.chromium.org/1721813002/diff/1/components/page_load_metric...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1721813002/diff/1/components/page_load_metric...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:376:
finished_nav->set_is_using_lofi(navigation_handle->IsUsingLofi());
On 2016/02/26 22:53:57, bengr wrote:
> Is there an existing pattern to set the PageLoadTracker? This seems ad hoc.

set_renderer_tracked is the only setter I see on PageLoadTracker. This seems to
follow that pattern.

https://codereview.chromium.org/1721813002/diff/1/components/page_load_metric...
File components/page_load_metrics/browser/metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/1721813002/diff/1/components/page_load_metric...
components/page_load_metrics/browser/metrics_web_contents_observer.h:169: bool
is_using_lofi_;
On 2016/02/26 22:53:57, bengr wrote:
> Add comments.

Done.

https://codereview.chromium.org/1721813002/diff/1/components/page_load_metric...
File components/page_load_metrics/browser/page_load_metrics_observer.h (right):

https://codereview.chromium.org/1721813002/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_observer.h:58: bool
was_fetched_via_data_reduction_proxy);
On 2016/02/26 22:53:57, bengr wrote:
> That's a mouthful. How about used_data_reduction_proxy? If you make the
change,
> do so everywhere.

Done.

https://codereview.chromium.org/1721813002/diff/1/components/page_load_metric...
components/page_load_metrics/browser/page_load_metrics_observer.h:87: bool
is_using_lofi;
On 2016/02/26 22:53:57, bengr wrote:
> Add comments to describe these.

Done.

https://codereview.chromium.org/1721813002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/1721813002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.h:281: bool is_using_lofi_;
On 2016/02/26 22:53:57, bengr wrote:
> Add comments.

Done.

https://codereview.chromium.org/1721813002/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/1721813002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_loader.cc:109: if (resource_context &&
host_delegate)
On 2016/02/26 22:53:57, bengr wrote:
> Use curly braces.

Done.

https://codereview.chromium.org/1721813002/diff/1/content/common/frame_messag...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1721813002/diff/1/content/common/frame_messag...
content/common/frame_messages.h:294: // True if Lofi was on for this navigation
On 2016/02/26 22:53:57, bengr wrote:
> Add a period here and below.

Done.

https://codereview.chromium.org/1721813002/diff/1/content/public/browser/navi...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/1721813002/diff/1/content/public/browser/navi...
content/public/browser/navigation_handle.h:170: // Whether the navigation
requested a LoFi version of the resource or not
On 2016/02/26 22:53:58, bengr wrote:
> All comments should be complete sentences. At the very least, end with a
period.

Done.

https://codereview.chromium.org/1721813002/diff/1/content/public/browser/navi...
content/public/browser/navigation_handle.h:171: virtual bool IsUsingLofi() = 0;
On 2016/02/26 22:53:57, bengr wrote:
> This should probably be a const method.

Done.

https://codereview.chromium.org/1721813002/diff/1/content/public/browser/navi...
content/public/browser/navigation_handle.h:174: virtual bool
WasFetchedViaDataReductionProxy() = 0;
On 2016/02/26 22:53:58, bengr wrote:
> Also const.

Done.

https://codereview.chromium.org/1721813002/diff/1/content/public/common/resou...
File content/public/common/resource_response_info.h (right):

https://codereview.chromium.org/1721813002/diff/1/content/public/common/resou...
content/public/common/resource_response_info.h:140: // Whether the request went
through the data reduction proxy or not
On 2016/02/26 22:53:58, bengr wrote:
> Period.

Done.

https://codereview.chromium.org/1721813002/diff/1/content/test/test_render_fr...
File content/test/test_render_frame_host.h (right):

https://codereview.chromium.org/1721813002/diff/1/content/test/test_render_fr...
content/test/test_render_frame_host.h:117: void
set_simulate_fetch_via_data_reduction_proxy(bool enabled) {
On 2016/02/26 22:53:58, bengr wrote:
> Add a comment.

Done.

[Charlie Harrison, 2016-03-01 19:50:42]

csharrison@chromium.org changed reviewers:
+ csharrison@chromium.org

[Charlie Harrison, 2016-03-01 19:50:43]

Drive by: Since all the extra data you're adding to the observer API is
available on the NavigationHandle, can you just pull that data from the handle
in the observer instead of adding fields to the page_load_metrics_observer API?

Additionally, it would be nice for this change to exist as a separate observer,
as the data you are logging is not really "core" PLT. See the various other
PageLoad.Clients* histograms in the observers/ directory.

The goal of the observer system was to ensure that the core classes in
components/page_load_metrics and core observer don't need to be tweaked.

https://codereview.chromium.org/1721813002/diff/1/chrome/browser/page_load_me...
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/1721813002/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc:376:
// Navigate again to force logging
On 2016/03/01 19:36:03, RyanSturm wrote:
> On 2016/02/26 22:53:57, bengr wrote:
> > Are these metrics only recorded on a subsequent navigation? Won't we miss a
> lot
> > of navigations, e.g., every single navigation session? I guess there's
nothing
> > to be done about that in this CL.
> > 
> > Also, add a period to the comment.
> 
> Like you said, there's nothing to be done about it in this CL.

They will also be recorded when the browser closes or the renderer shuts down.
We just navigate in the tests due to how they are set up.

[RyanSturm, 2016-03-01 21:50:36]

On 2016/03/01 19:50:43, csharrison wrote:
> Drive by: Since all the extra data you're adding to the observer API is
> available on the NavigationHandle, can you just pull that data from the handle
> in the observer instead of adding fields to the page_load_metrics_observer
API?
> 
> Additionally, it would be nice for this change to exist as a separate
observer,
> as the data you are logging is not really "core" PLT. See the various other
> PageLoad.Clients* histograms in the observers/ directory.
> 
> The goal of the observer system was to ensure that the core classes in
> components/page_load_metrics and core observer don't need to be tweaked.
> 
>
https://codereview.chromium.org/1721813002/diff/1/chrome/browser/page_load_me...
> File
>
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/1721813002/diff/1/chrome/browser/page_load_me...
>
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc:376:
> // Navigate again to force logging
> On 2016/03/01 19:36:03, RyanSturm wrote:
> > On 2016/02/26 22:53:57, bengr wrote:
> > > Are these metrics only recorded on a subsequent navigation? Won't we miss
a
> > lot
> > > of navigations, e.g., every single navigation session? I guess there's
> nothing
> > > to be done about that in this CL.
> > > 
> > > Also, add a period to the comment.
> > 
> > Like you said, there's nothing to be done about it in this CL.
> 
> They will also be recorded when the browser closes or the renderer shuts down.
> We just navigate in the tests due to how they are set up.

Sounds pretty reasonable to make those changes leaving page_load_metrics
untouched except for the new DataReductionProxyPageLoadMetricsObserver and
registering the observer.

[tbansal1, 2016-03-07 18:05:03]

tbansal@chromium.org changed reviewers:
+ tbansal@chromium.org

[tbansal1, 2016-03-07 18:05:05]

https://codereview.chromium.org/1721813002/diff/40001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
(right):

https://codereview.chromium.org/1721813002/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:46:
if (!timing.first_contentful_paint.is_zero()) {
For readability:
if (timing.first_contentful_paint.is_zero() || !used_data_reduction_proxy_) {
return;
}

https://codereview.chromium.org/1721813002/diff/40001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h
(right):

https://codereview.chromium.org/1721813002/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:47:
DISALLOW_COPY_AND_ASSIGN(DataReductionProxyMetricsObserver);
#include "base/macros.h"

https://codereview.chromium.org/1721813002/diff/40001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/1721813002/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:5:
#include
"chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h"
Linespace after this include.

https://codereview.chromium.org/1721813002/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:19:
tracker->AddObserver(make_scoped_ptr(
#include "base/memory/scoped_ptr.h"

https://codereview.chromium.org/1721813002/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:43:
timing.navigation_start = base::Time::FromDoubleT(1);
#include "base/time/time.h"

https://codereview.chromium.org/1721813002/diff/40001/content/browser/loader/...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/1721813002/diff/40001/content/browser/loader/...
content/browser/loader/resource_loader.cc:102: 
nit: remove the line break.

https://codereview.chromium.org/1721813002/diff/40001/content/browser/loader/...
content/browser/loader/resource_loader.cc:110:
response->head.used_data_reduction_proxy =
simplify to:
response->head.used_data_reduction_proxy = resource_context && host_delegate &&
host_delegate->UsedDataReductionProxy(request->proxy_server(),
resource_context);

https://codereview.chromium.org/1721813002/diff/40001/content/browser/loader/...
File content/browser/loader/resource_loader.h (right):

https://codereview.chromium.org/1721813002/diff/40001/content/browser/loader/...
content/browser/loader/resource_loader.h:28: class
ResourceDispatcherHostDelegate;
Alphabetical order.

https://codereview.chromium.org/1721813002/diff/40001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1721813002/diff/40001/content/common/frame_me...
content/common/frame_messages.h:294: // True if Lofi was on for this navigation.
s/Lofi/LoFi/

https://codereview.chromium.org/1721813002/diff/40001/content/public/browser/...
File content/public/browser/resource_dispatcher_host_delegate.h (right):

https://codereview.chromium.org/1721813002/diff/40001/content/public/browser/...
content/public/browser/resource_dispatcher_host_delegate.h:27: class
HostPortPair;
Alphabetical order.

https://codereview.chromium.org/1721813002/diff/40001/content/public/browser/...
content/public/browser/resource_dispatcher_host_delegate.h:134: virtual bool
UsedDataReductionProxy(const net::HostPortPair& proxy_server,
Please add function comments.

https://codereview.chromium.org/1721813002/diff/40001/content/test/test_web_c...
File content/test/test_web_contents.h (right):

https://codereview.chromium.org/1721813002/diff/40001/content/test/test_web_c...
content/test/test_web_contents.h:114: void
set_simulate_used_data_reduction_proxy(bool enabled) {
s/set_simulate_used_data_reduction_proxy/set_simulate_fetch_via_data_reduction_proxy

to match the variable name.

[RyanSturm, 2016-03-08 19:47:43]

https://codereview.chromium.org/1721813002/diff/40001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
(right):

https://codereview.chromium.org/1721813002/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:46:
if (!timing.first_contentful_paint.is_zero()) {
On 2016/03/07 18:05:04, tbansal1 wrote:
> For readability:
> if (timing.first_contentful_paint.is_zero() || !used_data_reduction_proxy_) {
> return;
> }

Done.

https://codereview.chromium.org/1721813002/diff/40001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h
(right):

https://codereview.chromium.org/1721813002/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:47:
DISALLOW_COPY_AND_ASSIGN(DataReductionProxyMetricsObserver);
On 2016/03/07 18:05:04, tbansal1 wrote:
> #include "base/macros.h"

Done.

https://codereview.chromium.org/1721813002/diff/40001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/1721813002/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:5:
#include
"chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h"
On 2016/03/07 18:05:04, tbansal1 wrote:
> Linespace after this include.

Done.

https://codereview.chromium.org/1721813002/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:19:
tracker->AddObserver(make_scoped_ptr(
On 2016/03/07 18:05:05, tbansal1 wrote:
> #include "base/memory/scoped_ptr.h"

Done.

https://codereview.chromium.org/1721813002/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:43:
timing.navigation_start = base::Time::FromDoubleT(1);
On 2016/03/07 18:05:05, tbansal1 wrote:
> #include "base/time/time.h"

Done.

https://codereview.chromium.org/1721813002/diff/40001/content/browser/loader/...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/1721813002/diff/40001/content/browser/loader/...
content/browser/loader/resource_loader.cc:102: 
On 2016/03/07 18:05:05, tbansal1 wrote:
> nit: remove the line break.

Done.

https://codereview.chromium.org/1721813002/diff/40001/content/browser/loader/...
content/browser/loader/resource_loader.cc:110:
response->head.used_data_reduction_proxy =
On 2016/03/07 18:05:05, tbansal1 wrote:
> simplify to:
> response->head.used_data_reduction_proxy = resource_context && host_delegate
&&
> host_delegate->UsedDataReductionProxy(request->proxy_server(),
> resource_context);

Done.

https://codereview.chromium.org/1721813002/diff/40001/content/browser/loader/...
File content/browser/loader/resource_loader.h (right):

https://codereview.chromium.org/1721813002/diff/40001/content/browser/loader/...
content/browser/loader/resource_loader.h:28: class
ResourceDispatcherHostDelegate;
On 2016/03/07 18:05:05, tbansal1 wrote:
> Alphabetical order.

Done.

https://codereview.chromium.org/1721813002/diff/40001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1721813002/diff/40001/content/common/frame_me...
content/common/frame_messages.h:294: // True if Lofi was on for this navigation.
On 2016/03/07 18:05:05, tbansal1 wrote:
> s/Lofi/LoFi/

Done.

https://codereview.chromium.org/1721813002/diff/40001/content/public/browser/...
File content/public/browser/resource_dispatcher_host_delegate.h (right):

https://codereview.chromium.org/1721813002/diff/40001/content/public/browser/...
content/public/browser/resource_dispatcher_host_delegate.h:27: class
HostPortPair;
On 2016/03/07 18:05:05, tbansal1 wrote:
> Alphabetical order.

Done.

https://codereview.chromium.org/1721813002/diff/40001/content/public/browser/...
content/public/browser/resource_dispatcher_host_delegate.h:134: virtual bool
UsedDataReductionProxy(const net::HostPortPair& proxy_server,
On 2016/03/07 18:05:05, tbansal1 wrote:
> Please add function comments.

Done.

https://codereview.chromium.org/1721813002/diff/40001/content/test/test_web_c...
File content/test/test_web_contents.h (right):

https://codereview.chromium.org/1721813002/diff/40001/content/test/test_web_c...
content/test/test_web_contents.h:114: void
set_simulate_used_data_reduction_proxy(bool enabled) {
On 2016/03/07 18:05:05, tbansal1 wrote:
>
s/set_simulate_used_data_reduction_proxy/set_simulate_fetch_via_data_reduction_proxy
> 
> to match the variable name.

Done.

[tbansal1, 2016-03-08 22:14:44]

https://codereview.chromium.org/1721813002/diff/60001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
(right):

https://codereview.chromium.org/1721813002/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:46:
if (timing.first_contentful_paint.is_zero() || used_data_reduction_proxy_)
did you mean !used_data_reduction_proxy?

https://codereview.chromium.org/1721813002/diff/60001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h
(right):

https://codereview.chromium.org/1721813002/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:40:
const page_load_metrics::PageLoadExtraInfo& info);
can this be a const method?

https://codereview.chromium.org/1721813002/diff/60001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/1721813002/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:15:
const char kDefaultTestUrl2[] = "https://whatever.com";
s/whatever.com/example.com/

https://codereview.chromium.org/1721813002/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:53:
SetSimulateUsedDataReductionProxy(false);
Why is this required?

https://codereview.chromium.org/1721813002/diff/60001/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.h
(right):

https://codereview.chromium.org/1721813002/diff/60001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.h:96: 
Remove the line break since this method is also overriding the method in
ResourceDispatcherHostDelegate.

https://codereview.chromium.org/1721813002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/1721813002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:291: // True if the
navigation used LoFi.
Is is_using_lofi_ true when network was slow and Chromium was in LoFi control
group?

https://codereview.chromium.org/1721813002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:292: bool is_using_lofi_;
Initialize these to false in the constructor?

https://codereview.chromium.org/1721813002/diff/60001/content/browser/loader/...
File content/browser/loader/resource_loader.h (right):

https://codereview.chromium.org/1721813002/diff/60001/content/browser/loader/...
content/browser/loader/resource_loader.h:29: class
ResourceDispatcherHostDelegate;
alphabetical order.

https://codereview.chromium.org/1721813002/diff/60001/content/public/browser/...
File content/public/browser/resource_dispatcher_host_delegate.cc (left):

https://codereview.chromium.org/1721813002/diff/60001/content/public/browser/...
content/public/browser/resource_dispatcher_host_delegate.cc:6: 
No need to delete this line break.

https://codereview.chromium.org/1721813002/diff/60001/content/public/browser/...
File content/public/browser/resource_dispatcher_host_delegate.h (right):

https://codereview.chromium.org/1721813002/diff/60001/content/public/browser/...
content/public/browser/resource_dispatcher_host_delegate.h:134: // Asks the
embedder if this HostPortPair is a data_reduction_proxy
s/HostPortPair/|proxy_server|/

https://codereview.chromium.org/1721813002/diff/60001/content/public/browser/...
content/public/browser/resource_dispatcher_host_delegate.h:135: // HostPortPair.
s/HostPortPair././

https://codereview.chromium.org/1721813002/diff/60001/content/public/common/r...
File content/public/common/resource_response_info.h (right):

https://codereview.chromium.org/1721813002/diff/60001/content/public/common/r...
content/public/common/resource_response_info.h:141: // True if the navigation
was proxied through data reduction proxy.
s/navigation was proxied/resource was fetched/

https://codereview.chromium.org/1721813002/diff/60001/content/test/test_rende...
File content/test/test_render_frame_host.h (right):

https://codereview.chromium.org/1721813002/diff/60001/content/test/test_rende...
content/test/test_render_frame_host.h:159:
DISALLOW_COPY_AND_ASSIGN(TestRenderFrameHost);
Line break before this macro.

https://codereview.chromium.org/1721813002/diff/60001/content/test/test_web_c...
File content/test/test_web_contents.cc (right):

https://codereview.chromium.org/1721813002/diff/60001/content/test/test_web_c...
content/test/test_web_contents.cc:141: params.used_data_reduction_proxy =
simulate_used_data_reduction_proxy_;
Is this really needed?

https://codereview.chromium.org/1721813002/diff/60001/content/test/test_web_c...
content/test/test_web_contents.cc:261:
rfh->set_simulate_used_data_reduction_proxy(false);
Why is this set to false?

[RyanSturm, 2016-03-10 00:37:58]

https://codereview.chromium.org/1721813002/diff/60001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
(right):

https://codereview.chromium.org/1721813002/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:46:
if (timing.first_contentful_paint.is_zero() || used_data_reduction_proxy_)
On 2016/03/08 22:14:43, tbansal1 wrote:
> did you mean !used_data_reduction_proxy?

Done.

https://codereview.chromium.org/1721813002/diff/60001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h
(right):

https://codereview.chromium.org/1721813002/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:40:
const page_load_metrics::PageLoadExtraInfo& info);
On 2016/03/08 22:14:43, tbansal1 wrote:
> can this be a const method?

Done.

https://codereview.chromium.org/1721813002/diff/60001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/1721813002/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:15:
const char kDefaultTestUrl2[] = "https://whatever.com";
On 2016/03/08 22:14:43, tbansal1 wrote:
> s/whatever.com/example.com/

Done.

https://codereview.chromium.org/1721813002/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:53:
SetSimulateUsedDataReductionProxy(false);
On 2016/03/08 22:14:43, tbansal1 wrote:
> Why is this required?

Done.

https://codereview.chromium.org/1721813002/diff/60001/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.h
(right):

https://codereview.chromium.org/1721813002/diff/60001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.h:96: 
On 2016/03/08 22:14:43, tbansal1 wrote:
> Remove the line break since this method is also overriding the method in
> ResourceDispatcherHostDelegate.

Done.

https://codereview.chromium.org/1721813002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/1721813002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:291: // True if the
navigation used LoFi.
On 2016/03/08 22:14:43, tbansal1 wrote:
> Is is_using_lofi_ true when network was slow and Chromium was in LoFi control
> group?

Should be. It's determined via ShouldEnableLoFiMode.

https://codereview.chromium.org/1721813002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:292: bool is_using_lofi_;
On 2016/03/08 22:14:43, tbansal1 wrote:
> Initialize these to false in the constructor?

Done.

https://codereview.chromium.org/1721813002/diff/60001/content/browser/loader/...
File content/browser/loader/resource_loader.h (right):

https://codereview.chromium.org/1721813002/diff/60001/content/browser/loader/...
content/browser/loader/resource_loader.h:29: class
ResourceDispatcherHostDelegate;
On 2016/03/08 22:14:43, tbansal1 wrote:
> alphabetical order.

Done.

https://codereview.chromium.org/1721813002/diff/60001/content/public/browser/...
File content/public/browser/resource_dispatcher_host_delegate.cc (left):

https://codereview.chromium.org/1721813002/diff/60001/content/public/browser/...
content/public/browser/resource_dispatcher_host_delegate.cc:6: 
On 2016/03/08 22:14:43, tbansal1 wrote:
> No need to delete this line break.

Done.

https://codereview.chromium.org/1721813002/diff/60001/content/public/browser/...
File content/public/browser/resource_dispatcher_host_delegate.h (right):

https://codereview.chromium.org/1721813002/diff/60001/content/public/browser/...
content/public/browser/resource_dispatcher_host_delegate.h:134: // Asks the
embedder if this HostPortPair is a data_reduction_proxy
On 2016/03/08 22:14:43, tbansal1 wrote:
> s/HostPortPair/|proxy_server|/

Done.

https://codereview.chromium.org/1721813002/diff/60001/content/public/browser/...
content/public/browser/resource_dispatcher_host_delegate.h:135: // HostPortPair.
On 2016/03/08 22:14:43, tbansal1 wrote:
> s/HostPortPair././

Done.

https://codereview.chromium.org/1721813002/diff/60001/content/public/common/r...
File content/public/common/resource_response_info.h (right):

https://codereview.chromium.org/1721813002/diff/60001/content/public/common/r...
content/public/common/resource_response_info.h:141: // True if the navigation
was proxied through data reduction proxy.
On 2016/03/08 22:14:43, tbansal1 wrote:
> s/navigation was proxied/resource was fetched/

Done.

https://codereview.chromium.org/1721813002/diff/60001/content/test/test_rende...
File content/test/test_render_frame_host.h (right):

https://codereview.chromium.org/1721813002/diff/60001/content/test/test_rende...
content/test/test_render_frame_host.h:159:
DISALLOW_COPY_AND_ASSIGN(TestRenderFrameHost);
On 2016/03/08 22:14:44, tbansal1 wrote:
> Line break before this macro.

Done.

https://codereview.chromium.org/1721813002/diff/60001/content/test/test_web_c...
File content/test/test_web_contents.cc (right):

https://codereview.chromium.org/1721813002/diff/60001/content/test/test_web_c...
content/test/test_web_contents.cc:141: params.used_data_reduction_proxy =
simulate_used_data_reduction_proxy_;
On 2016/03/08 22:14:44, tbansal1 wrote:
> Is this really needed?

Done.

https://codereview.chromium.org/1721813002/diff/60001/content/test/test_web_c...
content/test/test_web_contents.cc:261:
rfh->set_simulate_used_data_reduction_proxy(false);
On 2016/03/08 22:14:44, tbansal1 wrote:
> Why is this set to false?

Done.

[tbansal1, 2016-03-10 22:49:48]

https://codereview.chromium.org/1721813002/diff/80001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/1721813002/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:50:
SetSimulateUsedDataReductionProxy(true);
Since the two tests are similar (except for 1 line difference), it would be more
readable if you combine them into a single experiment. Have a for loop that
iterates over  { true, false} and change the value of the proxy. something
similar to
https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl...

It would make it clearer that the only difference between the two cases is the
state of the Flywheel.

https://codereview.chromium.org/1721813002/diff/80001/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/1721813002/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:729:
return data_reduction_proxy_io_data->config()->IsDataReductionProxy(
use braces because the content inside the if-block is more than 1 line long.

https://codereview.chromium.org/1721813002/diff/80001/content/browser/loader/...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/1721813002/diff/80001/content/browser/loader/...
content/browser/loader/resource_loader.cc:109: resource_context && host_delegate
&&
When is |resource_context| null? We should make sure that we are not missing  a
lot of data  because of null resource_context.

https://codereview.chromium.org/1721813002/diff/80001/content/child/weburlres...
File content/child/weburlresponse_extradata_impl.h (right):

https://codereview.chromium.org/1721813002/diff/80001/content/child/weburlres...
content/child/weburlresponse_extradata_impl.h:108: bool
used_data_reduction_proxy_;
Is this (and the functions above) used somewhere?

https://codereview.chromium.org/1721813002/diff/80001/content/public/browser/...
File content/public/browser/resource_dispatcher_host_delegate.h (right):

https://codereview.chromium.org/1721813002/diff/80001/content/public/browser/...
content/public/browser/resource_dispatcher_host_delegate.h:134: // Asks the
embedder if this |proxy_server| is a data reduction proxy.
s/if this |proxy_server|/if |proxy_server|/

[Charlie Harrison, 2016-03-11 16:06:04]

another drive by. Thanks for changing to the observer technique.

https://codereview.chromium.org/1721813002/diff/80001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
(right):

https://codereview.chromium.org/1721813002/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:16:
namespace internal {
nit: Add a newline after { and before }.

https://codereview.chromium.org/1721813002/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:18:
"PageLoad.Timing2.NavigationToFirstContentfulPaint_DataReductionProxy";
Can you use the more common "PageLoad.Clients.DataReductionProxy*" histogram
names? Checkout histograms.xml for the PageLoadClients suffix that already
exists. I noticed you already have a DataReductionProxy suffix so it may not be
worth it. We're trying to namespace all non core page_load_metrics clients into
PageLoad.Clients though.

https://codereview.chromium.org/1721813002/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:25:
}
  // namespace

https://codereview.chromium.org/1721813002/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:40:
RecordTimingHistograms(timing, info);
Why the need for a separate method?

https://codereview.chromium.org/1721813002/diff/80001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/1721813002/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:13:
namespace {
nit: Add a newline after { and before }.

[RyanSturm, 2016-03-14 21:00:46]

https://codereview.chromium.org/1721813002/diff/80001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
(right):

https://codereview.chromium.org/1721813002/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:16:
namespace internal {
On 2016/03/11 16:06:04, csharrison wrote:
> nit: Add a newline after { and before }.

Done.

https://codereview.chromium.org/1721813002/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:18:
"PageLoad.Timing2.NavigationToFirstContentfulPaint_DataReductionProxy";
On 2016/03/11 16:06:04, csharrison wrote:
> Can you use the more common "PageLoad.Clients.DataReductionProxy*" histogram
> names? Checkout histograms.xml for the PageLoadClients suffix that already
> exists. I noticed you already have a DataReductionProxy suffix so it may not
be
> worth it. We're trying to namespace all non core page_load_metrics clients
into
> PageLoad.Clients though.

Done.

https://codereview.chromium.org/1721813002/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:25:
}
On 2016/03/11 16:06:04, csharrison wrote:
>   // namespace

Done.

https://codereview.chromium.org/1721813002/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:40:
RecordTimingHistograms(timing, info);
On 2016/03/11 16:06:04, csharrison wrote:
> Why the need for a separate method?

I prefer longer methods be aptly named, and I expect RecordTimingHistograms to
grow as more of the metrics are recorded. I also expect in the near future,
OnComplete will be used to send some of this data back to the DataReductionProxy
server. If it seems more appropriate to wait for that to happen, I'll leave this
functionality in OnComplete.

https://codereview.chromium.org/1721813002/diff/80001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/1721813002/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:13:
namespace {
On 2016/03/11 16:06:04, csharrison wrote:
> nit: Add a newline after { and before }.

Done.

https://codereview.chromium.org/1721813002/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:50:
SetSimulateUsedDataReductionProxy(true);
On 2016/03/10 22:49:47, tbansal1 wrote:
> Since the two tests are similar (except for 1 line difference), it would be
more
> readable if you combine them into a single experiment. Have a for loop that
> iterates over  { true, false} and change the value of the proxy. something
> similar to
>
https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl...
> 
> It would make it clearer that the only difference between the two cases is the
> state of the Flywheel.

Since this test relies on subsequent navigations changing the state of the
histograms, it seems better to keep these tests separate. While it would not be
difficult to control for that, it seems like that will add some unnecessary
complexity to the tests.

https://codereview.chromium.org/1721813002/diff/80001/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/1721813002/diff/80001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:729:
return data_reduction_proxy_io_data->config()->IsDataReductionProxy(
On 2016/03/10 22:49:47, tbansal1 wrote:
> use braces because the content inside the if-block is more than 1 line long.

Done.

https://codereview.chromium.org/1721813002/diff/80001/content/browser/loader/...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/1721813002/diff/80001/content/browser/loader/...
content/browser/loader/resource_loader.cc:109: resource_context && host_delegate
&&
On 2016/03/10 22:49:47, tbansal1 wrote:
> When is |resource_context| null? We should make sure that we are not missing 
a
> lot of data  because of null resource_context.

It's only null for download and saveFile. These cases aren't top level frames,
so we won't track the data for them. It didn't seem valuable to even make an
evaluation in those cases.

https://codereview.chromium.org/1721813002/diff/80001/content/child/weburlres...
File content/child/weburlresponse_extradata_impl.h (right):

https://codereview.chromium.org/1721813002/diff/80001/content/child/weburlres...
content/child/weburlresponse_extradata_impl.h:108: bool
used_data_reduction_proxy_;
On 2016/03/10 22:49:47, tbansal1 wrote:
> Is this (and the functions above) used somewhere?

It's used in web_url_loader_impl.cc and render_frame_impl.cc; here is the
plumbing path to NavigationHandleImpl:

https://codereview.chromium.org/1721813002/diff/80001/content/browser/loader/...
ResourceResponse.head (ResourceResponseInfo)

https://codereview.chromium.org/1721813002/diff/80001/content/child/web_url_l...
ResourceResponseInfo -> WebURLResponseExtraDataImpl

https://codereview.chromium.org/1721813002/diff/80001/content/renderer/render...
WebURLResponseExtraDataImpl -> FrameHostMsg_DidCommitProvisionalLoad_Params 

https://codereview.chromium.org/1721813002/diff/80001/content/browser/frame_h...
FrameHostMsg_DidCommitProvisionalLoad_Params -> NavigationHandleImpl

https://codereview.chromium.org/1721813002/diff/80001/content/public/browser/...
File content/public/browser/resource_dispatcher_host_delegate.h (right):

https://codereview.chromium.org/1721813002/diff/80001/content/public/browser/...
content/public/browser/resource_dispatcher_host_delegate.h:134: // Asks the
embedder if this |proxy_server| is a data reduction proxy.
On 2016/03/10 22:49:47, tbansal1 wrote:
> s/if this |proxy_server|/if |proxy_server|/

Done.

[RyanSturm, 2016-03-22 19:05:12]

This is not ready for review. This is a first draft of a different
implementation. Specifically, the DEPS issue shouldn't exist, as the
NavigationUserData classes should be moved to a common location.

[RyanSturm, 2016-03-24 17:43:05]

ryansturm@chromium.org changed reviewers:
- csharrison@chromium.org

[RyanSturm, 2016-03-24 17:43:06]

https://codereview.chromium.org/1721813002/diff/140001/content/browser/frame_...
File content/browser/frame_host/navigation_user_data.h (right):

https://codereview.chromium.org/1721813002/diff/140001/content/browser/frame_...
content/browser/frame_host/navigation_user_data.h:27:
NavigationSupportsUserData* get_navigation_supports_user_data();
Consider changing to SetUserData(void* key, Data data). With this interface
change, we can ensure no dangling references are left on the IO thread. It is
important that the only way a reference to NavigationSupportsUserData gets out
is if ownership transfers with it. Essentially, this change forces the
NavigationUserData to be write only.

[bengr, 2016-04-21 23:04:29]

Chatted offline with nasko@ and ncarter@ and we agreed on an approach where the
embedder (ChromeResourceDispatcherHostDelegate) passes a subclass of a new
NavigationData object as a scoped_ptr to content/public/browser. This happens on
the IO thread. The NavigationData object provides a public virtual destructor
and a virtual clone method. The contract is that content will clone the object
and then pass it to the UI thread where it gets added to a NavigationHandle and
is then made available to a PageloadMetricsObserver.

[nasko, 2016-04-26 20:18:37]

nasko@chromium.org changed reviewers:
+ nasko@chromium.org

[nasko, 2016-04-26 20:18:40]

Looks good overall. Please expand DRP in the CL title and description on first
use, not everyone is familiar with what it means.

https://codereview.chromium.org/1721813002/diff/280001/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_navigation_data.cc (right):

https://codereview.chromium.org/1721813002/diff/280001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_navigation_data.cc:15: ChromeNavigationData*
copy = new ChromeNavigationData();
Why keep a raw pointer? Just use the unique_ptr.

https://codereview.chromium.org/1721813002/diff/280001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_navigation_data.cc:18: return
base::WrapUnique(copy);
You should WrapUnique the return value of the allocation.

https://codereview.chromium.org/1721813002/diff/280001/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_navigation_data.h (right):

https://codereview.chromium.org/1721813002/diff/280001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_navigation_data.h:22: // Creates a new
ChromeNavigationData that is completely independent of the
nit: s/completely independent/deep copy/

https://codereview.chromium.org/1721813002/diff/280001/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/1721813002/diff/280001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:732:
ChromeNavigationData* data = new ChromeNavigationData();
Use unique_ptr.

https://codereview.chromium.org/1721813002/diff/280001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:738:
data->SetDataReductionProxyData(data_reduction_proxy_data->DeepCopy());
So we end up doing two deep copies of data_reduction_proxy_data? As long as this
is intentional, it deserves a comment why.

https://codereview.chromium.org/1721813002/diff/280001/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.h
(right):

https://codereview.chromium.org/1721813002/diff/280001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.h:99:
std::unique_ptr<content::NavigationData> GetNavigationData(
Keep the ordering the same as the inherited interface - this needs to move right
after ShouldEnableLoFiMode with no empty line between them.

https://codereview.chromium.org/1721813002/diff/280001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/1721813002/diff/280001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:304: // Manages the lifetime
of the embedder's NavigationData.
It is the embedder data, it doesn't manage much.

https://codereview.chromium.org/1721813002/diff/280001/content/browser/frame_...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/1721813002/diff/280001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:326: if (navigation_data.get())
{
nit: No need for {} on one line if statements.

https://codereview.chromium.org/1721813002/diff/280001/content/browser/loader...
File content/browser/loader/navigation_resource_handler.cc (right):

https://codereview.chromium.org/1721813002/diff/280001/content/browser/loader...
content/browser/loader/navigation_resource_handler.cc:100: // Ask the embedder
for a NavigationData instance.
nit: Empty line above comment.

https://codereview.chromium.org/1721813002/diff/280001/content/browser/loader...
content/browser/loader/navigation_resource_handler.cc:105:
navigation_data.get());
Why not clone the data here and pass it to the method? It will keep the get/make
a copy/pass it along contract all in one place.

https://codereview.chromium.org/1721813002/diff/280001/content/browser/loader...
File content/browser/loader/navigation_resource_throttle.cc (right):

https://codereview.chromium.org/1721813002/diff/280001/content/browser/loader...
content/browser/loader/navigation_resource_throttle.cc:125: if
(navigation_data.get()) {
nit: No need for {} on one line if statements.

https://codereview.chromium.org/1721813002/diff/280001/content/browser/loader...
File content/browser/loader/navigation_resource_throttle.h (right):

https://codereview.chromium.org/1721813002/diff/280001/content/browser/loader...
content/browser/loader/navigation_resource_throttle.h:26:
ResourceDispatcherHostImpl* rdh);
Use full names for parameters, it isn't that much longer to use
resource_dispatcher_host.

https://codereview.chromium.org/1721813002/diff/280001/content/browser/loader...
content/browser/loader/navigation_resource_throttle.h:40:
ResourceDispatcherHostImpl* rdh_;
Same here, having a longer name makes it a bit easier to read in area of the
code where the type isn't apparent.

https://codereview.chromium.org/1721813002/diff/280001/content/browser/loader...
File content/browser/loader/navigation_url_loader_impl_core.cc (right):

https://codereview.chromium.org/1721813002/diff/280001/content/browser/loader...
content/browser/loader/navigation_url_loader_impl_core.cc:107: navigation_data ?
navigation_data->Clone() : nullptr;
Empty line after this and before next comment.

https://codereview.chromium.org/1721813002/diff/280001/content/public/browser...
File content/public/browser/navigation_data.h (right):

https://codereview.chromium.org/1721813002/diff/280001/content/public/browser...
content/public/browser/navigation_data.h:14: // Copyable interface for embedders
to pass opaque data to content.
Let's add also a bit about the contract of this data type. It is expected to be
set on the IO thread and content will transfer it over to the UI thread.

https://codereview.chromium.org/1721813002/diff/280001/content/public/browser...
content/public/browser/navigation_data.h:18: // Creates a new NavigationData
that is completely independent of the
nit: Empty line above the comment.

s/completely independent/deep copy/.

https://codereview.chromium.org/1721813002/diff/280001/content/public/browser...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/1721813002/diff/280001/content/public/browser...
content/public/browser/navigation_handle.h:10: #include
"base/supports_user_data.h"
No need anymore for supports_user_data.h

https://codereview.chromium.org/1721813002/diff/280001/content/public/browser...
content/public/browser/navigation_handle.h:175: //
ResourceDispatcherHostDelegate::GetNavigationData during commit. This may
"may be a clone" - isn't it definitely a clone/copy?

https://codereview.chromium.org/1721813002/diff/280001/content/public/browser...
content/public/browser/navigation_handle.h:177: virtual NavigationData*
GetNavigationData() const = 0;
No const usage in the content/ public API -
https://www.chromium.org/developers/content-module/content-api.

[RyanSturm, 2016-04-27 23:27:05]

Description was changed from

==========
Adding DRP specfic UMA for FirstContentfulPaint
This consumes the PageLoad.Timing2 code path to get data
specific to DRP with regards to having DRP on and being in the
lofi experiment. Right now, it only considers
NavigationToFirstContentfulPaint.

BUG=578321
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Adding DataReductionProxy (DRP) specfic UMA for FirstContentfulPaint.
This consumes the PageLoad.Timing2 code path to get data specific to DRP with
regards to having DRP on and being in the lofi experiment. Right now, it only
considers
NavigationToFirstContentfulPaint.

BUG=578321
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[RyanSturm, 2016-04-27 23:27:47]

bengr, tbansal1: ptal

https://codereview.chromium.org/1721813002/diff/140001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://codereview.chromium.org/1721813002/diff/140001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:164:
content::NavigationUserData* navigation_user_data =
Check if this is a DRP proxy and not some other proxy

https://codereview.chromium.org/1721813002/diff/280001/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_navigation_data.cc (right):

https://codereview.chromium.org/1721813002/diff/280001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_navigation_data.cc:15: ChromeNavigationData*
copy = new ChromeNavigationData();
On 2016/04/26 20:18:39, nasko (slow) wrote:
> Why keep a raw pointer? Just use the unique_ptr.

Done.

https://codereview.chromium.org/1721813002/diff/280001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_navigation_data.cc:18: return
base::WrapUnique(copy);
On 2016/04/26 20:18:39, nasko (slow) wrote:
> You should WrapUnique the return value of the allocation.

Done.

https://codereview.chromium.org/1721813002/diff/280001/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_navigation_data.h (right):

https://codereview.chromium.org/1721813002/diff/280001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_navigation_data.h:22: // Creates a new
ChromeNavigationData that is completely independent of the
On 2016/04/26 20:18:39, nasko (slow) wrote:
> nit: s/completely independent/deep copy/

Done.

https://codereview.chromium.org/1721813002/diff/280001/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/1721813002/diff/280001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:732:
ChromeNavigationData* data = new ChromeNavigationData();
On 2016/04/26 20:18:39, nasko (slow) wrote:
> Use unique_ptr.

Done.

https://codereview.chromium.org/1721813002/diff/280001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:738:
data->SetDataReductionProxyData(data_reduction_proxy_data->DeepCopy());
On 2016/04/26 20:18:39, nasko (slow) wrote:
> So we end up doing two deep copies of data_reduction_proxy_data? As long as
this
> is intentional, it deserves a comment why.

I'll add a comment about this; the basic idea I had: URLRequest should not lose
it's copy of the data, and ChromeNavigationData owns it's reference to
DataReductionProxyData, so a DeepCopy is needed here. The clone is not strictly
necessary later as of this CL, but it allows content an explicit way to tell the
embedder to give it an object of the UI thread. If a future CL handles ownership
for an object in ChromeNavigationData differently, the clone might be needed
later.

https://codereview.chromium.org/1721813002/diff/280001/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.h
(right):

https://codereview.chromium.org/1721813002/diff/280001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.h:99:
std::unique_ptr<content::NavigationData> GetNavigationData(
On 2016/04/26 20:18:39, nasko (slow) wrote:
> Keep the ordering the same as the inherited interface - this needs to move
right
> after ShouldEnableLoFiMode with no empty line between them.

Done.

https://codereview.chromium.org/1721813002/diff/280001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/1721813002/diff/280001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:304: // Manages the lifetime
of the embedder's NavigationData.
On 2016/04/26 20:18:39, nasko (slow) wrote:
> It is the embedder data, it doesn't manage much.

Done.

https://codereview.chromium.org/1721813002/diff/280001/content/browser/frame_...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/1721813002/diff/280001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:326: if (navigation_data.get())
{
On 2016/04/26 20:18:39, nasko (slow) wrote:
> nit: No need for {} on one line if statements.

Done.

https://codereview.chromium.org/1721813002/diff/280001/content/browser/loader...
File content/browser/loader/navigation_resource_handler.cc (right):

https://codereview.chromium.org/1721813002/diff/280001/content/browser/loader...
content/browser/loader/navigation_resource_handler.cc:100: // Ask the embedder
for a NavigationData instance.
On 2016/04/26 20:18:39, nasko (slow) wrote:
> nit: Empty line above comment.

Done.

https://codereview.chromium.org/1721813002/diff/280001/content/browser/loader...
content/browser/loader/navigation_resource_handler.cc:105:
navigation_data.get());
On 2016/04/26 20:18:39, nasko (slow) wrote:
> Why not clone the data here and pass it to the method? It will keep the
get/make
> a copy/pass it along contract all in one place. 

Done.

https://codereview.chromium.org/1721813002/diff/280001/content/browser/loader...
File content/browser/loader/navigation_resource_throttle.cc (right):

https://codereview.chromium.org/1721813002/diff/280001/content/browser/loader...
content/browser/loader/navigation_resource_throttle.cc:125: if
(navigation_data.get()) {
On 2016/04/26 20:18:40, nasko (slow) wrote:
> nit: No need for {} on one line if statements.

Done.

https://codereview.chromium.org/1721813002/diff/280001/content/browser/loader...
File content/browser/loader/navigation_resource_throttle.h (right):

https://codereview.chromium.org/1721813002/diff/280001/content/browser/loader...
content/browser/loader/navigation_resource_throttle.h:26:
ResourceDispatcherHostImpl* rdh);
On 2016/04/26 20:18:40, nasko (slow) wrote:
> Use full names for parameters, it isn't that much longer to use
> resource_dispatcher_host. 

Done.

https://codereview.chromium.org/1721813002/diff/280001/content/browser/loader...
content/browser/loader/navigation_resource_throttle.h:40:
ResourceDispatcherHostImpl* rdh_;
On 2016/04/26 20:18:40, nasko (slow) wrote:
> Same here, having a longer name makes it a bit easier to read in area of the
> code where the type isn't apparent.

Done.

https://codereview.chromium.org/1721813002/diff/280001/content/browser/loader...
File content/browser/loader/navigation_url_loader_impl_core.cc (right):

https://codereview.chromium.org/1721813002/diff/280001/content/browser/loader...
content/browser/loader/navigation_url_loader_impl_core.cc:107: navigation_data ?
navigation_data->Clone() : nullptr;
On 2016/04/26 20:18:40, nasko (slow) wrote:
> Empty line after this and before next comment.

Done.

https://codereview.chromium.org/1721813002/diff/280001/content/public/browser...
File content/public/browser/navigation_data.h (right):

https://codereview.chromium.org/1721813002/diff/280001/content/public/browser...
content/public/browser/navigation_data.h:14: // Copyable interface for embedders
to pass opaque data to content.
On 2016/04/26 20:18:40, nasko (slow) wrote:
> Let's add also a bit about the contract of this data type. It is expected to
be
> set on the IO thread and content will transfer it over to the UI thread.

Done.

https://codereview.chromium.org/1721813002/diff/280001/content/public/browser...
content/public/browser/navigation_data.h:18: // Creates a new NavigationData
that is completely independent of the
On 2016/04/26 20:18:40, nasko (slow) wrote:
> nit: Empty line above the comment.
> 
> s/completely independent/deep copy/.

Done.

https://codereview.chromium.org/1721813002/diff/280001/content/public/browser...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/1721813002/diff/280001/content/public/browser...
content/public/browser/navigation_handle.h:10: #include
"base/supports_user_data.h"
On 2016/04/26 20:18:40, nasko (slow) wrote:
> No need anymore for supports_user_data.h

Done.

https://codereview.chromium.org/1721813002/diff/280001/content/public/browser...
content/public/browser/navigation_handle.h:175: //
ResourceDispatcherHostDelegate::GetNavigationData during commit. This may
On 2016/04/26 20:18:40, nasko (slow) wrote:
> "may be a clone" - isn't it definitely a clone/copy?

Done.

https://codereview.chromium.org/1721813002/diff/280001/content/public/browser...
content/public/browser/navigation_handle.h:177: virtual NavigationData*
GetNavigationData() const = 0;
On 2016/04/26 20:18:40, nasko (slow) wrote:
> No const usage in the content/ public API -
> https://www.chromium.org/developers/content-module/content-api.

Done.

[tbansal1, 2016-04-27 23:44:48]

I will let bengr review the CL since at this point he is more familiar with it. 

(I did had 2 nits below).

https://codereview.chromium.org/1721813002/diff/300001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_data.cc
(right):

https://codereview.chromium.org/1721813002/diff/300001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.cc:26:
net::URLRequest* request,
Better to pass |request| by const ref. That makes it clearer that the function
is not going to modify |request|.

https://codereview.chromium.org/1721813002/diff/300001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/1721813002/diff/300001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:220: navigation_data_ =
std::move(navigation_data);
This implementation should go in .cc file. In general, if the implementation is
in .cc file, then the function name should be  in the format:
set_variable_name()

[RyanSturm, 2016-04-28 21:22:03]

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:163:
!data_reduction_proxy_config_->IsDataReductionProxy(
Add a proxy_server().is_valid() and proxy_server().is_direct checks before this
check. And don't early return. Instead, call the set_used_data_reduction_proxy()
call inside an if check. See crbug.com/607693 for tracking the early return.

[bengr, 2016-04-29 21:14:23]

Looking good. It might make sense to split this into two CLs. The first would do
all the plumbing. The second would use the plumbing to add DRP-specific data and
write UMA. Wdyt?

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
(right):

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:19:
namespace internal {
Why does this namespace need to be named? Can it be anonymous?

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:34:
: is_using_lofi_(false), used_data_reduction_proxy_(false) {}
Should these be an enum? I ask because used_data_reduction_proxy_ must be true
if is_using_lofi_ is true. In the future, we might want to record UMA per
individual optimization also. Something like:

enum {
  PREVIEW_NONE,
  PREVIEW_NONE_VIA_DATA_REDUCTION_PROXY,
  PREVIEW_LOFI,
  PREVIEW_WEBLITE,
  PREVIEW_RANGE_REQUESTS,
  ...
}

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:43:
// ResourceDispatcherHostDelegate::GetNavigationData during commit, which is
My parser stopped working here. When does cloning happen? What is returned
during commit, the original or the clone? Are both always ChromeNavigationData?

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:48:
if (!chrome_navigation_data) {
Remove curly braces.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:53:
if (data) {
I would follow the patter in line 48:

if (!data)
  return;
used_data_reduction_proxy_ = ...
...

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:65:
// Recording UMA for FirstContentfulPaint for various DataReductionProxy
nit: Recording UMA for FirstContentfulPaint -> Record first contentful paint UMA

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:75:
if (is_using_lofi_) {
if (!is_using_lofi_)
  return;
...

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h
(right):

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:23:
// Various UMA histogram names for DataReductionProxy core page load metrics
nit: Add a period.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:41:
void RecordTimingHistograms(
Add a blank line above.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:47:
bool is_using_lofi_;
Rename as used_lofi. Also, is this only if a lofi transformation was performed?
It shouldn't be true if LoFi is requested but the server does not transform.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:81:
bool is_using_lofi_;
lofi_used_?

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:111:
// Navigate again to force logging.
Add an explanation of why a second navigation forces logging. Also, you mean UMA
reporting, not logging, right?

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_navigation_data.h (right):

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_navigation_data.h:39: // Manages the
lifetime of optionally passed in DataReductionProxy
nit: optionally passed in -> optional

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_navigation_data_unittest.cc (right):

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_navigation_data_unittest.cc:21:
std::unique_ptr<ChromeNavigationData> data(new ChromeNavigationData());
#include "../chrome_navigation_data.h"

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:733: //
Get the existing Data instance off of URLRequest.
Delete this comment.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:735:
data_reduction_proxy::DataReductionProxyData::DataForRequest(request,
Maybe the reason you felt compelled to add a comment is that the function name
doesn't clearly indicate what it does. How about GetData()?

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:736:
false);
This second parameter is probably the cleaner way to go. Did you consider having
a separate function called GetDataAndCreateIfNecessary()?

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:739: //
same object. This copy will be shortlived as it will be DeepCopy'd again
DeepCopy'd -> deep copied

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:740: //
when content makes a clone of NavigationData for the UI thread.
You should make a note somewhere (probably on the DeepCopy method) that we
assume the Data object is reasonably inexpensive to deep copy.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.h
(right):

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.h:94:
net::URLRequest* request) const override;
Forward declare URLRequest.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
File
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc
(right):

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:111:
bool add_data_reduction_proxy_data_;
Ideally, you'd rename as should_add_data_reduction_proxy_data_to_request_;
should_add_data_reduction_proxy_data_ is ok too. Also do this for line 151.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:118:
// Helper class to track DidFinishNavigation and it verify NavigationData being
"and it verify" ?

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:138:
EXPECT_TRUE(!data->GetDataReductionProxyData());
I would flip this around:
EXPECT_FALSE(data->GetDataREductionProxyData());

and the next would would be EXPECT_TRUE.

Alternatively, you could explicitly check that it is equal to nullptr and not
nullptr in line 140.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:148:
std::unique_ptr<content::NavigationData> cloned_data;
Where is this used?

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:152:
// The MessageLoopRunner used to spin the message loop.
This comment is pretty useless. Delete.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:198:
void SetAddDataReductionProxyData(bool add_data) {
should_add_data

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:268:
"window.location.href = '" + embedded_test_server()->base_url().spec() +
This is weird to me. Are you just checking that you navigated to the url?

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
File
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_unittest.cc
(right):

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_unittest.cc:1:
// Copyright 2014 The Chromium Authors. All rights reserved.
Wrong year.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_unittest.cc:33:
GURL("content.request.any.port"), net::RequestPriority::IDLE, nullptr));
#include "url/gurl.h"

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_unittest.cc:34:
// Add DRP data to URLRequest
Add DataReductionProxyData to the URLRequest.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_unittest.cc:39:
std::unique_ptr<ChromeResourceDispatcherHostDelegate> delegate(
#include
"chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.h"

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_unittest.cc:45:
data_reduction_proxy::DataReductionProxyData* drp_data_copy =
You should spell out every drp as data_reduction_proxy everywhere in this CL.
Sorry.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_unittest.cc:47:
// The DRP_data should be a copy off the one on URLRequest
copy off -> deep copy of

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_unittest.cc:49:
// Make sure DRP Data was copied.
DataReductionProxyData

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h
(right):

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:19: //
DataReductionProxy related data that can be put into UserData or other
DataReductionProxy-related

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:20: //
storage vehicles to communicate information in one object.
"to communicated information in one object" -> "to associate this data with the
object that owns it"

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:25:
bool get_used_data_reduction_proxy() { return used_data_reduction_proxy_; }
remove "get_"

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:29: //
Whether Lo-Fi was turned on for this request or navigation.
If this really means that Lo-Fi was requested for this navigation and not that
Lo-Fi was used for this navigation, you should make the distinction in the
comment. You probably also want to change the variable name to something like
"is_lofi_mode_requested."

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:30:
bool get_is_using_lofi() { return is_using_lofi_; }
remove "get_"

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:36:
static DataReductionProxyData* DataForRequest(net::URLRequest* request,
GetData, or GetDataForRequest

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:39: //
a different thread.
Note that we expect several of deep copies per navigation, so the DeepCopy
should be kept inexpensive.

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:166:
// Get an existing Data instance off of URLRequest or create a new Data
I'd rewrite as:
Retrieves DataReductionProxyData from a request, creating a new instance if
needed.

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:185:
// Get an existing Data instance off of URLRequest or create a new Data
Same here.

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:188:
DataReductionProxyData::DataForRequest(request, true);
Why is if (data) needed? You know there will be data because request is non-null
and you passed in true.

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc
(right):

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:440:
net::ProxyInfo data_reduction_proxy_info;
#include "net/proxy/proxy_info.h"

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:442:
base::TrimString(params()->DefaultOrigin(), "/", &data_reduction_proxy);
#include "base/string/string_util.h"

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:447:
{
Add a blank line above.

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:452:
GURL("http://www.google.com/"), nullptr, std::string(), 0));
#include gurl.h

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:459:
if (!tests[i].used_drp) {
Why is this all negated?

if (tests[i].used_drp) {
  EXPECT_TRUE(data);
  ...
} else {
  EXPECT_FALSE(data);
}

https://codereview.chromium.org/1721813002/diff/340001/content/browser/frame_...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/1721813002/diff/340001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:325: if (navigation_data.get())
No need for .get()

https://codereview.chromium.org/1721813002/diff/340001/content/browser/frame_...
File content/browser/frame_host/navigation_request.h (right):

https://codereview.chromium.org/1721813002/diff/340001/content/browser/frame_...
content/browser/frame_host/navigation_request.h:179:
std::unique_ptr<StreamHandle> body,
Forward declare StreamHandle.

https://codereview.chromium.org/1721813002/diff/340001/content/browser/frame_...
File content/browser/frame_host/navigator_impl_unittest.cc (right):

https://codereview.chromium.org/1721813002/diff/340001/content/browser/frame_...
content/browser/frame_host/navigator_impl_unittest.cc:158: response,
MakeEmptyStream(), base::WrapUnique(new TestNavigationData()));
Do you have any tests where there isn't any NavigationData?

https://codereview.chromium.org/1721813002/diff/340001/content/browser/loader...
File content/browser/loader/navigation_resource_handler.cc (right):

https://codereview.chromium.org/1721813002/diff/340001/content/browser/loader...
content/browser/loader/navigation_resource_handler.cc:102:
std::unique_ptr<NavigationData> navigation_data(nullptr);
I'm not sure about Chromium style, but you can do:
std::unique_ptr<NavigationData> navigation_data;

https://codereview.chromium.org/1721813002/diff/340001/content/browser/loader...
content/browser/loader/navigation_resource_handler.cc:105: // Clone the embedder
NavigationData before moving it to the UI thread.
embedder -> embedder's

https://codereview.chromium.org/1721813002/diff/340001/content/browser/loader...
File content/browser/loader/navigation_resource_throttle.cc (right):

https://codereview.chromium.org/1721813002/diff/340001/content/browser/loader...
content/browser/loader/navigation_resource_throttle.cc:124: if
(navigation_data.get())
Remove .get()

https://codereview.chromium.org/1721813002/diff/340001/content/browser/loader...
content/browser/loader/navigation_resource_throttle.cc:230:
std::unique_ptr<NavigationData> navigation_data(nullptr);
remove (nullptr)

https://codereview.chromium.org/1721813002/diff/340001/content/browser/loader...
content/browser/loader/navigation_resource_throttle.cc:236:
std::unique_ptr<NavigationData> clone(nullptr);
This might be clearer if you put this up after 230:

std::unique_ptr<NavigationData> navigation_data, navigation_data_clone;
if (...) {
  navigation_data = ...
  if (navigation_data)
    clone = ...
}

https://codereview.chromium.org/1721813002/diff/340001/content/browser/loader...
File content/browser/loader/navigation_resource_throttle.h (right):

https://codereview.chromium.org/1721813002/diff/340001/content/browser/loader...
content/browser/loader/navigation_resource_throttle.h:41:
ResourceDispatcherHostImpl* resource_dispatcher_host_;
Is this guaranteed to be valid for the lifetime of the throttle?

https://codereview.chromium.org/1721813002/diff/340001/content/browser/loader...
File content/browser/loader/navigation_url_loader_delegate.h (right):

https://codereview.chromium.org/1721813002/diff/340001/content/browser/loader...
content/browser/loader/navigation_url_loader_delegate.h:35: // |body_stream|.
Also describe navigation_data

https://codereview.chromium.org/1721813002/diff/340001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/1721813002/diff/340001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:2332: new
NavigationResourceHandler(new_request.get(), loader, this));
I find it a little weird that NRH takes an Impl object, but they are in the same
directory, so I suppose it's ok.

https://codereview.chromium.org/1721813002/diff/340001/content/public/browser...
File content/public/browser/navigation_data.h (right):

https://codereview.chromium.org/1721813002/diff/340001/content/public/browser...
content/public/browser/navigation_data.h:10: #include "base/macros.h"
Why is this here? Did you mean to add DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/1721813002/diff/340001/content/public/browser...
content/public/browser/navigation_data.h:16: // UI thread as a Clone.
Clone -> clone

https://codereview.chromium.org/1721813002/diff/340001/content/public/browser...
content/public/browser/navigation_data.h:21: // Creates a new NavigationData
that is  a deep copy of the original, and
is  a -> is a

"and could be used cross-thread" seems unnecessary.

https://codereview.chromium.org/1721813002/diff/340001/content/public/browser...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/1721813002/diff/340001/content/public/browser...
content/public/browser/navigation_handle.h:11: #include
"content/public/browser/navigation_data.h"
Can't this be forward declared?

https://codereview.chromium.org/1721813002/diff/340001/content/public/browser...
File content/public/browser/resource_dispatcher_host_delegate.cc (right):

https://codereview.chromium.org/1721813002/diff/340001/content/public/browser...
content/public/browser/resource_dispatcher_host_delegate.cc:7: #include
"content/public/browser/navigation_data.h"
Forward declare instead.

https://codereview.chromium.org/1721813002/diff/340001/content/public/browser...
File content/public/browser/resource_dispatcher_host_delegate.h (right):

https://codereview.chromium.org/1721813002/diff/340001/content/public/browser...
content/public/browser/resource_dispatcher_host_delegate.h:137: net::URLRequest*
request) const;
Can this parameter be const?

[RyanSturm, 2016-05-02 19:52:22]

Addressed all comments raised by bengr@

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
(right):

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:19:
namespace internal {
On 2016/04/29 21:14:20, bengr wrote:
> Why does this namespace need to be named? Can it be anonymous?

So it has external linkage. I don't think the extern keyword is enough to make
unnamed namespace variables externally linked. Alternatively, I could copy these
to the unittests, but I prefer having a shared definition.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:34:
: is_using_lofi_(false), used_data_reduction_proxy_(false) {}
On 2016/04/29 21:14:20, bengr wrote:
> Should these be an enum? I ask because used_data_reduction_proxy_ must be true
> if is_using_lofi_ is true. In the future, we might want to record UMA per
> individual optimization also. Something like:
> 
> enum {
>   PREVIEW_NONE,
>   PREVIEW_NONE_VIA_DATA_REDUCTION_PROXY,
>   PREVIEW_LOFI,
>   PREVIEW_WEBLITE,
>   PREVIEW_RANGE_REQUESTS,
>   ...
> }

I think it should be an enum later if we find a good way. There are a lot more
combinations (what we request and what the sever gave us may differ), so I'm
hesitant right now. At some point we might change this, but right now, I don't
think we should. I think, we might actually want to do an enum of what mode was
requested, but right now, I want the bool for used_data_reduction_proxy to be
it's own state.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:43:
// ResourceDispatcherHostDelegate::GetNavigationData during commit, which is
On 2016/04/29 21:14:20, bengr wrote:
> My parser stopped working here. When does cloning happen? What is returned
> during commit, the original or the clone? Are both always
ChromeNavigationData?

Done.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:48:
if (!chrome_navigation_data) {
On 2016/04/29 21:14:20, bengr wrote:
> Remove curly braces.

Done.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:53:
if (data) {
On 2016/04/29 21:14:20, bengr wrote:
> I would follow the patter in line 48:
> 
> if (!data)
>   return;
> used_data_reduction_proxy_ = ...
> ...

Done.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:65:
// Recording UMA for FirstContentfulPaint for various DataReductionProxy
On 2016/04/29 21:14:20, bengr wrote:
> nit: Recording UMA for FirstContentfulPaint -> Record first contentful paint
UMA

Done.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:75:
if (is_using_lofi_) {
On 2016/04/29 21:14:20, bengr wrote:
> if (!is_using_lofi_)
>   return;
> ...

Done.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h
(right):

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:23:
// Various UMA histogram names for DataReductionProxy core page load metrics
On 2016/04/29 21:14:20, bengr wrote:
> nit: Add a period.

Done.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:41:
void RecordTimingHistograms(
On 2016/04/29 21:14:20, bengr wrote:
> Add a blank line above.

Done.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:47:
bool is_using_lofi_;
On 2016/04/29 21:14:20, bengr wrote:
> Rename as used_lofi. Also, is this only if a lofi transformation was
performed?
> It shouldn't be true if LoFi is requested but the server does not transform.

Right. I'll refactor to reflect that LoFi was requested for this navigation. The
current UMA appears to make the same decision.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:81:
bool is_using_lofi_;
On 2016/04/29 21:14:20, bengr wrote:
> lofi_used_?

Done.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:111:
// Navigate again to force logging.
On 2016/04/29 21:14:20, bengr wrote:
> Add an explanation of why a second navigation forces logging. Also, you mean
UMA
> reporting, not logging, right?

Done.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_navigation_data.h (right):

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_navigation_data.h:39: // Manages the
lifetime of optionally passed in DataReductionProxy
On 2016/04/29 21:14:20, bengr wrote:
> nit: optionally passed in -> optional

Done.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_navigation_data_unittest.cc (right):

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_navigation_data_unittest.cc:21:
std::unique_ptr<ChromeNavigationData> data(new ChromeNavigationData());
On 2016/04/29 21:14:20, bengr wrote:
> #include "../chrome_navigation_data.h"

:/

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:733: //
Get the existing Data instance off of URLRequest.
On 2016/04/29 21:14:20, bengr wrote:
> Delete this comment.

Done.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:735:
data_reduction_proxy::DataReductionProxyData::DataForRequest(request,
On 2016/04/29 21:14:20, bengr wrote:
> Maybe the reason you felt compelled to add a comment is that the function name
> doesn't clearly indicate what it does. How about GetData()?

Done.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:736:
false);
On 2016/04/29 21:14:20, bengr wrote:
> This second parameter is probably the cleaner way to go. Did you consider
having
> a separate function called GetDataAndCreateIfNecessary()? 

Because of the difference in const-ness of the URLRequest in the two functions,
the alternative makes a lot of sense. It allows the URLRequest* in
GetNavigationData to be const for instance.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:739: //
same object. This copy will be shortlived as it will be DeepCopy'd again
On 2016/04/29 21:14:20, bengr wrote:
> DeepCopy'd -> deep copied

Done.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:740: //
when content makes a clone of NavigationData for the UI thread.
On 2016/04/29 21:14:20, bengr wrote:
> You should make a note somewhere (probably on the DeepCopy method) that we
> assume the Data object is reasonably inexpensive to deep copy.

Done.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.h
(right):

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.h:94:
net::URLRequest* request) const override;
On 2016/04/29 21:14:21, bengr wrote:
> Forward declare URLRequest.

Done.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
File
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc
(right):

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:111:
bool add_data_reduction_proxy_data_;
On 2016/04/29 21:14:21, bengr wrote:
> Ideally, you'd rename as should_add_data_reduction_proxy_data_to_request_;
> should_add_data_reduction_proxy_data_ is ok too. Also do this for line 151.

Done.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:118:
// Helper class to track DidFinishNavigation and it verify NavigationData being
On 2016/04/29 21:14:21, bengr wrote:
> "and it verify" ?

Done.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:138:
EXPECT_TRUE(!data->GetDataReductionProxyData());
On 2016/04/29 21:14:21, bengr wrote:
> I would flip this around:
> EXPECT_FALSE(data->GetDataREductionProxyData());
> 
> and the next would would be EXPECT_TRUE.
> 
> Alternatively, you could explicitly check that it is equal to nullptr and not
> nullptr in line 140.

Done.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:148:
std::unique_ptr<content::NavigationData> cloned_data;
On 2016/04/29 21:14:21, bengr wrote:
> Where is this used?

Done.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:152:
// The MessageLoopRunner used to spin the message loop.
On 2016/04/29 21:14:21, bengr wrote:
> This comment is pretty useless. Delete.

Done.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:198:
void SetAddDataReductionProxyData(bool add_data) {
On 2016/04/29 21:14:21, bengr wrote:
> should_add_data

Done.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:268:
"window.location.href = '" + embedded_test_server()->base_url().spec() +
On 2016/04/29 21:14:21, bengr wrote:
> This is weird to me. Are you just checking that you navigated to the url?

Fixed the weirdness.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
File
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_unittest.cc
(right):

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_unittest.cc:1:
// Copyright 2014 The Chromium Authors. All rights reserved.
On 2016/04/29 21:14:21, bengr wrote:
> Wrong year.

Done.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_unittest.cc:33:
GURL("content.request.any.port"), net::RequestPriority::IDLE, nullptr));
On 2016/04/29 21:14:21, bengr wrote:
> #include "url/gurl.h"

Done.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_unittest.cc:34:
// Add DRP data to URLRequest
On 2016/04/29 21:14:21, bengr wrote:
> Add DataReductionProxyData to the URLRequest.

Done.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_unittest.cc:39:
std::unique_ptr<ChromeResourceDispatcherHostDelegate> delegate(
On 2016/04/29 21:14:21, bengr wrote:
> #include
> "chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.h"

Done.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_unittest.cc:45:
data_reduction_proxy::DataReductionProxyData* drp_data_copy =
On 2016/04/29 21:14:21, bengr wrote:
> You should spell out every drp as data_reduction_proxy everywhere in this CL.
> Sorry.

Done.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_unittest.cc:47:
// The DRP_data should be a copy off the one on URLRequest
On 2016/04/29 21:14:21, bengr wrote:
> copy off -> deep copy of

Done.

https://codereview.chromium.org/1721813002/diff/340001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_unittest.cc:49:
// Make sure DRP Data was copied.
On 2016/04/29 21:14:21, bengr wrote:
> DataReductionProxyData

Done.

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h
(right):

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:19: //
DataReductionProxy related data that can be put into UserData or other
On 2016/04/29 21:14:22, bengr wrote:
> DataReductionProxy-related

Done.

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:20: //
storage vehicles to communicate information in one object.
On 2016/04/29 21:14:21, bengr wrote:
> "to communicated information in one object" -> "to associate this data with
the
> object that owns it"

Done.

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:25:
bool get_used_data_reduction_proxy() { return used_data_reduction_proxy_; }
On 2016/04/29 21:14:21, bengr wrote:
> remove "get_"

Done.

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:29: //
Whether Lo-Fi was turned on for this request or navigation.
On 2016/04/29 21:14:21, bengr wrote:
> If this really means that Lo-Fi was requested for this navigation and not that
> Lo-Fi was used for this navigation, you should make the distinction in the
> comment. You probably also want to change the variable name to something like
> "is_lofi_mode_requested."

I shortened to lofi_requested.

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:30:
bool get_is_using_lofi() { return is_using_lofi_; }
On 2016/04/29 21:14:21, bengr wrote:
> remove "get_"

Done.

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:36:
static DataReductionProxyData* DataForRequest(net::URLRequest* request,
On 2016/04/29 21:14:22, bengr wrote:
> GetData, or GetDataForRequest

Done.

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:39: //
a different thread.
On 2016/04/29 21:14:22, bengr wrote:
> Note that we expect several of deep copies per navigation, so the DeepCopy
> should be kept inexpensive.

Done.

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:163:
!data_reduction_proxy_config_->IsDataReductionProxy(
On 2016/04/28 21:22:03, RyanSturm wrote:
> Add a proxy_server().is_valid() and proxy_server().is_direct checks before
this
> check. And don't early return. Instead, call the
set_used_data_reduction_proxy()
> call inside an if check. See crbug.com/607693 for tracking the early return.

Done.

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:166:
// Get an existing Data instance off of URLRequest or create a new Data
On 2016/04/29 21:14:22, bengr wrote:
> I'd rewrite as:
> Retrieves DataReductionProxyData from a request, creating a new instance if
> needed.

Done.

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:185:
// Get an existing Data instance off of URLRequest or create a new Data
On 2016/04/29 21:14:22, bengr wrote:
> Same here.

Done.

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:188:
DataReductionProxyData::DataForRequest(request, true);
On 2016/04/29 21:14:22, bengr wrote:
> Why is if (data) needed? You know there will be data because request is
non-null
> and you passed in true.

Done.

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc
(right):

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:440:
net::ProxyInfo data_reduction_proxy_info;
On 2016/04/29 21:14:22, bengr wrote:
> #include "net/proxy/proxy_info.h"

Done.

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:442:
base::TrimString(params()->DefaultOrigin(), "/", &data_reduction_proxy);
On 2016/04/29 21:14:22, bengr wrote:
> #include "base/string/string_util.h"

Done.

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:447:
{
On 2016/04/29 21:14:22, bengr wrote:
> Add a blank line above.

Done.

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:452:
GURL("http://www.google.com/"), nullptr, std::string(), 0));
On 2016/04/29 21:14:22, bengr wrote:
> #include gurl.h

Done.

https://codereview.chromium.org/1721813002/diff/340001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:459:
if (!tests[i].used_drp) {
On 2016/04/29 21:14:22, bengr wrote:
> Why is this all negated?
> 
> if (tests[i].used_drp) {
>   EXPECT_TRUE(data);
>   ...
> } else {
>   EXPECT_FALSE(data);
> }

Done.

https://codereview.chromium.org/1721813002/diff/340001/content/browser/frame_...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/1721813002/diff/340001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:325: if (navigation_data.get())
On 2016/04/29 21:14:22, bengr wrote:
> No need for .get()

Done.

https://codereview.chromium.org/1721813002/diff/340001/content/browser/frame_...
File content/browser/frame_host/navigation_request.h (right):

https://codereview.chromium.org/1721813002/diff/340001/content/browser/frame_...
content/browser/frame_host/navigation_request.h:179:
std::unique_ptr<StreamHandle> body,
On 2016/04/29 21:14:22, bengr wrote:
> Forward declare StreamHandle.

Done.

https://codereview.chromium.org/1721813002/diff/340001/content/browser/frame_...
File content/browser/frame_host/navigator_impl_unittest.cc (right):

https://codereview.chromium.org/1721813002/diff/340001/content/browser/frame_...
content/browser/frame_host/navigator_impl_unittest.cc:158: response,
MakeEmptyStream(), base::WrapUnique(new TestNavigationData()));
On 2016/04/29 21:14:22, bengr wrote:
> Do you have any tests where there isn't any NavigationData?

I changed the first one to be null, but I'll leave the rest as mock.

https://codereview.chromium.org/1721813002/diff/340001/content/browser/loader...
File content/browser/loader/navigation_resource_handler.cc (right):

https://codereview.chromium.org/1721813002/diff/340001/content/browser/loader...
content/browser/loader/navigation_resource_handler.cc:102:
std::unique_ptr<NavigationData> navigation_data(nullptr);
On 2016/04/29 21:14:22, bengr wrote:
> I'm not sure about Chromium style, but you can do:
> std::unique_ptr<NavigationData> navigation_data;

Done.

https://codereview.chromium.org/1721813002/diff/340001/content/browser/loader...
content/browser/loader/navigation_resource_handler.cc:105: // Clone the embedder
NavigationData before moving it to the UI thread.
On 2016/04/29 21:14:22, bengr wrote:
> embedder -> embedder's

Done.

https://codereview.chromium.org/1721813002/diff/340001/content/browser/loader...
File content/browser/loader/navigation_resource_throttle.cc (right):

https://codereview.chromium.org/1721813002/diff/340001/content/browser/loader...
content/browser/loader/navigation_resource_throttle.cc:124: if
(navigation_data.get())
On 2016/04/29 21:14:22, bengr wrote:
> Remove .get()

Done.

https://codereview.chromium.org/1721813002/diff/340001/content/browser/loader...
content/browser/loader/navigation_resource_throttle.cc:230:
std::unique_ptr<NavigationData> navigation_data(nullptr);
On 2016/04/29 21:14:22, bengr wrote:
> remove (nullptr)

Done.

https://codereview.chromium.org/1721813002/diff/340001/content/browser/loader...
content/browser/loader/navigation_resource_throttle.cc:236:
std::unique_ptr<NavigationData> clone(nullptr);
On 2016/04/29 21:14:22, bengr wrote:
> This might be clearer if you put this up after 230:
> 
> std::unique_ptr<NavigationData> navigation_data, navigation_data_clone;
> if (...) {
>   navigation_data = ...
>   if (navigation_data)
>     clone = ...
> }

Done.

https://codereview.chromium.org/1721813002/diff/340001/content/browser/loader...
File content/browser/loader/navigation_resource_throttle.h (right):

https://codereview.chromium.org/1721813002/diff/340001/content/browser/loader...
content/browser/loader/navigation_resource_throttle.h:41:
ResourceDispatcherHostImpl* resource_dispatcher_host_;
On 2016/04/29 21:14:23, bengr wrote:
> Is this guaranteed to be valid for the lifetime of the throttle?

RDH doesn't change and owns the ResourceLoader (via pending_loaders_), which
owns this. It will be valid here.

https://codereview.chromium.org/1721813002/diff/340001/content/browser/loader...
File content/browser/loader/navigation_url_loader_delegate.h (right):

https://codereview.chromium.org/1721813002/diff/340001/content/browser/loader...
content/browser/loader/navigation_url_loader_delegate.h:35: // |body_stream|.
On 2016/04/29 21:14:23, bengr wrote:
> Also describe navigation_data

Done.

https://codereview.chromium.org/1721813002/diff/340001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/1721813002/diff/340001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:2332: new
NavigationResourceHandler(new_request.get(), loader, this));
On 2016/04/29 21:14:23, bengr wrote:
> I find it a little weird that NRH takes an Impl object, but they are in the
same
> directory, so I suppose it's ok. 

I suppose this could take an rdhd instead, but I prefer rdh.

https://codereview.chromium.org/1721813002/diff/340001/content/public/browser...
File content/public/browser/navigation_data.h (right):

https://codereview.chromium.org/1721813002/diff/340001/content/public/browser...
content/public/browser/navigation_data.h:10: #include "base/macros.h"
On 2016/04/29 21:14:23, bengr wrote:
> Why is this here? Did you mean to add DISALLOW_COPY_AND_ASSIGN?

Removing, each inheritor should deal with disallowing copy/assign.

https://codereview.chromium.org/1721813002/diff/340001/content/public/browser...
content/public/browser/navigation_data.h:16: // UI thread as a Clone.
On 2016/04/29 21:14:23, bengr wrote:
> Clone -> clone

Done.

https://codereview.chromium.org/1721813002/diff/340001/content/public/browser...
content/public/browser/navigation_data.h:21: // Creates a new NavigationData
that is  a deep copy of the original, and
On 2016/04/29 21:14:23, bengr wrote:
> is  a -> is a
> 
> "and could be used cross-thread" seems unnecessary.

Done.

https://codereview.chromium.org/1721813002/diff/340001/content/public/browser...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/1721813002/diff/340001/content/public/browser...
content/public/browser/navigation_handle.h:11: #include
"content/public/browser/navigation_data.h"
On 2016/04/29 21:14:23, bengr wrote:
> Can't this be forward declared?

Done.

https://codereview.chromium.org/1721813002/diff/340001/content/public/browser...
File content/public/browser/resource_dispatcher_host_delegate.cc (right):

https://codereview.chromium.org/1721813002/diff/340001/content/public/browser...
content/public/browser/resource_dispatcher_host_delegate.cc:7: #include
"content/public/browser/navigation_data.h"
On 2016/04/29 21:14:23, bengr wrote:
> Forward declare instead.

Because the method using this is returning a std::unique_ptr, it needs the full
definition.

https://codereview.chromium.org/1721813002/diff/340001/content/public/browser...
File content/public/browser/resource_dispatcher_host_delegate.h (right):

https://codereview.chromium.org/1721813002/diff/340001/content/public/browser...
content/public/browser/resource_dispatcher_host_delegate.h:137: net::URLRequest*
request) const;
On 2016/04/29 21:14:23, bengr wrote:
> Can this parameter be const?

Done.

[RyanSturm, 2016-05-02 20:02:09]

With regards to splitting the CL up: I see the benefit of splitting it up, but
don't know if it is worth it. The proposed split would be something along the
lines of: the first CL would be all of the content work to plumb NavigationData
and the second CL would be all of the work to plumb DataReductionProxyData (and
adding the observer). I think there is a somewhat natural divide there. The
work-time cost of splitting the CL and tests is not expensive, but not cheap
either as the unittests will have to be broken up into different CL's in a lot
of cases. Let me know if you are strongly in favor of the split, and I will
start splitting the CL.

[nasko, 2016-05-02 22:02:49]

Overall it is getting there. The code implementing the functionality is pretty
close. I'm not sure yet we need all the test code that was added, especially the
one not exposed outside of content/.

https://codereview.chromium.org/1721813002/diff/380001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/1721813002/diff/380001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:219: void
SetNavigationData(std::unique_ptr<NavigationData> navigation_data) {
Simple setter methods should use hacker_case. This is just a pointer move, which
is more or less a simple copy over.

https://codereview.chromium.org/1721813002/diff/380001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl_unittest.cc (right):

https://codereview.chromium.org/1721813002/diff/380001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl_unittest.cc:664:
EXPECT_EQ(test_data, test_handle()->GetNavigationData());
This test case doesn't test much, what's the goal of it? Since this is
functionality geared at embedders, I think it would be fine to just have the
embedder exercise the functionality. Writing a full test to ensure we copy a
pointer over seems like an overkill.

https://codereview.chromium.org/1721813002/diff/380001/content/browser/frame_...
File content/browser/frame_host/navigator_impl_unittest.cc (right):

https://codereview.chromium.org/1721813002/diff/380001/content/browser/frame_...
content/browser/frame_host/navigator_impl_unittest.cc:122: class
TestNavigationData : public NavigationData {
Defining the same class over multiple unit tests files calls for having it
extracted out in its own file.

https://codereview.chromium.org/1721813002/diff/380001/content/browser/frame_...
content/browser/frame_host/navigator_impl_unittest.cc:158: response,
MakeEmptyStream(), base::WrapUnique(new TestNavigationData()));
So this change requires always passing NavigationData? That doesn't seem right.
What if an embedder doesn't supply this? Ideally the code will be structured
such that it can take the new data when needed but operate fine without it.
Changing all tests here seems unneeded churn and more overhead for new tests
being written.

https://codereview.chromium.org/1721813002/diff/380001/content/browser/loader...
File content/browser/loader/navigation_resource_handler.cc (right):

https://codereview.chromium.org/1721813002/diff/380001/content/browser/loader...
content/browser/loader/navigation_resource_handler.cc:105:
std::unique_ptr<NavigationData> clone_data;
nit: cloned_data

https://codereview.chromium.org/1721813002/diff/380001/content/browser/loader...
File content/browser/loader/navigation_resource_throttle.cc (right):

https://codereview.chromium.org/1721813002/diff/380001/content/browser/loader...
content/browser/loader/navigation_resource_throttle.cc:231:
std::unique_ptr<NavigationData> clone;
nit: Keep instances of the same code as identical as possible. This is named
clone_data in the other copy of this code (or cloned_data as I suggested there).

https://codereview.chromium.org/1721813002/diff/380001/content/public/browser...
File content/public/browser/navigation_data.h (right):

https://codereview.chromium.org/1721813002/diff/380001/content/public/browser...
content/public/browser/navigation_data.h:13: // expected to be created on the IO
thread, and content will transfer it to the
nit: "content/" so it is clear what is meant.

https://codereview.chromium.org/1721813002/diff/380001/content/public/browser...
File content/public/browser/resource_dispatcher_host_delegate.h (right):

https://codereview.chromium.org/1721813002/diff/380001/content/public/browser...
content/public/browser/resource_dispatcher_host_delegate.h:137: const
net::URLRequest* request) const;
Make sure mmenke@ or csharrison@ looks at this and is OK with the RDH parts of
the CL.

https://codereview.chromium.org/1721813002/diff/380001/content/public/test/we...
File content/public/test/web_contents_tester.h (right):

https://codereview.chromium.org/1721813002/diff/380001/content/public/test/we...
content/public/test/web_contents_tester.h:125: std::unique_ptr<NavigationData>
navigation_data) = 0;
Is there no other way to invoke the functionality from outside of content/
without explicit set method?

[RyanSturm, 2016-05-02 23:02:54]

ryansturm@chromium.org changed reviewers:
+ csharrison@chromium.org

[RyanSturm, 2016-05-02 23:02:57]

+csharrison: PTAL, specifically at:

The metrics observer
(	chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc)

The RDH use in NavigationResourceThrottle/NaivgationResourceHandler.

Would it be more appropriate to have a reference to the
ResourceDispatcherHostDelegate instead of ResourceDispatcherHost directly?

https://codereview.chromium.org/1721813002/diff/380001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/1721813002/diff/380001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:219: void
SetNavigationData(std::unique_ptr<NavigationData> navigation_data) {
On 2016/05/02 22:02:49, nasko (slow) wrote:
> Simple setter methods should use hacker_case. This is just a pointer move,
which
> is more or less a simple copy over.

Done.

https://codereview.chromium.org/1721813002/diff/380001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl_unittest.cc (right):

https://codereview.chromium.org/1721813002/diff/380001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl_unittest.cc:664:
EXPECT_EQ(test_data, test_handle()->GetNavigationData());
On 2016/05/02 22:02:49, nasko (slow) wrote:
> This test case doesn't test much, what's the goal of it? Since this is
> functionality geared at embedders, I think it would be fine to just have the
> embedder exercise the functionality. Writing a full test to ensure we copy a
> pointer over seems like an overkill.

I like to unittest everything individually, but this does seem like overkill.
I'll remove it because it really doesn't test much.

https://codereview.chromium.org/1721813002/diff/380001/content/browser/frame_...
File content/browser/frame_host/navigator_impl_unittest.cc (right):

https://codereview.chromium.org/1721813002/diff/380001/content/browser/frame_...
content/browser/frame_host/navigator_impl_unittest.cc:122: class
TestNavigationData : public NavigationData {
On 2016/05/02 22:02:49, nasko (slow) wrote:
> Defining the same class over multiple unit tests files calls for having it
> extracted out in its own file.

Removing this from the file.

https://codereview.chromium.org/1721813002/diff/380001/content/browser/frame_...
content/browser/frame_host/navigator_impl_unittest.cc:158: response,
MakeEmptyStream(), base::WrapUnique(new TestNavigationData()));
On 2016/05/02 22:02:49, nasko (slow) wrote:
> So this change requires always passing NavigationData? That doesn't seem
right.
> What if an embedder doesn't supply this? Ideally the code will be structured
> such that it can take the new data when needed but operate fine without it.
> Changing all tests here seems unneeded churn and more overhead for new tests
> being written.

I can pass in an null NavigationData; it's a good point that a test one really
isn't needed here. the last one in the file does this, but I will just do it for
all of them.

https://codereview.chromium.org/1721813002/diff/380001/content/browser/loader...
File content/browser/loader/navigation_resource_handler.cc (right):

https://codereview.chromium.org/1721813002/diff/380001/content/browser/loader...
content/browser/loader/navigation_resource_handler.cc:105:
std::unique_ptr<NavigationData> clone_data;
On 2016/05/02 22:02:49, nasko (slow) wrote:
> nit: cloned_data

Done.

https://codereview.chromium.org/1721813002/diff/380001/content/browser/loader...
File content/browser/loader/navigation_resource_throttle.cc (right):

https://codereview.chromium.org/1721813002/diff/380001/content/browser/loader...
content/browser/loader/navigation_resource_throttle.cc:231:
std::unique_ptr<NavigationData> clone;
On 2016/05/02 22:02:49, nasko (slow) wrote:
> nit: Keep instances of the same code as identical as possible. This is named
> clone_data in the other copy of this code (or cloned_data as I suggested
there).

Done.

https://codereview.chromium.org/1721813002/diff/380001/content/public/browser...
File content/public/browser/navigation_data.h (right):

https://codereview.chromium.org/1721813002/diff/380001/content/public/browser...
content/public/browser/navigation_data.h:13: // expected to be created on the IO
thread, and content will transfer it to the
On 2016/05/02 22:02:49, nasko (slow) wrote:
> nit: "content/" so it is clear what is meant.

Done.

https://codereview.chromium.org/1721813002/diff/380001/content/public/browser...
File content/public/browser/resource_dispatcher_host_delegate.h (right):

https://codereview.chromium.org/1721813002/diff/380001/content/public/browser...
content/public/browser/resource_dispatcher_host_delegate.h:137: const
net::URLRequest* request) const;
On 2016/05/02 22:02:49, nasko (slow) wrote:
> Make sure mmenke@ or csharrison@ looks at this and is OK with the RDH parts of
> the CL.

Adding csharrison@ for this.

https://codereview.chromium.org/1721813002/diff/380001/content/public/test/we...
File content/public/test/web_contents_tester.h (right):

https://codereview.chromium.org/1721813002/diff/380001/content/public/test/we...
content/public/test/web_contents_tester.h:125: std::unique_ptr<NavigationData>
navigation_data) = 0;
On 2016/05/02 22:02:49, nasko (slow) wrote:
> Is there no other way to invoke the functionality from outside of content/
> without explicit set method?

I wanted to cast NavigationHandle to NavigationHandleImpl in the test itself,
but that seemed wrong because navigation_handle_impl.h is not in public/. I
didn't see another way to do this without being in content/ code, so there would
need to be a test method somewhere.

[Charlie Harrison, 2016-05-03 01:08:31]

csharrison@chromium.org changed reviewers:
+ mmenke@chromium.org

[Charlie Harrison, 2016-05-03 01:08:33]

page_load_metrics looks generally good.

My gut is that using a pointer to the RDHI delegate is fine. The whole point is
you want info from the embedder. 
+mmenke@ for a quick spot check on this.

https://codereview.chromium.org/1721813002/diff/400001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
(right):

https://codereview.chromium.org/1721813002/diff/400001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:7:
#include "base/metrics/histogram.h"
Not sure you need this if you're only using PAGE_LOAD_HISTOGRAM.

https://codereview.chromium.org/1721813002/diff/400001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:41:
// As documented in content, this NavigationData is a clone of the
"//content" is clearer than "content". Better yet, the actual path to the
documentation :)

https://codereview.chromium.org/1721813002/diff/400001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:72:
PAGE_LOAD_HISTOGRAM(
You might want to filter out loads that occurred in the background. Loads in the
background skew FP metrics heavily, because painting only occurs when the user
foregrounds a tab. Look at some of the other metrics observers to see how.

https://codereview.chromium.org/1721813002/diff/400001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h
(right):

https://codereview.chromium.org/1721813002/diff/400001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:29:
}  // internal
nit: namespace internal.

https://codereview.chromium.org/1721813002/diff/400001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:39:
// page_load_metrics::PageLoadMetricsObserver implementation:
nit: I prefer "// page_load_metrics::PageLoadMetricsObserver:".

https://codereview.chromium.org/1721813002/diff/400001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/1721813002/diff/400001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:55:
data_reduction_proxy::DataReductionProxyData* DataForNavigationHandle(
Can you put this in an anonymous namespace? It will make this class easier to
follow.

https://codereview.chromium.org/1721813002/diff/400001/content/browser/loader...
File content/browser/loader/navigation_resource_throttle.h (right):

https://codereview.chromium.org/1721813002/diff/400001/content/browser/loader...
content/browser/loader/navigation_resource_throttle.h:18: class
ResourceDispatcherHostImpl;
Add a line break above this forward declaration.

[Bryan McQuade, 2016-05-03 13:00:38]

bmcquade@chromium.org changed reviewers:
+ bmcquade@chromium.org

[Bryan McQuade, 2016-05-03 13:00:40]

https://codereview.chromium.org/1721813002/diff/400001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
(right):

https://codereview.chromium.org/1721813002/diff/400001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:77:
if (data_reduction_proxy::params::IsIncludedInLoFiEnabledFieldTrial()) {
are these standard finch field trials? if so, you shouldn't need to log metrics,
as you can slice and analyze them via the finch dashboard (or via dremel queries
to restrict on field trial)

[mmenke, 2016-05-03 16:06:24]

https://codereview.chromium.org/1721813002/diff/400001/content/browser/loader...
File content/browser/loader/navigation_resource_handler.cc (right):

https://codereview.chromium.org/1721813002/diff/400001/content/browser/loader...
content/browser/loader/navigation_resource_handler.cc:108:
resource_dispatcher_host_->delegate()->GetNavigationData(request());
Accessing the RDH and/or its delegate here seems fine.  I am a bit concerned
about all the cloning going on here, though.

GetNavigationData creates the navigation data from scratch.  Then you copy it
all immediately to pass to another thread...

Also, should ChromeNavigationData really depend on every single module that
wants to smuggle data over to the UI thread?

[bengr, 2016-05-03 17:41:55]

https://codereview.chromium.org/1721813002/diff/400001/content/browser/loader...
File content/browser/loader/navigation_resource_handler.cc (right):

https://codereview.chromium.org/1721813002/diff/400001/content/browser/loader...
content/browser/loader/navigation_resource_handler.cc:108:
resource_dispatcher_host_->delegate()->GetNavigationData(request());
On 2016/05/03 16:06:24, mmenke wrote:
> Accessing the RDH and/or its delegate here seems fine.  I am a bit concerned
> about all the cloning going on here, though.

So we started with a unique_ptr, so we could simply transfer ownership when
moving it to a new thread, but got push back from nasko and ncarter that that
was a messier contract than simply cloning the object for use on a new thread.
We're fine either way, but all owners need to agree. 

> 
> GetNavigationData creates the navigation data from scratch.  Then you copy it
> all immediately to pass to another thread...
> 
> Also, should ChromeNavigationData really depend on every single module that
> wants to smuggle data over to the UI thread?

[RyanSturm, 2016-05-03 18:14:13]

Fixing everything except mmenke@'s two issues.

+ncarter@

The clone contract is more explicit and allows content/ to handle the thread
hand-off with more assurances that the object will be safe on the UI thread. It
is not strictly necessary (content/ could expect the embedder to return an
object that is not tied to the io thread). I am fine with either resolution, but
the owners should decide what the best approach is.

https://codereview.chromium.org/1721813002/diff/400001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
(right):

https://codereview.chromium.org/1721813002/diff/400001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:7:
#include "base/metrics/histogram.h"
On 2016/05/03 01:08:32, csharrison wrote:
> Not sure you need this if you're only using PAGE_LOAD_HISTOGRAM.

Done.

https://codereview.chromium.org/1721813002/diff/400001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:41:
// As documented in content, this NavigationData is a clone of the
On 2016/05/03 01:08:32, csharrison wrote:
> "//content" is clearer than "content". Better yet, the actual path to the
> documentation :)

Done.

https://codereview.chromium.org/1721813002/diff/400001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:72:
PAGE_LOAD_HISTOGRAM(
On 2016/05/03 01:08:32, csharrison wrote:
> You might want to filter out loads that occurred in the background. Loads in
the
> background skew FP metrics heavily, because painting only occurs when the user
> foregrounds a tab. Look at some of the other metrics observers to see how.

Thanks. That sounds like a good idea.

https://codereview.chromium.org/1721813002/diff/400001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:77:
if (data_reduction_proxy::params::IsIncludedInLoFiEnabledFieldTrial()) {
On 2016/05/03 13:00:40, Bryan McQuade wrote:
> are these standard finch field trials? if so, you shouldn't need to log
metrics,
> as you can slice and analyze them via the finch dashboard (or via dremel
queries
> to restrict on field trial)

Done.

https://codereview.chromium.org/1721813002/diff/400001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h
(right):

https://codereview.chromium.org/1721813002/diff/400001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:29:
}  // internal
On 2016/05/03 01:08:32, csharrison wrote:
> nit: namespace internal.

Done.

https://codereview.chromium.org/1721813002/diff/400001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:39:
// page_load_metrics::PageLoadMetricsObserver implementation:
On 2016/05/03 01:08:32, csharrison wrote:
> nit: I prefer "// page_load_metrics::PageLoadMetricsObserver:".

Done.

https://codereview.chromium.org/1721813002/diff/400001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/1721813002/diff/400001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:55:
data_reduction_proxy::DataReductionProxyData* DataForNavigationHandle(
On 2016/05/03 01:08:32, csharrison wrote:
> Can you put this in an anonymous namespace? It will make this class easier to
> follow.

Done.

https://codereview.chromium.org/1721813002/diff/400001/content/browser/loader...
File content/browser/loader/navigation_resource_handler.cc (right):

https://codereview.chromium.org/1721813002/diff/400001/content/browser/loader...
content/browser/loader/navigation_resource_handler.cc:108:
resource_dispatcher_host_->delegate()->GetNavigationData(request());
On 2016/05/03 16:06:24, mmenke wrote:
> Accessing the RDH and/or its delegate here seems fine.  I am a bit concerned
> about all the cloning going on here, though.

Switching to RDHDelegate.


> GetNavigationData creates the navigation data from scratch.  Then you copy it
> all immediately to pass to another thread...

Like bengr@ said, this makes a cleaner contract, so there needs to be ownership
agreement.
> 
> Also, should ChromeNavigationData really depend on every single module that
> wants to smuggle data over to the UI thread?
If a different module needs to smuggle data, they can implement a clone-able
version of SupportsUserData, or some other workaround. If they need to get
something off of URLRequest and move it to NavigationData without chrome or
content knowing about the key would be a copyable version of SupportsUserData or
the like. It seems like chrome should know about anything that crosses from net/
to content/ like this, so in some ways, this explicit ownership is helpful.

https://codereview.chromium.org/1721813002/diff/400001/content/browser/loader...
File content/browser/loader/navigation_resource_throttle.h (right):

https://codereview.chromium.org/1721813002/diff/400001/content/browser/loader...
content/browser/loader/navigation_resource_throttle.h:18: class
ResourceDispatcherHostImpl;
On 2016/05/03 01:08:33, csharrison wrote:
> Add a line break above this forward declaration.

Done.

[mmenke, 2016-05-03 19:32:53]

https://codereview.chromium.org/1721813002/diff/400001/content/browser/loader...
File content/browser/loader/navigation_resource_handler.cc (right):

https://codereview.chromium.org/1721813002/diff/400001/content/browser/loader...
content/browser/loader/navigation_resource_handler.cc:108:
resource_dispatcher_host_->delegate()->GetNavigationData(request());
On 2016/05/03 18:14:13, RyanSturm wrote:
> On 2016/05/03 16:06:24, mmenke wrote:
> > Accessing the RDH and/or its delegate here seems fine.  I am a bit concerned
> > about all the cloning going on here, though.
> 
> Switching to RDHDelegate.
> 
> 
> > GetNavigationData creates the navigation data from scratch.  Then you copy
it
> > all immediately to pass to another thread...
> 
> Like bengr@ said, this makes a cleaner contract, so there needs to be
ownership
> agreement.

The thing is, the object is created on demand, and *then* it's copied to pass
over to the UI thread.

It seems to me like either the object should be kept around (And possibly
updated by consumers) and then copied as needed to pass over to the UI thread. 
Or it should be created as needed, and then ownership passed over to the UI
thread.

I think create on demand and then copy doesn't really make any sense.  Also not
a big fan of punting cleanup to the next unfortunate who finds himself in need
of passing data along the same path.

> > 
> > Also, should ChromeNavigationData really depend on every single module that
> > wants to smuggle data over to the UI thread?
> If a different module needs to smuggle data, they can implement a clone-able
> version of SupportsUserData, or some other workaround. If they need to get
> something off of URLRequest and move it to NavigationData without chrome or
> content knowing about the key would be a copyable version of SupportsUserData
or
> the like. It seems like chrome should know about anything that crosses from
net/
> to content/ like this, so in some ways, this explicit ownership is helpful.

[bengr, 2016-05-03 22:13:16]

lgtm. The clones seem like a bit much, but there's disagreement among owners as
to how to proceed I'll let mmenke and nasko sort that out.

[bengr, 2016-05-03 22:13:32]

lgtm. The clones seem like a bit much, but there's disagreement among owners as
to how to proceed. I'll let mmenke and nasko sort that out.

[Bryan McQuade, 2016-05-04 14:28:23]

LGTM.

As an aside, the plumbing and static_casting needed to make this work makes me
wonder if Chrome's abstraction layers are helping us or hurting us. The
complexity of implementation required to do relatively straightforward things in
Chrome continues to amaze me.

https://codereview.chromium.org/1721813002/diff/420001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/1721813002/diff/420001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:80:
page_load_metrics::PageLoadMetricsObserverTestHarness* test_;
It feels a bit odd to be passing the whole test harness into the observer
wrapper. If the observer starts to depend deeply on the whole harness over time
then this code will become spaghetti and hard to reason about.

It looks like you currently only need the harness since DataForNavigationHandle
takes the harness as an arg, but it really only needs the webcontents. It seems
more sane to pass just the webcontents to the wrapper - can we do that instead?

https://codereview.chromium.org/1721813002/diff/420001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
(right):

https://codereview.chromium.org/1721813002/diff/420001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc:119:
content::WebContentsTester::For(web_contents())
likewise, looks like this is not needed. can we revert this file?

https://codereview.chromium.org/1721813002/diff/420001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.h
(right):

https://codereview.chromium.org/1721813002/diff/420001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.h:50:
void SetNavigationData(
it doesn't look like you're using this - your test impl sets the navigation data
directly on the tester, which seems much better. can we remove this?

[Bryan McQuade, 2016-05-04 14:59:46]

On 2016/05/04 at 14:28:23, Bryan McQuade wrote:
> LGTM.
> 
> As an aside, the plumbing and static_casting needed to make this work makes me
wonder if Chrome's abstraction layers are helping us or hurting us. The
complexity of implementation required to do relatively straightforward things in
Chrome continues to amaze me.

I should restate this. While there is a lot of complexity here, if providing an
embeddable content layer that doesn't know about DRP is a requirement, then I
understand that this complexity is likely necessary.

> 
>
https://codereview.chromium.org/1721813002/diff/420001/chrome/browser/page_lo...
> File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc
(right):
> 
>
https://codereview.chromium.org/1721813002/diff/420001/chrome/browser/page_lo...
>
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:80:
page_load_metrics::PageLoadMetricsObserverTestHarness* test_;
> It feels a bit odd to be passing the whole test harness into the observer
wrapper. If the observer starts to depend deeply on the whole harness over time
then this code will become spaghetti and hard to reason about.
> 
> It looks like you currently only need the harness since
DataForNavigationHandle takes the harness as an arg, but it really only needs
the webcontents. It seems more sane to pass just the webcontents to the wrapper
- can we do that instead?
> 
>
https://codereview.chromium.org/1721813002/diff/420001/chrome/browser/page_lo...
> File
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
(right):
> 
>
https://codereview.chromium.org/1721813002/diff/420001/chrome/browser/page_lo...
>
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc:119:
content::WebContentsTester::For(web_contents())
> likewise, looks like this is not needed. can we revert this file?
> 
>
https://codereview.chromium.org/1721813002/diff/420001/chrome/browser/page_lo...
> File
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.h
(right):
> 
>
https://codereview.chromium.org/1721813002/diff/420001/chrome/browser/page_lo...
>
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.h:50:
void SetNavigationData(
> it doesn't look like you're using this - your test impl sets the navigation
data directly on the tester, which seems much better. can we remove this?

[nasko, 2016-05-04 16:16:15]

On 2016/05/03 19:32:53, mmenke wrote:
>
https://codereview.chromium.org/1721813002/diff/400001/content/browser/loader...
> File content/browser/loader/navigation_resource_handler.cc (right):
> 
>
https://codereview.chromium.org/1721813002/diff/400001/content/browser/loader...
> content/browser/loader/navigation_resource_handler.cc:108:
> resource_dispatcher_host_->delegate()->GetNavigationData(request());
> On 2016/05/03 18:14:13, RyanSturm wrote:
> > On 2016/05/03 16:06:24, mmenke wrote:
> > > Accessing the RDH and/or its delegate here seems fine.  I am a bit
concerned
> > > about all the cloning going on here, though.
> > 
> > Switching to RDHDelegate.
> > 
> > 
> > > GetNavigationData creates the navigation data from scratch.  Then you copy
> it
> > > all immediately to pass to another thread...
> > 
> > Like bengr@ said, this makes a cleaner contract, so there needs to be
> ownership
> > agreement.
> 
> The thing is, the object is created on demand, and *then* it's copied to pass
> over to the UI thread.
> 
> It seems to me like either the object should be kept around (And possibly
> updated by consumers) and then copied as needed to pass over to the UI thread.

> Or it should be created as needed, and then ownership passed over to the UI
> thread.

This is the main reason for the design - content/ cannot assume that the object
is created on demand or not. It allow for an object that the embedder controls
fully and has no contract about its lifetime.

> I think create on demand and then copy doesn't really make any sense.  Also
not
> a big fan of punting cleanup to the next unfortunate who finds himself in need
> of passing data along the same path.

The clone was chosen so a long lived object on the IO thread can still live past

the point of transfer to the UI thread.
Also, if the embedder chooses to have thread bound objects as part of its
implementation, calling an explicit clone method gives it a chance to create
UI thread counterparts.

I'm curious to hear what cleanup is needed in the future?

> > > Also, should ChromeNavigationData really depend on every single module
that
> > > wants to smuggle data over to the UI thread?
> > If a different module needs to smuggle data, they can implement a clone-able
> > version of SupportsUserData, or some other workaround. If they need to get
> > something off of URLRequest and move it to NavigationData without chrome or
> > content knowing about the key would be a copyable version of
SupportsUserData
> or
> > the like. It seems like chrome should know about anything that crosses from
> net/
> > to content/ like this, so in some ways, this explicit ownership is helpful.

[nasko, 2016-05-04 16:20:28]

On 2016/05/04 14:59:46, Bryan McQuade wrote:
> On 2016/05/04 at 14:28:23, Bryan McQuade wrote:
> > LGTM.
> > 
> > As an aside, the plumbing and static_casting needed to make this work makes
me
> wonder if Chrome's abstraction layers are helping us or hurting us. The
> complexity of implementation required to do relatively straightforward things
in
> Chrome continues to amaze me.
> 
> I should restate this. While there is a lot of complexity here, if providing
an
> embeddable content layer that doesn't know about DRP is a requirement, then I
> understand that this complexity is likely necessary.

DRP is not part of the web platform, therefore content/ must not know about it.
I have proposed plumbing the actual proxy address through content/, which makes
sense, but it was stated that on its own is not enough.

DRP is a Chrome implementation detail and this is why bengr@ and ryansturm@ have
spent quite a bit of time trying to create an abstract path for the embedder to
have its own data. The complexity comes from the need of a thread hop which is
entirely contained within content/.

If there are ideas for a better design, I'm totally open to exploring them, but
this is what couple of weeks of discussions have resulted in.

[RyanSturm, 2016-05-04 18:11:41]

On 2016/05/04 16:20:28, nasko (slow) wrote:
> On 2016/05/04 14:59:46, Bryan McQuade wrote:
> > On 2016/05/04 at 14:28:23, Bryan McQuade wrote:
> > > LGTM.
> > > 
> > > As an aside, the plumbing and static_casting needed to make this work
makes
> me
> > wonder if Chrome's abstraction layers are helping us or hurting us. The
> > complexity of implementation required to do relatively straightforward
things
> in
> > Chrome continues to amaze me.
> > 
> > I should restate this. While there is a lot of complexity here, if providing
> an
> > embeddable content layer that doesn't know about DRP is a requirement, then
I
> > understand that this complexity is likely necessary.
> 
> DRP is not part of the web platform, therefore content/ must not know about
it.
> I have proposed plumbing the actual proxy address through content/, which
makes
> sense, but it was stated that on its own is not enough.
> 
> DRP is a Chrome implementation detail and this is why bengr@ and ryansturm@
have
> spent quite a bit of time trying to create an abstract path for the embedder
to
> have its own data. The complexity comes from the need of a thread hop which is
> entirely contained within content/.
> 
> If there are ideas for a better design, I'm totally open to exploring them,
but
> this is what couple of weeks of discussions have resulted in.

To handle both concerns from Nasko@ and Mmenke@, perhaps the
ChromeResourceDispatcherHostDelegate should own all IO thread
ChromeNavigationData instances in a std::map<URLRequest*,
std::unique_ptr<ChromeNavigationData>> (any other embedder would also be
responsible for lifetime of NavigationData instances). The entries into the map
would be added in GetNavigationData() and removed in RequestComplete(). Instead
of returning a std::unique_ptr from GetNavigationData(), it would return a raw
pointer that could be cloned for the UI thread. This opens the possibility to
creating ChromeNavigationData in earlier delegate calls for an URLRequest and
modifying it on the IO thread before GetNavigationData is called during commit;
although, this CL will still create the ChromeNavigationData in
GetNavigationData().

Thoughts?

[RyanSturm, 2016-05-04 19:45:46]

On 2016/05/04 18:11:41, RyanSturm wrote:
> On 2016/05/04 16:20:28, nasko (slow) wrote:
> > On 2016/05/04 14:59:46, Bryan McQuade wrote:
> > > On 2016/05/04 at 14:28:23, Bryan McQuade wrote:
> > > > LGTM.
> > > > 
> > > > As an aside, the plumbing and static_casting needed to make this work
> makes
> > me
> > > wonder if Chrome's abstraction layers are helping us or hurting us. The
> > > complexity of implementation required to do relatively straightforward
> things
> > in
> > > Chrome continues to amaze me.
> > > 
> > > I should restate this. While there is a lot of complexity here, if
providing
> > an
> > > embeddable content layer that doesn't know about DRP is a requirement,
then
> I
> > > understand that this complexity is likely necessary.
> > 
> > DRP is not part of the web platform, therefore content/ must not know about
> it.
> > I have proposed plumbing the actual proxy address through content/, which
> makes
> > sense, but it was stated that on its own is not enough.
> > 
> > DRP is a Chrome implementation detail and this is why bengr@ and ryansturm@
> have
> > spent quite a bit of time trying to create an abstract path for the embedder
> to
> > have its own data. The complexity comes from the need of a thread hop which
is
> > entirely contained within content/.
> > 
> > If there are ideas for a better design, I'm totally open to exploring them,
> but
> > this is what couple of weeks of discussions have resulted in.
> 
> To handle both concerns from Nasko@ and Mmenke@, perhaps the
> ChromeResourceDispatcherHostDelegate should own all IO thread
> ChromeNavigationData instances in a std::map<URLRequest*,
> std::unique_ptr<ChromeNavigationData>> (any other embedder would also be
> responsible for lifetime of NavigationData instances). The entries into the
map
> would be added in GetNavigationData() and removed in RequestComplete().
Instead
> of returning a std::unique_ptr from GetNavigationData(), it would return a raw
> pointer that could be cloned for the UI thread. This opens the possibility to
> creating ChromeNavigationData in earlier delegate calls for an URLRequest and
> modifying it on the IO thread before GetNavigationData is called during
commit;
> although, this CL will still create the ChromeNavigationData in
> GetNavigationData().
> 
> Thoughts?

The other alternative I thought of was tying the lifetime of
ChromeNavigationData to URLRequest via SupportsUserData; it seems a little
cleaner than the map of ChromeNavigationData instances.

[RyanSturm, 2016-05-04 22:16:23]

Patchset #23 (id:440001) has been deleted

[RyanSturm, 2016-05-04 22:35:41]

Changed lifetime of ChromeNavigationData to be owned by URLRequest via UserData.

https://codereview.chromium.org/1721813002/diff/420001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/1721813002/diff/420001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:80:
page_load_metrics::PageLoadMetricsObserverTestHarness* test_;
On 2016/05/04 14:28:23, Bryan McQuade wrote:
> It feels a bit odd to be passing the whole test harness into the observer
> wrapper. If the observer starts to depend deeply on the whole harness over
time
> then this code will become spaghetti and hard to reason about.
> 
> It looks like you currently only need the harness since
DataForNavigationHandle
> takes the harness as an arg, but it really only needs the webcontents. It
seems
> more sane to pass just the webcontents to the wrapper - can we do that
instead?

Done.

https://codereview.chromium.org/1721813002/diff/420001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
(right):

https://codereview.chromium.org/1721813002/diff/420001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc:119:
content::WebContentsTester::For(web_contents())
On 2016/05/04 14:28:23, Bryan McQuade wrote:
> likewise, looks like this is not needed. can we revert this file?

Done.

https://codereview.chromium.org/1721813002/diff/420001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.h
(right):

https://codereview.chromium.org/1721813002/diff/420001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.h:50:
void SetNavigationData(
On 2016/05/04 14:28:23, Bryan McQuade wrote:
> it doesn't look like you're using this - your test impl sets the navigation
data
> directly on the tester, which seems much better. can we remove this?

Done.

[RyanSturm, 2016-05-04 22:39:40]

Description was changed from

==========
Adding DataReductionProxy (DRP) specfic UMA for FirstContentfulPaint.
This consumes the PageLoad.Timing2 code path to get data specific to DRP with
regards to having DRP on and being in the lofi experiment. Right now, it only
considers
NavigationToFirstContentfulPaint.

BUG=578321
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Adding DataReductionProxy (DRP) specfic UMA for FirstContentfulPaint.
This consumes the PageLoad.Timing2 code path to get data specific to DRP with
regards to having DRP on and being in the lofi experiment. Right now, it only
considers
NavigationToFirstContentfulPaint.

This introduces:
DataReductionProxyData, which is stored on URLRequest;
NavigationData, a cloneable interface used in content/ to communicate
information to NavigationHandle;
ChromeNavigationData, an inheritor of NavigationData that owns a
DataReductionProxyData and is stored in NavigationHandleImpl (UI thread) and
URLRequest (IO thread, in UserData).

BUG=578321
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Bryan McQuade, 2016-05-06 00:38:53]

https://codereview.chromium.org/1721813002/diff/500001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
(right):

https://codereview.chromium.org/1721813002/diff/500001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc:12:
#include "content/public/browser/navigation_data.h"
let's revert this file

https://codereview.chromium.org/1721813002/diff/500001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.h
(right):

https://codereview.chromium.org/1721813002/diff/500001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.h:14:
namespace content {
let's revert this file

[mmenke, 2016-05-06 18:59:36]

https://codereview.chromium.org/1721813002/diff/520001/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/1721813002/diff/520001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:742:
data->SetDataReductionProxyData(data_reduction_proxy_data->DeepCopy());
This seems worse than the last iteration - we're reusing ChromeNavigationData,
but any time it's accessed, we overwrite its DataReductionProxyData.  I hadn't
realized you were intended to copy the DRP data here.

Oh well - I'll just defer to nasko on this one.

https://codereview.chromium.org/1721813002/diff/520001/content/browser/loader...
File content/browser/loader/navigation_resource_handler.cc (right):

https://codereview.chromium.org/1721813002/diff/520001/content/browser/loader...
content/browser/loader/navigation_resource_handler.cc:102:
std::unique_ptr<NavigationData> cloned_data;
Should this be std::unique_ptr<const NavigationData>, after cloning, to make it
clearer that modifications won't persist?

https://codereview.chromium.org/1721813002/diff/520001/content/public/browser...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/1721813002/diff/520001/content/public/browser...
content/public/browser/navigation_handle.h:176: virtual NavigationData*
GetNavigationData() = 0;
Again, should this return a const object (And be const itself, for that matter)?

[RyanSturm, 2016-05-06 20:55:44]

https://codereview.chromium.org/1721813002/diff/520001/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/1721813002/diff/520001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:742:
data->SetDataReductionProxyData(data_reduction_proxy_data->DeepCopy());
On 2016/05/06 18:59:36, mmenke wrote:
> This seems worse than the last iteration - we're reusing ChromeNavigationData,
> but any time it's accessed, we overwrite its DataReductionProxyData.  I hadn't
> realized you were intended to copy the DRP data here.
> 
> Oh well - I'll just defer to nasko on this one.

This method wasn't intended to be called more than once. I see your concern.
Instead of DeepCopying, it can copy the state from one to the other if
ChromeNavigationData already has a DataReductionProxyData. I'm not sure if that
is the approach we want. If we expected this to get called a few times, and the
DeepCopy does become expensive, it might be.

If SupportsUserData used ref counting instead of unique_ptr, we could just keep
the DataReductionProxyData in sync through that mechanism, but without the ref
counting it just isn't feasible.

Perhaps for clarity, this method should be renamed, CreateNavigationData, but
that also doesn't necessarily seem like the best approach due to the fact that
it might not create a new one. MaybeCreateNavigationDataForURLRequest? I'm
really not sure what the best approach is.

https://codereview.chromium.org/1721813002/diff/520001/content/browser/loader...
File content/browser/loader/navigation_resource_handler.cc (right):

https://codereview.chromium.org/1721813002/diff/520001/content/browser/loader...
content/browser/loader/navigation_resource_handler.cc:102:
std::unique_ptr<NavigationData> cloned_data;
On 2016/05/06 18:59:36, mmenke wrote:
> Should this be std::unique_ptr<const NavigationData>, after cloning, to make
it
> clearer that modifications won't persist?

Done.

https://codereview.chromium.org/1721813002/diff/520001/content/public/browser...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/1721813002/diff/520001/content/public/browser...
content/public/browser/navigation_handle.h:176: virtual NavigationData*
GetNavigationData() = 0;
On 2016/05/06 18:59:36, mmenke wrote:
> Again, should this return a const object (And be const itself, for that
matter)?

Done.

[nasko, 2016-05-09 17:35:44]

https://codereview.chromium.org/1721813002/diff/560001/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_navigation_data_unittest.cc (right):

https://codereview.chromium.org/1721813002/diff/560001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_navigation_data_unittest.cc:41:
EXPECT_FALSE(!clone_chrome_data->GetDataReductionProxyData());
Why not EXPECT_TRUE?

https://codereview.chromium.org/1721813002/diff/560001/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/1721813002/diff/560001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:811: if
(request) {
It is a more readable pattern to return early and do extra work after the
return. In this case something like:

if (!request) return data;

// The rest of DRP code
return data;

https://codereview.chromium.org/1721813002/diff/560001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:817: //
when content makes a clone of NavigationData for the UI thread.
Why can't we avoid the extra copy? We definitely need a clone of the data we
pass from the IO to the UI thread, but not sure about this one here.

https://codereview.chromium.org/1721813002/diff/560001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h
(right):

https://codereview.chromium.org/1721813002/diff/560001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:24: //
Whether the DataReductionProxy was used for this request or navigation.
nit: Using empty lines to separate methods that logically belong together from
other groups makes code much easier to read IMHO. Your CL generally lacks that
in a bunch of places.

https://codereview.chromium.org/1721813002/diff/560001/content/browser/frame_...
File content/browser/frame_host/navigation_request.h (right):

https://codereview.chromium.org/1721813002/diff/560001/content/browser/frame_...
content/browser/frame_host/navigation_request.h:30: class StreamHandle;
Why is this needed? How did the file compile before it?

https://codereview.chromium.org/1721813002/diff/560001/content/browser/frame_...
content/browser/frame_host/navigation_request.h:181: std::unique_ptr<const
NavigationData> navigation_data) override;
Why do we need const here? content/ can't really change anything within this
object, as the interface is pretty much empty.

https://codereview.chromium.org/1721813002/diff/560001/content/browser/frame_...
File content/browser/frame_host/navigator_impl_unittest.cc (right):

https://codereview.chromium.org/1721813002/diff/560001/content/browser/frame_...
content/browser/frame_host/navigator_impl_unittest.cc:22: #include
"content/public/browser/navigation_data.h"
Do we need an include if all calls use nullptr?

https://codereview.chromium.org/1721813002/diff/560001/content/browser/loader...
File content/browser/loader/navigation_url_loader_delegate.h (right):

https://codereview.chromium.org/1721813002/diff/560001/content/browser/loader...
content/browser/loader/navigation_url_loader_delegate.h:35: // |body_stream|.
|navigation_data| is added to the NavigationHandle.
nit: s/added to/passed to/

https://codereview.chromium.org/1721813002/diff/560001/content/public/browser...
File content/public/browser/navigation_data.h (right):

https://codereview.chromium.org/1721813002/diff/560001/content/public/browser...
content/public/browser/navigation_data.h:12: // Copyable interface for embedders
to pass opaque data to content. It is
nit: content/

https://codereview.chromium.org/1721813002/diff/560001/content/public/browser...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/1721813002/diff/560001/content/public/browser...
content/public/browser/navigation_handle.h:176: virtual const NavigationData*
GetNavigationData() const = 0;
I think we already covered that there is no const usage in the content/public/
API - https://www.chromium.org/developers/content-module/content-api.

https://codereview.chromium.org/1721813002/diff/560001/content/public/browser...
File content/public/browser/resource_dispatcher_host_delegate.h (right):

https://codereview.chromium.org/1721813002/diff/560001/content/public/browser...
content/public/browser/resource_dispatcher_host_delegate.h:135: // called for
requests that are navigation requests.
nit: "called only for navigation requests."

[RyanSturm, 2016-05-09 18:27:32]

Due to https://www.chromium.org/developers/content-module/content-api the
const-ness added to NavigationData in the previous CL was changed back to
non-const.

https://codereview.chromium.org/1721813002/diff/560001/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_navigation_data_unittest.cc (right):

https://codereview.chromium.org/1721813002/diff/560001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_navigation_data_unittest.cc:41:
EXPECT_FALSE(!clone_chrome_data->GetDataReductionProxyData());
On 2016/05/09 17:35:43, nasko (slow) wrote:
> Why not EXPECT_TRUE?

Done.

https://codereview.chromium.org/1721813002/diff/560001/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/1721813002/diff/560001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:811: if
(request) {
On 2016/05/09 17:35:43, nasko (slow) wrote:
> It is a more readable pattern to return early and do extra work after the
> return. In this case something like:
> 
> if (!request) return data;
> 
> // The rest of DRP code
> return data;

Done.

https://codereview.chromium.org/1721813002/diff/560001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:817: //
when content makes a clone of NavigationData for the UI thread.
On 2016/05/09 17:35:43, nasko (slow) wrote:
> Why can't we avoid the extra copy? We definitely need a clone of the data we
> pass from the IO to the UI thread, but not sure about this one here.

Because ChromeNavigationData owns it's DataReductionProxyData, and there is no
way to get ownership of DataReductionProxyData out of URLRequest's UserData (you
can only get a raw pointer or delete it), there needs to be a copy here. If
ChromeNavigationData doesn't own DataReductionProxyData, at some point it will
leak that. The correct answer would be to change SupportsUserData to use ref
counting or some other shared pointers, so that ChromeNavigationData can also
use shared pointers and avoid the copy.

I don't think changing every Data inheritor to use ref-counting is the right
answer though, so it appears we are stuck with this copy. It leaves two
DataReductionProxyData's on URLRequest's UserData, but I don't have a good
alternative.

https://codereview.chromium.org/1721813002/diff/560001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h
(right):

https://codereview.chromium.org/1721813002/diff/560001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:24: //
Whether the DataReductionProxy was used for this request or navigation.
On 2016/05/09 17:35:43, nasko (slow) wrote:
> nit: Using empty lines to separate methods that logically belong together from
> other groups makes code much easier to read IMHO. Your CL generally lacks that
> in a bunch of places.

I fixed it here to add spaces in logical places like you suggested; I admit I
have always been weak at knowing the best placement of vertical  whitespace.

https://codereview.chromium.org/1721813002/diff/560001/content/browser/frame_...
File content/browser/frame_host/navigation_request.h (right):

https://codereview.chromium.org/1721813002/diff/560001/content/browser/frame_...
content/browser/frame_host/navigation_request.h:30: class StreamHandle;
navigation_url_loader_delegate.h is one header that also forward declares this
class. If that file ever didn't need to forward declare StreamHandle anymore, it
would be weird to have a compiler error on building this. Leaving it for now,
but I'll remove it if it seems out of scope of this CL.

https://codereview.chromium.org/1721813002/diff/560001/content/browser/frame_...
content/browser/frame_host/navigation_request.h:181: std::unique_ptr<const
NavigationData> navigation_data) override;
On 2016/05/09 17:35:43, nasko (slow) wrote:
> Why do we need const here? content/ can't really change anything within this
> object, as the interface is pretty much empty.

Agreed. this was a constraint of trying to clarify that content won't change it,
but the embedder might choose to change it later anyway.

https://codereview.chromium.org/1721813002/diff/560001/content/browser/frame_...
File content/browser/frame_host/navigator_impl_unittest.cc (right):

https://codereview.chromium.org/1721813002/diff/560001/content/browser/frame_...
content/browser/frame_host/navigator_impl_unittest.cc:22: #include
"content/public/browser/navigation_data.h"
On 2016/05/09 17:35:43, nasko (slow) wrote:
> Do we need an include if all calls use nullptr?

Because it's a unique_ptr param the compiler has to change that nullptr into a
unique_ptr, it needs to know the destructor signature of NavigationData even
though it is nullptr so it can allocate that in the unique_ptr. At least that's
my understanding. The build error seems to reflect that.

https://codereview.chromium.org/1721813002/diff/560001/content/browser/loader...
File content/browser/loader/navigation_url_loader_delegate.h (right):

https://codereview.chromium.org/1721813002/diff/560001/content/browser/loader...
content/browser/loader/navigation_url_loader_delegate.h:35: // |body_stream|.
|navigation_data| is added to the NavigationHandle.
On 2016/05/09 17:35:43, nasko (slow) wrote:
> nit: s/added to/passed to/

Done.

https://codereview.chromium.org/1721813002/diff/560001/content/public/browser...
File content/public/browser/navigation_data.h (right):

https://codereview.chromium.org/1721813002/diff/560001/content/public/browser...
content/public/browser/navigation_data.h:12: // Copyable interface for embedders
to pass opaque data to content. It is
On 2016/05/09 17:35:43, nasko (slow) wrote:
> nit: content/

Done.

https://codereview.chromium.org/1721813002/diff/560001/content/public/browser...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/1721813002/diff/560001/content/public/browser...
content/public/browser/navigation_handle.h:176: virtual const NavigationData*
GetNavigationData() const = 0;
On 2016/05/09 17:35:43, nasko (slow) wrote:
> I think we already covered that there is no const usage in the
content/public/
> API - https://www.chromium.org/developers/content-module/content-api.

Done. Removed both consts as it seems like the embedder should be allowed to do
whatever it wants with the NavigationData, and shouldn't be constrained to keep
it the same.

https://codereview.chromium.org/1721813002/diff/560001/content/public/browser...
File content/public/browser/resource_dispatcher_host_delegate.h (right):

https://codereview.chromium.org/1721813002/diff/560001/content/public/browser...
content/public/browser/resource_dispatcher_host_delegate.h:135: // called for
requests that are navigation requests.
On 2016/05/09 17:35:44, nasko (slow) wrote:
> nit: "called only for navigation requests."

Done.

[nasko, 2016-05-09 18:40:46]

content/ LGTM.

https://codereview.chromium.org/1721813002/diff/560001/content/browser/frame_...
File content/browser/frame_host/navigator_impl_unittest.cc (right):

https://codereview.chromium.org/1721813002/diff/560001/content/browser/frame_...
content/browser/frame_host/navigator_impl_unittest.cc:22: #include
"content/public/browser/navigation_data.h"
On 2016/05/09 18:27:31, RyanSturm wrote:
> On 2016/05/09 17:35:43, nasko (slow) wrote:
> > Do we need an include if all calls use nullptr?
> 
> Because it's a unique_ptr param the compiler has to change that nullptr into a
> unique_ptr, it needs to know the destructor signature of NavigationData even
> though it is nullptr so it can allocate that in the unique_ptr. At least
that's
> my understanding. The build error seems to reflect that.

Acknowledged.

https://codereview.chromium.org/1721813002/diff/580001/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/1721813002/diff/580001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:813:
data_reduction_proxy::DataReductionProxyData* data_reduction_proxy_data =
nit: Empty line above.

[mmenke, 2016-05-09 18:45:54]

Pear earlier comment, I'll just defer to nasko.

[RyanSturm, 2016-05-10 16:59:31]

ryansturm@chromium.org changed reviewers:
+ asvitkine@chromium.org, thestig@chromium.org

[RyanSturm, 2016-05-10 16:59:33]

thestig: PTAL @ src/chrome/browser/renderer_host
asvitkine: PTAL @ histograms.xml

[Alexei Svitkine (slow), 2016-05-10 17:01:36]

lgtm

[Charlie Harrison, 2016-05-10 17:05:43]

page_load_metrics lgtm

[RyanSturm, 2016-05-10 17:08:36]

ryansturm@chromium.org changed reviewers:
- tbansal@chromium.org

[Lei Zhang, 2016-05-10 19:09:31]

lgtm

https://codereview.chromium.org/1721813002/diff/640001/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.h
(right):

https://codereview.chromium.org/1721813002/diff/640001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.h:33:
namespace content {
nit: put this above extensions

https://codereview.chromium.org/1721813002/diff/640001/chrome/browser/rendere...
File
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc
(right):

https://codereview.chromium.org/1721813002/diff/640001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:122:
explicit DidFinishNavigationObserver(content::WebContents* web_contents,
not explicit

https://codereview.chromium.org/1721813002/diff/640001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:132:
if (!add_data_reduction_proxy_data_) {
remove the '!' and flip the two cases?

[RyanSturm, 2016-05-10 21:05:32]

https://codereview.chromium.org/1721813002/diff/640001/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.h
(right):

https://codereview.chromium.org/1721813002/diff/640001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.h:33:
namespace content {
On 2016/05/10 19:09:31, Lei Zhang wrote:
> nit: put this above extensions

Done.

https://codereview.chromium.org/1721813002/diff/640001/chrome/browser/rendere...
File
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc
(right):

https://codereview.chromium.org/1721813002/diff/640001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:122:
explicit DidFinishNavigationObserver(content::WebContents* web_contents,
On 2016/05/10 19:09:31, Lei Zhang wrote:
> not explicit

Done.

https://codereview.chromium.org/1721813002/diff/640001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:132:
if (!add_data_reduction_proxy_data_) {
On 2016/05/10 19:09:31, Lei Zhang wrote:
> remove the '!' and flip the two cases?

Done.

[RyanSturm, 2016-05-11 21:14:44]

The CQ bit was checked by ryansturm@chromium.org

[RyanSturm, 2016-05-11 21:14:45]

The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org,
bmcquade@chromium.org, nasko@chromium.org, csharrison@chromium.org,
asvitkine@chromium.org, thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1721813002/#ps660001
(title: "Nits")

[commit-bot: I haz the power, 2016-05-11 21:15:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1721813002/660001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1721813002/660001

[commit-bot: I haz the power, 2016-05-11 22:29:01]

Description was changed from

==========
Adding DataReductionProxy (DRP) specfic UMA for FirstContentfulPaint.
This consumes the PageLoad.Timing2 code path to get data specific to DRP with
regards to having DRP on and being in the lofi experiment. Right now, it only
considers
NavigationToFirstContentfulPaint.

This introduces:
DataReductionProxyData, which is stored on URLRequest;
NavigationData, a cloneable interface used in content/ to communicate
information to NavigationHandle;
ChromeNavigationData, an inheritor of NavigationData that owns a
DataReductionProxyData and is stored in NavigationHandleImpl (UI thread) and
URLRequest (IO thread, in UserData).

BUG=578321
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Adding DataReductionProxy (DRP) specfic UMA for FirstContentfulPaint.
This consumes the PageLoad.Timing2 code path to get data specific to DRP with
regards to having DRP on and being in the lofi experiment. Right now, it only
considers
NavigationToFirstContentfulPaint.

This introduces:
DataReductionProxyData, which is stored on URLRequest;
NavigationData, a cloneable interface used in content/ to communicate
information to NavigationHandle;
ChromeNavigationData, an inheritor of NavigationData that owns a
DataReductionProxyData and is stored in NavigationHandleImpl (UI thread) and
URLRequest (IO thread, in UserData).

BUG=578321
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-05-11 22:29:03]

Committed patchset #33 (id:660001)

[commit-bot: I haz the power, 2016-05-11 22:30:50]

Description was changed from

==========
Adding DataReductionProxy (DRP) specfic UMA for FirstContentfulPaint.
This consumes the PageLoad.Timing2 code path to get data specific to DRP with
regards to having DRP on and being in the lofi experiment. Right now, it only
considers
NavigationToFirstContentfulPaint.

This introduces:
DataReductionProxyData, which is stored on URLRequest;
NavigationData, a cloneable interface used in content/ to communicate
information to NavigationHandle;
ChromeNavigationData, an inheritor of NavigationData that owns a
DataReductionProxyData and is stored in NavigationHandleImpl (UI thread) and
URLRequest (IO thread, in UserData).

BUG=578321
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Adding DataReductionProxy (DRP) specfic UMA for FirstContentfulPaint.
This consumes the PageLoad.Timing2 code path to get data specific to DRP with
regards to having DRP on and being in the lofi experiment. Right now, it only
considers
NavigationToFirstContentfulPaint.

This introduces:
DataReductionProxyData, which is stored on URLRequest;
NavigationData, a cloneable interface used in content/ to communicate
information to NavigationHandle;
ChromeNavigationData, an inheritor of NavigationData that owns a
DataReductionProxyData and is stored in NavigationHandleImpl (UI thread) and
URLRequest (IO thread, in UserData).

BUG=578321
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

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

[commit-bot: I haz the power, 2016-05-11 22:30:53]

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