Closed Bug 1452207 Opened 6 years ago Closed 6 years ago

Add a comment to browser/app/permissions that Data Collection Review is needed for additional hc_telemetry domains

Categories

(Toolkit :: Telemetry, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: chutten, Assigned: is2ei, Mentored)

References

Details

(Whiteboard: [good first bug] [lang=c++])

Attachments

(1 file, 2 obsolete files)

To make it clear to Data Stewards and others coming across hc_telemetry permission additions, we should note _inside_ the permissions file that Data Collection Review is needed.

There is already a link to the process in the file, but it's possible a reviewer may not follow it.
To help Mozilla out with this bug, here's the steps:

0) Comment here on the bug that you want to volunteer to help. I (or someone else) will assign it to you.
1) Download and build the Firefox source code: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build
- If you have any problems, please ask on IRC (https://wiki.mozilla.org/Irc) in the #introduction channel. They're there to help you get started.
- You can also read the Developer Guide, which has answers to most development questions: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction
2) Start working on this bug. The work here is simple, you just need to add a comment like "Adding hc_telemetry permission to a new domain requires Data Collection Review: https://wiki.mozilla.org/Firefox/Data_Collection" to the file browser/app/permissions
- If you have any problems with this bug, please comment on this bug and set the needinfo flag for me. Also, you can find me and my teammates on the #telemetry channel on IRC (https://wiki.mozilla.org/Irc) most hours of most days.
3) Build your change with `mach build` and test your change with `mach test toolkit/components/telemetry/tests/`. Also check your changes for adherence to our style guidelines by using `mach lint`
4) Submit the patch (including an automated test, if applicable) for review. Mark me as a reviewer so I'll get an email to come look at your code.
- Here's the guide: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch
5) After a series of reviews and changes to your patch, I'll mark it for checkin or push it to autoland. Your code will soon be shipping to Firefox users worldwide!
6) ...now you get to think about what kind of bug you'd like to work on next. Let me know what you're interested in and I can help you find your next contribution.
Mentor: chutten
Priority: -- → P3
Whiteboard: [good first bug] [lang=c++]
Hi, I'm Issei. I would like to work on this issue.
Could you assign it to me?
Flags: needinfo?(chutten)
Attached patch bug-1452207.patch (obsolete) — Splinter Review
I created a patch. Could you check it, please?
Attachment #8968033 - Flags: review?(chutten)
Thank you for your contribution! I have assigned the bug to you and will proceed with reviewing your patch.
Assignee: nobody → is2ei.horie
Status: NEW → ASSIGNED
Flags: needinfo?(chutten)
Comment on attachment 8968033 [details] [diff] [review]
bug-1452207.patch

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

Hi! I have a small comment about comment placement and then your change should be good to go.

It is missing a few pieces, though. It appears as though this is just a diff and not a full patch. Could you provide a nice commit message (there are hints in Instruction #4) and submit the patch file? (you appear to be using git, so to make a patch file you can use `git format-patch -1`)

::: browser/app/permissions
@@ +1,2 @@
>  # This file has default permissions for the permission manager.
> +# Adding hc_telemetry permission to a new domain requires Data Collection Review: https://wiki.mozilla.org/Firefox/Data_Collection

This comment might be easier to find if you place it closer to where the permission would be added towards the bottom. Just under the section heading here[1] might be best.

[1]: https://searchfox.org/mozilla-central/rev/bd326a2a6b729dc62a5aee57354a97ceac4d1dc0/browser/app/permissions#26
Attachment #8968033 - Flags: review?(chutten)
Attached patch bug-1452207.patch (obsolete) — Splinter Review
Thanks a lot! I updated the patch.
- I used "hg export" command to create the patch this time.
- I moved the comment to the bottom section.

Could you check it, please? ;)
Attachment #8968719 - Flags: review?(chutten)
Comment on attachment 8968719 [details] [diff] [review]
bug-1452207.patch

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

This is excellent. We need just one last thing and then we can mark this for checkin.

The commit message needs to have an "r=chutten" at the end of the first line to signify that I have reviewed it. That way the sheriffs know who to come asking questions of if something goes awry :)

Put that at the end and re-upload it here and I'll mark it for checkin. Then you can start thinking about what you might want to work on next!
Attachment #8968719 - Flags: review?(chutten) → review+
Thanks! I added "r=chutten" to the commit message.
Attachment #8968033 - Attachment is obsolete: true
Attachment #8968719 - Attachment is obsolete: true
Attachment #8969367 - Flags: review?(chutten)
Comment on attachment 8969367 [details] [diff] [review]
bug-1452207.patch

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

Great, thanks!
Attachment #8969367 - Flags: review?(chutten) → review+
I have marked this for checkin.

What would you like to do next? Do you already have something in mind?
Keywords: checkin-needed
I`m receiving the following error when applying the patch. 
Can you please take a look?

-------
applying bug-1452207.patch
patching file browser/app/permissions
Hunk #1 FAILED at 23
1 out of 1 hunks FAILED -- saving rejects to file browser/app/permissions.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug-1452207.patch
-------
Flags: needinfo?(chutten)
I'm afraid I have no idea why the patch would not apply. It is a very little patch that has nothing in its context that isn't in the file in the tip of tree.

Maybe there was another patch in the list you were applying that conflicted?
Flags: needinfo?(chutten)
I`ve checked again on two different PC`s and get the same error.
Sebastian, can you help us?
Flags: needinfo?(aryx.bugmail)
Keywords: checkin-needed
The `permissions` file in-tree contains CRLF line endings ("\r\n"), you can see that in [1] denoted by the small character ↩.
The patch only has LF line endings ("\n"), which confuses all tools.

[1]: https://searchfox.org/mozilla-central/source/browser/app/permissions

We should maybe convert that whole file to LF line endings.
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/06de003da627
Add a comment to browser/app/permissions that Data Collection Review is needed for additional hc_telemetry domains r=chutten DONTBUILD
Thanks! I have a question.

Do I need to change the character from CRLF to LF in the 'permissions' file in this patch or can it be another patch?
(In reply to Issei Horie from comment #16)
> Thanks! I have a question.
> 
> Do I need to change the character from CRLF to LF in the 'permissions' file
> in this patch or can it be another patch?

Looks like Aryx did it for us: https://hg.mozilla.org/integration/mozilla-inbound/rev/06de003da627

What would you like to do next?
Ok. Thanks for helping me! 

> What would you like to do next?

I'm not sure now... Is there any recommendation?
Next time, I'd like to work more than just adding a comment ;)
https://hg.mozilla.org/mozilla-central/rev/06de003da627
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.