Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2669533002:
[NTP::PhysicalWeb] In OnLost invalidate by |resolved_url|. (Closed)
|
Created:
3 years, 10 months ago by vitaliii Modified:
3 years, 10 months ago Reviewers:
Marc Treib CC:
chromium-reviews, noyau+watch_chromium.org, ntp-dev+reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[NTP::PhysicalWeb] In OnLost invalidate by |resolved_url|.
The OnLost notification is treated differently after this CL.
Previously, the obtained url (|scanned_url|) was propagated further,
which was not correct, because the |resolved_url| is used an an ID. In
this CL a mapping from |scanned_url| to |resolved_url| is stored for all
suggestions from the last update. This mapping is then used to obtain
|resolved_url|, which is then propagated further.
In addition to this, a case of multiple beacons for the same URL is
considered. The suggestion is invalidated, only when its last beacon has
been lost.
BUG=685245
Review-Url: https://codereview.chromium.org/2669533002
Cr-Commit-Position: refs/heads/master@{#447253}
Committed: https://chromium.googlesource.com/chromium/src/+/8666d444ab37fde30061683937c842156a08156b
Patch Set 1 : #
Total comments: 30
Patch Set 2 : treib@ comments. #Patch Set 3 : treib@ comments. #
Total comments: 2
Patch Set 4 : treib@ comments. #
Messages
Total messages: 27 (17 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by vitaliii@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...
treib@chromium.org changed reviewers: + treib@chromium.org
https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:292: const auto& it = shown_scanned_urls_.find(url); nit: just "auto it", no need for reference to an iterator https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:295: // TODO(vitaliii): Use |resolved_url| here when it is available. 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? https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:300: const GURL lost_resolved_url = it->second; nit: ref? https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:302: for (const auto& pair : shown_scanned_urls_) { optional: Could probably be a find_if, since you already use that elsewhere https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:336: for (const auto& page_metadata : *page_metadata_list) { 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? https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h (right): https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:103: std::multimap<GURL, GURL> shown_scanned_urls_; Map from what to what? Can we add a comment and/or a clearer name?
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 vitaliii@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...
Hi treib@, I addressed your comments, could you have a look? https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:292: const auto& it = shown_scanned_urls_.find(url); On 2017/01/31 11:02:41, Marc Treib wrote: > nit: just "auto it", no need for reference to an iterator Done. https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:295: // TODO(vitaliii): Use |resolved_url| here when it is available. On 2017/01/31 11:02:41, Marc Treib wrote: > What does this mean? It looks like sometimes we invalidate by scanned URL, > sometimes by resolved URL? We should invalidate by resolved URL, because the suggestion id is a resolved URL. Invalidating by scanned URL here is just a workaround. > 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? 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. https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:300: const GURL lost_resolved_url = it->second; On 2017/01/31 11:02:41, Marc Treib wrote: > nit: ref? the multimap pair is removed in the next line. https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:302: for (const auto& pair : shown_scanned_urls_) { On 2017/01/31 11:02:41, Marc Treib wrote: > optional: Could probably be a find_if, since you already use that elsewhere Done. https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:336: for (const auto& page_metadata : *page_metadata_list) { On 2017/01/31 11:02:41, Marc Treib wrote: > 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? 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. https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h (right): https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:103: std::multimap<GURL, GURL> shown_scanned_urls_; On 2017/01/31 11:02:41, Marc Treib wrote: > Map from what to what? Can we add a comment and/or a clearer name? Done. I went for a clearer name.
https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:295: // TODO(vitaliii): Use |resolved_url| here when it is available. On 2017/01/31 12:33:49, vitaliii wrote: > On 2017/01/31 11:02:41, Marc Treib wrote: > > What does this mean? It looks like sometimes we invalidate by scanned URL, > > sometimes by resolved URL? > > We should invalidate by resolved URL, because the suggestion id is a resolved > URL. > Invalidating by scanned URL here is just a workaround. But that generally won't do anything, right? (Except in the case where scanned URL and resolved URL happen to be the same.) > > 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? > > 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. 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? https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:300: const GURL lost_resolved_url = it->second; On 2017/01/31 12:33:49, vitaliii wrote: > On 2017/01/31 11:02:41, Marc Treib wrote: > > nit: ref? > > the multimap pair is removed in the next line. Ah, good point. Worth a comment maybe :) https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:336: for (const auto& page_metadata : *page_metadata_list) { On 2017/01/31 12:33:49, vitaliii wrote: > On 2017/01/31 11:02:41, Marc Treib wrote: > > 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? > > 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. 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?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi Marc, I replied to your comments, please have a look. https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:295: // TODO(vitaliii): Use |resolved_url| here when it is available. On 2017/01/31 13:31:04, Marc Treib wrote: > On 2017/01/31 12:33:49, vitaliii wrote: > > On 2017/01/31 11:02:41, Marc Treib wrote: > > > What does this mean? It looks like sometimes we invalidate by scanned URL, > > > sometimes by resolved URL? > > > > We should invalidate by resolved URL, because the suggestion id is a resolved > > URL. > > Invalidating by scanned URL here is just a workaround. > > But that generally won't do anything, right? (Except in the case where scanned > URL and resolved URL happen to be the same.) True. > > > > 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? > > > > 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. > > 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. 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. > 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? 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. https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:300: const GURL lost_resolved_url = it->second; On 2017/01/31 13:31:04, Marc Treib wrote: > On 2017/01/31 12:33:49, vitaliii wrote: > > On 2017/01/31 11:02:41, Marc Treib wrote: > > > nit: ref? > > > > the multimap pair is removed in the next line. > > Ah, good point. Worth a comment maybe :) Acknowledged. https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:336: for (const auto& page_metadata : *page_metadata_list) { On 2017/01/31 13:31:04, Marc Treib wrote: > On 2017/01/31 12:33:49, vitaliii wrote: > > On 2017/01/31 11:02:41, Marc Treib wrote: > > > 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? > > > > 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. > > 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? 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?
https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:295: // TODO(vitaliii): Use |resolved_url| here when it is available. On 2017/01/31 13:57:03, vitaliii wrote: > On 2017/01/31 13:31:04, Marc Treib wrote: > > On 2017/01/31 12:33:49, vitaliii wrote: > > > On 2017/01/31 11:02:41, Marc Treib wrote: > > > > What does this mean? It looks like sometimes we invalidate by scanned URL, > > > > sometimes by resolved URL? > > > > > > We should invalidate by resolved URL, because the suggestion id is a > resolved > > > URL. > > > Invalidating by scanned URL here is just a workaround. > > > > But that generally won't do anything, right? (Except in the case where scanned > > URL and resolved URL happen to be the same.) > > True. Then I'd say, rather than doing this half-fix, we should wait until we can fix it properly. > > > > > > 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? > > > > > > 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. > > > > 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. > > 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. 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. > > 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? > > 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. 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.) https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:300: const GURL lost_resolved_url = it->second; On 2017/01/31 13:57:03, vitaliii wrote: > On 2017/01/31 13:31:04, Marc Treib wrote: > > On 2017/01/31 12:33:49, vitaliii wrote: > > > On 2017/01/31 11:02:41, Marc Treib wrote: > > > > nit: ref? > > > > > > the multimap pair is removed in the next line. > > > > Ah, good point. Worth a comment maybe :) > > Acknowledged. ...but not done? :P https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:336: for (const auto& page_metadata : *page_metadata_list) { On 2017/01/31 13:57:03, vitaliii wrote: > On 2017/01/31 13:31:04, Marc Treib wrote: > > On 2017/01/31 12:33:49, vitaliii wrote: > > > On 2017/01/31 11:02:41, Marc Treib wrote: > > > > 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? > > > > > > 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. > > > > 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? > > 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. 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? > 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. Do we? Don't we just want to update the cache for the to-be-submitted suggestions anyway, i.e. after all the filtering? > 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? 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.
Hi treib@, I replied to your comments, please have a look. https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:295: // TODO(vitaliii): Use |resolved_url| here when it is available. On 2017/01/31 14:25:58, Marc Treib wrote: > On 2017/01/31 13:57:03, vitaliii wrote: > > On 2017/01/31 13:31:04, Marc Treib wrote: > > > On 2017/01/31 12:33:49, vitaliii wrote: > > > > On 2017/01/31 11:02:41, Marc Treib wrote: > > > > > What does this mean? It looks like sometimes we invalidate by scanned > URL, > > > > > sometimes by resolved URL? > > > > > > > > We should invalidate by resolved URL, because the suggestion id is a > > resolved > > > > URL. > > > > Invalidating by scanned URL here is just a workaround. > > > > > > But that generally won't do anything, right? (Except in the case where > scanned > > > URL and resolved URL happen to be the same.) > > > > True. > > Then I'd say, rather than doing this half-fix, we should wait until we can fix > it properly. Please see below why I think that this is 99% fix. > > > > > > > > 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? > > > > > > > > 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. > > > > > > 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. > > > > > 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. > > 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. 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. > > > > 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? > > > > 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. > > 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? 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). > Anyway, it seems to me that we won't get this properly fixed for M57 anyway. 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. > (And really, we shouldn't IMO. Branch point has passed and things are not done.) But this is clearly a bug, I am not sure why we shouldn't merge it? https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:300: const GURL lost_resolved_url = it->second; On 2017/01/31 14:25:58, Marc Treib wrote: > On 2017/01/31 13:57:03, vitaliii wrote: > > On 2017/01/31 13:31:04, Marc Treib wrote: > > > On 2017/01/31 12:33:49, vitaliii wrote: > > > > On 2017/01/31 11:02:41, Marc Treib wrote: > > > > > nit: ref? > > > > > > > > the multimap pair is removed in the next line. > > > > > > Ah, good point. Worth a comment maybe :) > > > > Acknowledged. > > ...but not done? :P 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. https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:336: for (const auto& page_metadata : *page_metadata_list) { On 2017/01/31 14:25:58, Marc Treib wrote: > On 2017/01/31 13:57:03, vitaliii wrote: > > On 2017/01/31 13:31:04, Marc Treib wrote: > > > On 2017/01/31 12:33:49, vitaliii wrote: > > > > On 2017/01/31 11:02:41, Marc Treib wrote: > > > > > 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? > > > > > > > > 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. > > > > > > 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? > > > > 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. > > The different treatment of the cache is easy to resolve: "all" clears first, > "more" doesn't. I see, fair enough. > The different "submit" paths don't seem relevant for this discussion? I assumed that you wanted |GetMostRecentPhysicalWebPagesWithFilter| to submit the suggestions. > > 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. > > Do we? Don't we just want to update the cache for the to-be-submitted > suggestions anyway, i.e. after all the filtering?\ 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|. > > > 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? > > 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. 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.
Thanks! LGTM with some last comments. Sorry for the many rounds of back-and-forth. But this whole thing is quite hard to follow. And I generally dislike adding complexity to our code for things that should really be handled elsewhere... https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:295: // TODO(vitaliii): Use |resolved_url| here when it is available. On 2017/01/31 15:10:57, vitaliii wrote: > On 2017/01/31 14:25:58, Marc Treib wrote: > > On 2017/01/31 13:57:03, vitaliii wrote: > > > On 2017/01/31 13:31:04, Marc Treib wrote: > > > > On 2017/01/31 12:33:49, vitaliii wrote: > > > > > On 2017/01/31 11:02:41, Marc Treib wrote: > > > > > > What does this mean? It looks like sometimes we invalidate by scanned > > URL, > > > > > > sometimes by resolved URL? > > > > > > > > > > We should invalidate by resolved URL, because the suggestion id is a > > > resolved > > > > > URL. > > > > > Invalidating by scanned URL here is just a workaround. > > > > > > > > But that generally won't do anything, right? (Except in the case where > > scanned > > > > URL and resolved URL happen to be the same.) > > > > > > True. > > > > Then I'd say, rather than doing this half-fix, we should wait until we can fix > > it properly. > > Please see below why I think that this is 99% fix. 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. > > > > > > > > > > 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? > > > > > > > > > > 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. > > > > > > > > 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. > > > > > > > > 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. > > > > 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. > > 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. 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...) > > > > 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? > > > > > > 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. > > > > 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? > > 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). Alright, fair enough. See above; maybe it's possible to do something about that. > > Anyway, it seems to me that we won't get this properly fixed for M57 anyway. > > 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? Okay, I think I see now. The case where we don't have the proper mapping shouldn't actually ever happen. > Apart from this speculative case this CL resolves the bug. 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... > > (And really, we shouldn't IMO. Branch point has passed and things are not > done.) > > But this is clearly a bug, I am not sure why we shouldn't merge it? 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. https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:300: const GURL lost_resolved_url = it->second; On 2017/01/31 15:10:57, vitaliii wrote: > On 2017/01/31 14:25:58, Marc Treib wrote: > > On 2017/01/31 13:57:03, vitaliii wrote: > > > On 2017/01/31 13:31:04, Marc Treib wrote: > > > > On 2017/01/31 12:33:49, vitaliii wrote: > > > > > On 2017/01/31 11:02:41, Marc Treib wrote: > > > > > > nit: ref? > > > > > > > > > > the multimap pair is removed in the next line. > > > > > > > > Ah, good point. Worth a comment maybe :) > > > > > > Acknowledged. > > > > ...but not done? :P > > 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. 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" :) https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:336: for (const auto& page_metadata : *page_metadata_list) { On 2017/01/31 15:10:57, vitaliii wrote: > On 2017/01/31 14:25:58, Marc Treib wrote: > > On 2017/01/31 13:57:03, vitaliii wrote: > > > On 2017/01/31 13:31:04, Marc Treib wrote: > > > > On 2017/01/31 12:33:49, vitaliii wrote: > > > > > On 2017/01/31 11:02:41, Marc Treib wrote: > > > > > > 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? > > > > > > > > > > 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. > > > > > > > > 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? > > > > > > 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. > > > > The different treatment of the cache is easy to resolve: "all" clears first, > > "more" doesn't. > > I see, fair enough. > > > The different "submit" paths don't seem relevant for this discussion? > > I assumed that you wanted |GetMostRecentPhysicalWebPagesWithFilter| to submit > the suggestions. > > > > > 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. > > > > Do we? Don't we just want to update the cache for the to-be-submitted > > suggestions anyway, i.e. after all the filtering?\ > > 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|. Ah, so the problem is that we filter out duplicate resolved_urls, I see. > > > 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? > > > > 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. > > 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. Nah, I'm not really worried about the complexity. It just seemed like we were doing the same thing twice. https://codereview.chromium.org/2669533002/diff/60001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2669533002/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:398: ShouldInvalidateSuggestionWhenTheOnlyItsBeaconIsLost) { nit: "TheOnlyItsBeacon" doesn't parse. "ItsOnlyBeacon"?
The CQ bit was checked by vitaliii@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...
Hi treib@, I addressed your comments, no need to look. Sorry for not explaining earlier how GetMostRecentPhysicalWebPagesWithFilter was different. https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:295: // TODO(vitaliii): Use |resolved_url| here when it is available. On 2017/01/31 15:38:31, Marc Treib wrote: > On 2017/01/31 15:10:57, vitaliii wrote: > > On 2017/01/31 14:25:58, Marc Treib wrote: > > > On 2017/01/31 13:57:03, vitaliii wrote: > > > > On 2017/01/31 13:31:04, Marc Treib wrote: > > > > > On 2017/01/31 12:33:49, vitaliii wrote: > > > > > > On 2017/01/31 11:02:41, Marc Treib wrote: > > > > > > > What does this mean? It looks like sometimes we invalidate by > scanned > > > URL, > > > > > > > sometimes by resolved URL? > > > > > > > > > > > > We should invalidate by resolved URL, because the suggestion id is a > > > > resolved > > > > > > URL. > > > > > > Invalidating by scanned URL here is just a workaround. > > > > > > > > > > But that generally won't do anything, right? (Except in the case where > > > scanned > > > > > URL and resolved URL happen to be the same.) > > > > > > > > True. > > > > > > Then I'd say, rather than doing this half-fix, we should wait until we can > fix > > > it properly. > > > > Please see below why I think that this is 99% fix. > > 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 was my assumption that it is better than not invalidating at all. I extended the comments. > > > > > > > > > > > > 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? > > > > > > > > > > > > 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. > > > > > > > > > > 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. > > > > > > > > > > > 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. > > > > > > 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. > > > > 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. > > 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...) 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. > > > > > > 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? > > > > > > > > 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. > > > > > > 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? > > > > 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). > > Alright, fair enough. See above; maybe it's possible to do something about that. As I said above, the could provide metadata as an argument, however, this would be too much for M57. > > > > Anyway, it seems to me that we won't get this properly fixed for M57 anyway. > > > > 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? > > Okay, I think I see now. The case where we don't have the proper mapping > shouldn't actually ever happen. Technically yes, but without any guarantees from PW side it is impossible to be sure. > > > Apart from this speculative case this CL resolves the bug. > > 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... This is partially true. Even if they provider metadata, we will have to count beacons per |resolved_url|, unless they change the interface completely. > > > > (And really, we shouldn't IMO. Branch point has passed and things are not > > done.) > > > > But this is clearly a bug, I am not sure why we shouldn't merge it? > > 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. I see, fair enough. Let's see how this fix behaves in Canary. https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:300: const GURL lost_resolved_url = it->second; On 2017/01/31 15:38:31, Marc Treib wrote: > On 2017/01/31 15:10:57, vitaliii wrote: > > On 2017/01/31 14:25:58, Marc Treib wrote: > > > On 2017/01/31 13:57:03, vitaliii wrote: > > > > On 2017/01/31 13:31:04, Marc Treib wrote: > > > > > On 2017/01/31 12:33:49, vitaliii wrote: > > > > > > On 2017/01/31 11:02:41, Marc Treib wrote: > > > > > > > nit: ref? > > > > > > > > > > > > the multimap pair is removed in the next line. > > > > > > > > > > Ah, good point. Worth a comment maybe :) > > > > > > > > Acknowledged. > > > > > > ...but not done? :P > > > > 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. > > 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" :) I see, fair enough. My understanding of "ack" is "I have read, but done nothing.". I will try commenting "ack" in future. https://codereview.chromium.org/2669533002/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:336: for (const auto& page_metadata : *page_metadata_list) { On 2017/01/31 15:38:31, Marc Treib wrote: > On 2017/01/31 15:10:57, vitaliii wrote: > > On 2017/01/31 14:25:58, Marc Treib wrote: > > > On 2017/01/31 13:57:03, vitaliii wrote: > > > > On 2017/01/31 13:31:04, Marc Treib wrote: > > > > > On 2017/01/31 12:33:49, vitaliii wrote: > > > > > > On 2017/01/31 11:02:41, Marc Treib wrote: > > > > > > > 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? > > > > > > > > > > > > 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. > > > > > > > > > > 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? > > > > > > > > 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. > > > > > > The different treatment of the cache is easy to resolve: "all" clears first, > > > "more" doesn't. > > > > I see, fair enough. > > > > > The different "submit" paths don't seem relevant for this discussion? > > > > I assumed that you wanted |GetMostRecentPhysicalWebPagesWithFilter| to submit > > the suggestions. > > > > > > > > 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. > > > > > > Do we? Don't we just want to update the cache for the to-be-submitted > > > suggestions anyway, i.e. after all the filtering?\ > > > > 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|. > > Ah, so the problem is that we filter out duplicate resolved_urls, I see. Yes, indeed. > > > > 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? > > > > > > 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. > > > > 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. > > Nah, I'm not really worried about the complexity. It just seemed like we were > doing the same thing twice. I see, sorry for not explaining earlier. https://codereview.chromium.org/2669533002/diff/60001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2669533002/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:398: ShouldInvalidateSuggestionWhenTheOnlyItsBeaconIsLost) { On 2017/01/31 15:38:31, Marc Treib wrote: > nit: "TheOnlyItsBeacon" doesn't parse. "ItsOnlyBeacon"? Done. "ItsOnlyBeacon"
The CQ bit was unchecked by vitaliii@chromium.org
The CQ bit was checked by vitaliii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org Link to the patchset: https://codereview.chromium.org/2669533002/#ps80001 (title: "treib@ comments.")
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": 1485880893184990,
"parent_rev": "04e0f45349341e37603aa31adfbbea9e81075129", "commit_rev":
"8666d444ab37fde30061683937c842156a08156b"}
Message was sent while issue was closed.
Description was changed from ========== [NTP::PhysicalWeb] In OnLost invalidate by |resolved_url|. The OnLost notification is treated differently after this CL. Previously, the obtained url (|scanned_url|) was propagated further, which was not correct, because the |resolved_url| is used an an ID. In this CL a mapping from |scanned_url| to |resolved_url| is stored for all suggestions from the last update. This mapping is then used to obtain |resolved_url|, which is then propagated further. In addition to this, a case of multiple beacons for the same URL is considered. The suggestion is invalidated, only when its last beacon has been lost. BUG=685245 ========== to ========== [NTP::PhysicalWeb] In OnLost invalidate by |resolved_url|. The OnLost notification is treated differently after this CL. Previously, the obtained url (|scanned_url|) was propagated further, which was not correct, because the |resolved_url| is used an an ID. In this CL a mapping from |scanned_url| to |resolved_url| is stored for all suggestions from the last update. This mapping is then used to obtain |resolved_url|, which is then propagated further. In addition to this, a case of multiple beacons for the same URL is considered. The suggestion is invalidated, only when its last beacon has been lost. BUG=685245 Review-Url: https://codereview.chromium.org/2669533002 Cr-Commit-Position: refs/heads/master@{#447253} Committed: https://chromium.googlesource.com/chromium/src/+/8666d444ab37fde30061683937c8... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/8666d444ab37fde30061683937c8... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
