Add HasTranscriptsForFrame() to avoid un-needed copies of transcripts [chromium/src : main]
0 views
Skip to first unread message
Ahmed Moussa (Gerrit)
Mar 11, 2026, 3:08:10 PMMar 11
to Muyao Xu, Chromium LUCI CQ, chromium...@chromium.org, chrome-intell...@chromium.org, chrome-intelligence-te...@google.com, dewitt...@chromium.org, feature-me...@chromium.org, mfoltz+wa...@chromium.org
Attention needed from Muyao Xu
Ahmed Moussa added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Related details
Attention is currently required from:
- Muyao Xu
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ahmed Moussa (Gerrit)
Mar 11, 2026, 3:09:14 PMMar 11
to Michael Crouse, Muyao Xu, Chromium LUCI CQ, chromium...@chromium.org, chrome-intell...@chromium.org, chrome-intelligence-te...@google.com, dewitt...@chromium.org, feature-me...@chromium.org, mfoltz+wa...@chromium.org
Attention needed from Michael Crouse and Muyao Xu
Ahmed Moussa added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Ahmed Moussa . resolved
Hello Michael, PTAL at `components/optimization_guide/content/browser/*`, thanks
Related details
Attention is currently required from:
- Michael Crouse
- Muyao Xu
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Muyao Xu (Gerrit)
Mar 11, 2026, 3:35:08 PMMar 11
to Ahmed Moussa, Michael Crouse, Chromium LUCI CQ, chromium...@chromium.org, chrome-intell...@chromium.org, chrome-intelligence-te...@google.com, dewitt...@chromium.org, feature-me...@chromium.org, mfoltz+wa...@chromium.org
Attention needed from Ahmed Moussa and Michael Crouse
Muyao Xu voted and added 1 comment![]( "Open in Gerrit")
Votes added by Muyao Xu
| Code-Review | +1 |
1 comment
File components/optimization_guide/content/browser/media_transcript_provider.h
Related details
Attention is currently required from:
- Ahmed Moussa
- Michael Crouse
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ahmed Moussa (Gerrit)
Mar 12, 2026, 2:03:18 PMMar 12
to Abigail Klein, Muyao Xu, Chromium LUCI CQ, chromium...@chromium.org, chrome-intell...@chromium.org, chrome-intelligence-te...@google.com, dewitt...@chromium.org, feature-me...@chromium.org, mfoltz+wa...@chromium.org
Attention needed from Abigail Klein
Ahmed Moussa added 1 comment![]( "Open in Gerrit")
Attention is currently required from:
- Abigail Klein
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Abigail Klein (Gerrit)
Mar 12, 2026, 5:14:14 PMMar 12
to Ahmed Moussa, Muyao Xu, Chromium LUCI CQ, chromium...@chromium.org, chrome-intell...@chromium.org, chrome-intelligence-te...@google.com, dewitt...@chromium.org, feature-me...@chromium.org, mfoltz+wa...@chromium.org
Attention needed from Ahmed Moussa
Abigail Klein added 1 comment![]( "Open in Gerrit")
File chrome/browser/glic/media/media_transcript_provider_impl.cc
Line 48, Patchset 3 (Latest):
content::RenderFrameHost* rfh) {Abigail Klein . unresolved
The fact that this is a pointer means that the memory could be released before the pointer is used. Can we either pass a safe pointer or a reference? I know that right now this is just called by PageContentMetadataObserver where we know that rfh exists, but I'm worried about a possible future case where the caller might be passing a dangling pointer.
Related details
Attention is currently required from:
- Ahmed Moussa
Submit Requirements:
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. |
Ahmed Moussa (Gerrit)
Mar 13, 2026, 2:37:56 PMMar 13
to Abigail Klein, Muyao Xu, Chromium LUCI CQ, chromium...@chromium.org, chrome-intell...@chromium.org, chrome-intelligence-te...@google.com, dewitt...@chromium.org, feature-me...@chromium.org, mfoltz+wa...@chromium.org
Attention needed from Abigail Klein
Ahmed Moussa added 1 comment![]( "Open in Gerrit")
File chrome/browser/glic/media/media_transcript_provider_impl.cc
The fact that this is a pointer means that the memory could be released before the pointer is used. Can we either pass a safe pointer or a reference? I know that right now this is just called by PageContentMetadataObserver where we know that rfh exists, but I'm worried about a possible future case where the caller might be passing a dangling pointer.
Attention is currently required from:
- Abigail Klein
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Abigail Klein (Gerrit)
Mar 16, 2026, 11:33:11 AMMar 16
to Ahmed Moussa, Muyao Xu, Chromium LUCI CQ, chromium...@chromium.org, chrome-intell...@chromium.org, chrome-intelligence-te...@google.com, dewitt...@chromium.org, feature-me...@chromium.org, mfoltz+wa...@chromium.org
Attention needed from Ahmed Moussa
Abigail Klein voted and added 1 comment![]( "Open in Gerrit")
Votes added by Abigail Klein
| Code-Review | +1 |
1 comment
File chrome/browser/glic/media/media_transcript_provider_impl.cc
Line 48, Patchset 3:
content::RenderFrameHost* rfh) {Abigail Klein . unresolved
Ahmed MoussaThe fact that this is a pointer means that the memory could be released before the pointer is used. Can we either pass a safe pointer or a reference? I know that right now this is just called by PageContentMetadataObserver where we know that rfh exists, but I'm worried about a possible future case where the caller might be passing a dangling pointer.
Done
Abigail Klein
Sorry for churn--I see now that the rfh pointer is passed around everywhere else in this file. So I think you can actually do what you want with respect to pointer vs reference; if you want to change it back to pointer that's fine.
Related details
Attention is currently required from:
- Ahmed Moussa
Submit Requirements:
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. |
Ahmed Moussa (Gerrit)
Mar 16, 2026, 3:51:27 PMMar 16
to Abigail Klein, Muyao Xu, Chromium LUCI CQ, chromium...@chromium.org, chrome-intell...@chromium.org, chrome-intelligence-te...@google.com, dewitt...@chromium.org, feature-me...@chromium.org, mfoltz+wa...@chromium.org
Ahmed Moussa added 1 comment![]( "Open in Gerrit")
File chrome/browser/glic/media/media_transcript_provider_impl.cc
Line 48, Patchset 3:
content::RenderFrameHost* rfh) {Abigail Klein . resolved
Ahmed MoussaThe fact that this is a pointer means that the memory could be released before the pointer is used. Can we either pass a safe pointer or a reference? I know that right now this is just called by PageContentMetadataObserver where we know that rfh exists, but I'm worried about a possible future case where the caller might be passing a dangling pointer.
Abigail KleinDone
Sorry for churn--I see now that the rfh pointer is passed around everywhere else in this file. So I think you can actually do what you want with respect to pointer vs reference; if you want to change it back to pointer that's fine.
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ahmed Moussa (Gerrit)
Mar 16, 2026, 3:51:40 PMMar 16
to Abigail Klein, Muyao Xu, Chromium LUCI CQ, chromium...@chromium.org, chrome-intell...@chromium.org, chrome-intelligence-te...@google.com, dewitt...@chromium.org, feature-me...@chromium.org, mfoltz+wa...@chromium.org
Ahmed Moussa added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ahmed Moussa (Gerrit)
Mar 16, 2026, 3:51:58 PMMar 16
to Abigail Klein, Muyao Xu, Chromium LUCI CQ, chromium...@chromium.org, chrome-intell...@chromium.org, chrome-intelligence-te...@google.com, dewitt...@chromium.org, feature-me...@chromium.org, mfoltz+wa...@chromium.org
Ahmed Moussa voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Mar 16, 2026, 7:11:24 PMMar 16
to Ahmed Moussa, Abigail Klein, Muyao Xu, chromium...@chromium.org, chrome-intell...@chromium.org, chrome-intelligence-te...@google.com, dewitt...@chromium.org, feature-me...@chromium.org, mfoltz+wa...@chromium.org
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Commit message:
Add HasTranscriptsForFrame() to avoid un-needed copies of transcripts
Change page_content_metadata_observer to call HasTranscriptsForFrame()
instead of GetTranscriptsForFrame() to avoid un-needed copies of
transcripts.
Bug: 483422796
Change-Id: Iefdb61c342b436cf06e4f6d6b0e0266bc55d0f63
Reviewed-by: Muyao Xu <muy...@google.com>
Commit-Queue: Ahmed Moussa <ahmed...@google.com>
Reviewed-by: Abigail Klein <abigai...@google.com>
Cr-Commit-Position: refs/heads/main@{#1600175}
Files:
- M chrome/browser/glic/media/glic_media_context.cc
- M chrome/browser/glic/media/glic_media_context.h
- M chrome/browser/glic/media/glic_media_context_unittest.cc
- M chrome/browser/glic/media/media_transcript_provider_impl.cc
- M chrome/browser/glic/media/media_transcript_provider_impl.h
- M chrome/browser/glic/media/media_transcript_provider_impl_unittest.cc
- M components/optimization_guide/content/browser/media_transcript_provider.h
- M components/optimization_guide/content/browser/mock_media_transcript_provider.h
- M components/optimization_guide/content/browser/page_content_metadata_observer.cc
Change size: M
Delta: 9 files changed, 63 insertions(+), 1 deletion(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Muyao Xu, +1 by Abigail Klein
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages