Factor TelemetryIOInterposeObserver out of Telemetry.cpp

RESOLVED FIXED in Firefox 56

Status

()

P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Dexter, Assigned: kalpa, Mentored)

Tracking

Trunk
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

a year ago
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.
(Reporter)

Updated

a year ago
Priority: -- → P3
Whiteboard: [measurement:client][lang=c++]
Depends on: 1335461
(Assignee)

Updated

a year ago
Assignee: nobody → avikalpakundu
(Assignee)

Comment 2

a year ago
Created attachment 8876922 [details] [diff] [review]
1370489

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+
(Assignee)

Comment 4

a year ago
Created attachment 8878272 [details] [diff] [review]
1370489
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+
(Reporter)

Comment 6

a year ago
ni? me for rebasing this after bug 1366294 lands.
Flags: needinfo?(alessio.placitelli)
(Reporter)

Comment 7

a year ago
Created attachment 8890776 [details] [diff] [review]
Rebased patch

I've only rebased the patch.
Attachment #8878272 - Attachment is obsolete: true
Flags: needinfo?(alessio.placitelli)
(Reporter)

Updated

a year ago
Attachment #8890776 - Flags: review+
(Reporter)

Updated

a year ago
Keywords: checkin-needed

Comment 9

a year ago
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

Comment 10

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/48ca21aa3f8a
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.