Support mapping V8 use counters to WebDXFeature use counters. [chromium/src : main]
0 views
Skip to first unread message
Johnny Stenback (Gerrit)
Nov 7, 2024, 1:35:54 PMNov 7
to Nate Chapin, Scott Haseley, chromium...@chromium.org, Kentaro Hara, blink-revie...@chromium.org, blink-...@chromium.org
Attention needed from Nate Chapin and Scott Haseley
Johnny Stenback added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 1 (Latest):
Johnny Stenback . resolved
Hey Nate and Scott, seems everyone's an owner of this file and I just need 2 pairs of committer eyeballs on this change... This merely adds logic to be able to map a V8 use counter to a WebDXFeature use counter in addition to the existing ability to map to a WebFeature use counter. There's now needs for that creeping up and we should have this in place...
Thanks!
Related details
Attention is currently required from:
- Nate Chapin
- Scott Haseley
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Nov 7, 2024, 2:56:03 PMNov 7
to Johnny Stenback, Chromium LUCI CQ, Nate Chapin, chromium...@chromium.org, Kentaro Hara, blink-revie...@chromium.org, blink-...@chromium.org
Attention needed from Johnny Stenback and Nate Chapin
Scott Haseley voted and added 2 comments![]( "Open in Gerrit")
Votes added by Scott Haseley
| Code-Review | +1 |
2 comments
Patchset-level comments
Scott Haseley . resolved
lgtm w/ comment
File third_party/blink/renderer/bindings/core/v8/use_counter_callback.cc
Line 24, Patchset 1 (Latest):
std::optional<WebDXFeature> webdx_feature;Scott Haseley . unresolved
nit: Since `webdx_feature` and `deprecated` are ignored if `blink_feature` is set,
you might want to add more CHECKs below (!deprecated in webdx arm, !webdx_feature in blink feature arm) to prevent misuse.
Alternatively, you could consider enforcing the mutual exclusion with `absl::variant`, something like:
```
struct BlinkFeature {
WebFeature web_feature;
bool deprecated;
};
absl::variant<absl::monostate, BlinkFeature, WebDXFeature> feature;
```
Related details
Attention is currently required from:
- Johnny Stenback
- Nate Chapin
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johnny Stenback (Gerrit)
Nov 11, 2024, 5:04:46 PMNov 11
to Scott Haseley, Chromium LUCI CQ, Nate Chapin, chromium...@chromium.org, Kentaro Hara, blink-revie...@chromium.org, blink-...@chromium.org
Attention needed from Nate Chapin and Scott Haseley
Johnny Stenback added 1 comment![]( "Open in Gerrit")
File third_party/blink/renderer/bindings/core/v8/use_counter_callback.cc
nit: Since `webdx_feature` and `deprecated` are ignored if `blink_feature` is set,
you might want to add more CHECKs below (!deprecated in webdx arm, !webdx_feature in blink feature arm) to prevent misuse.Alternatively, you could consider enforcing the mutual exclusion with `absl::variant`, something like:
```
struct BlinkFeature {
WebFeature web_feature;
bool deprecated;
};absl::variant<absl::monostate, BlinkFeature, WebDXFeature> feature;
```
Johnny Stenback
Good point! I opted for the additional CHECKs in the patch set I just uploaded.
Cheers!
Related details
Attention is currently required from:
- Nate Chapin
- Scott Haseley
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Nov 11, 2024, 6:06:17 PMNov 11
to Johnny Stenback, Chromium LUCI CQ, Nate Chapin, chromium...@chromium.org, Kentaro Hara, blink-revie...@chromium.org, blink-...@chromium.org
Attention needed from Johnny Stenback and Nate Chapin
Scott Haseley voted and added 1 comment![]( "Open in Gerrit")
Votes added by Scott Haseley
| Code-Review | +1 |
Attention is currently required from:
- Johnny Stenback
- Nate Chapin
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marja Hölttä (Gerrit)
Nov 12, 2024, 2:00:48 AMNov 12
to Johnny Stenback, Scott Haseley, Chromium LUCI CQ, Nate Chapin, chromium...@chromium.org, Kentaro Hara, blink-revie...@chromium.org, blink-...@chromium.org
Attention needed from Johnny Stenback and Nate Chapin
Marja Hölttä added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Marja Hölttä . resolved
Hi, could you land this so I can do the V8 use counter related changes which depend on this? Thanks!
Related details
Attention is currently required from:
- Johnny Stenback
- Nate Chapin
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Chapin (Gerrit)
Nov 12, 2024, 1:09:37 PMNov 12
to Johnny Stenback, Marja Hölttä, Scott Haseley, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara, blink-revie...@chromium.org, blink-...@chromium.org
Attention needed from Johnny Stenback
Nate Chapin voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
Attention is currently required from:
- Johnny Stenback
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johnny Stenback (Gerrit)
Nov 12, 2024, 1:12:09 PMNov 12
to Nate Chapin, Marja Hölttä, Scott Haseley, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara, blink-revie...@chromium.org, blink-...@chromium.org
Johnny Stenback voted Commit-Queue+2![]( "Open in Gerrit")
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Nov 12, 2024, 2:04:53 PMNov 12
to Johnny Stenback, Nate Chapin, Marja Hölttä, Scott Haseley, chromium...@chromium.org, Kentaro Hara, blink-revie...@chromium.org, blink-...@chromium.org
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Commit message:
Support mapping V8 use counters to WebDXFeature use counters.
This tweaks Blink's existing V8 callback for mapping V8 use counters to
Blink use counters to support the relatively new WebDXFeature use
counter type.
Bug: 366475997
Change-Id: I5debe2771e6b3c794707d2aa63537b1018628387
Commit-Queue: Johnny Stenback <jste...@google.com>
Reviewed-by: Scott Haseley <shas...@chromium.org>
Reviewed-by: Nate Chapin <jap...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1381899}
Files:
- M third_party/blink/renderer/bindings/core/v8/use_counter_callback.cc
Change size: S
Delta: 1 file changed, 16 insertions(+), 5 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Nate Chapin, +1 by Scott Haseley
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages