This method is responsible for toggling the state: http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js?force=1#497 Specifically, the probe needs to measure how long it takes for us from line 522 (setting the status to STATE_TRANSITION_STARTED) to the end of the method. The pieces before line 522 can potentially show UI, so we shouldn't measure those parts (since we'd be measuring user's response time ;-). We should ideally have two probes, one for entering and one for leaving the private browsing mode.

Comment on attachment 568477 [details] [diff] [review] telemetry for private mode switch r- because most of the interesting stuff in private browsing mode switching happens during the "private-browsing" notification. I'm going to write a patch on top of this one which is going to turn off recording in the telemetry service right before we record this histogram, and turn it back on if it was enabled to begin with.

While measuring how long it takes to enter/exit private browsing mode also implies usage of the feature, that too is in scope for the updated opt-in string caused by fixing bug 685373. There's no privacy risk analysis needed here, so clearing the review flag.

I'm assigning to ehsan for now to avoid other people snapping it up while we figure out the status of the work being done by an academic group.

It's unclear why that effort has not been documented elsewhere, especially in the bug. Would have saved our developers time in trying to fix an apparently unowned bug.

Sorry for the confusion here, guys. Allen has been working on this bug and we've been talking about this over email, and he has a work in progress patch. Paolo's patch here <https://bugzilla.mozilla.org/attachment.cgi?id=593836> is more complete though, and I'd like to move forward with that. I'll talk to Allen to see if he's willing to continue based on Paolo's patch or it he's willing to work on something else instead, and will update the bug accordingly.

Ehsan, any update on this?

(In reply to Dietrich Ayala (:dietrich) from comment #9) > Ehsan, any update on this? No, I have not heard back from either Allen or Paolo on this, I don't think.

(In reply to Ehsan Akhgari [:ehsan] from comment #10) > (In reply to Dietrich Ayala (:dietrich) from comment #9) > > Ehsan, any update on this? > > No, I have not heard back from either Allen or Paolo on this, I don't think. So, I think we can continue from where we left on bug 723075.

Comment on attachment 613512 [details] [diff] [review] The patch Review of attachment 613512 [details] [diff] [review]: ----------------------------------------------------------------- This looks mostly good except for the comments below. I'd like to look at another version of the patch which fixes these comments. ::: browser/components/privatebrowsing/src/nsPrivateBrowsingService.js @@ +338,5 @@ > + } else { > + if (this._quitting) { > + // If we are quitting the browser, we don't care collecting the data, > + // because we wouldn't be able to record it with telemetry. > + return; So, can the telemetry service submit the data gathered in the last run when Firefox gets restarted now? ::: browser/components/privatebrowsing/test/unit/test_privatebrowsing_autostart.js @@ +39,4 @@ > > function run_test() { > PRIVATEBROWSING_CONTRACT_ID = "@mozilla.org/privatebrowsing;1"; > + load("do_test_privatebrowsing_telemetry.js"); Please also add another test named test_privatebrowsingwrapper_telemetry.js which tests using the -wrapper PRIVATEBROWSING_CONTRACT_ID, similar to the other "wrapper" tests in this directory.

(In reply to Ehsan Akhgari [:ehsan] from comment #12) > So, can the telemetry service submit the data gathered in the last run when > Firefox gets restarted now? I couldn't see a similar change, from a quick look at the revision history. The fact is we weren't able to gather data at that point during shutdown. Taras might know more?

Forgot to update a comment line describing the new test file, sorry.

Posted to the try server: http://tbpl.mozilla.org/?tree=Try&rev=f83f5c396ec9

Try run for f83f5c396ec9 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=f83f5c396ec9 Results (out of 218 total builds): exception: 4 success: 171 warnings: 43 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-f83f5c396ec9

This is failing tests, Paolo can you please look into that? Thanks!

(In reply to Ehsan Akhgari [:ehsan] from comment #17) > This is failing tests, Paolo can you please look into that? Thanks! Yes, the telemetry test fails intermittently. Another run here: https://tbpl.mozilla.org/?tree=Try&rev=da6dbfde78f0 I'll look into this.

The issue was probably that we check the "sum" property to verify that we have data in the histogram, but it is zero when the time it takes for a transition is not measurable. In fact, the tests failed on Windows, where we have a coarse timer resolution. Checking if we have some data in the zeroth bucket, in addition to the sum calculated for the other buckets, apparently solves the issue.

Do you want me to review this patch?

Comment on attachment 618233 [details] [diff] [review] Fix check for data presence in histogram (In reply to Ehsan Akhgari [:ehsan] from comment #20) > Do you want me to review this patch? Sure, feel free to review it, the only change is how we check the test result.

Please mark this as checkin-needed when you test this on the try server. Thanks!

Pushed to mozilla-inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/51e532aa7c1d