Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1934743003:
Implementing client policy and controller. (Closed)
|
Created:
4 years, 7 months ago by romax Modified:
4 years, 7 months ago Reviewers:
fgorski CC:
chromium-reviews, fgorski+watch_chromium.org, romax+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, dimich+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplementing client policy and controller.
Implemented OfflinePageClientPolicy and its sub class LifetimePolicy.
They're applied to namespaces and are supposed to be used by storage
manager to clear expired offline pages automatically.
BUG=608080
Committed: https://crrev.com/1012d1379619ccd8975e6bc1a9941460d47bbe74
Cr-Commit-Position: refs/heads/master@{#391315}
Patch Set 1 #Patch Set 2 : Fixing build errors. #
Total comments: 48
Patch Set 3 : Addressed comments. #
Total comments: 22
Patch Set 4 : Removed macro and singleton. Addressed comments. #
Total comments: 16
Patch Set 5 : Addressed comments. #
Messages
Total messages: 16 (4 generated)
romax@chromium.org changed reviewers: + fgorski@chromium.org
PTAL
This is very good and concise. https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... File components/offline_pages/client_policy_controller.cc (right): https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.cc:16: OfflinePageClientPolicy TranslateToPolicy( fix indentation. (git cl format should align this to the beginning) https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.cc:16: OfflinePageClientPolicy TranslateToPolicy( MakePolicy is a better name Also you could make it a static method on the policy class/instance and use it in tests. https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.cc:17: std::string name_space, const ref https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.cc:19: base::TimeDelta expire_period, const ref https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.cc:33: if (iter != policies_.end()) { braces not needed. https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.cc:43: policies_.insert(std::make_pair("bookmark", you should be referring to a const here. and below (3x) https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.cc:51: ClientPolicyController::~ClientPolicyController() { Order of methods should be: ctor dtor static instance https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... File components/offline_pages/client_policy_controller.h (right): https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.h:19: static const char* kDefaultPolicy = "defaultPolicy"; const char kDefaultPolicy[] = is more common in the code base https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.h:20: static const int64_t kUnlimitedPage = 0; pages https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.h:22: {kDefaultPolicy, {LifetimePolicy::LifetimeType::TEMPORARY, run git cl format https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.h:27: static ClientPolicyController& GetInstance(); Search in components returns a * instead of reference for GetInstance. Please be consistent. For implementation, look up HistogramManager (lazy) or RLZTracker (using base::Singleton --- that would do). https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.h:29: OfflinePageClientPolicy GetPolicy(std::string name_space); const ref name_space and this function is also const. Add documentation (mention why you are making a copy of the policy) Alternative would be to return const ref as well. https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... File components/offline_pages/client_policy_controller_unittest.cc (right): https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/client_policy_controller_unittest.cc:28: TEST(ClientPolicyControllerTest, GetPredefinedPolicy) { Add verification that a policy for last_n is predefined. You can assert that both are TEMPORARY. For TEMPORARY pages, make an assertion that either they expire or have a limit on number of pages. https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... File components/offline_pages/offline_page_client_policy.h (right): https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/offline_page_client_policy.h:16: // The struct describing the lifetime policy of offlined pages. nit: offline pages no "-d" applies below https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/offline_page_client_policy.h:17: // Page Policies: The following behaviors are controlled by policy https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/offline_page_client_policy.h:18: // a. Automatically offlined or user-initiated. Persistency (lifetime) of the offline page. https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/offline_page_client_policy.h:22: public: nit: not needed for structs. https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/offline_page_client_policy.h:23: // Type of the client, indicating where the archived page would be saved. and whether it could be kept indefinitely. https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/offline_page_client_policy.h:30: LifetimeType type; lifetime_type would be a better name https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/offline_page_client_policy.h:33: base::TimeDelta expire_period; expiration period. I am wondering if storing that using a number of days or hours would not be a better way. After all you don't really want to set it to 30 minutes... but I agree that time delta is the most flexible. Let's ask others on Monday. https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/offline_page_client_policy.h:35: // The maximum number of pages allowd generated by this client. allowed to be saved by https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/offline_page_client_policy.h:36: // kPageUnlimited (0) means no limit set. kPageUnlimited should be defined in this file. https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/offline_page_client_policy.h:37: int64_t page_limit; int should be more than enough. https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/offline_page_client_policy.h:44: public: nit: not needed for structs.
https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... File components/offline_pages/offline_page_client_policy.h (right): https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/offline_page_client_policy.h:35: // The maximum number of pages allowd generated by this client. I'm not clear what "this client" means here - is it total max for whole client (whole application) or is it per namespace? If the latter, perhaps using "namespace" or "client namespace" could help make it clear.
Addressed comments, will send update soon. https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... File components/offline_pages/client_policy_controller.cc (right): https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.cc:16: OfflinePageClientPolicy TranslateToPolicy( On 2016/04/30 17:42:25, fgorski wrote: > fix indentation. (git cl format should align this to the beginning) Done. https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.cc:17: std::string name_space, On 2016/04/30 17:42:24, fgorski wrote: > const ref Done. https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.cc:19: base::TimeDelta expire_period, On 2016/04/30 17:42:25, fgorski wrote: > const ref Done. https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.cc:33: if (iter != policies_.end()) { On 2016/04/30 17:42:25, fgorski wrote: > braces not needed. Done. https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.cc:43: policies_.insert(std::make_pair("bookmark", On 2016/04/30 17:42:25, fgorski wrote: > you should be referring to a const here. and below (3x) Acknowledged. https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.cc:51: ClientPolicyController::~ClientPolicyController() { On 2016/04/30 17:42:25, fgorski wrote: > Order of methods should be: > ctor > dtor > static > instance Done. https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... File components/offline_pages/client_policy_controller.h (right): https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.h:19: static const char* kDefaultPolicy = "defaultPolicy"; On 2016/04/30 17:42:25, fgorski wrote: > const char kDefaultPolicy[] = > is more common in the code base Done. https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.h:20: static const int64_t kUnlimitedPage = 0; On 2016/04/30 17:42:25, fgorski wrote: > pages Done. https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.h:22: {kDefaultPolicy, {LifetimePolicy::LifetimeType::TEMPORARY, On 2016/04/30 17:42:25, fgorski wrote: > run git cl format Done. https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.h:27: static ClientPolicyController& GetInstance(); On 2016/04/30 17:42:25, fgorski wrote: > Search in components returns a * instead of reference for GetInstance. Please be > consistent. > > For implementation, look up HistogramManager (lazy) or RLZTracker (using > base::Singleton --- that would do). Done. https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.h:29: OfflinePageClientPolicy GetPolicy(std::string name_space); On 2016/04/30 17:42:25, fgorski wrote: > const ref name_space and this function is also const. > > Add documentation (mention why you are making a copy of the policy) > Alternative would be to return const ref as well. Done. https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... File components/offline_pages/client_policy_controller_unittest.cc (right): https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/client_policy_controller_unittest.cc:28: TEST(ClientPolicyControllerTest, GetPredefinedPolicy) { On 2016/04/30 17:42:25, fgorski wrote: > Add verification that a policy for last_n is predefined. > You can assert that both are TEMPORARY. > > For TEMPORARY pages, make an assertion that either they expire or have a limit > on number of pages. Done. https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... File components/offline_pages/offline_page_client_policy.h (right): https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/offline_page_client_policy.h:16: // The struct describing the lifetime policy of offlined pages. On 2016/04/30 17:42:26, fgorski wrote: > nit: offline pages > no "-d" > > applies below Done. https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/offline_page_client_policy.h:17: // Page Policies: On 2016/04/30 17:42:25, fgorski wrote: > The following behaviors are controlled by policy Done. https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/offline_page_client_policy.h:18: // a. Automatically offlined or user-initiated. On 2016/04/30 17:42:26, fgorski wrote: > Persistency (lifetime) of the offline page. Done. https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/offline_page_client_policy.h:22: public: On 2016/04/30 17:42:26, fgorski wrote: > nit: not needed for structs. Done. https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/offline_page_client_policy.h:23: // Type of the client, indicating where the archived page would be saved. On 2016/04/30 17:42:25, fgorski wrote: > and whether it could be kept indefinitely. Done. https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/offline_page_client_policy.h:30: LifetimeType type; On 2016/04/30 17:42:26, fgorski wrote: > lifetime_type would be a better name Done. https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/offline_page_client_policy.h:33: base::TimeDelta expire_period; On 2016/04/30 17:42:26, fgorski wrote: > expiration period. > I am wondering if storing that using a number of days or hours would not be a > better way. After all you don't really want to set it to 30 minutes... but I > agree that time delta is the most flexible. Let's ask others on Monday. Acknowledged. https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/offline_page_client_policy.h:35: // The maximum number of pages allowd generated by this client. On 2016/05/02 15:32:34, dougarnett wrote: > I'm not clear what "this client" means here - is it total max for whole client > (whole application) or is > it per namespace? If the latter, perhaps using "namespace" or "client namespace" > could help make it clear. Done. https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/offline_page_client_policy.h:36: // kPageUnlimited (0) means no limit set. On 2016/04/30 17:42:26, fgorski wrote: > kPageUnlimited should be defined in this file. Done. https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/offline_page_client_policy.h:37: int64_t page_limit; On 2016/04/30 17:42:25, fgorski wrote: > int should be more than enough. Done. https://codereview.chromium.org/1934743003/diff/20001/components/offline_page... components/offline_pages/offline_page_client_policy.h:44: public: On 2016/04/30 17:42:26, fgorski wrote: > nit: not needed for structs. Done.
Uploaded changes, PTAL. TIA!
https://codereview.chromium.org/1934743003/diff/40001/components/offline_page... File components/offline_pages/client_policies.h (right): https://codereview.chromium.org/1934743003/diff/40001/components/offline_page... components/offline_pages/client_policies.h:21: #define CLIENT_POLICIES \ no let's have a static method instead of a macro for this. https://codereview.chromium.org/1934743003/diff/40001/components/offline_page... File components/offline_pages/client_policy_controller.cc (right): https://codereview.chromium.org/1934743003/diff/40001/components/offline_page... components/offline_pages/client_policy_controller.cc:20: // TODO(romax) Make here somehow more clever. Don't! Make it as clear as you can. https://codereview.chromium.org/1934743003/diff/40001/components/offline_page... components/offline_pages/client_policy_controller.cc:21: // Using macros to insert predefined client policies in client_policies.h. Don't. Code it by hand, it will be replaced by experiment, and if not, you still benefit from not having a macro. https://codereview.chromium.org/1934743003/diff/40001/components/offline_page... File components/offline_pages/client_policy_controller.h (right): https://codereview.chromium.org/1934743003/diff/40001/components/offline_page... components/offline_pages/client_policy_controller.h:23: // to get client policies based on name_spaces. namespaces https://codereview.chromium.org/1934743003/diff/40001/components/offline_page... components/offline_pages/client_policy_controller.h:28: const OfflinePageClientPolicy& GetPolicy(const std::string& name_space); name_space) const; Add documentation: * use |name_space| to refer to the arguments. https://codereview.chromium.org/1934743003/diff/40001/components/offline_page... components/offline_pages/client_policy_controller.h:30: static const OfflinePageClientPolicy MakePolicy( put static methods first. Add documentation. https://codereview.chromium.org/1934743003/diff/40001/components/offline_page... components/offline_pages/client_policy_controller.h:33: const base::TimeDelta& expire_period, nit: expiration_period https://codereview.chromium.org/1934743003/diff/40001/components/offline_page... components/offline_pages/client_policy_controller.h:34: int page_limit); is this max number of pages? https://codereview.chromium.org/1934743003/diff/40001/components/offline_page... components/offline_pages/client_policy_controller.h:41: friend struct base::DefaultSingletonTraits<ClientPolicyController>; having read lines 5-12 in the https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/single... Can we make offline page model own this one? And consider this code as an effect of decomposition of hard coded policies? https://codereview.chromium.org/1934743003/diff/40001/components/offline_page... File components/offline_pages/client_policy_controller_unittest.cc (right): https://codereview.chromium.org/1934743003/diff/40001/components/offline_page... components/offline_pages/client_policy_controller_unittest.cc:23: bool isTemporary(const OfflinePageClientPolicy& policy) { did you consider putting this on OfflinePageClientPolicy as an instance method? https://codereview.chromium.org/1934743003/diff/40001/components/offline_page... File components/offline_pages/offline_page_client_policy.h (right): https://codereview.chromium.org/1934743003/diff/40001/components/offline_page... components/offline_pages/offline_page_client_policy.h:37: // The maximum number of pages allowd to be saved by the namespace. nit: allowed
Addressed comments and removed singleton/macros. https://codereview.chromium.org/1934743003/diff/40001/components/offline_page... File components/offline_pages/client_policies.h (right): https://codereview.chromium.org/1934743003/diff/40001/components/offline_page... components/offline_pages/client_policies.h:21: #define CLIENT_POLICIES \ On 2016/05/02 23:10:31, fgorski wrote: > no let's have a static method instead of a macro for this. Done. https://codereview.chromium.org/1934743003/diff/40001/components/offline_page... File components/offline_pages/client_policy_controller.cc (right): https://codereview.chromium.org/1934743003/diff/40001/components/offline_page... components/offline_pages/client_policy_controller.cc:20: // TODO(romax) Make here somehow more clever. On 2016/05/02 23:10:31, fgorski wrote: > Don't! Make it as clear as you can. Done. https://codereview.chromium.org/1934743003/diff/40001/components/offline_page... components/offline_pages/client_policy_controller.cc:21: // Using macros to insert predefined client policies in client_policies.h. On 2016/05/02 23:10:31, fgorski wrote: > Don't. Code it by hand, it will be replaced by experiment, and if not, you still > benefit from not having a macro. Done. https://codereview.chromium.org/1934743003/diff/40001/components/offline_page... File components/offline_pages/client_policy_controller.h (right): https://codereview.chromium.org/1934743003/diff/40001/components/offline_page... components/offline_pages/client_policy_controller.h:23: // to get client policies based on name_spaces. On 2016/05/02 23:10:31, fgorski wrote: > namespaces Done. https://codereview.chromium.org/1934743003/diff/40001/components/offline_page... components/offline_pages/client_policy_controller.h:28: const OfflinePageClientPolicy& GetPolicy(const std::string& name_space); On 2016/05/02 23:10:32, fgorski wrote: > name_space) const; > > Add documentation: > * use |name_space| to refer to the arguments. Done. https://codereview.chromium.org/1934743003/diff/40001/components/offline_page... components/offline_pages/client_policy_controller.h:30: static const OfflinePageClientPolicy MakePolicy( On 2016/05/02 23:10:31, fgorski wrote: > put static methods first. > > Add documentation. Done. https://codereview.chromium.org/1934743003/diff/40001/components/offline_page... components/offline_pages/client_policy_controller.h:33: const base::TimeDelta& expire_period, On 2016/05/02 23:10:31, fgorski wrote: > nit: expiration_period Done. https://codereview.chromium.org/1934743003/diff/40001/components/offline_page... components/offline_pages/client_policy_controller.h:34: int page_limit); On 2016/05/02 23:10:31, fgorski wrote: > is this max number of pages? yes https://codereview.chromium.org/1934743003/diff/40001/components/offline_page... components/offline_pages/client_policy_controller.h:41: friend struct base::DefaultSingletonTraits<ClientPolicyController>; On 2016/05/02 23:10:31, fgorski wrote: > having read lines 5-12 in the > https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/single... > > Can we make offline page model own this one? And consider this code as an effect > of decomposition of hard coded policies? Acknowledged. https://codereview.chromium.org/1934743003/diff/40001/components/offline_page... File components/offline_pages/client_policy_controller_unittest.cc (right): https://codereview.chromium.org/1934743003/diff/40001/components/offline_page... components/offline_pages/client_policy_controller_unittest.cc:23: bool isTemporary(const OfflinePageClientPolicy& policy) { On 2016/05/02 23:10:32, fgorski wrote: > did you consider putting this on OfflinePageClientPolicy as an instance method? no... I dont think it's useful to be a method in the class... since the enum in the ClientPolicy.LifetimePolicy would tell if it's a temporary one. But if you're referring to a validation method, it would be useful. However it wouldn't be necessary until we remove the hand written policies. https://codereview.chromium.org/1934743003/diff/40001/components/offline_page... File components/offline_pages/offline_page_client_policy.h (right): https://codereview.chromium.org/1934743003/diff/40001/components/offline_page... components/offline_pages/offline_page_client_policy.h:37: // The maximum number of pages allowd to be saved by the namespace. On 2016/05/02 23:10:32, fgorski wrote: > nit: allowed Done.
lgtm with nits https://codereview.chromium.org/1934743003/diff/60001/components/offline_page... File components/offline_pages/client_policy_controller.cc (right): https://codereview.chromium.org/1934743003/diff/60001/components/offline_page... components/offline_pages/client_policy_controller.cc:9: #include "base/logging.h" nit: do you need this? https://codereview.chromium.org/1934743003/diff/60001/components/offline_page... components/offline_pages/client_policy_controller.cc:35: base::TimeDelta::FromDays(1), kUnlimitedPages))); it should definitely not be unlimited by default if no reasonable policy is specified. https://codereview.chromium.org/1934743003/diff/60001/components/offline_page... File components/offline_pages/client_policy_controller_unittest.cc (right): https://codereview.chromium.org/1934743003/diff/60001/components/offline_page... components/offline_pages/client_policy_controller_unittest.cc:19: const OfflinePageClientPolicy kExpectedBookmarkPolicy = { not used. https://codereview.chromium.org/1934743003/diff/60001/components/offline_page... components/offline_pages/client_policy_controller_unittest.cc:47: ClientPolicyControllerTest::ClientPolicyControllerTest() {} I think you can skip this and definition. https://codereview.chromium.org/1934743003/diff/60001/components/offline_page... components/offline_pages/client_policy_controller_unittest.cc:55: void ClientPolicyControllerTest::TearDown() { do you really need this? https://codereview.chromium.org/1934743003/diff/60001/components/offline_page... components/offline_pages/client_policy_controller_unittest.cc:67: EXPECT_EQ(policy.name_space, "bookmark"); nit: kBookmark... https://codereview.chromium.org/1934743003/diff/60001/components/offline_page... components/offline_pages/client_policy_controller_unittest.cc:73: EXPECT_EQ(policy.name_space, "last_n"); nit: kLastN... https://codereview.chromium.org/1934743003/diff/60001/components/offline_page... File components/offline_pages/offline_page_client_policy.h (right): https://codereview.chromium.org/1934743003/diff/60001/components/offline_page... components/offline_pages/offline_page_client_policy.h:34: // The time before the page expires. s/before/after which/
Addressed comments and currently running on try bot. Will commit once it shows all green. https://codereview.chromium.org/1934743003/diff/60001/components/offline_page... File components/offline_pages/client_policy_controller.cc (right): https://codereview.chromium.org/1934743003/diff/60001/components/offline_page... components/offline_pages/client_policy_controller.cc:9: #include "base/logging.h" On 2016/05/03 04:43:07, fgorski wrote: > nit: do you need this? Done. https://codereview.chromium.org/1934743003/diff/60001/components/offline_page... components/offline_pages/client_policy_controller.cc:35: base::TimeDelta::FromDays(1), kUnlimitedPages))); On 2016/05/03 04:43:07, fgorski wrote: > it should definitely not be unlimited by default if no reasonable policy is > specified. changed to 10. https://codereview.chromium.org/1934743003/diff/60001/components/offline_page... File components/offline_pages/client_policy_controller_unittest.cc (right): https://codereview.chromium.org/1934743003/diff/60001/components/offline_page... components/offline_pages/client_policy_controller_unittest.cc:19: const OfflinePageClientPolicy kExpectedBookmarkPolicy = { On 2016/05/03 04:43:07, fgorski wrote: > not used. Acknowledged. https://codereview.chromium.org/1934743003/diff/60001/components/offline_page... components/offline_pages/client_policy_controller_unittest.cc:47: ClientPolicyControllerTest::ClientPolicyControllerTest() {} On 2016/05/03 04:43:07, fgorski wrote: > I think you can skip this and definition. Acknowledged. https://codereview.chromium.org/1934743003/diff/60001/components/offline_page... components/offline_pages/client_policy_controller_unittest.cc:55: void ClientPolicyControllerTest::TearDown() { On 2016/05/03 04:43:07, fgorski wrote: > do you really need this? i dont think it's necessary but good to have. https://codereview.chromium.org/1934743003/diff/60001/components/offline_page... components/offline_pages/client_policy_controller_unittest.cc:67: EXPECT_EQ(policy.name_space, "bookmark"); On 2016/05/03 04:43:07, fgorski wrote: > nit: kBookmark... Done. https://codereview.chromium.org/1934743003/diff/60001/components/offline_page... components/offline_pages/client_policy_controller_unittest.cc:73: EXPECT_EQ(policy.name_space, "last_n"); On 2016/05/03 04:43:07, fgorski wrote: > nit: kLastN... Done. https://codereview.chromium.org/1934743003/diff/60001/components/offline_page... File components/offline_pages/offline_page_client_policy.h (right): https://codereview.chromium.org/1934743003/diff/60001/components/offline_page... components/offline_pages/offline_page_client_policy.h:34: // The time before the page expires. On 2016/05/03 04:43:07, fgorski wrote: > s/before/after which/ Done.
The CQ bit was checked by romax@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/1934743003/#ps80001 (title: "Addressed comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1934743003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1934743003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Implementing client policy and controller. Implemented OfflinePageClientPolicy and its sub class LifetimePolicy. They're applied to namespaces and are supposed to be used by storage manager to clear expired offline pages automatically. BUG=608080 ========== to ========== Implementing client policy and controller. Implemented OfflinePageClientPolicy and its sub class LifetimePolicy. They're applied to namespaces and are supposed to be used by storage manager to clear expired offline pages automatically. BUG=608080 Committed: https://crrev.com/1012d1379619ccd8975e6bc1a9941460d47bbe74 Cr-Commit-Position: refs/heads/master@{#391315} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/1012d1379619ccd8975e6bc1a9941460d47bbe74 Cr-Commit-Position: refs/heads/master@{#391315} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
