Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2566123002:
Last visit dates of bookmarks - fix browsing data removal on desktop (Closed)
DescriptionLast visit dates of bookmarks - fix browsing data removal on desktop
Previously, last_visit_date metadata has was removed by
BookmarkSuggestionProvider via ContentSuggestionService::ClearHistory()
which only works on Android. This CL makes the removal platform-
independent. It aims at simplicity to make is easier to merge to M56
(and does not fix the pre-existing TODO).
The data is collected outside of BookmarkSuggestionProvider (by a tab
helper). Previously, the tab helper was only instantiated on Android.
Since M56 the tab helper is created and we thus collect the data on all
desktop platforms as well. For this reason, we also need to fix the
browsing data removal.
BUG=673268
Committed: https://crrev.com/f175406f0e8361524121499e5277c5d7c0bba087
Cr-Commit-Position: refs/heads/master@{#438129}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Adding a unittest #Patch Set 3 : Adding a unittest #
Total comments: 2
Patch Set 4 : Adding a unittest #
Total comments: 5
Patch Set 5 : Comments #2 #
Messages
Total messages: 32 (19 generated)
Description was changed from ========== Last visit dates of bookmarks - fix browsing data removal on desktop Previously, last_visit_date metadata has was removed by BookmarkSuggestionProvider via ContentSuggestionService::ClearHistory() which only works on Android. This CL makes the removal platform- independent. It aims at simplicity to make is easier to merge to M56. The data is collected outside of BookmarkSuggestionProvider (by a tab helper). Previously, the tab helper was only instantiated on Android. Since M56 the tab helper is created and we thus collect the data on all desktop platforms as well. For this reason, we also need to fix the browsing data removal. BUG=673268 ========== to ========== Last visit dates of bookmarks - fix browsing data removal on desktop Previously, last_visit_date metadata has was removed by BookmarkSuggestionProvider via ContentSuggestionService::ClearHistory() which only works on Android. This CL makes the removal platform- independent. It aims at simplicity to make is easier to merge to M56 (and does not fix the pre-existing TODO). The data is collected outside of BookmarkSuggestionProvider (by a tab helper). Previously, the tab helper was only instantiated on Android. Since M56 the tab helper is created and we thus collect the data on all desktop platforms as well. For this reason, we also need to fix the browsing data removal. BUG=673268 ==========
The CQ bit was checked by jkrcal@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...
jkrcal@chromium.org changed reviewers: + msramek@chromium.org, tschumann@chromium.org
Tim, could you PTAL at bookmark_suggestions_provider.cc? Martin, could you PTAL at browsing_data_remover.cc?
lgtm https://codereview.chromium.org/2566123002/diff/1/chrome/browser/browsing_dat... File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/2566123002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.cc:546: ntp_snippets::RemoveAllLastVisitDates(bookmark_model); should we handle nullptr? seems safer...
https://codereview.chromium.org/2566123002/diff/1/chrome/browser/browsing_dat... File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/2566123002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.cc:546: ntp_snippets::RemoveAllLastVisitDates(bookmark_model); On 2016/12/12 13:26:18, tschumann wrote: > should we handle nullptr? seems safer... +1 Also, please add a unittest; that should not hurt the simplicity w.r.t. merging, and is a good practice when you're fixing an oversight bug.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks, guys! Added the unit-test. PTAL, again! https://codereview.chromium.org/2566123002/diff/1/chrome/browser/browsing_dat... File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/2566123002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.cc:546: ntp_snippets::RemoveAllLastVisitDates(bookmark_model); On 2016/12/12 13:26:18, tschumann wrote: > should we handle nullptr? seems safer... True. Definitely safer. https://codereview.chromium.org/2566123002/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.cc:546: ntp_snippets::RemoveAllLastVisitDates(bookmark_model); On 2016/12/12 13:33:40, msramek wrote: > On 2016/12/12 13:26:18, tschumann wrote: > > should we handle nullptr? seems safer... > > +1 > > Also, please add a unittest; that should not hurt the simplicity w.r.t. merging, > and is a good practice when you're fixing an oversight bug. Hmm, I tried hard, the cost for the unit-test is quite high ;) - As this a simple call of a static function, I need to do it slightly more end-to-end for the test to make sense. Makes sense? - BookmarkModelFactory returns nullptr in unittests. I need to inject a fake inside the remover. Any objections? Any other comments?
https://codereview.chromium.org/2566123002/diff/40001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/2566123002/diff/40001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover.cc:545: bookmarks::BookmarkModel* bookmark_model; this seems quite heavy-handed. I wonder why something like this would also work in your case: https://codesearch.chromium.org/chromium/src/chrome/browser/bookmarks/bookmar...
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Thanks! PTAL, again. https://codereview.chromium.org/2566123002/diff/40001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/2566123002/diff/40001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover.cc:545: bookmarks::BookmarkModel* bookmark_model; On 2016/12/12 20:03:34, tschumann wrote: > this seems quite heavy-handed. I wonder why something like this would also work > in your case: > https://codesearch.chromium.org/chromium/src/chrome/browser/bookmarks/bookmar... > Oh, cool, I could not find this! Thanks!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm with a last nit. https://codereview.chromium.org/2566123002/diff/60001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right): https://codereview.chromium.org/2566123002/diff/60001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover_unittest.cc:3076: EXPECT_FALSE(ntp_snippets::GetRecentlyVisitedBookmarks( nit: using EXPECT_THAT() with gmock matchers, you can make this easier to read (and get nicer debug output): using ::testing::IsEmpty; using ::testing::Not; EXPECT_THAT(ntp_snippets::GetRecentlyVisitedBookmarks(bookmark_model, 2, base::Time::UnixEpoch(), /*consider_visits_from_desktop=*/false), Not(IsEmpty()); (and using just IsEmpty() in the expectations at the end).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
LGTM, looks much better without the injection. As a heads-up, I have a major refactoring in progress (https://codereview.chromium.org/2554413002/), so this code will move to a different class soon. This should not hinder your ability to merge (since you're merging to the past where BDR still looked as it does today), but future patches will be more difficult to merge. Please make sure you didn't forget anything :) https://codereview.chromium.org/2566123002/diff/60001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right): https://codereview.chromium.org/2566123002/diff/60001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover_unittest.cc:3084: remover->OverrideBookmarkModelForTesting(bookmark_model); You forgot this line, it's what makes the tests red.
Thanks! https://codereview.chromium.org/2566123002/diff/60001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right): https://codereview.chromium.org/2566123002/diff/60001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover_unittest.cc:3076: EXPECT_FALSE(ntp_snippets::GetRecentlyVisitedBookmarks( On 2016/12/13 09:03:31, tschumann wrote: > nit: using EXPECT_THAT() with gmock matchers, you can make this easier to read > (and get nicer debug output): > using ::testing::IsEmpty; > using ::testing::Not; > > EXPECT_THAT(ntp_snippets::GetRecentlyVisitedBookmarks(bookmark_model, 2, > base::Time::UnixEpoch(), /*consider_visits_from_desktop=*/false), > Not(IsEmpty()); > > (and using just IsEmpty() in the expectations at the end). Done. Well, this 3K+ lines unittest file did not have any instance of EXPECT_THAT. Martin, are you fine with your tests going in this direction? https://codereview.chromium.org/2566123002/diff/60001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover_unittest.cc:3084: remover->OverrideBookmarkModelForTesting(bookmark_model); On 2016/12/13 09:27:36, msramek wrote: > You forgot this line, it's what makes the tests red. Huh :) Sometimes CL-upload-without-local-compile hits on me ;)
The CQ bit was checked by jkrcal@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...
Still LGTM. https://codereview.chromium.org/2566123002/diff/60001/chrome/browser/browsing... File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right): https://codereview.chromium.org/2566123002/diff/60001/chrome/browser/browsing... chrome/browser/browsing_data/browsing_data_remover_unittest.cc:3076: EXPECT_FALSE(ntp_snippets::GetRecentlyVisitedBookmarks( On 2016/12/13 10:21:15, jkrcal wrote: > On 2016/12/13 09:03:31, tschumann wrote: > > nit: using EXPECT_THAT() with gmock matchers, you can make this easier to read > > (and get nicer debug output): > > using ::testing::IsEmpty; > > using ::testing::Not; > > > > EXPECT_THAT(ntp_snippets::GetRecentlyVisitedBookmarks(bookmark_model, 2, > > base::Time::UnixEpoch(), /*consider_visits_from_desktop=*/false), > > Not(IsEmpty()); > > > > (and using just IsEmpty() in the expectations at the end). > > Done. > > Well, this 3K+ lines unittest file did not have any instance of EXPECT_THAT. > Martin, are you fine with your tests going in this direction? It's fine. I personally do find simple statements such as EXPECT_FALSE more readable, but it's still the same testing framework, so I consider this style deviation to be within the scope of a personal taste :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jkrcal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tschumann@chromium.org Link to the patchset: https://codereview.chromium.org/2566123002/#ps80001 (title: "Comments #2")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481628564375310,
"parent_rev": "9bacff8f15c7c8b6aa4790e937558316e5860f16", "commit_rev":
"a4afa3a974f31989316d13044184824ec05094b1"}
Message was sent while issue was closed.
Description was changed from ========== Last visit dates of bookmarks - fix browsing data removal on desktop Previously, last_visit_date metadata has was removed by BookmarkSuggestionProvider via ContentSuggestionService::ClearHistory() which only works on Android. This CL makes the removal platform- independent. It aims at simplicity to make is easier to merge to M56 (and does not fix the pre-existing TODO). The data is collected outside of BookmarkSuggestionProvider (by a tab helper). Previously, the tab helper was only instantiated on Android. Since M56 the tab helper is created and we thus collect the data on all desktop platforms as well. For this reason, we also need to fix the browsing data removal. BUG=673268 ========== to ========== Last visit dates of bookmarks - fix browsing data removal on desktop Previously, last_visit_date metadata has was removed by BookmarkSuggestionProvider via ContentSuggestionService::ClearHistory() which only works on Android. This CL makes the removal platform- independent. It aims at simplicity to make is easier to merge to M56 (and does not fix the pre-existing TODO). The data is collected outside of BookmarkSuggestionProvider (by a tab helper). Previously, the tab helper was only instantiated on Android. Since M56 the tab helper is created and we thus collect the data on all desktop platforms as well. For this reason, we also need to fix the browsing data removal. BUG=673268 Review-Url: https://codereview.chromium.org/2566123002 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Last visit dates of bookmarks - fix browsing data removal on desktop Previously, last_visit_date metadata has was removed by BookmarkSuggestionProvider via ContentSuggestionService::ClearHistory() which only works on Android. This CL makes the removal platform- independent. It aims at simplicity to make is easier to merge to M56 (and does not fix the pre-existing TODO). The data is collected outside of BookmarkSuggestionProvider (by a tab helper). Previously, the tab helper was only instantiated on Android. Since M56 the tab helper is created and we thus collect the data on all desktop platforms as well. For this reason, we also need to fix the browsing data removal. BUG=673268 Review-Url: https://codereview.chromium.org/2566123002 ========== to ========== Last visit dates of bookmarks - fix browsing data removal on desktop Previously, last_visit_date metadata has was removed by BookmarkSuggestionProvider via ContentSuggestionService::ClearHistory() which only works on Android. This CL makes the removal platform- independent. It aims at simplicity to make is easier to merge to M56 (and does not fix the pre-existing TODO). The data is collected outside of BookmarkSuggestionProvider (by a tab helper). Previously, the tab helper was only instantiated on Android. Since M56 the tab helper is created and we thus collect the data on all desktop platforms as well. For this reason, we also need to fix the browsing data removal. BUG=673268 Committed: https://crrev.com/f175406f0e8361524121499e5277c5d7c0bba087 Cr-Commit-Position: refs/heads/master@{#438129} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/f175406f0e8361524121499e5277c5d7c0bba087 Cr-Commit-Position: refs/heads/master@{#438129} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
