Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1367063004:
Support undoing offline page deletion (Closed)
DescriptionSupport undoing offline page deletion
For a bookmark with offline page, the offline metadata is persisted in a
different data store that is keyed by bookmark_id. To support undoing
offline page deletion, we need to do the following:
1) Postpone deleting the offline page metadata (and hence offline file) until
some time after the duration of snackbar which is the only place Undo is
invoked for enhanced bookmarks.
2) When the bookmark node is restored, we need to update the offline page metadata to remove it from being deleted.
BUG=491352
TEST=new test added
Committed: https://crrev.com/d95ab36868115a5511f13d80abdc9fe8b5df29ff
Cr-Commit-Position: refs/heads/master@{#354183}
Patch Set 1 #
Total comments: 23
Patch Set 2 : Address sky's feedback #Patch Set 3 : Address Filip's feedback #
Total comments: 4
Patch Set 4 : Rework based on undoing bookmark deletion without new ID #
Total comments: 6
Patch Set 5 : Address feedback #Patch Set 6 : Add the call to removeObserver #
Total comments: 4
Patch Set 7 : One more change #
Total comments: 4
Messages
Total messages: 31 (5 generated)
jianli@chromium.org changed reviewers: + fgorski@chromium.org, sky@chromium.org
https://codereview.chromium.org/1367063004/diff/1/components/bookmarks/browse... File components/bookmarks/browser/bookmark_model.cc (right): https://codereview.chromium.org/1367063004/diff/1/components/bookmarks/browse... components/bookmarks/browser/bookmark_model.cc:1051: void BookmarkModel::OnBookmarkRenumbered(int64 old_id, int64 new_id) { Make position of declaration/definition match. https://codereview.chromium.org/1367063004/diff/1/components/bookmarks/browse... File components/bookmarks/browser/bookmark_model.h (right): https://codereview.chromium.org/1367063004/diff/1/components/bookmarks/browse... components/bookmarks/browser/bookmark_model.h:308: void OnBookmarkRenumbered(int64 old_id, int64 new_id); This shouldn't be public. https://codereview.chromium.org/1367063004/diff/1/components/bookmarks/browse... File components/bookmarks/browser/bookmark_model_observer.h (right): https://codereview.chromium.org/1367063004/diff/1/components/bookmarks/browse... components/bookmarks/browser/bookmark_model_observer.h:142: BookmarkModel* model, int64 old_id, int64 new_id) {} int64_t on these. And this description is a bit confusing. You need to clarify it. Can you outline how you are using this function? This function is confusing because it assumes you're keeping track of ids as they are changed. Why do you need that?
https://codereview.chromium.org/1367063004/diff/1/components/bookmarks/browse... File components/bookmarks/browser/bookmark_model.cc (right): https://codereview.chromium.org/1367063004/diff/1/components/bookmarks/browse... components/bookmarks/browser/bookmark_model.cc:1051: void BookmarkModel::OnBookmarkRenumbered(int64 old_id, int64 new_id) { On 2015/09/25 15:54:44, sky wrote: > Make position of declaration/definition match. Done. https://codereview.chromium.org/1367063004/diff/1/components/bookmarks/browse... File components/bookmarks/browser/bookmark_model.h (right): https://codereview.chromium.org/1367063004/diff/1/components/bookmarks/browse... components/bookmarks/browser/bookmark_model.h:308: void OnBookmarkRenumbered(int64 old_id, int64 new_id); On 2015/09/25 15:54:44, sky wrote: > This shouldn't be public. This method needs to be called from BookmarkUndoService::OnBookmarkRenumbered in order to have the model to send out the event. https://codereview.chromium.org/1367063004/diff/1/components/bookmarks/browse... File components/bookmarks/browser/bookmark_model_observer.h (right): https://codereview.chromium.org/1367063004/diff/1/components/bookmarks/browse... components/bookmarks/browser/bookmark_model_observer.h:142: BookmarkModel* model, int64 old_id, int64 new_id) {} On 2015/09/25 15:54:44, sky wrote: > int64_t on these. And this description is a bit confusing. You need to clarify > it. > Can you outline how you are using this function? This function is confusing > because it assumes you're keeping track of ids as they are changed. Why do you > need that? Changed to use int64_t and updated the comment. This is needed to support undoing deleting a bookmark with offline page saved. For a bookmark with offline page, the offline metadata is persisted in a different data store that is keyed by bookmark_id. When the undo occurs, the new bookmark (with new ID) is cloned from the deleted one. To support restoring the offline data, we need to do the following 2 things: 1) Postpone deleting the offline metadata (and hence offline file) by marking the offline metadata for deletion. 2) When the new bookmark corresponding to the restored old one is created, we need to update the bookmark id used in the offline metadata. Thus we need to know about the renumber event.
https://codereview.chromium.org/1367063004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageStorageSpacePolicy.java (right): https://codereview.chromium.org/1367063004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageStorageSpacePolicy.java:42: return true; /*getSizeOfAllPages() > MINIMUM_TOTAL_SIZE_BYTES Is this correct? https://codereview.chromium.org/1367063004/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_model.cc (right): https://codereview.chromium.org/1367063004/diff/1/components/offline_pages/of... components/offline_pages/offline_page_model.cc:151: offline_page_item.flags = static_cast<OfflinePageItem::Flags>( create MarkForDeletion method in OfflinePageItem https://codereview.chromium.org/1367063004/diff/1/components/offline_pages/of... components/offline_pages/offline_page_model.cc:326: // No need to report UMA for the undo operation. Why? https://codereview.chromium.org/1367063004/diff/1/components/offline_pages/of... components/offline_pages/offline_page_model.cc:403: // If there're left over pages that are marked for deletion, but not yet there are pages https://codereview.chromium.org/1367063004/diff/1/components/offline_pages/of... components/offline_pages/offline_page_model.cc:404: // deleted and Chrome restarts. Do it now. and service is restarted, delete the pages now. https://codereview.chromium.org/1367063004/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_model.h (right): https://codereview.chromium.org/1367063004/diff/1/components/offline_pages/of... components/offline_pages/offline_page_model.h:188: int64 old_bookmark_id, nit: align https://codereview.chromium.org/1367063004/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_model_unittest.cc (right): https://codereview.chromium.org/1367063004/diff/1/components/offline_pages/of... components/offline_pages/offline_page_model_unittest.cc:541: TEST_F(OfflinePageModelTest, MarkPageForDeletion) { is there a test covering the actual deletion after the delay?
https://codereview.chromium.org/1367063004/diff/1/components/bookmarks/browse... File components/bookmarks/browser/bookmark_model_observer.h (right): https://codereview.chromium.org/1367063004/diff/1/components/bookmarks/browse... components/bookmarks/browser/bookmark_model_observer.h:142: BookmarkModel* model, int64 old_id, int64 new_id) {} On 2015/09/25 20:29:24, jianli wrote: > On 2015/09/25 15:54:44, sky wrote: > > int64_t on these. And this description is a bit confusing. You need to clarify > > it. > > Can you outline how you are using this function? This function is confusing > > because it assumes you're keeping track of ids as they are changed. Why do you > > need that? > > Changed to use int64_t and updated the comment. > > This is needed to support undoing deleting a bookmark with offline page saved. > For a bookmark with offline page, the offline metadata is persisted in a > different data store that is keyed by bookmark_id. When the undo occurs, the new > bookmark (with new ID) is cloned from the deleted one. To support restoring the > offline data, we need to do the following 2 things: > 1) Postpone deleting the offline metadata (and hence offline file) by marking > the offline metadata for deletion. > 2) When the new bookmark corresponding to the restored old one is created, we > need to update the bookmark id used in the offline metadata. Thus we need to > know about the renumber event. I think bookmark undo should be changed such that it adds back the old item. This way you don't need to do anything here. This change should be done separately.
https://codereview.chromium.org/1367063004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageStorageSpacePolicy.java (right): https://codereview.chromium.org/1367063004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageStorageSpacePolicy.java:42: return true; /*getSizeOfAllPages() > MINIMUM_TOTAL_SIZE_BYTES On 2015/09/25 20:30:50, fgorski wrote: > Is this correct? Sorry, I forgot to remove this testing change. https://codereview.chromium.org/1367063004/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_model.cc (right): https://codereview.chromium.org/1367063004/diff/1/components/offline_pages/of... components/offline_pages/offline_page_model.cc:151: offline_page_item.flags = static_cast<OfflinePageItem::Flags>( On 2015/09/25 20:30:50, fgorski wrote: > create MarkForDeletion method in OfflinePageItem Done. https://codereview.chromium.org/1367063004/diff/1/components/offline_pages/of... components/offline_pages/offline_page_model.cc:326: // No need to report UMA for the undo operation. On 2015/09/25 20:30:50, fgorski wrote: > Why? I meant not do report the UMA like in OnAddOfflinePageDone. But probably we want to report something else. Removed this comment. https://codereview.chromium.org/1367063004/diff/1/components/offline_pages/of... components/offline_pages/offline_page_model.cc:403: // If there're left over pages that are marked for deletion, but not yet On 2015/09/25 20:30:50, fgorski wrote: > there are pages Done. https://codereview.chromium.org/1367063004/diff/1/components/offline_pages/of... components/offline_pages/offline_page_model.cc:404: // deleted and Chrome restarts. Do it now. On 2015/09/25 20:30:50, fgorski wrote: > and service is restarted, delete the pages now. Done. https://codereview.chromium.org/1367063004/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_model.h (right): https://codereview.chromium.org/1367063004/diff/1/components/offline_pages/of... components/offline_pages/offline_page_model.h:188: int64 old_bookmark_id, On 2015/09/25 20:30:50, fgorski wrote: > nit: align Done. https://codereview.chromium.org/1367063004/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_model_unittest.cc (right): https://codereview.chromium.org/1367063004/diff/1/components/offline_pages/of... components/offline_pages/offline_page_model_unittest.cc:541: TEST_F(OfflinePageModelTest, MarkPageForDeletion) { On 2015/09/25 20:30:50, fgorski wrote: > is there a test covering the actual deletion after the delay? It is hard to test this without switching to using some TaskRunner that could advance the clock but that one does not work well with RunLoop. I will do some investigation and might add a test for this in other patch.
lgtm with comments https://codereview.chromium.org/1367063004/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_model_unittest.cc (right): https://codereview.chromium.org/1367063004/diff/1/components/offline_pages/of... components/offline_pages/offline_page_model_unittest.cc:541: TEST_F(OfflinePageModelTest, MarkPageForDeletion) { On 2015/09/25 20:52:36, jianli wrote: > On 2015/09/25 20:30:50, fgorski wrote: > > is there a test covering the actual deletion after the delay? > > It is hard to test this without switching to using some TaskRunner that could > advance the clock but that one does not work well with RunLoop. I will do some > investigation and might add a test for this in other patch. ok, please add a task for that and we can do it in M48 https://codereview.chromium.org/1367063004/diff/40001/components/offline_page... File components/offline_pages/offline_page_model.cc (right): https://codereview.chromium.org/1367063004/diff/40001/components/offline_page... components/offline_pages/offline_page_model.cc:403: // OfflinePageModel gets reloaded. Delete the pages now. Your first sentence does not conclude. How about: If there were pages pending deletion when OfflinePageModel was reloaded, they will be deleted now. https://codereview.chromium.org/1367063004/diff/40001/components/offline_page... File components/offline_pages/offline_page_model.h (right): https://codereview.chromium.org/1367063004/diff/40001/components/offline_page... components/offline_pages/offline_page_model.h:188: int64 old_bookmark_id, fix types of bookmark_id to int64_t
https://codereview.chromium.org/1367063004/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_model_unittest.cc (right): https://codereview.chromium.org/1367063004/diff/1/components/offline_pages/of... components/offline_pages/offline_page_model_unittest.cc:541: TEST_F(OfflinePageModelTest, MarkPageForDeletion) { On 2015/09/25 21:23:56, fgorski wrote: > On 2015/09/25 20:52:36, jianli wrote: > > On 2015/09/25 20:30:50, fgorski wrote: > > > is there a test covering the actual deletion after the delay? > > > > It is hard to test this without switching to using some TaskRunner that could > > advance the clock but that one does not work well with RunLoop. I will do some > > investigation and might add a test for this in other patch. > > ok, please add a task for that and we can do it in M48 Done. https://codereview.chromium.org/1367063004/diff/40001/components/offline_page... File components/offline_pages/offline_page_model.cc (right): https://codereview.chromium.org/1367063004/diff/40001/components/offline_page... components/offline_pages/offline_page_model.cc:403: // OfflinePageModel gets reloaded. Delete the pages now. On 2015/09/25 21:23:57, fgorski wrote: > Your first sentence does not conclude. How about: > > If there were pages pending deletion when OfflinePageModel was reloaded, they > will be deleted now. Done. https://codereview.chromium.org/1367063004/diff/40001/components/offline_page... File components/offline_pages/offline_page_model.h (right): https://codereview.chromium.org/1367063004/diff/40001/components/offline_page... components/offline_pages/offline_page_model.h:188: int64 old_bookmark_id, On 2015/09/25 21:23:57, fgorski wrote: > fix types of bookmark_id to int64_t This is not longer needed.
On 2015/09/25 20:38:57, sky wrote: > https://codereview.chromium.org/1367063004/diff/1/components/bookmarks/browse... > File components/bookmarks/browser/bookmark_model_observer.h (right): > > https://codereview.chromium.org/1367063004/diff/1/components/bookmarks/browse... > components/bookmarks/browser/bookmark_model_observer.h:142: BookmarkModel* > model, int64 old_id, int64 new_id) {} > On 2015/09/25 20:29:24, jianli wrote: > > On 2015/09/25 15:54:44, sky wrote: > > > int64_t on these. And this description is a bit confusing. You need to > clarify > > > it. > > > Can you outline how you are using this function? This function is confusing > > > because it assumes you're keeping track of ids as they are changed. Why do > you > > > need that? > > > > Changed to use int64_t and updated the comment. > > > > This is needed to support undoing deleting a bookmark with offline page saved. > > For a bookmark with offline page, the offline metadata is persisted in a > > different data store that is keyed by bookmark_id. When the undo occurs, the > new > > bookmark (with new ID) is cloned from the deleted one. To support restoring > the > > offline data, we need to do the following 2 things: > > 1) Postpone deleting the offline metadata (and hence offline file) by marking > > the offline metadata for deletion. > > 2) When the new bookmark corresponding to the restored old one is created, we > > need to update the bookmark id used in the offline metadata. Thus we need to > > know about the renumber event. > > I think bookmark undo should be changed such that it adds back the old item. > This way you don't need to do anything here. This change should be done > separately. This patch has been updated based on the landed work that restored the removed bookmark with issuing new ID.
sky@chromium.org changed reviewers: - sky@chromium.org
I don't believe you need me anymore: -sky
Please make sure to pass a result of undoing (canceled) to the caller. Please make sure Saved offline filter gets refreshed. https://codereview.chromium.org/1367063004/diff/60001/components/offline_page... File components/offline_pages/offline_page_item.cc (right): https://codereview.chromium.org/1367063004/diff/60001/components/offline_page... components/offline_pages/offline_page_item.cc:63: flags = static_cast<Flags>(static_cast<int>(flags) | MARKED_FOR_DELETION); I am not sure what is different about this line and 59 in terms of static_casting the MARKED_FOR_DELETION part. https://codereview.chromium.org/1367063004/diff/60001/components/offline_page... File components/offline_pages/offline_page_model.cc (right): https://codereview.chromium.org/1367063004/diff/60001/components/offline_page... components/offline_pages/offline_page_model.cc:324: offline_pages_[offline_page.bookmark_id] = offline_page; you are missing a callback that will tell the caller that the deletion did not go through DeletePageResult::CANCELED https://codereview.chromium.org/1367063004/diff/60001/components/offline_page... File components/offline_pages/offline_page_model_unittest.cc (right): https://codereview.chromium.org/1367063004/diff/60001/components/offline_page... components/offline_pages/offline_page_model_unittest.cc:559: EXPECT_EQ(0UL, offline_pages.size()); Can you add a piece of test for when the bookmark is undeleted?
https://codereview.chromium.org/1367063004/diff/60001/components/offline_page... File components/offline_pages/offline_page_item.cc (right): https://codereview.chromium.org/1367063004/diff/60001/components/offline_page... components/offline_pages/offline_page_item.cc:63: flags = static_cast<Flags>(static_cast<int>(flags) | MARKED_FOR_DELETION); On 2015/10/12 20:59:52, fgorski wrote: > I am not sure what is different about this line and 59 in terms of > static_casting the MARKED_FOR_DELETION part. Remove unneeded static_cast at line 59. https://codereview.chromium.org/1367063004/diff/60001/components/offline_page... File components/offline_pages/offline_page_model.cc (right): https://codereview.chromium.org/1367063004/diff/60001/components/offline_page... components/offline_pages/offline_page_model.cc:324: offline_pages_[offline_page.bookmark_id] = offline_page; On 2015/10/12 20:59:52, fgorski wrote: > you are missing a callback that will tell the caller that the deletion did not > go through DeletePageResult::CANCELED We don't need to fire a callback since it has already been done at line 306. When undo is done, the observer will kick in to update the UI. https://codereview.chromium.org/1367063004/diff/60001/components/offline_page... File components/offline_pages/offline_page_model_unittest.cc (right): https://codereview.chromium.org/1367063004/diff/60001/components/offline_page... components/offline_pages/offline_page_model_unittest.cc:559: EXPECT_EQ(0UL, offline_pages.size()); On 2015/10/12 20:59:52, fgorski wrote: > Can you add a piece of test for when the bookmark is undeleted? Done.
Also added the logic to refresh offline page view when something is changed.
jianli@chromium.org changed reviewers: + ianwen@chromium.org
ianwen, could you please review enhanced bookmark changes?
https://codereview.chromium.org/1367063004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java (right): https://codereview.chromium.org/1367063004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java:278: mOfflinePageModelObserver = new OfflinePageModelObserver() { Q: why a model observer is added to this class? I suppose undo should only need to live in model layer?
https://codereview.chromium.org/1367063004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java (right): https://codereview.chromium.org/1367063004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java:278: mOfflinePageModelObserver = new OfflinePageModelObserver() { On 2015/10/13 00:35:04, Ian Wen wrote: > Q: why a model observer is added to this class? I suppose undo should only need > to live in model layer? The model layer doesn't know how to trigger updating the UI. When a bookmark with offline copy is being restored from the deletion, we need to update the bookmark page, esp when it shows "Saved offline" view. The "Saved offline" view will only list all bookmark items that have offline copies. When an offline copy is restored, it will take some time to update the offline metadata store and only after this "Saved offline" view can see the restored offline item. This is the reason we need to listen to offline model change event and refresh the UI here.
lgtm https://codereview.chromium.org/1367063004/diff/100001/components/offline_pag... File components/offline_pages/offline_page_item.cc (right): https://codereview.chromium.org/1367063004/diff/100001/components/offline_pag... components/offline_pages/offline_page_item.cc:68: static_cast<int>(flags) & ~(static_cast<int>(MARKED_FOR_DELETION))); what about here? Is the cast needed here or not?
https://codereview.chromium.org/1367063004/diff/100001/components/offline_pag... File components/offline_pages/offline_page_item.cc (right): https://codereview.chromium.org/1367063004/diff/100001/components/offline_pag... components/offline_pages/offline_page_item.cc:68: static_cast<int>(flags) & ~(static_cast<int>(MARKED_FOR_DELETION))); On 2015/10/13 16:48:25, fgorski wrote: > what about here? Is the cast needed here or not? Done.
https://chromiumcodereview.appspot.com/1367063004/diff/120001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java (right): https://chromiumcodereview.appspot.com/1367063004/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java:281: mDelegate.notifyStateChange(EnhancedBookmarkItemsAdapter.this); 1. Is offline model change capable of removing/adding bookmarks? If not, I would use notifyDataSetChanged() here. EBDelegate#notifyStateChange() will let bookmark model regenerate the list of bookmarks to show. Instead, notifyDataSetChange() only tells recycler view to relayout all items. 2. It seems offlinePageModelChanged() might be called after offlinepage model is loaded. If we iniatialize the offline page during EB's initialization, we might end up having to refresh this adapter twice (once for EB's loading, once for offline page). Are there better designs to prevent this refresh-twice problem?
https://codereview.chromium.org/1367063004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java (right): https://codereview.chromium.org/1367063004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java:281: mDelegate.notifyStateChange(EnhancedBookmarkItemsAdapter.this); On 2015/10/14 23:30:01, Ian Wen wrote: > 1. Is offline model change capable of removing/adding bookmarks? If not, I would > use notifyDataSetChanged() here. > > EBDelegate#notifyStateChange() will let bookmark model regenerate the list of > bookmarks to show. Instead, notifyDataSetChange() only tells recycler view to > relayout all items. We need to call notifyStateChange, since relayout provided by notifyDataSetChange is not enough because the removed items will reappear here. > > 2. It seems offlinePageModelChanged() might be called after offlinepage model is > loaded. If we iniatialize the offline page during EB's initialization, we might > end up having to refresh this adapter twice (once for EB's loading, once for > offline page). Are there better designs to prevent this refresh-twice problem? OfflinePageModelChanged will only be fired after the model is loaded and there is a request to add/remove/update offline page items, mostly due to user action. So it means that we do need to refresh the UI per the user action for most of the cases. In the rare case of bookmark sync, this is likely but probably we don't want to handle it.
https://codereview.chromium.org/1367063004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java (right): https://codereview.chromium.org/1367063004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java:281: mDelegate.notifyStateChange(EnhancedBookmarkItemsAdapter.this); On 2015/10/14 23:40:48, jianli wrote: > On 2015/10/14 23:30:01, Ian Wen wrote: > > 1. Is offline model change capable of removing/adding bookmarks? If not, I > would > > use notifyDataSetChanged() here. > > > > EBDelegate#notifyStateChange() will let bookmark model regenerate the list of > > bookmarks to show. Instead, notifyDataSetChange() only tells recycler view to > > relayout all items. > > We need to call notifyStateChange, since relayout provided by > notifyDataSetChange is not enough because the removed items will reappear here. > > > > > > 2. It seems offlinePageModelChanged() might be called after offlinepage model > is > > loaded. If we iniatialize the offline page during EB's initialization, we > might > > end up having to refresh this adapter twice (once for EB's loading, once for > > offline page). Are there better designs to prevent this refresh-twice problem? > > OfflinePageModelChanged will only be fired after the model is loaded and there > is a request to add/remove/update offline page items, mostly due to user action. > So it means that we do need to refresh the UI per the user action for most of > the cases. In the rare case of bookmark sync, this is likely but probably we > don't want to handle it. Hmm I saw another CL that adds delete signals to the model observer. If delete is handled specifically, are there other reasons that we want to use notifyStateChange()?
https://codereview.chromium.org/1367063004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java (right): https://codereview.chromium.org/1367063004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java:281: mDelegate.notifyStateChange(EnhancedBookmarkItemsAdapter.this); On 2015/10/15 00:02:16, Ian Wen wrote: > On 2015/10/14 23:40:48, jianli wrote: > > On 2015/10/14 23:30:01, Ian Wen wrote: > > > 1. Is offline model change capable of removing/adding bookmarks? If not, I > > would > > > use notifyDataSetChanged() here. > > > > > > EBDelegate#notifyStateChange() will let bookmark model regenerate the list > of > > > bookmarks to show. Instead, notifyDataSetChange() only tells recycler view > to > > > relayout all items. > > > > We need to call notifyStateChange, since relayout provided by > > notifyDataSetChange is not enough because the removed items will reappear > here. > > > > > > > > > > 2. It seems offlinePageModelChanged() might be called after offlinepage > model > > is > > > loaded. If we iniatialize the offline page during EB's initialization, we > > might > > > end up having to refresh this adapter twice (once for EB's loading, once for > > > offline page). Are there better designs to prevent this refresh-twice > problem? > > > > OfflinePageModelChanged will only be fired after the model is loaded and there > > is a request to add/remove/update offline page items, mostly due to user > action. > > So it means that we do need to refresh the UI per the user action for most of > > the cases. In the rare case of bookmark sync, this is likely but probably we > > don't want to handle it. > > Hmm I saw another CL that adds delete signals to the model observer. If delete > is handled specifically, are there other reasons that we want to use > notifyStateChange()? Yes, we need it for undoing the bookmark deletion that will also bring the offline page back.
lgtm. My previous concern was that we might trigger onModelChanged() in the process of onModelLoaded(), which means we refresh the UI twice. Yet after offline discussion Jian helped me figure out in offline_page_model, even if finalizedDelete() is called in doneLoad(), a java observer call won't happen.
The CQ bit was checked by jianli@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/1367063004/#ps120001 (title: "One more change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1367063004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1367063004/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/d95ab36868115a5511f13d80abdc9fe8b5df29ff Cr-Commit-Position: refs/heads/master@{#354183} |
