Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
336543003:
Add AllowNetworkPrediction preference name and enum. (Closed)
DescriptionAdd AllowNetworkPrediction preference name and enum.
BUG=334602
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278789
Patch Set 1 #
Total comments: 25
Patch Set 2 : Address msg2. #
Total comments: 6
Patch Set 3 : Address msg4. #
Total comments: 6
Patch Set 4 : Address msg8. #
Messages
Total messages: 14 (0 generated)
https://codereview.chromium.org/336543003/diff/1/chrome/common/network_predic... File chrome/common/network_prediction_options.h (right): https://codereview.chromium.org/336543003/diff/1/chrome/common/network_predic... chrome/common/network_prediction_options.h:5: #ifndef CHROME_COMMON_NETWORK_PREDICTION_OPTIONS_H_ I don't see any other enums for preferences defined in this directory. Let's move this to chrome/browser/net/predictor.h so that it becomes chrome::Predictor::NetworkPredictionOptions. https://codereview.chromium.org/336543003/diff/1/chrome/common/network_predic... chrome/common/network_prediction_options.h:8: namespace { Should be 'namespace chrome'. Things in an unnamed namespace can't be seen outside of the given file. https://codereview.chromium.org/336543003/diff/1/chrome/common/network_predic... chrome/common/network_prediction_options.h:12: enum class AllowNetworkPredictionOptions = { Remove 'class'. Suggest 'NetworkPredictionOptions' for the name. https://codereview.chromium.org/336543003/diff/1/chrome/common/network_predic... chrome/common/network_prediction_options.h:13: ALWAYS, NETWORK_PREDICTION_ALWAYS. Most of our enum values are prefixed with the name of the enum. https://codereview.chromium.org/336543003/diff/1/chrome/common/network_predic... chrome/common/network_prediction_options.h:14: WIFI_ONLY, NETWORK_PREDICTION_UNMETERED_ONLY Let's go with UNMETERED as opposed to WIFI. It's closer to what we actually want. https://codereview.chromium.org/336543003/diff/1/chrome/common/network_predic... chrome/common/network_prediction_options.h:15: NEVER NETWORK_PREDICTION_NEVER https://codereview.chromium.org/336543003/diff/1/chrome/common/network_predic... chrome/common/network_prediction_options.h:18: } // namespace // namespace chrome https://codereview.chromium.org/336543003/diff/1/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/336543003/diff/1/chrome/common/pref_names.cc#... chrome/common/pref_names.cc:500: // NOTE: This will be obsoleted per issue 334602. Replace above with TODO(bnc): Remove kNetworkPredictionEnabled once kAllowNetworkPrediction is functioning as per crbug.com/334602. https://codereview.chromium.org/336543003/diff/1/chrome/common/pref_names.cc#... chrome/common/pref_names.cc:504: // ALWAYS, NEVER, or WIFI_ONLY. Suggest pointing to where the enum is defined chrome::Predictor::NetworkPredictionOptions (after you move it) instead of enumerating here. That way we can change the values without updating this comment. https://codereview.chromium.org/336543003/diff/1/chrome/common/pref_names.cc#... chrome/common/pref_names.cc:507: // NOTE: This preference is not respected yet. See issue 334602. Replace NOTE line above with: TODO(bnc): Implement this preference as per crbug.com/334602. https://codereview.chromium.org/336543003/diff/1/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/336543003/diff/1/chrome/common/pref_names.h#n... chrome/common/pref_names.h:182: extern const char kNetworkPredictionEnabled[]; // WILL BE OBSOLETE I would remove this comment and the one below as there is nothing that the developer reading this can do about it until kAllowNetworkPrediction is implemented. Also, two spaces between code and comment per style guide. https://codereview.chromium.org/336543003/diff/1/chrome/common/pref_names.h#n... chrome/common/pref_names.h:183: extern const char kAllowNetworkPrediction[]; // NOT IMPLEMENTED YET Remove comment
https://codereview.chromium.org/336543003/diff/1/chrome/common/network_predic... File chrome/common/network_prediction_options.h (right): https://codereview.chromium.org/336543003/diff/1/chrome/common/network_predic... chrome/common/network_prediction_options.h:5: #ifndef CHROME_COMMON_NETWORK_PREDICTION_OPTIONS_H_ On 2014/06/13 11:26:39, jkarlin wrote: > I don't see any other enums for preferences defined in this directory. Let's > move this to chrome/browser/net/predictor.h so that it becomes > chrome::Predictor::NetworkPredictionOptions. Done. https://codereview.chromium.org/336543003/diff/1/chrome/common/network_predic... chrome/common/network_prediction_options.h:8: namespace { On 2014/06/13 11:26:40, jkarlin wrote: > Should be 'namespace chrome'. Things in an unnamed namespace can't be seen > outside of the given file. Everything in chrome/browser/net/predictor.h is in the chrome_browser_net namespace, I moved it into that namespace for consistency. https://codereview.chromium.org/336543003/diff/1/chrome/common/network_predic... chrome/common/network_prediction_options.h:12: enum class AllowNetworkPredictionOptions = { On 2014/06/13 11:26:39, jkarlin wrote: > Remove 'class'. Suggest 'NetworkPredictionOptions' for the name. Done. Also removed =. https://codereview.chromium.org/336543003/diff/1/chrome/common/network_predic... chrome/common/network_prediction_options.h:13: ALWAYS, On 2014/06/13 11:26:39, jkarlin wrote: > NETWORK_PREDICTION_ALWAYS. Most of our enum values are prefixed with the name > of the enum. Done. https://codereview.chromium.org/336543003/diff/1/chrome/common/network_predic... chrome/common/network_prediction_options.h:14: WIFI_ONLY, On 2014/06/13 11:26:39, jkarlin wrote: > NETWORK_PREDICTION_UNMETERED_ONLY > > Let's go with UNMETERED as opposed to WIFI. It's closer to what we actually > want. Are we not worried about the inconsistency between this name and what we are actually doing (and what is reflected on the UI)? https://codereview.chromium.org/336543003/diff/1/chrome/common/network_predic... chrome/common/network_prediction_options.h:15: NEVER On 2014/06/13 11:26:40, jkarlin wrote: > NETWORK_PREDICTION_NEVER Done. https://codereview.chromium.org/336543003/diff/1/chrome/common/network_predic... chrome/common/network_prediction_options.h:18: } // namespace On 2014/06/13 11:26:40, jkarlin wrote: > // namespace chrome Mute: moved to the namespace that is used by everything else in chrome/browser/net/predictor.h https://codereview.chromium.org/336543003/diff/1/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/336543003/diff/1/chrome/common/pref_names.cc#... chrome/common/pref_names.cc:500: // NOTE: This will be obsoleted per issue 334602. On 2014/06/13 11:26:40, jkarlin wrote: > Replace above with TODO(bnc): Remove kNetworkPredictionEnabled once > kAllowNetworkPrediction is functioning as per crbug.com/334602. Done. https://codereview.chromium.org/336543003/diff/1/chrome/common/pref_names.cc#... chrome/common/pref_names.cc:504: // ALWAYS, NEVER, or WIFI_ONLY. On 2014/06/13 11:26:40, jkarlin wrote: > Suggest pointing to where the enum is defined > chrome::Predictor::NetworkPredictionOptions (after you move it) instead of > enumerating here. That way we can change the values without updating this > comment. Done. https://codereview.chromium.org/336543003/diff/1/chrome/common/pref_names.cc#... chrome/common/pref_names.cc:507: // NOTE: This preference is not respected yet. See issue 334602. On 2014/06/13 11:26:40, jkarlin wrote: > Replace NOTE line above with: > > TODO(bnc): Implement this preference as per crbug.com/334602. Done. https://codereview.chromium.org/336543003/diff/1/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/336543003/diff/1/chrome/common/pref_names.h#n... chrome/common/pref_names.h:182: extern const char kNetworkPredictionEnabled[]; // WILL BE OBSOLETE On 2014/06/13 11:26:40, jkarlin wrote: > I would remove this comment and the one below as there is nothing that the > developer reading this can do about it until kAllowNetworkPrediction is > implemented. > > Also, two spaces between code and comment per style guide. Done. https://codereview.chromium.org/336543003/diff/1/chrome/common/pref_names.h#n... chrome/common/pref_names.h:183: extern const char kAllowNetworkPrediction[]; // NOT IMPLEMENTED YET On 2014/06/13 11:26:40, jkarlin wrote: > Remove comment Done.
Great! Just a couple of nits. Also, please move NetworkPredictionOptions inside of Predictor. https://codereview.chromium.org/336543003/diff/20001/chrome/browser/net/predi... File chrome/browser/net/predictor.h (right): https://codereview.chromium.org/336543003/diff/20001/chrome/browser/net/predi... chrome/browser/net/predictor.h:612: // enum describing when to allow network predictions based on connection type. Comments should be complete sentences (with capitalization). https://codereview.chromium.org/336543003/diff/20001/chrome/browser/net/predi... chrome/browser/net/predictor.h:614: // TODO(bnc) implement as per crbug.com/334602. don't forget the ':', TODO(bnc): https://codereview.chromium.org/336543003/diff/20001/chrome/browser/net/predi... chrome/browser/net/predictor.h:615: enum NetworkPredictionOptions { Please move the enum into the Predictor class in public:
https://codereview.chromium.org/336543003/diff/1/chrome/common/network_predic... File chrome/common/network_prediction_options.h (right): https://codereview.chromium.org/336543003/diff/1/chrome/common/network_predic... chrome/common/network_prediction_options.h:14: WIFI_ONLY, On 2014/06/13 13:49:05, bnc wrote: > On 2014/06/13 11:26:39, jkarlin wrote: > > NETWORK_PREDICTION_UNMETERED_ONLY > > > > Let's go with UNMETERED as opposed to WIFI. It's closer to what we actually > > want. > > Are we not worried about the inconsistency between this name and what we are > actually doing (and what is reflected on the UI)? Yes, that is a good point. Okay, let's keep it.
mmenke: Please review everything. Thanks.
Sorry, just finished implementing jkarlin's last requests. https://codereview.chromium.org/336543003/diff/20001/chrome/browser/net/predi... File chrome/browser/net/predictor.h (right): https://codereview.chromium.org/336543003/diff/20001/chrome/browser/net/predi... chrome/browser/net/predictor.h:612: // enum describing when to allow network predictions based on connection type. On 2014/06/13 14:09:09, jkarlin wrote: > Comments should be complete sentences (with capitalization). Done. https://codereview.chromium.org/336543003/diff/20001/chrome/browser/net/predi... chrome/browser/net/predictor.h:614: // TODO(bnc) implement as per crbug.com/334602. On 2014/06/13 14:09:09, jkarlin wrote: > don't forget the ':', TODO(bnc): Done. https://codereview.chromium.org/336543003/diff/20001/chrome/browser/net/predi... chrome/browser/net/predictor.h:615: enum NetworkPredictionOptions { On 2014/06/13 14:09:09, jkarlin wrote: > Please move the enum into the Predictor class in public: Done.
https://codereview.chromium.org/336543003/diff/40001/chrome/browser/net/predi... File chrome/browser/net/predictor.h (right): https://codereview.chromium.org/336543003/diff/40001/chrome/browser/net/predi... chrome/browser/net/predictor.h:123: }; nit: Looks like enums go before constants, according to the style guide. https://codereview.chromium.org/336543003/diff/40001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/336543003/diff/40001/chrome/common/pref_names... chrome/common/pref_names.cc:515: const char kAllowNetworkPrediction[] = "allow-network-prediction"; nit: Looks like underscores are much more common in names than dashes. https://codereview.chromium.org/336543003/diff/40001/chrome/common/pref_names... chrome/common/pref_names.cc:515: const char kAllowNetworkPrediction[] = "allow-network-prediction"; Should this start with "settings."? I'm not sure if there's a clear distinction between what starts with "settings" and what does not, in this file, but I'm assuming it will appear on the settings screen.
mmenke: thanks for the suggestions. https://codereview.chromium.org/336543003/diff/40001/chrome/browser/net/predi... File chrome/browser/net/predictor.h (right): https://codereview.chromium.org/336543003/diff/40001/chrome/browser/net/predi... chrome/browser/net/predictor.h:123: }; On 2014/06/20 14:48:28, mmenke wrote: > nit: Looks like enums go before constants, according to the style guide. Done. https://codereview.chromium.org/336543003/diff/40001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/336543003/diff/40001/chrome/common/pref_names... chrome/common/pref_names.cc:515: const char kAllowNetworkPrediction[] = "allow-network-prediction"; On 2014/06/20 14:48:28, mmenke wrote: > nit: Looks like underscores are much more common in names than dashes. Done. https://codereview.chromium.org/336543003/diff/40001/chrome/common/pref_names... chrome/common/pref_names.cc:515: const char kAllowNetworkPrediction[] = "allow-network-prediction"; On 2014/06/20 14:48:28, mmenke wrote: > Should this start with "settings."? I'm not sure if there's a clear distinction > between what starts with "settings" and what does not, in this file, but I'm > assuming it will appear on the settings screen. Good call. After looking at all the categories, I propose to put this in "net.".
LGTM
The CQ bit was checked by bnc@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bnc@chromium.org/336543003/60001
Message was sent while issue was closed.
Change committed as 278789 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
