[AdTracker] Expose GetAdScriptInStack and GetAncestry [chromium/src : main]
AI Code Reviewer (Gerrit)
AI Code Reviewer added 1 comment![]( "Open in Gerrit")
const std::optional<AdScriptOrContext>& ad_script_or_context() const {nit: Blink Style Guide: Naming - Use 'CamelCase' for all function names. Since the capitalized name `AdScriptOrContext` would collide with the return type name, prefix it with 'Get'. Consider renaming this getter to `GetAdScriptOrContext()` to match the CamelCase convention used by other methods in this class (e.g., `Schedule`, `Cancel`, `Id`).
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Related details
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. |
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
- Josh Karlin
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. |
Josh Karlin (Gerrit)
Josh Karlin added 1 comment![]( "Open in Gerrit")
- Exposing `GetAdScriptInStack` allows us to retrieve a single script
without reconstructing the entire ancestry chain immediately. ThisYou're effectively returning an identifier that can be turned back into an ancestry object later. I'm supportive of the idea if the cost of creating the ancestry is in fact prohibitive (which I'm not yet convinced of).
I think in the case of an ad context however, we want more than just AdExecutionContext right? Like, I'd think we want to know that it's an ad frame and what script created that ad frame and its call chain, or its matching rule.
That seems like a change to AdProvenance and IsAdScriptInStack that we should make prior to this CL.
Then, we can talk about just using that full AdAncestry everywhere, or if necessary, providing some sort of AdAncestryIdentifier or a partially-filled AdAncestry object that can later be fleshed out.
In either case, I think we'll only need one API and not two.
Related details
- Yao Xiao
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. |
Yao Xiao (Gerrit)
Yao Xiao added 1 comment![]( "Open in Gerrit")
- Exposing `GetAdScriptInStack` allows us to retrieve a single script
without reconstructing the entire ancestry chain immediately. ThisYou're effectively returning an identifier that can be turned back into an ancestry object later. I'm supportive of the idea if the cost of creating the ancestry is in fact prohibitive (which I'm not yet convinced of).
I think in the case of an ad context however, we want more than just AdExecutionContext right? Like, I'd think we want to know that it's an ad frame and what script created that ad frame and its call chain, or its matching rule.
That seems like a change to AdProvenance and IsAdScriptInStack that we should make prior to this CL.
Then, we can talk about just using that full AdAncestry everywhere, or if necessary, providing some sort of AdAncestryIdentifier or a partially-filled AdAncestry object that can later be fleshed out.
In either case, I think we'll only need one API and not two.
As discussed offline, this CL bundles too many refactorings and has become difficult to review. I'm going to abandon it.
I'll make a narrowly scoped CL for my immediate need first, and will revisit these refactorings later as smaller, separate patches.
Related details
- Josh Karlin
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. |
Yao Xiao (Gerrit)
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |