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.

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

Add OS pid to web-contents #9222

Merged
merged 5 commits into from May 15, 2017

Conversation

Projects
None yet
3 participants
@alexstrat
Contributor

alexstrat commented Apr 18, 2017

This pull request adds a method (getOSProcessId) to webContents to have access to the pid (actual OS process id) of the associated host render process.

  • documentation and testing: should I add a note in documentation and test about it? getProcessId is not documented nor tested: what is the logic?
  • naming: is method naming ok? getProcessId is already taken but undocumented.

I'm trying to have an overview of webcontents resource consumption (in detail, not in a whole), inspired from Chromium task manager (related #820, #3653).
As far as i understand, process.getProcessMemoryInfo gives only the current process statistics (and no CPU) => having an overview would require to "instrument" every new web-contents and report the stats.

I guessed a more handy and extensible way would be have access to the pids of the webcontents and use whatever userland method to get the statistics.

alexstrat added some commits Apr 18, 2017

@poiru

This comment has been minimized.

Member

poiru commented Apr 18, 2017

See also #9214.

@alexstrat

This comment has been minimized.

Contributor

alexstrat commented Apr 18, 2017

@poiru oh thanks! Interesting: I missed this. Does it make this PR irrelevant?

I see 2 reasons why it can still be relevant:

  1. access to more possibilities. For example: accessing the CPU usage, that getAppMemoryInfo does not give.
  2. link stats to actual web-contents (unless answer to this question is positive)

Edit

  1. #9373 introduced access to CPU data but only for the current process. Some work can be done to adapt the implementation for every processes like #9214 did with memory.
  2. With #9214 it's possible to link stats to web-contents through to pid (even if it's not the OS' pid) #9214 returns pid as the actual OS process id, the link is not possible
@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented May 15, 2017

naming: is method naming ok? getProcessId is already taken but undocumented.

I think the naming is okay since getProcessId is taken, we can revisit this in 2.0

Can you add a simple spec for this method just to ensure it does not crash on any platforms and returns a number?

alexstrat added some commits May 15, 2017

describe('getOSProcessId()', function () {
it('returns a valid procress id', function () {
// load URL otherwise getOSProcessId() returns 0
w.loadURL('file://' + path.join(__dirname, 'fixtures', 'pages', 'focus-web-contents.html'))

This comment has been minimized.

@kevinsawicki

kevinsawicki May 15, 2017

Contributor

You could just load about:blank here if you want to instead of this page, makes it more concise and makes this spec independent of this fixture.

@kevinsawicki kevinsawicki self-assigned this May 15, 2017

@@ -82,6 +82,7 @@ class WebContents : public mate::TrackableObject<WebContents>,
int64_t GetID() const;
int GetProcessID() const;
int GetOSProcessID() const;

This comment has been minimized.

@kevinsawicki

kevinsawicki May 15, 2017

Contributor

The return value of this could be just base::ProcessId.

@kevinsawicki kevinsawicki merged commit d79ac8d into electron:master May 15, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented May 15, 2017

Thanks for adding this 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment