Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2956703002:
Adds Server Previews protocol UMA plus UMA for not accepting previews (Closed)
|
Created:
3 years, 6 months ago by dougarnett Modified:
3 years, 5 months ago CC:
chromium-reviews, tbansal+watch-data-reduction-proxy_chromium.org, asvitkine+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds Server Previews protocol UMA plus UMA for not accepting previews
BUG=731895
patch from issue 2945423002 at patchset 80001 (http://crrev.com/2945423002#ps80001)
Review-Url: https://codereview.chromium.org/2956703002
Cr-Commit-Position: refs/heads/master@{#483782}
Committed: https://chromium.googlesource.com/chromium/src/+/e6e350f9b03a035a1a027d6aff7d4d134360a1fb
Patch Set 1 #Patch Set 2 : Fixed an enum val #
Total comments: 4
Patch Set 3 : Removed Exp histogram variant #Patch Set 4 : Adds compressed-video as AcceptTransform #Patch Set 5 : Removed debug line #Patch Set 6 : Unittests for AcceptTransform histogram #Patch Set 7 : Added tests for NotAcceptingTransform histogram and fixed unittest hitting new NOTREACHED check #
Total comments: 10
Patch Set 8 : Updates from Ben's feedback #
Total comments: 25
Patch Set 9 : UPdates for Ben's feedback #Patch Set 10 : Merge #Patch Set 11 : TransformType renamed to TransformDirective #Patch Set 12 : Fix merge issue for unittest wrt AlwaysOn and PreviewsDecider #Messages
Total messages: 62 (46 generated)
The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dougarnett@chromium.org changed reviewers: + tbansal@chromium.org
Split out new CL for using 2 histograms. Please see if you like better.
dougarnett@chromium.org changed reviewers: + bengr@chromium.org
https://codereview.chromium.org/2956703002/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2956703002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:189: UMA_HISTOGRAM_ENUMERATION("DataReductionProxy.Protocol.AcceptTransform.Exp", I am still not convinced that we need this separate histogram for ~10 or so dogfood users. In the worst case, it is possible to filter out these ~10 users in the UMA dashboard (there is a way to exclude data from dogfood population). https://codereview.chromium.org/2956703002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:208: } Add NOTREACHED() here?
The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2956703002/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2956703002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:189: UMA_HISTOGRAM_ENUMERATION("DataReductionProxy.Protocol.AcceptTransform.Exp", On 2017/06/23 21:29:36, tbansal1 wrote: > I am still not convinced that we need this separate histogram for ~10 or so > dogfood users. In the worst case, it is possible to filter out these ~10 users > in the UMA dashboard (there is a way to exclude data from dogfood population). Ok, dropped it https://codereview.chromium.org/2956703002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:208: } On 2017/06/23 21:29:36, tbansal1 wrote: > Add NOTREACHED() here? Done.
lgtm
dougarnett@chromium.org changed reviewers: + rkaplow@chromium.org
Robert, would you be able to review these new histograms?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2956703002/diff/120001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2956703002/diff/120001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:193: accept_transform_header_value.find(lite_page_directive())) { This is a very brittle way of finding directives in the CP header. We should have a method that parses the CP header into an object. https://codereview.chromium.org/2956703002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2956703002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/enums.xml:7062: + <int value="5" label="AcceptTransform compressed-video sent"/> Is there a corresponding CT value for this one? https://codereview.chromium.org/2956703002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/enums.xml:7066: + <int value="0" label="Not accepting proxy transforms, transforms disabled"/> Disabled by what? https://codereview.chromium.org/2956703002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/enums.xml:7067: + <int value="1" label="Not accepting proxy transforms, page blacklisted"/> By client side blacklist? https://codereview.chromium.org/2956703002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/enums.xml:7068: + <int value="2" label="Not accepting proxy transforms, cellular-only mode"/> And the network is not cellular? Be explicit.
The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2956703002/diff/120001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2956703002/diff/120001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:193: accept_transform_header_value.find(lite_page_directive())) { On 2017/06/26 18:11:47, bengr wrote: > This is a very brittle way of finding directives in the CP header. We should > have a method that parses the CP header into an object. Added methods to parse headers into a single TransformType and now calling them from these Record methods. https://codereview.chromium.org/2956703002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2956703002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/enums.xml:7062: + <int value="5" label="AcceptTransform compressed-video sent"/> On 2017/06/26 18:11:47, bengr wrote: > Is there a corresponding CT value for this one? No, Flywheel will respond with a 302 redirect to the compressed resource (according to the g3doc). Not sure I want to expand the scope for that (rather skip including the cpat for compressed video instead for this iteration). https://codereview.chromium.org/2956703002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/enums.xml:7066: + <int value="0" label="Not accepting proxy transforms, transforms disabled"/> On 2017/06/26 18:11:48, bengr wrote: > Disabled by what? Eventually, either by finch or user flag disabling the feature. Currently though (depending on blacklist fieldtrial), it may be disabled for one of 3 cases (flag, too many opt outs, too many loads of placeholders). There is currently no reason plumbed through SetLoFiModeOff() to let us distinguish. I think the latter 2 go away with the new blacklist support (which does have a separate code). Clarified that this is disabled on the client though. https://codereview.chromium.org/2956703002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/enums.xml:7067: + <int value="1" label="Not accepting proxy transforms, page blacklisted"/> On 2017/06/26 18:11:48, bengr wrote: > By client side blacklist? Done. https://codereview.chromium.org/2956703002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/enums.xml:7068: + <int value="2" label="Not accepting proxy transforms, cellular-only mode"/> On 2017/06/26 18:11:48, bengr wrote: > And the network is not cellular? Be explicit. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/06/26 21:19:17, dougarnett wrote: > https://codereview.chromium.org/2956703002/diff/120001/components/data_reduct... > File > components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc > (right): > > https://codereview.chromium.org/2956703002/diff/120001/components/data_reduct... > components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:193: > accept_transform_header_value.find(lite_page_directive())) { > On 2017/06/26 18:11:47, bengr wrote: > > This is a very brittle way of finding directives in the CP header. We should > > have a method that parses the CP header into an object. > > Added methods to parse headers into a single TransformType and now calling them > from these Record methods. > > https://codereview.chromium.org/2956703002/diff/120001/tools/metrics/histogra... > File tools/metrics/histograms/enums.xml (right): > > https://codereview.chromium.org/2956703002/diff/120001/tools/metrics/histogra... > tools/metrics/histograms/enums.xml:7062: + <int value="5" > label="AcceptTransform compressed-video sent"/> > On 2017/06/26 18:11:47, bengr wrote: > > Is there a corresponding CT value for this one? > > No, Flywheel will respond with a 302 redirect to the compressed resource > (according to the g3doc). Not sure I want to expand the scope for that (rather > skip including the cpat for compressed video instead for this iteration). > > https://codereview.chromium.org/2956703002/diff/120001/tools/metrics/histogra... > tools/metrics/histograms/enums.xml:7066: + <int value="0" label="Not accepting > proxy transforms, transforms disabled"/> > On 2017/06/26 18:11:48, bengr wrote: > > Disabled by what? > > Eventually, either by finch or user flag disabling the feature. > > Currently though (depending on blacklist fieldtrial), it may be disabled for one > of 3 cases (flag, too many opt outs, too many loads of placeholders). There is > currently no reason plumbed through SetLoFiModeOff() to let us distinguish. I > think the latter 2 go away with the new blacklist support (which does have a > separate code). > > Clarified that this is disabled on the client though. > > https://codereview.chromium.org/2956703002/diff/120001/tools/metrics/histogra... > tools/metrics/histograms/enums.xml:7067: + <int value="1" label="Not accepting > proxy transforms, page blacklisted"/> > On 2017/06/26 18:11:48, bengr wrote: > > By client side blacklist? > > Done. > > https://codereview.chromium.org/2956703002/diff/120001/tools/metrics/histogra... > tools/metrics/histograms/enums.xml:7068: + <int value="2" label="Not accepting > proxy transforms, cellular-only mode"/> > On 2017/06/26 18:11:48, bengr wrote: > > And the network is not cellular? Be explicit. > > Done. Ping Ben - please check if I addressed your feedback sufficiently.
lgtm histograms lg
https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... File components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc (right): https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc:774: headers.SetHeader(chrome_proxy_accept_transform_header(), "empty-image"); Is there anything magical about this string, or did you make the change just so readers get the context? https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc (right): https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:1592: 2 /* NOT_ACCEPTING_TRANSFORM_DISABLED */, 1); Why does NOT_ACCEPTING_TRANSFORM_DISABLED have a value 2 here and a value 0 on line 1571? https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:51: ACCEPT_LITE_PAGE_SENT = 0, How about: enum AcceptTransformEvent { LITE_PAGE_REQUESTED, LITE_PAGE_RECEIVED, EMPTY_IMAGE_POLICY_RECEIVED, EMPTY_IMAGE_REQUESTED, EMPTY_IMAGE_RECEIVED, VIDEO_REQUESTED, ... https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:56: ACCEPT_COMPRESSED_VIDEO_SENT = 5, Why no VIDEO_RECEIVED? https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:209: if (response_headers) { I prefer the style: if (!response_headers) return; switch (...) { ... https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc (right): https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:2180: 0 /* ACCEPT_LITE_PAGE_SENT */, 1); Why are you not using the enum? https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... File components/data_reduction_proxy/core/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_headers.cc:138: base::SplitStringPiece(page_policies_value, "|", base::TRIM_WHITESPACE, So you split and return the first one you find. Would it make more sense to return all policies you find and let the caller verify that only one was provided and that it was EMPTY_IMAGE? https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... File components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h (right): https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h:26: // Transform types that may be parsed out of http headers. http -> http response https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h:27: enum TransformType { A policy isn't a transform type. How about "TransformAnnotations" ? https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h:28: TRANSFORM_NONE, Do we need the prefix "TRANSFORM_" ? https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h:31: TRANSFORM_COMPRESS_VIDEO, Is this a request or a response? If the latter (and maybe also the former), "COMPRESSED" might make more sense. https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h:130: // Parse out the type of accepted transform, if any, in |headers|. How about: // Retrieve the transform type, if any, from |headers|. Also, it's worth saying that |headers| must contain at most one transform type, and that you will do something specific if more than one appears in headers.
The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... File components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc (right): https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc:774: headers.SetHeader(chrome_proxy_accept_transform_header(), "empty-image"); On 2017/06/28 16:45:50, bengr wrote: > Is there anything magical about this string, or did you make the change just so > readers get the context? This was some accepted value to avoid hitting a NOTREACHED check that "foo" now hits https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc (right): https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:1592: 2 /* NOT_ACCEPTING_TRANSFORM_DISABLED */, 1); On 2017/06/28 16:45:50, bengr wrote: > Why does NOT_ACCEPTING_TRANSFORM_DISABLED have a value 2 here and a value 0 on > line 1571? thx - fixed commented name https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:51: ACCEPT_LITE_PAGE_SENT = 0, On 2017/06/28 16:45:50, bengr wrote: > How about: > > enum AcceptTransformEvent { > LITE_PAGE_REQUESTED, > LITE_PAGE_RECEIVED, > EMPTY_IMAGE_POLICY_RECEIVED, > EMPTY_IMAGE_REQUESTED, > EMPTY_IMAGE_RECEIVED, > VIDEO_REQUESTED, > ... Done. https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:56: ACCEPT_COMPRESSED_VIDEO_SENT = 5, On 2017/06/28 16:45:50, bengr wrote: > Why no VIDEO_RECEIVED? Because it is a 302 redirect to the compressed video rather than CPCT. [Btw, I'd rather drop recording this SENT value than try to capture that 302 in the scope of this CL.] https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:209: if (response_headers) { On 2017/06/28 16:45:50, bengr wrote: > I prefer the style: > > if (!response_headers) > return; > > switch (...) { > ... Done. https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc (right): https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:2180: 0 /* ACCEPT_LITE_PAGE_SENT */, 1); On 2017/06/28 16:45:51, bengr wrote: > Why are you not using the enum? 1. not exposed; and 2. because we mean to be testing values that are also defined in histograms as well so inserting a new enum value causing this to break would be a good thing. https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... File components/data_reduction_proxy/core/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_headers.cc:138: base::SplitStringPiece(page_policies_value, "|", base::TRIM_WHITESPACE, On 2017/06/28 16:45:51, bengr wrote: > So you split and return the first one you find. Would it make more sense to > return all policies you find and let the caller verify that only one was > provided and that it was EMPTY_IMAGE? I don't think we should support multiple types here until we actually support more policy types in the code otherwise. The protocol specifies that we ignore unknown policy directives (that have well formed syntax) so we should also not try to enforce that only known ones are returned to us. https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... File components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h (right): https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h:26: // Transform types that may be parsed out of http headers. On 2017/06/28 16:45:51, bengr wrote: > http -> http response but also request headers (was trying to avoid defining twice) https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h:27: enum TransformType { On 2017/06/28 16:45:51, bengr wrote: > A policy isn't a transform type. How about "TransformAnnotations" ? DIRECTIVE? That terminoloy is used on the constant names. https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h:28: TRANSFORM_NONE, On 2017/06/28 16:45:51, bengr wrote: > Do we need the prefix "TRANSFORM_" ? Wanted to avoid confusion with PreviewsState::LITE_PAGE et al. These are about the proxy proto which uses the transform terminology (CPAT/CPCT). Was thinking of keeping narrow scope on this enum for now as we indeed may want to instead parse into a struct instead if it gets more complicated (eg, both CPCT and page-policies or multiple page-policies values). https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h:31: TRANSFORM_COMPRESS_VIDEO, On 2017/06/28 16:45:51, bengr wrote: > Is this a request or a response? If the latter (and maybe also the former), > "COMPRESSED" might make more sense. Done. https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h:130: // Parse out the type of accepted transform, if any, in |headers|. On 2017/06/28 16:45:51, bengr wrote: > How about: > // Retrieve the transform type, if any, from |headers|. > > > Also, it's worth saying that |headers| must contain at most one transform type, > and that you will do something specific if more than > one appears in headers. Done - added note about behavior for dual transforms to next method (that handles responses).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm modulo the naming nit. https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... File components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h (right): https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h:27: enum TransformType { On 2017/06/28 22:12:39, dougarnett wrote: > On 2017/06/28 16:45:51, bengr wrote: > > A policy isn't a transform type. How about "TransformAnnotations" ? > > DIRECTIVE? That terminoloy is used on the constant names. Sgtm.
The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run
On 2017/06/29 00:25:06, bengr wrote: > lgtm modulo the naming nit. > > https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... > File components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h > (right): > > https://codereview.chromium.org/2956703002/diff/140001/components/data_reduct... > components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h:27: > enum TransformType { > On 2017/06/28 22:12:39, dougarnett wrote: > > On 2017/06/28 16:45:51, bengr wrote: > > > A policy isn't a transform type. How about "TransformAnnotations" ? > > > > DIRECTIVE? That terminoloy is used on the constant names. > > Sgtm. Done
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by dougarnett@chromium.org
The CQ bit was checked by dougarnett@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tbansal@chromium.org, rkaplow@chromium.org, bengr@chromium.org Link to the patchset: https://codereview.chromium.org/2956703002/#ps220001 (title: "Fix merge issue for unittest wrt AlwaysOn and PreviewsDecider")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1498847302029350,
"parent_rev": "a45318049ffb398bb8af3309a8b5d9b6882baba3", "commit_rev":
"e6e350f9b03a035a1a027d6aff7d4d134360a1fb"}
Message was sent while issue was closed.
Description was changed from ========== Adds Server Previews protocol UMA plus UMA for not accepting previews BUG=731895 patch from issue 2945423002 at patchset 80001 (http://crrev.com/2945423002#ps80001) ========== to ========== Adds Server Previews protocol UMA plus UMA for not accepting previews BUG=731895 patch from issue 2945423002 at patchset 80001 (http://crrev.com/2945423002#ps80001) Review-Url: https://codereview.chromium.org/2956703002 Cr-Commit-Position: refs/heads/master@{#483782} Committed: https://chromium.googlesource.com/chromium/src/+/e6e350f9b03a035a1a027d6aff7d... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/e6e350f9b03a035a1a027d6aff7d... |
