Skip to content

GitLab

T
tangled
Commit 676e4ce9 authored by Robert Ogden's avatar Robert Ogden Committed by Commit Bot
Browse files
Options

Ignore deleted Offline page events 

The OfflinePageModel likes to call OfflinePageDeleted when entries are
refreshed or otherwise cleaned up. When this happens it comes with an
OfflinePageAdded event (before the deletion) with a different offline
page id but the same url. Thus, the offline helper entry is removed
whenever that happens because we are not keying on offline page id.

This CL makes it so we may trigger a preview attempt on a truly
deleted page, but only for a single session. This comes at the benefit
of triggering on all true positives.

Bug: 969163
Change-Id: I4cf9d303ae997f1acba46652a48112bf6d3afabd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1715529Reviewed-by: default avatarRyan Sturm <ryansturm@chromium.org>
Commit-Queue: Robert Ogden <robertogden@chromium.org>
Cr-Commit-Position: refs/heads/master@{#680163}
parent d1c07e22
Hide whitespace changes
Inline Side-by-side
Showing with 6 additions and 38 deletions
chrome/browser/previews/previews_offline_helper.cc
View file @ 676e4ce9
......@@ -274,11 +274,10 @@ void PreviewsOfflineHelper::OfflinePageAdded(
void PreviewsOfflineHelper::OfflinePageDeleted(
const offline_pages::OfflinePageItem& deleted_page) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
// Has no effect if the url was never in the dictionary.
available_pages_->RemoveKey(HashURL(deleted_page.url));
UpdatePref();
// Do nothing. OfflinePageModel calls |OfflinePageDeleted| when pages are
// refreshed, but because we only key on URL and not the offline page id, it
// is difficult to tell when this happens. So instead, it's ok if we
// over-trigger for a few pages until the next DB query.
}
void PreviewsOfflineHelper::UpdatePref() {
......
chrome/browser/previews/previews_offline_helper_unittest.cc
View file @ 676e4ce9
......@@ -52,12 +52,6 @@ class PreviewsOfflineHelperTest : public ChromeRenderViewHostTestHarness {
return item;
}
offline_pages::OfflinePageItem MakeDeletedPageItem(const std::string& url) {
offline_pages::OfflinePageItem item;
item.url = GURL(url); // Only |url| is needed.
return item;
}
private:
std::unique_ptr<PreviewsOfflineHelper> helper_;
};
......@@ -68,7 +62,6 @@ TEST_F(PreviewsOfflineHelperTest, TestAddRemovePages) {
bool enable_feature;
std::vector<std::string> add_fresh_pages;
std::vector<std::string> add_expired_pages;
std::vector<std::string> delete_pages;
std::vector<std::string> want_pages;
std::vector<std::string> not_want_pages;
std::string original_url;
......@@ -80,7 +73,6 @@ TEST_F(PreviewsOfflineHelperTest, TestAddRemovePages) {
.enable_feature = false,
.add_fresh_pages = {},
.add_expired_pages = {},
.delete_pages = {},
.want_pages = {"http://chromium.org"},
.not_want_pages = {},
.original_url = "",
......@@ -91,7 +83,6 @@ TEST_F(PreviewsOfflineHelperTest, TestAddRemovePages) {
.enable_feature = true,
.add_fresh_pages = {},
.add_expired_pages = {},
.delete_pages = {},
.want_pages = {},
.not_want_pages = {"http://chromium.org"},
.original_url = "",
......@@ -102,7 +93,6 @@ TEST_F(PreviewsOfflineHelperTest, TestAddRemovePages) {
.enable_feature = true,
.add_fresh_pages = {"http://chromium.org"},
.add_expired_pages = {},
.delete_pages = {},
.want_pages = {"http://chromium.org"},
.not_want_pages = {},
.original_url = "",
......@@ -113,7 +103,6 @@ TEST_F(PreviewsOfflineHelperTest, TestAddRemovePages) {
.enable_feature = true,
.add_fresh_pages = {"http://chromium.org"},
.add_expired_pages = {},
.delete_pages = {},
.want_pages = {"http://google.com"},
.not_want_pages = {},
.original_url = "http://google.com",
......@@ -124,18 +113,6 @@ TEST_F(PreviewsOfflineHelperTest, TestAddRemovePages) {
.enable_feature = true,
.add_fresh_pages = {},
.add_expired_pages = {"http://chromium.org"},
.delete_pages = {},
.want_pages = {},
.not_want_pages = {"http://chromium.org"},
.original_url = "",
.want_pref_size = 0,
},
{
.msg = "Added then deleted page returns false",
.enable_feature = true,
.add_fresh_pages = {"http://chromium.org"},
.add_expired_pages = {},
.delete_pages = {"http://chromium.org"},
.want_pages = {},
.not_want_pages = {"http://chromium.org"},
.original_url = "",
......@@ -146,7 +123,6 @@ TEST_F(PreviewsOfflineHelperTest, TestAddRemovePages) {
.enable_feature = true,
.add_fresh_pages = {"http://chromium.org"},
.add_expired_pages = {"http://chromium.org"},
.delete_pages = {},
.want_pages = {"http://chromium.org"},
.not_want_pages = {},
.original_url = "",
......@@ -157,7 +133,6 @@ TEST_F(PreviewsOfflineHelperTest, TestAddRemovePages) {
.enable_feature = true,
.add_fresh_pages = {"http://chromium.org"},
.add_expired_pages = {},
.delete_pages = {},
.want_pages = {"http://chromium.org",
"http://chromium.org/#previews"},
.not_want_pages = {},
......@@ -167,13 +142,10 @@ TEST_F(PreviewsOfflineHelperTest, TestAddRemovePages) {
{
.msg = "URLs with paths are different",
.enable_feature = true,
.add_fresh_pages = {"http://chromium.org/fresh",
"http://chromium.org/fresh_but_deleted"},
.add_fresh_pages = {"http://chromium.org/fresh"},
.add_expired_pages = {"http://chromium.org/old"},
.delete_pages = {"http://chromium.org/fresh_but_deleted"},
.want_pages = {"http://chromium.org/fresh"},
.not_want_pages = {"http://chromium.org/old",
"http://chromium.org/fresh_but_deleted"},
.not_want_pages = {"http://chromium.org/old"},
.original_url = "",
.want_pref_size = 1,
},
......@@ -213,9 +185,6 @@ TEST_F(PreviewsOfflineHelperTest, TestAddRemovePages) {
nullptr,
MakeAddedPageItem(fresh_page, test_case.original_url, fresh));
}
for (const std::string& deleted_page : test_case.delete_pages) {
helper->OfflinePageDeleted(MakeDeletedPageItem(deleted_page));
}
EXPECT_EQ(test_prefs.GetDictionary(kDictKey)->size(),
test_case.want_pref_size);
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment