1229274 - Investigate tracking when a screenshot is taken in Firefox

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: screenshot-observer WIP

When someone takes a screenshot of the current webpage in Firefox, it's a signal that the page has some importance. Android does not allows apps to catch the hardware screenshot hardware buttons (volume down + power), so we need to use other approaches.

This WIP patch cane detect when a screenshot occurs. It uses a MediaStore content observer to listen for new media files. A screenshot is placed in the "Screenshots" album. Testing shows this works.

Testing also shows that we can set other MediaStore content values on the screenshot, like URL, but the Photos app does not display the additional values.

The WIP approach could be used to add "weight" the the current URL for frecency. It could also be used to tag the URL as "screenshotted" when viewing history.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Just want to make a note that this could be a privacy issue whether we share the data beyond the user's device or not: users might be confused and sketched out why Firefox tracks when you take screenshots (e.g. like how I was sketched out when I heard that Chrome tracks your cursor movements (which I never verified if it was true or not)).

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Michael Comella (:mcomella) from comment #1)
> Just want to make a note that this could be a privacy issue whether we share
> the data beyond the user's device or not: users might be confused and
> sketched out why Firefox tracks when you take screenshots (e.g. like how I
> was sketched out when I heard that Chrome tracks your cursor movements
> (which I never verified if it was true or not)).

Good point. It doesn't remove the privacy concern, but the code will only observe screenshots while Firefox is the foreground app. We only care about screenshots of Firefox content.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: screenshot-observer v0.1

This patch adds simple telemetry for sharing via a screenshot.

A few things:
1. I plan to remove the Logs before landing
2. I modeled the helper on DynamicToolbar, which creates an instance in the class definition and then sets a listener. Is this OK? or do we want to create an instance in BrowserApp.onCreate and pass in the context and listener at that point?

NI'ing Nick for #2 feedback.

Comment 4

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8698131 [details] [diff] [review]
screenshot-observer v0.1

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

For the initialization, I don't have strong feelings.  I prefer `final` everywhere; if you create in `onCreate`, you can make the context final.  Six of one, I think.

I would add a class comment explaining that we only witness events when Fennec is in the foreground.

Overall, just a few nits.  Crafty patch!

::: mobile/android/base/util/ScreenshotObserver.java
@@ +1,1 @@
> +package org.mozilla.gecko.util;

nit: license header.

