[AdTagging] Frontend: Show ad provenance in "ad" adorner tooltip [devtools/devtools-frontend : main]
Yao Xiao (Gerrit)
Yao Xiao voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yao Xiao
| Commit-Queue | +1 |
1 comment
Related details
- Simon Zünd
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Simon Zünd (Gerrit)
Simon Zünd voted and added 4 comments![]( "Open in Gerrit")
Votes added by Simon Zünd
| Code-Review | +1 |
4 comments
lgtm % comments
@pfa...@chromium.org can you check if the adorner ref is doing the right thing? I didn't review the other ones and don't know if it follows the same pattern.
#adProvenanceInternal?: Protocol.Network.AdProvenance;nit: Since the CL changes this we can use the opportunity to drop the `Internal` suffix. This is a left-over from some migration script of the past.
return {} as Protocol.Network.AdProvenance;Is this cast actually necessary? Both properties are optional. I'd rather remove the `as` cast in case new props get added in the future that become mandatory. In that case we want a compilation erorr here.
<div style="color: var(--sys-color-on-surface-subtle);">Filterlist Rule</div>This string needs translation, same for `Creator Ad Script Ancestry` and `Root Script Filterlist Rule`. Please note that in DevTools we use title case. So unless these have a very concrete recognized casing (e.g. "DevTools", "Chrome" or some web API), these should probably be `Creator ad script ancestry` and `Root script filter list rule`.
If `Filterlist` is a concrete web API concept, then we can leave it as-is, but make sure to mark it with back-ticks in the `UIStrings` definition so it does ont get translated.
Related details
- Philip Pfaffe
- Yao Xiao
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yao Xiao (Gerrit)
Yao Xiao added 3 comments![]( "Open in Gerrit")
nit: Since the CL changes this we can use the opportunity to drop the `Internal` suffix. This is a left-over from some migration script of the past.
Done
Is this cast actually necessary? Both properties are optional. I'd rather remove the `as` cast in case new props get added in the future that become mandatory. In that case we want a compilation erorr here.
Removed!
<div style="color: var(--sys-color-on-surface-subtle);">Filterlist Rule</div>This string needs translation, same for `Creator Ad Script Ancestry` and `Root Script Filterlist Rule`. Please note that in DevTools we use title case. So unless these have a very concrete recognized casing (e.g. "DevTools", "Chrome" or some web API), these should probably be `Creator ad script ancestry` and `Root script filter list rule`.
If `Filterlist` is a concrete web API concept, then we can leave it as-is, but make sure to mark it with back-ticks in the `UIStrings` definition so it does ont get translated.
Done! I separated "filter list" into two words, which should be fine to translate as-is.
Related details
- Philip Pfaffe
- Simon Zünd
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Simon Zünd (Gerrit)
Simon Zünd voted and added 1 comment![]( "Open in Gerrit")
Votes added by Simon Zünd
| Code-Review | +1 |
1 comment
Nice, still lgtm! Wait for Philips' +1 though as I'm unsure about the adorner <-> lit interaction.
Related details
- Philip Pfaffe
- Yao Xiao
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Pfaffe (Gerrit)
Philip Pfaffe added 1 comment![]( "Open in Gerrit")
${adAdornerRef(input.adProvenance, input.target)}>Have you considered using <devtools-tooltip> instead of GlassPane/PopoverHelper? It saves you from having to do all that state management and is fully declarative.
Only thing you'll need to do is make sure the tooltip ids are unique, like by having a global counter for instance.
Related details
- Yao Xiao
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |