1261836 - Content notifications: Check if new content is already in history

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

Technically you might have already seen the "new" content we notify you about. Before showing a notification we might want to check if the URL is already in the history.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Not a hard dependency but in bug 1232706 I'm adding a method to check the history of a single URL.

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Note to myself: It would be interesting to add telemetry to that and see how often users are faster than the notification.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1261836 - Content notifications: Check if new content is already in history. r=mcomella,mfinkle

If the URL of new content is already in the user's history then we won't show a notification
for it.

Review commit: https://reviewboard.mozilla.org/r/46077/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46077/

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

This can fail if the feed uses a different URL and redirects to content. But it's better than no checks at all. I did verify that it works for wordpress.com blogs.

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

Thinking about it some more it might make sense to remove query strings and anchors and see whether this URL is somewhere in the history too.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8740906 [details]
MozReview Request: Bug 1261836 - Content notifications: Check if new content is already in history. r=mcomella,mfinkle

https://reviewboard.mozilla.org/r/46077/#review42591

Looks good to me. I agree we might want to add some tweaks to how check for visits. Your idea about stripping anchors, and maybe query params are worth remembering. Shouldn't block this patch though.

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/CheckForUpdatesAction.java:81
(Diff revision 1)
>                  FeedFetcher.FeedResponse response = checkFeedForUpdates(subscription);
>                  if (response != null) {
> -                    updatedFeeds.add(response.feed);
> +                    final Feed feed = response.feed;
> +
> +                    if (!hasBeenVisited(browserDB, feed.getLastItem().getURL())) {
> +                        // Only notify update this update if the last item hasn't been visited yet.

Only notify about this update...  ?

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8740906 [details]
MozReview Request: Bug 1261836 - Content notifications: Check if new content is already in history. r=mcomella,mfinkle

https://reviewboard.mozilla.org/r/46077/#review44331

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/CheckForUpdatesAction.java:80
(Diff revision 1)
>  
>                  FeedFetcher.FeedResponse response = checkFeedForUpdates(subscription);
>                  if (response != null) {
> -                    updatedFeeds.add(response.feed);
> +                    final Feed feed = response.feed;
> +
> +                    if (!hasBeenVisited(browserDB, feed.getLastItem().getURL())) {

There's duplicated knowledge here and in `showNotification` (and the methods that it calls) that you're using the last item from the feed. It'd be more robust if you passed in this last item as an argument to these methods and used the same reference to get the url here.

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/CheckForUpdatesAction.java:122
(Diff revision 1)
>  
> +    private boolean hasBeenVisited(BrowserDB browserDB, String url) {

You should the text from https://bugzilla.mozilla.org/show_bug.cgi?id=1261836#c4 as Javadoc here.

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/CheckForUpdatesAction.java:123
(Diff revision 1)
>          }
>  
>          return null;
>      }
>  
> +    private boolean hasBeenVisited(BrowserDB browserDB, String url) {

nit: `final` here and below

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

https://reviewboard.mozilla.org/r/46077/#review44331

> There's duplicated knowledge here and in `showNotification` (and the methods that it calls) that you're using the last item from the feed. It'd be more robust if you passed in this last item as an argument to these methods and used the same reference to get the url here.

showNotification() will need the feed object though. The "last" item is actually the only item we have. :)

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8740906 [details]
MozReview Request: Bug 1261836 - Content notifications: Check if new content is already in history. r=mcomella,mfinkle

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46077/diff/1-2/

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/fx-team/rev/9a49926c3edbf1bad4424cd206f3bd30424784b4
Bug 1261836 - Content notifications: Check if new content is already in history. r=mcomella,mfinkle

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9a49926c3edb