Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2455503004:
[MD History] Fix deletion in heavily scrolled list. (Closed)
|
Created:
4 years, 1 month ago by calamity Modified:
4 years, 1 month ago CC:
chromium-reviews, asanka, Patrick Dubroy, michaelpg+watch-md-ui_chromium.org, dbeam+watch-history_chromium.org, pam+watch_chromium.org, arv+watch_chromium.org, dbeam+watch-downloads_chromium.org, chrome-apps-syd-reviews_chromium.org, lshang, msramek Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[MD History] Fix deletion in heavily scrolled list.
This CL fixes an issue where results would reload when an item was
deleted in a history list that was scrolled beyond the first 'page'.
This was happening because code that refreshed the results on external
deletion was getting triggered for internal deletes due to the web
history service.
This has been fixed by disabling the reload when web history
deletions are occurring, bringing it in line with the local history
service.
BUG=660267, 630164
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/a1ee6c76c90bfbe3cd842ac6d68cca5a6364738d
Cr-Commit-Position: refs/heads/master@{#429529}
Patch Set 1 #Patch Set 2 : prevent web history reloads #Patch Set 3 : add test #
Total comments: 1
Messages
Total messages: 28 (15 generated)
Description was changed from ========== [MD History] Fix deletion in heavily scrolled list. This CL fixes an issue where results would reload when an item was deleted in a history list that was scrolled beyond the first 'page'. This was happening because code that refreshed the results on deletion was not taking into account the extra results that were loaded. BUG= ========== to ========== [MD History] Fix deletion in heavily scrolled list. This CL fixes an issue where results would reload when an item was deleted in a history list that was scrolled beyond the first 'page'. This was happening because code that refreshed the results on deletion was not taking into account the extra results that were loaded. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== [MD History] Fix deletion in heavily scrolled list. This CL fixes an issue where results would reload when an item was deleted in a history list that was scrolled beyond the first 'page'. This was happening because code that refreshed the results on deletion was not taking into account the extra results that were loaded. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD History] Fix deletion in heavily scrolled list. This CL fixes an issue where results would reload when an item was deleted in a history list that was scrolled beyond the first 'page'. This was happening because code that refreshed the results on deletion was not taking into account the extra results that were loaded. BUG=660267 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== [MD History] Fix deletion in heavily scrolled list. This CL fixes an issue where results would reload when an item was deleted in a history list that was scrolled beyond the first 'page'. This was happening because code that refreshed the results on deletion was not taking into account the extra results that were loaded. BUG=660267 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD History] Fix deletion in heavily scrolled list. This CL fixes an issue where results would reload when an item was deleted in a history list that was scrolled beyond the first 'page'. This was happening because code that refreshed the results on deletion was not taking into account the extra results that were loaded. This has been fixed by disabling the reload, meaning that deleting via Clear Browsing Data will not update data in the history page. BUG=660267, 630164 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
calamity@chromium.org changed reviewers: + dbeam@chromium.org
We'll probably want to find another way to deal with this, possibly by sending all deleted items through in the historyDeleted callback. Unfortunately, web history deletion doesn't tell us what rows went away, but maybe we can live with that. Another option is to requery for as many results as are currently shown, but with the focus issues in iron-list right now, it seems better to just ignore external deletions for now.
but wait, doesn't this regress the other thing this fixed?
On 2016/10/28 03:42:55, Dan Beam wrote: > but wait, doesn't this regress the other thing this fixed? Yeahhhhhhhhhh. I think we should just take that hit since this is also going to cause well-scrolled history pages to jump around whenever any synced history deletes happen. We need to rethink the approach to live-updating external deletes.
but without this, it means that you could've cleared like a ton of history ... then come back to a stale history page (where you're unsure whether you deleted something or not). does clicking (X) on a removed history item work? I don't generally like this. why can't we just nuke the whole page and select the item at the top of the list?
Description was changed from ========== [MD History] Fix deletion in heavily scrolled list. This CL fixes an issue where results would reload when an item was deleted in a history list that was scrolled beyond the first 'page'. This was happening because code that refreshed the results on deletion was not taking into account the extra results that were loaded. This has been fixed by disabling the reload, meaning that deleting via Clear Browsing Data will not update data in the history page. BUG=660267, 630164 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD History] Fix deletion in heavily scrolled list. This CL fixes an issue where results would reload when an item was deleted in a history list that was scrolled beyond the first 'page'. This was happening because code that refreshed the results on external deletion was getting triggered for internal deletes due to the web history service. This has been fixed by disabling the reload when web history deletions are occurring, bring it in line with the local history service which has this check. BUG=660267, 630164 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== [MD History] Fix deletion in heavily scrolled list. This CL fixes an issue where results would reload when an item was deleted in a history list that was scrolled beyond the first 'page'. This was happening because code that refreshed the results on external deletion was getting triggered for internal deletes due to the web history service. This has been fixed by disabling the reload when web history deletions are occurring, bring it in line with the local history service which has this check. BUG=660267, 630164 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD History] Fix deletion in heavily scrolled list. This CL fixes an issue where results would reload when an item was deleted in a history list that was scrolled beyond the first 'page'. This was happening because code that refreshed the results on external deletion was getting triggered for internal deletes due to the web history service. This has been fixed by disabling the reload when web history deletions are occurring, bringing it in line with the local history service. BUG=660267, 630164 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
On 2016/10/28 22:28:14, Dan Beam wrote: > but without this, it means that you could've cleared like a ton of history ... > then come back to a stale history page (where you're unsure whether you deleted > something or not). > > does clicking (X) on a removed history item work? > > I don't generally like this. why can't we just nuke the whole page and select > the item at the top of the list? Okay, here's a more palatable solution. The local history does the right thing and blocks the reload on history-page triggered deletes. This roughly achieves the same behavior for the web history service.
lgtm if components/history owners are OK with this can you write a test to ensure the order?
calamity@chromium.org changed reviewers: + sdefresne@chromium.org
+sdefresne for web_history_service.cc OWNERS +FYI msramek PTAL, thanks!
lgtm
The CQ bit was checked by calamity@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2455503004/#ps60001 (title: "add test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by calamity@chromium.org
The CQ bit was checked by calamity@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [MD History] Fix deletion in heavily scrolled list. This CL fixes an issue where results would reload when an item was deleted in a history list that was scrolled beyond the first 'page'. This was happening because code that refreshed the results on external deletion was getting triggered for internal deletes due to the web history service. This has been fixed by disabling the reload when web history deletions are occurring, bringing it in line with the local history service. BUG=660267, 630164 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD History] Fix deletion in heavily scrolled list. This CL fixes an issue where results would reload when an item was deleted in a history list that was scrolled beyond the first 'page'. This was happening because code that refreshed the results on external deletion was getting triggered for internal deletes due to the web history service. This has been fixed by disabling the reload when web history deletions are occurring, bringing it in line with the local history service. BUG=660267, 630164 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [MD History] Fix deletion in heavily scrolled list. This CL fixes an issue where results would reload when an item was deleted in a history list that was scrolled beyond the first 'page'. This was happening because code that refreshed the results on external deletion was getting triggered for internal deletes due to the web history service. This has been fixed by disabling the reload when web history deletions are occurring, bringing it in line with the local history service. BUG=660267, 630164 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD History] Fix deletion in heavily scrolled list. This CL fixes an issue where results would reload when an item was deleted in a history list that was scrolled beyond the first 'page'. This was happening because code that refreshed the results on external deletion was getting triggered for internal deletes due to the web history service. This has been fixed by disabling the reload when web history deletions are occurring, bringing it in line with the local history service. BUG=660267, 630164 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/a1ee6c76c90bfbe3cd842ac6d68cca5a6364738d Cr-Commit-Position: refs/heads/master@{#429529} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a1ee6c76c90bfbe3cd842ac6d68cca5a6364738d Cr-Commit-Position: refs/heads/master@{#429529}
Message was sent while issue was closed.
msramek@chromium.org changed reviewers: + msramek@chromium.org
Message was sent while issue was closed.
I just returned from OOO, so I'm adding a retrospective LGTM and an improvement request. Thanks for fixing! https://codereview.chromium.org/2455503004/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/browsing_history_handler.cc (right): https://codereview.chromium.org/2455503004/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/browsing_history_handler.cc:1013: if (!has_pending_delete_request_) It's possible that OnWebHistoryDeleted() was called for a request not coming from this page (e.g. from CBD or another instance of the history page). In that case, we should reload to display the effects of that deletion. It's just a corner case, and it requires further changes to OnWebHistoryDeleted(), so I'm not asking you to fix it now, but can you please document that we know about it? Someone will look at this code later and will wonder why can we assume that the web history deletion comes from this handler. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
