Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2343743002:
[Offline pages] Updating the UpdateCallback in OPMStoreSQL (Closed)
|
Created:
4 years, 3 months ago by fgorski 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] Updating the UpdateCallback in OPMStoreSQL
* Introduces an StoreUdpateResult object to carry the result of update in unique ptr.
* Propagates the changes to consumers of the store.
* Adds/Updates tests.
BUG=645522
Committed: https://crrev.com/7c81890adbfc05d426e76e2850e6b07022c58b6b
Cr-Commit-Position: refs/heads/master@{#419926}
Patch Set 1 #
Total comments: 20
Patch Set 2 : Modifying the UpdateCallback, adding StoreUdpateResult #
Total comments: 7
Patch Set 3 : Addressing feedback #
Total comments: 5
Patch Set 4 : Applying offline_page_item operator== in test code #Patch Set 5 : Addressing feedback from Doug and rebasing #
Total comments: 6
Patch Set 6 : Addressing final feedback #
Total comments: 4
Messages
Total messages: 50 (25 generated)
The CQ bit was checked by fgorski@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...
fgorski@chromium.org changed reviewers: + dewittj@chromium.org, dimich@chromium.org, dougarnett@chromium.org, petewil@chromium.org
PTAL. This is a bit from being done, but captures the essence of how I want to change the callbacks. Let me know what you think. I'll be working on making it nicer tomorrow and will accept suggestions.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
There is somethign cumbersome in returning 2 collections that are subtly different but mostly the same... Mayeb we can discuss offline.
There is somethign cumbersome in returning 2 collections that are subtly different but mostly the same... Mayeb we can discuss offline.
On 2016/09/15 17:22:44, Dmitry Titov wrote:
> There is somethign cumbersome in returning 2 collections that are subtly
> different but mostly the same... Mayeb we can discuss offline.
To capture the discussion so far:
1. The 2 collections are disjoint:
* first includes items that failed to be udpated/deleted, and the reason for
it
* second includes the items that were updated (which might be required by
certain observers)
2. Having a single collection is technically doable if all observers are
stateful, and that might be the case for download_ui_adapter, however
there are other consumers of OPM that currently rely on client_id passed
in OnPageDeleted.
3. Some observers should/could/need to be stateless. E.g.
RequestCoordinator::Observer is used to show notifications by simply parsing
the GUID for the download/async items. If we only provide offline_id we have
to keep state in it and I am not sure that that is the best solution.
Let's discuss further.
https://codereview.chromium.org/2343743002/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_metadata_store_impl_unittest.cc (right): https://codereview.chromium.org/2343743002/diff/1/components/offline_pages/of... components/offline_pages/offline_page_metadata_store_impl_unittest.cc:297: last_status_ = updated_items.size() > 0 ? STATUS_TRUE : STATUS_FALSE; Should we save more information here? This doesn't capture if any items failed the update. Maybe save a copy of failed_items, so we can see if the size is non-zero in unit tests. https://codereview.chromium.org/2343743002/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_metadata_store_sql.cc (right): https://codereview.chromium.org/2343743002/diff/1/components/offline_pages/of... components/offline_pages/offline_page_metadata_store_sql.cc:291: while (statement.Step()) { Nit - why use "while" here if offline_id is unique? Would "if" be better? https://codereview.chromium.org/2343743002/diff/1/components/offline_pages/of... components/offline_pages/offline_page_metadata_store_sql.cc:370: for (auto& page : pages) { Should this be const auto& ? https://codereview.chromium.org/2343743002/diff/1/components/offline_pages/of... components/offline_pages/offline_page_metadata_store_sql.cc:403: for (auto offline_id : offline_ids) { const? https://codereview.chromium.org/2343743002/diff/1/components/offline_pages/of... components/offline_pages/offline_page_metadata_store_sql.cc:405: if (!GetPageByOfflineIdSync(db, offline_id, &page)) { Do we need this SQL operation when we can just check the return value of DeleteByOfflineId()? https://codereview.chromium.org/2343743002/diff/1/components/offline_pages/of... components/offline_pages/offline_page_metadata_store_sql.cc:510: PostStoreErrorForAllIds(base::ThreadTaskRunnerHandle::Get(), offline_ids, Is "STORE_ERROR" the right error for an empty list of things to remove? That seems like it should be a different kind of error code to me. https://codereview.chromium.org/2343743002/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_model_impl.cc (right): https://codereview.chromium.org/2343743002/diff/1/components/offline_pages/of... components/offline_pages/offline_page_model_impl.cc:244: for (const auto& page : deleted_pages) { Nice change, this is much more readable than iter->second.<something> https://codereview.chromium.org/2343743002/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_test_store.cc (right): https://codereview.chromium.org/2343743002/diff/1/components/offline_pages/of... components/offline_pages/offline_page_test_store.cc:57: // TODO(fgorski): Cover scenario, where new items are being created, while nit - this sentence would read better with no commas.
https://codereview.chromium.org/2343743002/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_metadata_store.h (right): https://codereview.chromium.org/2343743002/diff/1/components/offline_pages/of... components/offline_pages/offline_page_metadata_store.h:61: typedef base::Callback<void(const std::map<int64_t, ItemActionStatus>&, nit - seems like successful collection should come first and then failure collections (but realize this may be easier iteration to keep same order). https://codereview.chromium.org/2343743002/diff/1/components/offline_pages/of... components/offline_pages/offline_page_metadata_store.h:61: typedef base::Callback<void(const std::map<int64_t, ItemActionStatus>&, perhaps in the future, if we never end up using SUCCESS status code, ItemActionStatus could become FailureStatus or similar and then this typedef would be a bit more self documenting.
The CQ bit was checked by fgorski@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...
I didn't get the chance to apply all the feedback, but was able to redo the UdpateCallback interface based on our discussion. PTAL. If you find other things that irk you please point out, even if they duplicate the old comments. I'll be addressing comments and some of my TODO's next.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2343743002/diff/20001/components/offline_page... File components/offline_pages/offline_page_metadata_store.h (right): https://codereview.chromium.org/2343743002/diff/20001/components/offline_page... components/offline_pages/offline_page_metadata_store.h:46: std::vector<std::pair<int64_t, ItemActionStatus>> item_statuses; I still don't see the usefulness of including SUCCESS statuses here. It makes it harder to tease out the failures. Did you identify any consuming use case where it is helpful to have them? That is, if this were just "failures" or "failed_statuses", then it would be easier for consumer to simply check size of failures or simply the code iterating over them to not have to filter out SUCCESS codes. Why duplicate the success knowledge that alread is returned in the updated_requests collection unless it makes using it easier rather than harder? https://codereview.chromium.org/2343743002/diff/20001/components/offline_page... components/offline_pages/offline_page_metadata_store.h:48: // List of update offline page items as seen after operation concludes. List of successfully updated ... ? https://codereview.chromium.org/2343743002/diff/20001/components/offline_page... File components/offline_pages/offline_page_model_impl.cc (right): https://codereview.chromium.org/2343743002/diff/20001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:976: if (result->updated_items.size() == 0 && result->item_statuses.size() > 0 && It we had 'failures' rather than 'item_statuses' for all items this could be much simpler check.
On 2016/09/19 16:37:29, dougarnett wrote: > https://codereview.chromium.org/2343743002/diff/20001/components/offline_page... > File components/offline_pages/offline_page_metadata_store.h (right): > > https://codereview.chromium.org/2343743002/diff/20001/components/offline_page... > components/offline_pages/offline_page_metadata_store.h:46: > std::vector<std::pair<int64_t, ItemActionStatus>> item_statuses; > I still don't see the usefulness of including SUCCESS statuses here. It makes > it harder to tease out the failures. Did you identify any consuming use case > where it is helpful to have them? > > That is, if this were just "failures" or "failed_statuses", then it would be > easier for consumer to simply check size of failures or simply the code > iterating over them to not have to filter out SUCCESS codes. Why duplicate the > success knowledge that alread is returned in the updated_requests collection > unless it makes using it easier rather than harder? > > https://codereview.chromium.org/2343743002/diff/20001/components/offline_page... > components/offline_pages/offline_page_metadata_store.h:48: // List of update > offline page items as seen after operation concludes. > List of successfully updated ... ? > > https://codereview.chromium.org/2343743002/diff/20001/components/offline_page... > File components/offline_pages/offline_page_model_impl.cc (right): > > https://codereview.chromium.org/2343743002/diff/20001/components/offline_page... > components/offline_pages/offline_page_model_impl.cc:976: if > (result->updated_items.size() == 0 && result->item_statuses.size() > 0 && > It we had 'failures' rather than 'item_statuses' for all items this could be > much simpler check. New approach looks good to me, waiting on other fixes.
The CQ bit was checked by fgorski@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...
PTAL. 2 action items for tomorrow: * Merge with a patch that fixes a crash in GetOfflinePages * Update the tests to use operator== for OPI https://codereview.chromium.org/2343743002/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_metadata_store.h (right): https://codereview.chromium.org/2343743002/diff/1/components/offline_pages/of... components/offline_pages/offline_page_metadata_store.h:61: typedef base::Callback<void(const std::map<int64_t, ItemActionStatus>&, On 2016/09/15 19:57:24, dougarnett wrote: > nit - seems like successful collection should come first and then failure > collections (but realize this may be easier iteration to keep same order). Done. (Callback redesigned in patch 1) https://codereview.chromium.org/2343743002/diff/1/components/offline_pages/of... components/offline_pages/offline_page_metadata_store.h:61: typedef base::Callback<void(const std::map<int64_t, ItemActionStatus>&, On 2016/09/15 19:57:24, dougarnett wrote: > perhaps in the future, if we never end up using SUCCESS status code, > ItemActionStatus could become FailureStatus or similar and then this typedef > would be a bit more self documenting. Done.(Callback redesigned in patch 1) https://codereview.chromium.org/2343743002/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_metadata_store_impl_unittest.cc (right): https://codereview.chromium.org/2343743002/diff/1/components/offline_pages/of... components/offline_pages/offline_page_metadata_store_impl_unittest.cc:297: last_status_ = updated_items.size() > 0 ? STATUS_TRUE : STATUS_FALSE; On 2016/09/15 18:05:43, Pete Williamson wrote: > Should we save more information here? This doesn't capture if any items failed > the update. Maybe save a copy of failed_items, so we can see if the size is > non-zero in unit tests. Done. I actually take the ownership of the result. I am doing basic validation in tests, but we can extend that further. https://codereview.chromium.org/2343743002/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_metadata_store_sql.cc (right): https://codereview.chromium.org/2343743002/diff/1/components/offline_pages/of... components/offline_pages/offline_page_metadata_store_sql.cc:291: while (statement.Step()) { On 2016/09/15 18:05:43, Pete Williamson wrote: > Nit - why use "while" here if offline_id is unique? Would "if" be better? Done. https://codereview.chromium.org/2343743002/diff/1/components/offline_pages/of... components/offline_pages/offline_page_metadata_store_sql.cc:370: for (auto& page : pages) { On 2016/09/15 18:05:43, Pete Williamson wrote: > Should this be const auto& ? Done. https://codereview.chromium.org/2343743002/diff/1/components/offline_pages/of... components/offline_pages/offline_page_metadata_store_sql.cc:403: for (auto offline_id : offline_ids) { On 2016/09/15 18:05:43, Pete Williamson wrote: > const? Switched to int64_t instead. https://codereview.chromium.org/2343743002/diff/1/components/offline_pages/of... components/offline_pages/offline_page_metadata_store_sql.cc:405: if (!GetPageByOfflineIdSync(db, offline_id, &page)) { On 2016/09/15 18:05:43, Pete Williamson wrote: > Do we need this SQL operation when we can just check the return value of > DeleteByOfflineId()? Yes, because we want to return the deleted items for observers. https://codereview.chromium.org/2343743002/diff/1/components/offline_pages/of... components/offline_pages/offline_page_metadata_store_sql.cc:510: PostStoreErrorForAllIds(base::ThreadTaskRunnerHandle::Get(), offline_ids, On 2016/09/15 18:05:43, Pete Williamson wrote: > Is "STORE_ERROR" the right error for an empty list of things to remove? That > seems like it should be a different kind of error code to me. Done. https://codereview.chromium.org/2343743002/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_model_impl.cc (right): https://codereview.chromium.org/2343743002/diff/1/components/offline_pages/of... components/offline_pages/offline_page_model_impl.cc:244: for (const auto& page : deleted_pages) { On 2016/09/15 18:05:43, Pete Williamson wrote: > Nice change, this is much more readable than iter->second.<something> Acknowledged. https://codereview.chromium.org/2343743002/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_test_store.cc (right): https://codereview.chromium.org/2343743002/diff/1/components/offline_pages/of... components/offline_pages/offline_page_test_store.cc:57: // TODO(fgorski): Cover scenario, where new items are being created, while On 2016/09/15 18:05:43, Pete Williamson wrote: > nit - this sentence would read better with no commas. Done. https://codereview.chromium.org/2343743002/diff/20001/components/offline_page... File components/offline_pages/offline_page_metadata_store.h (right): https://codereview.chromium.org/2343743002/diff/20001/components/offline_page... components/offline_pages/offline_page_metadata_store.h:46: std::vector<std::pair<int64_t, ItemActionStatus>> item_statuses; On 2016/09/19 16:37:29, dougarnett wrote: > I still don't see the usefulness of including SUCCESS statuses here. It makes > it harder to tease out the failures. Did you identify any consuming use case > where it is helpful to have them? > > That is, if this were just "failures" or "failed_statuses", then it would be > easier for consumer to simply check size of failures or simply the code > iterating over them to not have to filter out SUCCESS codes. Why duplicate the > success knowledge that alread is returned in the updated_requests collection > unless it makes using it easier rather than harder? I agree with you that there is certain information that feels duplicated here. We discussed it with the team here and decided that putting statuses for all items makes sense, because that status collection is consumed by the original caller (e.g. consumer requesting to delete the items), while the latter is used with the observers. Caller has better context, therefor it is enough to just get statuses, and that is our plan. Also constructing the whole list of results would require traversing both lists in the alternative approach that you proposed. WDYT? https://codereview.chromium.org/2343743002/diff/20001/components/offline_page... components/offline_pages/offline_page_metadata_store.h:48: // List of update offline page items as seen after operation concludes. On 2016/09/19 16:37:29, dougarnett wrote: > List of successfully updated ... ? Done. https://codereview.chromium.org/2343743002/diff/20001/components/offline_page... File components/offline_pages/offline_page_model_impl.cc (right): https://codereview.chromium.org/2343743002/diff/20001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:976: if (result->updated_items.size() == 0 && result->item_statuses.size() > 0 && On 2016/09/19 16:37:29, dougarnett wrote: > It we had 'failures' rather than 'item_statuses' for all items this could be > much simpler check. I meant to update the code. We have a pretty simple check now.
looks good so far, I'll look at tommorow's changes before stamping it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2343743002/diff/20001/components/offline_page... File components/offline_pages/offline_page_metadata_store.h (right): https://codereview.chromium.org/2343743002/diff/20001/components/offline_page... components/offline_pages/offline_page_metadata_store.h:46: std::vector<std::pair<int64_t, ItemActionStatus>> item_statuses; On 2016/09/19 23:24:30, fgorski wrote: > On 2016/09/19 16:37:29, dougarnett wrote: > > I still don't see the usefulness of including SUCCESS statuses here. It makes > > it harder to tease out the failures. Did you identify any consuming use case > > where it is helpful to have them? > > > > That is, if this were just "failures" or "failed_statuses", then it would be > > easier for consumer to simply check size of failures or simply the code > > iterating over them to not have to filter out SUCCESS codes. Why duplicate the > > success knowledge that alread is returned in the updated_requests collection > > unless it makes using it easier rather than harder? > > I agree with you that there is certain information that feels duplicated here. > We discussed it with the team here and decided that putting statuses for all > items makes sense, because that status collection is consumed by the original > caller (e.g. consumer requesting to delete the items), while the latter is used > with the observers. Caller has better context, therefor it is enough to just get > statuses, and that is our plan. > > Also constructing the whole list of results would require traversing both lists > in the alternative approach that you proposed. > > WDYT? Perhaps my alternative wasn't clear, I was not proposing constructing a whole list of results. Only the successful ones. And that is the part I care most about in the callback interface - that I can just process the updated_results knowing they were all successful (P1, the failure status idea is P3). I don't buy the caller context argument where it is possible for the caller to only have the id for the object but then will need details of the object. The updated_requests will be what that caller will want to iterate over. Indeed, for the multiple response callback code that I wrote (for request queue), I found myself wanting to traverse both subset collections - 1. handle the successes (with full information since my caller only had the context of request ids (pause, resume)); and 2. handle the failures, if any. Maybe some callers with more context will find it useful to just traverse the statuses but I am skeptical lacking an example. But we can agree to disagree.
On 2016/09/20 01:39:40, dougarnett wrote: > https://codereview.chromium.org/2343743002/diff/20001/components/offline_page... > File components/offline_pages/offline_page_metadata_store.h (right): > > https://codereview.chromium.org/2343743002/diff/20001/components/offline_page... > components/offline_pages/offline_page_metadata_store.h:46: > std::vector<std::pair<int64_t, ItemActionStatus>> item_statuses; > On 2016/09/19 23:24:30, fgorski wrote: > > On 2016/09/19 16:37:29, dougarnett wrote: > > > I still don't see the usefulness of including SUCCESS statuses here. It > makes > > > it harder to tease out the failures. Did you identify any consuming use case > > > where it is helpful to have them? > > > > > > That is, if this were just "failures" or "failed_statuses", then it would be > > > easier for consumer to simply check size of failures or simply the code > > > iterating over them to not have to filter out SUCCESS codes. Why duplicate > the > > > success knowledge that alread is returned in the updated_requests collection > > > unless it makes using it easier rather than harder? > > > > I agree with you that there is certain information that feels duplicated here. > > We discussed it with the team here and decided that putting statuses for all > > items makes sense, because that status collection is consumed by the original > > caller (e.g. consumer requesting to delete the items), while the latter is > used > > with the observers. Caller has better context, therefor it is enough to just > get > > statuses, and that is our plan. > > > > Also constructing the whole list of results would require traversing both > lists > > in the alternative approach that you proposed. > > > > WDYT? > > Perhaps my alternative wasn't clear, I was not proposing constructing a > whole list of results. Only the successful ones. And that is the part I care > most about in the callback interface - that I can just process the > updated_results knowing they were all successful (P1, the failure status > idea is P3). That is how updated_items work now. These are only the results that were successfully updated/removed. > > I don't buy the caller context argument where it is possible for the caller to > only have the id for the object but then will need details of the object. The > updated_requests will be what that caller will want to iterate over. > The reason that works is because you can curry whatever you want to the call, and create context for the IDs to status mapping that come back. > Indeed, for the multiple response callback code that I wrote (for request > queue), I found myself wanting to traverse both subset collections - > 1. handle the successes (with full information since my caller only had > the context of request ids (pause, resume)); and 2. handle the failures, > if any. We can always return more to the caller when that is necessary. Let's look at the code and see where such cases appear. > Maybe some callers with more context will find it useful to just traverse the > statuses but I am skeptical lacking an example. But we can agree to disagree. In case of OfflinePageModel this is the first time we return anything else than a single boolean, so it is easy to find examples of how this is consumed without any collection in a callback. Granted we are trying to make that part better. https://cs.chromium.org/chromium/src/components/offline_pages/offline_page_me...
On 2016/09/20 04:04:20, fgorski wrote: ... > > Perhaps my alternative wasn't clear, I was not proposing constructing a > > whole list of results. Only the successful ones. And that is the part I care > > most about in the callback interface - that I can just process the > > updated_results knowing they were all successful (P1, the failure status > > idea is P3). > > That is how updated_items work now. These are only the results that were > successfully updated/removed. Oh, good. It was not the way request queue worked so was conflating with that. > > > > I don't buy the caller context argument where it is possible for the caller to > > only have the id for the object but then will need details of the object. The > > updated_requests will be what that caller will want to iterate over. > > > > The reason that works is because you can curry whatever you want to the call, > and create context for the IDs to status mapping that come back. My point was that if the caller only starts with object ids (again I am drawing on request queue example pause/resume/remove, not offline page model), but may want more information than success/failure - even if just for UMA - then it will iterate over the updated_requests.
https://codereview.chromium.org/2343743002/diff/40001/components/offline_page... File components/offline_pages/offline_page_model_impl.cc (right): https://codereview.chromium.org/2343743002/diff/40001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:957: // This pat of the loop is explicitly broken out, as it should be gone in pat => part ? https://codereview.chromium.org/2343743002/diff/40001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:976: delete_result = DeletePageResult::SUCCESS; Is this mapping accurate? Does the DeletePageResult here just mean whether the request was able to be successfully attempted against the store vs. whether items were actually found and deleted (vs. DOESNT_EXIST)? If so, might be good to briefly comment that intent.
The CQ bit was checked by fgorski@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...
Addressed, rebased and applied reminder of changes. PTAL https://codereview.chromium.org/2343743002/diff/40001/components/offline_page... File components/offline_pages/offline_page_model_impl.cc (right): https://codereview.chromium.org/2343743002/diff/40001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:957: // This pat of the loop is explicitly broken out, as it should be gone in On 2016/09/20 16:12:44, dougarnett wrote: > pat => part ? Done. https://codereview.chromium.org/2343743002/diff/40001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:976: delete_result = DeletePageResult::SUCCESS; On 2016/09/20 16:12:44, dougarnett wrote: > Is this mapping accurate? Does the DeletePageResult here just mean whether the > request was able to be successfully attempted against the store vs. whether > items were actually found and deleted (vs. DOESNT_EXIST)? If so, might be good > to briefly comment that intent. 1. I agree we have some work to do here. For now we are not changing the semantics of this call, but I'll add the TODO for fixing the offline page model interface and pass more reasonable result. 2. The behavior you ask for is already documented with DeletePageResult in offline_page_types.h. The reason we don't treat DOESNT_EXIST as failure is that we want to remove the items, and they are gone, so no issue there, from the caller perspective. Result is the same. As per point 1 this behavior definitely requires a refinement.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
lgtm On Tue, Sep 20, 2016 at 11:43 AM, <fgorski@chromium.org> wrote: > Addressed, rebased and applied reminder of changes. > PTAL > > > https://codereview.chromium.org/2343743002/diff/40001/ > components/offline_pages/offline_page_model_impl.cc > File components/offline_pages/offline_page_model_impl.cc (right): > > https://codereview.chromium.org/2343743002/diff/40001/ > components/offline_pages/offline_page_model_impl.cc#newcode957 > components/offline_pages/offline_page_model_impl.cc:957: // This pat of > the loop is explicitly broken out, as it should be gone in > On 2016/09/20 16:12:44, dougarnett wrote: > > pat => part ? > > Done. > > https://codereview.chromium.org/2343743002/diff/40001/ > components/offline_pages/offline_page_model_impl.cc#newcode976 > components/offline_pages/offline_page_model_impl.cc:976: delete_result = > DeletePageResult::SUCCESS; > On 2016/09/20 16:12:44, dougarnett wrote: > > Is this mapping accurate? Does the DeletePageResult here just mean > whether the > > request was able to be successfully attempted against the store vs. > whether > > items were actually found and deleted (vs. DOESNT_EXIST)? If so, might > be good > > to briefly comment that intent. > > 1. I agree we have some work to do here. For now we are not changing the > semantics of this call, but I'll add the TODO for fixing the offline > page model interface and pass more reasonable result. > > 2. The behavior you ask for is already documented with DeletePageResult > in offline_page_types.h. The reason we don't treat DOESNT_EXIST as > failure is that we want to remove the items, and they are gone, so no > issue there, from the caller perspective. Result is the same. As per > point 1 this behavior definitely requires a refinement. > > https://codereview.chromium.org/2343743002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm https://codereview.chromium.org/2343743002/diff/40001/components/offline_page... File components/offline_pages/offline_page_model_impl.cc (right): https://codereview.chromium.org/2343743002/diff/40001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:976: delete_result = DeletePageResult::SUCCESS; On 2016/09/20 18:43:55, fgorski wrote: > On 2016/09/20 16:12:44, dougarnett wrote: > > Is this mapping accurate? Does the DeletePageResult here just mean whether the > > request was able to be successfully attempted against the store vs. whether > > items were actually found and deleted (vs. DOESNT_EXIST)? If so, might be good > > to briefly comment that intent. > > 1. I agree we have some work to do here. For now we are not changing the > semantics of this call, but I'll add the TODO for fixing the offline page model > interface and pass more reasonable result. > > 2. The behavior you ask for is already documented with DeletePageResult in > offline_page_types.h. The reason we don't treat DOESNT_EXIST as failure is that > we want to remove the items, and they are gone, so no issue there, from the > caller perspective. Result is the same. As per point 1 this behavior definitely > requires a refinement. Ah, ok, I see the deprecated comment on NOT_FOUND now (was just looking at the top comment and SUCCESS).
lgtm with nits: https://codereview.chromium.org/2343743002/diff/80001/components/offline_page... File components/offline_pages/offline_page_item.cc (right): https://codereview.chromium.org/2343743002/diff/80001/components/offline_page... components/offline_pages/offline_page_item.cc:65: return url == other.url && offline_id == other.offline_id && It'd be more human-readable if it was just a single column. A bit more lines but less strain on eyes. https://codereview.chromium.org/2343743002/diff/80001/components/offline_page... File components/offline_pages/offline_page_metadata_store.h (right): https://codereview.chromium.org/2343743002/diff/80001/components/offline_page... components/offline_pages/offline_page_metadata_store.h:25: FAILED_INITIALIZATION, It would be goog to have those values documented. For exampel, FAILED_INITIALIZATION - is it some initial initialization of empty store or is it in fact FAILED_LOADING? https://codereview.chromium.org/2343743002/diff/80001/components/offline_page... components/offline_pages/offline_page_metadata_store.h:33: DOESNT_EXIST, Naming suggestion: DOESNT_EXIST -> NOT_FOUND?
The CQ bit was checked by fgorski@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...
Addressed. Going for a dry run now. https://codereview.chromium.org/2343743002/diff/80001/components/offline_page... File components/offline_pages/offline_page_item.cc (right): https://codereview.chromium.org/2343743002/diff/80001/components/offline_page... components/offline_pages/offline_page_item.cc:65: return url == other.url && offline_id == other.offline_id && On 2016/09/20 20:43:37, Dmitry Titov wrote: > It'd be more human-readable if it was just a single column. A bit more lines but > less strain on eyes. Done. The problem is caused by "git cl format" and I absolutely agree with you. I made a manual update at the end to prevent automatic formatting. https://codereview.chromium.org/2343743002/diff/80001/components/offline_page... File components/offline_pages/offline_page_metadata_store.h (right): https://codereview.chromium.org/2343743002/diff/80001/components/offline_page... components/offline_pages/offline_page_metadata_store.h:25: FAILED_INITIALIZATION, On 2016/09/20 20:43:37, Dmitry Titov wrote: > It would be goog to have those values documented. For exampel, > FAILED_INITIALIZATION - is it some initial initialization of empty store or is > it in fact FAILED_LOADING? Done. https://codereview.chromium.org/2343743002/diff/80001/components/offline_page... components/offline_pages/offline_page_metadata_store.h:33: DOESNT_EXIST, On 2016/09/20 20:43:37, Dmitry Titov wrote: > Naming suggestion: DOESNT_EXIST -> NOT_FOUND? Done.
lgtm
The CQ bit was unchecked by fgorski@chromium.org
The CQ bit was checked by fgorski@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dougarnett@chromium.org, dimich@chromium.org Link to the patchset: https://codereview.chromium.org/2343743002/#ps100001 (title: "Addressing final feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Offline pages] Updating the UpdateCallback in OPMStoreSQL BUG=645522 ========== to ========== [Offline pages] Updating the UpdateCallback in OPMStoreSQL * Introduces an StoreUdpateResult object to carry the result of update in unique ptr. * Propagates the changes to consumers of the store. * Adds/Updates tests. BUG=645522 ==========
Message was sent while issue was closed.
Description was changed from ========== [Offline pages] Updating the UpdateCallback in OPMStoreSQL * Introduces an StoreUdpateResult object to carry the result of update in unique ptr. * Propagates the changes to consumers of the store. * Adds/Updates tests. BUG=645522 ========== to ========== [Offline pages] Updating the UpdateCallback in OPMStoreSQL * Introduces an StoreUdpateResult object to carry the result of update in unique ptr. * Propagates the changes to consumers of the store. * Adds/Updates tests. BUG=645522 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [Offline pages] Updating the UpdateCallback in OPMStoreSQL * Introduces an StoreUdpateResult object to carry the result of update in unique ptr. * Propagates the changes to consumers of the store. * Adds/Updates tests. BUG=645522 ========== to ========== [Offline pages] Updating the UpdateCallback in OPMStoreSQL * Introduces an StoreUdpateResult object to carry the result of update in unique ptr. * Propagates the changes to consumers of the store. * Adds/Updates tests. BUG=645522 Committed: https://crrev.com/7c81890adbfc05d426e76e2850e6b07022c58b6b Cr-Commit-Position: refs/heads/master@{#419926} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/7c81890adbfc05d426e76e2850e6b07022c58b6b Cr-Commit-Position: refs/heads/master@{#419926}
Message was sent while issue was closed.
lgtm with a few nits https://codereview.chromium.org/2343743002/diff/100001/components/offline_pag... File components/offline_pages/offline_page_item.h (right): https://codereview.chromium.org/2343743002/diff/100001/components/offline_pag... components/offline_pages/offline_page_item.h:57: bool operator==(const OfflinePageItem& other) const; This should be documented; in particular, two OfflinePageItems could refer to the same file and be !=, if one had its access_count or other fields modified and the other didn't. https://codereview.chromium.org/2343743002/diff/100001/components/offline_pag... File components/offline_pages/offline_page_metadata_store_sql.cc (right): https://codereview.chromium.org/2343743002/diff/100001/components/offline_pag... components/offline_pages/offline_page_metadata_store_sql.cc:286: "SELECT * FROM " OFFLINE_PAGES_TABLE_NAME " WHERE offline_id = ?"; is 'SELECT *' discouraged in Chromium? It has been for me in other jobs https://codereview.chromium.org/2343743002/diff/100001/components/offline_pag... File components/offline_pages/offline_page_model_impl.h (right): https://codereview.chromium.org/2343743002/diff/100001/components/offline_pag... components/offline_pages/offline_page_model_impl.h:198: void OnMarkPageAccesseDone(const OfflinePageItem& offline_page_item, nit: spelling https://codereview.chromium.org/2343743002/diff/100001/components/offline_pag... File components/offline_pages/offline_page_test_store.cc (right): https://codereview.chromium.org/2343743002/diff/100001/components/offline_pag... components/offline_pages/offline_page_test_store.cc:7: #include <map> no longer necessary |
