[NTP::PhysicalWeb] In OnLost invalidate by |resolved_url|.

components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc

Index: components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc
diff --git a/components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc b/components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc
index c612b36191aad7426b62886cdeabefd1890e3739..8e8a8c946c048c27ce5199a802a4e363abc5efb9 100644
--- a/components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc
+++ b/components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc
@@ -152,11 +152,13 @@ void PhysicalWebPageSuggestionsProvider::Fetch(
     const std::set<std::string>& known_suggestion_ids,
     const FetchDoneCallback& callback) {
   DCHECK_EQ(category, provided_category_);
+  std::vector<ContentSuggestion> suggestions =
+      GetMostRecentPhysicalWebPagesWithFilter(kMaxSuggestionsCount,
+                                              known_suggestion_ids);
+  AppendToShownScannedUrls(suggestions);
   base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE,
-      base::Bind(callback, Status::Success(),
-                 base::Passed(GetMostRecentPhysicalWebPagesWithFilter(
-                     kMaxSuggestionsCount, known_suggestion_ids))));
+      FROM_HERE, base::Bind(callback, Status::Success(),
+                            base::Passed(std::move(suggestions))));
 }
 
 void PhysicalWebPageSuggestionsProvider::ClearHistory(
@@ -215,21 +217,25 @@ void PhysicalWebPageSuggestionsProvider::NotifyStatusChanged(
 
 void PhysicalWebPageSuggestionsProvider::FetchPhysicalWebPages() {
   DCHECK_EQ(CategoryStatus::AVAILABLE, category_status_);
+  std::vector<ContentSuggestion> suggestions =
+      GetMostRecentPhysicalWebPagesWithFilter(
+          kMaxSuggestionsCount,
+          /*excluded_ids=*/std::set<std::string>());
+  shown_scanned_urls_.clear();
+  AppendToShownScannedUrls(suggestions);
   observer()->OnNewSuggestions(this, provided_category_,
-                               GetMostRecentPhysicalWebPagesWithFilter(
-                                   kMaxSuggestionsCount,
-                                   /*excluded_ids=*/std::set<std::string>()));
+                               std::move(suggestions));
 }
 
 std::vector<ContentSuggestion>
 PhysicalWebPageSuggestionsProvider::GetMostRecentPhysicalWebPagesWithFilter(
-    int max_quantity,
+    int max_count,
     const std::set<std::string>& excluded_ids) {
   std::unique_ptr<physical_web::MetadataList> page_metadata_list =
       physical_web_data_source_->GetMetadataList();
 
   // These is to filter out dismissed suggestions and at the same time prune the
-  // dismissed IDs list removing nonavailable pages (this is need since some
+  // dismissed IDs list removing nonavailable pages (this is needed since some
   // OnLost() calls may have been missed).
   const std::set<std::string> old_dismissed_ids = ReadDismissedIDsFromPrefs();
   std::set<std::string> new_dismissed_ids;
@@ -256,7 +262,7 @@ PhysicalWebPageSuggestionsProvider::GetMostRecentPhysicalWebPagesWithFilter(
 
   std::vector<ContentSuggestion> suggestions;
   for (const auto& page_metadata : filtered_metadata_list) {
-    if (static_cast<int>(suggestions.size()) == max_quantity) {
+    if (static_cast<int>(suggestions.size()) == max_count) {
       break;
     }
     suggestions.push_back(ConvertPhysicalWebPage(page_metadata));
@@ -283,7 +289,24 @@ void PhysicalWebPageSuggestionsProvider::OnFound(const GURL& url) {
 }
 
 void PhysicalWebPageSuggestionsProvider::OnLost(const GURL& url) {
-  InvalidateSuggestion(url.spec());
+  const auto& it = shown_scanned_urls_.find(url);

[Marc Treib, 2017/01/31 11:02:41]

nit: just "auto it", no need for reference to an iterator

[vitaliii, 2017/01/31 12:33:49]

Done.

+  if (it == shown_scanned_urls_.end()) {
+    // The suggestion may be shown on old NTPs.
+    // TODO(vitaliii): Use |resolved_url| here when it is available.

[Marc Treib, 2017/01/31 11:02:41]

What does this mean? It looks like sometimes we invalidate by scanned URL,
sometimes by resolved URL?
If we don't have a resolved URL for this entry, doesn't that mean that we never
served it as a suggestion? So no need for invalidation here?

[vitaliii, 2017/01/31 12:33:49]

We should invalidate by resolved URL, because the suggestion id is a resolved
URL.
Invalidating by scanned URL here is just a workaround.
It seems that it may be present on old NTPs (i.e. before last FetchRecentTabs
call), so we cannot simply ignore this call. I do not want to store all
suggestions, so I just invalidate |scanned_url| for now. I hope PW team will
provide both.

[Marc Treib, 2017/01/31 13:31:04]

But that generally won't do anything, right? (Except in the case where scanned
URL and resolved URL happen to be the same.)
Invalidating makes sense, but we need to send the correct URL, or it won't do
anything (or not reliably, anyway).

Storing all currently-known PW URLs doesn't seem so bad in terms of storage. But
it's kind of silly that we should have to do that, since PW should have that
info anyway. Is there any timeline for them providing the mapping?

[vitaliii, 2017/01/31 13:57:03]

True.
The case I described above assumes that that URL is not in PW list anymore.
I do not know whether this is possible (I have never seen any guarantees about
when OnLost is called).
Storing all PW items all the time does not look good.
It is not about a mapping, it is about adding |resolved_url| to all
notifications.
I am not aware of any timeline, however, not M57 for sure.

[Marc Treib, 2017/01/31 14:25:58]

Then I'd say, rather than doing this half-fix, we should wait until we can fix
it properly.
I think it should be safe to assume that PW will call OnLost when an entry gets
removed?
If they don't call us back reliably, then things are hopeless anyway.
Maybe the notifications should provide the metadata instead of the URL?
Or, alternatively, is there a way to get the metadata from the scanned URL?

Anyway, it seems to me that we won't get this properly fixed for M57 anyway.
(And really, we shouldn't IMO. Branch point has passed and things are not done.)

[vitaliii, 2017/01/31 15:10:57]

Please see below why I think that this is 99% fix.
Yes, it is safe.
However, I meant when exactly OnLost is called. Currently, the list of PW items
fetched inside OnLost does not contain the removed item, so there may be a gap
between removing it from the list and calling OnLost.
I agree that they should. 
Currently there is no other way except querying all items (but this does not
work for OnLost, there the item is removed from the list before the
notification).
I do not think this bug is that dramatic. The only thing we cannot implement is
a never observed speculation and it is applicable only to old NTPs, which are
likely to be out of sync with real world anyway (e.g. articles for you). Is not
being able (with low probability as of now) to remove some PW suggestion from
old NTPs a launch blocker?

Apart from this speculative case this CL resolves the bug.
But this is clearly a bug, I am not sure why we shouldn't merge it?

[Marc Treib, 2017/01/31 15:38:31]

The (presumed) 99% fix is the thing below - invalidating the scanned_url here
seems just wrong to me.
Maybe it's better than not invalidating anything, but please at least add a
detailed comment explaining this.
That just sounds like OnLost is called after the item has been removed from the
list, which makes sense (even though it's kinda unfortunate, because we can't
query the mapping anymore. Maybe they could provide an "OnAboutToLose"
notification...)
Alright, fair enough. See above; maybe it's possible to do something about that.
Okay, I think I see now. The case where we don't have the proper mapping
shouldn't actually ever happen.
Fair enough.
I still don't like the fact that we have to add a lot of logic here that should
really mostly live in PW, but oh well...
Well, it's a bit of a grey area. If this were a regression (in an
already-released feature), then we would clearly merge it. If it were a
pre-existing bug in an already-released feature, then we probably wouldn't (if
it was broken before, then it can't be that urgent).
For a not-yet-released feature like this, it's a bit unclear. If we're somewhat
confident that this actually fixes the problem, then we're probably still close
enough to branch point that we can merge it. It's still not really nice, because
basically all testing would have to be done again.

[vitaliii, 2017/01/31 16:15:02]

That was my assumption that it is better than not invalidating at all.
I extended the comments.
Yes. Instead they could provider metadata (or |resolved_url|) as an argument.
However, we will still need to count beacons per |resolved_url|, unless they
change their interface completely (from |scanned_url| to urls to show), which
seems unlikely.
As I said above, the could provide metadata as an argument, however, this would
be too much for M57.
Technically yes, but without any guarantees from PW side it is impossible to be
sure.
This is partially true. Even if they provider metadata, we will have to count
beacons per |resolved_url|, unless they change the interface completely.
I see, fair enough. Let's see how this fix behaves in Canary.

+    InvalidateSuggestion(url.spec());
+    return;
+  }
+
+  const GURL lost_resolved_url = it->second;

[Marc Treib, 2017/01/31 11:02:41]

nit: ref?

[vitaliii, 2017/01/31 12:33:49]

the multimap pair is removed in the next line.

[Marc Treib, 2017/01/31 13:31:04]

Ah, good point. Worth a comment maybe :)

[vitaliii, 2017/01/31 13:57:03]

Acknowledged.

[Marc Treib, 2017/01/31 14:25:58]

...but not done? :P

[vitaliii, 2017/01/31 15:10:57]

Done.

"Maybe" suggested that you did not insist and I considered drawing attention to
absence of |&| vain, since the next line explains it anyway. However, since you
find it that confusing, I added a comment.

[Marc Treib, 2017/01/31 15:38:31]

Thanks!
I wouldn't have insisted, but if you choose to ignore a comment, I'd expect a
short explanation rather than just "ack". Maybe I just don't know how to
interpret "ack" :)

[vitaliii, 2017/01/31 16:15:02]

I see, fair enough. My understanding of "ack" is "I have read,  but done
nothing.".
I will try commenting "ack" in future.

+  shown_scanned_urls_.erase(it);
+  for (const auto& pair : shown_scanned_urls_) {

[Marc Treib, 2017/01/31 11:02:41]

optional: Could probably be a find_if, since you already use that elsewhere

[vitaliii, 2017/01/31 12:33:49]

Done.

+    const GURL& resolved_url = pair.second;
+    if (resolved_url == lost_resolved_url) {
+      // There are remaining beacons transmiting this URL.
+      return;
+    }
+  }
+  InvalidateSuggestion(lost_resolved_url.spec());
 }
 
 void PhysicalWebPageSuggestionsProvider::OnDistanceChanged(
@@ -306,6 +329,21 @@ void PhysicalWebPageSuggestionsProvider::InvalidateSuggestion(
   }
 }
 
+void PhysicalWebPageSuggestionsProvider::AppendToShownScannedUrls(
+    const std::vector<ContentSuggestion>& suggestions) {
+  std::unique_ptr<physical_web::MetadataList> page_metadata_list =
+      physical_web_data_source_->GetMetadataList();
+  for (const auto& page_metadata : *page_metadata_list) {

[Marc Treib, 2017/01/31 11:02:41]

This seems quite convoluted. Isn't there a way to get the scanned->resolved URL
mapping at the point where we initially build the suggestions?

[vitaliii, 2017/01/31 12:33:49]

There may a way. However, if there are multiple scanned_urls for the same
resolved_url, we need to process all pages anyway. Thus, I do not see any
benefit of doing it there. Also querying happens in a function, which does not
submit the suggestions, so it would be inappropriate to update this cache there.

[Marc Treib, 2017/01/31 13:31:04]

I'm not sure I follow. It seems to me that
GetMostRecentPhysicalWebPagesWithFilter is the correct place to do this - it's
where we collect all the suggestions to send out, and there we iterate over all
PW pages anyway. Maybe that method then needs to be renamed since it'll do more
than just "Get", but I still think that's the right place for this. WDYT?

[vitaliii, 2017/01/31 13:57:03]

GetMostRecentPhysicalWebPagesWithFilter was created to support Fetch "More". Now
it is used for both Fetch "More" and fetching all pages. These 2 cases require
handling the url cache differently (Fetch "More" appends, while fetching all
pages rewrites). Also they "submit" suggestions differently - Fetch "More"
through a callback, fetching all pages through OnNewSuggestions.

Moreover, in GetMostRecentPhysicalWebPagesWithFilter we filter out a lot of
pages before obtaining the ones to be submitted. In order to update the url
cache we need to know the ones to be submitted, so we will have to copy all PW
pages anyway.

I still don't think that there is large benefit of trying to fit this into
GetMostRecentPhysicalWebPagesWithFilter. Why having this method does not work
for you?

[Marc Treib, 2017/01/31 14:25:58]

The different treatment of the cache is easy to resolve: "all" clears first,
"more" doesn't.
The different "submit" paths don't seem relevant for this discussion?
Do we? Don't we just want to update the cache for the to-be-submitted
suggestions anyway, i.e. after all the filtering?
It just seems unnecessarily convoluted to me to run an extra O(n^2) loop here to
collect information which (IIUC) you have readily available at another point. I
guess I'm just missing something, and I'm trying to understand what it is.

[vitaliii, 2017/01/31 15:10:57]

I see, fair enough.
I assumed that you wanted |GetMostRecentPhysicalWebPagesWithFilter| to submit
the suggestions.
No, that's the problem.
The cache contains pairs (|scanned_url|, |resolved_url|), such that
|resolved_url| is among to-be-submitted suggestions. There may be pairs with the
same |resolved_url| and different |scanned_url| (or even same |scanned_url|,
AFAIK this is not implemented yet). During the filtering we leave only one
|scanned_url| for each |resolved_url|. 

We need to have all |scanned_url|s for each |resolved_url| in the cache, because
otherwise we may receive OnLost for one of the |scanned_url|s and invalidate
|resolved_url|, even though there are remaining beacons with the same
|resolved_url|.
As I wrote above, it is not readily available in
GetMostRecentPhysicalWebPagesWithFilter.
There may be multiple |scanned_url|s for a |resolved_url| and we need all of
them.
Also it is not O(n^2), but O(kMaxSuggestionsCount * n). I can build a multiset
if you are concerned about the complexity. 

[Marc Treib, 2017/01/31 15:38:31]

Ah, so the problem is that we filter out duplicate resolved_urls, I see.
Nah, I'm not really worried about the complexity. It just seemed like we were
doing the same thing twice.

[vitaliii, 2017/01/31 16:15:02]

Yes, indeed.
I see, sorry for not explaining earlier.

+    if (std::find_if(suggestions.begin(), suggestions.end(),
+                     [page_metadata](const ContentSuggestion& suggestion) {
+                       return suggestion.url() == page_metadata.resolved_url;
+                     }) != suggestions.end()) {
+      shown_scanned_urls_.insert(std::make_pair(page_metadata.scanned_url,
+                                                page_metadata.resolved_url));
+    }
+  }
+}
+
 std::set<std::string>
 PhysicalWebPageSuggestionsProvider::ReadDismissedIDsFromPrefs() const {
   return prefs::ReadDismissedIDsFromPrefs(