Create background service for subscribing to website feeds

RESOLVED FIXED in Firefox 48

Status

()

Firefox for Android
General
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: sebastian, Assigned: sebastian)

Tracking

unspecified
Firefox 48
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(20 attachments, 5 obsolete attachments)

58 bytes, text/x-review-board-request
mcomella
: review+
Details | Review
58 bytes, text/x-review-board-request
mcomella
: review+
Details | Review
58 bytes, text/x-review-board-request
mcomella
: review+
Details | Review
58 bytes, text/x-review-board-request
mcomella
: review+
Details | Review
58 bytes, text/x-review-board-request
mcomella
: review+
Details | Review
58 bytes, text/x-review-board-request
mcomella
: review+
Details | Review
58 bytes, text/x-review-board-request
mcomella
: review+
Details | Review
58 bytes, text/x-review-board-request
mcomella
: review+
Details | Review
58 bytes, text/x-review-board-request
mcomella
: review+
Details | Review
58 bytes, text/x-review-board-request
mcomella
: review+
Details | Review
58 bytes, text/x-review-board-request
mcomella
: review+
Details | Review
58 bytes, text/x-review-board-request
mcomella
: review+
Details | Review
58 bytes, text/x-review-board-request
mcomella
: review+
Details | Review
58 bytes, text/x-review-board-request
mcomella
: review+
Details | Review
58 bytes, text/x-review-board-request
mfinkle
: review+
mcomella
: review+
Details | Review
58 bytes, text/x-review-board-request
Details | Review
58 bytes, text/x-review-board-request
Details | Review
58 bytes, text/x-review-board-request
Details | Review
58 bytes, text/x-review-board-request
Details | Review
58 bytes, text/x-review-board-request
Details | Review
(Assignee)

Description

3 years ago
To implement the "website changed" notifications (bug 1238087) we need a background service for subscribing to website feeds and regularly checking those feeds for updates. This bug tracks just the work for the (headless) background service, independently from the mechanism of choosing websites to subscribe to.

Some requirements:
* Lightweight. We are not building a full-featured feed reader
* Avoid parsing complete feeds as much as possible. Try to use HTTP headers (ETag, Modified-Since) and HEAD requests to detect changes instead of parsing feeds
* Avoid excessive usage of battery and network: We do not want to have real-time notifications.

Ideally this service can already be used / tested via an adb command - while we decide what's the best mechanism to pick sites.
(Assignee)

Updated

3 years ago
Assignee: nobody → s.kaspari
(Assignee)

Updated

2 years ago
Depends on: 1248901
(Assignee)

Updated

2 years ago
Depends on: 1250707
(Assignee)

Comment 1

2 years ago
Created attachment 8723306 [details]
MozReview Request: Bug 1241810 - Introduce SimpleFeedParser for implementing "content notifications". r?mcomella

This is just a simple XML Pull Parser that can read arbitrary ATOM and RSS feeds and returns an
object describing the feed and the latest entry. It's goal is to be small and simple rather than
a full-featured feed parser library.

Review commit: https://reviewboard.mozilla.org/r/32633/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32633/
(Assignee)

Comment 2

2 years ago
Created attachment 8723307 [details]
MozReview Request: Bug 1241810 - FeedFetcher: Simple helper class for fetching feeds using ETag and If-Modified-Since headers. r?mcomella

Review commit: https://reviewboard.mozilla.org/r/36459/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36459/
(Assignee)

Comment 3

2 years ago
Created attachment 8723308 [details]
MozReview Request: Bug 1241810 - Add JSON-based storage for feed subscriptions. r?mcomella

Review commit: https://reviewboard.mozilla.org/r/36461/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36461/
(Assignee)

Comment 4

2 years ago
Created attachment 8723309 [details]
MozReview Request: Bug 1241810 - Add background service and action to check feeds for updates. r?mcomella

Review commit: https://reviewboard.mozilla.org/r/36463/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36463/
(Assignee)

Comment 5

2 years ago
Created attachment 8723310 [details]
MozReview Request: Bug 1241810 - Add action for subscribing to new feeds. r?mcomella

Review commit: https://reviewboard.mozilla.org/r/36465/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36465/
(Assignee)

Comment 6

2 years ago
Created attachment 8723311 [details]
MozReview Request: Bug 1241810 - Add configuration/whitelist for known sites we want to show notifications for. r=mcomella

We'll start with Blogger and Medium.

Review commit: https://reviewboard.mozilla.org/r/36467/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36467/
(Assignee)

Comment 7

2 years ago
Created attachment 8723312 [details]
MozReview Request: Bug 1241810 - Add EnrollAction for finding bookmarks of known sites to subscribe to. r=mcomella

Review commit: https://reviewboard.mozilla.org/r/36469/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36469/
(Assignee)

Comment 8

2 years ago
Created attachment 8723313 [details]
MozReview Request: Bug 1241810 - Add WithdrawAction for removing subscriptions of bookmarks that do not exist anymore. r=mcomella

Review commit: https://reviewboard.mozilla.org/r/36471/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36471/
(Assignee)

Comment 9

2 years ago
Created attachment 8723314 [details]
MozReview Request: Bug 1241810 - Add SetupAction for setting up various alarms to start FeedService. r=mcomella

Review commit: https://reviewboard.mozilla.org/r/36473/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36473/
(Assignee)

Comment 10

2 years ago
Created attachment 8723315 [details]
MozReview Request: Bug 1241810 - Run setup on app start and when the device boots. r=mcomella

Review commit: https://reviewboard.mozilla.org/r/36475/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36475/
(Assignee)

Comment 11

2 years ago
Created attachment 8723316 [details]
MozReview Request: Bug 1241810 - Do not do anything if not connected to a network or network is metered. r=mcomella

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

