Closed Bug 1261836 Opened 4 years ago Closed 4 years ago

Content notifications: Check if new content is already in history

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: sebastian, Assigned: sebastian)

References

Details

Attachments

(1 file)

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.
Assignee: nobody → s.kaspari
Not a hard dependency but in bug 1232706 I'm adding a method to check the history of a single URL.
Depends on: 1232706
Note to myself: It would be interesting to add telemetry to that and see how often users are faster than the notification.
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/
Attachment #8740906 - Flags: review?(michael.l.comella)
Attachment #8740906 - Flags: review?(mark.finkle)
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.
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 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...  ?
Attachment #8740906 - Flags: review?(mark.finkle) → review+
Status: NEW → ASSIGNED
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
Attachment #8740906 - Flags: review?(michael.l.comella) → review+
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 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/
Attachment #8740906 - Attachment description: MozReview Request: Bug 1261836 - Content notifications: Check if new content is already in history. r?mcomella,mfinkle → MozReview Request: Bug 1261836 - Content notifications: Check if new content is already in history. r=mcomella,mfinkle
https://hg.mozilla.org/integration/fx-team/rev/9a49926c3edbf1bad4424cd206f3bd30424784b4
Bug 1261836 - Content notifications: Check if new content is already in history. r=mcomella,mfinkle
https://hg.mozilla.org/mozilla-central/rev/9a49926c3edb
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.