@@ +6,5 @@
> +import android.provider.MediaStore;
> +import android.util.Log;
> +
> +public class ScreenshotObserver {
> +	private static final String LOGTAG = "ScreenshotObserver";

nit: indenting throughout is funky.

LOGTAG should start with Gecko.

@@ +39,5 @@
> +                    MediaStore.Images.ImageColumns.TITLE
> +	};
> +
> +	public void start() {
> +        if (android.os.Build.VERSION.SDK_INT < 14) {

We generally use `Versions` for this.  I don't have a strong opinion here, though.

@@ +45,5 @@
> +        }
> +
> +        try {
> +            if (mediaObserver == null) {
> +                context.getContentResolver().registerContentObserver(MediaStore.Images.Media.EXTERNAL_CONTENT_URI, false, mediaObserver = new MediaObserver());

Can you explain why you only want external, not internal as well?

Also, this inline = pattern is hard to read -- lift that to the prev statement, please.

@@ +72,5 @@
> +        }
> +
> +        @Override
> +        public void onChange(boolean selfChange) {
> +            super.onChange(selfChange);

Is this on the main thread?  (I think it can be, although it's not guaranteed.)  Consider specifying the Handler to be the Gecko background thread handler.  The `query()` below is blocking.

@@ +74,5 @@
> +        @Override
> +        public void onChange(boolean selfChange) {
> +            super.onChange(selfChange);
> +
> +            // Find the most recent image added to the MediaStore and see if it's a screenshot

nit: full sentence.

@@ +76,5 @@
> +            super.onChange(selfChange);
> +
> +            // Find the most recent image added to the MediaStore and see if it's a screenshot
> +            try {
> +                Cursor cursor = context.getContentResolver().query(MediaStore.Images.Media.EXTERNAL_CONTENT_URI, mediaProjections, null, null, "date_added DESC LIMIT 1");

Prefer http://developer.android.com/reference/android/provider/MediaStore.MediaColumns.html#DATE_ADDED.

@@ +77,5 @@
> +
> +            // Find the most recent image added to the MediaStore and see if it's a screenshot
> +            try {
> +                Cursor cursor = context.getContentResolver().query(MediaStore.Images.Media.EXTERNAL_CONTENT_URI, mediaProjections, null, null, "date_added DESC LIMIT 1");
> +                if (cursor != null) {

Prefer to early abort, especially with the `listener != null` check.

@@ +88,5 @@
> +                        Log.i(LOGTAG, "album: " + album);
> +                        long date = cursor.getLong(3);
> +                        String title = cursor.getString(4);
> +                        Log.i(LOGTAG, "title: " + title);
> +                        if (album != null && album.toLowerCase().contains("screenshot")) {

Is there a constant for this somewhere in Android?  (I don't see one.)

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: screenshot-observer v0.2

I made the changes Nick suggested. I am currently using EXTERNAL because it seems to work on my devices. I refactored the code to be able to take multiple content observers (one for INTERNAL and one for EXTERNAL) if needed. We can see what happens in Nightly.

Chenxia - review to you for a simple UI Telemetry probe. It is consistent with our other SHARE probes.

Comment 6

[Chenxia Liu [:liuche]]

Comment on attachment 8699175 [details] [diff] [review]
screenshot-observer v0.2

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

lgtm

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/51cf33ff7408

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/51cf33ff7408

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8699175 [details] [diff] [review]
screenshot-observer v0.2

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Gives us more telemetry on how people share
[Describe test coverage new/current, TreeHerder]: Working on Nightly for a few days
[Risks and why]: Low risk, but I will likely need a tweaked patch for Beta
[String/UUID change made/needed]: none

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8699175 [details] [diff] [review]
screenshot-observer v0.2

The code change is fairly large, let's stabilize on Aurora45 first and then decide about Beta.

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/645124a8698c

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This seems to need a rebased patch for 44 due to Bug 1107811 not being uplifted. Can one of you add a patch to work around that? If it comes in the next hour or so, it can get into 44b5, otherwise it'll slip to b6.

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8699175 [details] [diff] [review]
screenshot-observer v0.2

While Fennec stability has been less than ideal past few releases and patches of this kind (fairly large) seem more risky, we need better telemetry data to determine areas of improvement in future releases. Taking this fix as it's been on Aurora for a few days without any known issues. Beta44+

Comment 14

[:Margaret Leibovic]

(In reply to Wes Kocher (:KWierso) (On vacation until Dec 28) from comment #12)
> This seems to need a rebased patch for 44 due to Bug 1107811 not being
> uplifted. Can one of you add a patch to work around that? If it comes in the
> next hour or so, it can get into 44b5, otherwise it'll slip to b6.

I think it's fine if it slips. mfinkle can deal with this when he gets around to it.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: screenshot-observer for beta (fx44)

Comment 16

[Carsten Book [:Tomcat]]

(In reply to Mark Finkle (:mfinkle) from comment #15)
> Created attachment 8702338 [details] [diff] [review]
> screenshot-observer for beta (fx44)

Hi Mark, this failed to apply to beta:

adding 1229274 to series file
renamed 1229274 -> screenshot-observer
applying screenshot-observer
patching file mobile/android/base/BrowserApp.java
Hunk #4 FAILED at 876
1 out of 4 hunks FAILED -- saving rejects to file mobile/android/base/BrowserApp.java.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh screenshot-observer

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/releases/mozilla-beta/rev/afcf7a7af40a

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/afcf7a7af40a