::: mobile/android/base/java/org/mozilla/gecko/feeds/FeedService.java:114
(Diff revision 1)
> +    private boolean isConnectedToNetwork() {

Do these need to be separate calls?
https://reviewboard.mozilla.org/r/32633/#review33089

::: mobile/android/app/src/main/resources/feed_atom_feedburner.xml:1
(Diff revision 1)
> +<?xml version="1.0" encoding="UTF-8"?>

mobile/android/app/src/main/resources
is this a real location we should be putting files? These look like test files. Shouldn't they be in a "test" location?

::: mobile/android/base/java/org/mozilla/gecko/feeds/parser/SimpleFeedParser.java:29
(Diff revision 1)
> +public class SimpleFeedParser {

Adding our own parser is a slippery slope. This is dangerous. Be careful.

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/feeds/parser/TestSimpleFeedParser.java:25
(Diff revision 1)
> +public class TestSimpleFeedParser {

I'd like to see a Wordpress feed in here as well.
https://reviewboard.mozilla.org/r/36465/#review33079

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/SubscribeAction.java:38
(Diff revision 1)
> +        // TODO: Using a random UUID as fallback just so that I can subscribe for things that are not bookmarks (testing)

We should probably not tie this to bookmarks. We talked about just looking at your recent history too. Why not just use URLs?
https://reviewboard.mozilla.org/r/36467/#review33081

::: mobile/android/base/moz.build:283
(Diff revision 1)
> +    'feeds/knownsites/KnownSiteMedium.java',

Why not Wordpress? Wordpress self hosted accounts for about 25% of the Web:
http://ma.tt/2015/11/seventy-five-to-go/
https://reviewboard.mozilla.org/r/36469/#review33083

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1625
(Diff revision 1)
> +    public Cursor getBookmarksForPartialUrl(ContentResolver cr, String partialUrl) {

Again, I'm not sure we should tie ourselves to bookmarks here.
https://reviewboard.mozilla.org/r/36473/#review33085

::: mobile/android/base/java/org/mozilla/gecko/feeds/FeedAlarmReceiver.java:17
(Diff revision 1)
> +public class FeedAlarmReceiver extends WakefulBroadcastReceiver {

Why not just use the FeedService for this directly?
https://reviewboard.mozilla.org/r/36475/#review33087

::: mobile/android/base/java/org/mozilla/gecko/BootReceiver.java:17
(Diff revision 1)
> +public class BootReceiver extends BroadcastReceiver {

Again, why not just use the FeedService directly?
Let's not tie to Bookmarks. Please use URLs.
(Assignee)

Comment 20

2 years ago
(In reply to Mark Finkle (:mfinkle) from comment #12)
> https://reviewboard.mozilla.org/r/36477/#review33091
> 
> ::: mobile/android/base/java/org/mozilla/gecko/feeds/FeedService.java:114
> (Diff revision 1)
> > +    private boolean isConnectedToNetwork() {
> 
> Do these need to be separate calls?

Nope, I can merge them.


(In reply to Mark Finkle (:mfinkle) from comment #13)
> mobile/android/app/src/main/resources
> is this a real location we should be putting files? These look like test
> files. Shouldn't they be in a "test" location?

Good point. Back when I started working on this patch this looked like it was only used by the test package in the gradle configuration; but this isn't true (anymore?). I'll see how/where I can move them.


(In reply to Mark Finkle (:mfinkle) from comment #13)
> Adding our own parser is a slippery slope. This is dangerous. Be careful.

I agree. I looked at some feed libraries out there, but:
* They are massive. Some include their own HTTP clients, and caching implementations.
* They build "perfect" representations of the feed in memory. But we do not care about most things in the feed.

Just peeking into a feed, grabbing some values and getting out fast seemed to be a good alternative. As long as we use a whitelist of known sites and have unit tests for all of them I'm pretty confident that this is "good enough". Nevertheless every test case I added uncovered some case that was not handled properly.


(In reply to Mark Finkle (:mfinkle) from comment #13)
> I'd like to see a Wordpress feed in here as well.

OK.


(In reply to Mark Finkle (:mfinkle) from comment #14)
> We should probably not tie this to bookmarks. We talked about just looking
> at your recent history too. Why not just use URLs?

On Monday we decided to start with bookmarks because that's a very strong indication that the user somehow cares about this page. But I'm open to changing/improving this - either before we push it to Nightly or during the test. Is recent history something we can add after this is in Nightly? Subscribing to websites from recent history should be easy; How about unsubscribing? As soon as this is not in "recent history" anymore?


(In reply to Mark Finkle (:mfinkle) from comment #15)
> Why not Wordpress? Wordpress self hosted accounts for about 25% of the Web:
> http://ma.tt/2015/11/seventy-five-to-go/

Okay, I'll add that. However are we talking about (custom) Wordpress installations or blogs hosted on wordpress.com? The initial list of known sites was pretty much random and definitely should be extended.


(In reply to Mark Finkle (:mfinkle) from comment #16)
> Again, I'm not sure we should tie ourselves to bookmarks here.


For the storage I want to switch to the new URL annotations table (bug 1250707). After that we should be pretty much independent from bookmarks - at least from a storage point of view. Finding websites to subscribe to from different sources is easy but I wonder how do we know what to unsubscribe from? For bookmarks this is easy: Bookmark exists -> Subscribe, Bookmark does not exist anymore -> Unsubscribe.


(In reply to Mark Finkle (:mfinkle) from comment #17)
> https://reviewboard.mozilla.org/r/36473/#review33085
> 
> :::
> mobile/android/base/java/org/mozilla/gecko/feeds/FeedAlarmReceiver.java:17
> (Diff revision 1)
> > +public class FeedAlarmReceiver extends WakefulBroadcastReceiver {
> 
> Why not just use the FeedService for this directly?

The AlarmManager sends broadcasts and the system sends a broadcast after boot. I don't think we can start a service directly from those broadcasts?


(In reply to Mark Finkle (:mfinkle) from comment #19)
> Let's not tie to Bookmarks. Please use URLs.

To summarize the comments from above: Technically this should be independent from bookmarks after I switch to the new database table (bug 1250707). However to make a decision about whether this site is still relevant to the user I need to look at the bookmark, history, .. wherever we picked this site up. Or is there some better alternative?
(Assignee)

Comment 21

2 years ago
Open ToDos on my list:
* Switch to url annotations table: bug 1250707
* Notifications setting to disable this: bug 1247788
* More tests
* Talk to Anthony about what we should show in the notification and if/how we should group them
* Add telemetry

@mfinkle: Embarrassingly I still don't know how to create Telemetry calls properly (What event or method do I use?). I guess we are interested in:
* Notification shown
* Notification clicked
And maybe:
* Subscribed to website
* Unsubscribed from website
Flags: needinfo?(mark.finkle)

Comment 22

2 years ago
(In reply to Sebastian Kaspari (:sebastian) from comment #21)

> @mfinkle: Embarrassingly I still don't know how to create Telemetry calls
> properly (What event or method do I use?).

I've been referencing the UI telemetry docs here:
https://gecko.readthedocs.org/en/latest/mobile/android/fennec/uitelemetry.html

Although I agree I also have a hard time knowing the best event/method types to use, since it depends on how we want to do an analysis.

> I guess we are interested in:
> * Notification shown
> * Notification clicked

I imagine we'd do something like:

(SHOW, NOTIFICATION, "content_update")
(ACTION, NOTIFICATION, "content_update")

> And maybe:
> * Subscribed to website
> * Unsubscribed from website

Do we have UI for these? Would this also be part of the notification UI? If so, maybe we could do something like:

(ACTION, NOTIFICATION, "content_update_subscribe")
(ACTION, NOTIFICATION, "content_update_unsubscribe")
(In reply to :Margaret Leibovic from comment #22)
> (In reply to Sebastian Kaspari (:sebastian) from comment #21)
> 
> > @mfinkle: Embarrassingly I still don't know how to create Telemetry calls
> > properly (What event or method do I use?).
> 
> I've been referencing the UI telemetry docs here:
> https://gecko.readthedocs.org/en/latest/mobile/android/fennec/uitelemetry.
> html
> 
> Although I agree I also have a hard time knowing the best event/method types
> to use, since it depends on how we want to do an analysis.
> 
> > I guess we are interested in:
> > * Notification shown
> > * Notification clicked
> 
> I imagine we'd do something like:
> 
> (SHOW, NOTIFICATION, "content_update")
> (ACTION, NOTIFICATION, "content_update")

Yes, and remember to add a (ACTION, BUTTON, "notification-settings") for the button that... opens the notification settings UI.

> > And maybe:
> > * Subscribed to website
> > * Unsubscribed from website
> 
> Do we have UI for these? Would this also be part of the notification UI? If
> so, maybe we could do something like:
> 
> (ACTION, NOTIFICATION, "content_update_subscribe")
> (ACTION, NOTIFICATION, "content_update_unsubscribe")

Instead of (ACTION, NOTIFICATION, "content_update_subscribe"), I would suggest (SAVE, SERVICE, "content_update")

Reasons:
* ACTION is something a user explicitly does, with their finger :). This is more about the service doing the subscription.
* SAVE captures the explicit act of saving (or UNSAVE/unsaving) something important and worth tracking. This seems like a SAVE operation.
* If the user can press a UI element to subscribe or unsubscribe, we'd fire two probes: 1 for the ACTION and 1 for the SAVE/UNSAVE. We do this for bookmarks right now.

*
Flags: needinfo?(mark.finkle)
Yes, my suggestion of SERVICE creates a new method. One used for automatic systems making decisions. We also have SYSTEM, but this seems different from SYSTEM which is supposed to be for OS level things. I also think we have more opportunities for using SERVICE in other engagement projects involving background services.
(Assignee)

Comment 25

2 years ago
Comment on attachment 8723306 [details]
MozReview Request: Bug 1241810 - Introduce SimpleFeedParser for implementing "content notifications". r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32633/diff/1-2/
Attachment #8723306 - Attachment description: MozReview Request: Bug 1241810 - Introduce SimpleFeedParser for implementing "content notifications". r? → MozReview Request: Bug 1241810 - Introduce SimpleFeedParser for implementing "content notifications". r?mcomella
Attachment #8723306 - Flags: review?(michael.l.comella)
(Assignee)

Comment 26

2 years ago
Comment on attachment 8723307 [details]
MozReview Request: Bug 1241810 - FeedFetcher: Simple helper class for fetching feeds using ETag and If-Modified-Since headers. r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36459/diff/1-2/
Attachment #8723307 - Attachment description: MozReview Request: Bug 1241810 - FeedFetcher: Simple helper class for fetching feeds using ETag and If-Modified-Since headers. r? → MozReview Request: Bug 1241810 - FeedFetcher: Simple helper class for fetching feeds using ETag and If-Modified-Since headers. r?mcomella
Attachment #8723307 - Flags: review?(michael.l.comella)
(Assignee)

Updated

2 years ago
Attachment #8723308 - Attachment description: MozReview Request: Bug 1241810 - Add JSON-based storage for feed subscriptions. r? → MozReview Request: Bug 1241810 - Add JSON-based storage for feed subscriptions. r?mcomella
Attachment #8723308 - Flags: review?(michael.l.comella)
(Assignee)

Comment 27

2 years ago
Comment on attachment 8723308 [details]
MozReview Request: Bug 1241810 - Add JSON-based storage for feed subscriptions. r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36461/diff/1-2/
(Assignee)

Updated

2 years ago
Attachment #8723309 - Attachment description: MozReview Request: Bug 1241810 - Add background service and action to check feeds for updates. r? → MozReview Request: Bug 1241810 - Add background service and action to check feeds for updates. r?mcomella
Attachment #8723309 - Flags: review?(michael.l.comella)
(Assignee)

Comment 28

2 years ago
Comment on attachment 8723309 [details]
MozReview Request: Bug 1241810 - Add background service and action to check feeds for updates. r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36463/diff/1-2/
(Assignee)

Comment 29

2 years ago
Comment on attachment 8723310 [details]
MozReview Request: Bug 1241810 - Add action for subscribing to new feeds. r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36465/diff/1-2/
Attachment #8723310 - Attachment description: MozReview Request: Bug 1241810 - Add action for subscribing to new feeds. r? → MozReview Request: Bug 1241810 - Add action for subscribing to new feeds. r?mcomella
Attachment #8723310 - Flags: review?(michael.l.comella)
(Assignee)

Comment 30

2 years ago
Comment on attachment 8723311 [details]
MozReview Request: Bug 1241810 - Add configuration/whitelist for known sites we want to show notifications for. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36467/diff/1-2/
Attachment #8723311 - Attachment description: MozReview Request: Bug 1241810 - Add configuration/whitelist for known sites we want to show notifications for. r? → MozReview Request: Bug 1241810 - Add configuration/whitelist for known sites we want to show notifications for. r?mcomella
Attachment #8723311 - Flags: review?(michael.l.comella)
(Assignee)

Updated

2 years ago
Attachment #8723312 - Attachment description: MozReview Request: Bug 1241810 - Add EnrollAction for finding bookmarks of known sites to subscribe to. r? → MozReview Request: Bug 1241810 - Add EnrollAction for finding bookmarks of known sites to subscribe to. r?mcomella
Attachment #8723312 - Flags: review?(michael.l.comella)
(Assignee)

Comment 31

2 years ago
Comment on attachment 8723312 [details]
MozReview Request: Bug 1241810 - Add EnrollAction for finding bookmarks of known sites to subscribe to. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36469/diff/1-2/
(Assignee)

Comment 32

2 years ago
Comment on attachment 8723313 [details]
MozReview Request: Bug 1241810 - Add WithdrawAction for removing subscriptions of bookmarks that do not exist anymore. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36471/diff/1-2/
Attachment #8723313 - Attachment description: MozReview Request: Bug 1241810 - Add WithdrawAction for removing subscriptions of bookmarks that do not exist anymore. r? → MozReview Request: Bug 1241810 - Add WithdrawAction for removing subscriptions of bookmarks that do not exist anymore. r?mcomella
Attachment #8723313 - Flags: review?(michael.l.comella)
(Assignee)

Comment 33

2 years ago
Comment on attachment 8723314 [details]
MozReview Request: Bug 1241810 - Add SetupAction for setting up various alarms to start FeedService. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36473/diff/1-2/
Attachment #8723314 - Attachment description: MozReview Request: Bug 1241810 - Add SetupAction for setting up various alarms to start FeedService. r? → MozReview Request: Bug 1241810 - Add SetupAction for setting up various alarms to start FeedService. r?mcomella
Attachment #8723314 - Flags: review?(michael.l.comella)
(Assignee)

Comment 34

2 years ago
Comment on attachment 8723315 [details]
MozReview Request: Bug 1241810 - Run setup on app start and when the device boots. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36475/diff/1-2/
Attachment #8723315 - Attachment description: MozReview Request: Bug 1241810 - Run setup on app start and when the device boots. r? → MozReview Request: Bug 1241810 - Run setup on app start and when the device boots. r?mcomella
Attachment #8723315 - Flags: review?(michael.l.comella)
(Assignee)

Updated

2 years ago
Attachment #8723316 - Attachment description: MozReview Request: Bug 1241810 - Do not do anything if not connected to a network or network is metered. r? → MozReview Request: Bug 1241810 - Do not do anything if not connected to a network or network is metered. r?mcomella
Attachment #8723316 - Flags: review?(michael.l.comella)
(Assignee)

Comment 35

2 years ago
Comment on attachment 8723316 [details]
MozReview Request: Bug 1241810 - Do not do anything if not connected to a network or network is metered. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36477/diff/1-2/
(Assignee)

Comment 36

2 years ago
Created attachment 8726176 [details]
MozReview Request: Bug 1241810 - FeedSubscription: Treat ETag and "Last modified" header as optional. r=mcomella

Some services just do not return any of those headers.

Review commit: https://reviewboard.mozilla.org/r/36765/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36765/
Attachment #8726176 - Flags: review?(michael.l.comella)
(Assignee)

Comment 37

2 years ago
Created attachment 8726177 [details]
MozReview Request: Bug 1241810 - Bug 1238087 - Update style of content notification. r=mcomella

Group multiple updates into one notification and use a different style
for single and multiple updates.

Review commit: https://reviewboard.mozilla.org/r/36767/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36767/
Attachment #8726177 - Flags: review?(michael.l.comella)
(Assignee)

Comment 38

2 years ago
Created attachment 8726178 [details]
MozReview Request: Bug 1241810 - Restructure FeedService to always complete wakeful intent. r=mcomella

Review commit: https://reviewboard.mozilla.org/r/36769/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36769/
Attachment #8726178 - Flags: review?(michael.l.comella)
(Assignee)

Comment 39

2 years ago
Created attachment 8726179 [details]
MozReview Request: Bug 1241810 - Bug 1247788 - Add "Notifications" in Settings. r=mcomella

Review commit: https://reviewboard.mozilla.org/r/36771/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36771/
Attachment #8726179 - Flags: review?(michael.l.comella)
(Assignee)

Comment 40

2 years ago
Created attachment 8726180 [details]
MozReview Request: Bug 1241810 - Add support for wordpress.com. r=mcomella

Review commit: https://reviewboard.mozilla.org/r/36997/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36997/
Attachment #8726180 - Flags: review?(michael.l.comella)
(Assignee)

Comment 41

2 years ago
Created attachment 8726181 [details]
MozReview Request: Bug 1241810 - Add additional test case for Blogger. r=mcomella

Review commit: https://reviewboard.mozilla.org/r/36999/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36999/
Attachment #8726181 - Flags: review?(michael.l.comella)
(Assignee)

Comment 42

2 years ago
Created attachment 8726182 [details]
MozReview Request: Bug 1241810 - Use UrlAnnotations table as storage. r=mcomella

This patch removes the JSON flat file based storage and instead uses the
new url_annotations table.

Two mappings will be created in the database:
* Key "feed": website URL -> feed URL
  This maps an URL to its feed URL. Multiple URLs can have the same feed
  URL. Later this mapping could be used to highlight URLs with feeds or
  subscriptions in the UI.
* Key "feed_subscription": feed URL -> Object describing the feed
  This is the actual subscription and saves the state of a subscription
  linked to the feed URL.

Review commit: https://reviewboard.mozilla.org/r/37001/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37001/
Attachment #8726182 - Flags: review?(michael.l.comella)
(Assignee)

Comment 43

2 years ago
Created attachment 8726183 [details]
MozReview Request: Bug 1241810 - Bug 1238087 - Add "notifications setting" action to content notification. r=mcomella

Review commit: https://reviewboard.mozilla.org/r/37841/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37841/
Attachment #8726183 - Flags: review?(michael.l.comella)
(Assignee)

Comment 44

2 years ago
Created attachment 8726184 [details]
MozReview Request: Bug 1241810 - Bug 1251317 - Add telemetry for content notifications. r=mcomella,mfinkle

Review commit: https://reviewboard.mozilla.org/r/37843/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37843/
Attachment #8726184 - Flags: review?(michael.l.comella)
Attachment #8726184 - Flags: review?(mark.finkle)
(Assignee)

Comment 45

2 years ago
@mcomella: Sorry, that's a lot of stuff. Let me know if I should give you a high-level description about what this does. Some follow-up patches required small refactorings so maybe it makes sense to look at the "Squashed Diff" too - but this will show ALL changes.
(Assignee)

Comment 46

2 years ago
(In reply to Mark Finkle (:mfinkle) from comment #13)
> https://reviewboard.mozilla.org/r/32633/#review33089
> 
> ::: mobile/android/app/src/main/resources/feed_atom_feedburner.xml:1
> (Diff revision 1)
> > +<?xml version="1.0" encoding="UTF-8"?>
> 
> mobile/android/app/src/main/resources
> is this a real location we should be putting files? These look like test
> files. Shouldn't they be in a "test" location?

@nalexander: I'm looking for a place to put files that I can use in my junit4 tests. Those files should only be read by a test and not ship. Do you have a suggestion? I picked the one above (mobile/android/app/src/main/resources) because that's where robolectric.properties lives - and this is read by the tests. At some point we had a resource directory defined in test { .. } but this seems to be gone - maybe it's time to add one again? However I need to test if I can access files in there from a junit4 test case somehow.
Flags: needinfo?(nalexander)
(Assignee)

Comment 47

2 years ago
@mcomella: Oh, and regarding the UrlAnnotations changed: They might clash with your changes in bug 1243558. I'll rebase and adapt those after your patches have landed.
Comment on attachment 8726184 [details]
MozReview Request: Bug 1241810 - Bug 1251317 - Add telemetry for content notifications. r=mcomella,mfinkle

https://reviewboard.mozilla.org/r/37843/#review34813

Looks good, but if we are opening URLs, I want to see a LOAD_URL probe somewhere too.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:3726
(Diff revision 1)
>              Telemetry.sendUIEvent(TelemetryContract.Event.ACTION, TelemetryContract.Method.NOTIFICATION, "tabqueue");

Just marking this for reference

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:3744
(Diff revision 1)
> +                Telemetry.sendUIEvent(TelemetryContract.Event.ACTION, TelemetryContract.Method.NOTIFICATION, "content_update");

This matches "tabqueue" above, but openQueuedTabs will also add LOAD_URL telemetry. Does openUrls do that?

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#976
Attachment #8726184 - Flags: review?(mark.finkle) → review+
(In reply to Sebastian Kaspari (:sebastian) from comment #46)
> (In reply to Mark Finkle (:mfinkle) from comment #13)
> > https://reviewboard.mozilla.org/r/32633/#review33089
> > 
> > ::: mobile/android/app/src/main/resources/feed_atom_feedburner.xml:1
> > (Diff revision 1)
> > > +<?xml version="1.0" encoding="UTF-8"?>
> > 
> > mobile/android/app/src/main/resources
> > is this a real location we should be putting files? These look like test
> > files. Shouldn't they be in a "test" location?
> 
> @nalexander: I'm looking for a place to put files that I can use in my
> junit4 tests. Those files should only be read by a test and not ship. Do you
> have a suggestion? I picked the one above
> (mobile/android/app/src/main/resources) because that's where
> robolectric.properties lives - and this is read by the tests. At some point
> we had a resource directory defined in test { .. } but this seems to be gone
> - maybe it's time to add one again? However I need to test if I can access
> files in there from a junit4 test case somehow.

I don't remember why this is in app/src/MAIN/resources -- I would have expected to put it in app/src/TEST/resources.

The default location for unittest data files is app/src/test/resources, to the best of my knowledge.  If it works, that's my preference.  Otherwise, feel free to add a resources declaration.
Flags: needinfo?(nalexander)
Comment on attachment 8723306 [details]
MozReview Request: Bug 1241810 - Introduce SimpleFeedParser for implementing "content notifications". r?mcomella

https://reviewboard.mozilla.org/r/32633/#review35207

Still working on reviewing this patch – I'll hunker down and get a few more of these done tomorrow.

::: mobile/android/base/java/org/mozilla/gecko/feeds/parser/Feed.java:27
(Diff revision 2)
> +    /* package-private */ Feed() {}

nit: Why do you add a comment for the access level here? It doesn't seem necessary to me.

nit: Is there any reason you prefer package private over `protected`?

::: mobile/android/base/java/org/mozilla/gecko/feeds/parser/Feed.java:49
(Diff revision 2)
> +        if (TextUtils.isEmpty(title)) {

nit: It's more complex to edit but slightly more condensed to read if you used one if statement:

if (cond1 ||
        cond2 || ...

::: mobile/android/base/java/org/mozilla/gecko/feeds/parser/Feed.java:71
(Diff revision 2)
> +            return true; // How to detect if this same item and it only has been updated?

nit: Is this solved later? Should there be a TODO or a bug # here?

::: mobile/android/base/java/org/mozilla/gecko/feeds/parser/Feed.java:79
(Diff revision 2)
> +        if (!lastItem.getURL().equals(otherItem.getURL())) {
> +            // URL changed: Could be a different item

This last part of the algorithm seems to assume we're comparing this to an older version of the same feed. I think that assumption should be made clearer by indicating that in the method name (can't think of any good ones now!) & argument name and/or asserting that the feed is the same (e.g. same url) before making comparisons.

::: mobile/android/base/java/org/mozilla/gecko/feeds/parser/Item.java:44
(Diff revision 2)
> +     * Return the number of milliseconds since Jan. 1, 1970, midnight GMT.

nit: `@return`
Attachment #8723306 - Flags: review?(michael.l.comella)
Attachment #8723306 - Flags: review?(michael.l.comella)
Attachment #8723306 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8723306 [details]
MozReview Request: Bug 1241810 - Introduce SimpleFeedParser for implementing "content notifications". r?mcomella

https://reviewboard.mozilla.org/r/32633/#review35457

This appears that it would correctly handle valid feeds (if I correctly assume what the valid feed format is) but I'm afraid of how it might handle invalid data. In order to not block you from landing (i.e. file a follow-up if you don't fix it now!), r+.

Two concerning issues I saw (marked in the review) were:
* We make assumptions that the feed is valid (e.g. we replace the current item if we ever see another `<item>` tag even if the last one is closed)
* We make assumptions about the parent tag (e.g. if we're not in `<image>` or `<source>`, update the title. What if there is another parent tag we missed?)

I saw an approach that could be helpful here: I read [this brief essay](http://www.xmlpull.org/history/index.html) on pull parsers and it mentioned the benefit is that the code can follow the structure of the feed (their example is useful). In this case, we could follow their example by:

while (true) {
  // handle simple top-level tags
  // ...
  } else if (start_tag == "item") {
    handleItem(...);
  }
  // ...
}

where handleItem will handle all containing tags until it sees it's matching END_TAG. This creates a more encapsulated and accurate state machine where the inputs we parse inside the `<item>` parent tag are more limited (e.g. we could throw if we see another `<item>` tag while handling an item tag).

::: mobile/android/base/java/org/mozilla/gecko/feeds/parser/SimpleFeedParser.java:69
(Diff revision 2)
> +    public Feed parse(InputStream in) throws ParserException, IOException {

Any reason not to make this (and all of the other helper methods) `static`?

::: mobile/android/base/java/org/mozilla/gecko/feeds/parser/SimpleFeedParser.java:77
(Diff revision 2)
> +            parser.setInput(in, null);

I see in the `XmlPullParser` docs that you can set the parser to run in validation mode – is there a reason we didn't run in that? If so, I'd add a comment.

::: mobile/android/base/java/org/mozilla/gecko/feeds/parser/SimpleFeedParser.java:83
(Diff revision 2)
> +                    case XmlPullParser.START_DOCUMENT:
> +                        handleStartDocument(state);
> +                        break;

nit: I think it'd be clearer that this is only called when no data has been read yet and thus can only be called once (as the docs say) if it was read outside of the loop.

But at the same time, it breaks encapsulation of the tag logic... so your call.

::: mobile/android/base/java/org/mozilla/gecko/feeds/parser/SimpleFeedParser.java:128
(Diff revision 2)
> +            case TAG_ITEM:
> +            case TAG_ENTRY:
> +                state.currentItem = new Item();
> +                break;

This will cause problems if item or entry tags can appear inside one another. While this might be against the feed specification, we could still receive an invalid feed – you should probably ensure `state.currentItem == null` before assigning (and it also has the benefit of explicitly stating assumptions in the code).

::: mobile/android/base/java/org/mozilla/gecko/feeds/parser/SimpleFeedParser.java:181
(Diff revision 2)
> +        if (state.inSource || state.inImage) {
> +            // We do not care about titles in <source> or <image> tags.
> +            return;
> +        }

Ignoring specific tags as a blacklist is concerning – do we know for sure that there are no other feed elements that contain `<title>`? What if a new revision of RSS comes out that contains a new element that contains a title?

I think this would be safer if we used a whitelist by checking what the most recent parent tag is – see my overview comment about an alternative approach.

Similarly for the other `handle*StartTag` methods.

::: mobile/android/base/java/org/mozilla/gecko/feeds/parser/SimpleFeedParser.java:268
(Diff revision 2)
> +        try {
> +            Date date = format.parse(pubDate);
> +            state.currentItem.setTimestamp(date.getTime());
> +        } catch (ParseException e) {
> +            Log.w(LOGTAG, "Could not parse 'pubDate': " + pubDate);
> +        }

This code (and some of the code above this) is repeated in this method, updated, & date – should this be a helper function?

::: mobile/android/base/java/org/mozilla/gecko/feeds/parser/SimpleFeedParser.java:298
(Diff revision 2)
> +        updated = updated.replaceFirst("Z$", "+0000");
> +        updated = updated.replaceFirst("([0-9]{2})([\\+\\-])([0-9]{2}):([0-9]{2})$", "$1$2$3$4");

Can you give some examples in the comments?

::: mobile/android/base/java/org/mozilla/gecko/feeds/parser/SimpleFeedParser.java:304
(Diff revision 2)
> +                state.currentItem.setTimestamp(date.getTime());

I see a few `TAG_*`, PUBDATE, UPDATED, and DATE, are competing to assign to the same field – should we prefer one over the other so it's more consistent (e.g. a feed reorders an element)?

::: mobile/android/base/java/org/mozilla/gecko/feeds/parser/SimpleFeedParser.java:332
(Diff revision 2)
> +            throw  new RuntimeException(e);

The other two versions of this method don't throw `RuntimeException` – I assume this was not intentional?

::: mobile/android/base/java/org/mozilla/gecko/feeds/parser/SimpleFeedParser.java:349
(Diff revision 2)
> +        while (parser.next() != XmlPullParser.END_DOCUMENT) {
> +            if (parser.getEventType() == XmlPullParser.TEXT) {
> +                builder.append(parser.getText());
> +            } else if (parser.getEventType() == XmlPullParser.END_TAG && tag.equals(parser.getName())) {
> +                break;
> +            }
> +        }

I'm unfamiliar with the RSS spec, but this can be concerning if a new tag opens and we start appending its mysterious text content, e.g.:

`<title>We are trying to get this text<image src=...>Pretend this is alt-text for an emoji</image> and we want this text too</title>`

But I can see a case for it with `<title><a ...></a></title>`. Can you add a comment explaining your thinking here?

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/feeds/parser/TestSimpleFeedParser.java:25
(Diff revision 2)
> +public class TestSimpleFeedParser {

Might be good to add some invalid feeds in here too, e.g. empty feeds, feeds with `<item>` inside `<item>` tags, invalid xml (e.g. non-closed tags, broken tags), etc.
Attachment #8723307 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8723307 [details]
MozReview Request: Bug 1241810 - FeedFetcher: Simple helper class for fetching feeds using ETag and If-Modified-Since headers. r?mcomella

https://reviewboard.mozilla.org/r/36459/#review35481

This is the first time I've seen `HttpURLConnection` in action but it looks about right.

::: mobile/android/base/java/org/mozilla/gecko/feeds/FeedFetcher.java:51
(Diff revision 2)
> +    public static FeedResponse fetchAndParseFeedIfModified(String url, String eTag, String lastModified) {

nit: `@Nullable` annotation
nit: javadoc explaining what happens when eTag or lastModified is null.

::: mobile/android/base/java/org/mozilla/gecko/feeds/FeedFetcher.java:76
(Diff revision 2)
> +            if (!TextUtils.isEmpty(responseEtag) && responseEtag.startsWith("W/")) {
> +                // Weak ETag, get actual ETag value
> +                responseEtag = responseEtag.substring(2);

Why do we remove the weak indicator? I'd assume the server would want the identical etag back.
Comment on attachment 8723308 [details]
MozReview Request: Bug 1241810 - Add JSON-based storage for feed subscriptions. r?mcomella

https://reviewboard.mozilla.org/r/36461/#review35485

r+. Didn't look over `SubscriptionStorage` because it's later removed.

::: mobile/android/base/java/org/mozilla/gecko/feeds/subscriptions/FeedSubscription.java:27
(Diff revision 2)
> +    private static final String JSON_KEY_LAST_MODIFIED = "lastModified";

nit: last_modified

::: mobile/android/base/java/org/mozilla/gecko/feeds/subscriptions/FeedSubscription.java:86
(Diff revision 2)
> +    public boolean isNewer(FeedFetcher.FeedResponse response) {

This method is duplicated from `Feed` – is there any place we can put this code so we don't have to repeat it?
Attachment #8723308 - Flags: review?(michael.l.comella) → review+
https://reviewboard.mozilla.org/r/36461/#review35493

::: mobile/android/base/java/org/mozilla/gecko/feeds/subscriptions/FeedSubscription.java:86
(Diff revision 2)
> +    public boolean isNewer(FeedFetcher.FeedResponse response) {

nit: Maybe change the name to make it clearer what's newer than what, e.g. `this.isNewerThan(...)`
Attachment #8723309 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8723309 [details]
MozReview Request: Bug 1241810 - Add background service and action to check feeds for updates. r?mcomella

https://reviewboard.mozilla.org/r/36463/#review35491

::: mobile/android/base/java/org/mozilla/gecko/feeds/FeedService.java:25
(Diff revision 2)
> +    public static final String ACTION_CHECK = AppConstants.ANDROID_PACKAGE_NAME + ".FEEDS.CHECK";

nit: system convention for actions is kind of, `<package>.intent.action.ACTION_NAME`: https://developer.android.com/reference/android/content/Intent.html#ACTION_SEND

But I don't think it matters that much.

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/CheckAction.java:48
(Diff revision 2)
> +            Log.i(LOGTAG, "Checking feed: " + subscription.getFeedTitle());

This may be considered too personal to log.

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/CheckAction.java:56
(Diff revision 2)
> +                Log.d(LOGTAG, "* Feed has changed. New item: " + response.feed.getLastItem().getTitle());

Also potentially too personal.

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/CheckAction.java:60
(Diff revision 2)
> +                notify(response.feed);

Perhaps for a follow-up: do we want to display a separate notification for each item of content that's been updated?

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/CheckAction.java:65
(Diff revision 2)
> +    private void notify(Feed feed) {

nit: -> `notifyUser` or `addNotification` or similar

I associated `notify` with dispatching an Event and was confused by the title.

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/CheckAction.java:72
(Diff revision 2)
> +        intent.setComponent(new ComponentName(context, BrowserApp.class));

nit: You can combine this with the constructor by passing the class into the constructor.

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/CheckAction.java:87
(Diff revision 2)
> +        NotificationManagerCompat.from(context).notify(R.id.websiteContentNotification, notification);

The ID is constant so I believe that means we can only show one notification at a time – is that desireable?
Comment on attachment 8723310 [details]
MozReview Request: Bug 1241810 - Add action for subscribing to new feeds. r?mcomella

https://reviewboard.mozilla.org/r/36465/#review35497

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/SubscribeAction.java:43
(Diff revision 2)
> +            Log.d(LOGTAG, "Already subscribed to " + feedUrl + ". Skipping.");

Also too personal

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/SubscribeAction.java:53
(Diff revision 2)
> +            Log.w(LOGTAG, String.format("Could not fetch feed (%s). Not subscribing for now.", feedUrl));

Too personal to log.

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/SubscribeAction.java:57
(Diff revision 2)
> +        Log.d(LOGTAG, "Subscribing to feed: " + response.feed.getTitle());
> +        Log.d(LOGTAG, "               GUID: " + guid);
> +        Log.d(LOGTAG, "          Last item: " + response.feed.getLastItem().getTitle());

Too personal
Attachment #8723310 - Flags: review?(michael.l.comella) → review+
(Assignee)

Comment 57

2 years ago
Thanks for the review so far, mcomella! Very good points. I'll go and address them in follow-up patches (I'll push them to this bug later too; and I'll only land this as one package) because navigating back and forth in this patch history makes my head hurt. :)
Comment on attachment 8723311 [details]
MozReview Request: Bug 1241810 - Add configuration/whitelist for known sites we want to show notifications for. r=mcomella

https://reviewboard.mozilla.org/r/36467/#review35645

No r+ because I think the regex are wrong and since they're tricky, we should probably discuss and agree on them.

::: mobile/android/base/java/org/mozilla/gecko/feeds/knownsites/KnownSite.java:13
(Diff revision 2)
> +     * Get a (partial) domain we can look for in our database to find URLs pointing to this site.
> +     *
> +     * Example:
> +     * - medium.com

This comment doesn't really explain how "search" comes into play – can you give an example?

e.g. I'm thinking of querying their list of posts on their website like, `medium.com?postSearch=postName`

But perhaps you meant searching the DB? So `getDomainForDBSearch` or similar? In that case, why is one `.blogspot.com` and the other `medium.com`?

::: mobile/android/base/java/org/mozilla/gecko/feeds/knownsites/KnownSite.java:25
(Diff revision 2)
> +     * - Output: https://medium.com/feed/@antlam

nit: `@return the url representing a feed, or null if a feed could not be determined`

::: mobile/android/base/java/org/mozilla/gecko/feeds/knownsites/KnownSiteBlogger.java:22
(Diff revision 2)
> +        Pattern pattern = Pattern.compile("https?://(www\\.)?(.*?)\\.blogspot\\.com(/.+)?");

I think you'd want:

`https?://(www\\.)?(.*)\\.blogspot\\.com(/.*)?`

My two changes:
* `(.*?)` -> `(.*)`: the `*` indicates the item should be repeated 0 or more times so the question mark is unnecessary.
* `(/.+)` -> `(/.*)`: the plus indicates the following character needs to appear at least once. We want to match `...blogspot.com/` as well as `...blogspot.com/post...`

If you're going to use regex to match, I think you should write a test for each `getFeedFromURL` method because regex are notorious for bringing up edge cases.

::: mobile/android/base/java/org/mozilla/gecko/feeds/knownsites/KnownSiteBlogger.java:25
(Diff revision 2)
> +            return "https://" + matcher.group(2) + ".blogspot.com/feeds/posts/default";

nit: should we be re-using the search domain when we create these feed urls?

::: mobile/android/base/java/org/mozilla/gecko/feeds/knownsites/KnownSiteMedium.java:21
(Diff revision 2)
> +    public String getFeedFromURL(String url) {

I wonder if it'd be worth moving the repeated regex code to a helper function.

::: mobile/android/base/java/org/mozilla/gecko/feeds/knownsites/KnownSiteMedium.java:22
(Diff revision 2)
> +        Pattern pattern = Pattern.compile("https?://medium.com/([^/]+)(/.+)?");

`https?://medium.com/([^/]+)(/.*)?`

I changed:
* `(/.+)` => `(/.*)`
Attachment #8723311 - Flags: review?(michael.l.comella)
(In reply to Sebastian Kaspari (:sebastian) from comment #57)
> Thanks for the review so far, mcomella! Very good points. I'll go and
> address them in follow-up patches (I'll push them to this bug later too; and
> I'll only land this as one package) because navigating back and forth in
> this patch history makes my head hurt. :)

If it's easier, don't be afraid to land standalone, r+'d parts of the code – I find managing such a big changeset stack messy and just not seeing the tentative changes makes it simpler for me. Be warned that rb might remove your changesets (and thus the comments) from this bug if you push again though.
https://reviewboard.mozilla.org/r/36463/#review35653

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/CheckAction.java:31
(Diff revision 2)
> +public class CheckAction {

nit: -> `CheckForUpdateAction`
https://reviewboard.mozilla.org/r/36465/#review35657

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/SubscribeAction.java:24
(Diff revision 2)
> +    public static final String EXTRA_GUID = "guid";

nit: -> `EXTRA_BOOKMARK_GUID`? It seems we only check this against the bookmarks database.
Comment on attachment 8723312 [details]
MozReview Request: Bug 1241810 - Add EnrollAction for finding bookmarks of known sites to subscribe to. r=mcomella

https://reviewboard.mozilla.org/r/36469/#review35649

r+ assuming the `getBookmarksForPartialUrl` argument is figured out.

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1635
(Diff revision 2)
> +                new String[]{Bookmarks.GUID, Bookmarks._ID, Bookmarks.URL},

nit: `new String[] { stuff, stuff, stuff }`

I don't know why IJ likes removing the spaces from these expressions. (It'd probably save us time if we used a shared style sheet w/ IJ)

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/EnrollAction.java:24
(Diff revision 2)
> +public class EnrollAction {

nit: `EnrollAllBookmarksAction`?

I think `SubscribeAllBookmarksAction` is more clear though.

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/EnrollAction.java:25
(Diff revision 2)
> +    private static final String LOGTAG = "FeedEnrollAction";

nit: "Gecko" + blah blah?

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/EnrollAction.java:49
(Diff revision 2)
> +        Cursor cursor = db.getBookmarksForPartialUrl(context.getContentResolver(), "://" + knownSite.getSearchDomain() + "/");

I'm unclear how the String additions help here.

Looking at blogspot, it becomes:

`://.blogspot.com/`, which is put into the query with wildcards on either side. I'm surprised this pulls anything from the DB. Can you explain what these are doing (and perhaps add a comment)?

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/EnrollAction.java:62
(Diff revision 2)
> +                Log.d(LOGTAG, " (" + guid + ") " + url);

too personal
Attachment #8723312 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8723313 [details]
MozReview Request: Bug 1241810 - Add WithdrawAction for removing subscriptions of bookmarks that do not exist anymore. r=mcomella

https://reviewboard.mozilla.org/r/36471/#review35665

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/WithdrawAction.java:19
(Diff revision 2)
> + *

Empty class comment?

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/WithdrawAction.java:21
(Diff revision 2)
> +public class WithdrawAction {

nit: -> `UnsubscribeAllBookmarkOrphansAction` ?

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/WithdrawAction.java:47
(Diff revision 2)
> +        Log.d(LOGTAG, "Unsubscribing from: (" + subscription.getBookmarkGUID() + ") " + subscription.getFeedUrl());

too personal
Attachment #8723313 - Flags: review?(michael.l.comella) → review+
Attachment #8723314 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8723314 [details]
MozReview Request: Bug 1241810 - Add SetupAction for setting up various alarms to start FeedService. r=mcomella

https://reviewboard.mozilla.org/r/36473/#review35667

r+ once we figure out the wake lock issue.

::: mobile/android/base/AndroidManifest.xml.in:367
(Diff revision 2)
> +        <receiver android:name="org.mozilla.gecko.feeds.FeedAlarmReceiver" />

exported=false

::: mobile/android/base/java/org/mozilla/gecko/feeds/FeedAlarmReceiver.java:17
(Diff revision 2)
> +public class FeedAlarmReceiver extends WakefulBroadcastReceiver {

Why do we need a wake-lock for this? All my research seems to indicate they're really dangerous (e.g. https://plus.google.com/+ColtMcAnlis/posts/AXoD7daHVg6) and I can't find any articles on when you'd actually want to use them over just waiting for the cpu to wake up again.

Is it to ensure our alarms are set so we get those at the appropriate time (rather than waiting for the device to wake next to set the alarms and then waiting again for the alarms to go off)?

If so, add a comment.

::: mobile/android/base/java/org/mozilla/gecko/feeds/FeedAlarmReceiver.java:18
(Diff revision 2)
> +    private static final String LOGTAG = "FeedCheckAction";

nit: Gecko

::: mobile/android/base/java/org/mozilla/gecko/feeds/FeedService.java:65
(Diff revision 2)
>              return;

You need `completeWakefulIntent` here and above.

I'd just wrap the whole thing in `try`/`finally`. Maybe you'd want to make a subclass like, `WakefulIntentService` to do this, or to have this class:

onHandleIntent...
  try {
    onHandleIntentHelper(...)
  } finally {
    // release wakelock

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/SetupAction.java:21
(Diff revision 2)
> +public class SetupAction {

nit: -> `SetupAlarmAction`

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/SetupAction.java:50
(Diff revision 2)
> +    private void scheduleAlarms(AlarmManager alarmManager) {

These are declared in this order for a reason, I take it? Add a comment.

fwiw, I'm not sure they're guaranteed to run in this order (but I guess that's okay).

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/SetupAction.java:53
(Diff revision 2)
> +                SystemClock.elapsedRealtime() + AlarmManager.INTERVAL_FIFTEEN_MINUTES,

How did you come up with the different intervals to start the broadcasts? Are they staggered so they don't overlap? If so, why is that necessary? Also, add a comment.

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/SetupAction.java:74
(Diff revision 2)
> +    private PendingIntent withdrawPendingIntent() {

nit: `getWithdrawPendingIntent`. Same for the ones below this.
Comment on attachment 8726181 [details]
MozReview Request: Bug 1241810 - Add additional test case for Blogger. r=mcomella

https://reviewboard.mozilla.org/r/36999/#review35683
Attachment #8726181 - Flags: review?(michael.l.comella) → review+
(Assignee)

Comment 66

2 years ago
Okay, I'm going to rebase the patch series now and start to address your review comments.

(In reply to Michael Comella (:mcomella) from comment #59)
> If it's easier, don't be afraid to land standalone, r+'d parts of the code –
> I find managing such a big changeset stack messy and just not seeing the
> tentative changes makes it simpler for me. Be warned that rb might remove
> your changesets (and thus the comments) from this bug if you push again
> though.

Yeah, I might actually do that. Getting some of the patches out reduces the overwhelming wall of patches here.
(Assignee)

Comment 67

2 years ago
https://reviewboard.mozilla.org/r/32633/#review35457

As long as we can grab all the data we are interested in we will accept the (possibly invalid) feed. With the unit tests I'm confident that this is good enough for our limited set of supported sites. I agree that it's a problem if the feed is seriously broken (<item>..<item>..</item>..</iterm>).

The essay is interesting. That's actually how I started and then I dumbed it down to just have one method for handling start/end/.. nodes. I'll file a follow-up to look into refactoring the parse. With the unit tests in place this should be quite nice.

> Any reason not to make this (and all of the other helper methods) `static`?

No strong reason. While writing this I changed the implementation multiple times, from static to keeping the state as members to this final version with the short-lived ParserState object. Non-static classes/methods are usually nicer to test because you can mock/stub parts of it as needed. But this isn't needed here.

> I see in the `XmlPullParser` docs that you can set the parser to run in validation mode – is there a reason we didn't run in that? If so, I'd add a comment.

No reason. I'll check the docs.

> This will cause problems if item or entry tags can appear inside one another. While this might be against the feed specification, we could still receive an invalid feed – you should probably ensure `state.currentItem == null` before assigning (and it also has the benefit of explicitly stating assumptions in the code).

I'll move this to the parser follow-up bug.

> Ignoring specific tags as a blacklist is concerning – do we know for sure that there are no other feed elements that contain `<title>`? What if a new revision of RSS comes out that contains a new element that contains a title?
> 
> I think this would be safer if we used a whitelist by checking what the most recent parent tag is – see my overview comment about an alternative approach.
> 
> Similarly for the other `handle*StartTag` methods.

I absolutely agree. I found these issues when writing the unit tests and I haven't took the time to write a generic solution.

> I see a few `TAG_*`, PUBDATE, UPDATED, and DATE, are competing to assign to the same field – should we prefer one over the other so it's more consistent (e.g. a feed reorders an element)?

A given feed should only have one of them. Depending on the standard they are following. If one feed would use multiple for whatever reason then I wouldn't know which one to prefer and they /should/ all have the same date.

> The other two versions of this method don't throw `RuntimeException` – I assume this was not intentional?

Oops, yeah, I used this for testing.

> Might be good to add some invalid feeds in here too, e.g. empty feeds, feeds with `<item>` inside `<item>` tags, invalid xml (e.g. non-closed tags, broken tags), etc.

That's a good idea. I will add some!
Comment on attachment 8723315 [details]
MozReview Request: Bug 1241810 - Run setup on app start and when the device boots. r=mcomella

https://reviewboard.mozilla.org/r/36475/#review36475

A general note (before I forget): should this be behind build flags?

::: mobile/android/base/AndroidManifest.xml.in:369
(Diff revision 2)
>              android:name="org.mozilla.gecko.feeds.FeedService">
>          </service>
>  
>          <receiver android:name="org.mozilla.gecko.feeds.FeedAlarmReceiver" />
>  
> +        <receiver android:name="org.mozilla.gecko.BootReceiver">

Does this need to be `exported=true`?

::: mobile/android/base/FennecManifest_permissions.xml.in:28
(Diff revision 2)
>      <uses-permission android:name="android.permission.CHANGE_WIFI_STATE"/>
>      <uses-permission android:name="android.permission.ACCESS_WIFI_STATE"/>
>      <uses-permission android:name="android.permission.ACCESS_FINE_LOCATION"/>
>      <uses-permission android:name="android.permission.ACCESS_NETWORK_STATE"/>
>      <uses-permission android:name="android.permission.INTERNET"/>
> +    <uses-permission android:name="android.permission.RECEIVE_BOOT_COMPLETED"/>

Should we put this behind a flag?

::: mobile/android/base/java/org/mozilla/gecko/BootReceiver.java:15
(Diff revision 2)
> +import android.content.Intent;
> +
> +import org.mozilla.gecko.feeds.FeedService;
> +
> +/**
> + *

javadoc?
Attachment #8723315 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8723316 [details]
MozReview Request: Bug 1241810 - Do not do anything if not connected to a network or network is metered. r=mcomella

https://reviewboard.mozilla.org/r/36477/#review36477
Attachment #8723316 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8726176 [details]
MozReview Request: Bug 1241810 - FeedSubscription: Treat ETag and "Last modified" header as optional. r=mcomella

https://reviewboard.mozilla.org/r/36765/#review36479
Attachment #8726176 - Flags: review?(michael.l.comella) → review+
Attachment #8726177 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8726177 [details]
MozReview Request: Bug 1241810 - Bug 1238087 - Update style of content notification. r=mcomella

https://reviewboard.mozilla.org/r/36767/#review36481

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:3781
(Diff revision 1)
> +            JSONObject object = new JSONObject();
> +            object.put("urls", array);
> +
> +            GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Tabs:OpenMultiple", object.toString()));
> +        } catch (JSONException e) {
> +            Log.e(LOGTAG, "Unable to create JSON for opening multiple URLs", e);

Logging the exception could log the json object which could leak urls! Probably shouldn't do that.

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/CheckAction.java:74
(Diff revision 1)
> +
> +        notify(updatedFeeds);
> -            }
> +    }
> +
> +    private void notify(List<Feed> updatedFeeds) {
> +        final int feeds = updatedFeeds.size();

nit: feeds -> feedCount

::: mobile/android/base/locales/en-US/android_strings.dtd:278
(Diff revision 1)
>       has been queued but we are missing the system permission to show an overlay. -->
>  <!ENTITY tab_queue_notification_settings "To \&quot;Open multiple links\&quot;, please enable the \'Draw over other apps\' permission for &brandShortName;">
>  
> +<!ENTITY content_notification_summary "&brandShortName;">
> +<!ENTITY content_notification_title_plural "&formatD; websites updated">
> +<!ENTITY content_notification_action_settings "Notifications Setting">

I'd say, "Notification Settings", but perhaps these are not the final strings?

::: mobile/android/base/locales/en-US/android_strings.dtd:279
(Diff revision 1)
>  <!ENTITY tab_queue_notification_settings "To \&quot;Open multiple links\&quot;, please enable the \'Draw over other apps\' permission for &brandShortName;">
>  
> +<!ENTITY content_notification_summary "&brandShortName;">
> +<!ENTITY content_notification_title_plural "&formatD; websites updated">
> +<!ENTITY content_notification_action_settings "Notifications Setting">
> +<!ENTITY content_notification_updated_on "Updated on &formatS;">

l10n note for what `&formatS` is (I'm assuming this is awaiting UX approval?).

::: mobile/android/base/locales/en-US/android_strings.dtd:281
(Diff revision 1)
> +<!ENTITY content_notification_summary "&brandShortName;">
> +<!ENTITY content_notification_title_plural "&formatD; websites updated">
> +<!ENTITY content_notification_action_settings "Notifications Setting">
> +<!ENTITY content_notification_updated_on "Updated on &formatS;">
> +
> +

nit: ws
fyi, goal for tomorrow is to finish this up, Sebastian.
(Assignee)

Updated

2 years ago
Depends on: 1256640
(Assignee)

Updated

2 years ago
Depends on: 1256641
(Assignee)

Updated

2 years ago
Keywords: leave-open
(Assignee)

Comment 73

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/19f1c6b2aa4ecc786ab426f27a395d9056f4bfd1
Bug 1241810 - Introduce SimpleFeedParser for implementing "content notifications". r=mcomella
(Assignee)

Comment 74

2 years ago
https://reviewboard.mozilla.org/r/36459/#review35481

> Why do we remove the weak indicator? I'd assume the server would want the identical etag back.

In my tests the server always returned the full content (HTTP 200) if I send the ETag including weak indicator. Only after stripping the weak indicator and just sending the ETag I got 304 Not Modified.

Compare:
http -p hH https://googleblog.blogspot.de/atom.xml
http -p hH https://googleblog.blogspot.de/atom.xml If-None-Match:W/"b74c6093-3602-4de1-b935-9bd274b56db4"   (Replace with the current Etag)
http -p hH https://googleblog.blogspot.de/atom.xml If-None-Match:"b74c6093-3602-4de1-b935-9bd274b56db4"

RFC 7232 ETag
https://tools.ietf.org/html/rfc7232#section-2.3

RFC 7232 If-None-Match
https://tools.ietf.org/html/rfc7232#section-3.2

From the RFCs I also assume that we should send the weak indicator - but this is not what blogspot (The only whitelisted service that sends ETags) understands. Looking at the RFC it looks like it would be safe to just send both "If-None-Match: weak, without" but it looks like blogspot doesn't understand this either.

Also see RFC 2616:
https://www.w3.org/Protocols/rfc2616/rfc2616-sec13.html#sec13.3.3

My understanding is that you are right: We should not strip the indicator but it seems like blogspot is behaving differently (or am I doing something wrong here?). It's worth exploring this is a follow-up bug further. Especially if we add more services that may have conflicting ETag validators.
(Assignee)

Updated

2 years ago
Depends on: 1256656
(Assignee)

Comment 75

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/f00e7b9da3a88df9356a69f0f44f37783e64c184
Bug 1241810 - FeedFetcher: Simple helper class for fetching feeds using ETag and If-Modified-Since headers. r=mcomella
(Assignee)

Comment 76

2 years ago
https://reviewboard.mozilla.org/r/36461/#review35485

> This method is duplicated from `Feed` – is there any place we can put this code so we don't have to repeat it?

This is definitely an error. There should only be one. I'll clean this up in a follow-up patch if this is still in the final version.
(Assignee)

Comment 78

2 years ago
https://reviewboard.mozilla.org/r/36463/#review35491

> nit: system convention for actions is kind of, `<package>.intent.action.ACTION_NAME`: https://developer.android.com/reference/android/content/Intent.html#ACTION_SEND
> 
> But I don't think it matters that much.

I modelled this after DownloadContentService which is inspired by UpdateServiceHelper I think. It looks like we do not have a consistent convetion:
https://dxr.mozilla.org/mozilla-central/search?q=%27AppConstants.ANDROID_PACKAGE_NAME+%2B+%22.%27&redirect=true&case=false

> This may be considered too personal to log.

Those logs have been verfy helpful for developing and debugging this. I understand that we do not want to log those but I would like to keep the logs behind a flag. I'll move them behind a flag in a follow-up patch - and before anything gets enabled. Changing all those lines in between patches is going to create a bunch of conflicts I guess.

> Perhaps for a follow-up: do we want to display a separate notification for each item of content that's been updated?

See bug 1238087 (and screenshots there): We squash all updates into a single notification - But this is implemented in one of the later patches.

> nit: -> `notifyUser` or `addNotification` or similar
> 
> I associated `notify` with dispatching an Event and was confused by the title.

I'll move this to a follow-up patch too because other patches modify this code here. :)

> The ID is constant so I believe that means we can only show one notification at a time – is that desireable?

Yes, but a later patch will merge multiple content updates into a single notification.
(Assignee)

Comment 79

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/3412bad24f8b69cf36acc3c77c50a7462c3f5ffd
Bug 1241810 - Add background service and action to check feeds for updates. r=mcomella
Attachment #8726178 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8726178 [details]
MozReview Request: Bug 1241810 - Restructure FeedService to always complete wakeful intent. r=mcomella

https://reviewboard.mozilla.org/r/36769/#review36697

r+ but don't forget to fix the potential NPE.

::: mobile/android/base/java/org/mozilla/gecko/feeds/FeedService.java:77
(Diff revision 1)
> -            return;
> +                return;
> -        }
> +            }
>  
> -        if (!isConnectedToNetwork() || isActiveNetworkMetered()) {
> +            BaseAction action = createActionForIntent(intent);
> +
> +            if (action.requiresNetwork() && !isConnectedToUnmeteredNetwork()) {

`action` can be null

::: mobile/android/base/java/org/mozilla/gecko/feeds/FeedService.java:92
(Diff revision 1)
> +        } finally {
> +            FeedAlarmReceiver.completeWakefulIntent(intent);
> +        }
> +    }
> +
> +    private BaseAction createActionForIntent(Intent intent) {

`@Nullable` on the return value

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/BaseAction.java:10
(Diff revision 1)
> +
> +package org.mozilla.gecko.feeds.action;
> +
> +import android.content.Intent;
> +
> +public interface BaseAction {

nit: `Base*` implies base, or super, class to me. I think this could be better named (I might even name it `Action`).

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/CheckAction.java:46
(Diff revision 1)
>      public CheckAction(Context context, SubscriptionStorage storage) {
>          this.context = context;
>          this.storage = storage;
>      }
>  
> -    public void perform() {
> +    public void perform(Intent intent) {

It's unfortunate this parameter is unused... (I'd be surprised if there's no lint warning for that! :P)

Maybe add a comment to indicate it's intentionally unused? I think the IDE might print out warnings so `@SupressWarnings("unused")` (or whatever it is) might serve the same function.

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/EnrollAction.java:51
(Diff revision 1)
>          }
>      }
>  
> +    @Override
> +    public boolean requiresNetwork() {
> +        return false;

This service starts the subscribe service, which requires network. If the network is off, subscribe will fail. Since we don't try to resend these subscribe requests, we have to wait until enroll is called again. Thus, enroll is a no-op if the network is off so we might as well say it requires network.
I just noticed the switchboard branch again – ignore my comments about Nightly flags. :)
Comment on attachment 8726179 [details]
MozReview Request: Bug 1241810 - Bug 1247788 - Add "Notifications" in Settings. r=mcomella

https://reviewboard.mozilla.org/r/36771/#review36763

Another thought – there might be less namespace pollution if these are `*FeedAction` (e.g. if we add other `Action`s later, but you can understandably argue that we can cross that bridge when we come to it.

::: mobile/android/base/java/org/mozilla/gecko/feeds/FeedService.java:79
(Diff revision 1)
> +            if (action.requiresPreferenceEnabled() && !isPreferenceEnabled()) {
> +                Log.d(LOGTAG, "Preference is disabled. Skipping.");
> +                return;
> +            }

This feature is entirely based around this preference being enabled (e.g. only `SetupAction` runs without the pref enabled and I'd argue that it should only run with the pref enabled, provided we add some more logic) – why don't we do this check all the time?

::: mobile/android/base/resources/xml-v11/preferences.xml:50
(Diff revision 1)
>                        android:fragment="org.mozilla.gecko.preferences.GeckoPreferenceFragment" >
>          <extra android:name="resource"
>                 android:value="preferences_accessibility" />
>      </PreferenceScreen>
>  
> +    <PreferenceScreen android:title="@string/pref_category_notifications"

The other `PreferenceScreen`s here also have a summary – do we need one?
Attachment #8726179 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8726180 [details]
MozReview Request: Bug 1241810 - Add support for wordpress.com. r=mcomella

https://reviewboard.mozilla.org/r/36997/#review36777

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/EnrollAction.java:64
(Diff revision 1)
>      }
>  
>      private void searchFor(BrowserDB db, KnownSite knownSite) {
> -        Cursor cursor = db.getBookmarksForPartialUrl(context.getContentResolver(), "://" + knownSite.getSearchDomain() + "/");
> +        Cursor cursor = db.getBookmarksForPartialUrl(context.getContentResolver(), knownSite.getSearchDomain());
>          if (cursor == null) {
> -            Log.d(LOGTAG, "Nothing found");
> +            Log.d(LOGTAG, "Nothing found (" + knownSite.getClass().getSimpleName() + ")");

arguably personal info (e.g. subscribed to wordpress but not medium).

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/EnrollAction.java:81
(Diff revision 1)
>  
>                  String feedUrl = knownSite.getFeedFromURL(url);
>                  if (!TextUtils.isEmpty(feedUrl)) {
>                      FeedService.subscribe(context, guid, feedUrl);
> +                } else {
> +                    Log.w(LOGTAG, "Could not determine feed for URL: " + url);

personal info

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/SubscribeAction.java:46
(Diff revision 1)
>          if (storage.hasSubscriptionForBookmark(guid)) {
>              Log.d(LOGTAG, "Already subscribed to " + feedUrl + ". Skipping.");
>              return;
>          }
>  
> +        Log.d(LOGTAG, "Subscribing to feed: " + feedUrl);

personal info

::: mobile/android/base/java/org/mozilla/gecko/feeds/knownsites/KnownSiteWordpress.java:17
(Diff revision 1)
> +        return ".wordpress.com";
> +    }
> +
> +    @Override
> +    public String getFeedFromURL(String url) {
> +        //

nit: empty comment

::: mobile/android/base/java/org/mozilla/gecko/feeds/knownsites/KnownSiteWordpress.java:18
(Diff revision 1)
> +    }
> +
> +    @Override
> +    public String getFeedFromURL(String url) {
> +        //
> +        Pattern pattern = Pattern.compile("https?://(.*?).wordpress.com(/.*)?");

-> `https?://(.*).wordpress.com(/.*)?`

* `(.*?)` -> `(.*)`

I think.

::: mobile/android/base/java/org/mozilla/gecko/feeds/parser/SimpleFeedParser.java:163
(Diff revision 1)
> +            case TAG_CONTENT:
> +                state.inContent = true;
> +                break;

Similarly, I'd structure this as `processContentTag` (or similar) and skip to the end tag.
Attachment #8726180 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8726183 [details]
MozReview Request: Bug 1241810 - Bug 1238087 - Add "notifications setting" action to content notification. r=mcomella

https://reviewboard.mozilla.org/r/37841/#review36797

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/CheckAction.java:181
(Diff revision 1)
>          NotificationManagerCompat.from(context).notify(R.id.websiteContentNotification, notification);
>      }
>  
> +    private NotificationCompat.Action createNotificationSettingsAction() {
> +        final Intent intent = new Intent(GeckoApp.ACTION_LAUNCH_SETTINGS);
> +        intent.setClassName(AppConstants.ANDROID_PACKAGE_NAME, AppConstants.MOZ_ANDROID_BROWSER_INTENT_CLASS);

I dislike that the constant `MOZ_ANDROID_BROWSER_INTENT_CLASS` is a String (when it could be a `Class` instance), but I suppose that's not your doing. :P
Attachment #8726183 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8726182 [details]
MozReview Request: Bug 1241810 - Use UrlAnnotations table as storage. r=mcomella

https://reviewboard.mozilla.org/r/37001/#review36787

I'm not digging that we have a separate db entry for website -> feed urls, as opposed to website -> subscription. Besides taking up extra disk space, it adds a lot of complexity (e.g. I've struggling to figure out for 20 minutes whether or not it's actually necessary).

Is it necessary? Can we remove it (e.g. move it to the JSONObject)? I think the complexity could be reduced if we modified the schema to include this value (/me starts wondering if this key-value store in sql was a good idea or not...), but then again a `key value value2` schema isn't great either.

r+ because it appears to work but I'm getting red flags from the complexity.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:465
(Diff revision 1)
>                  break;
>              }
>  
> +            case URL_ANNOTATIONS:
> +                trace("Delete on URL_ANNOTATIONS: " + uri);
> +                deleteUrlAnnotation(uri, selection, selectionArgs);

;D

::: mobile/android/base/java/org/mozilla/gecko/db/LocalUrlAnnotations.java:22
(Diff revision 1)
> +
> +import java.util.ArrayList;
> +import java.util.List;
>  
>  public class LocalUrlAnnotations implements UrlAnnotations {
> +    private static final String LOGTAG = "LocalUrlAnnotations";

nit: "Gecko" +

::: mobile/android/base/java/org/mozilla/gecko/db/LocalUrlAnnotations.java:60
(Diff revision 1)
>      public LocalUrlAnnotations(final String profile) {
>          urlAnnotationsTableWithProfile = DBUtils.appendProfile(profile, BrowserContract.UrlAnnotations.CONTENT_URI);
>      }
>  
> +    @Override
> +    public Cursor getFeedSubscriptions(ContentResolver cr) {

fwiw, this can use my `queryByKey` method.

::: mobile/android/base/java/org/mozilla/gecko/db/LocalUrlAnnotations.java:91
(Diff revision 1)
> +        if (cursor == null) {
> +            return false;
> +        }
> +
> +        try {
> +            return cursor.moveToNext();

I think `getCount` is clearer but if you prefer otherwise, `moveToFirst` is safer than `...Next`.

::: mobile/android/base/java/org/mozilla/gecko/db/LocalUrlAnnotations.java:101
(Diff revision 1)
> +
> +    /**
> +     * Returns true if there's a website URL with this feed URL. False otherwise.
> +     */
> +    @Override
> +    public boolean hasWebsiteForFeedUrl(ContentResolver cr, String feedUrl) {

This code is really similar to `hasFeedUrlForWebsite` – I'd make a helper function.

::: mobile/android/base/java/org/mozilla/gecko/db/LocalUrlAnnotations.java:112
(Diff revision 1)
> +        if (cursor == null) {
> +            return false;
> +        }
> +
> +        try {
> +            return cursor.moveToNext();

`getCount`

::: mobile/android/base/java/org/mozilla/gecko/db/LocalUrlAnnotations.java:153
(Diff revision 1)
> +        if (cursor == null) {
> +            return false;
> +        }
> +
> +        try {
> +            return cursor.moveToNext();

`getCount`

::: mobile/android/base/java/org/mozilla/gecko/db/LocalUrlAnnotations.java:167
(Diff revision 1)
> +    @Override
> +    public void insertFeedSubscription(ContentResolver cr, FeedSubscription subscription) {
> +        try {
> +            insertAnnotation(cr, subscription.getFeedUrl(), Key.FEED_SUBSCRIPTION, subscription.toJSON().toString());
> +        } catch (JSONException e) {
> +            Log.w(LOGTAG, "Could not serialize subscription", e);

`e` could be personal info

::: mobile/android/base/java/org/mozilla/gecko/db/LocalUrlAnnotations.java:179
(Diff revision 1)
> +    @Override
> +    public void updateFeedSubscription(ContentResolver cr, FeedSubscription subscription) {
> +        try {
> +            updateAnnotation(cr, subscription.getFeedUrl(), Key.FEED_SUBSCRIPTION, subscription.toJSON().toString());
> +        } catch (JSONException e) {
> +            Log.w(LOGTAG, "Could not serialize subscription", e);

personal

::: mobile/android/base/java/org/mozilla/gecko/feeds/FeedService.java:66
(Diff revision 1)
> +
>      @Override
>      public void onCreate() {
>          super.onCreate();
>  
> -        storage = new SubscriptionStorage(getApplicationContext());
> +        browserDB = GeckoProfile.get(this).getDB();

In we want to be profile-independent (but did we kill multiple profiles?), we should get the profile as an argument to the Intent. See: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryUploadService.java#169

::: mobile/android/base/java/org/mozilla/gecko/feeds/FeedService.java:106
(Diff revision 1)
>                  return new SetupAction(this);
>  
>              case ACTION_SUBSCRIBE:
> -                return new SubscribeAction(storage);
> +                return new SubscribeAction(this);
>  
>              case ACTION_CHECK:
> -                return new CheckAction(this, storage);
> +                return new CheckAction(this);
>  
>              case ACTION_ENROLL:
>                  return new EnrollAction(this);
>  
>              case ACTION_WITHDRAW:
> -                return new WithdrawAction(this, storage);
> +                return new WithdrawAction(this);

While it's not as necessary for `Service`s, can we pass in the Application Context here to avoid memory leaks?

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/CheckAction.java:62
(Diff revision 1)
> +        if (cursor == null) {
> +            return;
> +        }
>  
> -        Log.d(LOGTAG, "Checking feeds for updates (" + subscriptions.size() + " feeds) ..");
> +        try {
> +            while (cursor.moveToNext()) {

Start with `moveToFirst`

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/CheckAction.java:71
(Diff revision 1)
> +                if (response != null) {
> +                    updatedFeeds.add(response.feed);
> +                }
> +            }
> +        } catch (JSONException e) {
> +            Log.w(LOGTAG, "Could not deserialize subscription", e);

personal

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/CheckAction.java:79
(Diff revision 1)
> +        }
>  
> -        List<Feed> updatedFeeds = new ArrayList<>();
> +        notify(updatedFeeds);
> +    }
>  
> -        for (FeedSubscription subscription : subscriptions) {
> +    private FeedFetcher.FeedResponse checkFeedForUpdates(UrlAnnotations urlAnnotations, FeedSubscription subscription) {

nit: -> `checkAndMaybeStoreFeedForUpdates` or similar.

Or move the store outside this function (which I think is clearer).

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/EnrollAction.java:63
(Diff revision 1)
>      @Override
>      public boolean requiresPreferenceEnabled() {
>          return true;
>      }
>  
> -    private void searchFor(BrowserDB db, KnownSite knownSite) {
> +    private void searchFor(BrowserDB db, UrlAnnotations urlAnnotations, ContentResolver contentResolver, KnownSite knownSite) {

nit: If we're passing in `db`, we don't actually have to pass in `urlAnnotations`

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/EnrollAction.java:64
(Diff revision 1)
>      public boolean requiresPreferenceEnabled() {
>          return true;
>      }
>  
> -    private void searchFor(BrowserDB db, KnownSite knownSite) {
> -        Cursor cursor = db.getBookmarksForPartialUrl(context.getContentResolver(), knownSite.getSearchDomain());
> +    private void searchFor(BrowserDB db, UrlAnnotations urlAnnotations, ContentResolver contentResolver, KnownSite knownSite) {
> +        final Cursor cursor = db.getBookmarksForPartialUrl(contentResolver, knownSite.getSearchDomain());

It might be good to write some tests for queries like this.

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/EnrollAction.java:76
(Diff revision 1)
>  
>              while (cursor.moveToNext()) {
> -                final String guid = cursor.getString(cursor.getColumnIndex(BrowserContract.Bookmarks.GUID));
>                  final String url = cursor.getString(cursor.getColumnIndex(BrowserContract.Bookmarks.URL));
>  
> -                Log.d(LOGTAG, " (" + guid + ") " + url);
> +                Log.d(LOGTAG, " URL: " + url);

personal

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/EnrollAction.java:80
(Diff revision 1)
>  
>                  String feedUrl = knownSite.getFeedFromURL(url);
> -                if (!TextUtils.isEmpty(feedUrl)) {
> +                if (TextUtils.isEmpty(feedUrl)) {
> -                    FeedService.subscribe(context, guid, feedUrl);
> -                } else {
>                      Log.w(LOGTAG, "Could not determine feed for URL: " + url);

personal

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/EnrollAction.java:83
(Diff revision 1)
> +                if (!urlAnnotations.hasFeedUrlForWebsite(contentResolver, url)) {
> +                    urlAnnotations.insertFeedUrl(contentResolver, url, feedUrl);

I don't recall the details, but I'm pretty sure you can do this in one query (e.g. http://stackoverflow.com/a/21110662). Mentored follow-up?

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/WithdrawAction.java:45
(Diff revision 1)
> +        removeSubscriptionsOfRemovedFeeds(urlAnnotations, resolver);
> +    }
> +
> +    /**
> +     * Search for website URLs with a feed assigned. Remove entry if website URL is not known anymore:
> +     * For now this means the website is not bookmarked.

It'd be great if the logic of what quantifies a "known" website was written in the same place. Deletions are here and insertions are in another action.

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/WithdrawAction.java:54
(Diff revision 1)
> +        if (cursor == null) {
> +            return;
> +        }
>  
> -        List<FeedSubscription> subscriptions = storage.getSubscriptions();
> +        try {
> +            while (cursor.moveToNext()) {

`moveToFirst` first

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/WithdrawAction.java:78
(Diff revision 1)
> +        if (cursor == null) {
> +            return;
> +        }
> +
> +        try {
> +             while (cursor.moveToNext()) {

`moveToFirst` first

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/WithdrawAction.java:82
(Diff revision 1)
> +        try {
> +             while (cursor.moveToNext()) {
> +                 final FeedSubscription subscription = FeedSubscription.fromCursor(cursor);
> +
> +                 if (!urlAnnotations.hasWebsiteForFeedUrl(resolver, subscription.getFeedUrl())) {
> +                     Log.d(LOGTAG, "Removing subscription for feed: " + subscription.getFeedUrl());

personal

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/WithdrawAction.java:88
(Diff revision 1)
> +
> +                     urlAnnotations.deleteFeedSubscription(resolver, subscription);
> +                 }
> +             }
> +        } catch (JSONException e) {
> +            Log.w(LOGTAG, "Could not deserialize subscription", e);

personal

::: mobile/android/base/java/org/mozilla/gecko/feeds/subscriptions/FeedSubscription.java:57
(Diff revision 1)
>  
>          return subscription;
>      }
>  
> -    /* package-private */ void update(FeedFetcher.FeedResponse response) {
> -        final String feedUrl = response.feed.getFeedURL();
> +    private void fromJSON(JSONObject object) throws JSONException {
> +        feedTitle = object.getString(JSON_KEY_FEED_TITLE);

Why don't we get the feed url from the response anymore?
Attachment #8726182 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8726184 [details]
MozReview Request: Bug 1241810 - Bug 1251317 - Add telemetry for content notifications. r=mcomella,mfinkle

https://reviewboard.mozilla.org/r/37843/#review36861

We should definitely include a probe to indicate whether users have the notification turned on or off. Follow-up?

::: mobile/android/base/java/org/mozilla/gecko/feeds/action/CheckAction.java:121
(Diff revision 1)
>              notifySingle(updatedFeeds.get(0));
> -        } else if (feeds > 1) {
> +        } else {
>              notifyMultiple(updatedFeeds);
>          }
> +
> +        Telemetry.sendUIEvent(TelemetryContract.Event.SHOW, TelemetryContract.Method.NOTIFICATION, "content_update");

Are these numbers useful if the user doesn't influence whether or not the action occurs? Is this just to make sure it's working? Here and all around.
Attachment #8726184 - Flags: review?(michael.l.comella) → review+
(Assignee)

Comment 89

2 years ago
https://reviewboard.mozilla.org/r/36467/#review35645

> I think you'd want:
> 
> `https?://(www\\.)?(.*)\\.blogspot\\.com(/.*)?`
> 
> My two changes:
> * `(.*?)` -> `(.*)`: the `*` indicates the item should be repeated 0 or more times so the question mark is unnecessary.
> * `(/.+)` -> `(/.*)`: the plus indicates the following character needs to appear at least once. We want to match `...blogspot.com/` as well as `...blogspot.com/post...`
> 
> If you're going to use regex to match, I think you should write a test for each `getFeedFromURL` method because regex are notorious for bringing up edge cases.

I use .*? to make the .* less greedy:

If my input is: http://example.blogspot.com/2016/03/i-moved-to-example.blogspot.com


https?://(www\.)?(.*?)\.blogspot\.com(/.+)?
MATCH 1
2.	[7-14]	`example`
3.	[27-67]	`/2016/03/i-moved-to-example.blogspot.com`

https?://(www\.)?(.*)\.blogspot\.com(/.*)?
MATCH 1
2.	[7-54]	`example.blogspot.com/2016/03/i-moved-to-example`
(The .* is greedy and matches as many characters as possible)

You can test regex directly in AS and try different strings on them. Unfortunately it does not show the matched groups. I used https://regex101.com/ for this example.

Regarding the end (.+ vs .*): You are absolutely right. I'll fix that. And I absolutely agree about the tests. I remember taking a note to add tests for that but it seems like I never did. I'll add some tests to this commit and push it for review again.

> nit: should we be re-using the search domain when we create these feed urls?

You mean as a constant that I insert in this string? I'm not sure if the search string will always be a part of the feed URL - but it's here. Isn't it more readable as just a plain string? In fact I guess I'll turn this into a String.format() call. Let me know in the re-review what you think about that. :)

> I wonder if it'd be worth moving the repeated regex code to a helper function.

Right now I can't see how I could do this nicely: The pattern is different and building the string from a match is different.

> `https?://medium.com/([^/]+)(/.*)?`
> 
> I changed:
> * `(/.+)` => `(/.*)`

Aye, again this mistake. Thank you!
(Assignee)

Comment 90

2 years ago
https://reviewboard.mozilla.org/r/36469/#review35649

> nit: "Gecko" + blah blah?

I'm always afraid to get over the tag size limit (23?). Is there a strong reason why we want the prefix?

> I'm unclear how the String additions help here.
> 
> Looking at blogspot, it becomes:
> 
> `://.blogspot.com/`, which is put into the query with wildcards on either side. I'm surprised this pulls anything from the DB. Can you explain what these are doing (and perhaps add a comment)?

Yeah, the blogspot is broken here (I tested with medium only at that time). But this should be fixed in a later patch.
(Assignee)

Comment 91

2 years ago
Comment on attachment 8723311 [details]
MozReview Request: Bug 1241810 - Add configuration/whitelist for known sites we want to show notifications for. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36467/diff/2-3/
Attachment #8723311 - Attachment description: MozReview Request: Bug 1241810 - Add configuration/whitelist for known sites we want to show notifications for. r?mcomella → MozReview Request: Bug 1241810 - Add configuration/whitelist for known sites we want to show notifications for. r=mcomella
Attachment #8723311 - Flags: review?(michael.l.comella)
(Assignee)

Comment 92

2 years ago
Comment on attachment 8723312 [details]
MozReview Request: Bug 1241810 - Add EnrollAction for finding bookmarks of known sites to subscribe to. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36469/diff/2-3/
Attachment #8723312 - Attachment description: MozReview Request: Bug 1241810 - Add EnrollAction for finding bookmarks of known sites to subscribe to. r?mcomella → MozReview Request: Bug 1241810 - Add EnrollAction for finding bookmarks of known sites to subscribe to. r=mcomella
(Assignee)

Comment 93

2 years ago
Comment on attachment 8723313 [details]
MozReview Request: Bug 1241810 - Add WithdrawAction for removing subscriptions of bookmarks that do not exist anymore. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36471/diff/2-3/
Attachment #8723313 - Attachment description: MozReview Request: Bug 1241810 - Add WithdrawAction for removing subscriptions of bookmarks that do not exist anymore. r?mcomella → MozReview Request: Bug 1241810 - Add WithdrawAction for removing subscriptions of bookmarks that do not exist anymore. r=mcomella
(Assignee)

Comment 94

2 years ago
Comment on attachment 8723314 [details]
MozReview Request: Bug 1241810 - Add SetupAction for setting up various alarms to start FeedService. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36473/diff/2-3/
(Assignee)

Comment 95

2 years ago
Comment on attachment 8723315 [details]
MozReview Request: Bug 1241810 - Run setup on app start and when the device boots. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36475/diff/2-3/
(Assignee)

Comment 96

2 years ago
Comment on attachment 8723316 [details]
MozReview Request: Bug 1241810 - Do not do anything if not connected to a network or network is metered. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36477/diff/2-3/
(Assignee)

Comment 97

2 years ago
Comment on attachment 8726176 [details]
MozReview Request: Bug 1241810 - FeedSubscription: Treat ETag and "Last modified" header as optional. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36765/diff/1-2/
(Assignee)

Comment 98

2 years ago
Comment on attachment 8726177 [details]
MozReview Request: Bug 1241810 - Bug 1238087 - Update style of content notification. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36767/diff/1-2/
(Assignee)

Comment 99

2 years ago
Comment on attachment 8726178 [details]
MozReview Request: Bug 1241810 - Restructure FeedService to always complete wakeful intent. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36769/diff/1-2/
(Assignee)

Comment 100

2 years ago
Comment on attachment 8726179 [details]
MozReview Request: Bug 1241810 - Bug 1247788 - Add "Notifications" in Settings. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36771/diff/1-2/
(Assignee)

Comment 101

2 years ago
Comment on attachment 8726180 [details]
MozReview Request: Bug 1241810 - Add support for wordpress.com. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36997/diff/1-2/
(Assignee)

Comment 102

2 years ago
Comment on attachment 8726181 [details]
MozReview Request: Bug 1241810 - Add additional test case for Blogger. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36999/diff/1-2/
(Assignee)

Comment 103

2 years ago
Comment on attachment 8726182 [details]
MozReview Request: Bug 1241810 - Use UrlAnnotations table as storage. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37001/diff/1-2/
(Assignee)

Comment 104

2 years ago
Comment on attachment 8726183 [details]
MozReview Request: Bug 1241810 - Bug 1238087 - Add "notifications setting" action to content notification. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37841/diff/1-2/
(Assignee)

Comment 105

2 years ago
Comment on attachment 8726184 [details]
MozReview Request: Bug 1241810 - Bug 1251317 - Add telemetry for content notifications. r=mcomella,mfinkle

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37843/diff/1-2/
(Assignee)

Updated

2 years ago
Attachment #8723306 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8723307 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8723308 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8723309 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8723310 - Attachment is obsolete: true
(Assignee)

Comment 106

2 years ago
Aye, so many comments when pushing a new version for review.

@mcomella: I landed some first pieces after addressing your review comments (Some of them I'll address in follow-up patches though) and re-flagged you for the last outstanding and now updated patch. See also comment 89 for that.n I'll continue with the other patches and continue to land pieces as soon as the outstanding patch is r+. :)
(Assignee)

Comment 107

2 years ago
https://reviewboard.mozilla.org/r/36473/#review35667

> exported=false

I had the impression that this needs to be exported because the broadcast is send by the alarmmanager and therefore not really send by our application. But it looks like AlarmManager can launch send the broadcast even if the receiver is not exported. I'll test that and update this.

> Why do we need a wake-lock for this? All my research seems to indicate they're really dangerous (e.g. https://plus.google.com/+ColtMcAnlis/posts/AXoD7daHVg6) and I can't find any articles on when you'd actually want to use them over just waiting for the cpu to wake up again.
> 
> Is it to ensure our alarms are set so we get those at the appropriate time (rather than waiting for the device to wake next to set the alarms and then waiting again for the alarms to go off)?
> 
> If so, add a comment.

Using WakefulBroadcastReceiver is an established pattern:
http://developer.android.com/reference/android/support/v4/content/WakefulBroadcastReceiver.html

The final version of FeedService is always calling completeWakefulIntent() in a finally block.

It can happen that the device goes back to sleep after executing the broadcast receiver and just before the service is launched. This is prevented here: The broadcast receiver starts a wakelock, passes it to the service and the service releases the lock when completed. Without that we have no guarantee that the service is running anytime soon after the broadcast.

Additionally we are planning to have some experiments with triggering checks for content updates at specific times during the day.

> You need `completeWakefulIntent` here and above.
> 
> I'd just wrap the whole thing in `try`/`finally`. Maybe you'd want to make a subclass like, `WakefulIntentService` to do this, or to have this class:
> 
> onHandleIntent...
>   try {
>     onHandleIntentHelper(...)
>   } finally {
>     // release wakelock

This should be in the final version (a later patch refactored this a bit; I'll doublecheck).

> These are declared in this order for a reason, I take it? Add a comment.
> 
> fwiw, I'm not sure they're guaranteed to run in this order (but I guess that's okay).

Yeah, I assume the order will be preserved because of the time offset - but it's not too important. This is also only a first shot - I filed follow-up bugs to define or test other timings.

> How did you come up with the different intervals to start the broadcasts? Are they staggered so they don't overlap? If so, why is that necessary? Also, add a comment.

Mostly just to define an order (e.g. subscribe to something before checking for updates). We will experiment with different values here via A/B testing.
(Assignee)

Comment 109

2 years ago
https://reviewboard.mozilla.org/r/36769/#review36697

> It's unfortunate this parameter is unused... (I'd be surprised if there's no lint warning for that! :P)
> 
> Maybe add a comment to indicate it's intentionally unused? I think the IDE might print out warnings so `@SupressWarnings("unused")` (or whatever it is) might serve the same function.

The IDE does not warn because the parameter is enforced by the interface.

> This service starts the subscribe service, which requires network. If the network is off, subscribe will fail. Since we don't try to resend these subscribe requests, we have to wait until enroll is called again. Thus, enroll is a no-op if the network is off so we might as well say it requires network.

There are two reasons why I would like to keep this:
* Having no internet connection while we process the EnrollAction does not mean we won't have a connection when we start to process the subscribe requests.
* Currently we drop the subscribe actions on the floor if they fail. But I would like to do some proper scheduling of the tasks (bug 1248901) and re-try them if they fail (in a follow-up bug).
(Assignee)

Comment 110

2 years ago
https://reviewboard.mozilla.org/r/36771/#review36763

> This feature is entirely based around this preference being enabled (e.g. only `SetupAction` runs without the pref enabled and I'd argue that it should only run with the pref enabled, provided we add some more logic) – why don't we do this check all the time?

Good point. As time has passed I'm actually not entirely sure anymore. I think I wanted to make sure that at least the alarms will self-destruct and not be restored - but it looks like this does not happen yet. This code will have to be restructured to fit our planned experiments anyways (bug 1258812).

> The other `PreferenceScreen`s here also have a summary – do we need one?

I talked to antlam about this during our work week. For now we do not have a summary but will add one later if needed.
(Assignee)

Comment 111

2 years ago
https://reviewboard.mozilla.org/r/37001/#review36787

The reason for this is that you can have multiple bookmarks for the same wesite but different pages. All those pages reference the same feed. We want to have a subscription for the feed as long as we have at least one page pointing to this feed.

For example I could have bookmarked several articles of antlam's blog. But I only want to have one subscription and one notification if content on his blog changes.

website_url -> feed_url (n:1)
This tracks for what website_urls we found/determined a feed url. This could be used to annotate bookmarks in the UI or a smart folder or website based notification settings.

feed_url -> subscription (1:1)
This is the actual subscription for this feed. It's independent from the actual website.

> I think `getCount` is clearer but if you prefer otherwise, `moveToFirst` is safer than `...Next`.

I had the impression that getCount() would be more expensive because it would require the db to actually count all results now (instead of delivering the results as they come in). But it turns out moveToFirst() and others do call getCount() internally anyways. I'll change it to getCount() then.

> This code is really similar to `hasFeedUrlForWebsite` – I'd make a helper function.

Oh yeah, I created a generic hasResultsForSelection() and replaced all methods here that just returned a boolean.

> In we want to be profile-independent (but did we kill multiple profiles?), we should get the profile as an argument to the Intent. See: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryUploadService.java#169

Hmm. This service is often triggered by alarms. We would have to carry the profile name / directory through all those instances. I'll look at this in a follow-up bug.

> While it's not as necessary for `Service`s, can we pass in the Application Context here to avoid memory leaks?

Those objects only live for the scope of onHandleIntent(). Where do you see a potential leak?

> Start with `moveToFirst`

What's the thinking here? Is it that UrlAnnotations does not necessarily give us a fresh Cursor?

> I don't recall the details, but I'm pretty sure you can do this in one query (e.g. http://stackoverflow.com/a/21110662). Mentored follow-up?

Nice! I'll file a bug for that.

> It'd be great if the logic of what quantifies a "known" website was written in the same place. Deletions are here and insertions are in another action.

That's a good point and something that gets more important as we look at other sources for websites (e.g. history). I'll file a bug for that too.

> Why don't we get the feed url from the response anymore?

Changing feed urls on-the-fly is a PITA if we are using them as keys in the database. I don't think we'll need a feed URL migration path for this experiment. But that's something we should get back to at some point. I'll file a bug for that too. I didn't remove getting the URL from the feed data.
(Assignee)

Comment 112

2 years ago
https://reviewboard.mozilla.org/r/37843/#review36861

> Are these numbers useful if the user doesn't influence whether or not the action occurs? Is this just to make sure it's working? Here and all around.

Yeah, I think we will need them to evaluate whether users do not click the notification because they do not care or whether there's nothing to show them.

I see it as some kind of funnel:
Known sites with Feed  >  Subscription  >  Update / Notification  >  Click

If we want engangement (which is defined as clicking the notification here) then we have multiple places where we can optimize the flow: Do we need to support more sites? Do we need sites with more frequent updates? Do we need to optimizie the notification?
(Assignee)

Comment 113

2 years ago
Comment on attachment 8723311 [details]
MozReview Request: Bug 1241810 - Add configuration/whitelist for known sites we want to show notifications for. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36467/diff/3-4/
(Assignee)

Comment 114

2 years ago
Comment on attachment 8723312 [details]
MozReview Request: Bug 1241810 - Add EnrollAction for finding bookmarks of known sites to subscribe to. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36469/diff/3-4/
(Assignee)

Comment 115

2 years ago
Comment on attachment 8723313 [details]
MozReview Request: Bug 1241810 - Add WithdrawAction for removing subscriptions of bookmarks that do not exist anymore. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36471/diff/3-4/
(Assignee)

Comment 116

2 years ago
Comment on attachment 8723314 [details]
MozReview Request: Bug 1241810 - Add SetupAction for setting up various alarms to start FeedService. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36473/diff/3-4/
Attachment #8723314 - Attachment description: MozReview Request: Bug 1241810 - Add SetupAction for setting up various alarms to start FeedService. r?mcomella → MozReview Request: Bug 1241810 - Add SetupAction for setting up various alarms to start FeedService. r=mcomella
(Assignee)

Comment 117

2 years ago
Comment on attachment 8723315 [details]
MozReview Request: Bug 1241810 - Run setup on app start and when the device boots. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36475/diff/3-4/
Attachment #8723315 - Attachment description: MozReview Request: Bug 1241810 - Run setup on app start and when the device boots. r?mcomella → MozReview Request: Bug 1241810 - Run setup on app start and when the device boots. r=mcomella
(Assignee)

Comment 118

2 years ago
Comment on attachment 8723316 [details]
MozReview Request: Bug 1241810 - Do not do anything if not connected to a network or network is metered. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36477/diff/3-4/
Attachment #8723316 - Attachment description: MozReview Request: Bug 1241810 - Do not do anything if not connected to a network or network is metered. r?mcomella → MozReview Request: Bug 1241810 - Do not do anything if not connected to a network or network is metered. r=mcomella
(Assignee)

Updated

2 years ago
Attachment #8726176 - Attachment description: MozReview Request: Bug 1241810 - FeedSubscription: Treat ETag and "Last modified" header as optional. r?mcomella → MozReview Request: Bug 1241810 - FeedSubscription: Treat ETag and "Last modified" header as optional. r=mcomella
(Assignee)

Comment 119

2 years ago
Comment on attachment 8726176 [details]
MozReview Request: Bug 1241810 - FeedSubscription: Treat ETag and "Last modified" header as optional. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36765/diff/2-3/
(Assignee)

Comment 120

2 years ago
Comment on attachment 8726177 [details]
MozReview Request: Bug 1241810 - Bug 1238087 - Update style of content notification. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36767/diff/2-3/
Attachment #8726177 - Attachment description: MozReview Request: Bug 1241810 - Bug 1238087 - Update style of content notification. r?mcomella → MozReview Request: Bug 1241810 - Bug 1238087 - Update style of content notification. r=mcomella
(Assignee)

Comment 121

2 years ago
Comment on attachment 8726178 [details]
MozReview Request: Bug 1241810 - Restructure FeedService to always complete wakeful intent. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36769/diff/2-3/
Attachment #8726178 - Attachment description: MozReview Request: Bug 1241810 - Restructure FeedService to always complete wakeful intent. r?mcomella → MozReview Request: Bug 1241810 - Restructure FeedService to always complete wakeful intent. r=mcomella
(Assignee)

Comment 122

2 years ago
Comment on attachment 8726179 [details]
MozReview Request: Bug 1241810 - Bug 1247788 - Add "Notifications" in Settings. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36771/diff/2-3/
Attachment #8726179 - Attachment description: MozReview Request: Bug 1241810 - Bug 1247788 - Add "Notifications" in Settings. r?mcomella → MozReview Request: Bug 1241810 - Bug 1247788 - Add "Notifications" in Settings. r=mcomella
(Assignee)

Comment 123

2 years ago
Comment on attachment 8726180 [details]
MozReview Request: Bug 1241810 - Add support for wordpress.com. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36997/diff/2-3/
Attachment #8726180 - Attachment description: MozReview Request: Bug 1241810 - Add support for wordpress.com. r?mcomella → MozReview Request: Bug 1241810 - Add support for wordpress.com. r=mcomella
(Assignee)

Comment 124

2 years ago
Comment on attachment 8726181 [details]
MozReview Request: Bug 1241810 - Add additional test case for Blogger. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36999/diff/2-3/
Attachment #8726181 - Attachment description: MozReview Request: Bug 1241810 - Add additional test case for Blogger. r?mcomella → MozReview Request: Bug 1241810 - Add additional test case for Blogger. r=mcomella
(Assignee)

Updated

2 years ago
Attachment #8726182 - Attachment description: MozReview Request: Bug 1241810 - Use UrlAnnotations table as storage. r?mcomella → MozReview Request: Bug 1241810 - Use UrlAnnotations table as storage. r=mcomella
(Assignee)

Comment 125

2 years ago
Comment on attachment 8726182 [details]
MozReview Request: Bug 1241810 - Use UrlAnnotations table as storage. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37001/diff/2-3/
(Assignee)

Updated

2 years ago
Attachment #8726183 - Attachment description: MozReview Request: Bug 1241810 - Bug 1238087 - Add "notifications setting" action to content notification. r?mcomella → MozReview Request: Bug 1241810 - Bug 1238087 - Add "notifications setting" action to content notification. r=mcomella
(Assignee)

Comment 126

2 years ago
Comment on attachment 8726183 [details]
MozReview Request: Bug 1241810 - Bug 1238087 - Add "notifications setting" action to content notification. r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37841/diff/2-3/
(Assignee)

Comment 127

2 years ago
Comment on attachment 8726184 [details]
MozReview Request: Bug 1241810 - Bug 1251317 - Add telemetry for content notifications. r=mcomella,mfinkle

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37843/diff/2-3/
Attachment #8726184 - Attachment description: MozReview Request: Bug 1241810 - Bug 1251317 - Add telemetry for content notifications. r?mcomella,mfinkle → MozReview Request: Bug 1241810 - Bug 1251317 - Add telemetry for content notifications. r=mcomella,mfinkle
(Assignee)

Comment 128

2 years ago
Created attachment 8733904 [details]
MozReview Request: Bug 1241810 - Review Follow-up: Remove duplicated hasBeenUpdated() / isNewer() methods. r=me

Review commit: https://reviewboard.mozilla.org/r/42001/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42001/
(Assignee)

Comment 129

2 years ago
Created attachment 8733905 [details]
MozReview Request: Bug 1241810 - Review follow-up: Avoid logging potential personal information. r=me

Review commit: https://reviewboard.mozilla.org/r/42003/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42003/
(Assignee)

Comment 130

2 years ago
Created attachment 8733906 [details]
MozReview Request: Bug 1241810 - Review follow-up: Rename CheckAction.notify() to CheckAction.showNotification(). r=me

Review commit: https://reviewboard.mozilla.org/r/42005/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42005/
(Assignee)

Comment 131

2 years ago
Created attachment 8733907 [details]
MozReview Request: Bug 1241810 - Review follow-up: Rename actions to be more descriptive. r=me

Review commit: https://reviewboard.mozilla.org/r/42007/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42007/
(Assignee)

Comment 132

2 years ago
Created attachment 8733908 [details]
MozReview Request: Bug 1241810 - Review follow-up: Ensure that FeedService always calls completeWakefulIntent(). r=me

Review commit: https://reviewboard.mozilla.org/r/42009/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42009/
(Assignee)

Updated

2 years ago
Depends on: 1259165
(Assignee)

Updated

2 years ago
Depends on: 1259170
(Assignee)

Updated

2 years ago
Depends on: 1259171
(Assignee)

Updated

2 years ago
Depends on: 1259179
Comment on attachment 8723311 [details]
MozReview Request: Bug 1241810 - Add configuration/whitelist for known sites we want to show notifications for. r=mcomella

https://reviewboard.mozilla.org/r/36467/#review38575
Attachment #8723311 - Flags: review?(michael.l.comella) → review+
(In reply to Sebastian Kaspari (:sebastian) from comment #67)
> > Any reason not to make this (and all of the other helper methods) `static`?
> 
> No strong reason. While writing this I changed the implementation multiple
> times, from static to keeping the state as members to this final version
> with the short-lived ParserState object. Non-static classes/methods are
> usually nicer to test because you can mock/stub parts of it as needed. But
> this isn't needed here.

I'd argue that static methods are easier to test – side effects can only occur on the method arguments so you don't need to worry about the full object state when writing a test for it. Additionally, you can pass your dependencies in as arguments (e.g. like dependency injection) and, if necessary, mock them as input arguments.

That being said static methods can be harder to write because you can only call other static methods and need to find references to the objects you want to pass in (rather than using member variables, which are kind of like global variables) but I find that style of programming makes me write programs that are easier to reason about and saves time in the long run.

(In reply to Sebastian Kaspari (:sebastian) from comment #90)
> > nit: "Gecko" + blah blah?
> 
> I'm always afraid to get over the tag size limit (23?). Is there a strong
> reason why we want the prefix?

Someone in the past told me to include it – I think it's to make `logcat | grep Gecko` more convenient (though there are better solutions to that problem). So beyond that, I'm not sure why we'd want it (maybe it makes our output identifiable to other users?). In any case, I'm not concerned about the line length limit until we make more progress on bug 1242091.

> > Start with `moveToFirst`
> What's the thinking here? Is it that UrlAnnotations does not necessarily give us a fresh Cursor?

Oh! I didn't realize the docs (looking at SqliteDatabse.query) specified that the Cursor is positioned before the first entry – carry on. :)
(Assignee)

Comment 135

2 years ago
(In reply to Michael Comella (:mcomella) from comment #134)
> I'd argue that static methods are easier to test – side effects can only
> occur on the method arguments so you don't need to worry about the full
> object state when writing a test for it. Additionally, you can pass your
> dependencies in as arguments (e.g. like dependency injection) and, if
> necessary, mock them as input arguments.

I think this is true as long as you are testing a single isolated static method. What I'm referring to is testing code that uses those static methods internally. You are basically helpless and can't mock/stub this part.

That's for example why I'm hiding this static method behind a local method here:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/dlc/StudyAction.java#38-40

In tests I might want to stub or test this part:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/tests/background/junit4/src/org/mozilla/gecko/dlc/TestStudyAction.java#50

Another example are the static ConnectivityManagerCompat method. I could inline those calls but then I can't control/mock them:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/dlc/DownloadAction.java#253-256
(Assignee)

Updated

2 years ago
Keywords: leave-open
(Assignee)

Comment 137

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/a9f6d0f685da1c48e43aa744e711b9633a68accc
Bug 1241810 - Add configuration/whitelist for known sites we want to show notifications for. r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/58708bf1143cf66aab4f832aa2e2ec671c7da105
Bug 1241810 - Add EnrollAction for finding bookmarks of known sites to subscribe to. r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/5fbac6114c4eec86122cb4d386da9cd7d6e73cc0
Bug 1241810 - Add WithdrawAction for removing subscriptions of bookmarks that do not exist anymore. r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/0fa36d169a1f23fe3be1f708c055b2ada8312482
Bug 1241810 - Add SetupAction for setting up various alarms to start FeedService. r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/9f74cd8ed6144b740ae03163130a68c0f5bcae3d
Bug 1241810 - Run setup on app start and when the device boots. r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/9cb1ee777a66bce0f353a7527e16ef365371a859
Bug 1241810 - Do not do anything if not connected to a network or network is metered. r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/fb1c859114bd5dc4f0031aaa7a9cc04246e221b8
Bug 1241810 - FeedSubscription: Treat ETag and "Last modified" header as optional. r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/7c17a283a38a3887580baba972bd921723f83975
Bug 1241810 - Bug 1238087 - Update style of content notification. r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/67019279f9719a1957aa52cea5e937d296e7bedb
Bug 1241810 - Restructure FeedService to always complete wakeful intent. r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/6adb4ec5f1c8f38f72cfe5011ba0bb18c2036f25
Bug 1241810 - Bug 1247788 - Add "Notifications" in Settings. r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/78006d1bbcb29e09a7bc1eea3ac2e42b97731877
Bug 1241810 - Add support for wordpress.com. r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/3727b211b1e2c86c250925d15258e9ff4a9fca7b
Bug 1241810 - Add additional test case for Blogger. r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/5b263728ff7830134d42163fe702c02ae76358b4
Bug 1241810 - Use UrlAnnotations table as storage. r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/d0fd7d32ee5eb5caeb0340ff2ba2e2ab65a9d415
Bug 1241810 - Bug 1238087 - Add "notifications setting" action to content notification. r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/84180ddf65a78f5c7df8f2380939ac84fb81a6f5
Bug 1241810 - Bug 1251317 - Add telemetry for content notifications. r=mcomella,mfinkle

https://hg.mozilla.org/integration/fx-team/rev/5a3acee9a8080bfe3e2b68db13ceff218844b7af
Bug 1241810 - Review Follow-up: Remove duplicated hasBeenUpdated() / isNewer() methods. r=me

https://hg.mozilla.org/integration/fx-team/rev/ebd9aea169b65de33f24720758e7fc9054c1c6ce
Bug 1241810 - Review follow-up: Avoid logging potential personal information. r=me

https://hg.mozilla.org/integration/fx-team/rev/ef6b86cd60a4aa3535469da584e2a3b6807a2cb2
Bug 1241810 - Review follow-up: Rename CheckAction.notify() to CheckAction.showNotification(). r=me

https://hg.mozilla.org/integration/fx-team/rev/f409361bb6d5184d246f059f2e2ac71ec493e1b5
Bug 1241810 - Review follow-up: Rename actions to be more descriptive. r=me

https://hg.mozilla.org/integration/fx-team/rev/27e3abf6da862ba8f0e0b61e4fc5cf539ee52b66
Bug 1241810 - Review follow-up: Ensure that FeedService always calls completeWakefulIntent(). r=me

https://hg.mozilla.org/integration/fx-team/rev/4bc1d001be5c8a8307a1bc7bedc536b47e0d975e
Bug 1241810 - Follow-up: Fix FeedSubscription.hasBeenUpdated() after conflicts. r=me
(In reply to Michael Comella (:mcomella) from comment #71)

> > +<!ENTITY content_notification_title_plural "&formatD; websites updated">

There's no l10n note, but I assume &formatD; is going to be replaced by a number.

Besides this being broken for English ("1 websites updated"), it's even more broken for l10n, where there are up to 6 different forms depending on the number.
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_and_Plurals

Is there any way to work around this? I don't think we support plural forms in these .dtd, maybe we should drop the number all together.
sebastian: I think this bumped permissions -- I got asked about allowing Nightly to "Run at Startup".  Known?  Intentional?  We should co-ordinate with the new Push notifications permission.
Flags: needinfo?(s.kaspari)
(Assignee)

Comment 141

2 years ago
(In reply to Nick Alexander :nalexander from comment #140)
> sebastian: I think this bumped permissions -- I got asked about allowing
> Nightly to "Run at Startup".  Known?  Intentional?  We should co-ordinate
> with the new Push notifications permission.

Yeah intentional but not communicated well by me. I added the permission to receive BOOT_COMPLETED broadcasts and restore the alarms for the background tasks. How/where can we coordinate?
Flags: needinfo?(s.kaspari)
(Assignee)

Updated

2 years ago
Depends on: 1260390

Comment 142

2 years ago
(In reply to Sebastian Kaspari (:sebastian) from comment #141)
> (In reply to Nick Alexander :nalexander from comment #140)
> > sebastian: I think this bumped permissions -- I got asked about allowing
> > Nightly to "Run at Startup".  Known?  Intentional?  We should co-ordinate
> > with the new Push notifications permission.
> 
> Yeah intentional but not communicated well by me. I added the permission to
> receive BOOT_COMPLETED broadcasts and restore the alarms for the background
> tasks. How/where can we coordinate?

We should make sure any feature that adds a new permission is tracked in Aha, and we have a tag for tracking this. We need to make sure we update any appropriate SUMO articles when there's a new permission, and if a manual update is going to be required we need to do a cost-benefit analysis of shipping the feature with the product team.
(Assignee)

Updated

2 years ago
Depends on: 1261302
(Assignee)

Comment 143

2 years ago
(In reply to :Margaret Leibovic from comment #142)
> (In reply to Sebastian Kaspari (:sebastian) from comment #141)
> > (In reply to Nick Alexander :nalexander from comment #140)
> > > sebastian: I think this bumped permissions -- I got asked about allowing
> > > Nightly to "Run at Startup".  Known?  Intentional?  We should co-ordinate
> > > with the new Push notifications permission.
> > 
> > Yeah intentional but not communicated well by me. I added the permission to
> > receive BOOT_COMPLETED broadcasts and restore the alarms for the background
> > tasks. How/where can we coordinate?
> 
> We should make sure any feature that adds a new permission is tracked in
> Aha, and we have a tag for tracking this. We need to make sure we update any
> appropriate SUMO articles when there's a new permission, and if a manual
> update is going to be required we need to do a cost-benefit analysis of
> shipping the feature with the product team.

I filed bug 1261302 to discuss shipping a version without the permission and flagged barbara. I'll also linked the bug from the AHA card.
You need to log in before you can comment on or make changes to this bug.