Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2415473003:
Query API: Introduces an OfflinePageModelQuery object. (Closed)
|
Created:
4 years, 2 months ago by dewittj Modified:
4 years, 1 month 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, chili Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionQuery API: Introduces an OfflinePageModelQuery object.
Replaces most query operations within OfflinePageModelImpl with query-based versions.
BUG=622763
Committed: https://crrev.com/cc2f7d0c5c3ab42762d9e88ef1f4b293b03b6215
Cr-Commit-Position: refs/heads/master@{#428396}
Patch Set 1 #Patch Set 2 : Stop making a new client policy controller, have a dangling pointer. #
Total comments: 8
Patch Set 3 : Add tests and address comments. #
Total comments: 8
Patch Set 4 : Moves everything to Requirement-based matching. #
Total comments: 6
Patch Set 5 : Updated re comments. #Patch Set 6 : Address comments, add more tests, rename enum. #
Total comments: 41
Patch Set 7 : address comments. #Patch Set 8 : Address more comments. #Messages
Total messages: 54 (37 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: 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 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...
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== Query API: Introduces an OfflinePageModelQuery object. Initial implementation replaces GetAllPages with a generic query. BUG=622763 ========== to ========== Query API: Introduces an OfflinePageModelQuery object. Initial implementation replaces GetAllPages with a generic query. BUG=622763 ==========
dewittj@chromium.org changed reviewers: + dimich@chromium.org
Description was changed from ========== Query API: Introduces an OfflinePageModelQuery object. Initial implementation replaces GetAllPages with a generic query. BUG=622763 ========== to ========== Query API: Introduces an OfflinePageModelQuery object. Replaces most query operations within OfflinePageModelImpl with query-based versions. BUG=622763 ==========
ptal, I know I need more tests in the query object but could you give this a once-over? Some dubious things are: * Just having a dangling ClientPolicyController in the Builder object. This seems marginally OK because the lifetime of Builder objects is typically very short. Other options include: - hold a pointer to an OfflinePageModel and observe it for shutdown (or hold a weak pointer) - have OfflinePageModel vend weak pointers to the policy object * The support for negatives in the namespace precludes using unknown namespaces in test. Is that ok? Thoughts?
dewittj@chromium.org changed reviewers: + fgorski@chromium.org
https://codereview.chromium.org/2415473003/diff/80001/components/offline_page... File components/offline_pages/client_policy_controller.h (right): https://codereview.chromium.org/2415473003/diff/80001/components/offline_page... components/offline_pages/client_policy_controller.h:39: std::vector<std::string> GetAllNamespacesExcept( It's a weird method to have here... Why not "GetAllNamespaces"? https://codereview.chromium.org/2415473003/diff/80001/components/offline_page... File components/offline_pages/offline_page_model_query.h (right): https://codereview.chromium.org/2415473003/diff/80001/components/offline_page... components/offline_pages/offline_page_model_query.h:59: // multiple times, takes the intersection of the IDs in all the requests. Intersection is probably the least intuitive result of SetFoo kind of function. The name implies replacing of the set. If you want the union semantics, it probably needs to be called AddAllowedOfflineIds... If you want intersection, the naming can be hard. Maybe "LimitToOfflineIds()"? Which brings a question, why would you want an intersection? https://codereview.chromium.org/2415473003/diff/80001/components/offline_page... components/offline_pages/offline_page_model_query.h:65: OfflinePageModelQueryBuilder& SetClientIds(const std::vector<ClientId>& ids); Same here. I think SetClientIds should simply replace the previous set, not compute accumulated resulting set from previous calls... https://codereview.chromium.org/2415473003/diff/80001/components/offline_page... components/offline_pages/offline_page_model_query.h:74: // a recently visited site. I think for policy bits they (policy bits) should be directly referred to here, since the comments may diverge with time. Something like "Only include pages whose namespaces satisfy ClientPolicyController::IsShownAsRecentlyVisitedSite() test"...
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...
https://codereview.chromium.org/2415473003/diff/80001/components/offline_page... File components/offline_pages/client_policy_controller.h (right): https://codereview.chromium.org/2415473003/diff/80001/components/offline_page... components/offline_pages/client_policy_controller.h:39: std::vector<std::string> GetAllNamespacesExcept( On 2016/10/20 19:40:53, Dmitry Titov wrote: > It's a weird method to have here... Why not "GetAllNamespaces"? Can do. https://codereview.chromium.org/2415473003/diff/80001/components/offline_page... File components/offline_pages/offline_page_model_query.h (right): https://codereview.chromium.org/2415473003/diff/80001/components/offline_page... components/offline_pages/offline_page_model_query.h:59: // multiple times, takes the intersection of the IDs in all the requests. On 2016/10/20 19:40:54, Dmitry Titov wrote: > Intersection is probably the least intuitive result of SetFoo kind of function. > The name implies replacing of the set. If you want the union semantics, it > probably needs to be called AddAllowedOfflineIds... If you want intersection, > the naming can be hard. Maybe "LimitToOfflineIds()"? Which brings a question, > why would you want an intersection? Intersection seemed useful if you wanted different parts of the system to add different constraints to a Builder. Also it matches better the idea of the namespace calls below, which each intersect namespaces. Perhaps it's just best to DCHECK if the restriction is already set? https://codereview.chromium.org/2415473003/diff/80001/components/offline_page... components/offline_pages/offline_page_model_query.h:65: OfflinePageModelQueryBuilder& SetClientIds(const std::vector<ClientId>& ids); On 2016/10/20 19:40:54, Dmitry Titov wrote: > Same here. I think SetClientIds should simply replace the previous set, not > compute accumulated resulting set from previous calls... Done. https://codereview.chromium.org/2415473003/diff/80001/components/offline_page... components/offline_pages/offline_page_model_query.h:74: // a recently visited site. On 2016/10/20 19:40:54, Dmitry Titov wrote: > I think for policy bits they (policy bits) should be directly referred to here, > since the comments may diverge with time. > > Something like "Only include pages whose namespaces satisfy > ClientPolicyController::IsShownAsRecentlyVisitedSite() test"... Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2415473003/diff/100001/components/offline_pag... File components/offline_pages/offline_page_model_query.h (right): https://codereview.chromium.org/2415473003/diff/100001/components/offline_pag... components/offline_pages/offline_page_model_query.h:52: Unset, Enum values should be either CAPITALIZED or kNamed: https://google.github.io/styleguide/cppguide.html#Enumerator_Names https://codereview.chromium.org/2415473003/diff/100001/components/offline_pag... components/offline_pages/offline_page_model_query.h:57: explicit OfflinePageModelQueryBuilder(); doesn't have to be explicit https://codereview.chromium.org/2415473003/diff/100001/components/offline_pag... components/offline_pages/offline_page_model_query.h:62: OfflinePageModelQueryBuilder& SetOfflinePageIds( I wonder if those "Set" methods can also take Requirement param to specify inclusion/exclusion/indifferent... Then all the methods can share common naming pattern, for example SelectWhereClientIds(ids, Requirement::Matching) SelectWhereSupportedByDownload(Requirement::Matching) https://codereview.chromium.org/2415473003/diff/100001/components/offline_pag... components/offline_pages/offline_page_model_query.h:107: void IntersectWithNamespaces(ClientPolicyController* controller, If I understand correctly, "intersect with inversion" is basically "remove specified namespaces". Maybe simply add a second method that would be named "RemoveNamespaces"? It might produce a bit larger but hopefully more readable code in the impl as well...
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...
https://codereview.chromium.org/2415473003/diff/100001/components/offline_pag... File components/offline_pages/offline_page_model_query.h (right): https://codereview.chromium.org/2415473003/diff/100001/components/offline_pag... components/offline_pages/offline_page_model_query.h:52: Unset, On 2016/10/21 20:21:09, Dmitry Titov wrote: > Enum values should be either CAPITALIZED or kNamed: > https://google.github.io/styleguide/cppguide.html#Enumerator_Names Done. https://codereview.chromium.org/2415473003/diff/100001/components/offline_pag... components/offline_pages/offline_page_model_query.h:57: explicit OfflinePageModelQueryBuilder(); On 2016/10/21 20:21:09, Dmitry Titov wrote: > doesn't have to be explicit Done. https://codereview.chromium.org/2415473003/diff/100001/components/offline_pag... components/offline_pages/offline_page_model_query.h:62: OfflinePageModelQueryBuilder& SetOfflinePageIds( On 2016/10/21 20:21:09, Dmitry Titov wrote: > I wonder if those "Set" methods can also take Requirement param to specify > inclusion/exclusion/indifferent... > > Then all the methods can share common naming pattern, for example > SelectWhereClientIds(ids, Requirement::Matching) > SelectWhereSupportedByDownload(Requirement::Matching) Done. What do you think? If you like the new shape, I'll add tests for the EXCLUDE side of these new features. https://codereview.chromium.org/2415473003/diff/100001/components/offline_pag... components/offline_pages/offline_page_model_query.h:107: void IntersectWithNamespaces(ClientPolicyController* controller, On 2016/10/21 20:21:09, Dmitry Titov wrote: > If I understand correctly, "intersect with inversion" is basically "remove > specified namespaces". Maybe simply add a second method that would be named > "RemoveNamespaces"? It might produce a bit larger but hopefully more readable > code in the impl as well... Moved to RemoveNamespaces & IntersectNamespaces. Seems cleaner now, wdyt?
almost there: https://codereview.chromium.org/2415473003/diff/120001/components/offline_pag... File components/offline_pages/offline_page_model_query.h (right): https://codereview.chromium.org/2415473003/diff/120001/components/offline_pag... components/offline_pages/offline_page_model_query.h:23: MATCH, INCLUDE_MATCHING/EXCLUDE_MATCHING ? https://codereview.chromium.org/2415473003/diff/120001/components/offline_pag... components/offline_pages/offline_page_model_query.h:55: // Used to create an offline page model query. Set policy bits to limit by "Set policy bits to limit by namespaces" is unclear. Also, it looks like an empty query includes everything except expired pages, and the additional calls are all reducing the set, filtering it, correct? If that is how you want it, it makes sense to document this way, it's a clear model - query starts as all-inclusive, you specify a filter on it. https://codereview.chromium.org/2415473003/diff/120001/components/offline_pag... components/offline_pages/offline_page_model_query.h:57: // zero, the builder will DCHECK. Call |Build| to generate an actual query. Not sure why would Builder DCHECK... This seems like a possible code flow to restrict query to the point it returns nothing.
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...
thanks! ptal https://codereview.chromium.org/2415473003/diff/120001/components/offline_pag... File components/offline_pages/offline_page_model_query.h (right): https://codereview.chromium.org/2415473003/diff/120001/components/offline_pag... components/offline_pages/offline_page_model_query.h:23: MATCH, On 2016/10/24 18:28:34, Dmitry Titov wrote: > INCLUDE_MATCHING/EXCLUDE_MATCHING ? Done. https://codereview.chromium.org/2415473003/diff/120001/components/offline_pag... components/offline_pages/offline_page_model_query.h:55: // Used to create an offline page model query. Set policy bits to limit by On 2016/10/24 18:28:34, Dmitry Titov wrote: > "Set policy bits to limit by namespaces" is unclear. > > Also, it looks like an empty query includes everything except expired pages, and > the additional calls are all reducing the set, filtering it, correct? If that is > how you want it, it makes sense to document this way, it's a clear model - query > starts as all-inclusive, you specify a filter on it. Done. https://codereview.chromium.org/2415473003/diff/120001/components/offline_pag... components/offline_pages/offline_page_model_query.h:57: // zero, the builder will DCHECK. Call |Build| to generate an actual query. On 2016/10/24 18:28:34, Dmitry Titov wrote: > Not sure why would Builder DCHECK... This seems like a possible code flow to > restrict query to the point it returns nothing. This was outdated anyway. The whole comment is now rewritten.
lgtm
+chili@ as FYI for policy controller changes and usage.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
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: This issue passed the CQ dry run.
Partial review. I am going through query model now. Will take some more time. https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... File components/offline_pages/client_policy_controller.cc (right): https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/client_policy_controller.cc:7: #include <set> remove? https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... File components/offline_pages/client_policy_controller.h (right): https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/client_policy_controller.h:59: void AddPolicyForTest(std::string name_space, why not const ref ? https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... File components/offline_pages/offline_page_model.h (right): https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/offline_page_model.h:18: #include "components/offline_pages/offline_page_model_query.h" is this necessary? does this file really depend on the model query? https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... File components/offline_pages/offline_page_model_impl.cc (right): https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/offline_page_model_impl.cc:519: // TODO(dewittj): some sort of signalling here? since this is private, perhaps a DCHECK instead? https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/offline_page_model_impl.cc:528: if (query->GetRestrictedToOfflineIds(&offline_ids) == Is there a chance we drop this first clause in the if? And the method we call here? I don't find it to be more readable, and the else clause already does everything we need. (even if it is not optimized). https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... File components/offline_pages/offline_page_model_impl.h (right): https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/offline_page_model_impl.h:123: enum class GetAllPageMode { remove? https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... File components/offline_pages/offline_page_model_impl_unittest.cc (right): https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/offline_page_model_impl_unittest.cc:891: ASSERT_NE(nullptr, page); shouldn't comparisons to nullptr be combined with get() ? ASSERT_TRUE(page); is more appropriate here and below I think. https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... File components/offline_pages/offline_page_model_query.cc (right): https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/offline_page_model_query.cc:98: ClientPolicyController* controller) { dcheck controller https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/offline_page_model_query.cc:114: std::vector<std::string> allowed_namespaces = controller->GetAllNamespaces(); this can wait until you know you have something in op_list https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... File components/offline_pages/offline_page_model_query.h (right): https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/offline_page_model_query.h:21: enum class Requirement { this almost wants to be called constraint. What do you think? Also, could you please document it? https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/offline_page_model_query.h:32: Requirement GetRestrictedToOfflineIds(std::set<int64_t>* ids_out); This pattern would work for a status variable, but it doesn't seem like the right thing with this types. Could this be either a pair or both values returned through out params? I know this is convenient for the if statement, but it does not feel right. https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/offline_page_model_query.h:39: bool Matches(const OfflinePageItem& page); Now that I think about that. Everything between destructor and this method feels odd. Why do we need getters on the query if we have a Match? https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/offline_page_model_query.h:44: bool allow_expired_ = false; constructor should take care of that I believe. https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/offline_page_model_query.h:46: std::unique_ptr<std::set<std::string>> restricted_to_namespaces_; I wonder if we can apply this to eliminate last_n restriction. https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/offline_page_model_query.h:59: // that can be used to specify whether the input restriction should match all perhaps: should include or exclude matching pages? https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/offline_page_model_query.h:74: // Sets the client IDs that are valid for this request. If called // multiple remove "// " before multiple. https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/offline_page_model_query.h:84: // Only include namespaces whose namespaces satisfy If I am not mistaken you meant: Only include *pages* whose namespaces satisfy? This applies to documentation below. https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/offline_page_model_query.h:109: // Returns the built-up query based on the above APIs. This resets the Builds the query using the provided policy |controller|. Resets the internal state of the builder. |controller| should not be nullptr.
Extra comment we discussed. https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... File components/offline_pages/offline_page_model_query.cc (right): https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/offline_page_model_query.cc:141: case Requirement::INCLUDE_MATCHING: suggestion split in 2 ifs. Please check if that passes the tests. (It would remove quite a bit of code I think.) std::vector<std::string> allowed_namespaces; bool uses_namespace_restrictions = false; for (auto& name_space : controller->GetAllNamespaces()) { // handle exclusions. if ((supported_by_download_ == Requirement::EXCLUDE_MATCHING && controller->IsSupportedByDownload(name_space)) || (rencent_tab_namespace_cache_ == Requirement::EXCLUDE_MATCHING && controller->IsShownAsRecentlyVisitedSite(name_space)) || (restricted_to_original_tab_ == Requirement::EXCLUDE_MATCHING && controller->IsRestrictedToOriginalTab(name_space))) { // Skip namespaces that meet exclusion requirements. uses_namespace_restrictions = true; continue; } if ((supported_by_download_ == Requirement::INCLUDE_MATCHING && !controller->IsSupportedByDownload(name_space)) || (rencent_tab_namespace_cache_ == Requirement::INCLUDE_MATCHING && !controller->IsShownAsRecentlyVisitedSite(name_space)) || (restricted_to_original_tab_ == Requirement::INCLUDE_MATCHING && !controller->IsRestrictedToOriginalTab(name_space))) { // Skip namespaces that don't meet inclusion requirement. uses_namespace_restrictions = true; continue; } // Add namespace otherwise. allowed_namespaces.emplace_back(name_space); }
https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... File components/offline_pages/client_policy_controller.cc (right): https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/client_policy_controller.cc:7: #include <set> On 2016/10/26 18:05:41, fgorski wrote: > remove? Done. https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... File components/offline_pages/client_policy_controller.h (right): https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/client_policy_controller.h:59: void AddPolicyForTest(std::string name_space, On 2016/10/26 18:05:41, fgorski wrote: > why not const ref ? Done. In my experience, strings were not required to be const ref. However, that is just a memory and definitely deviates from common style in this file. https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... File components/offline_pages/offline_page_model.h (right): https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/offline_page_model.h:18: #include "components/offline_pages/offline_page_model_query.h" On 2016/10/26 18:05:41, fgorski wrote: > is this necessary? does this file really depend on the model query? Done. No, remnant from previous iteration. https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... File components/offline_pages/offline_page_model_impl.cc (right): https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/offline_page_model_impl.cc:519: // TODO(dewittj): some sort of signalling here? On 2016/10/26 18:05:41, fgorski wrote: > since this is private, perhaps a DCHECK instead? Done. https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/offline_page_model_impl.cc:528: if (query->GetRestrictedToOfflineIds(&offline_ids) == On 2016/10/26 18:05:41, fgorski wrote: > Is there a chance we drop this first clause in the if? And the method we call > here? > > I don't find it to be more readable, and the else clause already does everything > we need. (even if it is not optimized). Done. https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... File components/offline_pages/offline_page_model_impl.h (right): https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/offline_page_model_impl.h:123: enum class GetAllPageMode { On 2016/10/26 18:05:41, fgorski wrote: > remove? Done. https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... File components/offline_pages/offline_page_model_impl_unittest.cc (right): https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/offline_page_model_impl_unittest.cc:891: ASSERT_NE(nullptr, page); On 2016/10/26 18:05:41, fgorski wrote: > shouldn't comparisons to nullptr be combined with get() ? > > ASSERT_TRUE(page); is more appropriate here and below I think. http://en.cppreference.com/w/cpp/memory/unique_ptr/operator_cmp std::unique_ptr implements comparisons with nullptr. But I agree that ASSERT_TRUE is better looking. https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... File components/offline_pages/offline_page_model_query.cc (right): https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/offline_page_model_query.cc:98: ClientPolicyController* controller) { On 2016/10/26 18:05:42, fgorski wrote: > dcheck controller Done. https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/offline_page_model_query.cc:114: std::vector<std::string> allowed_namespaces = controller->GetAllNamespaces(); On 2016/10/26 18:05:42, fgorski wrote: > this can wait until you know you have something in op_list Acknowledged. https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/offline_page_model_query.cc:141: case Requirement::INCLUDE_MATCHING: On 2016/10/26 18:46:02, fgorski wrote: > suggestion split in 2 ifs. Please check if that passes the tests. (It would > remove quite a bit of code I think.) > > std::vector<std::string> allowed_namespaces; > bool uses_namespace_restrictions = false; > > for (auto& name_space : controller->GetAllNamespaces()) { > // handle exclusions. > if ((supported_by_download_ == Requirement::EXCLUDE_MATCHING && > controller->IsSupportedByDownload(name_space)) || > (rencent_tab_namespace_cache_ == Requirement::EXCLUDE_MATCHING && > controller->IsShownAsRecentlyVisitedSite(name_space)) || > (restricted_to_original_tab_ == Requirement::EXCLUDE_MATCHING && > controller->IsRestrictedToOriginalTab(name_space))) { > // Skip namespaces that meet exclusion requirements. > uses_namespace_restrictions = true; > continue; > } > > if ((supported_by_download_ == Requirement::INCLUDE_MATCHING && > !controller->IsSupportedByDownload(name_space)) || > (rencent_tab_namespace_cache_ == Requirement::INCLUDE_MATCHING && > !controller->IsShownAsRecentlyVisitedSite(name_space)) || > (restricted_to_original_tab_ == Requirement::INCLUDE_MATCHING && > !controller->IsRestrictedToOriginalTab(name_space))) { > // Skip namespaces that don't meet inclusion requirement. > uses_namespace_restrictions = true; > continue; > } > > // Add namespace otherwise. > allowed_namespaces.emplace_back(name_space); > } Done. I like that better, thanks! https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... File components/offline_pages/offline_page_model_query.h (right): https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/offline_page_model_query.h:21: enum class Requirement { On 2016/10/26 18:05:42, fgorski wrote: > this almost wants to be called constraint. > What do you think? > > Also, could you please document it? Could be, I like requirement but if you feel strongly I will change it. Docs done. https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/offline_page_model_query.h:32: Requirement GetRestrictedToOfflineIds(std::set<int64_t>* ids_out); On 2016/10/26 18:05:42, fgorski wrote: > This pattern would work for a status variable, but it doesn't seem like the > right thing with this types. > > Could this be either a pair or both values returned through out params? > > I know this is convenient for the if statement, but it does not feel right. Done. https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/offline_page_model_query.h:39: bool Matches(const OfflinePageItem& page); On 2016/10/26 18:05:42, fgorski wrote: > Now that I think about that. Everything between destructor and this method feels > odd. Why do we need getters on the query if we have a Match? Match is not going to be usable when we move to a SQL-Query based model. We already are using the offline ID restriction to optimize the executequery method. https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/offline_page_model_query.h:44: bool allow_expired_ = false; On 2016/10/26 18:05:42, fgorski wrote: > constructor should take care of that I believe. This is in the C++11 Allowed Features section of https://chromium-cpp.appspot.com/ https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/offline_page_model_query.h:46: std::unique_ptr<std::set<std::string>> restricted_to_namespaces_; On 2016/10/26 18:05:42, fgorski wrote: > I wonder if we can apply this to eliminate last_n restriction. I'm not sure what you mean, can you elaborate? https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/offline_page_model_query.h:59: // that can be used to specify whether the input restriction should match all On 2016/10/26 18:05:42, fgorski wrote: > perhaps: should include or exclude matching pages? Done. https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/offline_page_model_query.h:74: // Sets the client IDs that are valid for this request. If called // multiple On 2016/10/26 18:05:42, fgorski wrote: > remove "// " before multiple. Done. https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/offline_page_model_query.h:84: // Only include namespaces whose namespaces satisfy On 2016/10/26 18:05:42, fgorski wrote: > If I am not mistaken you meant: > Only include *pages* whose namespaces satisfy? > > This applies to documentation below. Done. https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/offline_page_model_query.h:109: // Returns the built-up query based on the above APIs. This resets the On 2016/10/26 18:05:42, fgorski wrote: > Builds the query using the provided policy |controller|. > Resets the internal state of the builder. |controller| should not be nullptr. 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: This issue passed the CQ dry run.
lgtm. Good work. https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... File components/offline_pages/offline_page_model_impl_unittest.cc (right): https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/offline_page_model_impl_unittest.cc:891: ASSERT_NE(nullptr, page); On 2016/10/27 22:49:17, dewittj wrote: > On 2016/10/26 18:05:41, fgorski wrote: > > shouldn't comparisons to nullptr be combined with get() ? > > > > ASSERT_TRUE(page); is more appropriate here and below I think. > http://en.cppreference.com/w/cpp/memory/unique_ptr/operator_cmp > > std::unique_ptr implements comparisons with nullptr. > > But I agree that ASSERT_TRUE is better looking. OK, I was looking for that an couldn't find. Had I known earlier I wouldn't complain, but I am glad you like the alternative. https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... File components/offline_pages/offline_page_model_query.h (right): https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/offline_page_model_query.h:44: bool allow_expired_ = false; On 2016/10/27 22:49:18, dewittj wrote: > On 2016/10/26 18:05:42, fgorski wrote: > > constructor should take care of that I believe. > > This is in the C++11 Allowed Features section of > https://chromium-cpp.appspot.com/ Acknowledged. https://codereview.chromium.org/2415473003/diff/160001/components/offline_pag... components/offline_pages/offline_page_model_query.h:46: std::unique_ptr<std::set<std::string>> restricted_to_namespaces_; On 2016/10/27 22:49:18, dewittj wrote: > On 2016/10/26 18:05:42, fgorski wrote: > > I wonder if we can apply this to eliminate last_n restriction. > > I'm not sure what you mean, can you elaborate? Nerer mind, I realized that the policy is written in a very special way...
The CQ bit was checked by dewittj@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dimich@chromium.org Link to the patchset: https://codereview.chromium.org/2415473003/#ps190001 (title: "Address more 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
Failed to apply the patch. On branch working_branch Your branch is up-to-date with 'origin/refs/pending/heads/master'. nothing to commit, working tree clean
On 2016/10/28 17:03:09, commit-bot: I haz the power wrote: > Failed to apply the patch. > On branch working_branch > Your branch is up-to-date with 'origin/refs/pending/heads/master'. > nothing to commit, working tree clean Commit-bot went a little crazy, but successfully landed PS 8.
Message was sent while issue was closed.
Description was changed from ========== Query API: Introduces an OfflinePageModelQuery object. Replaces most query operations within OfflinePageModelImpl with query-based versions. BUG=622763 ========== to ========== Query API: Introduces an OfflinePageModelQuery object. Replaces most query operations within OfflinePageModelImpl with query-based versions. BUG=622763 Committed: https://crrev.com/cc2f7d0c5c3ab42762d9e88ef1f4b293b03b6215 Cr-Commit-Position: refs/heads/master@{#428396} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/cc2f7d0c5c3ab42762d9e88ef1f4b293b03b6215 Cr-Commit-Position: refs/heads/master@{#428396} |
