Bug 1484611 moved the location of a few Telemetry files within their source directory. As a consequence of that, includes in the implementation files are now unsorted. This bug is about fixing the style of the includes, making them sorted again.

In order to fix this: - go through all the .cpp/.h files in toolkit/components/telemetry (+ subdirectories); - sort all the #include directives according to the logic at [1]; - build using ./mach build - make sure all the tests pass with ./mach test toolkit/components/telemetry [1] - https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC_practices

can I take up this bug. This would be my second!

(In reply to Divyansh Sharma [:spiro] from comment #2) > can I take up this bug. This would be my second! Hi, unfortunately the blocker bug has not landed, so this bug is not actionable yet. It will be in a couple of days or so.

(In reply to Alessio Placitelli [:Dexter] from comment #3) > (In reply to Divyansh Sharma [:spiro] from comment #2) > > can I take up this bug. This would be my second! > > Hi, unfortunately the blocker bug has not landed, so this bug is not > actionable yet. It will be in a couple of days or so. Hi :spiro, the dependency just landed so this is good to work on now :)

Do I have to download latest code of firefox for build?

(In reply to Divyansh Sharma [:spiro] from comment #5) > Do I have to download latest code of firefox for build? Yes, you do.

Hi :Dexter, :spiro, There are almost 20 .h and 20.cpp files. Can I do some part (say .h one)? PS: I am newbie here and looking to fix first bug :)

(In reply to Anushi Maheshwari[:Anushi1998] from comment #7) > Hi :Dexter, :spiro, There are almost 20 .h and 20.cpp files. Can I do some > part (say .h one)? > PS: I am newbie here and looking to fix first bug :) Hi :Anushi1998, while these might seem a lot of files, not all of them require re-ordering. Moreover, we want all the changes to be in a single changeset, so this doesn't naturally fit into splitting. Would you consider taking bug 1473603 instead?

:Dexter Sure, thanks :)

no activity for 6 days, can I take this bug or is someone working?

:gurungrahul: I have done half work and was waiting for Dexter to unassign. Will it be good if I take this? :)

sure.

Divyansh, how is this progressing? Are you blocked and need help?

I am almost finished. Will submit a patch today or latest by tomorrow.

Alessio I am unable to run tests. It is my error log : https://pastebin.com/W3xRwGq6. How to resolve this?

:spiro Looks like this is unrelated to your changes. I have the same error report :) It may be the case that command to run should be different or maybe some bug ;)

Bug 1487332 - Re-sorted the header files of .cpp/.h telemetry files.

(In reply to Divyansh Sharma [:spiro] from comment #15) > Alessio > > I am unable to run tests. It is my error log : > https://pastebin.com/W3xRwGq6. How to resolve this? Hey :spiro, this looks unrelated :)

Dexter Is 'nspr' the only change that is needed now? If not, then how to determine where does 'include' look for files?

(In reply to Divyansh Sharma [:spiro] from comment #19) > Dexter > > Is 'nspr' the only change that is needed now? If not, then how to determine > where does 'include' look for files? No, that was an example: we don't need to change any path in this patch. We need to swap the position of include files so that are in order, like described in comment 1.

Dexter You are saying that we need to re-order include files in alphabetical order, right?

(In reply to Divyansh Sharma [:spiro] from comment #21) > Dexter > > You are saying that we need to re-order include files in alphabetical order, > right? Almost. I'm saying that it needs to be both alphabetically sorted AND following the criteria from https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC_practices , as discussed in comment 1.

Dexter I am unable to run test files. I am encountering some errors. Error log : https://pastebin.com/vTHGCB6x Please help!

(In reply to Divyansh Sharma [:spiro] from comment #23) > Dexter > > I am unable to run test files. I am encountering some errors. > > Error log : https://pastebin.com/vTHGCB6x > > Please help! Hey, this doesn't look related. Unfortunately I can't be sure without taking a look at the patch :) Would you mind uploading the most recent one?

Dexter In some files, I needed to include telemetryCommon.h [or some similar files] above all the include files, otherwise stdint.h would not have been included and uint64_t would not be recognised. I am uploading the latest diff.

Dexter Please ignore current diff. It has got some extra files included. I will separate them and update the diff.

(In reply to Divyansh Sharma [:spiro] from comment #26) > Dexter > > Please ignore current diff. It has got some extra files included. I will > separate them and update the diff. I took a look anyway and left a few comments :)

In some files, I need to place Telemetry.h before main header file, else it throws error about some undeclared variables. Like: 22:39.43 In file included from /home/inspiro/src/mercurial/toolkit/components/telemetry/core/TelemetryEvent.cpp:7: 22:39.43 /home/inspiro/src/mercurial/toolkit/components/telemetry/core/TelemetryEvent.h:33:1: error: unknown type name 'nsresult' 22:39.43 nsresult RecordEvent(const nsACString& aCategory, const nsACString& aMethod, 22:39.43 ^ 22:39.43 /home/inspiro/src/mercurial/toolkit/components/telemetry/core/TelemetryEvent.h:33:28: error: unknown type name 'nsACString' 22:39.43 nsresult RecordEvent(const nsACString& aCategory, const nsACString& aMethod,

Bug 1487332 - Resorted the include files as per coding standards

Alessio I have created a new diff with asked changes. But it has some issues for you to consider: 1) File 'TelemetryEvent.cpp' has include 'Telemetry.h' before 'TelemetryEvent.h', else it throws error described in earlier comment 28. 2) Some tests are failing now too, as per comment 23. Have a look into this. Thanks :)

(In reply to Divyansh Sharma [:spiro] from comment #30) > Alessio > > I have created a new diff with asked changes. But it has some issues for you > to consider: Mh, can you update the previous one? Creating a request makes it hard to track down the requested changes. Updating the original patch simplifies things quite a lot. > > 1) File 'TelemetryEvent.cpp' has include 'Telemetry.h' before > 'TelemetryEvent.h', else it throws error described in earlier comment 28. Where is this happening? Can you comment on phabricator? > 2) Some tests are failing now too, as per comment 23. This still looks unrelated to me.

Hi, We received the following error when trying to land this patch: With Diff 22167 on Tue, October 9, 2018, 4:06 PM GMT+3, by rvandermeulen@mozilla.com. Error: hg error in cmd: hg push -r tip upstream: pushing to ssh://hg.mozilla.org/integration/autoland searching for changes remote: adding changesets remote: adding manifests remote: adding file changes remote: added 1 changesets with 26 changes to 26 files remote: remote: remote: ************************** ERROR **************************** remote: Rev 9d3da511671f contains git-format-patch "[PATCH]" cruft. Use git-format-patch -k to avoid this. remote: divyansh <sharma.divyansh.501@iitg.ernet.in> remote: Bug 1487332 [PATCH] r=Dexter remote: remote: Bug 1487332 - Re-sorted the header files of .cpp/.h telemetry files. remote: remote: Differential Revision: https://phabricator.services.mozilla.com/D5840 remote: ************************************************************* remote: remote: remote: transaction abort! remote: rollback completed remote: pretxnchangegroup.c_commitmessage hook failed abort: push failed on remote spiro: Can you please have a look at this and update the patch format? Thanks!

Pushed by ebalazs@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/fd465ab13d0f Re-sorted the header files of .cpp/.h telemetry files. r=Dexter