Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2875993002:
Converging the two previews type enums (Closed)
DescriptionConverging the two previews type enums
This CL removes the infobar previews type enum and uses the
previews::PreviewsType enum instead. This also cleans up the code
slightly and introduces a switch statement in infobar_delegate to
enforce that new previews are handled by the infobar link click
appropriately.
BUG=704335
Review-Url: https://codereview.chromium.org/2875993002
Cr-Commit-Position: refs/heads/master@{#472122}
Committed: https://chromium.googlesource.com/chromium/src/+/4e27eb337661bda1d327121f954418af29c223f7
Patch Set 1 #Patch Set 2 : test fix #Patch Set 3 : header change #
Total comments: 16
Patch Set 4 : megjablon comments #
Total comments: 2
Patch Set 5 : megjablon nit #
Messages
Total messages: 32 (25 generated)
The CQ bit was checked by ryansturm@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...
ryansturm@chromium.org changed reviewers: + megjablon@chromium.org
megjablon: PTAL, let me know if I went overboard with the refactoring.
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 ryansturm@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.
The CQ bit was checked by ryansturm@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/2875993002/diff/40001/chrome/browser/net/spdy... File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc (right): https://codereview.chromium.org/2875993002/diff/40001/chrome/browser/net/spdy... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:68: web_contents, previews::PreviewsType::LOFI, true /* is_data_saver_user */, #include "components/previews/core/previews_experiments.h" https://codereview.chromium.org/2875993002/diff/40001/chrome/browser/previews... File chrome/browser/previews/previews_infobar_delegate.cc (right): https://codereview.chromium.org/2875993002/diff/40001/chrome/browser/previews... chrome/browser/previews/previews_infobar_delegate.cc:33: static_cast<int32_t>(PreviewsInfoBarDelegate::INFOBAR_INDEX_BOUNDARY); Can we just use INFOBAR_INDEX_BOUNDARY without casting? I see other FactoryGet()'s just using the enum max directly. https://codereview.chromium.org/2875993002/diff/40001/chrome/browser/previews... chrome/browser/previews/previews_infobar_delegate.cc:39: ->Add(static_cast<int>(action)); Does this need to be cast? https://codereview.chromium.org/2875993002/diff/40001/chrome/browser/previews... chrome/browser/previews/previews_infobar_delegate.cc:80: // crbug.com/707272 nit: extra space https://codereview.chromium.org/2875993002/diff/40001/chrome/browser/previews... chrome/browser/previews/previews_infobar_delegate.cc:208: previews_type_ == previews::PreviewsType::LOFI) { Don't you still want: if (!data_reduction_proxy::params::IsBlackListEnabledForServerPreviews()) https://codereview.chromium.org/2875993002/diff/40001/chrome/browser/previews... File chrome/browser/previews/previews_infobar_delegate.h (right): https://codereview.chromium.org/2875993002/diff/40001/chrome/browser/previews... chrome/browser/previews/previews_infobar_delegate.h:68: previews::PreviewsType previews_type_; Either rename these all to previews_type or keep as infobar_type_ https://codereview.chromium.org/2875993002/diff/40001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc (right): https://codereview.chromium.org/2875993002/diff/40001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:115: // state even if opt out information was reported. Similarly, if the app was I don't understand this first sentence. https://codereview.chromium.org/2875993002/diff/40001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:116: // backgrounded (android) do not report opt out information as an opt out can nit: capitalize Android Also does it matter if we set the opt out value if the pingback was already sent?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ryansturm@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/2875993002/diff/40001/chrome/browser/net/spdy... File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc (right): https://codereview.chromium.org/2875993002/diff/40001/chrome/browser/net/spdy... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:68: web_contents, previews::PreviewsType::LOFI, true /* is_data_saver_user */, On 2017/05/11 23:49:45, megjablon wrote: > #include "components/previews/core/previews_experiments.h" Already included https://codereview.chromium.org/2875993002/diff/40001/chrome/browser/previews... File chrome/browser/previews/previews_infobar_delegate.cc (right): https://codereview.chromium.org/2875993002/diff/40001/chrome/browser/previews... chrome/browser/previews/previews_infobar_delegate.cc:33: static_cast<int32_t>(PreviewsInfoBarDelegate::INFOBAR_INDEX_BOUNDARY); On 2017/05/11 23:49:45, megjablon wrote: > Can we just use INFOBAR_INDEX_BOUNDARY without casting? I see other > FactoryGet()'s just using the enum max directly. I'd rather be explicit about casts from enum to int32_t as it's not a direct mapping on all machines and not use an implicit c-style cast (https://google.github.io/styleguide/cppguide.html#Casting). Although in the case of enum, which is implicitly cast-able to all integral types especially for a well-defined set of values, it's probably fine. Let me now if it bothers you because the implicit cast should be safe. https://codereview.chromium.org/2875993002/diff/40001/chrome/browser/previews... chrome/browser/previews/previews_infobar_delegate.cc:39: ->Add(static_cast<int>(action)); On 2017/05/11 23:49:45, megjablon wrote: > Does this need to be cast? Same as above comment. I am changing it to int32_t, since I copied it from elsewhere that was wrong. https://codereview.chromium.org/2875993002/diff/40001/chrome/browser/previews... chrome/browser/previews/previews_infobar_delegate.cc:80: // crbug.com/707272 On 2017/05/11 23:49:45, megjablon wrote: > nit: extra space Done. https://codereview.chromium.org/2875993002/diff/40001/chrome/browser/previews... chrome/browser/previews/previews_infobar_delegate.cc:208: previews_type_ == previews::PreviewsType::LOFI) { On 2017/05/11 23:49:45, megjablon wrote: > Don't you still want: > > if (!data_reduction_proxy::params::IsBlackListEnabledForServerPreviews()) Done. IncrementDataReductionProxyPrefs had an early return for that check, but I moved it out since I don't have a better name for that helper method that takes into account that it might or might not change prefs. https://codereview.chromium.org/2875993002/diff/40001/chrome/browser/previews... File chrome/browser/previews/previews_infobar_delegate.h (right): https://codereview.chromium.org/2875993002/diff/40001/chrome/browser/previews... chrome/browser/previews/previews_infobar_delegate.h:68: previews::PreviewsType previews_type_; On 2017/05/11 23:49:45, megjablon wrote: > Either rename these all to previews_type or keep as infobar_type_ Done. https://codereview.chromium.org/2875993002/diff/40001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc (right): https://codereview.chromium.org/2875993002/diff/40001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:115: // state even if opt out information was reported. Similarly, if the app was On 2017/05/11 23:49:45, megjablon wrote: > I don't understand this first sentence. Done. https://codereview.chromium.org/2875993002/diff/40001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:116: // backgrounded (android) do not report opt out information as an opt out can On 2017/05/11 23:49:45, megjablon wrote: > nit: capitalize Android > > Also does it matter if we set the opt out value if the pingback was already > sent? Done. Re-worded
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % nit https://codereview.chromium.org/2875993002/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc (right): https://codereview.chromium.org/2875993002/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:114: // Only report opt out information if a server previews was shown (otherwise, nit: s/previews/preview
The CQ bit was checked by ryansturm@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/2875993002/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc (right): https://codereview.chromium.org/2875993002/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:114: // Only report opt out information if a server previews was shown (otherwise, On 2017/05/15 19:22:01, megjablon wrote: > nit: s/previews/preview Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ryansturm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from megjablon@chromium.org Link to the patchset: https://codereview.chromium.org/2875993002/#ps80001 (title: "megjablon nit")
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": 80001, "attempt_start_ts": 1494950689972150,
"parent_rev": "1918812f554cb98a80103562674f5b5b569648db", "commit_rev":
"4e27eb337661bda1d327121f954418af29c223f7"}
Message was sent while issue was closed.
Description was changed from ========== Converging the two previews type enums This CL removes the infobar previews type enum and uses the previews::PreviewsType enum instead. This also cleans up the code slightly and introduces a switch statement in infobar_delegate to enforce that new previews are handled by the infobar link click appropriately. BUG=704335 ========== to ========== Converging the two previews type enums This CL removes the infobar previews type enum and uses the previews::PreviewsType enum instead. This also cleans up the code slightly and introduces a switch statement in infobar_delegate to enforce that new previews are handled by the infobar link click appropriately. BUG=704335 Review-Url: https://codereview.chromium.org/2875993002 Cr-Commit-Position: refs/heads/master@{#472122} Committed: https://chromium.googlesource.com/chromium/src/+/4e27eb337661bda1d327121f9544... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/4e27eb337661bda1d327121f9544... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
