4.83 KB, text/plain
50 bytes, text/x-github-pull-request
|Details | Review|
51 bytes, text/x-github-pull-request
|Details | Review|
98.44 KB, application/x-xpinstall
Repo is here: https://github.com/mozilla/FastBlockShield
[Tracking Requested - why for this release]:
Peer Review Assignment
Hey Dave, can you look at or assign a peer to review this study? Thanks.
Flags: needinfo?(nhnt11) → needinfo?(dtownsend)
Looks like Nihanth was already assigned here.
Flags: needinfo?(dtownsend) → needinfo?(nhnt11)
Chris, if you are unable to do this data review next week, let me know and I'll find someone else who can. This is for an opt-out Shield study that will launch on 10 Sep. I believe that all of the data is category 1 or 2. Everything is documented publicly on the product hypothesis document: https://docs.google.com/document/d/1LQdOFIZeoiD38NNMxvpm32bX6Urnv0KbEsGfgAuw5L8/edit#heading=h.mn1ovnang5jv
Attachment #9005756 - Flags: review?(chutten)
Attachment #9005857 - Flags: review?(nhnt11)
Comment on attachment 9005756 [details] fastblock-data-review-request.txt DATA COLLECTION REVIEW RESPONSE: Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate? Yes, see TELEMETRY.md Is there a control mechanism that allows the user to turn the data collection on and off? Yes, standard Shield Study mechanisms apply. If the request is for permanent data collection, is there someone who will monitor the data over time? N/A timeboxed study for two weeks. Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under? I'm pretty sure hashed ETLD+1 counts as Category 3, even with a per-participant salt. It is equivalent to a monotonically-increasing id per etld+1 unique per profile which would still classify it as Cat3 as you're capturing data per etld+1. Ultimately, since this study is on a pre-release population with a well-understood opt-out Cat2/3 doesn't affect the review result, but I think it's important to call out that we are collecting data per etld+1 and that is stronger than just user interaction with the browser. Is the data collection request for default-on or default-off? Default-on for study participants. Does the instrumentation include the addition of any new identifiers (whether anonymous or otherwise; e.g., username, random IDs, etc. See the appendix for more details)? Yes: the aforementioned etld+1 hash. It is designed to not be linkable to anything outside of this study. Is the data collection covered by the existing Firefox privacy notice? Yes. Does there need to be a check-in in the future to determine whether to renew the data? No, Shield cleans up after itself and renewal involves starting a new study. --- Result: datareview+
Attachment #9005756 - Flags: review?(chutten) → review+
attached zip file for signing please
Comment on attachment 9005857 [details] [review] GitHub Pull Request Review of attachment 9005857 [details] [review]: ----------------------------------------------------------------- r+ for the Shield study from a code-sanity point of view. I did not look too hard at the overall repository; I focused on the code that's shipping to users. Please let me know if there's further code that's pushed to the repo that you'd like me to look at.
Comment on attachment 9005857 [details] [review] GitHub Pull Request I set r+ from the Splinter review UI, I think it automatically changed the mime-type. Reverting.
UX Review Request
(In reply to Tony Cinotto [:tcinotto] (UTC-8) from comment #15) > UX Review Request UX looks good.
zip file for signing please
Hi Nihanth, We've added an additional branch to the experiment, could you kindly re-review and provide a peer review sign-off prior to our next Fastblock check in? (September 13th). Thank you.
Please note we are now aiming to launch this shield study on September 17th. Thank you.
Can we get this one signed as well? :) Thanks!
Nihanth, this is the PR with the diff between the code you last reviewed and current master, it's mostly configuration changes. (We need a procedure for when the code in the shield study was reviewed by a peer on GitHub already)
Assignee: nobody → jhofmann
Attachment #9008086 - Flags: review?(nhnt11)
Comment on attachment 9008086 [details] [review] GitHub Pull Request v2 Review of attachment 9008086 [details] [review]: ----------------------------------------------------------------- Looks ok, thanks. To be clear I'm not really looking at the config stuff, just the actual extension source code.
10 months ago
We would need another signature :) Thanks!
For signing please
Hi Nihanth, Could we trouble you for a new peer review as we have a new signed build? Thank you for your help!
Ah, sorry, that was the wrong study. Here's the FastBlock study. Thanks!
Comment on attachment 9009073 [details] mozilla_fastblock_user_study-1.0.4.zip Making another minor change...
Attachment #9009073 - Attachment is obsolete: true
Science review: R+
final zip for signing please
Attachment #9006933 - Attachment is obsolete: true
Attachment #9006950 - Attachment is obsolete: true
Attachment #9007797 - Attachment is obsolete: true
Attachment #9007820 - Attachment is obsolete: true
Attachment #9008084 - Attachment is obsolete: true
Attachment #9008090 - Attachment is obsolete: true
Attachment #9008513 - Attachment is obsolete: true
Attachment #9008516 - Attachment is obsolete: true
Attachment #9008880 - Attachment is obsolete: true
Attachment #9008881 - Attachment is obsolete: true
Another study for signing, please...
Attachment #9009258 - Attachment is obsolete: true
Attachment #9009654 - Attachment is obsolete: true
Attachment #9009261 - Attachment is obsolete: true
Attachment #9009599 - Attachment is obsolete: true
FastBlock Targeted: Firefox Beta 63 We have finished testing the FastBlock experiment. All the reported issues were fixed and verified. QA’s recommendation: YELLOW - SHIP IT, CONDITIONALLY Reasoning: The shield study is looking ok overall and the lists are acting better after the latest fixes, however, the QA has one major concern. Although branches seem to block the trackers accordingly to the lists they are connected, we were only able to verify randomly a small number of them. This results in us not being 100% sure if all trackers will be blocked accordingly. Testing Summary: - Full Functional test suite: TestRail (https://goo.gl/aT9fjX) Tested Platforms: - Windows 7 x64 - Windows 10 x64 Tested Firefox versions: - Firefox Beta 63.0b4 - Firefox Beta 63.0b5 - Firefox Beta 63.0b6 - Firefox Beta Unbranded 63.0b6
Since Nihanth was a bit confused about what the latest changes are for review, let me clarify: I've kept the branch open that shows the changes between the original review and the latest master here: https://github.com/mozilla/FastBlockShield/pull/121 Sorry for not mentioning that earlier! (Note that since these changes were reviewed by me and they also went into Cookie Restrictions, I think we're generally in a good state). Thanks!
Based on the multiple rounds of testing that we've done and the fact that we've run much more aggressive default-on tracking protection studies in the past, from a product team perspective, I'm comfortable with the level of risk to ship the shield study to beta. Note: Francois is doing an extra validation of the lists (since he has greater familiarity with them), so we'd like him to give his okay as well before we move forward. (NI'ing him here)
Francois signed off on email. With that I can sign off too, on Pascal's (63 release owner) behalf.
From François' email: Francois Marier 1:46 PM (1 hour ago) to Ritu, Marnie, Peter, Carmen, Johann, Erica, Tony, Ehsan, ettseng, Tania, Wennie, Matthew, Gregg, release-signoff, experiments-qa I have completed the extra validation I wanted to do on version 1.0.6 of the Shield extension. This is good to go from my point of view. In terms of testing every tracker, that's not something we are planning to do as it would be a very large amount of work and likely wouldn't really tell us more than the random sampling that was done by SV. We have been comfortable shipping tracking protection without that level of verification in the past. I believe we can do the same here. Since FastBlock is a performance feature, as opposed to a privacy feature, trackers getting through is not a major concern. If we were to find a tracker on the list that's not blocked as it should, then all that would mean is that the feature wouldn't give us as much as a performance improvement as it could. Francois
I reviewed the changes that Johann linked, consider the previous r+ carried-forward.
We're live here :)
Marking the 63 status as fixed as it went live to beta 3 weeks ago.
Recipe disabled - (Delivery console #584).
Status: NEW → RESOLVED
Closed: 9 days ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.