1205181 - Remove Deprecated.jsm import from TelemetryStorage.jsm

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Georg Fritzsche [:gfritzsche]]

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/

Comment 1

[debkanya]

Hi, I would like to work on this bug. Is anyone working on it?

Comment 2

[Georg Fritzsche [:gfritzsche]]

No-one yet, assigning it to you.

Comment 3

[Sanchit Nevgi]

Hi Georg, Is the bug assigned or is it still open? Can I try fixing it?

Comment 4

[Georg Fritzsche [:gfritzsche]]

Hi Sanchit, nothing happened here in a while, so you can go ahead and try to fix it!

Comment 5

[Sanchit Nevgi]

(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?

Comment 6

[Sanchit Nevgi]

Attachment: Removed Deprecated module import

Comment 7

[Georg Fritzsche [:gfritzsche]]

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.

Comment 8

[Georg Fritzsche [:gfritzsche]]

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.

Comment 9

[Sanchit Nevgi]

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?

Comment 10

[Sanchit Nevgi]

Attachment: bug1205181.patch

Comment 11

[Georg Fritzsche [: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.

Comment 12

[Sanchit Nevgi]

Attachment: bug1205181.patch

Comment 13

[Sanchit Nevgi]

I have updated the patch. Please review and tell me if any changes are needed :)

Comment 14

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8673797 [details] [diff] [review]
bug1205181.patch

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

This looks fine, thank you.

Comment 15

[Georg Fritzsche [:gfritzsche]]

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.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/1c1433d29427

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1c1433d29427