Closed
Bug 1261836
Opened 8 years ago
Closed 8 years ago
Content notifications: Check if new content is already in history
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox48 fixed)
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 | ||
Updated•8 years ago
|
Assignee: nobody → s.kaspari
Assignee | ||
Comment 1•8 years ago
|
||
Not a hard dependency but in bug 1232706 I'm adding a method to check the history of a single URL.
Depends on: 1232706
Assignee | ||
Comment 2•8 years ago
|
||
Note to myself: It would be interesting to add telemetry to that and see how often users are faster than the notification.
Assignee | ||
Comment 3•8 years ago
|
||
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)
Assignee | ||
Comment 4•8 years ago
|
||
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.
Assignee | ||
Comment 5•8 years ago
|
||
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•8 years ago
|
||
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+
Assignee | ||
Updated•8 years ago
|
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+
Assignee | ||
Comment 8•8 years ago
|
||
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. :)
Assignee | ||
Comment 9•8 years ago
|
||
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
Assignee | ||
Comment 10•8 years ago
|
||
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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9a49926c3edb
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•