Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2364253002:
[Offline Pages] Adds new policy bits and reverse lookup. (Closed)
|
Created:
4 years, 3 months ago by dewittj Modified:
4 years, 2 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] Adds new policy bits and reverse lookup.
This is part of the query API project to support looking up offline
pages by feature (which may contain multiple policies). Also removes
the namespace check found in OfflinePageUtils.
BUG=622763
Committed: https://crrev.com/7be8aa05df5cdfc13cb1b68dbbd9710080ff2059
Cr-Commit-Position: refs/heads/master@{#423912}
Patch Set 1 #Patch Set 2 : Add a test. #Patch Set 3 : Whitespace. #Patch Set 4 : Comments. #Patch Set 5 : Construction order. #
Total comments: 14
Patch Set 6 : Make const by marking caches mutable. #Patch Set 7 : Rename. #
Messages
Total messages: 32 (20 generated)
The CQ bit was checked by dewittj@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_...) linux_chromium_chromeos_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 dewittj@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 dewittj@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 dewittj@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...
dewittj@chromium.org changed reviewers: + chili@chromium.org, dimich@chromium.org
chili@ - PTAL dimich@ FYI, do you like the naming of the new policy bits?
The CQ bit was checked by dewittj@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...
lgtm with minor comments Thanks for doing this! https://codereview.chromium.org/2364253002/diff/80001/components/offline_page... File components/offline_pages/client_policy_controller.h (right): https://codereview.chromium.org/2364253002/diff/80001/components/offline_page... components/offline_pages/client_policy_controller.h:43: const std::vector<std::string>& GetNamespacesWithDownloadSupport(); should these methods be const as well? https://codereview.chromium.org/2364253002/diff/80001/components/offline_page... File components/offline_pages/offline_page_client_policy.h (right): https://codereview.chromium.org/2364253002/diff/80001/components/offline_page... components/offline_pages/offline_page_client_policy.h:63: is_removed_on_cache_reset(true){}; nit: since you're changing this anyway... should we add a space between the (true) and {}?
https://codereview.chromium.org/2364253002/diff/80001/components/offline_page... File components/offline_pages/client_policy_controller.h (right): https://codereview.chromium.org/2364253002/diff/80001/components/offline_page... components/offline_pages/client_policy_controller.h:43: const std::vector<std::string>& GetNamespacesWithDownloadSupport(); On 2016/09/23 23:52:43, chili wrote: > should these methods be const as well? Because they are memoizing they are not actually const. https://codereview.chromium.org/2364253002/diff/80001/components/offline_page... File components/offline_pages/offline_page_client_policy.h (right): https://codereview.chromium.org/2364253002/diff/80001/components/offline_page... components/offline_pages/offline_page_client_policy.h:63: is_removed_on_cache_reset(true){}; On 2016/09/23 23:52:43, chili wrote: > nit: since you're changing this anyway... should we add a space between the > (true) and {}? Interestingly, git cl format removes the space. I don't like going against git cl format. The other option is to move the constructor out-of-line. Which at this point might be worthwhile. Other option is to just use default values inline. Do you have a preference among the 3?
On 2016/09/23 23:55:55, dewittj wrote: > https://codereview.chromium.org/2364253002/diff/80001/components/offline_page... > File components/offline_pages/client_policy_controller.h (right): > > https://codereview.chromium.org/2364253002/diff/80001/components/offline_page... > components/offline_pages/client_policy_controller.h:43: const > std::vector<std::string>& GetNamespacesWithDownloadSupport(); > On 2016/09/23 23:52:43, chili wrote: > > should these methods be const as well? > > Because they are memoizing they are not actually const. I could mark the cache members 'mutable'...
still lgtm https://codereview.chromium.org/2364253002/diff/80001/components/offline_page... File components/offline_pages/client_policy_controller.h (right): https://codereview.chromium.org/2364253002/diff/80001/components/offline_page... components/offline_pages/client_policy_controller.h:43: const std::vector<std::string>& GetNamespacesWithDownloadSupport(); On 2016/09/23 23:55:55, dewittj wrote: > On 2016/09/23 23:52:43, chili wrote: > > should these methods be const as well? > > Because they are memoizing they are not actually const. Ah oops forgot about the memoizing bit. darn ^_^ https://codereview.chromium.org/2364253002/diff/80001/components/offline_page... File components/offline_pages/offline_page_client_policy.h (right): https://codereview.chromium.org/2364253002/diff/80001/components/offline_page... components/offline_pages/offline_page_client_policy.h:63: is_removed_on_cache_reset(true){}; On 2016/09/23 23:55:55, dewittj wrote: > On 2016/09/23 23:52:43, chili wrote: > > nit: since you're changing this anyway... should we add a space between the > > (true) and {}? > > Interestingly, git cl format removes the space. I don't like going against git > cl format. The other option is to move the constructor out-of-line. Which at > this point might be worthwhile. > > Other option is to just use default values inline. Do you have a preference > among the 3? I'm ok keeping it as is. Didn't know that git cl format would remove the space :D TIL
When I thought about it, the caches should be invisible to the outside so it's a good use case for "mutable".
https://codereview.chromium.org/2364253002/diff/80001/components/offline_page... File components/offline_pages/client_policy_controller.cc (right): https://codereview.chromium.org/2364253002/diff/80001/components/offline_page... components/offline_pages/client_policy_controller.cc:29: .SetIsSupportedByRecentTabs(true) This set of bits looks conflicting? If we only can show the page in original tab, it makes less sense to show it in some list of recently visited sites, because it implies that user cna click on items in that list to re-open the snapshots, and we can't do that once the tab is navigated/closed... https://codereview.chromium.org/2364253002/diff/80001/components/offline_page... File components/offline_pages/client_policy_controller.h (right): https://codereview.chromium.org/2364253002/diff/80001/components/offline_page... components/offline_pages/client_policy_controller.h:43: const std::vector<std::string>& GetNamespacesWithDownloadSupport(); I think naming-wise, those "get namespaces" methods should follow the "isFoo" methods, for example in this case, GetNamespacesWithDownloadSupport -> GetNamspacesSupportedByDownload https://codereview.chromium.org/2364253002/diff/80001/components/offline_page... components/offline_pages/client_policy_controller.h:47: bool IsRecentTab(const std::string& name_space) const; What is this feature? Can't understand from the comment or the name... Is this "recently visited sites" on NTP? If so, then maybe call it IsShownAsRecentlyVisitedSite ? https://codereview.chromium.org/2364253002/diff/80001/components/offline_page... components/offline_pages/client_policy_controller.h:52: bool ShouldOnlyBeShownInOriginalTab(const std::string& name_space) const; Naming suggestion: IsRestrictedToOriginalTab
dimich: ptal, thanks! https://codereview.chromium.org/2364253002/diff/80001/components/offline_page... File components/offline_pages/client_policy_controller.cc (right): https://codereview.chromium.org/2364253002/diff/80001/components/offline_page... components/offline_pages/client_policy_controller.cc:29: .SetIsSupportedByRecentTabs(true) On 2016/09/24 00:09:29, Dmitry Titov wrote: > This set of bits looks conflicting? If we only can show the page in original > tab, it makes less sense to show it in some list of recently visited sites, > because it implies that user cna click on items in that list to re-open the > snapshots, and we can't do that once the tab is navigated/closed... These are not strictly conflicting. Imagine if we ship last-n = n; then that would be supported by recent tabs, but not shown in original tab. I think NTP plans to get the wiring set up for switching to a tab that has an offline copy. Something like iterating the current list of tabs and intersecting with the recent tabs list by tab ID https://codereview.chromium.org/2364253002/diff/80001/components/offline_page... File components/offline_pages/client_policy_controller.h (right): https://codereview.chromium.org/2364253002/diff/80001/components/offline_page... components/offline_pages/client_policy_controller.h:43: const std::vector<std::string>& GetNamespacesWithDownloadSupport(); On 2016/09/24 00:09:29, Dmitry Titov wrote: > I think naming-wise, those "get namespaces" methods should follow the "isFoo" > methods, for example in this case, > GetNamespacesWithDownloadSupport -> GetNamspacesSupportedByDownload Done. https://codereview.chromium.org/2364253002/diff/80001/components/offline_page... components/offline_pages/client_policy_controller.h:47: bool IsRecentTab(const std::string& name_space) const; On 2016/09/24 00:09:29, Dmitry Titov wrote: > What is this feature? Can't understand from the comment or the name... Is this > "recently visited sites" on NTP? If so, then maybe call it > IsShownAsRecentlyVisitedSite ? Done. https://codereview.chromium.org/2364253002/diff/80001/components/offline_page... components/offline_pages/client_policy_controller.h:52: bool ShouldOnlyBeShownInOriginalTab(const std::string& name_space) const; On 2016/09/24 00:09:29, Dmitry Titov wrote: > Naming suggestion: IsRestrictedToOriginalTab Done.
The CQ bit was checked by dewittj@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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
ping
The CQ bit was checked by dewittj@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chili@chromium.org Link to the patchset: https://codereview.chromium.org/2364253002/#ps120001 (title: "Rename.")
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.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Adds new policy bits and reverse lookup. This is part of the query API project to support looking up offline pages by feature (which may contain multiple policies). Also removes the namespace check found in OfflinePageUtils. BUG=622763 ========== to ========== [Offline Pages] Adds new policy bits and reverse lookup. This is part of the query API project to support looking up offline pages by feature (which may contain multiple policies). Also removes the namespace check found in OfflinePageUtils. BUG=622763 Committed: https://crrev.com/7be8aa05df5cdfc13cb1b68dbbd9710080ff2059 Cr-Commit-Position: refs/heads/master@{#423912} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/7be8aa05df5cdfc13cb1b68dbbd9710080ff2059 Cr-Commit-Position: refs/heads/master@{#423912} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
