Mark Finkle (:mfinkle) (use needinfo?) Reporter
	Description • 10 years ago

In order to move ahead on some partnership opportunities, we'll need to start posting metrics for views and clicks to the Mozilla Content Services system.

Data formats are described here:
https://wiki.mozilla.org/Tiles/Data_Collection
https://github.com/oyiptong/onyx/blob/master/README.md

	Mark Finkle (:mfinkle) (use needinfo?) Reporter
	Comment 1 • 10 years ago

Given that the UI and thumbnails are all in Hava, we'll be writing the uploader in Java as well. Make sure we follow the same data collection rules as Desktop concerning enable/disable and security.

The current posting format expects a realtime ping, which is bad on mobile for bandwidth and power consumption. I've talked to the folks building the server and we'll be working on a batch-friendly method of posting the metrics.

	Richard Newman [:rnewman]
	Comment 2 • 10 years ago

Some notes from my thinking:

* Probably we'll use a BrowserHealthRecorder/HealthReportUploadService pattern here.

* The data format in Comment 0 doesn't include timestamps, because it expects realtime uploads. We need to resolve that, but -- for reasons that are obvious to mobile programmers, if not everyone -- timestamps are hard to come by.

The client clock is wrong. The server clock isn't immediately available -- we have to make a request to find it out.

So we need to capture enough state that we're able to provide the server with enough information to deduce when the event happened.

For example, we might track both currentTimeMillis and elapsedRealtime. If we know the server timestamp from the last upload, we can correlate this and estimate the current server timestamp.

When we submit, if we haven't rebooted since a capture, we can include a relative "milliseconds ago" value directly from elapsedRealtime. If we have rebooted (invalidating elapsedRealtime), we should provide a value calculated from the other two values.

* Uploads need to be frequent enough to hit the right day, and infrequent enough to be kind to the device. We've learned lots of lessons about the flaws in alarms on earlier Android versions, so we need to be careful here.

* We should respect background data policies on the device. (That also implies that we might need to do uploads during browsing, which shouldn't interfere with the user's activities.)

* In the future, we should consider allowing the server to drive scheduling -- "it's getting close to a day boundary, so please upload again in an hour if you have data to send".

* Remember to handle UA leakage, and to use a safe HttpClient version. a-s does this automatically.

	Brian Nicholson (:bnicholson) Assignee
	Comment 3 • 10 years ago

Implements the CP, upload service, and other necessary background components. Hopefully there are enough comments that I don't need to go into much detail here!

Still to be done:
* Add clock calculated/minimum values
* Implementing flushing/deleting files to/from disk in CP
* Improve algorithm for next upload time
* Tests

Here's a build with these patches applied: https://people.mozilla.org/~bnicholson/fennec_builds/tiles-1.apk. To test it, view/click the top sites tiles and check the POST data at http://requestb.in/14nrwqn1?inspect (current upload interval for debugging is 30 seconds).

	Brian Nicholson (:bnicholson) Assignee
	Comment 4 • 10 years ago

Adds the browser layer over the CP. As noted in my comments, I'm not too pleased with the browser side owning the data format, so I might move the JSON serialization inside of the CP.

	Brian Nicholson (:bnicholson) Assignee
	Comment 5 • 10 years ago

TopSitesGridView currently owns its own click listeners, making TilesRecorder integration a bit difficult. This dumbs down TopSitesGridView by extracting these listeners and putting them into TopSitesPanel. I think this makes more sense anyway since views shouldn't be tied to their controller logic.

	Brian Nicholson (:bnicholson) Assignee
	Comment 6 • 10 years ago

Adds the actual hooks to send to the service. Because orientation changes and relayouts can cause the grid to refresh, I added a boolean to make sure we don't send duplicate events for a single "view".

Still remaining: don't collect events fired from private browsing tabs. The API may also change depending on rnewman's feedback for the first two patches here.

	Lucas Rocha (:lucasr)
	Comment 7 • 10 years ago

Comment on attachment 8504517 [details] [diff] [review]
Move TopSitesGridView click listeners to TopSitesPanel

Review of attachment 8504517 [details] [diff] [review]:
-----------------------------------------------------------------

Fair enough.

	Richard Newman [:rnewman]
	Comment 8 • 10 years ago

Comment on attachment 8504510 [details] [diff] [review]
Implement background components for tiles metrics recording

Review of attachment 8504510 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent stuff!

::: mobile/android/base/AndroidManifest.xml.in
@@ +345,5 @@
>  
>  #include ../services/manifests/FxAccountAndroidManifest_activities.xml.in
>  #include ../services/manifests/HealthReportAndroidManifest_activities.xml.in
>  #include ../services/manifests/SyncAndroidManifest_activities.xml.in
> +#include ../services/manifests/TilesAndroidManifest_activities.xml.in

