Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
684223003:
Data Reduction Proxy Interstitials (Closed)
DescriptionData Reduction Proxy Interstitials
A resource throttle and ui manager for the Data Reduction Proxy
bypass interstitials. The intersitial is shown when the user
loads a resource that returns a data reduction proxy bypass on http.
The user can choose to continue loading the page or go back to the
previous page. If the user accepts loading the page, the intertitial
will not be shown again for another 5 minutes.
BUG=428408
Committed: https://crrev.com/5effca2eb326a4c6bc379e1035e183c2d2cdea83
Cr-Commit-Position: refs/heads/master@{#316330}
Patch Set 1 : #Patch Set 2 : Rebase #
Total comments: 61
Patch Set 3 : Addressing comments #
Total comments: 66
Patch Set 4 : Adding tests and addressing comments #
Total comments: 90
Patch Set 5 : Addressing bengr comments #
Total comments: 32
Patch Set 6 : Rebase #Patch Set 7 : Addressing bengr comments #Patch Set 8 : Rebase #
Total comments: 4
Patch Set 9 : Rebase #
Total comments: 5
Patch Set 10 : Addressing thestig and mmenke comments #Patch Set 11 : Fixed crash #Patch Set 12 : Update copyright year #
Total comments: 15
Patch Set 13 : Addressing mmenke comments #Patch Set 14 : Rebase #
Total comments: 7
Patch Set 15 : Addressing mmenke and blundell comments #Patch Set 16 : Rebase #
Total comments: 34
Patch Set 17 : Rebase #Patch Set 18 : Addressing bengr comments #Patch Set 19 : Rebase #
Total comments: 6
Patch Set 20 : Rebase #Patch Set 21 : Nits #
Total comments: 2
Patch Set 22 : Thestig comment and test fix #Messages
Total messages: 81 (42 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
megjablon@chromium.org changed reviewers: + bengr@chromium.org
https://codereview.chromium.org/684223003/diff/60001/chrome/browser/about_fla... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/684223003/diff/60001/chrome/browser/about_fla... chrome/browser/about_flags.cc:2010: #if defined(OS_ANDROID) Please also limit this to Chromium builds and Dev channel. There's a function in this file for restricting the release of the flag. https://codereview.chromium.org/684223003/diff/60001/chrome/browser/profiles/... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/684223003/diff/60001/chrome/browser/profiles/... chrome/browser/profiles/profile_impl.cc:645: scoped_ptr<data_reduction_proxy::DataReductionProxyUIService> Move the scoped_ptr outside the "#if defined" block, and then remove the platform check at line 704. https://codereview.chromium.org/684223003/diff/60001/chrome/browser/profiles/... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/684223003/diff/60001/chrome/browser/profiles/... chrome/browser/profiles/profile_impl_io_data.cc:144: scoped_ptr<data_reduction_proxy::DataReductionProxyUIService> Hmm. This UI service won't be available on iOS. I'd hate to have to guard all references to it with #if !defined(OS_IOS). Let's look around for a pattern that other layered components use. E.g., a base class that's not in content. https://codereview.chromium.org/684223003/diff/60001/chrome/browser/profiles/... chrome/browser/profiles/profile_impl_io_data.cc:185: io_data_->set_data_reduction_proxy_ui_service( Now would be a good time to write a DataReductionProxyIOData class that contains all of this initialization logic. https://codereview.chromium.org/684223003/diff/60001/chrome/browser/profiles/... chrome/browser/profiles/profile_impl_io_data.cc:447: #if defined(OS_ANDROID) Can you do this if the ui_service() is not NULL instead? https://codereview.chromium.org/684223003/diff/60001/chrome/browser/profiles/... File chrome/browser/profiles/profile_io_data.h (right): https://codereview.chromium.org/684223003/diff/60001/chrome/browser/profiles/... chrome/browser/profiles/profile_io_data.h:213: data_reduction_proxy::DataReductionProxyParams* data_reduction_proxy_params() Can the pointer be const too? https://codereview.chromium.org/684223003/diff/60001/chrome/browser/renderer_... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/684223003/diff/60001/chrome/browser/renderer_... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:521: #if defined(OS_ANDROID) Again, gating this on object being instantiated instead of on an #ifdef is better https://codereview.chromium.org/684223003/diff/60001/chrome/browser/renderer_... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:531: request, These params can probably all fit on one line. https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... File components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle.cc (right): https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle.cc:24: content::ResourceType resource_type, Needs #include https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle.cc:25: DataReductionProxyUIService* ui_service, Needs #include https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle.cc:37: CHECK(state_ == NOT_BYPASSED); Why CHECK and not DCHECK? https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle.cc:61: !(request_->load_flags() & net::LOAD_BYPASS_PROXY)) { What is the point of this check? Explain in a comment. https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle.cc:72: if (params_->IsDataReductionProxy(request_->proxy_server(), NULL) || Is this the only scheme you'll want to ignore? What about "ws", e.g. https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle.cc:83: DCHECK(info) https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle.cc:108: return "DataReductionProxyResourceThrottle"; define this as a constant in an anonymous namespace above. https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... File components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle.h (right): https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle.h:10: #include "components/data_reduction_proxy/content/data_reduction_proxy_ui_service.h" Remove. https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle.h:21: class DataReductionProxyUIManager; class DataReductionProxyUIService; class DataReductionProxyParams; https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle.h:29: content::ResourceType resource_type, #include "content/public/common/resource_type.h" https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle.h:35: virtual void WillStartUsingNetwork(bool* defer) override; Add a comment that names the class these override. https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle.h:36: virtual void WillRedirectRequest(const GURL& new_url, bool* defer) override; #include "url/gurl.h" https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle.h:38: virtual const char* GetNameForLogging() const override; The style is moving to not using the "virtual" keyword on overrides. Remove it here and elsewhere. https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle.h:47: static const char* kProceedHeader; Is this used? https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle.h:49: // Creates a bypass resource and calls StartDisplayingBlockingPage on the UI What's a "bypass resource"? Please clarify in the comment. https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle.h:65: net::URLRequest* request_; Is this guaranteed to be valid for the lifetime of this class? If so, say so in a comment. https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... File components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle_unittest.cc (right): https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle_unittest.cc:3: // found in the LICENSE file. You need tests, but I suppose you know that. https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... File components/data_reduction_proxy/content/data_reduction_proxy_ui_manager.cc (right): https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_ui_manager.cc:32: DataReductionProxyUIManager::BypassResource::~BypassResource() { } style nit: I always put the closing brace on a new line. https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_ui_manager.cc:48: if (proceed) { curly braces not needed. https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_ui_manager.cc:89: // TODO(megjablon): Show blocking page. For now, continue on to the page. What's left to do? https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... File components/data_reduction_proxy/content/data_reduction_proxy_ui_manager.h (right): https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_ui_manager.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. It's 2014, fyi. :) https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_ui_manager.h:54: DRPBlockingPageType blocking_page_type; Spell out DataReductionProxy instead of using DRP https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_ui_manager.h:76: friend class base::RefCountedThreadSafe<DataReductionProxyUIManager>; Why do you need to friend the UIManager?
Patchset #3 (id:80001) has been deleted
https://codereview.chromium.org/684223003/diff/60001/chrome/browser/about_fla... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/684223003/diff/60001/chrome/browser/about_fla... chrome/browser/about_flags.cc:2010: #if defined(OS_ANDROID) On 2014/10/31 17:06:47, bengr wrote: > Please also limit this to Chromium builds and Dev channel. There's a function in > this file for restricting the release of the flag. Done. https://codereview.chromium.org/684223003/diff/60001/chrome/browser/profiles/... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/684223003/diff/60001/chrome/browser/profiles/... chrome/browser/profiles/profile_impl.cc:645: scoped_ptr<data_reduction_proxy::DataReductionProxyUIService> On 2014/10/31 17:06:47, bengr wrote: > Move the scoped_ptr outside the "#if defined" block, and then remove the > platform check at line 704. Acknowledged. https://codereview.chromium.org/684223003/diff/60001/chrome/browser/profiles/... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/684223003/diff/60001/chrome/browser/profiles/... chrome/browser/profiles/profile_impl_io_data.cc:185: io_data_->set_data_reduction_proxy_ui_service( On 2014/10/31 17:06:47, bengr wrote: > Now would be a good time to write a DataReductionProxyIOData class that contains > all of this initialization logic. Done. https://codereview.chromium.org/684223003/diff/60001/chrome/browser/profiles/... chrome/browser/profiles/profile_impl_io_data.cc:447: #if defined(OS_ANDROID) On 2014/10/31 17:06:47, bengr wrote: > Can you do this if the ui_service() is not NULL instead? Done. https://codereview.chromium.org/684223003/diff/60001/chrome/browser/profiles/... File chrome/browser/profiles/profile_io_data.h (right): https://codereview.chromium.org/684223003/diff/60001/chrome/browser/profiles/... chrome/browser/profiles/profile_io_data.h:213: data_reduction_proxy::DataReductionProxyParams* data_reduction_proxy_params() On 2014/10/31 17:06:47, bengr wrote: > Can the pointer be const too? Acknowledged. https://codereview.chromium.org/684223003/diff/60001/chrome/browser/renderer_... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/684223003/diff/60001/chrome/browser/renderer_... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:521: #if defined(OS_ANDROID) On 2014/10/31 17:06:47, bengr wrote: > Again, gating this on object being instantiated instead of on an #ifdef is > better Acknowledged. https://codereview.chromium.org/684223003/diff/60001/chrome/browser/renderer_... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:531: request, On 2014/10/31 17:06:47, bengr wrote: > These params can probably all fit on one line. Done. https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... File components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle.cc (right): https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle.cc:24: content::ResourceType resource_type, On 2014/10/31 17:06:48, bengr wrote: > Needs #include Done. https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle.cc:25: DataReductionProxyUIService* ui_service, On 2014/10/31 17:06:47, bengr wrote: > Needs #include Done. https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle.cc:37: CHECK(state_ == NOT_BYPASSED); On 2014/10/31 17:06:47, bengr wrote: > Why CHECK and not DCHECK? Whoops https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle.cc:61: !(request_->load_flags() & net::LOAD_BYPASS_PROXY)) { On 2014/10/31 17:06:48, bengr wrote: > What is the point of this check? Explain in a comment. Done. https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle.cc:72: if (params_->IsDataReductionProxy(request_->proxy_server(), NULL) || On 2014/10/31 17:06:47, bengr wrote: > Is this the only scheme you'll want to ignore? What about "ws", e.g. Why would we want to ignore ws? https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle.cc:83: On 2014/10/31 17:06:48, bengr wrote: > DCHECK(info) Done. https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle.cc:108: return "DataReductionProxyResourceThrottle"; On 2014/10/31 17:06:47, bengr wrote: > define this as a constant in an anonymous namespace above. Done. https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... File components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle.h (right): https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle.h:10: #include "components/data_reduction_proxy/content/data_reduction_proxy_ui_service.h" On 2014/10/31 17:06:48, bengr wrote: > Remove. Done. https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle.h:21: class DataReductionProxyUIManager; On 2014/10/31 17:06:48, bengr wrote: > class DataReductionProxyUIService; > class DataReductionProxyParams; Done. https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle.h:29: content::ResourceType resource_type, On 2014/10/31 17:06:48, bengr wrote: > #include "content/public/common/resource_type.h" Already there. https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle.h:35: virtual void WillStartUsingNetwork(bool* defer) override; On 2014/10/31 17:06:48, bengr wrote: > Add a comment that names the class these override. Done. https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle.h:36: virtual void WillRedirectRequest(const GURL& new_url, bool* defer) override; On 2014/10/31 17:06:48, bengr wrote: > #include "url/gurl.h" Done. https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle.h:38: virtual const char* GetNameForLogging() const override; On 2014/10/31 17:06:48, bengr wrote: > The style is moving to not using the "virtual" keyword on overrides. Remove it > here and elsewhere. Done. https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle.h:47: static const char* kProceedHeader; On 2014/10/31 17:06:48, bengr wrote: > Is this used? No. Removed. https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle.h:49: // Creates a bypass resource and calls StartDisplayingBlockingPage on the UI On 2014/10/31 17:06:48, bengr wrote: > What's a "bypass resource"? Please clarify in the comment. Done. https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle.h:65: net::URLRequest* request_; On 2014/10/31 17:06:48, bengr wrote: > Is this guaranteed to be valid for the lifetime of this class? If so, say so in > a comment. Done. https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... File components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle_unittest.cc (right): https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_resource_throttle_unittest.cc:3: // found in the LICENSE file. On 2014/10/31 17:06:48, bengr wrote: > You need tests, but I suppose you know that. Haha yep these are TODOs. https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... File components/data_reduction_proxy/content/data_reduction_proxy_ui_manager.cc (right): https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_ui_manager.cc:32: DataReductionProxyUIManager::BypassResource::~BypassResource() { } On 2014/10/31 17:06:48, bengr wrote: > style nit: I always put the closing brace on a new line. Done. https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_ui_manager.cc:48: if (proceed) { On 2014/10/31 17:06:48, bengr wrote: > curly braces not needed. Done. https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_ui_manager.cc:89: // TODO(megjablon): Show blocking page. For now, continue on to the page. On 2014/10/31 17:06:48, bengr wrote: > What's left to do? This doesn't display anything. I need to add the blocking page class and then call it's display method here. https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... File components/data_reduction_proxy/content/data_reduction_proxy_ui_manager.h (right): https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_ui_manager.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2014/10/31 17:06:48, bengr wrote: > It's 2014, fyi. :) Done. https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_ui_manager.h:54: DRPBlockingPageType blocking_page_type; On 2014/10/31 17:06:48, bengr wrote: > Spell out DataReductionProxy instead of using DRP Removing the blocking page type since this page is just for testing. https://codereview.chromium.org/684223003/diff/60001/components/data_reductio... components/data_reduction_proxy/content/data_reduction_proxy_ui_manager.h:76: friend class base::RefCountedThreadSafe<DataReductionProxyUIManager>; On 2014/10/31 17:06:48, bengr wrote: > Why do you need to friend the UIManager? https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/ref_co... Since it is ref counted.
Obviously this needs tests. https://codereview.chromium.org/684223003/diff/100001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/684223003/diff/100001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2201: // enable-data-reduction-proxy-alt is only available for the Chromium builds Fix the comment. https://codereview.chromium.org/684223003/diff/100001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/684223003/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_impl.cc:115: #include "components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_service.h" Include unconditionally. https://codereview.chromium.org/684223003/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_impl.cc:652: data_reduction_proxy_ui_service( Declare for all platforms and instantiate only if Android. https://codereview.chromium.org/684223003/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_impl.cc:722: #if defined(OS_ANDROID) Instead of changing the prototype for Android, pass an empty scoped_ptr here, and check it. https://codereview.chromium.org/684223003/diff/100001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/684223003/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:64: #if defined(OS_ANDROID) Include unconditionally. https://codereview.chromium.org/684223003/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:151: #if defined(OS_ANDROID) Remove #if defined https://codereview.chromium.org/684223003/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:196: #if defined(OS_ANDROID) Remove #if defined https://codereview.chromium.org/684223003/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:471: #if defined(OS_ANDROID) Remove #if defined https://codereview.chromium.org/684223003/diff/100001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.h (right): https://codereview.chromium.org/684223003/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.h:79: #if defined(OS_ANDROID) Remove the #if defined. https://codereview.chromium.org/684223003/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.h:80: scoped_ptr<data_reduction_proxy::DataReductionProxyUIService> Forward declare. https://codereview.chromium.org/684223003/diff/100001/chrome/browser/profiles... File chrome/browser/profiles/profile_io_data.h (right): https://codereview.chromium.org/684223003/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.h:38: #if defined(OS_ANDROID) Remove the #if defined https://codereview.chromium.org/684223003/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.h:222: #if defined(OS_ANDROID) Remove the #if defined https://codereview.chromium.org/684223003/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.h:458: #if defined(OS_ANDROID) Remove the #if defined https://codereview.chromium.org/684223003/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.h:690: #if defined(OS_ANDROID) Remove the #if defined. Also add a comment that this must be declared after the DataReductionProxyChromeConfigurator, because the configurator must be valid for its entire lifetime. https://codereview.chromium.org/684223003/diff/100001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/684223003/diff/100001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:116: #if defined(OS_ANDROID) Remove #if defined. Remove usings. Better to spell it all out where you use it. https://codereview.chromium.org/684223003/diff/100001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:532: #if defined(OS_ANDROID) Remove #if defined. https://codereview.chromium.org/684223003/diff/100001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:543: Remove blank line. https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc (right): https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc:23: const char* kResouceThrottleLogName = "DataReductionProxyResourceThrottle"; const char kResouceThrottleLogName[] = "DataReductionProxyResourceThrottle"; https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc:36: is_subresource_(resource_type != content::RESOURCE_TYPE_MAIN_FRAME) { DCHECK(request) DCHECK(params) and anything else you expect to be non-null https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc:39: DataReductionProxyResourceThrottle::~DataReductionProxyResourceThrottle() { } Move closing brace to new line. https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc:78: if(state_ != NOT_BYPASSED) Fix the space after "if" and the indent of "return" as in: if (... return; https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc:84: request_->url().SchemeIs("file")) { Where did this list of schemes come from? what about ftp and https, e.g. Shouldn't you only continue if SchemeIs("http")? https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc:109: FROM_HERE, Move up with previous line. https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc:114: resource)); Move these up onto same like as "AsWeakPtr()" https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.h (right): https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.h:25: class DataReductionProxyResourceThrottle Comment. https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.h:29: DataReductionProxyResourceThrottle( Comment. https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.h:30: net::URLRequest* request, Can this be const? https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.h:32: DataReductionProxyUIService* ui_service, Can this be const? https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.h:67: State state_; Document all member variables. https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.cc (right): https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.cc:16: using content::BrowserThread; I prefer to avoid using "using" https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.cc:59: base::TimeDelta::FromMinutes(5)) { indent 4 https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.cc:69: RenderViewHost::FromID(resource.render_process_host_id, Indent only 4 https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.h (right): https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.h:61: virtual ~DataReductionProxyUIManager(); Add a blank line above and add comments.
Patchset #4 (id:120001) has been deleted
Added a resource throttle test. I started on a ui manager test but with the thread hopping there wasn't an easy or useful way that I could find to test it. Please let me know if you have any suggestions on how I could handle that. https://codereview.chromium.org/684223003/diff/100001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/684223003/diff/100001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2201: // enable-data-reduction-proxy-alt is only available for the Chromium builds On 2014/12/17 00:30:19, bengr wrote: > Fix the comment. Done. https://codereview.chromium.org/684223003/diff/100001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/684223003/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_impl.cc:115: #include "components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_service.h" On 2014/12/17 00:30:19, bengr wrote: > Include unconditionally. We decided not to have a base class as discussed offline. https://codereview.chromium.org/684223003/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_impl.cc:652: data_reduction_proxy_ui_service( On 2014/12/17 00:30:19, bengr wrote: > Declare for all platforms and instantiate only if Android. We decided not to have a base class as discussed offline. https://codereview.chromium.org/684223003/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_impl.cc:722: #if defined(OS_ANDROID) On 2014/12/17 00:30:19, bengr wrote: > Instead of changing the prototype for Android, pass an empty scoped_ptr here, > and check it. We decided not to have a base class as discussed offline. https://codereview.chromium.org/684223003/diff/100001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/684223003/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:64: #if defined(OS_ANDROID) On 2014/12/17 00:30:19, bengr wrote: > Include unconditionally. We decided not to have a base class as discussed offline. https://codereview.chromium.org/684223003/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:151: #if defined(OS_ANDROID) On 2014/12/17 00:30:19, bengr wrote: > Remove #if defined We decided not to have a base class as discussed offline. https://codereview.chromium.org/684223003/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:196: #if defined(OS_ANDROID) On 2014/12/17 00:30:19, bengr wrote: > Remove #if defined We decided not to have a base class as discussed offline. https://codereview.chromium.org/684223003/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:471: #if defined(OS_ANDROID) On 2014/12/17 00:30:19, bengr wrote: > Remove #if defined We decided not to have a base class as discussed offline. https://codereview.chromium.org/684223003/diff/100001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.h (right): https://codereview.chromium.org/684223003/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.h:79: #if defined(OS_ANDROID) On 2014/12/17 00:30:19, bengr wrote: > Remove the #if defined. We decided not to have a base class as discussed offline. https://codereview.chromium.org/684223003/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.h:80: scoped_ptr<data_reduction_proxy::DataReductionProxyUIService> On 2014/12/17 00:30:19, bengr wrote: > Forward declare. Done. https://codereview.chromium.org/684223003/diff/100001/chrome/browser/profiles... File chrome/browser/profiles/profile_io_data.h (right): https://codereview.chromium.org/684223003/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.h:38: #if defined(OS_ANDROID) On 2014/12/17 00:30:19, bengr wrote: > Remove the #if defined We decided not to have a base class as discussed offline. https://codereview.chromium.org/684223003/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.h:222: #if defined(OS_ANDROID) On 2014/12/17 00:30:19, bengr wrote: > Remove the #if defined We decided not to have a base class as discussed offline. https://codereview.chromium.org/684223003/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.h:458: #if defined(OS_ANDROID) On 2014/12/17 00:30:19, bengr wrote: > Remove the #if defined We decided not to have a base class as discussed offline. https://codereview.chromium.org/684223003/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.h:690: #if defined(OS_ANDROID) On 2014/12/17 00:30:19, bengr wrote: > Remove the #if defined. Also add a comment that this must be declared after the > DataReductionProxyChromeConfigurator, because the configurator must be valid for > its entire lifetime. Done. https://codereview.chromium.org/684223003/diff/100001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/684223003/diff/100001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:116: #if defined(OS_ANDROID) On 2014/12/17 00:30:19, bengr wrote: > Remove #if defined. > > Remove usings. Better to spell it all out where you use it. Done. https://codereview.chromium.org/684223003/diff/100001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:532: #if defined(OS_ANDROID) On 2014/12/17 00:30:19, bengr wrote: > Remove #if defined. We decided not to have a base class as discussed offline. https://codereview.chromium.org/684223003/diff/100001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:543: On 2014/12/17 00:30:19, bengr wrote: > Remove blank line. Done. https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc (right): https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc:23: const char* kResouceThrottleLogName = "DataReductionProxyResourceThrottle"; On 2014/12/17 00:30:19, bengr wrote: > const char kResouceThrottleLogName[] = "DataReductionProxyResourceThrottle"; Done. https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc:36: is_subresource_(resource_type != content::RESOURCE_TYPE_MAIN_FRAME) { On 2014/12/17 00:30:19, bengr wrote: > DCHECK(request) > DCHECK(params) > > and anything else you expect to be non-null Done. https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc:39: DataReductionProxyResourceThrottle::~DataReductionProxyResourceThrottle() { } On 2014/12/17 00:30:20, bengr wrote: > Move closing brace to new line. Done. https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc:78: if(state_ != NOT_BYPASSED) On 2014/12/17 00:30:20, bengr wrote: > Fix the space after "if" and the indent of "return" as in: > > if (... > return; Done. https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc:84: request_->url().SchemeIs("file")) { On 2014/12/17 00:30:19, bengr wrote: > Where did this list of schemes come from? what about ftp and https, e.g. > Shouldn't you only continue if SchemeIs("http")? I wanted to skip any local requests. We can skip warning on https or ftp if we want for debugging. This is a product decision on what kinds of pages we don't want to show the interstitial on. For debugging this makes sense to just warn on http requests. https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc:109: FROM_HERE, On 2014/12/17 00:30:20, bengr wrote: > Move up with previous line. Done. https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc:114: resource)); On 2014/12/17 00:30:20, bengr wrote: > Move these up onto same like as "AsWeakPtr()" Done. https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.h (right): https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.h:25: class DataReductionProxyResourceThrottle On 2014/12/17 00:30:20, bengr wrote: > Comment. Done. https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.h:29: DataReductionProxyResourceThrottle( On 2014/12/17 00:30:20, bengr wrote: > Comment. Done. https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.h:30: net::URLRequest* request, On 2014/12/17 00:30:20, bengr wrote: > Can this be const? Done. https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.h:32: DataReductionProxyUIService* ui_service, On 2014/12/17 00:30:20, bengr wrote: > Can this be const? Done. https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.h:67: State state_; On 2014/12/17 00:30:20, bengr wrote: > Document all member variables. Done. https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.cc (right): https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.cc:16: using content::BrowserThread; On 2014/12/17 00:30:20, bengr wrote: > I prefer to avoid using "using" Done. https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.cc:59: base::TimeDelta::FromMinutes(5)) { On 2014/12/17 00:30:20, bengr wrote: > indent 4 Done. https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.cc:69: RenderViewHost::FromID(resource.render_process_host_id, On 2014/12/17 00:30:20, bengr wrote: > Indent only 4 Done. https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.h (right): https://codereview.chromium.org/684223003/diff/100001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.h:61: virtual ~DataReductionProxyUIManager(); On 2014/12/17 00:30:20, bengr wrote: > Add a blank line above and add comments. Done.
https://codereview.chromium.org/684223003/diff/140001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/684223003/diff/140001/chrome/app/generated_re... chrome/app/generated_resources.grd:6386: + Data reduction proxy bypass warnings The feature is now called "Data Saver" so use the new name in strings, but not (yet) in variable/class names: "Data Saver proxy bypass warnings" https://codereview.chromium.org/684223003/diff/140001/chrome/app/generated_re... chrome/app/generated_resources.grd:6389: + Use the data reduction proxy with bypass warnings. The proxy must be enabled in settings for this flag to take effect. Use Data Saver with bypass warnings. Data Saver must be enabled in settings for this flag to take effect. https://codereview.chromium.org/684223003/diff/140001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/684223003/diff/140001/chrome/browser/profiles... chrome/browser/profiles/profile_impl.cc:723: data_reduction_proxy_ui_service.Pass(), I'm fine with you deferring this to a future CL, but we really shouldn't have #if defines. The UIService could subclass a base class with no-op implementations. https://codereview.chromium.org/684223003/diff/140001/chrome/browser/profiles... File chrome/browser/profiles/profile_io_data.h (right): https://codereview.chromium.org/684223003/diff/140001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.h:217: data_reduction_proxy::DataReductionProxyParams* data_reduction_proxy_params() Forward declare DRPParams. https://codereview.chromium.org/684223003/diff/140001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/684223003/diff/140001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:93: #include "components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_service.h" Can you forward declare this instead? https://codereview.chromium.org/684223003/diff/140001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:95: #include "components/data_reduction_proxy/core/common/data_reduction_proxy_switches.h" Is this needed? https://codereview.chromium.org/684223003/diff/140001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:539: if (params && service) { Is it possible for params to be null here? If not, DCHECK instead. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc (right): https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc:58: This blank line and others in your methods aren't needed. Consider removing some or all of them, but leave them in when you think they improve readability. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc:81: if (params_->IsDataReductionProxy(request_->proxy_server(), NULL) || I don't understand this. You proceed if the proxy is DRP *or* its a scheme the DRP can't proxy? Why? https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc:82: !request_->url().SchemeIs("http")) { The checks against the scheme are brittle, because the DRP may eventually proxy other schemes. Suggest adding a static to DRPParams called 'bool CanProxyURLScheme(const GURL& url);' https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc:117: void DataReductionProxyResourceThrottle::StartDisplayingBlockingPage( Make sure the definition order matches the declaration order. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.h (right): https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.h:49: enum State { Please add a comment that describes these states. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.h:66: // prerendering. Called on the UI thread. "Must only be called on the UI thread." Also add descriptions of the parameters. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.h:67: static void StartDisplayingBlockingPage( static members should be declared before non-static ones. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle_unittest.cc (right): https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle_unittest.cc:15: #include "net/url_request/url_request_status.h" Is this needed? https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle_unittest.cc:16: #include "net/url_request/url_request_test_job.h" Is this needed? https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle_unittest.cc:30: content::ResourceType resource_type, #include "src/content/public/common/resource_type.h" https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle_unittest.cc:65: class DataReductionProxyParamsMock : public DataReductionProxyParams { Can you use https://code.google.com/p/chromium/codesearch#chromium/src/components/data_re... instead? If not, can you update the test utils so you can? https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle_unittest.cc:73: MOCK_CONST_METHOD2( When possible, I'd like to move away from mocks. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle_unittest.cc:83: bool(const net::URLRequest& request, base::TimeDelta* min_retry_delay)); Forward declare base::TimeDelta. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle_unittest.cc:113: base::MessageLoopForIO loop_; #include "base/message_loop/message_loop.h" https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle_unittest.cc:141: resource_throttle_->WillRedirectRequest(GURL(), &defer); #include "url/gurl.h" https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle_unittest.cc:145: TEST_F(DataReductionProxyResourceThrottleTest, WillProcessResponse) { I know we don't do a great job of this, but would you please add comments for each of these tests that explain what the test is testing. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.cc (right): https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.cc:8: #include "base/bind_helpers.h" Why is this needed? https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.cc:9: #include "base/threading/thread.h" Why is this needed? https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.cc:13: #include "net/url_request/url_request_context.h" Why is this needed? https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.cc:14: #include "net/url_request/url_request_context_getter.h" Why is this needed? https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.cc:33: void DataReductionProxyUIManager::OnBlockingPageDone( This needs a test, that checks that the callbacks of the resources are called. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.cc:44: proceed_blocking_page_ = base::Time::Now(); #include "base/time/time.h" https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.cc:48: void DataReductionProxyUIManager::DisplayBlockingPage( This needs tests. You should cover what happens if proceed_blocking_page_ is more or less than 5 minutes from Now(), and what happens when you have a null/non-null RenderViewHost and a null/non-null WebContents. You'll probably want to move some of the logic out to virtual helpers. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.cc:54: base::TimeDelta::FromMinutes(5)) { define 5 as a constant at the top of the file and comment it. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.cc:66: content::WebContents* web_contents = NULL; nullptr https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.cc:83: LOG(WARNING) << "BLOCKING PAGE!"; Remove. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.cc:84: std::vector<BypassResource> resources; #include <vector> https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.h (right): https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.h:16: class Time; Remove this forward declaration. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.h:17: class Thread; Why is this needed? https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.h:66: base::Time proceed_blocking_page_; Why is this a Time? Consider a more descriptive variable name, and add a comment. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_service.h (right): https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_service.h:8: #include "base/memory/scoped_ptr.h" Not needed. But you do need "base/memory/ref_counted.h" https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_service.h:9: #include "components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.h" Can this be forward declared? https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_service.h:19: typedef base::Callback<const net::ProxyConfig&()> ProxyConfigGetter; This is defined in DRPNetworkDelegate, no? If so, use that one. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_service.h:28: const scoped_refptr<DataReductionProxyUIManager>& ui_manager() const; Do you really need a scoped_refptr? Would a scoped_ptr do instead? https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_service.h:34: ProxyConfigGetter proxy_config_getter_; #include DRPNetworkDelegate https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc (right): https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:91: bool DataReductionProxyParams::IsIncludedInBypassWarningTrial() { This name is misleading because there's no actual trial yet. Maybe: IsBypassWarningEnabledInFlags()? https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.h (right): https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:143: virtual bool IsBypassedByDataReductionProxyLocalRules( As I mentioned previously, I'd prefer you find a way to use the test params in test utils instead of virtualizing these methods and creating your own. https://codereview.chromium.org/684223003/diff/140001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/684223003/diff/140001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:49533: + <int value="-2099035488" label="enable-data-reduction-proxy-bypass-warning"/> Would you please double check that all of our flags in about::flags show up here and if not file a bug against me?
Patchset #5 (id:160001) has been deleted
Patchset #5 (id:160001) has been deleted
https://codereview.chromium.org/684223003/diff/140001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/684223003/diff/140001/chrome/app/generated_re... chrome/app/generated_resources.grd:6386: + Data reduction proxy bypass warnings On 2014/12/29 18:45:41, bengr wrote: > The feature is now called "Data Saver" so use the new name in strings, but not > (yet) in variable/class names: > > "Data Saver proxy bypass warnings" Done. https://codereview.chromium.org/684223003/diff/140001/chrome/app/generated_re... chrome/app/generated_resources.grd:6389: + Use the data reduction proxy with bypass warnings. The proxy must be enabled in settings for this flag to take effect. On 2014/12/29 18:45:41, bengr wrote: > Use Data Saver with bypass warnings. Data Saver must be enabled in settings for > this flag to take effect. Done. https://codereview.chromium.org/684223003/diff/140001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/684223003/diff/140001/chrome/browser/profiles... chrome/browser/profiles/profile_impl.cc:723: data_reduction_proxy_ui_service.Pass(), On 2014/12/29 18:45:41, bengr wrote: > I'm fine with you deferring this to a future CL, but we really shouldn't have > #if defines. The UIService could subclass a base class with no-op > implementations. Acknowledged. https://codereview.chromium.org/684223003/diff/140001/chrome/browser/profiles... File chrome/browser/profiles/profile_io_data.h (right): https://codereview.chromium.org/684223003/diff/140001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.h:217: data_reduction_proxy::DataReductionProxyParams* data_reduction_proxy_params() On 2014/12/29 18:45:41, bengr wrote: > Forward declare DRPParams. I wonder why this wasn't done before. Done. https://codereview.chromium.org/684223003/diff/140001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/684223003/diff/140001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:93: #include "components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_service.h" On 2014/12/29 18:45:41, bengr wrote: > Can you forward declare this instead? Done. https://codereview.chromium.org/684223003/diff/140001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:95: #include "components/data_reduction_proxy/core/common/data_reduction_proxy_switches.h" On 2014/12/29 18:45:41, bengr wrote: > Is this needed? No. Done. https://codereview.chromium.org/684223003/diff/140001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:539: if (params && service) { On 2014/12/29 18:45:41, bengr wrote: > Is it possible for params to be null here? If not, DCHECK instead. Done. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc (right): https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc:58: On 2014/12/29 18:45:41, bengr wrote: > This blank line and others in your methods aren't needed. Consider removing some > or all of them, but leave them in when you think they improve readability. Done. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc:81: if (params_->IsDataReductionProxy(request_->proxy_server(), NULL) || On 2014/12/29 18:45:41, bengr wrote: > I don't understand this. You proceed if the proxy is DRP *or* its a scheme the > DRP can't proxy? Why? Acknowledged. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc:82: !request_->url().SchemeIs("http")) { On 2014/12/29 18:45:41, bengr wrote: > The checks against the scheme are brittle, because the DRP may eventually proxy > other schemes. Suggest adding a static to DRPParams called 'bool > CanProxyURLScheme(const GURL& url);' Done. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc:117: void DataReductionProxyResourceThrottle::StartDisplayingBlockingPage( On 2014/12/29 18:45:41, bengr wrote: > Make sure the definition order matches the declaration order. Done. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.h (right): https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.h:49: enum State { On 2014/12/29 18:45:41, bengr wrote: > Please add a comment that describes these states. Done. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.h:66: // prerendering. Called on the UI thread. On 2014/12/29 18:45:41, bengr wrote: > "Must only be called on the UI thread." Also add descriptions of the parameters. Done. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.h:67: static void StartDisplayingBlockingPage( On 2014/12/29 18:45:41, bengr wrote: > static members should be declared before non-static ones. Done. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle_unittest.cc (right): https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle_unittest.cc:15: #include "net/url_request/url_request_status.h" On 2014/12/29 18:45:41, bengr wrote: > Is this needed? Removed. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle_unittest.cc:16: #include "net/url_request/url_request_test_job.h" On 2014/12/29 18:45:42, bengr wrote: > Is this needed? Removed. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle_unittest.cc:30: content::ResourceType resource_type, On 2014/12/29 18:45:41, bengr wrote: > #include "src/content/public/common/resource_type.h" Done. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle_unittest.cc:65: class DataReductionProxyParamsMock : public DataReductionProxyParams { On 2014/12/29 18:45:41, bengr wrote: > Can you use > https://code.google.com/p/chromium/codesearch#chromium/src/components/data_re... > instead? If not, can you update the test utils so you can? Done. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle_unittest.cc:73: MOCK_CONST_METHOD2( On 2014/12/29 18:45:41, bengr wrote: > When possible, I'd like to move away from mocks. Done. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle_unittest.cc:83: bool(const net::URLRequest& request, base::TimeDelta* min_retry_delay)); On 2014/12/29 18:45:42, bengr wrote: > Forward declare base::TimeDelta. Done. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle_unittest.cc:113: base::MessageLoopForIO loop_; On 2014/12/29 18:45:42, bengr wrote: > #include "base/message_loop/message_loop.h" Done. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle_unittest.cc:141: resource_throttle_->WillRedirectRequest(GURL(), &defer); On 2014/12/29 18:45:41, bengr wrote: > #include "url/gurl.h" Done. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle_unittest.cc:145: TEST_F(DataReductionProxyResourceThrottleTest, WillProcessResponse) { On 2014/12/29 18:45:41, bengr wrote: > I know we don't do a great job of this, but would you please add comments for > each of these tests that explain what the test is testing. Done. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.cc (right): https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.cc:8: #include "base/bind_helpers.h" On 2014/12/29 18:45:42, bengr wrote: > Why is this needed? Removed. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.cc:9: #include "base/threading/thread.h" On 2014/12/29 18:45:42, bengr wrote: > Why is this needed? Removed. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.cc:13: #include "net/url_request/url_request_context.h" On 2014/12/29 18:45:42, bengr wrote: > Why is this needed? Removed. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.cc:14: #include "net/url_request/url_request_context_getter.h" On 2014/12/29 18:45:42, bengr wrote: > Why is this needed? Removed. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.cc:33: void DataReductionProxyUIManager::OnBlockingPageDone( On 2014/12/29 18:45:42, bengr wrote: > This needs a test, that checks that the callbacks of the resources are called. Done. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.cc:44: proceed_blocking_page_ = base::Time::Now(); On 2014/12/29 18:45:42, bengr wrote: > #include "base/time/time.h" Done. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.cc:48: void DataReductionProxyUIManager::DisplayBlockingPage( On 2014/12/29 18:45:42, bengr wrote: > This needs tests. You should cover what happens if proceed_blocking_page_ is > more or less than 5 minutes from Now(), and what happens when you have a > null/non-null RenderViewHost and a null/non-null WebContents. You'll probably > want to move some of the logic out to virtual helpers. Done. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.cc:54: base::TimeDelta::FromMinutes(5)) { On 2014/12/29 18:45:42, bengr wrote: > define 5 as a constant at the top of the file and comment it. Done. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.cc:66: content::WebContents* web_contents = NULL; On 2014/12/29 18:45:42, bengr wrote: > nullptr Done. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.cc:83: LOG(WARNING) << "BLOCKING PAGE!"; On 2014/12/29 18:45:42, bengr wrote: > Remove. Done. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.cc:84: std::vector<BypassResource> resources; On 2014/12/29 18:45:42, bengr wrote: > #include <vector> Done. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.h (right): https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.h:16: class Time; On 2014/12/29 18:45:42, bengr wrote: > Remove this forward declaration. Done. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.h:17: class Thread; On 2014/12/29 18:45:42, bengr wrote: > Why is this needed? No longer needed. Removed. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.h:66: base::Time proceed_blocking_page_; On 2014/12/29 18:45:42, bengr wrote: > Why is this a Time? Consider a more descriptive variable name, and add a > comment. What other type do you suggest? https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_service.h (right): https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_service.h:8: #include "base/memory/scoped_ptr.h" On 2014/12/29 18:45:42, bengr wrote: > Not needed. But you do need "base/memory/ref_counted.h" Done. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_service.h:9: #include "components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.h" On 2014/12/29 18:45:42, bengr wrote: > Can this be forward declared? Done. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_service.h:19: typedef base::Callback<const net::ProxyConfig&()> ProxyConfigGetter; On 2014/12/29 18:45:42, bengr wrote: > This is defined in DRPNetworkDelegate, no? If so, use that one. Done. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_service.h:28: const scoped_refptr<DataReductionProxyUIManager>& ui_manager() const; On 2014/12/29 18:45:42, bengr wrote: > Do you really need a scoped_refptr? Would a scoped_ptr do instead? The ui manager is a ref counted object since we invoke one of its class methods with base::Bind(). https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_service.h:34: ProxyConfigGetter proxy_config_getter_; On 2014/12/29 18:45:42, bengr wrote: > #include DRPNetworkDelegate Done. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc (right): https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:91: bool DataReductionProxyParams::IsIncludedInBypassWarningTrial() { On 2014/12/29 18:45:42, bengr wrote: > This name is misleading because there's no actual trial yet. Maybe: > IsBypassWarningEnabledInFlags()? Done. https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.h (right): https://codereview.chromium.org/684223003/diff/140001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:143: virtual bool IsBypassedByDataReductionProxyLocalRules( On 2014/12/29 18:45:43, bengr wrote: > As I mentioned previously, I'd prefer you find a way to use the test params in > test utils instead of virtualizing these methods and creating your own. Still need to keep virtual in order to override in test params https://codereview.chromium.org/684223003/diff/140001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/684223003/diff/140001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:49533: + <int value="-2099035488" label="enable-data-reduction-proxy-bypass-warning"/> On 2014/12/29 18:45:43, bengr wrote: > Would you please double check that all of our flags in about::flags show up here > and if not file a bug against me? Done.
https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc (right): https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc:42: // Enforces warning when the proxy will not be used for a request because it What does "Enforces warning" mean? Please reword. https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc:53: // Do not display a blocking page if the data reduction proxy is bypassed by "a blocking page" -> "the interstitial" "if the data reduction proxy is" -> "if" https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc:63: // Enforces warning when the data reduction proxy explicitly returns a response Reword "enforces warning" here too. https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc:79: // We already checked if the proxy is bypassed by local rules and if the data Rewrite to remove all instances of "we". Capitalize "Data Reduction Proxy" everywhere. Change "blocking page" to "interstitial" everywhere. https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc:94: return kResouceThrottleLogName; indent -2. https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.h (right): https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.h:36: // This feature is for users that wish to be warned when configurations that are Reword: This class is useful, e.g., for users that wish... https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle_unittest.cc (right): https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle_unittest.cc:87: params_.reset(new TestDataReductionProxyParams(0, 0)); #include https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle_unittest.cc:109: // Tests that WillStartUsingNetwork calls DisplayBlockingPage when I like adding parentheses when referring to functions, e.g., WillStartUsingNetwork(), but I don't know of a specific style requirement. Also, this comment is harder to follow than the code. Maybe say something like: Expect to show the interstitial (defer == true), when the data reduction proxy is bypassed by the proxy server, but not when bypassed by local rules on the client (even when the proxy has already been bypassed by the server). https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle_unittest.cc:147: // Tests that WillProcessResponse dose not call DisplayBlockingPage when Rewrite this too. (From the test code I have no idea how DisplayBlockingPage is involved.) Maybe: Expect to show the interstitial (defer = true) when the response is not from the Data Reduction Proxy, but not when it is. And "dose" -> "does". https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.h (right): https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.h:70: // Records the last time the blocking page was shown. Once the blocking page "for another five minutes" -> "for a period of time" because this might change and you already say for how long in the .cc https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.h:76: scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner_; Add a blank line below. https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager_unittest.cc (right): https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager_unittest.cc:54: : state_ (NOT_CALLED), remove space. https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_service.cc (right): https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_service.cc:16: content::BrowserThread::UI), I think this is ok, but better might be to pass these in to the constructor of the UIService. https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.h (right): https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:97: // Returns true if the scheme of |url| is able to go through the data Returns true if the Data Reduction Proxy supports the scheme of the provided |url|. https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... File components/data_reduction_proxy/core/common/data_reduction_proxy_params_test_utils.h (right): https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params_test_utils.h:32: bool IsDataReductionProxy( Add: // Overrides from net::DataReductionProxyParams: https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params_test_utils.h:42: void MockIsDataReductionProxy(bool return_value); Add a comment (a single one will do) for these mock methods.
Patchset #7 (id:220001) has been deleted
https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc (right): https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc:42: // Enforces warning when the proxy will not be used for a request because it On 2014/12/31 01:10:21, bengr wrote: > What does "Enforces warning" mean? Please reword. Done. https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc:53: // Do not display a blocking page if the data reduction proxy is bypassed by On 2014/12/31 01:10:21, bengr wrote: > "a blocking page" -> "the interstitial" > "if the data reduction proxy is" -> "if" Done. https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc:63: // Enforces warning when the data reduction proxy explicitly returns a response On 2014/12/31 01:10:21, bengr wrote: > Reword "enforces warning" here too. Done. https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc:79: // We already checked if the proxy is bypassed by local rules and if the data On 2014/12/31 01:10:21, bengr wrote: > Rewrite to remove all instances of "we". > Capitalize "Data Reduction Proxy" everywhere. > Change "blocking page" to "interstitial" everywhere. Done. https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc:94: return kResouceThrottleLogName; On 2014/12/31 01:10:21, bengr wrote: > indent -2. Done. https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.h (right): https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.h:36: // This feature is for users that wish to be warned when configurations that are On 2014/12/31 01:10:21, bengr wrote: > Reword: > This class is useful, e.g., for users that wish... Done. https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle_unittest.cc (right): https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle_unittest.cc:87: params_.reset(new TestDataReductionProxyParams(0, 0)); On 2014/12/31 01:10:21, bengr wrote: > #include Done. https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle_unittest.cc:109: // Tests that WillStartUsingNetwork calls DisplayBlockingPage when On 2014/12/31 01:10:21, bengr wrote: > I like adding parentheses when referring to functions, e.g., > WillStartUsingNetwork(), but I don't know of a specific style requirement. > > Also, this comment is harder to follow than the code. Maybe say something like: > > Expect to show the interstitial (defer == true), when the data reduction proxy > is bypassed by the proxy server, but not when bypassed by local rules on the > client (even when the proxy has already been bypassed by the server). Done. https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle_unittest.cc:147: // Tests that WillProcessResponse dose not call DisplayBlockingPage when On 2014/12/31 01:10:21, bengr wrote: > Rewrite this too. (From the test code I have no idea how DisplayBlockingPage is > involved.) Maybe: > > Expect to show the interstitial (defer = true) when the response is not from the > Data Reduction Proxy, but not when it is. > > And "dose" -> "does". Done. https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.h (right): https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.h:70: // Records the last time the blocking page was shown. Once the blocking page On 2014/12/31 01:10:21, bengr wrote: > "for another five minutes" -> "for a period of time" because this might change > and you already say for how long in the .cc Done. https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager.h:76: scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner_; On 2014/12/31 01:10:21, bengr wrote: > Add a blank line below. Done. https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager_unittest.cc (right): https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_manager_unittest.cc:54: : state_ (NOT_CALLED), On 2014/12/31 01:10:21, bengr wrote: > remove space. Done. https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_service.cc (right): https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_ui_service.cc:16: content::BrowserThread::UI), On 2014/12/31 01:10:21, bengr wrote: > I think this is ok, but better might be to pass these in to the constructor of > the UIService. Acknowledged. https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.h (right): https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:97: // Returns true if the scheme of |url| is able to go through the data On 2014/12/31 01:10:21, bengr wrote: > Returns true if the Data Reduction Proxy supports the scheme of the provided > |url|. Done. https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... File components/data_reduction_proxy/core/common/data_reduction_proxy_params_test_utils.h (right): https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params_test_utils.h:32: bool IsDataReductionProxy( On 2014/12/31 01:10:21, bengr wrote: > Add: > // Overrides from net::DataReductionProxyParams: Done. https://codereview.chromium.org/684223003/diff/180001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params_test_utils.h:42: void MockIsDataReductionProxy(bool return_value); On 2014/12/31 01:10:21, bengr wrote: > Add a comment (a single one will do) for these mock methods. Done.
Patchset #7 (id:240001) has been deleted
megjablon@chromium.org changed reviewers: + asvitkine@chromium.org, thestig@chromium.org
asvitkine@chromium.org: histograms.xml thestig@chromium.org: chrome/*
Patchset #8 (id:280001) has been deleted
thestig@chromium.org changed reviewers: + lei zhang. mmenke@chromium.org
+mmenke for ProfileIOData and friends. https://codereview.chromium.org/684223003/diff/300001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/684223003/diff/300001/chrome/browser/BUILD.gn... chrome/browser/BUILD.gn:227: "//components/data_reduction_proxy/content", Shouldn't this be Android only? Same in the .gypi file. https://codereview.chromium.org/684223003/diff/300001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/684223003/diff/300001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2246: chrome::VersionInfo::GetChannel() != If you don't mind, just call GetChannel() once and reuse the result in this entire #if block.
histograms lgtm
megjablon@chromium.org changed reviewers: + mmenke@chromium.org - lei zhang. mmenke@chromium.org
Re-adding mmenke for ProfileIOData and friends. Period delimiter error.
https://codereview.chromium.org/684223003/diff/320001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/684223003/diff/320001/chrome/browser/profiles... chrome/browser/profiles/profile_impl.cc:651: #if defined(OS_ANDROID) Why is this only for Android? Why are we passing ownership of a "UI service" over to the IO thread? UI generally lives on the UI thread. https://codereview.chromium.org/684223003/diff/320001/chrome/browser/profiles... chrome/browser/profiles/profile_impl.cc:651: #if defined(OS_ANDROID) Also, why are we creating this on the UI thread in the first place? Can we just create where we call its "set_proxy_config_getter" method?
Patchset #10 (id:340001) has been deleted
Patchset #10 (id:360001) has been deleted
https://codereview.chromium.org/684223003/diff/300001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/684223003/diff/300001/chrome/browser/BUILD.gn... chrome/browser/BUILD.gn:227: "//components/data_reduction_proxy/content", On 2015/01/07 23:49:42, Lei Zhang wrote: > Shouldn't this be Android only? Same in the .gypi file. Done. https://codereview.chromium.org/684223003/diff/300001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/684223003/diff/300001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2246: chrome::VersionInfo::GetChannel() != On 2015/01/07 23:49:42, Lei Zhang wrote: > If you don't mind, just call GetChannel() once and reuse the result in this > entire #if block. Done. https://codereview.chromium.org/684223003/diff/320001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/684223003/diff/320001/chrome/browser/profiles... chrome/browser/profiles/profile_impl.cc:651: #if defined(OS_ANDROID) On 2015/01/09 15:25:13, mmenke wrote: > Why is this only for Android? Why are we passing ownership of a "UI service" > over to the IO thread? UI generally lives on the UI thread. Sent an email with why this is Android only. https://codereview.chromium.org/684223003/diff/320001/chrome/browser/profiles... chrome/browser/profiles/profile_impl.cc:651: #if defined(OS_ANDROID) On 2015/01/09 15:25:13, mmenke wrote: > Also, why are we creating this on the UI thread in the first place? Can we just > create where we call its "set_proxy_config_getter" method? Ya, it makes more sense where we call "set_proxy_config_getter". Moved. Also, this will be moved into a Data Reduction Proxy class in bengr's cl https://codereview.chromium.org/778463002/.
https://codereview.chromium.org/684223003/diff/320001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/684223003/diff/320001/chrome/browser/profiles... chrome/browser/profiles/profile_impl.cc:651: #if defined(OS_ANDROID) On 2015/01/12 22:01:58, megjablon wrote: > On 2015/01/09 15:25:13, mmenke wrote: > > Why is this only for Android? Why are we passing ownership of a "UI service" > > over to the IO thread? UI generally lives on the UI thread. > > Sent an email with why this is Android only. Sorry, I meant why is this not on desktop (Where, admittedly, DRP isn't enabled yet, but we're hooking up other DRP stuff on desktop)? I don't believe this file is even compiled on iOS.
The latest patch fixes the logic of this interstitial to be more like that of safe browsing. This fixes a crash from displaying an interstitial from WillProcessResponse. blundell@chromium.org:components/components_unittests.isolate
megjablon@chromium.org changed reviewers: + blundell@chromium.org
Patchset #11 (id:400001) has been deleted
I haven't looked at the code for correctness. I'd like to hold off on landing this until Ben's CL to mov everything into one IO thread class is done, so we can just call into that to create the throttle, and get rid of the UI service calls https://codereview.chromium.org/684223003/diff/440001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/684223003/diff/440001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:482: (new data_reduction_proxy::DataReductionProxyUIService( DataReductionProxyDebugUIService? Should make it clear that it's only used for debugging, as opposed to a general purpose UI for everyone. https://codereview.chromium.org/684223003/diff/440001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/684223003/diff/440001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:528: #if defined(OS_ANDROID) All these OS_ANDROID checks seem very regression prone, and just a bad idea. Should either add a new #define for them (ENABLE_DATA_REDUCTION_PROXY_DEBUGGING), which at makes it clear which ifdefs are related how, or have a member of DRPPArams that we only set to true on Android when we have the corresponding commandline option set, and look at that everywhere. The latter option will bload up binary size a little, of course (And makes the platform restriction fairly pointless). https://codereview.chromium.org/684223003/diff/440001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:531: IsInterstitalsCommandLineSwitchOn()) { I think "IsInterstitalsCommandLineSwitchOn" could be clearer. Maybe "ForceDataProxy" or "WarnIfNoDataProxy" or something. https://codereview.chromium.org/684223003/diff/440001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:531: IsInterstitalsCommandLineSwitchOn()) { Also suggest indenting IsInterstitalsCommandLineSwitchOn four more. https://codereview.chromium.org/684223003/diff/440001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:540: new data_reduction_proxy::DataReductionProxyResourceThrottle( Suggest renaming this the "DataReductionProxyDebugResourceThrottle" https://codereview.chromium.org/684223003/diff/440001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:543: } Think it's better if the RDHDelegate knows as little as possible about what the service is up to. So I suggest having only the following here: scoped_ptr<content::ResourceThrottle> data_reduction_proxy_throttle = data_reduction_proxy::DataReductionProxyResourceThrottle::MaybeCreate(...); if (data_reduction_proxy_throttle ) throttles.push_back(data_reduction_proxy_throttle.release()); The static MaybeCreate method checks the relevant command line flag, and if the DRP is enabled, etc. You may actually want to remove the enabled check - at least I assume when using the flag, you'd care if the DRP were disabled due to the probe results. https://codereview.chromium.org/684223003/diff/440001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc (right): https://codereview.chromium.org/684223003/diff/440001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc:45: DCHECK(state_ == NOT_BYPASSED); DCHECK_EQ(NOT_BYPASSED, state_) https://codereview.chromium.org/684223003/diff/440001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc:48: return; nit: Use braces when the condition takes more than 1 line
Patchset #13 (id:460001) has been deleted
https://codereview.chromium.org/684223003/diff/440001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/684223003/diff/440001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:482: (new data_reduction_proxy::DataReductionProxyUIService( On 2015/01/16 20:15:27, mmenke wrote: > DataReductionProxyDebugUIService? Should make it clear that it's only used for > debugging, as opposed to a general purpose UI for everyone. Done. https://codereview.chromium.org/684223003/diff/440001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/684223003/diff/440001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:528: #if defined(OS_ANDROID) On 2015/01/16 20:15:27, mmenke wrote: > All these OS_ANDROID checks seem very regression prone, and just a bad idea. > Should either add a new #define for them > (ENABLE_DATA_REDUCTION_PROXY_DEBUGGING), which at makes it clear which ifdefs > are related how, or have a member of DRPPArams that we only set to true on > Android when we have the corresponding commandline option set, and look at that > everywhere. The latter option will bload up binary size a little, of course > (And makes the platform restriction fairly pointless). Done. https://codereview.chromium.org/684223003/diff/440001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:531: IsInterstitalsCommandLineSwitchOn()) { On 2015/01/16 20:15:27, mmenke wrote: > Also suggest indenting IsInterstitalsCommandLineSwitchOn four more. Done. https://codereview.chromium.org/684223003/diff/440001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:540: new data_reduction_proxy::DataReductionProxyResourceThrottle( On 2015/01/16 20:15:27, mmenke wrote: > Suggest renaming this the "DataReductionProxyDebugResourceThrottle" Done. https://codereview.chromium.org/684223003/diff/440001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:543: } On 2015/01/16 20:15:27, mmenke wrote: > Think it's better if the RDHDelegate knows as little as possible about what the > service is up to. So I suggest having only the following here: > > scoped_ptr<content::ResourceThrottle> data_reduction_proxy_throttle = > data_reduction_proxy::DataReductionProxyResourceThrottle::MaybeCreate(...); > if (data_reduction_proxy_throttle ) > throttles.push_back(data_reduction_proxy_throttle.release()); > > The static MaybeCreate method checks the relevant command line flag, and if the > DRP is enabled, etc. > > You may actually want to remove the enabled check - at least I assume when using > the flag, you'd care if the DRP were disabled due to the probe results. Done. https://codereview.chromium.org/684223003/diff/440001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc (right): https://codereview.chromium.org/684223003/diff/440001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc:45: DCHECK(state_ == NOT_BYPASSED); On 2015/01/16 20:15:28, mmenke wrote: > DCHECK_EQ(NOT_BYPASSED, state_) Done. https://codereview.chromium.org/684223003/diff/440001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_resource_throttle.cc:48: return; On 2015/01/16 20:15:28, mmenke wrote: > nit: Use braces when the condition takes more than 1 line Done.
Could you merge this now that Ben's CL has landed?
Patchset #16 (id:540001) has been deleted
Patchset #15 (id:520001) has been deleted
Patchset #14 (id:500001) has been deleted
On 2015/01/30 20:27:35, mmenke wrote: > Could you merge this now that Ben's CL has landed? It's merged now. I added an interface for DRPUIService in core since DRPIOData cannot have dependencies on content. Please take a look at this in particular, and let me know if you have any suggestions. Thanks!
https://codereview.chromium.org/684223003/diff/560001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_ui_service.h (right): https://codereview.chromium.org/684223003/diff/560001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_ui_service.h:22: : public DataReductionProxyDebugUIServiceCore { It doesn't seem that this class has any dependence on //content and thus it could just live in the core part of the component? Is there a plan to add a dependence on //content to this class later on? Note: If it's necessary to keep the two-level hierarchy, the standard naming would be for the interface in the core code to be named DataReductionProxyDebugUIService, and the //content-based impl to be named ContentDataReductionProxyDebugUIService.
profiles/ LGTM https://codereview.chromium.org/684223003/diff/560001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/684223003/diff/560001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2306: channel2 != chrome::VersionInfo::CHANNEL_DEV) { Hrm...Can we just make ENABLE_DATA_REDUCTION_PROXY_DEBUGGING only true on those channels? https://codereview.chromium.org/684223003/diff/560001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/684223003/diff/560001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:542: io_data->data_reduction_proxy_io_data()->params()); Rather than dig into data_reduction_proxy_io_data's guts, can we just pass in io_data->data_reduction_proxy_io_data() here, and have DataReductionProxyDebugResourceThrottle get what it needs from it? Same goes for io_data->IsDataReductionProxyEnabled(): The data_reduction_proxy_io_data already knows if it's enabled, so this should just be: MaybeCreate(request, resource_type, io_data->data_reduction_proxy_io_data()) Then you can also get rid of the data_reduction_proxy_io_data.h include, though you'll still need the other one.
https://codereview.chromium.org/684223003/diff/560001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/684223003/diff/560001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2306: channel2 != chrome::VersionInfo::CHANNEL_DEV) { On 2015/02/02 15:45:52, mmenke wrote: > Hrm...Can we just make ENABLE_DATA_REDUCTION_PROXY_DEBUGGING only true on those > channels? Is this possible? I'm not sure how to do this. https://codereview.chromium.org/684223003/diff/560001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/684223003/diff/560001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:542: io_data->data_reduction_proxy_io_data()->params()); On 2015/02/02 15:45:53, mmenke wrote: > Rather than dig into data_reduction_proxy_io_data's guts, can we just pass in > io_data->data_reduction_proxy_io_data() here, and have > DataReductionProxyDebugResourceThrottle get what it needs from it? > > Same goes for io_data->IsDataReductionProxyEnabled(): The > data_reduction_proxy_io_data already knows if it's enabled, so this should just > be: > > MaybeCreate(request, resource_type, io_data->data_reduction_proxy_io_data()) > > Then you can also get rid of the data_reduction_proxy_io_data.h include, though > you'll still need the other one. Done. https://codereview.chromium.org/684223003/diff/560001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_ui_service.h (right): https://codereview.chromium.org/684223003/diff/560001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_ui_service.h:22: : public DataReductionProxyDebugUIServiceCore { On 2015/02/02 08:12:22, blundell wrote: > It doesn't seem that this class has any dependence on //content and thus it > could just live in the core part of the component? Is there a plan to add a > dependence on //content to this class later on? > > Note: If it's necessary to keep the two-level hierarchy, the standard naming > would be for the interface in the core code to be named > DataReductionProxyDebugUIService, and the //content-based impl to be named > ContentDataReductionProxyDebugUIService. It is dependent on DataReductionProxyUIManager which lives in content and has dependencies on content. Renamed.
https://codereview.chromium.org/684223003/diff/560001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/684223003/diff/560001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2306: channel2 != chrome::VersionInfo::CHANNEL_DEV) { On 2015/02/03 23:21:26, megjablon wrote: > On 2015/02/02 15:45:52, mmenke wrote: > > Hrm...Can we just make ENABLE_DATA_REDUCTION_PROXY_DEBUGGING only true on > those > > channels? > > Is this possible? I'm not sure how to do this. I'm assuming that the channel is a gyp variable, and you can set gyp variables and build-time constants based on other gyp variables.... Anyhow, up to you if you want to investigate.
//components lgtm https://codereview.chromium.org/684223003/diff/600001/components/components_t... File components/components_tests.gyp (right): https://codereview.chromium.org/684223003/diff/600001/components/components_t... components/components_tests.gyp:644: # Dependencies of data_reduction_proxy nit: We're not actually separating out the dependencies like this anymore, they're just lists the same as in any other target now.
Getting close. https://codereview.chromium.org/684223003/diff/600001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/684223003/diff/600001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2292: #if defined(ENABLE_DATA_REDUCTION_PROXY_DEBUGGING) Add a blank line above this line. https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... File components/data_reduction_proxy/content/browser/content_data_reduction_proxy_debug_ui_service.h (right): https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... components/data_reduction_proxy/content/browser/content_data_reduction_proxy_debug_ui_service.h:25: class ContentDataReductionProxyDebugUIService Add a comment that describes the class. https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... components/data_reduction_proxy/content/browser/content_data_reduction_proxy_debug_ui_service.h:28: ContentDataReductionProxyDebugUIService( Add a comment. https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... components/data_reduction_proxy/content/browser/content_data_reduction_proxy_debug_ui_service.h:32: virtual ~ContentDataReductionProxyDebugUIService(); Add a blank line above. https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... components/data_reduction_proxy/content/browser/content_data_reduction_proxy_debug_ui_service.h:35: virtual const net::ProxyConfig& data_reduction_proxy_config() const override; I don't think you need the virtual keyword since it's marked override. https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_resource_throttle.cc (right): https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_resource_throttle.cc:20: Remove blank line. https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_resource_throttle.cc:23: Remove blank line. https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_resource_throttle.cc:47: scoped_ptr<DataReductionProxyDebugResourceThrottle> Move static methods above constructor, here and in .h https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_resource_throttle.cc:51: const DataReductionProxyIOData* io_data) { DCHECK request? https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_ui_manager.cc (right): https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_ui_manager.cc:18: Remove blank line. https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_ui_manager.cc:22: Remove blank line. https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_ui_manager.cc:44: bool DataReductionProxyDebugUIManager::IsTabClosed( Can this be called from any thread? If not, enforce. https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_debug_ui_service.h (right): https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_debug_ui_service.h:18: class DataReductionProxyDebugUIService { Please add comments to this class including a class description and a comment for every method. https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... File components/data_reduction_proxy/core/common/data_reduction_proxy_params_test_utils.cc (right): https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params_test_utils.cc:53: return DataReductionProxyParams::IsBypassedByDataReductionProxyLocalRules( #include drp_params https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... File components/data_reduction_proxy/core/common/data_reduction_proxy_params_test_utils.h (right): https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params_test_utils.h:37: const net::URLRequest& request, you need forward declarations for these classes from net, and for TimeDelta. https://codereview.chromium.org/684223003/diff/600001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/684223003/diff/600001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:50713: + <int value="-2099035488" label="enable-data-reduction-proxy-bypass-warning"/> Consider changing "warning" to "warnings" here and in flags.
New patchsets have been uploaded after l-g-t-m from blundell@chromium.org
Patchset #18 (id:640001) has been deleted
Patchset #18 (id:660001) has been deleted
Patchset #18 (id:680001) has been deleted
Patchset #20 (id:740001) has been deleted
Patchset #19 (id:720001) has been deleted
Patchset #19 (id:720001) has been deleted
Patchset #18 (id:700001) has been deleted
https://codereview.chromium.org/684223003/diff/600001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/684223003/diff/600001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2292: #if defined(ENABLE_DATA_REDUCTION_PROXY_DEBUGGING) On 2015/02/05 00:23:46, bengr wrote: > Add a blank line above this line. Done. https://codereview.chromium.org/684223003/diff/600001/components/components_t... File components/components_tests.gyp (right): https://codereview.chromium.org/684223003/diff/600001/components/components_t... components/components_tests.gyp:644: # Dependencies of data_reduction_proxy On 2015/02/04 09:17:53, blundell wrote: > nit: We're not actually separating out the dependencies like this anymore, > they're just lists the same as in any other target now. Done. https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... File components/data_reduction_proxy/content/browser/content_data_reduction_proxy_debug_ui_service.h (right): https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... components/data_reduction_proxy/content/browser/content_data_reduction_proxy_debug_ui_service.h:25: class ContentDataReductionProxyDebugUIService On 2015/02/05 00:23:46, bengr wrote: > Add a comment that describes the class. Done. https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... components/data_reduction_proxy/content/browser/content_data_reduction_proxy_debug_ui_service.h:28: ContentDataReductionProxyDebugUIService( On 2015/02/05 00:23:46, bengr wrote: > Add a comment. Done. https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... components/data_reduction_proxy/content/browser/content_data_reduction_proxy_debug_ui_service.h:32: virtual ~ContentDataReductionProxyDebugUIService(); On 2015/02/05 00:23:46, bengr wrote: > Add a blank line above. Done. https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... components/data_reduction_proxy/content/browser/content_data_reduction_proxy_debug_ui_service.h:35: virtual const net::ProxyConfig& data_reduction_proxy_config() const override; On 2015/02/05 00:23:46, bengr wrote: > I don't think you need the virtual keyword since it's marked override. Done. https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_resource_throttle.cc (right): https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_resource_throttle.cc:20: On 2015/02/05 00:23:47, bengr wrote: > Remove blank line. Done. https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_resource_throttle.cc:23: On 2015/02/05 00:23:46, bengr wrote: > Remove blank line. Done. https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_resource_throttle.cc:47: scoped_ptr<DataReductionProxyDebugResourceThrottle> On 2015/02/05 00:23:46, bengr wrote: > Move static methods above constructor, here and in .h Done. https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_resource_throttle.cc:51: const DataReductionProxyIOData* io_data) { On 2015/02/05 00:23:46, bengr wrote: > DCHECK request? Done. https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_ui_manager.cc (right): https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_ui_manager.cc:18: On 2015/02/05 00:23:47, bengr wrote: > Remove blank line. Done. https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_ui_manager.cc:22: On 2015/02/05 00:23:47, bengr wrote: > Remove blank line. Done. https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_ui_manager.cc:44: bool DataReductionProxyDebugUIManager::IsTabClosed( On 2015/02/05 00:23:47, bengr wrote: > Can this be called from any thread? If not, enforce. Done. https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_debug_ui_service.h (right): https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_debug_ui_service.h:18: class DataReductionProxyDebugUIService { On 2015/02/05 00:23:47, bengr wrote: > Please add comments to this class including a class description and a comment > for every method. Done. https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... File components/data_reduction_proxy/core/common/data_reduction_proxy_params_test_utils.cc (right): https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params_test_utils.cc:53: return DataReductionProxyParams::IsBypassedByDataReductionProxyLocalRules( On 2015/02/05 00:23:47, bengr wrote: > #include drp_params included in the .h https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... File components/data_reduction_proxy/core/common/data_reduction_proxy_params_test_utils.h (right): https://codereview.chromium.org/684223003/diff/600001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params_test_utils.h:37: const net::URLRequest& request, On 2015/02/05 00:23:47, bengr wrote: > you need forward declarations for these classes from net, and for TimeDelta. Done. https://codereview.chromium.org/684223003/diff/600001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/684223003/diff/600001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:50713: + <int value="-2099035488" label="enable-data-reduction-proxy-bypass-warning"/> On 2015/02/05 00:23:47, bengr wrote: > Consider changing "warning" to "warnings" here and in flags. Done.
Patchset #18 (id:760001) has been deleted
Patchset #19 (id:800001) has been deleted
Patchset #19 (id:800001) has been deleted
Patchset #19 (id:820001) has been deleted
Patchset #19 (id:840001) has been deleted
Patchset #19 (id:860001) has been deleted
lgtm, with nits. https://codereview.chromium.org/684223003/diff/880001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_resource_throttle.h (right): https://codereview.chromium.org/684223003/diff/880001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_resource_throttle.h:73: // Overrides content::ResourceThrottle. Either say "Overrides of content::ResourceThrottle:" or "content::ResourceThrottle overrides:" https://codereview.chromium.org/684223003/diff/880001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_debug_ui_service.h (right): https://codereview.chromium.org/684223003/diff/880001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_debug_ui_service.h:30: scoped_refptr<DataReductionProxyDebugUIManager>& ui_manager() const = 0; I'd put the line break just before ui_manager() instead. https://codereview.chromium.org/684223003/diff/880001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h (right): https://codereview.chromium.org/684223003/diff/880001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:125: // |temporary_statistics_prefs_| is used only until DataReductionProxySettings Add a blank line above.
New patchsets have been uploaded after l-g-t-m from bengr@chromium.org
https://codereview.chromium.org/684223003/diff/880001/components/data_reducti... File components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_resource_throttle.h (right): https://codereview.chromium.org/684223003/diff/880001/components/data_reducti... components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_resource_throttle.h:73: // Overrides content::ResourceThrottle. On 2015/02/13 16:23:54, bengr wrote: > Either say "Overrides of content::ResourceThrottle:" or > "content::ResourceThrottle overrides:" Done. https://codereview.chromium.org/684223003/diff/880001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_debug_ui_service.h (right): https://codereview.chromium.org/684223003/diff/880001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_debug_ui_service.h:30: scoped_refptr<DataReductionProxyDebugUIManager>& ui_manager() const = 0; On 2015/02/13 16:23:54, bengr wrote: > I'd put the line break just before ui_manager() instead. Done. https://codereview.chromium.org/684223003/diff/880001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h (right): https://codereview.chromium.org/684223003/diff/880001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:125: // |temporary_statistics_prefs_| is used only until DataReductionProxySettings On 2015/02/13 16:23:55, bengr wrote: > Add a blank line above. Done.
chrome/ lgtm https://codereview.chromium.org/684223003/diff/920001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/684223003/diff/920001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2344: chrome::VersionInfo::Channel channel2 = chrome::VersionInfo::GetChannel(); I would just do: #if defined(OS_ANDROID) || defined(ENABLE_DATA_REDUCTION_PROXY_DEBUGGING) chrome::VersionInfo::Channel channel = chrome::VersionInfo::GetChannel(); #endif #if defined(OS_ANDROID) ... #endif #if defined(ENABLE_DATA_REDUCTION_PROXY_DEBUGGING) ... #endif
New patchsets have been uploaded after l-g-t-m from thestig@chromium.org
Patchset #22 (id:940001) has been deleted
https://codereview.chromium.org/684223003/diff/920001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/684223003/diff/920001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2344: chrome::VersionInfo::Channel channel2 = chrome::VersionInfo::GetChannel(); On 2015/02/13 20:11:11, Lei Zhang wrote: > I would just do: > #if defined(OS_ANDROID) || defined(ENABLE_DATA_REDUCTION_PROXY_DEBUGGING) > chrome::VersionInfo::Channel channel = chrome::VersionInfo::GetChannel(); > #endif > #if defined(OS_ANDROID) > ... > #endif > > #if defined(ENABLE_DATA_REDUCTION_PROXY_DEBUGGING) > ... > #endif Done.
The CQ bit was checked by megjablon@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/684223003/960001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
megjablon@chromium.org changed reviewers: + creis@chromium.org
creis: Deps
components/data_reduction_proxy/content/DEPS LGTM.
The CQ bit was checked by megjablon@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/684223003/960001
Message was sent while issue was closed.
Committed patchset #22 (id:960001)
Message was sent while issue was closed.
Patchset 22 (id:??) landed as https://crrev.com/5effca2eb326a4c6bc379e1035e183c2d2cdea83 Cr-Commit-Position: refs/heads/master@{#316330} |
