feat: Add creationTime function to process #13542

husayn · 2018-07-03T04:12:53Z

Expose the creationTime that chromium provides us on the process object.

juturu · 2018-07-03T04:22:04Z

docs/api/process.md

+
+Returns [`number`]
+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)


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

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).

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).

And can be null or empty if the API fails.

Please return null if the time cannot be obtained.

juturu · 2018-07-03T04:46:51Z

atom/common/api/atom_bindings.cc

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


this is deprecated

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

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

alexeykuzmin · 2018-07-09T08:42:43Z

docs/api/process.md

+
+Returns [`number`]
+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)


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).

alexeykuzmin · 2018-07-09T08:45:54Z

spec/api-process-spec.js

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


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)

alexeykuzmin · 2018-07-09T08:47:02Z

docs/api/process.md

+
+Returns [`number`]
+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)


And can be null or empty if the API fails.

Please return null if the time cannot be obtained.

codebytere · 2018-07-20T17:59:10Z

@husayn if you could please address the requested changes so we can get this across the finish line, that'd be great!

MarshallOfSound · 2018-07-30T00:31:18Z

atom/common/api/atom_bindings.cc

+    return v8::Null(isolate);
+  }
+  double jsTime = timeValue.ToJsTime();
+  return v8::Number::New(isolate, jsTime);


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

@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); }

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

MarshallOfSound

Requesting changes RE return type

alexeykuzmin · 2018-08-09T21:56:39Z

docs/api/process.md

@@ -98,6 +98,12 @@ The `process` object has the following methods:

 Causes the main thread of the current process crash.

+### `process.getCreationTime()`
+
+Returns [`number`]


If we return null sometimes, it should be documented )

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

ckerr

LGTM.

alexeykuzmin · 2018-08-09T21:57:24Z

@husayn
Can you please rebase your branch on the latest electron:master? to resolve conflicts.

ckerr · 2018-08-10T01:08:43Z

docs/api/process.md

+Returns `number`
+Indicates the creation time of the application.
+The time is represented as number of milliseconds since epoch. It returns null if it is unable to get the process creation time.
+


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

done @ckerr

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

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

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

@ckerr capitalized it.

ckerr · 2018-08-10T14:03:17Z

The CI failures appear to be unrelated.

electron-gn-linux-ia32-debug-fyi is failing with wrong ELF class: ELFCLASS64

electron-gn-linux-ia32-release-fyi is failing with linker errors

husayn added 2 commits July 2, 2018 13:56

Add process creation time

e27592e

Making docs clear for process creation time

78b9aaf

husayn requested review from a team July 3, 2018 04:12

juturu approved these changes Jul 3, 2018

View reviewed changes

juturu reviewed Jul 3, 2018

View reviewed changes

alexeykuzmin suggested changes Jul 9, 2018

View reviewed changes

husayn added 2 commits July 29, 2018 08:50

Address comments for process creation time

c4cdfc9

Add process info cc file

14c452c

MarshallOfSound reviewed Jul 30, 2018

View reviewed changes

MarshallOfSound requested changes Jul 30, 2018

View reviewed changes

alexeykuzmin reviewed Aug 9, 2018

View reviewed changes

alexeykuzmin approved these changes Aug 9, 2018

View reviewed changes

ckerr approved these changes Aug 9, 2018

View reviewed changes

ckerr and others added 2 commits August 9, 2018 16:59

Merge branch 'master' into master

81d2dbd

fixing comments around documentation

9025cef

ckerr requested changes Aug 10, 2018

View reviewed changes

husayn added 2 commits August 9, 2018 18:13

Update doc for return val

34bb5d4

Capitalize number in docs

9c03832

ckerr approved these changes Aug 10, 2018

View reviewed changes

MarshallOfSound mentioned this pull request Aug 10, 2018

fix: allow primitives in NodeJS.Process modifications electron/typescript-definitions#107

Merged

chore: bump electron-typescript-definitions

6deffff

MarshallOfSound approved these changes Aug 10, 2018

View reviewed changes

MarshallOfSound added the semver/minor backwards-compatible functionality label Aug 10, 2018

ckerr merged commit 19cb5ba into electron:master Aug 10, 2018

ckerr mentioned this pull request Dec 18, 2018

Draft 4.0.0 Release Notes #16117

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add creationTime function to process #13542

feat: Add creationTime function to process #13542

husayn commented Jul 3, 2018

juturu Jul 3, 2018

husayn Jul 3, 2018

alexeykuzmin Jul 9, 2018

alexeykuzmin Jul 9, 2018

juturu Jul 3, 2018

MarshallOfSound Jul 3, 2018

MarshallOfSound Jul 3, 2018 •

edited

alexeykuzmin Jul 9, 2018

alexeykuzmin Jul 9, 2018

alexeykuzmin Jul 9, 2018

codebytere commented Jul 20, 2018

MarshallOfSound Jul 30, 2018

alexeykuzmin Aug 9, 2018 •

edited

husayn Aug 10, 2018 •

edited

MarshallOfSound left a comment

alexeykuzmin Aug 9, 2018

alexeykuzmin Aug 9, 2018

ckerr left a comment

alexeykuzmin commented Aug 9, 2018

ckerr Aug 10, 2018

husayn Aug 10, 2018

MarshallOfSound Aug 10, 2018

ckerr Aug 10, 2018

ckerr Aug 10, 2018

husayn Aug 10, 2018

ckerr commented Aug 10, 2018

feat: Add creationTime function to process #13542

feat: Add creationTime function to process #13542

Conversation

husayn commented Jul 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarshallOfSound Jul 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codebytere commented Jul 20, 2018

Choose a reason for hiding this comment

alexeykuzmin Aug 9, 2018 • edited

Choose a reason for hiding this comment

husayn Aug 10, 2018 • edited

Choose a reason for hiding this comment

MarshallOfSound left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ckerr left a comment

Choose a reason for hiding this comment

alexeykuzmin commented Aug 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ckerr commented Aug 10, 2018

MarshallOfSound Jul 3, 2018 •

edited

alexeykuzmin Aug 9, 2018 •

edited

husayn Aug 10, 2018 •

edited