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 2 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
8 changes: 8 additions & 0 deletions atom/common/api/atom_bindings.cc
View file
Open in desktop
Expand Up @@ -15,6 +15,7 @@
#include "atom/common/node_includes.h"
#include "base/logging.h"
#include "base/process/process_metrics_iocounters.h"
#include "base/process/process_info.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,12 @@ v8::Local<v8::Value> AtomBindings::GetProcessMemoryInfo(v8::Isolate* isolate) {
return dict.GetHandle();
}

// static
int64_t AtomBindings::GetCreationTime(v8::Isolate* isolate) {
auto metrics = base::CurrentProcessInfo::CreationTime();
return metrics.ToInternalValue();
Copy link
Contributor

Choose a reason for hiding this comment

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

this is deprecated

Copy link
Member

Choose a reason for hiding this comment

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

Should probably use

base::CurrentProcessInfo::CreationTime().ToJsTime()

Refs: ToJsTime()

Then users can just do new Date() on the ret value to get a JS date

Copy link
Member

Choose a reason for hiding this comment

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

Bonus points for a new mate converter to v8::Date

https://v8docs.nodesource.com/node-0.8/d4/df7/classv8_1_1_date.html

I.e. Convert base::Time to v8::Date

}

// 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 int64_t 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 microseconds (s/1,000,000) since the Windows epoch (1601-01-01 00:00:00 UTC)
Copy link
Contributor

Choose a reason for hiding this comment

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

mac & linux seem to return seconds from the start of the process.
And can be null or empty if the API fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at the run from Mac it is giving back time since windows epoch as well
Here is what we get when run on a Mac.
131750664685357260
2018-07-03T04:41:08.535Z

Comment from time.h in chromium

/ Time represents an absolute point in coordinated universal time (UTC),
// internally represented as microseconds (s/1,000,000) since the Windows epoch
// (1601-01-01 00:00:00 UTC).

Copy link
Contributor

Choose a reason for hiding this comment

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

microseconds since the Windows epoch

The returned value should be easy to use in JavaScript.
JavaScript Date object uses "number of milliseconds since 1 January 1970 UTC" (the epoch).

Copy link
Contributor

Choose a reason for hiding this comment

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

And can be null or empty if the API fails.

Please return null if the time cannot be obtained.


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
7 changes: 7 additions & 0 deletions spec/api-process-spec.js
View file
Open in desktop
@@ -1,6 +1,13 @@
const assert = require('assert')

describe('process module', () => {
describe('process.getCreationTime()', () => {
it('returns a creation time', () => {
const creationTime = process.getCreationTime()
assert.equal(typeof creationTime, 'number')
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use expect() assertions from the Chai library, which produce much better failure message than assert.

const {expect} = require('chai')  // On the top of the file.
...
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