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

Add app.getGPUFeatureStatus #9623

Merged
merged 4 commits into from May 31, 2017
Merged

Add app.getGPUFeatureStatus #9623

merged 4 commits into from May 31, 2017

Conversation

miniak
Copy link
Contributor

@miniak miniak commented May 30, 2017 •
edited

This method returns the Graphics Feature Status from chrome://gpu/ as JSON.
It allows detecting the status of GPU acceleration without having to open a hidden BrowserWindow and parsing the data from HTML.

Example output:

{  
   "2d_canvas":"enabled",
   "flash_3d":"enabled",
   "flash_stage3d":"enabled",
   "flash_stage3d_baseline":"enabled",
   "gpu_compositing":"enabled",
   "multiple_raster_threads":"enabled_on",
   "native_gpu_memory_buffers":"enabled",
   "rasterization":"enabled",
   "video_decode":"enabled",
   "video_encode":"enabled",
   "vpx_decode":"enabled",
   "webgl":"enabled",
   "webgl2":"enabled"
}"

The list of possible values can be found in chrome://gpu/gpu_internals.js (statusMap).

@miniak
Copy link
Contributor Author

miniak commented May 30, 2017

@kevinsawicki, should I name it getGPUFeatureStatus or getGpuFeatureStatus?

@kevinsawicki
Copy link
Contributor

should I name it getGPUFeatureStatus or getGpuFeatureStatus?

I'm thinking getGPUFeatureStatus, since we have getCPUUsage and getIOCounters APIs.

@miniak miniak changed the title Add app.getGpuFeatureStatus Add app.getGPUFeatureStatus May 30, 2017
@miniak
Copy link
Contributor Author

miniak commented May 30, 2017

@kevinsawicki renamed & added docs.

kevinsawicki
kevinsawicki reviewed May 30, 2017
View reviewed changes
atom/browser/api/atom_api_app.cc Outdated
mate::Dictionary App::GetGPUFeatureStatus(v8::Isolate* isolate) {
auto result = mate::Dictionary::CreateEmpty(isolate);

if (auto status = content::GetFeatureStatus()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There should already be a converter setup for base::DictionaryValue.

Would this work here?

return mate::ConvertToV8(isolate, status);

Then you wouldn't need the ConvertDictionary helper.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does not work. There does not seem to be a converter from base::DictionaryValue. In generic case the values can have different types. In this case it can only be a string.

Copy link
Contributor

Choose a reason for hiding this comment

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

@miniak I pushed up a spec and commit that uses the converter, please let me know if you are okay with that approach.

kevinsawicki
kevinsawicki reviewed May 30, 2017
View reviewed changes
atom/browser/api/atom_api_app.cc Outdated
@@ -1095,7 +1126,8 @@ void App::BuildPrototype(
.SetMethod("getFileIcon", &App::GetFileIcon)
.SetMethod("getAppMetrics", &App::GetAppMetrics)
// TODO(juturu): Remove in 2.0, deprecate before then with warnings
.SetMethod("getAppMemoryInfo", &App::GetAppMetrics);
.SetMethod("getAppMemoryInfo", &App::GetAppMetrics)
.SetMethod("getGPUFeatureStatus", &App::GetGPUFeatureStatus);
Copy link
Contributor

Choose a reason for hiding this comment

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

Mind moving this above the // TODO comment so we don't think it is marked for deprecation like getAppMemoryInfo is?

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

miniak
miniak commented May 31, 2017
View reviewed changes
spec/api-app-spec.js Outdated
@@ -558,4 +558,14 @@ describe('app module', function () {
assert.ok(types.includes('Tab'))
})
})

describe('getGPUFeatureStatus() API', function () {
if (process.platform !== 'darwin') return
Copy link
Contributor Author

@miniak miniak May 31, 2017 •
edited

Choose a reason for hiding this comment

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

@kevinsawicki Do we need to check for Mac? It should work on all platforms.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, wasn't sure, will remove this line 👍

@kevinsawicki kevinsawicki merged commit 9250b55 into electron:master May 31, 2017
@kevinsawicki
Copy link
Contributor

Thanks for this @miniak 👍

@miniak
Copy link
Contributor Author

miniak commented May 31, 2017

@kevinsawicki thanks for merging

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

@kevinsawicki kevinsawicki kevinsawicki left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@miniak @kevinsawicki