We upload in onStart because: 1) we want to ping the server ASAP (e.g. before a possible crash) 2) An alternative, onStop, is not guaranteed to be called 3) We don't want to upload in onPause/onResume because it occurs too frequently (e.g. when fennec is obscured by dialogs). However, if a user uses fennec for the first time and decides never to open it, we won't receive their session data – do we care? Is it worth the complexity to capture this data? I think it depends on how we plan to use this data in the future and if that first run is important.

Margaret, do you know how important this data is? Or if not, who I should talk to to find out?

I think this is a good question for Barbara.

This is considered _very important data_ for product, so please yes!

(In reply to Barbara Bermes [:barbara] from comment #3) > This is considered _very important data_ for product, so please yes! Barbara, I was going to implement this so we only get this information for the very first session but I realized it's possible a user may stop using Firefox on the second or third time. Is there a particular number of sessions after which I should stop trying to upload their last session information? The downside of trying to upload all of the time (both when the browser is opened & closed) is that we'll be using more user data/battery. fwiw, I came up with a more comprehensive, but less robust implementation idea in bug 1277091. It's simple to implement but I'm afraid of the long tail of debugging needs so I don't think I'll have time to implement it.

This lets us put the two paths of upload code all in the same place. Review commit: https://reviewboard.mozilla.org/r/56712/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/56712/

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

The same preferences will be used by the new code & the old code. Review commit: https://reviewboard.mozilla.org/r/56716/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/56716/

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

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

We are doing more than just uploading in the delegate now. I didn't fix this in the previous commits because version control makes this non-trivial. Review commit: https://reviewboard.mozilla.org/r/56722/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/56722/

The pushed implementation only gets the users' last session info for the very first session (see comment 4 for more).

That's fine as per comment 11.

Comment on attachment 8758472 [details] MozReview Request: Bug 1273689 - Move core ping upload to BrowserAppDelegate. r=sebastian https://reviewboard.mozilla.org/r/56712/#review53578 Nice! ::: mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryCorePingUploadDelegate.java:52 (Diff revision 1) > + final SearchEngineManager searchEngineManager = browserApp.getSearchEngineManager(); > + searchEngineManager.getEngine(new UploadTelemetryCorePingCallback(browserApp)); > + } > + > + @WorkerThread // via constructor > + private TelemetryDispatcher getTelemetryDispatcher(final BrowserApp browserApp) { I wonder why this method is part of the outer class if it's only used by the inner class. ::: mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryCorePingUploadDelegate.java:61 (Diff revision 1) > + } > + return telemetryDispatcher; > + } > + > + private class UploadTelemetryCorePingCallback implements SearchEngineManager.SearchEngineCallback { > + private final WeakReference<BrowserApp> activityWeakReference; You can use BrowserAppDelegateWithReference if you do not want to handle the reference yourself. However if you move it inside UploadTelemetryCorePingCallback there's not much left in the outer delegate class. Maybe the delegate could implement SearchEngineManager.SearchEngineCallback directly and then there wouldn't be any inner class?

Comment on attachment 8758473 [details] MozReview Request: Bug 1273689 - Elaborate on why we upload when we do. r=sebastian https://reviewboard.mozilla.org/r/56714/#review53586

Comment on attachment 8758474 [details] MozReview Request: Bug 1273689 - Factor out getSharedPreferences call. r=sebastian https://reviewboard.mozilla.org/r/56716/#review53612

Comment on attachment 8758475 [details] MozReview Request: Bug 1273689 - Move session measurement calls to CorePingDelegate. r=sebastian https://reviewboard.mozilla.org/r/56718/#review53614

Comment on attachment 8758476 [details] MozReview Request: Bug 1273689 - Upload in onStop for first run. r=sebastian https://reviewboard.mozilla.org/r/56720/#review53616

Comment on attachment 8758477 [details] MozReview Request: Bug 1273689 - Rename CorePingDelegate. r=sebastian https://reviewboard.mozilla.org/r/56722/#review53618 ::: mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryCorePingDelegate.java:1 (Diff revision 1) > +/* Is it just reviewboard that gets confused and shows this as a delete and an add?

https://reviewboard.mozilla.org/r/56722/#review53618 > Is it just reviewboard that gets confused and shows this as a delete and an add? Yeah, I think so. Then again, hg seems to model renames as delete/adds in some contexts as well.

https://reviewboard.mozilla.org/r/56712/#review53578 > You can use BrowserAppDelegateWithReference if you do not want to handle the reference yourself. > > However if you move it inside UploadTelemetryCorePingCallback there's not much left in the outer delegate class. Maybe the delegate could implement SearchEngineManager.SearchEngineCallback directly and then there wouldn't be any inner class? I like it! Done.

https://hg.mozilla.org/integration/fx-team/rev/93325fc7131cb36584c85641132090ca8ba40cdd Bug 1273689 - Move core ping upload to BrowserAppDelegate. r=sebastian https://hg.mozilla.org/integration/fx-team/rev/6d3576194df89669978874205caacd4ef5196451 Bug 1273689 - Elaborate on why we upload when we do. r=sebastian https://hg.mozilla.org/integration/fx-team/rev/ab5501b1b7d5ebdae96199d1bc6ceaf3defe35f9 Bug 1273689 - Factor out getSharedPreferences call. r=sebastian https://hg.mozilla.org/integration/fx-team/rev/e28024927ff6727dd04f58244fe663f9a0290043 Bug 1273689 - Move session measurement calls to CorePingDelegate. r=sebastian https://hg.mozilla.org/integration/fx-team/rev/11a6d5a59addedc2f282e9c4d33cc2884dad00cf Bug 1273689 - Upload in onStop for first run. r=sebastian https://hg.mozilla.org/integration/fx-team/rev/8644fcfe29da58befd2c3ea28e230570570af925 Bug 1273689 - Rename CorePingDelegate. r=sebastian https://hg.mozilla.org/integration/fx-team/rev/7c0dce574b3f2d5e8f6e4e60caab5532f8e13fd6 Bug 1273689 - review - remove inner class & move methods to containing class; use BrowserAppDelegateWithReference. r=me

https://hg.mozilla.org/mozilla-central/rev/93325fc7131c https://hg.mozilla.org/mozilla-central/rev/6d3576194df8 https://hg.mozilla.org/mozilla-central/rev/ab5501b1b7d5 https://hg.mozilla.org/mozilla-central/rev/e28024927ff6 https://hg.mozilla.org/mozilla-central/rev/11a6d5a59add https://hg.mozilla.org/mozilla-central/rev/8644fcfe29da https://hg.mozilla.org/mozilla-central/rev/7c0dce574b3f