While you're here, newline after, and I think we want a build flag for this, no?

@@ +449,5 @@
>  
>  #include ../services/manifests/FxAccountAndroidManifest_services.xml.in
>  #include ../services/manifests/HealthReportAndroidManifest_services.xml.in
>  #include ../services/manifests/SyncAndroidManifest_services.xml.in
> +#include ../services/manifests/TilesAndroidManifest_services.xml.in

Same.

::: mobile/android/base/android-services.mozbuild
@@ +822,5 @@
>      'background/healthreport/upload/SubmissionPolicy.java',
>      'background/nativecode/NativeCrypto.java',
>      'background/preferences/PreferenceFragment.java',
>      'background/preferences/PreferenceManagerCompat.java',
> +    'background/tiles/TilesBroadcastReceiver.java',

You'll need to make these changes in a separate mozbuild file. This one is automatically generated from android-services, so your changes will be overwritten.

::: mobile/android/base/background/tiles/TilesConstants.java.in
@@ +14,5 @@
> +
> +  // BRN: Update these with real values.
> +  public static final String USER_AGENT = "Firefox-Android-Tiles/@MOZ_APP_VERSION@ (@MOZ_APP_DISPLAYNAME@)";
> +  public static final String SERVER_SPEC = "http://requestb.in/14nrwqn1";
> +  public static final int UPLOAD_INTERVAL = 1000 * 30; // testing only

UPLOAD_INTERVAL_MSEC (throughout).

::: mobile/android/base/background/tiles/TilesContentProvider.java
@@ +29,5 @@
> + * after the activity's onPause, so flushEventsToDisk should be (synchronously!) called
> + * there to minimize data loss.
> + *
> + * Note that since we're relying explicit flushes to write the queue, we're still susceptible
> + * to data loss if Fennec crashes.

+1 for excellent commenting.

@@ +34,5 @@
> + *
> + * [1] https://groups.google.com/d/msg/android-developers/PD-XxFn1hvI/vjoHVTJILPAJ
> + */
> +public class TilesContentProvider extends ContentProvider {
> +  private static final String LOG_TAG = TilesContentProvider.class.getSimpleName();

"GeckoTilesProvider" would make me happier.

@@ +38,5 @@
> +  private static final String LOG_TAG = TilesContentProvider.class.getSimpleName();
> +  private static final UriMatcher URI_MATCHER = new UriMatcher(UriMatcher.NO_MATCH);
> +  private static final int PATH_EVENT = 0;
> +
> +  private static final String[] COLUMNS = {

Might be worth a little comment, cribbing from a spec if we have one, to explain what these are and when each field might be null.

@@ +67,5 @@
> +    if (realtime < lastRealtime) {
> +      // If the current realtime is less than the last realtime, assume we've rebooted.
> +      clock++;
> +      Log.d(LOG_TAG, "New clock: " + clock);
> +      prefs.edit().putInt(TilesConstants.PREF_CLOCK, clock).apply();

Consider doing these two things, which are kinda related.

1. Rejig getAndUpdateRealtime and the clock setting to be read, compare, put both, apply once. Apply is async, but it's better to make sure these both get committed in the same transaction.

2. Lift this little fragment out into a static method. Input is prefs, current clock, and current realtime, side effect is new timestamp and new clock being written to the provided prefs, return value is new clock value. This'll make testing and isolation easier.

::: mobile/android/base/background/tiles/TilesUploadService.java
@@ +261,5 @@
> +      connection.setConnectTimeout(TilesConstants.UPLOAD_TIMEOUT);
> +      connection.setRequestMethod("POST");
> +      connection.setDoOutput(true);
> +      connection.setRequestProperty(HTTP_HEADER_UA, TilesConstants.USER_AGENT);
> +      connection.setRequestProperty(HTTP_HEADER_CONTENT_TYPE, "application/json");

Consider using BaseResource/BaseResourceDelegate instead.

Firstly it's not vulnerable to some Android sec vulns, secondly it's already tested and works with Mozilla service infra (e.g., Bagheera), thirdly it's not buggy on earlier Android releases -- the same code runs everywhere.

See BagheeraClient.java for example usage.

@@ +322,5 @@
> +    }
> +
> +    final PendingIntent pendingIntent = PendingIntent.getService(this, 0, uploadIntent, PendingIntent.FLAG_CANCEL_CURRENT);
> +    alarm.cancel(pendingIntent);
> +    alarm.set(AlarmManager.ELAPSED_REALTIME_WAKEUP, triggerRealtime, pendingIntent);

Fine for testing, but you probably want just ELAPSED_REALTIME when this ships. _WAKEUP will trigger a device wake, and probably kick off a bunch of other pending services, and thus negatively impact battery life.

	Richard Newman [:rnewman]
	Comment 9 • 10 years ago

Comment on attachment 8504513 [details] [diff] [review]
Implement TilesRecorder to interface with background components

Review of attachment 8504513 [details] [diff] [review]:
-----------------------------------------------------------------

Mainly perf concerns.

::: mobile/android/base/BrowserApp.java
@@ +678,5 @@
>                  Log.e(LOGTAG, "Error initializing media manager", ex);
>              }
>          }
> +
> +        mTilesRecorder = new TilesRecorder(this);

