Closed Bug 1229274 Opened 4 years ago Closed 4 years ago

Investigate tracking when a screenshot is taken in Firefox

Categories

(Firefox for Android :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 46
Tracking Status
firefox44 --- fixed
firefox45 --- fixed
firefox46 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: mfinkle, Assigned: mfinkle)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

Attached patch screenshot-observer WIP (obsolete) — Splinter Review
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.
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)).
(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.
Attached patch screenshot-observer v0.1 (obsolete) — Splinter Review
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.
Assignee: nobody → mark.finkle
Attachment #8693977 - Attachment is obsolete: true
Attachment #8698131 - Flags: review?(liuche)
Attachment #8698131 - Flags: feedback?(nalexander)
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.)
Attachment #8698131 - Flags: feedback?(nalexander) → feedback+
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.
Attachment #8698131 - Attachment is obsolete: true
Attachment #8698131 - Flags: review?(liuche)
Attachment #8699175 - Flags: review?(liuche)
Comment on attachment 8699175 [details] [diff] [review]
screenshot-observer v0.2

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

lgtm
Attachment #8699175 - Flags: review?(liuche) → review+
https://hg.mozilla.org/mozilla-central/rev/51cf33ff7408
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
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
Attachment #8699175 - Flags: approval-mozilla-beta?
Attachment #8699175 - Flags: approval-mozilla-aurora?
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.
Attachment #8699175 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Flags: needinfo?(michael.l.comella)
Flags: needinfo?(mark.finkle)
Flags: needinfo?(margaret.leibovic)
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+
Attachment #8699175 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
Flags: needinfo?(michael.l.comella)
Flags: needinfo?(margaret.leibovic)
Flags: needinfo?(mark.finkle)
(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
Flags: needinfo?(mark.finkle)
You need to log in before you can comment on or make changes to this bug.