Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Add creationTime function to process #13542

Merged
merged 9 commits into from Aug 10, 2018
Merged

feat: Add creationTime function to process #13542

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e27592e
Add process creation time
husayn Jul 2, 2018
78b9aaf
Making docs clear for process creation time
husayn Jul 2, 2018
c4cdfc9
Address comments for process creation time
husayn Jul 29, 2018
14c452c
Add process info cc file
husayn Jul 29, 2018
81d2dbd
Merge branch 'master' into master
ckerr Aug 9, 2018
9025cef
fixing comments around documentation
husayn Aug 10, 2018
34bb5d4
Update doc for return val
husayn Aug 10, 2018
9c03832
Capitalize number in docs
husayn Aug 10, 2018
6deffff
chore: bump electron-typescript-definitions
MarshallOfSound Aug 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 12 additions & 0 deletions atom/common/api/atom_bindings.cc
View file
Open in desktop
Expand Up @@ -14,6 +14,7 @@
#include "atom/common/native_mate_converters/string16_converter.h"
#include "atom/common/node_includes.h"
#include "base/logging.h"
#include "base/process/process_info.h"
#include "base/process/process_metrics_iocounters.h"
#include "base/sys_info.h"
#include "native_mate/dictionary.h"
Expand Down Expand Up @@ -55,6 +56,7 @@ void AtomBindings::BindTo(v8::Isolate* isolate, v8::Local<v8::Object> process) {
dict.SetMethod("log", &Log);
dict.SetMethod("getHeapStatistics", &GetHeapStatistics);
dict.SetMethod("getProcessMemoryInfo", &GetProcessMemoryInfo);
dict.SetMethod("getCreationTime", &GetCreationTime);
dict.SetMethod("getSystemMemoryInfo", &GetSystemMemoryInfo);
dict.SetMethod("getCPUUsage", base::Bind(&AtomBindings::GetCPUUsage,
base::Unretained(this)));
Expand Down Expand Up @@ -175,6 +177,16 @@ v8::Local<v8::Value> AtomBindings::GetProcessMemoryInfo(v8::Isolate* isolate) {
return dict.GetHandle();
}

// static
v8::Local<v8::Value> AtomBindings::GetCreationTime(v8::Isolate* isolate) {
auto timeValue = base::CurrentProcessInfo::CreationTime();
if (timeValue.is_null()) {
return v8::Null(isolate);
}
double jsTime = timeValue.ToJsTime();
return v8::Number::New(isolate, jsTime);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we not do v8 conversions manually, native_mate will handle the conversions in a consistent way if you just return a cpp type.

I.e. Just make this return double

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MarshallOfSound

double doesn't work if we want to return an "empty value".
And we have to return null when the time cannot be obtained.

if (timeValue.is_null()) {
  return v8::Null(isolate);
}

Copy link
Contributor Author

@husayn husayn Aug 10, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MarshallOfSound can you please approve the changes if this looks good?

}

// static
v8::Local<v8::Value> AtomBindings::GetSystemMemoryInfo(v8::Isolate* isolate,
mate::Arguments* args) {
Expand Down
1 change: 1 addition & 0 deletions atom/common/api/atom_bindings.h
View file
Open in desktop
Expand Up @@ -37,6 +37,7 @@ class AtomBindings {
static void Hang();
static v8::Local<v8::Value> GetHeapStatistics(v8::Isolate* isolate);
static v8::Local<v8::Value> GetProcessMemoryInfo(v8::Isolate* isolate);
static v8::Local<v8::Value> GetCreationTime(v8::Isolate* isolate);
static v8::Local<v8::Value> GetSystemMemoryInfo(v8::Isolate* isolate,
mate::Arguments* args);
v8::Local<v8::Value> GetCPUUsage(v8::Isolate* isolate);
Expand Down
6 changes: 6 additions & 0 deletions docs/api/process.md
View file
Open in desktop
Expand Up @@ -98,6 +98,12 @@ The `process` object has the following methods:

Causes the main thread of the current process crash.

### `process.getCreationTime()`

Returns [`number`]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we return null sometimes, it should be documented )

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And btw there's no reasons to have square brackets there )

Indicates the creation time of the application.
The time is represented as number of milliseconds since epoch.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to document this as "Returns Menu | null" for typescript. Something like

Returns `Menu | null` - The number of milliseconds since epoch, or `null` if the information is unavailable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done @ckerr

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this comment got misplaced, no menu stuff here 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MarshallOfSound you're correct, that should've read Number | null. I was cribbing from another documentation line and didn't clean it up correctly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@husayn I don't know if it the capitalization is required, but all our other documentation instances use Number rather than number

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ckerr capitalized it.

### `process.getCPUUsage()`

Returns [`CPUUsage`](structures/cpu-usage.md)
Expand Down
8 changes: 8 additions & 0 deletions spec/api-process-spec.js
View file
Open in desktop
@@ -1,6 +1,14 @@
const {expect} = require('chai')
const assert = require('assert')

describe('process module', () => {
describe('process.getCreationTime()', () => {
it('returns a creation time', () => {
const creationTime = process.getCreationTime()
expect(creationTime).to.be.a('number').and.be.at.least(0)
})
})

describe('process.getCPUUsage()', () => {
it('returns a cpu usage object', () => {
const cpuUsage = process.getCPUUsage()
Expand Down