Closed Bug 1370489 Opened 7 years ago Closed 7 years ago

Factor TelemetryIOInterposeObserver out of Telemetry.cpp

Categories

(Toolkit :: Telemetry, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: Dexter, Assigned: kalpa, Mentored)

References

Details

(Whiteboard: [measurement:client][lang=c++])

Attachments

(1 file, 2 obsolete files)

With bug 1335461 landing, we can finally factor some of the remaining classes out of Telemetry.cpp.

This bug is about moving TelemetryIOInterposeObserver out of Telemetry.cpp.
Priority: -- → P3
Whiteboard: [measurement:client][lang=c++]
Depends on: 1335461
Assignee: nobody → avikalpakundu
Attached patch 1370489 (obsolete) — Splinter Review
I have refactored it as specified. 

However, I'm presented with the following errors.

> Undefined symbols for architecture x86_64:
>   "mozilla::Telemetry::TelemetryIOInterposeObserver::ReflectIntoJS(JSContext*, JS::Handle<JSObject*>)", referenced from:
>       (anonymous namespace)::TelemetryImpl::GetFileIOReports(JSContext*, JS::MutableHandle<JS::Value>) in Telemetry.o
>   "mozilla::Telemetry::TelemetryIOInterposeObserver::AddPath(nsAString const&, nsAString const&)", referenced from:
>       mozilla::Telemetry::SetProfileDir(nsIFile*) in Telemetry.o
>   "mozilla::Telemetry::TelemetryIOInterposeObserver::TelemetryIOInterposeObserver(nsIFile*)", referenced from:
>       mozilla::Telemetry::InitIOReporting(nsIFile*) in Telemetry.o
> ld: symbol(s) not found for architecture x86_64
> clang: error: linker command failed with exit code 1 (use -v to see invocation)

Would it be possible for you to tell me what am I doing wrong?
Flags: needinfo?(gfritzsche)
Attachment #8876922 - Attachment is patch: true
Flags: needinfo?(gfritzsche)
Comment on attachment 8876922 [details] [diff] [review]
1370489

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

This is what we want, thanks.

The build issue is because we need to make the build system aware of the new source file here:
https://dxr.mozilla.org/mozilla-central/rev/2a63a6c35033b5cbc6c98cabc7657c7290284691/toolkit/components/telemetry/moz.build#55
Otherwise it won't be included in the build, which is why you see the linking errors.

I commented on some high-level issues below, but there is nothing critical.
Once this works you can flag me for review on the updated patch, then i can take a look at any remaining details.

::: toolkit/components/telemetry/TelemetryIOInterposeObserver.h
@@ +19,5 @@
> +#include "jsapi.h"
> +#include "nsTHashtable.h"
> +#include "nsHashKeys.h"
> +#include "nsBaseHashtable.h"
> +#include "nsClassHashtable.h"

Are all the includes here actually needed?

@@ +26,5 @@
> +
> +namespace mozilla {
> +namespace Telemetry {
> +
> +using namespace mozilla;

We should usually not have any using statements in header files.
This is because everybody that includes this header file will get their effects, which can be confusing or have unintended side-effects.

@@ +90,5 @@
> +  /**
> +   * Get size of hash table with file stats
> +   */
> +  size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const {
> +    return aMallocSizeOf(this) + SizeOfExcludingThis(aMallocSizeOf);

Lets move this implementation into the .cpp file.

@@ +94,5 @@
> +    return aMallocSizeOf(this) + SizeOfExcludingThis(aMallocSizeOf);
> +  }
> +
> +  size_t SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) const {
> +    size_t size = 0;

Same here.
Attachment #8876922 - Flags: feedback+
Attached patch 1370489 (obsolete) — Splinter Review
Attachment #8876922 - Attachment is obsolete: true
Attachment #8878272 - Flags: review?(gfritzsche)
Attachment #8878272 - Attachment is patch: true
Comment on attachment 8878272 [details] [diff] [review]
1370489

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

Thanks, this looks good me!

I pushed it to try to see if it successfully builds in different configurations:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b2b247abeae45fb87efa33216d30776183bcbe7

If that looks fine we can land it.
Attachment #8878272 - Flags: review?(gfritzsche) → review+
ni? me for rebasing this after bug 1366294 lands.
Flags: needinfo?(alessio.placitelli)
Attached patch Rebased patchSplinter Review
I've only rebased the patch.
Attachment #8878272 - Attachment is obsolete: true
Flags: needinfo?(alessio.placitelli)
Attachment #8890776 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/48ca21aa3f8a
Factor out TelemetryIOInterposeObserver from Telemetry.cpp. r=gfritzsche
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/48ca21aa3f8a
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.