1221505 - Remove osfile.jsm dependency from TelemetrySession.jsm

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[Alessio Placitelli [:Dexter]]

With bug 1190801 landing, TelemetrySession.jsm doesn't need to depend on osfile.jsm anymore.

In order to remove the dependency, we should:

1) Remove the FILE_PATH constant from _saveAbortedSessionPing in [0]
2) Drop FILE_PATH from the comment in [1]
3) Remove the osfile.jsm import in [2]

To test that everything works correctly after the changes, run the xpcshell tests with the following command:

./mach xpcshell-test toolkit/components/telemetry

[0] - https://dxr.mozilla.org/mozilla-central/rev/f742b9412ed5aace90ad863b276faae0641090a8/toolkit/components/telemetry/TelemetrySession.jsm#1975
[1] - https://dxr.mozilla.org/mozilla-central/rev/f742b9412ed5aace90ad863b276faae0641090a8/toolkit/components/telemetry/TelemetrySession.jsm#1977
[2] - https://dxr.mozilla.org/mozilla-central/rev/f742b9412ed5aace90ad863b276faae0641090a8/toolkit/components/telemetry/TelemetrySession.jsm#15

Comment 1

[simplyblue24]

Hi, I like to work on this bug,  can you please assign the bug to me. Thanks :)

Comment 2

[Alessio Placitelli [:Dexter]]

Hi! I've assigned the bug to you. Please let me know if you have any doubts or questions about how to proceed!

Comment 3

[simplyblue24]

Attachment: 1221505.diff

Hi,  I have attached the diff file of the changes that are specified.As you told i ran |./mach xpcshell-test toolkit/components/telemetry| to check if everything is looking good. But 6/234 tests failed. Can you point me in right direction on solving this.

Comment 4

[Alessio Placitelli [:Dexter]]

Thank you! You should submit a patch file rather than a diff. You can generate it by following this guide: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

That said, your changes look ok, but bug 1190801 took a bit longer and has not been merged yet (this will probably be done today).

So, if you create a patch using that guide, than pop it off ("hg qpop -a"), update the repository tomorrow/later today ("hg pull -u"), push the patch back ("hg qpush -a") and then build again, all the tests will probably pass.

If you have any problem, please post again here or get in touch over IRC in #introduction!

Comment 5

[Alessio Placitelli [:Dexter]]

Hi there! Do you have any problem with the bug and need help?

Comment 6

[simplyblue24]

Attachment: bug1221505.patch

Hi,I have made the appropriate changes, and ran the tests.Everything seems ok. can you please review my patch. :)

Comment 7

[Alessio Placitelli [:Dexter]]

Comment on attachment 8688405 [details] [diff] [review]
bug1221505.patch

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

This looks good with the suggestion below addressed!

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +1838,5 @@
>     *                 session ping. The reason of this payload is changed to aborted-session.
>     *                 If not provided, a new payload is gathered.
>     */
>    _saveAbortedSessionPing: function(aProvidedPayload = null) {
> +    this._log.trace("_saveAbortedSessionPing - ping path: ");

Since you removed FILE_PATH, you should also remove the "- ping path: " part from the log line, as the path is no longer there ;)

Comment 8

[simplyblue24]

Attachment: bug1221505.patch

Hi, I have made the changes as you suggested. :)  I think this looks good now

Comment 9

[Alessio Placitelli [:Dexter]]

Comment on attachment 8688407 [details] [diff] [review]
bug1221505.patch

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

This looks good now, thanks. Did you make sure there are no more errors locally when running the tests?

Comment 10

[simplyblue24]

Yes, I ran the tests locally, and the tests ran fine, with no errors.

Comment 11

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d45ae8db8eb8

Comment 12

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/fx-team/rev/929982f95a788e798b848da372725dd80cc44b36
Bug 1221505 - Remove osfile.jsm dependency from TelemetrySession.jsm. r=dexter

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/929982f95a78