cpu_performance: Add prototype implementation [chromium/src : main]
Nikolaos Papaspyrou (Gerrit)
Nikolaos Papaspyrou added 3 comments![]( "Open in Gerrit")
I rewrote this CL from scratch in patchset 5.
PTAL and ignore the previous patchsets.
base::BindRepeating(&ContentBrowserClient::BindCPUInfoProvider,Nikolaos PapaspyrouWhy not match the call on line 1147?
This was rewritten in PS 5.
[Sync] GetCPUInfo() => (uint16 tier);Nikolaos PapaspyrouCould we make this non-sync?
This was rewritten in PS 5.
Related details
- Camille Lamy
- Dominic Farolino
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Farolino (Gerrit)
Dominic Farolino added 3 comments![]( "Open in Gerrit")
{"CpuPerformance", raw_ref(blink::features::kCpuPerformance)},Related to my other comment, I'm hopeful we can get rid of this if we can get the right runtime enabled features incantation?
// the whole feature work.Hmm, that's pretty standard for runtime enabled features though, including many that don't have `base_feature: none`, but actually list a real feature. Could you check out the documentation at the top of this file, in particular https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/runtime_enabled_features.json5;l=120-185;drc=2d4ffc91620fc1cae131415e81c8fed36dfabfaa, and see if we're able to modify this so that it auto generates the right feature?
niko...@chromium.orgWPT doesn't really operate with owners files. This folder is synced with the external https://github.com/web-platform-tests/wpt repository, so instead we should just add DIR_METADATA and META.yml files. See https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/dom, for example.
Related details
- Camille Lamy
- Nikolaos Papaspyrou
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nikolaos Papaspyrou (Gerrit)
Nikolaos Papaspyrou added 3 comments![]( "Open in Gerrit")
{"CpuPerformance", raw_ref(blink::features::kCpuPerformance)},Related to my other comment, I'm hopeful we can get rid of this if we can get the right runtime enabled features incantation?
Done
Hmm, that's pretty standard for runtime enabled features though, including many that don't have `base_feature: none`, but actually list a real feature. Could you check out the documentation at the top of this file, in particular https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/runtime_enabled_features.json5;l=120-185;drc=2d4ffc91620fc1cae131415e81c8fed36dfabfaa, and see if we're able to modify this so that it auto generates the right feature?
Done. This was written like this because in the previous version (PS#1) there was IPC between the renderer and the browser, and the latter consulted the real feature. It did not work correctly when `--enable-blink-features` was set but not `--enable-features`. If this is needed again, we'll see about it.
WPT doesn't really operate with owners files. This folder is synced with the external https://github.com/web-platform-tests/wpt repository, so instead we should just add DIR_METADATA and META.yml files. See https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/dom, for example.
- Camille Lamy
- Dominic Farolino
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Camille Lamy (Gerrit)
Camille Lamy added 1 comment![]( "Open in Gerrit")
Now that there are no longer any changes in content/, I think you don't need my review anymore? Let me know if that is not the case.
Related details
- Nikolaos Papaspyrou
Nikolaos Papaspyrou (Gerrit)
Nikolaos Papaspyrou voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nikolaos Papaspyrou
| Commit-Queue | +1 |
1 comment
There seem to be some issues with the tests, after I cleaned up the feature according to your comments. Please wait until I fix them.
Related details
- Nikolaos Papaspyrou
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Raphael Kubo da Costa (Gerrit)
Raphael Kubo da Costa added 1 comment![]( "Open in Gerrit")
Just a drive-by question: is there a corresponding spec and/or tracking bug somewhere? I don't see any links in the IDL file, a tracking bug or an Intent email on blink-dev.
I got curious because the idea sounds similar to the [Compute Pressure API](https://w3c.github.io/compute-pressure/) and was looking for more information about this API.
Nikolaos Papaspyrou (Gerrit)
Nikolaos Papaspyrou added 1 comment![]( "Open in Gerrit")
Just a drive-by question: is there a corresponding spec and/or tracking bug somewhere? I don't see any links in the IDL file, a tracking bug or an Intent email on blink-dev.
I got curious because the idea sounds similar to the [Compute Pressure API](https://w3c.github.io/compute-pressure/) and was looking for more information about this API.
The compute pressure API gives dynamic information (user device current CPU usage). This simpler API gives static information (user device CPU performance in general). An external (non-Google) explainer document will be circulated shortly. Thanks for asking!
Related details
- Raphael Kubo da Costa
Dominic Farolino (Gerrit)
Dominic Farolino added 5 comments![]( "Open in Gerrit")
Nikolaos PapaspyrouJust a drive-by question: is there a corresponding spec and/or tracking bug somewhere? I don't see any links in the IDL file, a tracking bug or an Intent email on blink-dev.
I got curious because the idea sounds similar to the [Compute Pressure API](https://w3c.github.io/compute-pressure/) and was looking for more information about this API.
The compute pressure API gives dynamic information (user device current CPU usage). This simpler API gives static information (user device CPU performance in general). An external (non-Google) explainer document will be circulated shortly. Thanks for asking!
We'll also need a Chrome Status entry for this feature (you can create one in https://chromestatus.com/), and an Intent to Prototype would be great, pointing to the explainer once its public. We should link to both from the commit description.
All of this should be in https://www.chromium.org/blink/launching-features/ though, so let's make sure we follow all the steps there!
Embedders may override this default with more complicated,This isn't yet true though, right? Do you have a design in mind that will let embedders override this value, to be layered on top of the current Blink implementation?
If so, maybe we could just mention what that design looks like, and say that this ability is "coming soon".
#include "third_party/blink/renderer/core/frame/navigator.h"Can we get away with a forward declaration here?
- nikolaosIs https://github.com/nikolaos you? (Just FYI this file refers to GitHub accounts)
}, "navigator.cpuPerformance is valid, if the feature is enabled");It might not be obvious, but our VirtualTestSuite infra which prompted you to create an enabled and disabled variant, is actually entirely internal to the Chromium project (because it doesn't exist inside the `external` directory). This means other browsers, and the overall public WPT project in general, will not know what the enabled/disabled distinction means, and they will have nothing to drive that behavior.
What we should do is just add this enabled test, but NOT under the `/enabled` directory, and under a `tentative` directory instead, as is usual for non-standardized features. Then we'll just ensure that the test runs exclusively with your flag enabled via the VTS file. Let me know if that makes sense.
Related details
- Nikolaos Papaspyrou
- Raphael Kubo da Costa
Nikolaos Papaspyrou (Gerrit)
Nikolaos Papaspyrou added 1 comment![]( "Open in Gerrit")
- nikolaosIs https://github.com/nikolaos you? (Just FYI this file refers to GitHub accounts)
It's https://github.com/nickie, so I suppose I have to change this to `nickie`.
Related details
- Dominic Farolino
- Raphael Kubo da Costa