Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2067143004:
[Offline Pages] Duplicate pages when save/delete bookmarks. (Closed)
|
Created:
4 years, 6 months ago by romax Modified:
4 years, 6 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. |
Description[Offline Pages] Duplicate pages when save/delete bookmarks.
There were duplicates since when saving bookmarks we'll not check if
pages with same url has been saved already, which was caused since we
were deleting offline pages associated with bookmarks before. Hence
changing it to the same logic as Last N, find pages with same url,
delete, then save new pages.
BUG=619223
Committed: https://crrev.com/af225ed9d44ab001730c795228af5c756595a7a6
Cr-Commit-Position: refs/heads/master@{#401798}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Moving to model level. #
Total comments: 3
Patch Set 3 : Using policies. #
Total comments: 7
Patch Set 4 : comments. #
Total comments: 10
Patch Set 5 : comments. #
Total comments: 10
Patch Set 6 : comments from dimich@. #
Messages
Total messages: 25 (6 generated)
romax@chromium.org changed reviewers: + fgorski@chromium.org
PTAL
https://codereview.chromium.org/2067143004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2067143004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:94: offlinePageBridge.getPagesByOnlineUrl(url, new Callback<List<OfflinePageItem>>() { this solution does not cover the case where you fail to save the offline page when hitting bookmark... you will end up with no saved pages in that case. How about solving that at the model level, by deleting cached pages once a successful "user initiated" save happens? (this call should be made based on which page is the newest and whether deleted page will be outlived by the one that will be shown or not)
PTAL, sorry for the lengthy comment. https://codereview.chromium.org/2067143004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2067143004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:94: offlinePageBridge.getPagesByOnlineUrl(url, new Callback<List<OfflinePageItem>>() { On 2016/06/15 20:19:01, fgorski wrote: > this solution does not cover the case where you fail to save the offline page > when hitting bookmark... you will end up with no saved pages in that case. > > How about solving that at the model level, by deleting cached pages once a > successful "user initiated" save happens? (this call should be made based on > which page is the newest and whether deleted page will be outlived by the one > that will be shown or not) I think we should have some logic here but I'm not sure 'newest' would be the best one. Since the user may visit a page from last-n and bookmark the same page, there'll be 2 copies of the same page. However, if we only keep one page per online url, it seems against the assumption we have already (getPagesByOnlineURL is returning multiple pages). Also, say a user opens a url, bookmark it, and continue browsing in the same tab, eventually the archive would be deleted by the purging logic in recent_tab_helper, so the user would not be able to access the bookmarked page. I think we should have a talk, but please briefly let me know what you think.
dimich@chromium.org changed reviewers: + dimich@chromium.org
+1 for doing it on Page Model level, and thanks for long comment! Some comments: https://codereview.chromium.org/2067143004/diff/20001/components/offline_page... File components/offline_pages/offline_page_model_impl.cc (right): https://codereview.chromium.org/2067143004/diff/20001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:802: OfflinePageModel::IsUserInitiated(client_id)) { So far, we seem to gravitate to a model where each namespace maintains one page per online URL. However, this is not clearly the only solution we need - for example, when using download action, user may download the same page multiple times - for example, various snapshots of his Facebook timeline or today's job openings etc. When saving other downloads, traditional UX concept is to add a new entry rather then override. We might end up with similar approach in Download Home. On the other hand, 'cache'-like namespaces like bookmarking or lastN might want to keep only the latest snapshot of the online URL. I would not link user-initiated with this policy because apparently it can be different case by case. Also, the method like this that adds some client-specific classification to the OPM shoudl not be on OPM... Perhaps better variant would be to pass additional parameter to SavePage, a boolean 'overwriteExistingPages' or something like that which will modify behavior of SavePage - then specific callers could invoke SavePage differently. At the place of call it is always known what we want in terms of overwrite behavior. A agree the purge should be confined to a namspace. https://codereview.chromium.org/2067143004/diff/20001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:835: // TODO(romax) Seems like nothing to do here. UMA on failures would be interesting.
https://codereview.chromium.org/2067143004/diff/20001/components/offline_page... File components/offline_pages/offline_page_model_impl.cc (right): https://codereview.chromium.org/2067143004/diff/20001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:802: OfflinePageModel::IsUserInitiated(client_id)) { On 2016/06/16 04:07:15, Dmitry Titov wrote: > So far, we seem to gravitate to a model where each namespace maintains one page > per online URL. > However, this is not clearly the only solution we need - for example, when using > download action, user may download the same page multiple times - for example, > various snapshots of his Facebook timeline or today's job openings etc. When > saving other downloads, traditional UX concept is to add a new entry rather then > override. We might end up with similar approach in Download Home. > > On the other hand, 'cache'-like namespaces like bookmarking or lastN might want > to keep only the latest snapshot of the online URL. > > I would not link user-initiated with this policy because apparently it can be > different case by case. Also, the method like this that adds some > client-specific classification to the OPM shoudl not be on OPM... Perhaps better > variant would be to pass additional parameter to SavePage, a boolean > 'overwriteExistingPages' or something like that which will modify behavior of > SavePage - then specific callers could invoke SavePage differently. At the place > of call it is always known what we want in terms of overwrite behavior. > > A agree the purge should be confined to a namspace. I think we already have the policy object which should be used for the purpose of making the call: Keep one or allow multiple. I agree that this call can be orthogonal from whether the snapshot is user initiated, especially because bookmarked page is technically cache, that was triggered by the user behavior, which makes it something in between the two. Please use OfflinePageClientPolicy here.
sorry for sending this late. PTAL if this is looking like in the right direction (to use policy) and I'm not sure if we still need the API level support 'isReplacingOldPage' in savePage() as an argument, if we're using policy now.
almost there https://codereview.chromium.org/2067143004/diff/40001/components/offline_page... File components/offline_pages/offline_page_model.cc (right): https://codereview.chromium.org/2067143004/diff/40001/components/offline_page... components/offline_pages/offline_page_model.cc:7: #include "components/offline_pages/client_namespace_constants.h" is this needed? https://codereview.chromium.org/2067143004/diff/40001/components/offline_page... File components/offline_pages/offline_page_model_impl.cc (right): https://codereview.chromium.org/2067143004/diff/40001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:810: base::ThreadTaskRunnerHandle::Get()->PostTask( It would probably make sense to run this code after you complete the prior delete operation (in case you are removing duplicate pages for the same url) https://codereview.chromium.org/2067143004/diff/40001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:835: return this->offline_pages_[a].last_access_time > rewrite this to not use offline_pages_ so that it still works when there is no offline_pages_ I suggest changing pages_to_delete to be a vector of offlinepageitem.
https://codereview.chromium.org/2067143004/diff/40001/components/offline_page... File components/offline_pages/client_policy_controller.h (right): https://codereview.chromium.org/2067143004/diff/40001/components/offline_page... components/offline_pages/client_policy_controller.h:31: size_t pages_allowed_per_url); just curious, and for my information... do we see a case in the future where we *want* to allow multiple pages per url? When/how would it happen, other than last_n & bookmarks? Also, how do we define url? As the full url with query + fragment? (like http://www.google.com?query=foo#header) or do we cut off the fragment?
Addressed comment, PTAL. Adding UMA was added as a to-do. https://codereview.chromium.org/2067143004/diff/40001/components/offline_page... File components/offline_pages/offline_page_model.cc (right): https://codereview.chromium.org/2067143004/diff/40001/components/offline_page... components/offline_pages/offline_page_model.cc:7: #include "components/offline_pages/client_namespace_constants.h" On 2016/06/17 22:51:06, fgorski wrote: > is this needed? Done. https://codereview.chromium.org/2067143004/diff/40001/components/offline_page... File components/offline_pages/offline_page_model_impl.cc (right): https://codereview.chromium.org/2067143004/diff/40001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:810: base::ThreadTaskRunnerHandle::Get()->PostTask( On 2016/06/17 22:51:06, fgorski wrote: > It would probably make sense to run this code after you complete the prior > delete operation (in case you are removing duplicate pages for the same url) Done. https://codereview.chromium.org/2067143004/diff/40001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:835: return this->offline_pages_[a].last_access_time > On 2016/06/17 22:51:06, fgorski wrote: > rewrite this to not use offline_pages_ so that it still works when there is no > offline_pages_ > I suggest changing pages_to_delete to be a vector of offlinepageitem. Done.
On 2016/06/20 18:17:47, chili wrote: > https://codereview.chromium.org/2067143004/diff/40001/components/offline_page... > File components/offline_pages/client_policy_controller.h (right): > > https://codereview.chromium.org/2067143004/diff/40001/components/offline_page... > components/offline_pages/client_policy_controller.h:31: size_t > pages_allowed_per_url); > just curious, and for my information... do we see a case in the future where we > *want* to allow multiple pages per url? When/how would it happen, other than > last_n & bookmarks? > > Also, how do we define url? As the full url with query + fragment? (like > http://www.google.com?query=foo#header) or do we cut off the fragment? I think Dmitry had a good point, say we have a page but it was full of dynamic information, like facebook timeline... We can have multiple pages for the same url. But then I guess it's another story for us to decide which page to show... For the definition of URL, i'm not 100% sure but we're using the url::GURL::spec to get the string for URL. You may find definitions there. I don't think it would cut off the fragment..
https://codereview.chromium.org/2067143004/diff/60001/components/offline_page... File components/offline_pages/offline_page_model_impl.cc (right): https://codereview.chromium.org/2067143004/diff/60001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:832: // Only keep |pages_allowed|-1 of most fresh pages, and delete others, by remove the coma in front of "and" please. https://codereview.chromium.org/2067143004/diff/60001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:833: // sorting pages with fresh ones first and resize the vector. should say: sorting with oldest first https://codereview.chromium.org/2067143004/diff/60001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:837: return a.last_access_time > b.last_access_time; should match the comment above < instead of > Please add appropriate tests for this. https://codereview.chromium.org/2067143004/diff/60001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:842: for (const auto& item : pages_to_delete) { nit: braces not needed
lgtm after previous comments applied (and problem fixed + test) dimich@ PTAL as well. https://codereview.chromium.org/2067143004/diff/60001/components/offline_page... File components/offline_pages/client_policy_controller.cc (right): https://codereview.chromium.org/2067143004/diff/60001/components/offline_page... components/offline_pages/client_policy_controller.cc:42: {lifetime_type, expire_period, page_limit}, nit: please let me know if you did git cl format. I am simply curious if that is default formatting for such case. If so I am perfectly happy
https://codereview.chromium.org/2067143004/diff/60001/components/offline_page... File components/offline_pages/client_policy_controller.cc (right): https://codereview.chromium.org/2067143004/diff/60001/components/offline_page... components/offline_pages/client_policy_controller.cc:42: {lifetime_type, expire_period, page_limit}, On 2016/06/21 19:49:51, fgorski wrote: > nit: please let me know if you did git cl format. > I am simply curious if that is default formatting for such case. If so I am > perfectly happy yes that's what git cl format gives me. https://codereview.chromium.org/2067143004/diff/60001/components/offline_page... File components/offline_pages/offline_page_model_impl.cc (right): https://codereview.chromium.org/2067143004/diff/60001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:832: // Only keep |pages_allowed|-1 of most fresh pages, and delete others, by On 2016/06/21 19:48:10, fgorski wrote: > remove the coma in front of "and" please. Done. https://codereview.chromium.org/2067143004/diff/60001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:833: // sorting pages with fresh ones first and resize the vector. On 2016/06/21 19:48:10, fgorski wrote: > should say: sorting with oldest first Done. https://codereview.chromium.org/2067143004/diff/60001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:837: return a.last_access_time > b.last_access_time; On 2016/06/21 19:48:10, fgorski wrote: > should match the comment above > < instead of > > > Please add appropriate tests for this. Done. https://codereview.chromium.org/2067143004/diff/60001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:842: for (const auto& item : pages_to_delete) { On 2016/06/21 19:48:10, fgorski wrote: > nit: braces not needed Done.
Nice, sorry for late review! https://codereview.chromium.org/2067143004/diff/80001/components/offline_page... File components/offline_pages/client_policy_controller.cc (right): https://codereview.chromium.org/2067143004/diff/80001/components/offline_page... components/offline_pages/client_policy_controller.cc:25: base::TimeDelta::FromDays(2), 20, 1))); I know it's not in this checkin, but why 20 instead of kUnlimitedPages for last_n? The spec seems to go away from fixed 'N' and rely solely on expiration. https://codereview.chromium.org/2067143004/diff/80001/components/offline_page... File components/offline_pages/offline_page_client_policy.h (right): https://codereview.chromium.org/2067143004/diff/80001/components/offline_page... components/offline_pages/offline_page_client_policy.h:52: // How many pages are allowed for an URL. It looks the comment can be made clearer - how about "How many pages for the same online URL can be stored at any time"? https://codereview.chromium.org/2067143004/diff/80001/components/offline_page... File components/offline_pages/offline_page_model_impl.cc (right): https://codereview.chromium.org/2067143004/diff/80001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:799: // Remove other user initiated pages with same url if the new save request was this comment can be re-phrsed without "user initiated" but with mentioning of policy. https://codereview.chromium.org/2067143004/diff/80001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:803: if (result == SavePageResult::SUCCESS && pages_allowed > 0) { pages_allowed > 0 looks strange here. If it can never be <=0 and you guard against invalid initialization of policies, then it's better to do it in the policy controller, possibly clamping it to a valid range. If <=0 can be valid values and mean something (looking at this code, I can start thinking that it means "no limit") then this should be documented in the policy class. https://codereview.chromium.org/2067143004/diff/80001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:810: base::ThreadTaskRunnerHandle::Get()->PostTask( This PostTask call is repeated in OnDeleteOldPagesWithSameURL(). It'd be nice to extract it into separate method to show clearly that it's the same step.
dimich@ PTAL, updated addressing comments. https://codereview.chromium.org/2067143004/diff/80001/components/offline_page... File components/offline_pages/client_policy_controller.cc (right): https://codereview.chromium.org/2067143004/diff/80001/components/offline_page... components/offline_pages/client_policy_controller.cc:25: base::TimeDelta::FromDays(2), 20, 1))); On 2016/06/23 04:23:14, Dmitry Titov wrote: > I know it's not in this checkin, but why 20 instead of kUnlimitedPages for > last_n? The spec seems to go away from fixed 'N' and rely solely on expiration. Ahh yes, I just thought there was kind of a 'limit' for last-n namespace. https://codereview.chromium.org/2067143004/diff/80001/components/offline_page... File components/offline_pages/offline_page_client_policy.h (right): https://codereview.chromium.org/2067143004/diff/80001/components/offline_page... components/offline_pages/offline_page_client_policy.h:52: // How many pages are allowed for an URL. On 2016/06/23 04:23:15, Dmitry Titov wrote: > It looks the comment can be made clearer - how about "How many pages for the > same online URL can be stored at any time"? Done. https://codereview.chromium.org/2067143004/diff/80001/components/offline_page... File components/offline_pages/offline_page_model_impl.cc (right): https://codereview.chromium.org/2067143004/diff/80001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:799: // Remove other user initiated pages with same url if the new save request was On 2016/06/23 04:23:15, Dmitry Titov wrote: > this comment can be re-phrsed without "user initiated" but with mentioning of > policy. Done. https://codereview.chromium.org/2067143004/diff/80001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:803: if (result == SavePageResult::SUCCESS && pages_allowed > 0) { On 2016/06/23 04:23:15, Dmitry Titov wrote: > pages_allowed > 0 looks strange here. > > If it can never be <=0 and you guard against invalid initialization of policies, > then it's better to do it in the policy controller, possibly clamping it to a > valid range. > > If <=0 can be valid values and mean something (looking at this code, I can start > thinking that it means "no limit") then this should be documented in the policy > class. Documented in the client policy class, and since the type is size_t, I don't think it will be <0. But made some changes to make it looks more reasonable. https://codereview.chromium.org/2067143004/diff/80001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:810: base::ThreadTaskRunnerHandle::Get()->PostTask( On 2016/06/23 04:23:15, Dmitry Titov wrote: > This PostTask call is repeated in OnDeleteOldPagesWithSameURL(). It'd be nice to > extract it into separate method to show clearly that it's the same step. Done.
lgtm
The CQ bit was checked by romax@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2067143004/#ps100001 (title: "comments from dimich@.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2067143004/100001
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] Duplicate pages when save/delete bookmarks. There were duplicates since when saving bookmarks we'll not check if pages with same url has been saved already, which was caused since we were deleting offline pages associated with bookmarks before. Hence changing it to the same logic as Last N, find pages with same url, delete, then save new pages. BUG=619233 ========== to ========== [Offline Pages] Duplicate pages when save/delete bookmarks. There were duplicates since when saving bookmarks we'll not check if pages with same url has been saved already, which was caused since we were deleting offline pages associated with bookmarks before. Hence changing it to the same logic as Last N, find pages with same url, delete, then save new pages. BUG=619233 Committed: https://crrev.com/af225ed9d44ab001730c795228af5c756595a7a6 Cr-Commit-Position: refs/heads/master@{#401798} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/af225ed9d44ab001730c795228af5c756595a7a6 Cr-Commit-Position: refs/heads/master@{#401798}
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Duplicate pages when save/delete bookmarks. There were duplicates since when saving bookmarks we'll not check if pages with same url has been saved already, which was caused since we were deleting offline pages associated with bookmarks before. Hence changing it to the same logic as Last N, find pages with same url, delete, then save new pages. BUG=619233 Committed: https://crrev.com/af225ed9d44ab001730c795228af5c756595a7a6 Cr-Commit-Position: refs/heads/master@{#401798} ========== to ========== [Offline Pages] Duplicate pages when save/delete bookmarks. There were duplicates since when saving bookmarks we'll not check if pages with same url has been saved already, which was caused since we were deleting offline pages associated with bookmarks before. Hence changing it to the same logic as Last N, find pages with same url, delete, then save new pages. BUG=619223 Committed: https://crrev.com/af225ed9d44ab001730c795228af5c756595a7a6 Cr-Commit-Position: refs/heads/master@{#401798} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