This will trigger a service init (and a CP init) at the end of onCreate.

Those will happen on a background thread, but that thread will be allocated and swap itself in right now, during critical init.

This happened for Stumbler, and it caused a noticeable startup regression.

Unlike with Stumbler, we can't just push this into delayed startup.

For BHR I made the recorder itself lazy. The tiles recorder doesn't have much to it, so I suggest delaying the *service* part (so we don't cause the service to init too soon), and see how painful the CP stuff is.

If it regresses startup, we'll need to figure out a way forward.

::: mobile/android/base/tiles/TilesRecorder.java
@@ +33,5 @@
> +    private final ContentProviderClient providerClient;
> +
> +    public TilesRecorder(final Context context) {
> +        ThreadUtils.assertOnUiThread();
> +        providerClient = context.getContentResolver().acquireContentProviderClient(TilesConstants.AUTHORITY);

Make sure this doesn't jank the UI if the CP takes time to start up. (Add a thread wait in the CP.)

@@ +76,5 @@
> +            // is disconnected from the tiles CP. But supporting true CP-style data would mean
> +            // separate JSON "tables", which would be a lot of extra work...
> +            // Perhaps TilesSnapshot can be a simple LinkedList-backed structure, and
> +            // TilesContentProvider can expose a method for queue -> JSON stringification.
> +            values.put(EventsContract.COLUMN_TILES, tiles.toString());

It slightly worries me that we're doing this much stuff on the UI thread. But TilesSnapshot is mutable, so punting it is difficult -- it could change before we record it.

Please measure this.

	Lucas Rocha (:lucasr)
	Comment 10 • 10 years ago

Comment on attachment 8504519 [details] [diff] [review]
Add TilesRecorder hooks to TopSitesPanel

Review of attachment 8504519 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good but I suspect that didRecordTilesView variable should be bound to the about:home life-cycle instead.

::: mobile/android/base/home/TopSitesPanel.java
@@ +497,5 @@
> +
> +            // Orientation changes and other events can cause the loader to reset, causing the UI
> +            // to be updated. We consider a "view" to be a single session where the user explicitly
> +            // opens and closes the fragment, so ignore any additional updates.
> +            if (!didRecordTilesView) {

How is this supposed to behave if you switching to other panels (causing the top sites panel to be destroyed) and then switching back to top sites? I suppose this variable should be bound to the about:home life-cycle instead of this specific fragment?

	u279076
	Comment 11 • 10 years ago

Ioana, can you please make sure this gets assigned and test planning started? Thanks.

	Brian Nicholson (:bnicholson) Assignee
	Comment 12 • 10 years ago

Posting these incrementally as I go along. This still needs to be updated with the clock calculation algorithm, disk flushing, and test cases, but I've addressed most of the feedback with some comments below.

(In reply to Richard Newman [:rnewman] from comment #8)
> >  #include ../services/manifests/FxAccountAndroidManifest_activities.xml.in
> >  #include ../services/manifests/HealthReportAndroidManifest_activities.xml.in
> >  #include ../services/manifests/SyncAndroidManifest_activities.xml.in
> > +#include ../services/manifests/TilesAndroidManifest_activities.xml.in
> 
> While you're here, newline after, and I think we want a build flag for this,
> no?

Any suggestions on how to partition Java code behind the build flag? If I cut out everything in background.*, the o.m.g.tiles.* imports/refs will be broken. If I cut those out, the TopSitesPanel imports/refs will be broken. I guess we could use reflection somewhere along the line to tear them apart, unless you have a better idea.

> You'll need to make these changes in a separate mozbuild file. This one is
> automatically generated from android-services, so your changes will be
> overwritten.

Where is that file?

> Consider doing these two things, which are kinda related.
> 
> 1. Rejig getAndUpdateRealtime and the clock setting to be read, compare, put
> both, apply once. Apply is async, but it's better to make sure these both
> get committed in the same transaction.
> 
> 2. Lift this little fragment out into a static method. Input is prefs,
> current clock, and current realtime, side effect is new timestamp and new
> clock being written to the provided prefs, return value is new clock value.
> This'll make testing and isolation easier.

I did #2, but didn't touch getAndUpdateRealtime(). Note that TilesUploadService calls this method when building the payload, so I'm not sure it makes sense to always touch the clock when calling this method.

> Consider using BaseResource/BaseResourceDelegate instead.

Done, though I can't say I'm a fan of the API. Why are exceptions handled in the delegate interface instead of being thrown? If I want to bubble the exceptions or return something derived from the response, I have to awkwardly store these in the delegate and fetch them separately. I much prefer the standard synchronous-style execute/return/throw used by most HTTP libs.

	Richard Newman [:rnewman]
	Comment 13 • 10 years ago

(In reply to Brian Nicholson (:bnicholson) from comment #12)

> > While you're here, newline after, and I think we want a build flag for this,
> > no?
> 
> Any suggestions on how to partition Java code behind the build flag?

I'm mainly concerned with the manifest includes. Conditionalizing those is the first big step.

For code:

* Put the build flag in AppConstants.java.in. (See the pile of flags at the bottom.)
* Check the flag at your call sites.

Additionally:

* Exclude any files from the build that aren't referenced from code. E.g., if your ContentProvider doesn't appear in the manifest, and you don't directly refer to FooBarContentProvider, you can eliminate it in moz.build.


> > You'll need to make these changes in a separate mozbuild file. This one is
> > automatically generated from android-services, so your changes will be
> > overwritten.
> 
> Where is that file?

Either use m/a/base/moz.build, or make a new one.

(In theory you could add these files to the template that a-s uses, but that's a recipe for disaster when some other human adds a new file to tiles/ and the reviewer doesn't remember that the mozbuild file will be overwritten.)

	Brian Nicholson (:bnicholson) Assignee
	Comment 14 • 10 years ago

Clock logic added, with other minor cleanup. Next up: writing to disk.

(In reply to Richard Newman [:rnewman] from comment #13)
> Either use m/a/base/moz.build, or make a new one.
> 
> (In theory you could add these files to the template that a-s uses, but
> that's a recipe for disaster when some other human adds a new file to tiles/
> and the reviewer doesn't remember that the mozbuild file will be
> overwritten.)

What's the difference between tiles and everything else in background/ (fxa, healthreport, etc.)? I assume those components do use the a-s template, so why wouldn't they be prone to the same issues?

	Brian Nicholson (:bnicholson) Assignee
	Comment 15 • 10 years ago

Untested, but modified the same places other flags are defined. Probably want this to default to off in some cases -- maybe just local builds?

	Richard Newman [:rnewman]
	Comment 16 • 10 years ago

(In reply to Brian Nicholson (:bnicholson) from comment #14)

> What's the difference between tiles and everything else in background/ (fxa,
> healthreport, etc.)? I assume those components do use the a-s template, so
> why wouldn't they be prone to the same issues?

That's exactly my point: fxa, healthreport, and everything else in background/ -- not just Sync -- is imported from the services repo.

You're writing the very first piece of background code that isn't implemented in Eclipse on GitHub first, and then used by Fennec.

	Ioana Chiorean
	Comment 17 • 10 years ago

Flaviu will take care of this from the QA Side.

	Richard Newman [:rnewman]
	Comment 18 • 10 years ago

Comment on attachment 8513110 [details] [diff] [review]
Add build flag for tiles metrics reporting

Review of attachment 8513110 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me, but…

	Richard Newman [:rnewman]
	Comment 19 • 10 years ago

Comment on attachment 8513107 [details] [diff] [review]
Implement background components for tiles metrics recording, v3

Review of attachment 8513107 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good; keep rollin'.

Might be time to start thinking about edge cases. See notes inline.

::: mobile/android/base/background/tiles/TilesAlarmService.java
@@ +17,5 @@
> + *
> + * This service simply ensures that a TilesUploadService intent is pending in the AlarmManager.
> + */
> +public class TilesAlarmService extends IntentService {
> +  private static final String LOG_TAG = TilesAlarmService.class.getSimpleName();

"GeckoTilesAlarm".

@@ +20,5 @@
> +public class TilesAlarmService extends IntentService {
> +  private static final String LOG_TAG = TilesAlarmService.class.getSimpleName();
> +
> +  public TilesAlarmService() {
> +    super(LOG_TAG);

Excellent use of logtag!

@@ +24,5 @@
> +    super(LOG_TAG);
> +  }
> +
> +  public void onCreate() {
> +    Log.d(LOG_TAG, "onCreate");

The mfinkle on my shoulder says to delete this, and take a shot.

@@ +30,5 @@
> +  }
> +
> +  @Override
> +  public void onDestroy() {
> +    Log.d(LOG_TAG, "onDestroy");

Two shots.

::: mobile/android/base/background/tiles/TilesBroadcastReceiver.java
@@ +21,5 @@
> + * We want to schedule uploads in this case, even though they haven't launched
> + * Firefox since boot.
> + */
> +public class TilesBroadcastReceiver extends BroadcastReceiver {
> +  public static final String LOG_TAG = TilesBroadcastReceiver.class.getSimpleName();

Same Gecko comment.

This getSimpleName stuff is a Nick-ism on which he and I disagree; if you're super keen on it, fine, but it doesn't float my boat.

::: mobile/android/base/background/tiles/TilesConstants.java.in
@@ +20,5 @@
> +
> +  public static final String ACTION_UPLOAD = "@ANDROID_PACKAGE_NAME@.tiles.UPLOAD";
> +
> +  // Android SharedPreferences branch where global (not per-profile) uploader
> +  // settings are stored.

Is this a global thing? Do we need it to be per-profile? (Are we finally abandoning the idea of multiple profiles?)

@@ +80,5 @@
> +
> +     /**
> +      * The calculated absolute time, relative to the server time, for this event.
> +      *
> +      * 0 if the time cannot be calculated.

Could not, versus cannot?

Do you retroactively set?

::: mobile/android/base/background/tiles/TilesContentProvider.java
@@ +103,5 @@
> +    TileEvent event;
> +    while ((event = recordedEvents.poll()) != null) {
> +      // BRN: Write these to disk instead of using them directly.
> +      cursor.addRow(new Object[] { event.event, event.index, event.tiles, clock, event.realtime,
> +                                   event.calcTime, event.minTime });

Can we populate calcTime at this point? Or are we happy to leave it up to the server?

@@ +124,5 @@
> +    // last upload server absolute time to make a reasonable guess for the absolute time this
> +    // event occurred, relative to the server time.
> +    final long realtime = SystemClock.elapsedRealtime();
> +    final long calcTime;
> +    if (lastUploadRealtime != 0) {

Perhaps > 0, instead of !=, just in case something insane happens and it's set to a negative? (Thanks, Java.)

::: mobile/android/base/background/tiles/TilesUploadService.java
@@ +62,5 @@
> +    providerClient = getContentResolver().acquireContentProviderClient(TilesConstants.AUTHORITY);
> +    if (providerClient == null) {
> +      // acquireContentProviderClient returns null only when there's no ContentProvider
> +      // associated with the given authority. In our case, that should be never.
> +      throw new IllegalStateException("Tiles CPC could not be acquired");

This'll also happen in safe mode, fwiw. If you want to save a future version of yourself work, maybe allow the providerClient to be null, and just make everything else a no-op?

@@ +89,5 @@
> +    ConnectivityManager connectivity = (ConnectivityManager) this.getSystemService(Context.CONNECTIVITY_SERVICE);
> +    NetworkInfo networkInfo = connectivity.getActiveNetworkInfo();
> +    if (networkInfo == null) {
> +      return false;
> +    }

This worries me a little, at a high level.

That is: if we're never connected (which will be the case if background data usage is disabled), then when do we discard data? If background data is disabled, should we never upload? (Sometimes you could call this foreground data, no?)

Worth noodling on a little bit.

@@ +260,5 @@
> +
> +  private static class TilesResourceDelegate extends BaseResourceDelegate {
> +    // BaseResource has an unusual API where exceptions must be handled by the methods in the
> +    // delegate. Store the exception here so we can throw it later.
> +    private TilesUploadException uploadException;

How about a didSucceed method, too?

	Nick Alexander :nalexander [he/him]
	Comment 20 • 10 years ago

Comment on attachment 8513110 [details] [diff] [review]
Add build flag for tiles metrics reporting

Review of attachment 8513110 [details] [diff] [review]:
-----------------------------------------------------------------

I expect you will need to add a stanza to m/a/base/moz.build adding this to the defines, but otherwise, it's looking fine.  Sorry for the delay.

::: configure.in
@@ +8606,1 @@
>  AC_SUBST(ENABLE_STRIP)

Include the matching AC_DEFINE.  It's rare to not need it.

::: mobile/android/confvars.sh
@@ +97,5 @@
>  if test ! "$RELEASE_BUILD"; then
>    MOZ_ANDROID_DOWNLOADS_INTEGRATION=1
>  fi
>  
> +# Enable tiles metrics reporting.

Should this be conditional on a channel, at least at first?

	Brian Nicholson (:bnicholson) Assignee
	Comment 21 • 10 years ago

New patches for simplified tiles reporting. If a tile with an ID was clicked, this sends a message to Gecko, which queues the event to post an XHR on pageshow. This should be pretty much complete as-is, though we need to change the test URL to the official one.

For testing, I'm planning on making a Robocop test that clicks a tile, posts to a .sjs file, then records and verifies the posted result.

	Brian Nicholson (:bnicholson) Assignee
	Comment 22 • 10 years ago

Comment on attachment 8516356 [details] [diff] [review]
Implement tile reporting components

Review of attachment 8516356 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/modules/TilesReporter.jsm
@@ +97,5 @@
> +
> +    let evt;
> +    while (evt = this._queuedEvents.pop()) {
> +      let xhr = new XMLHttpRequest();
> +      xhr.open("POST", "http://requestb.in/1k9tels1", true);

Changed this to `xhr.open("POST", this._reportURL, true);` locally.

	Richard Newman [:rnewman]
	Comment 23 • 10 years ago

Comment on attachment 8516356 [details] [diff] [review]
Implement tile reporting components

Review of attachment 8516356 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/tiles/TilesRecorder.java
@@ +43,5 @@
> +                    // Tiles may be null if there are pinned tiles with blank tiles in between.
> +                    continue;
> +                }
> +
> +                tilesJson.put(tileToJson(tile, currentTileIndex, i));

tileToJson can return {}. Is that a meaningful thing to send? If so, commenting that here would be worthwhile.

@@ +62,5 @@
> +
> +            final JSONObject data = new JSONObject();
> +            data.put(action, clickedTileIndex);
> +            data.put("tiles", tilesJson);
> +            GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Tiles:Click", data.toString()));

Extract constants.

@@ +68,5 @@
> +            Log.e(LOG_TAG, "JSON error", e);
> +        }
> +    }
> +
> +    private JSONObject tileToJson(Tile tile, int tileIndex, int viewIndex) throws JSONException {

I have a moderate preference for tileToJSON, or jsonForTile.

::: mobile/android/confvars.sh
@@ +95,5 @@
>  fi
>  
> +# Enable tiles reporting only in release builds.
> +if test "$RELEASE_BUILD"; then
> +  MOZ_ANDROID_TILES_REPORTING=1

We should probably also turn this off if MOZ_DATA_REPORTING is unset (or set that to true if this is on) -- but see Bug 1093401, which gets you off the hook :)

::: mobile/android/modules/TilesReporter.jsm
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +"use strict"

Newline before, semicolon after, please.

@@ +20,5 @@
> +  _reportURL: null,
> +
> +  init: function () {
> +    if (this._reportURL) {
> +      Log.w(LOG_TAG, "TilesReporter already initted");

s/initted/inited

(Source: http://en.wiktionary.org/wiki/inited)

@@ +32,5 @@
> +      Log.d(LOG_TAG, "Tiles reporting is disabled");
> +      return;
> +    }
> +
> +    Services.obs.addObserver(this, "Tiles:Click", false);

const, plz

@@ +95,5 @@
> +      return;
> +    }
> +
> +    let evt;
> +    while (evt = this._queuedEvents.pop()) {

This doesn't seem quite right. Tell me if I'm misunderstanding the code.

If any window fires a pageshow, we'll flush the queued events.

So if I tap a tile for a dead site, then return to about:home, the load for aboutHome.xhtml will fire pageshow, and we'll report the page load as successful.

I think that what we ought to be doing is tying the reporting event to that particular page load itself.

The browser.js Tab entity has an id, various target tracking stuff, and onStateChange. Perhaps that's a place to hook in to indicate that this load is from a tile, and detect correctly when it succeeds or fails?

@@ +98,5 @@
> +    let evt;
> +    while (evt = this._queuedEvents.pop()) {
> +      let xhr = new XMLHttpRequest();
> +      xhr.open("POST", "http://requestb.in/1k9tels1", true);
> +      xhr.setRequestHeader("Content-type","application/json");

Nit: space after comma, "Content-Type".

	Brian Nicholson (:bnicholson) Assignee
	Comment 24 • 10 years ago

(In reply to Richard Newman [:rnewman] from comment #23)
> tileToJson can return {}. Is that a meaningful thing to send? If so,
> commenting that here would be worthwhile.

It's what's expected by the API if none of the optional fields are set, so yes.

On the other hand, I think the new plan is to send the minimum we need to record tracking clicks, and we'll manage telemetry for clicks/pins/views on our own. So we're now sending only click events, and we send the event only if the click happened on a tracked tile.

If we wanted, we could reduce this even further by sending only the tile that was clicked and nothing else -- e.g., every uploaded click event will look like this: { click: 0, tiles: [ { id: N } ] }. That means we'd be sending the bare minimum (while complying with the existing API) for every significant click event.

But it shouldn't hurt anything to include this additional data for now since it's already been implemented, so I just left it in. We can refine this in a follow-up if needed.

> This doesn't seem quite right. Tell me if I'm misunderstanding the code.
> 
> If any window fires a pageshow, we'll flush the queued events.

That's correct -- I thought we were using pageshow only to prevent the upload from potentially slowing down pageload, but making sure the clicked page loaded makes sense. I've refactored this by killing off the JSM, made the events tab-specific, and hooking into STATE_STOP to reset the data after any missing pageshows.

> @@ +98,5 @@
> > +    let evt;
> > +    while (evt = this._queuedEvents.pop()) {
> > +      let xhr = new XMLHttpRequest();
> > +      xhr.open("POST", "http://requestb.in/1k9tels1", true);
> > +      xhr.setRequestHeader("Content-type","application/json");
> 
> Nit: space after comma, "Content-Type".

Meh, copy-pasted from elsewhere!

	Brian Nicholson (:bnicholson) Assignee
	Comment 25 • 10 years ago

Minor change -- moves the BrowserApp bits out of this patch.

	Brian Nicholson (:bnicholson) Assignee
	Comment 26 • 10 years ago

Test that verifies the tiles upload process from start to finish; that is:
1) tracked suggested sites load and appear in the top sites,
2) IDs are correctly attached to tiles, and
3) clicking a tracked tile makes the expected POST request.

I originally wanted to have a separate testTilesReporter instead of piggybacking onto testDistribution, but there's lots of shared code there that would need to be factored out.

	Brian Nicholson (:bnicholson) Assignee
	Comment 27 • 10 years ago

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=727467b079e5

	Brian Nicholson (:bnicholson) Assignee
	Comment 28 • 10 years ago

Updated for the new TilesRecorder implementation. We're no longer keeping track of view events, so we only have to worry about clicks.

	Brian Nicholson (:bnicholson) Assignee
	Comment 29 • 10 years ago

And I just remembered my private browsing comment from comment 6, so here's an updated patch that omits private browsing tabs.

	Richard Newman [:rnewman]
	Comment 30 • 10 years ago

Comment on attachment 8516968 [details] [diff] [review]
Implement tile reporting components, v3

Review of attachment 8516968 [details] [diff] [review]:
-----------------------------------------------------------------

Please add the confvar to AppConstants.java.in.

This looks good. I have two small high-level concerns:

* Bah more code in browser.js. Alas.
* I gently wonder whether we need TilesRecorder and the separate event, or if this is something that SuggestedSites should be calculating and sending along *as* the "open this page" event. Food for thought in separate patches.

::: mobile/android/chrome/content/browser.js
@@ +479,5 @@
> +      // is, a report URL is set in prefs).
> +      gTilesReportURL = Services.prefs.getCharPref("browser.tiles.reportURL");
> +      Services.obs.addObserver(this, "Tiles:Click", false);
> +    } catch (e) {
> +      dump("Tiles reporting is disabled");

dumps need a trailing newline, but this line should probably just evaporate.

@@ +4157,5 @@
>          // only send pageshow for the top-level document
>          if (aEvent.originalTarget.defaultView != this.browser.contentWindow)
>            return;
>  
> +        // Tiles data will be set for this tab only if:

set or sent?

@@ +4164,5 @@
> +        if (this.tilesData) {
> +          let xhr = new XMLHttpRequest();
> +          xhr.open("POST", gTilesReportURL, true);
> +          xhr.setRequestHeader("Content-Type", "application/json");
> +          xhr.send(this.tilesData);

I think you probably want to do this work waaaay down on line 4224, after we've done all the important user-facing stuff.

::: mobile/android/confvars.sh
@@ +95,5 @@
>  fi
>  
> +# Enable tiles reporting only in release builds.
> +if test "$RELEASE_BUILD"; then
> +  MOZ_ANDROID_TILES_REPORTING=1

A small red flag here. Nothing to get worked up about, so just noting for completeness or commenting.

You're always *building* the tiles components; all this does is eliminate the pref.

That -- either by virtue of the missing pref, or by later assumption -- might cause trouble with tests (we'll see in the next patch!), and it also misses an opportunity to exclude code.

You might consider having two flags: one a build flag, and one the reporting URL, or something like that.

	Richard Newman [:rnewman]
	Comment 31 • 10 years ago

Comment on attachment 8517113 [details] [diff] [review]
Add tile reporting tests

Review of attachment 8517113 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/tests/testDistribution.java
@@ +454,5 @@
>          TestableDistribution.clearReferrerDescriptorForTesting();
>      }
>  
> +    public void checkTilesReporting() throws Exception {
> +        // Slight hack: Force top sites grid to reload.

Top hack, much slight.

	Lucas Rocha (:lucasr)
	Comment 32 • 10 years ago

Comment on attachment 8517122 [details] [diff] [review]
Add TilesRecorder hooks to TopSitesPanel, v3

Review of attachment 8517122 [details] [diff] [review]:
-----------------------------------------------------------------

Good.

	Brian Nicholson (:bnicholson) Assignee
	Comment 33 • 10 years ago

(In reply to Richard Newman [:rnewman] from comment #30)
> Please add the confvar to AppConstants.java.in.

Didn't make this change since we aren't using this flag anywhere, and I wasn't sure what to do with it. But I ended up just removing MOZ_ANDROID_TILES_REPORTING altogether since it's not a true build flag. The pref is now just guarded by RELEASE_BUILD.

> * I gently wonder whether we need TilesRecorder and the separate event, or
> if this is something that SuggestedSites should be calculating and sending
> along *as* the "open this page" event. Food for thought in separate patches.

Not really sure how SuggestedSites would own this. Can you elaborate on what this API would look like? We could move everything from TilesRecorder into SuggestedSites, but I can't tell if that's what you're suggesting.

> > +        // Tiles data will be set for this tab only if:
> 
> set or sent?

Well, both. By "set" I meant "non-null".

	Brian Nicholson (:bnicholson) Assignee
	Comment 34 • 10 years ago

https://hg.mozilla.org/integration/fx-team/rev/50f624561c25
https://hg.mozilla.org/integration/fx-team/rev/01a4dd4275fe
https://hg.mozilla.org/integration/fx-team/rev/0f445e73157c
https://hg.mozilla.org/integration/fx-team/rev/53fa79d7cb03

	Brian Nicholson (:bnicholson) Assignee
	Comment 35 • 10 years ago

Need to uplift this after it lands in m-c.

	Richard Newman [:rnewman]
	Comment 36 • 10 years ago

(In reply to Brian Nicholson (:bnicholson) from comment #33)

> Didn't make this change since we aren't using this flag anywhere, and I
> wasn't sure what to do with it. But I ended up just removing
> MOZ_ANDROID_TILES_REPORTING altogether since it's not a true build flag. The
> pref is now just guarded by RELEASE_BUILD.

Works for me.


> Not really sure how SuggestedSites would own this. Can you elaborate on what
> this API would look like? We could move everything from TilesRecorder into
> SuggestedSites, but I can't tell if that's what you're suggesting.

Sorry, I really meant TopSitesPanel (which you already hook into in part 3), but moreover the UrlOpenListener that we really use to trigger the page load. I was kinda implying that the UrlOpenListener channel that we use would be 'widened', with only one message sent over the bridge to Gecko. "Open this link, and here's why".

You're not far from that point now, though, and the clarity of two messages recombined at the other end isn't bad, so nbd.

	Carsten Book [:Tomcat]
	Comment 37 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/50f624561c25
https://hg.mozilla.org/mozilla-central/rev/01a4dd4275fe
https://hg.mozilla.org/mozilla-central/rev/0f445e73157c
https://hg.mozilla.org/mozilla-central/rev/53fa79d7cb03

	Brian Nicholson (:bnicholson) Assignee
	Comment 38 • 10 years ago

Comment on attachment 8504517 [details] [diff] [review]
Move TopSitesGridView click listeners to TopSitesPanel

Approval Request Comment
[Feature/regressing bug #]: New feature.
[User impact if declined]: Mozilla won't get tiles tracking data, meaning we aren't able to take advantage of partner deals.
[Describe test coverage new/current, TBPL]: Includes testDistribution rc test changes. This test fails intermittently, and is fixed by bug 1095514.
[Risks and why]: Low risk. Messages Gecko to fire off an XHR when a tile is clicked, and should have no user-facing changes.
[String/UUID change made/needed]: None.

	Ryan VanderMeulen [:RyanVM]
	Comment 39 • 10 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/e1a303b5e43f
https://hg.mozilla.org/releases/mozilla-aurora/rev/18c86a0bf105
https://hg.mozilla.org/releases/mozilla-aurora/rev/6cce1dc5f97e
https://hg.mozilla.org/releases/mozilla-aurora/rev/3a07770613e6