Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2202113002:
API to provide status of save page reqeusts to API. (Closed)
|
Created:
4 years, 4 months ago by Pete Williamson Modified:
4 years, 4 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 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAPI to provide status of save page reqeusts to API.
For components to drive background offlining programmatically, this
provides a new status API to check the current status of offline
page requests.
BUG=610521
Committed: https://crrev.com/7b2bb2f29d275938d82f478128dc7154c6dc47a8
Cr-Commit-Position: refs/heads/master@{#409828}
Patch Set 1 #
Total comments: 39
Patch Set 2 : CR fixes per DougArnett and FGorski. Also gets rid of status. #
Total comments: 14
Patch Set 3 : CR changes per FGorski #Patch Set 4 : Merge with delete req delete request change. #Patch Set 5 : Clean up naming #
Messages
Total messages: 20 (8 generated)
The CQ bit was checked by petewil@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...
petewil@chromium.org changed reviewers: + dougarnett@chromium.org, fgorski@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:92: for(auto iter = requests.begin(); iter != requests.end(); ++iter) { for (const auto& request : requests) is the simplest allowed form I believe, per: https://chromium-cpp.appspot.com/ (Range-Based For Loops section) https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:95: iter->GetStatus(base::Time::Now())); consider extracting the time to a variable. Also, is there a chance to use Timer instead of base::Time::Now()? That would simplify testing. https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:101: nit: remove extra line https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.h:63: std::string client_namespace, RequestStatusCallback callback); const&, const& https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.h:113: std::string client_namespace, const& https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator_unittest.cc:551: request1, // TODO: This is a bug, should be request2, fix it later. nit: TODO(whom) https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator_unittest.cc:618: nit: remove empty line Question: Do you intend to verify the second request some other time, or is this an omission? (please add a TODO in former case, or fix the code in the latter.) https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_queue.cc (right): https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_queue.cc:20: DVLOG(0) << "@@@@@@ " << __func__; nit: do you need to keep these? https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_queue_in_memory_store.cc (right): https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_queue_in_memory_store.cc:21: for (const auto& id_request_pair : requests_) { nit: do you need the braces here? https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_queue_store_unittest.cc (right): https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_queue_store_unittest.cc:117: DVLOG(0) << "@@@@@@ " << __func__; nit: do you need to keep this? https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_status.h (right): https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_status.h:15: std::string client_id; why not a ClientId? you will get partial information from this and this approach precludes the struct from being used in a more generic queries. https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_status.h:19: RequestStatus(std::string id, SavePageRequest::Status request_status) const & for id. https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_status.h:22: }; nit: git cl format, which will solve a few issues in this file. https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_status.h:25: nit: remove extra line.
https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.h:118: // Gets the result of add requests to the request queue. Maybe match the previous doc style here and next one: // Receives ... https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.h:122: // Gets the result of update and delete reqeusts to the request queue. requests https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_status.h (right): https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_status.h:16: SavePageRequest::Status status; Later, we may end up wanting to move this Status enum out of SavePageRequest. https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_status.h:17: // TODO(petewil): We can easily add the URL, would it help? Seems like a good thing to include although don't have strong use case (might depend on how ClientIds are constructed by client (if they have any real information or not))
https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:92: for(auto iter = requests.begin(); iter != requests.end(); ++iter) { On 2016/08/02 04:18:11, fgorski wrote: > for (const auto& request : requests) is the simplest allowed form I believe, > per: > https://chromium-cpp.appspot.com/ (Range-Based For Loops section) Right, find_if would take a lambda and the collection, and search for matching items. I'm wondering if the lambda version introduces more or less cognitive loading for the reader (ie, is it more or less readable?) https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:95: iter->GetStatus(base::Time::Now())); On 2016/08/02 04:18:12, fgorski wrote: > consider extracting the time to a variable. > Also, is there a chance to use Timer instead of base::Time::Now()? That would > simplify testing. Removed this code instead, we don't need a time anymore. https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:101: On 2016/08/02 04:18:11, fgorski wrote: > nit: remove extra line Done. https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.h:63: std::string client_namespace, RequestStatusCallback callback); On 2016/08/02 04:18:12, fgorski wrote: > const&, const& Done. https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.h:113: std::string client_namespace, On 2016/08/02 04:18:12, fgorski wrote: > const& Done. https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.h:118: // Gets the result of add requests to the request queue. On 2016/08/02 20:39:28, dougarnett wrote: > Maybe match the previous doc style here and next one: > > // Receives ... Done. https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.h:122: // Gets the result of update and delete reqeusts to the request queue. On 2016/08/02 20:39:28, dougarnett wrote: > requests Done. https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator_unittest.cc:551: request1, // TODO: This is a bug, should be request2, fix it later. On 2016/08/02 04:18:12, fgorski wrote: > nit: TODO(whom) Done. https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator_unittest.cc:618: On 2016/08/02 04:18:12, fgorski wrote: > nit: remove empty line > > Question: Do you intend to verify the second request some other time, or is this > an omission? (please add a TODO in former case, or fix the code in the latter.) Done. https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_queue.cc (right): https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_queue.cc:20: DVLOG(0) << "@@@@@@ " << __func__; On 2016/08/02 04:18:12, fgorski wrote: > nit: do you need to keep these? Nope, leftover from debugging. I must have missed them on my pass to clean up for code review. https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_queue_in_memory_store.cc (right): https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_queue_in_memory_store.cc:21: for (const auto& id_request_pair : requests_) { On 2016/08/02 04:18:12, fgorski wrote: > nit: do you need the braces here? Done. https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_queue_store_unittest.cc (right): https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_queue_store_unittest.cc:117: DVLOG(0) << "@@@@@@ " << __func__; On 2016/08/02 04:18:12, fgorski wrote: > nit: do you need to keep this? Done. https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_status.h (right): https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_status.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. Removing this whole file, we will use ClientIds instead. https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_status.h:15: std::string client_id; On 2016/08/02 04:18:12, fgorski wrote: > why not a ClientId? you will get partial information from this and this approach > precludes the struct from being used in a more generic queries. The idea was that the client already knows the namespace (since they had to supply it with the request to get statuses), so we only need to give them the id within their namespace. https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_status.h:16: SavePageRequest::Status status; On 2016/08/02 20:39:28, dougarnett wrote: > Later, we may end up wanting to move this Status enum out of SavePageRequest. File deleted https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_status.h:17: // TODO(petewil): We can easily add the URL, would it help? On 2016/08/02 20:39:28, dougarnett wrote: > Seems like a good thing to include although don't have strong use case (might > depend on how ClientIds are constructed by client (if they have any real > information or not)) File deleted https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_status.h:19: RequestStatus(std::string id, SavePageRequest::Status request_status) On 2016/08/02 04:18:12, fgorski wrote: > const & for id. File deleted https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_status.h:22: }; On 2016/08/02 04:18:12, fgorski wrote: > nit: git cl format, which will solve a few issues in this file. Done. Generally I'm not a fan of git cl format, while it fixes these issues, it does the indentation in a way that I don't prefer, I think it chooses a less readable of the allowed options. If you ask for it, I'll run it, but I will likely just fix things up by hand otherwise. https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_status.h:25: On 2016/08/02 04:18:12, fgorski wrote: > nit: remove extra line. Done.
https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:92: for(auto iter = requests.begin(); iter != requests.end(); ++iter) { On 2016/08/03 00:24:31, Pete Williamson wrote: > On 2016/08/02 04:18:11, fgorski wrote: > > for (const auto& request : requests) is the simplest allowed form I believe, > > per: > > https://chromium-cpp.appspot.com/ (Range-Based For Loops section) > > Right, find_if would take a lambda and the collection, and search for matching > items. I'm wondering if the lambda version introduces more or less cognitive > loading for the reader (ie, is it more or less readable?) What I suggested is not a lambda, but a C++11 idiom equivalent to your for loop, except that it also enforces const. If you insist on going with iterator, use cbeing() and cend(). https://codereview.chromium.org/2202113002/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2202113002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:89: std::vector<ClientId> found_requests; Consider renaming to client_ids. https://codereview.chromium.org/2202113002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:91: // TODO(petewil): Is find_if better that a for loop here? no, it would be incorrect, because: find_if returns the first element in the range [first, last) that satisfies specific criteria. Following: http://en.cppreference.com/w/cpp/algorithm/find Please remove the TODO. Also, copy_if would not work, and for_each is not reasonable here. I think you have the right solution. https://codereview.chromium.org/2202113002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:93: if (client_namespace == iter->client_id().name_space) { nit: remove {} https://codereview.chromium.org/2202113002/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2202113002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.h:112: QueuedRequestCallback callback, const &
lgtm with resolving comments https://codereview.chromium.org/2202113002/diff/20001/components/offline_page... File components/offline_pages/background/BUILD.gn (right): https://codereview.chromium.org/2202113002/diff/20001/components/offline_page... components/offline_pages/background/BUILD.gn:29: "request_status.h", revert https://codereview.chromium.org/2202113002/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2202113002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.h:59: // For a client namespace, get the client_id of all requests for that for comment probably better "client_id" => "client id" or "ClientId" https://codereview.chromium.org/2202113002/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/2202113002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator_unittest.cc:164: void GetQueuedRequestsDone(const std::vector<ClientId>& statuses); statuses => client_ids
https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2202113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:92: for(auto iter = requests.begin(); iter != requests.end(); ++iter) { On 2016/08/03 03:13:15, fgorski wrote: > On 2016/08/03 00:24:31, Pete Williamson wrote: > > On 2016/08/02 04:18:11, fgorski wrote: > > > for (const auto& request : requests) is the simplest allowed form I believe, > > > per: > > > https://chromium-cpp.appspot.com/ (Range-Based For Loops section) > > > > Right, find_if would take a lambda and the collection, and search for matching > > items. I'm wondering if the lambda version introduces more or less cognitive > > loading for the reader (ie, is it more or less readable?) > > What I suggested is not a lambda, but a C++11 idiom equivalent to your for loop, > except that it also enforces const. > If you insist on going with iterator, use cbeing() and cend(). I changed this to use a ranged for loop as you suggest. Just out of curiosity, why cbegin and cend instead of begin and end? https://codereview.chromium.org/2202113002/diff/20001/components/offline_page... File components/offline_pages/background/BUILD.gn (right): https://codereview.chromium.org/2202113002/diff/20001/components/offline_page... components/offline_pages/background/BUILD.gn:29: "request_status.h", On 2016/08/03 04:27:19, dougarnett wrote: > revert Done. https://codereview.chromium.org/2202113002/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2202113002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:89: std::vector<ClientId> found_requests; On 2016/08/03 03:13:15, fgorski wrote: > Consider renaming to client_ids. Done. https://codereview.chromium.org/2202113002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:91: // TODO(petewil): Is find_if better that a for loop here? On 2016/08/03 03:13:15, fgorski wrote: > no, it would be incorrect, because: > find_if returns the first element in the range [first, last) that satisfies > specific criteria. > Following: http://en.cppreference.com/w/cpp/algorithm/find > > Please remove the TODO. > > Also, copy_if would not work, and for_each is not reasonable here. I think you > have the right solution. Acknowledged. https://codereview.chromium.org/2202113002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:93: if (client_namespace == iter->client_id().name_space) { On 2016/08/03 03:13:15, fgorski wrote: > nit: remove {} Done. https://codereview.chromium.org/2202113002/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2202113002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.h:59: // For a client namespace, get the client_id of all requests for that On 2016/08/03 04:27:19, dougarnett wrote: > for comment probably better "client_id" => "client id" or "ClientId" Done. https://codereview.chromium.org/2202113002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.h:112: QueuedRequestCallback callback, On 2016/08/03 03:13:15, fgorski wrote: > const & Done. https://codereview.chromium.org/2202113002/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/2202113002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator_unittest.cc:164: void GetQueuedRequestsDone(const std::vector<ClientId>& statuses); On 2016/08/03 04:27:19, dougarnett wrote: > statuses => client_ids Done.
lgtm
lgtm
The CQ bit was checked by petewil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dougarnett@chromium.org Link to the patchset: https://codereview.chromium.org/2202113002/#ps80001 (title: "Clean up naming")
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 #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== API to provide status of save page reqeusts to API. For components to drive background offlining programmatically, this provides a new status API to check the current status of offline page requests. BUG=610521 ========== to ========== API to provide status of save page reqeusts to API. For components to drive background offlining programmatically, this provides a new status API to check the current status of offline page requests. BUG=610521 Committed: https://crrev.com/7b2bb2f29d275938d82f478128dc7154c6dc47a8 Cr-Commit-Position: refs/heads/master@{#409828} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/7b2bb2f29d275938d82f478128dc7154c6dc47a8 Cr-Commit-Position: refs/heads/master@{#409828} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
