Add js-profiling-mode enum Document Policy with eager/lazy V8 profiler initialization [chromium/src : main]
Michal Mocny (Gerrit)
Michal Mocny added 11 comments![]( "Open in Gerrit")
This is not my area of code expertise (doc policy not js profiling) but here is an initial review.
I haven't really analyzed the tests, but I think this is a solid start and we can expand the test cases a bit later, as needed.
You may be able/want to address some of my superficial comments, but don't hesitate to start to ask experts to start review for feedback / guidance on direction.
This is the first enum-type Document Policy feature in Chromium.Fancy!
const char* EnumValueToToken(int32_t value) {Style on returning char* based strings...
const char* token = EnumValueToToken(value.IntValue());So *any* enum will map to this specific enum values?
This feels fine as a local prototype, but I think you need some machinery to handle mapping the right enum type to the right enum strings. As a bad alternative, find a way to break this loudly when the next enum type is introduced so the next person has to deal with it...
default_value: "0",The `default_value` of `0` corresponds to `JSProfilingMode::kNone`. However, there isn't a corresponding token in `TokenToEnumValue` (line 29 in `document_policy_parser.cc`) or `EnumValueToToken` (line 40 in `document_policy.cc`) to represent `kNone`.
This means that while `kNone` is the default, it cannot be explicitly set via a Document Policy header. Only `eager` and `lazy` can be specified.
Is this intentional? If so, it might be worth adding a comment to clarify that `kNone` is an implicit default and cannot be set explicitly via the policy header, to avoid confusion if someone tries to write `js-profiling-mode='none'`.
std::optional<int32_t> TokenToEnumValue(const std::string& token) {Hmm, we go from enum string to enum value back to enum string later... Ideally these to_string and from_string would be defined together, indeally in `profiler_group.h`?
(Also this implementation has the same issue as the other one in that its generic for all enums but specific to this particular enum.)
---
WDYT about adding a second enum directly to `third_party/blink/public/mojom/permissions_policy/document_policy_feature.mojom`?
And then use `document_policy_features.json5`?
...that might need some build file pre-processing changes to support enum *values*, but I think this feels like the correct approach.
(I'm totally new to this area, but this is what I found with a bit of digging.)
void InitV8Profiler(v8::CpuProfilingLoggingMode logging_mode);Similarly here, can we just have one variant?
void OnProfilingContextAddedLazy(ExecutionContext* context);Nit: Just add a mode arg to the method above, with a default of eager?
int32_t enum_val = mode_value.IntValue();
if (enum_val == static_cast<int32_t>(JSProfilingMode::kEager) ||
enum_val == static_cast<int32_t>(JSProfilingMode::kLazy)) {
return static_cast<JSProfilingMode>(enum_val);
}Nit: style, but let's iterate on the design of the enum first
return true;If `GetProfilingMode()` returns `kNone`, it means that `IsFeatureEnabled(kJSProfiling)` has already returned `false`. Therefore, the `IsFeatureEnabled` check on line 102 will also be `false`, making this `return true;` unreachable.
Additionally, my understanding is that `IsFeatureEnabled` do some logging/reporting and the GetProfilingMode would skip its check if the new profiling mode was supplied without the general profiling policy.
---
If kJSProfiling is still required alongside kJSProfilingMode, I think you could swap the order of feature detection and if kJSProfiling is disabled always return kNone, otherwise return provided policy or default to eager.
If kJSProfiling is optional... maybe the current implementation is fine, but perhaps add a comment here?
context_observers_.insert(observer);THis will require expert review, but, it surpises me that you can init via `InitV8Profiler(v8::kLazyLogging);` but that we choose not to do so.
So you can have tri-state:
- Un-init
- Init w/ Lazy
- Init w/ Eager
This document policy adds 2 modes and picks (1) and (3).
(But it also creates a `ProfilingContextObserver` eagerly, which afaik isn't needed until you start to profile? Not sure.)
Related details
- Thomas BERTET
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |