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)
Toolkit
Telemetry
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)
946 bytes,
patch
|
chutten
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•6 years ago
|
||
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++]
Assignee | ||
Comment 2•6 years ago
|
||
Hi, I'm Issei. I would like to work on this issue. Could you assign it to me?
Flags: needinfo?(chutten)
Assignee | ||
Comment 3•6 years ago
|
||
I created a patch. Could you check it, please?
Attachment #8968033 -
Flags: review?(chutten)
Reporter | ||
Comment 4•6 years ago
|
||
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)
Reporter | ||
Comment 5•6 years ago
|
||
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)
Assignee | ||
Comment 6•6 years ago
|
||
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)
Reporter | ||
Comment 7•6 years ago
|
||
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+
Assignee | ||
Comment 8•6 years ago
|
||
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)
Reporter | ||
Comment 9•6 years ago
|
||
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+
Reporter | ||
Comment 10•6 years ago
|
||
I have marked this for checkin. What would you like to do next? Do you already have something in mind?
Keywords: checkin-needed
Comment 11•6 years ago
|
||
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)
Reporter | ||
Comment 12•6 years ago
|
||
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)
Comment 13•6 years ago
|
||
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
Comment 14•6 years ago
|
||
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.
Comment 15•6 years ago
|
||
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
Updated•6 years ago
|
Flags: needinfo?(aryx.bugmail)
Assignee | ||
Comment 16•6 years ago
|
||
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?
Reporter | ||
Comment 17•6 years ago
|
||
(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?
Assignee | ||
Comment 18•6 years ago
|
||
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 ;)
Comment 19•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/06de003da627
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment hidden (spam) |
You need to log in
before you can comment on or make changes to this bug.
Description
•