Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2263613002:
Make BrowsingDataHandler observe WebHistoryService deletions (Closed)
|
Created:
4 years, 4 months ago by msramek Modified:
4 years, 3 months ago CC:
chromium-reviews, Patrick Dubroy, dbeam+watch-history_chromium.org, pam+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake BrowsingDataHandler observe WebHistoryService deletions
BrowsingDataHandler observes local history deletions, but not synced
history deletions. In cases where there are no local history entries
to delete, it can happen that the history page does not reload and it
looks as if the deletion failed.
See https://docs.google.com/document/d/1Fd6CdBf6UMbYbkwSjEKyFOxew0Xid5IaT-QwnFchjig/
for background.
This CL
1. Adds an Observer subclass to the WebHistoryService.
2. Registers BrowsingHistoryHandler as a WebHistoryService::Observer;
and since WebHistoryService's existence is based on whether history
sync is enabled, we also register as a SyncServiceObserver.
3. Adds a test to browsing_history_handler_unittest.cc.
Also tested manually on Android - seems to solve the problem described
in the above mentioned document.
BUG=604114, 630164
Committed: https://crrev.com/d39070b1ac01776e5a3800a9ef55fe43dce74225
Cr-Commit-Position: refs/heads/master@{#414679}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fixed the test. #Patch Set 3 : Fix Android compilation. #
Total comments: 11
Patch Set 4 : Addressed comments. #
Total comments: 4
Patch Set 5 : Moved comments inside 'if'. #Messages
Total messages: 49 (30 generated)
The CQ bit was checked by msramek@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...
Description was changed from ========== Make BrowsingDataHandler observer WebHistoryService deletions BrowsingDataHandler observes local history deletions, but not synced history deletions. In cases where there are no local history entries to delete, it can happen that the history page does not reload and it looks as if the deletion failed. See https://docs.google.com/document/d/1Fd6CdBf6UMbYbkwSjEKyFOxew0Xid5IaT-QwnFchjig/ for more details. BUG=604114,630164 ========== to ========== Make BrowsingDataHandler observe WebHistoryService deletions BrowsingDataHandler observes local history deletions, but not synced history deletions. In cases where there are no local history entries to delete, it can happen that the history page does not reload and it looks as if the deletion failed. See https://docs.google.com/document/d/1Fd6CdBf6UMbYbkwSjEKyFOxew0Xid5IaT-QwnFchjig/ for background. This CL 1. Adds an Observer subclass to the WebHistoryService. 2. Registers BrowsingHistoryHandler as a WebHistoryService::Observer; and since WebHistoryService's existence is based on whether history sync is enabled, we also register as a SyncServiceObserver. 3. Adds a test to browsing_history_handler_unittest.cc. Also tested manually on Android - seems to solve the problem described in the above mentioned document. BUG=604114,630164 ==========
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
The CQ bit was checked by msramek@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...
msramek@chromium.org changed reviewers: + lshang@chromium.org, sky@chromium.org, tsergeant@chromium.org
Hello folks! Please have a look! This is my attempt to fix the elusive bug where the history deletion seemingly failed. See the linked document for background. Liu: Please confirm that this doesn't collide with your work. I know that you have been fixing something similar, but AFAIK not this exactly. Tim: PTAL at BrowsingHistoryHandler and its test. Scott: PTAL at [Fake]WebHistoryService. Thanks, Martin
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Liu is travelling and so might not see this, but her work was to do with ensuring the Javascript side of `historyDeleted` works in MD History, and so shouldn't conflict with this. https://codereview.chromium.org/2263613002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/browsing_history_handler_unittest.cc (right): https://codereview.chromium.org/2263613002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/browsing_history_handler_unittest.cc:91: bool sync_active_; sync_active_ doesn't appear to be actually used anywhere? It seems as though the second section of the test below passes because WebHistoryService is always available, and not because of any changes to the Sync Service.
The CQ bit was checked by msramek@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...
Thanks for the heads-up, Tim! https://codereview.chromium.org/2263613002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/browsing_history_handler_unittest.cc (right): https://codereview.chromium.org/2263613002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/browsing_history_handler_unittest.cc:91: bool sync_active_; On 2016/08/24 08:47:04, tsergeant wrote: > sync_active_ doesn't appear to be actually used anywhere? It seems as though the > second section of the test below passes because WebHistoryService is always > available, and not because of any changes to the Sync Service. Thanks for spotting - I forgot the hardcoded "true" there. I added |sync_active_| to IsSyncActive(), and also a NotifyObservers() call to SetSyncActive(). I changed the class to inherit from TestProfileSyncService, since NotifyObservers() is private in the regular ProfileSyncService. I also added a third section to the test as a "control group" to prevent this mistake from happening again. Finally, I changed |sync_active_| to initialize to true, otherwise the WebHistoryServiceFactory::GetForProfile() call in the test constructor would return nullptr. This rigid relationship between ProfileSyncService and WebHistoryService is actually the reason why we need this complicated test in the first place.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by msramek@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
My work was to reload the history list when 'historyDeleted' gets called on Javascript side, so that doesn't collide with this CL. And thanks for fixing this!
Thanks for expanding out the test. I'm not very familiar with these sync internals, but browsing_history_handler lgtm
Thank you! Scott - friendly ping :)
https://codereview.chromium.org/2263613002/diff/80001/components/history/core... File components/history/core/browser/web_history_service.cc (right): https://codereview.chromium.org/2263613002/diff/80001/components/history/core... components/history/core/browser/web_history_service.cc:433: request_ptr->Start(); Is there a particular reason for this change? https://codereview.chromium.org/2263613002/diff/80001/components/history/core... components/history/core/browser/web_history_service.cc:568: FOR_EACH_OBSERVER(Observer, observer_list_, OnWebHistoryDeleted()); Shouldn't you only call this on success? https://codereview.chromium.org/2263613002/diff/80001/components/history/core... File components/history/core/browser/web_history_service.h (right): https://codereview.chromium.org/2263613002/diff/80001/components/history/core... components/history/core/browser/web_history_service.h:50: class Observer { Move this into it's own header (see style guide), and make destructor protected. https://codereview.chromium.org/2263613002/diff/80001/components/history/core... components/history/core/browser/web_history_service.h:52: virtual void OnWebHistoryDeleted() = 0; This name is misleading. It sort of implies all of web history was deleted, when that isn't the case. At a minimum add a comment. https://codereview.chromium.org/2263613002/diff/80001/components/history/core... File components/history/core/test/fake_web_history_service.cc (right): https://codereview.chromium.org/2263613002/diff/80001/components/history/core... components/history/core/test/fake_web_history_service.cc:122: if (base_url == GURL(kDeleteUrl) && client == kChromeClient) { nit: no {} https://codereview.chromium.org/2263613002/diff/80001/components/history/core... components/history/core/test/fake_web_history_service.cc:122: if (base_url == GURL(kDeleteUrl) && client == kChromeClient) { Shouldn't this and 127 be else conditions of the if started on 110?
The CQ bit was checked by msramek@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...
https://codereview.chromium.org/2263613002/diff/80001/components/history/core... File components/history/core/browser/web_history_service.cc (right): https://codereview.chromium.org/2263613002/diff/80001/components/history/core... components/history/core/browser/web_history_service.cc:433: request_ptr->Start(); On 2016/08/25 16:07:59, sky wrote: > Is there a particular reason for this change? Yes. The request implementation in the testing FakeWebHistoryService is synchronous. It immediately calls the callback which tries to find the request in |pending_expire_requests_|, but it's not there - it's going to be added on the next line. I considered changing FakeWebHistoryService to be asynchronous and use PostTask() instead. But that would also add unnecessary boilerplate code to the tests. I think it's clearer when we do not hardcode the assumption that Request is asynchronous. https://codereview.chromium.org/2263613002/diff/80001/components/history/core... components/history/core/browser/web_history_service.cc:568: FOR_EACH_OBSERVER(Observer, observer_list_, OnWebHistoryDeleted()); On 2016/08/25 16:08:00, sky wrote: > Shouldn't you only call this on success? Done. Good point. https://codereview.chromium.org/2263613002/diff/80001/components/history/core... File components/history/core/browser/web_history_service.h (right): https://codereview.chromium.org/2263613002/diff/80001/components/history/core... components/history/core/browser/web_history_service.h:50: class Observer { On 2016/08/25 16:08:00, sky wrote: > Move this into it's own header (see style guide), and make destructor protected. Done. https://codereview.chromium.org/2263613002/diff/80001/components/history/core... components/history/core/browser/web_history_service.h:52: virtual void OnWebHistoryDeleted() = 0; On 2016/08/25 16:08:00, sky wrote: > This name is misleading. It sort of implies all of web history was deleted, when > that isn't the case. At a minimum add a comment. Done. Added a comment. I considered renaming it to something like "OnSuccessfulDeletionRequest" or such, but I think it would be more awkward. Here, the comment should be enough to set the expectation that "some of history" as opposed to "all of history" was deleted. https://codereview.chromium.org/2263613002/diff/80001/components/history/core... File components/history/core/test/fake_web_history_service.cc (right): https://codereview.chromium.org/2263613002/diff/80001/components/history/core... components/history/core/test/fake_web_history_service.cc:122: if (base_url == GURL(kDeleteUrl) && client == kChromeClient) { On 2016/08/25 16:08:00, sky wrote: > Shouldn't this and 127 be else conditions of the if started on 110? Done. Yes, these conditions are mutually exclusive. > nit: no {} N/A after I changed it to "else if".
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2263613002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/browsing_history_handler.cc (right): https://codereview.chromium.org/2263613002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/browsing_history_handler.cc:994: web_ui()->CallJavascriptFunctionUnsafe("historyDeleted"); note: if the page is in the middle of a reload when this is called, the message may be dropped (and might eventually crash chrome if we re-instate a CHECK() that once was here) we've slowly been changing things to OnJavascript{Allowed,Disallowed} throughout webui. just FYI, that's why it's named "Unsafe"
https://codereview.chromium.org/2263613002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/browsing_history_handler.cc (right): https://codereview.chromium.org/2263613002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/browsing_history_handler.cc:994: web_ui()->CallJavascriptFunctionUnsafe("historyDeleted"); On 2016/08/25 19:20:10, Dan Beam wrote: > note: if the page is in the middle of a reload when this is called, the message > may be dropped (and might eventually crash chrome if we re-instate a CHECK() > that once was here) > > we've slowly been changing things to OnJavascript{Allowed,Disallowed} throughout > webui. > > just FYI, that's why it's named "Unsafe" Thanks for checking. On the JS side, "historyDeleted" should do exactly that - reload the page. If the request is dropped during a reload, that's not a problem - it's already doing what it's supposed to. If it doesn't cause a crash for other reasons, I would still prefer to go forward with this CL, as I'm trying to fix a long-standing user complaint. I'm happy to work with you on a followup where we apply OnJavascript{Allowed,Disallowed}.
LGTM https://codereview.chromium.org/2263613002/diff/100001/components/history/cor... File components/history/core/test/fake_web_history_service.cc (right): https://codereview.chromium.org/2263613002/diff/100001/components/history/cor... components/history/core/test/fake_web_history_service.cc:120: // Deletion query. Having the comments above the conditional is weird. I would move them inside the corresponding if.
Thanks Scott! I'm going to land this now in hopes of being in time for the branchpoint. Dan - in case I misunderstood you and that WebUI call is not permissible from your POV, feel free to simply uncheck the CQ and I will follow up tomorrow. https://codereview.chromium.org/2263613002/diff/100001/components/history/cor... File components/history/core/test/fake_web_history_service.cc (right): https://codereview.chromium.org/2263613002/diff/100001/components/history/cor... components/history/core/test/fake_web_history_service.cc:120: // Deletion query. On 2016/08/25 20:23:03, sky wrote: > Having the comments above the conditional is weird. I would move them inside the > corresponding if. Done.
The CQ bit was checked by msramek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsergeant@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2263613002/#ps120001 (title: "Moved comments inside 'if'.")
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 commit-bot@chromium.org
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...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by msramek@chromium.org
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 commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by msramek@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 ========== Make BrowsingDataHandler observe WebHistoryService deletions BrowsingDataHandler observes local history deletions, but not synced history deletions. In cases where there are no local history entries to delete, it can happen that the history page does not reload and it looks as if the deletion failed. See https://docs.google.com/document/d/1Fd6CdBf6UMbYbkwSjEKyFOxew0Xid5IaT-QwnFchjig/ for background. This CL 1. Adds an Observer subclass to the WebHistoryService. 2. Registers BrowsingHistoryHandler as a WebHistoryService::Observer; and since WebHistoryService's existence is based on whether history sync is enabled, we also register as a SyncServiceObserver. 3. Adds a test to browsing_history_handler_unittest.cc. Also tested manually on Android - seems to solve the problem described in the above mentioned document. BUG=604114,630164 ========== to ========== Make BrowsingDataHandler observe WebHistoryService deletions BrowsingDataHandler observes local history deletions, but not synced history deletions. In cases where there are no local history entries to delete, it can happen that the history page does not reload and it looks as if the deletion failed. See https://docs.google.com/document/d/1Fd6CdBf6UMbYbkwSjEKyFOxew0Xid5IaT-QwnFchjig/ for background. This CL 1. Adds an Observer subclass to the WebHistoryService. 2. Registers BrowsingHistoryHandler as a WebHistoryService::Observer; and since WebHistoryService's existence is based on whether history sync is enabled, we also register as a SyncServiceObserver. 3. Adds a test to browsing_history_handler_unittest.cc. Also tested manually on Android - seems to solve the problem described in the above mentioned document. BUG=604114,630164 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Make BrowsingDataHandler observe WebHistoryService deletions BrowsingDataHandler observes local history deletions, but not synced history deletions. In cases where there are no local history entries to delete, it can happen that the history page does not reload and it looks as if the deletion failed. See https://docs.google.com/document/d/1Fd6CdBf6UMbYbkwSjEKyFOxew0Xid5IaT-QwnFchjig/ for background. This CL 1. Adds an Observer subclass to the WebHistoryService. 2. Registers BrowsingHistoryHandler as a WebHistoryService::Observer; and since WebHistoryService's existence is based on whether history sync is enabled, we also register as a SyncServiceObserver. 3. Adds a test to browsing_history_handler_unittest.cc. Also tested manually on Android - seems to solve the problem described in the above mentioned document. BUG=604114,630164 ========== to ========== Make BrowsingDataHandler observe WebHistoryService deletions BrowsingDataHandler observes local history deletions, but not synced history deletions. In cases where there are no local history entries to delete, it can happen that the history page does not reload and it looks as if the deletion failed. See https://docs.google.com/document/d/1Fd6CdBf6UMbYbkwSjEKyFOxew0Xid5IaT-QwnFchjig/ for background. This CL 1. Adds an Observer subclass to the WebHistoryService. 2. Registers BrowsingHistoryHandler as a WebHistoryService::Observer; and since WebHistoryService's existence is based on whether history sync is enabled, we also register as a SyncServiceObserver. 3. Adds a test to browsing_history_handler_unittest.cc. Also tested manually on Android - seems to solve the problem described in the above mentioned document. BUG=604114,630164 Committed: https://crrev.com/d39070b1ac01776e5a3800a9ef55fe43dce74225 Cr-Commit-Position: refs/heads/master@{#414679} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/d39070b1ac01776e5a3800a9ef55fe43dce74225 Cr-Commit-Position: refs/heads/master@{#414679} |
