Closed Bug 1205181 Opened 9 years ago Closed 9 years ago

Remove Deprecated.jsm import from TelemetryStorage.jsm

Categories

(Toolkit :: Telemetry, defect)

defect
Not set
normal
Points:
1

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox43 --- affected
firefox44 --- fixed

People

(Reporter: gfritzsche, Assigned: sanchit.nevgi, Mentored)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 2 obsolete files)

Deprecated.jsm is (lazily) imported into TelemetryStorage.jsm, but not actually used in this module:
https://dxr.mozilla.org/mozilla-central/rev/9ed17db42e3e46f1c712e4dffd62d54e915e0fac/toolkit/components/telemetry/TelemetryStorage.jsm#24

We should just remove this import.
To confirm that the change does not break anything, we can run the Telemetry unit tests locally:
mach xpcshell-test toolkit/components/telemetry/tests/unit/
Hi, I would like to work on this bug. Is anyone working on it?
No-one yet, assigning it to you.
Assignee: nobody → rimjhim.mazumder17
Hi Georg, Is the bug assigned or is it still open? Can I try fixing it?
Assignee: rimjhim.mazumder17 → nobody
Hi Sanchit, nothing happened here in a while, so you can go ahead and try to fix it!

(In reply to Georg Fritzsche [:gfritzsche] from comment #4)
> Hi Sanchit, nothing happened here in a while, so you can go ahead and try to
> fix it!

Thanks Georg! As you mentioned I removed the Deprecated module import and ran the unit tests. All of them have passed(So I guess it's not being used). Do I need to make any more changes?
Attached patch Removed Deprecated module import (obsolete) — Splinter Review
Thanks for the patch Sanchit!
Once you have written a patch, you can request feedback or review on it as described here:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_reviewed
That makes it turn up as a request for me in Bugzilla here so i don't miss it somehow.
I'll set that here now.
Attachment #8673214 - Flags: review?(gfritzsche)
Assignee: nobody → sanchit.nevgi
Status: NEW → ASSIGNED
Comment on attachment 8673214 [details] [diff] [review]
Removed Deprecated module import

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

Thanks Sanchit, this looks like everything that has to happen!

The next step would be to update this patch with a proper commit message, see here: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions
Then we can get this landed.

Usually we would push this to our "try server" first (which runs our automated tests on multiple platforms etc.) to see it doesn't break anything. However this change is trivial and fine on my machine, so we can skip that.
Attachment #8673214 - Flags: review?(gfritzsche) → review+
Thanks for your help Georg! I've made a new patch(using hg qnew) with the commit message "Removed Deprecated.jsm import from TelemetryStorage.js". What should be my next step?
Attached patch bug1205181.patch (obsolete) — Splinter Review
Attachment #8673214 - Attachment is obsolete: true
Attachment #8673778 - Flags: review?(gfritzsche)
Comment on attachment 8673778 [details] [diff] [review]
bug1205181.patch

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

Thanks! Two small details:
* its "TelemetryStorage.jsm" instead of .js
* the commit message should end with a note on the reviewer, e.g. "Bug 1205181 - Blah Blah. r=gfritzsche"

Can you upload the patch with those changes? Then i will see that it gets landed.
Attachment #8673778 - Flags: review?(gfritzsche) → feedback+
Attached patch bug1205181.patchSplinter Review
Attachment #8673778 - Attachment is obsolete: true
Attachment #8673797 - Flags: review?(gfritzsche)
I have updated the patch. Please review and tell me if any changes are needed :)
Comment on attachment 8673797 [details] [diff] [review]
bug1205181.patch

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

This looks fine, thank you.
Attachment #8673797 - Flags: review?(gfritzsche) → review+
This is a change without any real code impact and passes tests fine on my machine, so we can skip pushing it to try.
Setting "checkin-needed" so it can be landed by people with the proper access.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1c1433d29427
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.