Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2289143005:
[Offline pages] Add a builder and feature struct to policy (Closed)
|
Created:
4 years, 3 months ago by chili Modified:
4 years, 3 months ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, dimich+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Offline pages] Add a builder and feature struct to policy
BUG=641053
Committed: https://crrev.com/c7ae4807842f90c0808c2a6544b785ab51c4a151
Cr-Commit-Position: refs/heads/master@{#418330}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Renaming #
Total comments: 24
Patch Set 3 : Update tests and code review comments #
Total comments: 5
Patch Set 4 : update comments #Patch Set 5 : make constructor more readable #
Total comments: 4
Patch Set 6 : POD to constructor #
Total comments: 4
Patch Set 7 : final fixes to Justin's comments #Patch Set 8 : fix compile issues in test #
Messages
Total messages: 37 (16 generated)
The CQ bit was checked by chili@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...
chili@chromium.org changed reviewers: + dewittj@chromium.org, fgorski@chromium.org
Hi! This CL isn't quite done yet, but want you guys to take a quick look. The idea is, policy will keep a track of 'features' that each namespace is part of. Then in other places of the code, we can see if the policy supports <download> or <user requested> rather than validating against a specific set of namespaces (that can become out of date) Thanks!
mostly looks good. https://codereview.chromium.org/2289143005/diff/1/components/offline_pages/cl... File components/offline_pages/client_policy_controller.cc (right): https://codereview.chromium.org/2289143005/diff/1/components/offline_pages/cl... components/offline_pages/client_policy_controller.cc:46: .SetUserRequested(true) I see a relative inconsistency between SetUserRequested/IsUserRequested and SetIsDownloaded/IsDownloaded https://codereview.chromium.org/2289143005/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_client_policy.h (right): https://codereview.chromium.org/2289143005/diff/1/components/offline_pages/of... components/offline_pages/offline_page_client_policy.h:60: size_t pages_allowed_per_url; wouldn't this qualify as feature policy?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
romax@chromium.org changed reviewers: + romax@chromium.org
Can you please also update the code in OfflinePageModelImpl::IsUserRequestedPage with your changes? Also the builder looks much better!
Description was changed from ========== [Offline pages] Add a builder and feature struct to policy BUG=641053 ========== to ========== [Offline pages] Add a builder and feature struct to policy BUG=641053 ==========
romax@chromium.org changed reviewers: - romax@chromium.org
On 2016/08/31 23:23:58, romax wrote: > Can you please also update the code in OfflinePageModelImpl::IsUserRequestedPage > with your changes? > Also the builder looks much better! Yes, that's the change I will be making next; wanted to double check that this is a good direction before moving forward
dimich@chromium.org changed reviewers: + dimich@chromium.org
https://codereview.chromium.org/2289143005/diff/1/components/offline_pages/cl... File components/offline_pages/client_policy_controller.h (right): https://codereview.chromium.org/2289143005/diff/1/components/offline_pages/cl... components/offline_pages/client_policy_controller.h:37: bool IsUserRequested(const std::string& name_space) const; Naming - it is not "user requested" that we actually do here. If it is to replace the code in cache clearing feature, the better name woudl eb specifically about what is going on there: "IsRemovedOnChromeCachesReset()" for example. https://codereview.chromium.org/2289143005/diff/1/components/offline_pages/cl... components/offline_pages/client_policy_controller.h:40: bool IsDownloaded(const std::string& name_space) const; Naming - if this is to replace the filter in DownloadUIAdapter::IsVisibleInUI, then perhaps better name would be "IsVisibleInDownloadHomeUI()". It is good to have those to be long and descriptive, since we want to avoid situations when different but similar-looking characteristics are lumped together, or when inspection of call sites is necessary to understand what these bits are enabling/disabling.
Should I lump the 'change code to use new policy bits' here or would you guys prefer separate CL? https://codereview.chromium.org/2289143005/diff/1/components/offline_pages/cl... File components/offline_pages/client_policy_controller.cc (right): https://codereview.chromium.org/2289143005/diff/1/components/offline_pages/cl... components/offline_pages/client_policy_controller.cc:46: .SetUserRequested(true) On 2016/08/31 21:31:00, fgorski wrote: > I see a relative inconsistency between SetUserRequested/IsUserRequested and > SetIsDownloaded/IsDownloaded Done. https://codereview.chromium.org/2289143005/diff/1/components/offline_pages/cl... File components/offline_pages/client_policy_controller.h (right): https://codereview.chromium.org/2289143005/diff/1/components/offline_pages/cl... components/offline_pages/client_policy_controller.h:37: bool IsUserRequested(const std::string& name_space) const; On 2016/09/01 00:06:12, Dmitry Titov wrote: > Naming - it is not "user requested" that we actually do here. If it is to > replace the code in cache clearing feature, the better name woudl eb > specifically about what is going on there: "IsRemovedOnChromeCachesReset()" for > example. Done. https://codereview.chromium.org/2289143005/diff/1/components/offline_pages/cl... components/offline_pages/client_policy_controller.h:40: bool IsDownloaded(const std::string& name_space) const; On 2016/09/01 00:06:12, Dmitry Titov wrote: > Naming - if this is to replace the filter in DownloadUIAdapter::IsVisibleInUI, > then perhaps better name would be "IsVisibleInDownloadHomeUI()". > > It is good to have those to be long and descriptive, since we want to avoid > situations when different but similar-looking characteristics are lumped > together, or when inspection of call sites is necessary to understand what these > bits are enabling/disabling. How does 'IsSupportedByDownload' sound? While I think it's good to have descriptive names, I also want to avoid being so specific that it becomes necessary to have 2 or more of the same methods that do the exact same thing... This is also taken from offline pages suggestions under ntp (https://cs.chromium.org/chromium/src/components/ntp_snippets/offline_pages/of...) which I think is out of date based on the comments there https://codereview.chromium.org/2289143005/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_client_policy.h (right): https://codereview.chromium.org/2289143005/diff/1/components/offline_pages/of... components/offline_pages/offline_page_client_policy.h:60: size_t pages_allowed_per_url; On 2016/08/31 21:31:00, fgorski wrote: > wouldn't this qualify as feature policy? I see feature policies (perhaps I should change name here too) as boolean bits answering whether a particular policy has a particular feature enabled. While pages allowed per url may be a per-feature policy, it doesn't quite fit in the same way as the others.
please update client_policy_controller tests as well. https://codereview.chromium.org/2289143005/diff/20001/components/offline_page... File components/offline_pages/client_policy_controller.cc (right): https://codereview.chromium.org/2289143005/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.cc:20: kBookmarkNamespace, should be removed on cache reset. https://codereview.chromium.org/2289143005/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.cc:24: kLastNNamespace, MakePolicy(kLastNNamespace, LifetimeType::TEMPORARY, should be removed on cache reset. https://codereview.chromium.org/2289143005/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.cc:33: .SetIsRemovedOnCacheReset(true) [This comment was added first, before I marked other places in file.] Items that were requested by the user, like async and downloads are not supposed to be removed by cache reset. Hence when the flag was renamed, we should reverse the value of the flag. (Here and in other marked places.) https://codereview.chromium.org/2289143005/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.cc:36: kCCTNamespace, discuss with dewittj@, but should probably not be removed on cache reset, as this one is controlled externally. https://codereview.chromium.org/2289143005/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.cc:47: .SetIsSupportedByDownload(true) should not be removed on cache reset. https://codereview.chromium.org/2289143005/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.cc:58: kDefaultNamespace, MakePolicy(kDefaultNamespace, LifetimeType::TEMPORARY, should be removed on cache reset. https://codereview.chromium.org/2289143005/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.cc:95: nit: remove extra empty line. https://codereview.chromium.org/2289143005/diff/20001/components/offline_page... File components/offline_pages/client_policy_controller.h (right): https://codereview.chromium.org/2289143005/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.h:36: // Returns whether the policy for |name_space| is user requested. Returns whether offline pages belonging to |name_space| are removed when cache is being reset by user. https://codereview.chromium.org/2289143005/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.h:39: // Returns whether the policy for |name_space| is downloaded. Returns whether offline pages belonging to |name_space| are shown in downloads UI. https://codereview.chromium.org/2289143005/diff/20001/components/offline_page... File components/offline_pages/offline_page_client_policy.h (right): https://codereview.chromium.org/2289143005/diff/20001/components/offline_page... components/offline_pages/offline_page_client_policy.h:72: {lifetime_type, base::TimeDelta::FromDays(0), page_limit}, was this formatting produced by "git cl format". If so I am totally OK with it, just mildly surprised. https://codereview.chromium.org/2289143005/diff/20001/components/offline_page... components/offline_pages/offline_page_client_policy.h:74: {false, false}}); this is where I would probably stuff true in the second position. https://codereview.chromium.org/2289143005/diff/20001/components/offline_page... components/offline_pages/offline_page_client_policy.h:79: const OfflinePageClientPolicy Build() { return policy_; } Document that calling build does not reset the object inside, and mark method as const. Or do the opposite.
Thanks! :) https://codereview.chromium.org/2289143005/diff/20001/components/offline_page... File components/offline_pages/client_policy_controller.cc (right): https://codereview.chromium.org/2289143005/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.cc:20: kBookmarkNamespace, On 2016/09/07 03:25:04, fgorski wrote: > should be removed on cache reset. Changed so that the policy defaults to delete on cache reset https://codereview.chromium.org/2289143005/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.cc:24: kLastNNamespace, MakePolicy(kLastNNamespace, LifetimeType::TEMPORARY, On 2016/09/07 03:25:04, fgorski wrote: > should be removed on cache reset. Done. https://codereview.chromium.org/2289143005/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.cc:33: .SetIsRemovedOnCacheReset(true) On 2016/09/07 03:25:04, fgorski wrote: > [This comment was added first, before I marked other places in file.] > > Items that were requested by the user, like async and downloads are not supposed > to be removed by cache reset. Hence when the flag was renamed, we should reverse > the value of the flag. (Here and in other marked places.) Done. https://codereview.chromium.org/2289143005/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.cc:36: kCCTNamespace, On 2016/09/07 03:25:04, fgorski wrote: > discuss with dewittj@, but should probably not be removed on cache reset, as > this one is controlled externally. Acknowledged. Will talk with dewittj@ when he gets in the office https://codereview.chromium.org/2289143005/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.cc:47: .SetIsSupportedByDownload(true) On 2016/09/07 03:25:05, fgorski wrote: > should not be removed on cache reset. Done. https://codereview.chromium.org/2289143005/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.cc:58: kDefaultNamespace, MakePolicy(kDefaultNamespace, LifetimeType::TEMPORARY, On 2016/09/07 03:25:04, fgorski wrote: > should be removed on cache reset. Done. https://codereview.chromium.org/2289143005/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.cc:95: On 2016/09/07 03:25:04, fgorski wrote: > nit: remove extra empty line. Done. https://codereview.chromium.org/2289143005/diff/20001/components/offline_page... File components/offline_pages/client_policy_controller.h (right): https://codereview.chromium.org/2289143005/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.h:36: // Returns whether the policy for |name_space| is user requested. On 2016/09/07 03:25:05, fgorski wrote: > Returns whether offline pages belonging to |name_space| are removed when cache > is being reset by user. Done. https://codereview.chromium.org/2289143005/diff/20001/components/offline_page... components/offline_pages/client_policy_controller.h:39: // Returns whether the policy for |name_space| is downloaded. On 2016/09/07 03:25:05, fgorski wrote: > Returns whether offline pages belonging to |name_space| are shown in downloads > UI. Done. https://codereview.chromium.org/2289143005/diff/20001/components/offline_page... File components/offline_pages/offline_page_client_policy.h (right): https://codereview.chromium.org/2289143005/diff/20001/components/offline_page... components/offline_pages/offline_page_client_policy.h:72: {lifetime_type, base::TimeDelta::FromDays(0), page_limit}, On 2016/09/07 03:25:05, fgorski wrote: > was this formatting produced by "git cl format". If so I am totally OK with it, > just mildly surprised. Done. https://codereview.chromium.org/2289143005/diff/20001/components/offline_page... components/offline_pages/offline_page_client_policy.h:74: {false, false}}); On 2016/09/07 03:25:05, fgorski wrote: > this is where I would probably stuff true in the second position. Done. https://codereview.chromium.org/2289143005/diff/20001/components/offline_page... components/offline_pages/offline_page_client_policy.h:79: const OfflinePageClientPolicy Build() { return policy_; } On 2016/09/07 03:25:05, fgorski wrote: > Document that calling build does not reset the object inside, and mark method as > const. > Or do the opposite. Done.
lgtm with 2 notes: https://codereview.chromium.org/2289143005/diff/40001/components/offline_page... File components/offline_pages/offline_page_client_policy.h (right): https://codereview.chromium.org/2289143005/diff/40001/components/offline_page... components/offline_pages/offline_page_client_policy.h:77: {false, true}}); could you add in-line comments here to say what those are? https://codereview.chromium.org/2289143005/diff/40001/components/offline_page... components/offline_pages/offline_page_client_policy.h:104: OfflinePageClientPolicy policy_; DISALLOW_COPY_AND_ASSIGN...
I think leaving CCT as cleared with cache clearing is OK, but will loop in relevant folks over email. https://codereview.chromium.org/2289143005/diff/1/components/offline_pages/cl... File components/offline_pages/client_policy_controller.h (right): https://codereview.chromium.org/2289143005/diff/1/components/offline_pages/cl... components/offline_pages/client_policy_controller.h:39: // Returns whether the policy for |name_space| is downloaded. This is worded strangely, does it mean that it should be shown in the download home? https://codereview.chromium.org/2289143005/diff/40001/components/offline_page... File components/offline_pages/offline_page_client_policy.h (right): https://codereview.chromium.org/2289143005/diff/40001/components/offline_page... components/offline_pages/offline_page_client_policy.h:77: {false, true}}); On 2016/09/12 17:43:34, Dmitry Titov wrote: > could you add in-line comments here to say what those are? I'd propose adding default values when possible to all the above structs. For example, FeaturePolicy could have =false, =true on each of its properties. This complex initialization is really hard to read.
lgtm
https://codereview.chromium.org/2289143005/diff/1/components/offline_pages/cl... File components/offline_pages/client_policy_controller.h (right): https://codereview.chromium.org/2289143005/diff/1/components/offline_pages/cl... components/offline_pages/client_policy_controller.h:39: // Returns whether the policy for |name_space| is downloaded. On 2016/09/12 18:06:35, dewittj wrote: > This is worded strangely, does it mean that it should be shown in the download > home? Please see latest upload. I think it clears this up? https://codereview.chromium.org/2289143005/diff/40001/components/offline_page... File components/offline_pages/offline_page_client_policy.h (right): https://codereview.chromium.org/2289143005/diff/40001/components/offline_page... components/offline_pages/offline_page_client_policy.h:77: {false, true}}); On 2016/09/12 17:43:34, Dmitry Titov wrote: > could you add in-line comments here to say what those are? Done. https://codereview.chromium.org/2289143005/diff/40001/components/offline_page... components/offline_pages/offline_page_client_policy.h:104: OfflinePageClientPolicy policy_; On 2016/09/12 17:43:34, Dmitry Titov wrote: > DISALLOW_COPY_AND_ASSIGN... Done.
https://codereview.chromium.org/2289143005/diff/80001/components/offline_page... File components/offline_pages/offline_page_client_policy.h (right): https://codereview.chromium.org/2289143005/diff/80001/components/offline_page... components/offline_pages/offline_page_client_policy.h:32: LifetimeType lifetime_type; I might also assign defaults here and in page_limit, just so that it's obvious. Are you trying to keep this POD? If not then it might be useful to just define a constructor or two for these data types. https://codereview.chromium.org/2289143005/diff/80001/components/offline_page... components/offline_pages/offline_page_client_policy.h:73: policy_ = OfflinePageClientPolicy(); Is this line strictly necessary? I think it should already be a default policy object since it's not in the member initializer list
https://codereview.chromium.org/2289143005/diff/80001/components/offline_page... File components/offline_pages/offline_page_client_policy.h (right): https://codereview.chromium.org/2289143005/diff/80001/components/offline_page... components/offline_pages/offline_page_client_policy.h:32: LifetimeType lifetime_type; On 2016/09/12 21:40:53, dewittj wrote: > I might also assign defaults here and in page_limit, just so that it's obvious. > > Are you trying to keep this POD? If not then it might be useful to just define > a constructor or two for these data types. Done. https://codereview.chromium.org/2289143005/diff/80001/components/offline_page... components/offline_pages/offline_page_client_policy.h:73: policy_ = OfflinePageClientPolicy(); On 2016/09/12 21:40:53, dewittj wrote: > Is this line strictly necessary? I think it should already be a default policy > object since it's not in the member initializer list Done.
lgtm! https://codereview.chromium.org/2289143005/diff/100001/components/offline_pag... File components/offline_pages/offline_page_client_policy.h (right): https://codereview.chromium.org/2289143005/diff/100001/components/offline_pag... components/offline_pages/offline_page_client_policy.h:34: // The time after which the page expires. nit: please document that a TimeDelta of 0 means no expiration. https://codereview.chromium.org/2289143005/diff/100001/components/offline_pag... components/offline_pages/offline_page_client_policy.h:87: name_space(namespace_val), possible optimization: delegated constructors (https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/0zVA8Ctx3Xo)
Thanks! https://codereview.chromium.org/2289143005/diff/100001/components/offline_pag... File components/offline_pages/offline_page_client_policy.h (right): https://codereview.chromium.org/2289143005/diff/100001/components/offline_pag... components/offline_pages/offline_page_client_policy.h:34: // The time after which the page expires. On 2016/09/13 16:56:35, dewittj wrote: > nit: please document that a TimeDelta of 0 means no expiration. Done. https://codereview.chromium.org/2289143005/diff/100001/components/offline_pag... components/offline_pages/offline_page_client_policy.h:87: name_space(namespace_val), On 2016/09/13 16:56:35, dewittj wrote: > possible optimization: delegated constructors > (https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/0zVA8Ctx3Xo) Done.
The CQ bit was checked by chili@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dimich@chromium.org, fgorski@chromium.org, dewittj@chromium.org Link to the patchset: https://codereview.chromium.org/2289143005/#ps120001 (title: "final fixes to Justin's comments")
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by chili@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dewittj@chromium.org, dimich@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2289143005/#ps140001 (title: "fix compile issues in test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Offline pages] Add a builder and feature struct to policy BUG=641053 ========== to ========== [Offline pages] Add a builder and feature struct to policy BUG=641053 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [Offline pages] Add a builder and feature struct to policy BUG=641053 ========== to ========== [Offline pages] Add a builder and feature struct to policy BUG=641053 Committed: https://crrev.com/c7ae4807842f90c0808c2a6544b785ab51c4a151 Cr-Commit-Position: refs/heads/master@{#418330} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/c7ae4807842f90c0808c2a6544b785ab51c4a151 Cr-Commit-Position: refs/heads/master@{#418330} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
