Closed Bug 1441892 Opened 6 years ago Closed 6 years ago

Remove TelemetryController.savePing

Categories

(Toolkit :: Telemetry, defect, P3)

Product:
Toolkit
Component:
Telemetry
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: chutten, Assigned: akriti.v10, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good first bug] [lang=js])

Attachments

(1 file, 6 obsolete files)

Bug 1441892 : TelemetryController.savePing removed ,
59 bytes, text/x-review-board-request
chutten
: review+
Details 
When looking for untested code I found TelemetryController.savePing which is not only untested, but unused.

Let's get rid of 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. You'll be editing toolkit/components/telemetry/TelemetryController.jsm to remove the savePing function (and the Impl.savePing implementation, and any storage used only by those functions).
- 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 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.
Blocks: 1201022
I am interested to work on this bug.
I would like to take this as my first bug
I would like to take this as my first bug
Let me know if you have any questions, Souvik!
Assignee: nobody → somondal96
Status: NEW → ASSIGNED
Chris, I am finding some problem building Firefox in Ubuntu. Seems some files are "not found" when running bootstrap.py.Can you help me? Link for terminal result :https://drive.google.com/file/d/1kUZCbF10INNJ-bjKGwyc5w-KEHqq8q6H/view?usp=sharing
It is attempting to install the build software (and some utilities to help with tests, too, if I'm not mistaken) using the line 

sudo apt-get install autoconf2.13 build-essential ccache nodejs python-dev python-pip python-setuptools unzip uuid zip

Your ubuntu system's `apt` is trying to install these things but isn't able to download them. I'm not sure why, unfortunately. Can you ask on IRC on the channel #introduction? There will be people there who may have seen something like this before. (IRC information: https://wiki.mozilla.org/Irc)

Or maybe running that command yourself will give you some more useful error messages (beyond just 404s). Or maybe it was an intermittent error and it will all work if you try again?
Did you manage to puzzle out getting Firefox to build on your Ubuntu machine? Anything I can help with?
Flags: needinfo?(somondal96)
Hi Chris, can i try this bug?
I think we should give Souvik another couple of days to reply first.
Okay, akriti, are you still interested in this one?
Assignee: somondal96 → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(akriti.v10)
Yes , i will work on it,
Thanks.
Flags: needinfo?(akriti.v10)
I am trying to upload a patch , but i am not able to push for review because it says my API key is not configured. How can i configure it?
Looks like you've figured it out. Excellent!

A couple of notes before I look into these:

1) please edit your commit history so that only the one patch you want me to review is being uploaded. I think the command to do that is `hg histedit` but I'm not sure. (I use git through an adaptor)

2) The syntax mozreview understands for marking me as a reviewer is something like "r?chutten". While you're editing your history, you may wish to reword that commit message so I receive an email letting me know my review is requested.
Flags: needinfo?(somondal96)
Comment on attachment 8959974 [details]
Bug 1441892: TelemetryController.savePing function removed , r:Chris H-C :chutten

https://reviewboard.mozilla.org/r/228726/#review234646

::: toolkit/components/telemetry/TelemetryController.jsm:265
(Diff revision 1)
>     */
>    removeAbortedSessionPing() {
>      return Impl.removeAbortedSessionPing();
>    },
>  
>    /**

Please remove the function comments as well.
Comment on attachment 8959974 [details]
Bug 1441892: TelemetryController.savePing function removed , r:Chris H-C :chutten

https://reviewboard.mozilla.org/r/228726/#review234648

::: toolkit/components/telemetry/TelemetryController.jsm
(Diff revision 1)
> -    let options = aOptions;
> -    options.addClientId = aOptions.addClientId || false;
> -    options.addEnvironment = aOptions.addEnvironment || false;
> -    options.overwrite = aOptions.overwrite || false;
> -
> -    return Impl.savePing(aType, aPayload, aFilePath, options);

Oh, you can also remove the impl's savePing as this was the only caller.
Attachment #8959974 - Attachment is obsolete: true
Attachment #8959975 - Attachment is obsolete: true
Attachment #8959976 - Attachment is obsolete: true
Attachment #8959977 - Attachment is obsolete: true
Attachment #8959978 - Attachment is obsolete: true
Assignee: nobody → akriti.v10
Status: NEW → ASSIGNED
Comment on attachment 8960972 [details]
Bug 1441892 : Remove TelemetryController.savePing ,

https://reviewboard.mozilla.org/r/229710/#review235536

Please also remove the Impl's savePing, as you have removed its only caller.
Attachment #8960972 - Flags: review?(chutten) → review-
Attachment #8960972 - Attachment is obsolete: true
Comment on attachment 8961436 [details]
Bug 1441892 : TelemetryController.savePing removed ,

https://reviewboard.mozilla.org/r/230186/#review235810

Looks good! How did you test this to make sure it didn't bother any adjacent code?
Attachment #8961436 - Flags: review?(chutten) → review+
(In reply to Chris H-C :chutten from comment #24)
> Comment on attachment 8961436 [details]
> Bug 1441892 : TelemetryController.savePing removed ,
> 
> https://reviewboard.mozilla.org/r/230186/#review235810
> 
> Looks good! How did you test this to make sure it didn't bother any adjacent
> code?

Yes Chris for testing this i ran the 'mach test toolkit/components/telemetry/tests/unit/test_TelemetryController.js' command which did not fail, so i assumed nothing is broken. Is there something else i must to do?
Thanks for your positive comments.
Nope, that's it! I have marked this for integration. It should land soon, and then it will ship in the next version of Firefox Nightly.

Thank you for your contribution! Do you have your next task lined up, or would you like some help choosing one?
Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8b9c47146c70
TelemetryController.savePing removed , r=chutten
https://hg.mozilla.org/mozilla-central/rev/8b9c47146c70
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Thanks Chris, and yes please help me find some good bugs to work on.
From my bugmail I see you may have already found some good next bugs to work on :)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: