[OOPIF PDF] Hide PDF inner frames from chrome.debugger.getTargets() [chromium/src : main]
findit-for-me@appspot.gserviceaccount.com (Gerrit)
findit...@appspot.gserviceaccount.com voted Code-Coverage+1![]( "Open in Gerrit")
This change meets the code coverage requirements.
| Code-Coverage | +1 |
Related details
- Alex Rudenko
- Andrey Kosyakov
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Phan (Gerrit)
Andy Phan voted and added 1 comment![]( "Open in Gerrit")
Votes added by Andy Phan
| Commit-Queue | +1 |
1 comment
// exposed to chrome.debugger clients.Andy PhanHmm.. That has an effect way beyond `chrome.debugger` clients though. Can we keep this limitation specific to extensions? I imagine there may be legitimate cases other types of clients may be interested.
Alex RudenkoI'm not sure if there's a way to tell if the agent host is for the PDF extension frame and the PDF content frame at the //chrome/browser/extensions/api level. The list of agent hosts is retrieved for chrome.debugger.getTargets() here [1]. The type for those frames is "other".
We could block the PDF extension target by checking for the PDF extension URL. Maybe that works for the PDF extension frame? But the PDF content frame will have the PDF URL, e.g. https://www.pdf995.com/samples/pdf.pdf.
Maybe we can block the PDF extension frame in //chrome/browser/extensions/api using the URL check above, and block the PDF content frame in //content/browser/devtools like in patchset 6?
Andy PhanI think if it is an implementation detail (inner iframes) of the pdf viewer, it might be okay to hide it for everyone from the frame tree view/frame-level auto-attach. But I think the user-defined iframes pointing to PDF or auto-attach via the extension target should probably still work.
Alex RudenkoTo clarify, is the current approach of hiding both the inner PDF frames in content/browser/devtools/render_frame_devtools_agent_host.cc okay then?
But I think the user-defined iframes pointing to PDF or auto-attach via the extension target should probably still work.
The follow-up CL, https://crrev.com/c/5528140, fixes attaching debuggers to the PDF embedder frame. So debuggers can attach to the main frame in a full-page PDF viewer, or the <iframe> or <embed> to a PDF viewer.
Not sure if it's related, but a user can still open the context menu and inspect the frames. I believe that is desired behavior.
Andy Phanso I ran the Puppeteer tests with this CL and I am still able to attach to all frames (including inner PDF viewer extensions). There are probably multiple entrypoints to how CDP discovers agent hosts. It sounds like it would be better to filter this out in the debugger_api for extensions only like the CL description implies or make sure that it is completely hidden from all CDP clients. Probably doing it only for extensions makes sense as Andrey pointed out other clients might be eventually interested in debugging PDF viewer internals. It's probably best to add filtering here https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/extensions/api/debugger/debugger_api.cc;l=904;drc=5bf1a4a403490afa7294232e383a1750aefcc881
Andy PhanFiltering is now done in the debugger_api.cc level, although a new DevToolsAgentHost type had to be added. PTAL.
Wrapped debugger_api.cc changes in the OOPIF PDF feature flag, and added a kill switch for the DevToolsAgentHost type, since we're planning on merging to M126.
Related details
- Alex Rudenko
- Andrey Kosyakov
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
Could you please review? not sure if we need a new target type for this?
Related details
- Andrey Kosyakov
- Andy Phan
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
findit-for-me@appspot.gserviceaccount.com (Gerrit)
findit...@appspot.gserviceaccount.com voted Code-Coverage+1![]( "Open in Gerrit")
This change meets the code coverage requirements.
| Code-Coverage | +1 |
Related details
- Andrey Kosyakov
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Phan (Gerrit)
Andy Phan added 1 comment![]( "Open in Gerrit")
Could you please review? not sure if we need a new target type for this?
Okay, I think patchset 12 is able to hide the PDF inner frames purely at the //chrome layer.
Related details
- Alex Rudenko
- Andrey Kosyakov
- Devlin Cronin
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin voted and added 2 comments![]( "Open in Gerrit")
Votes added by Devlin Cronin
| Code-Review | +1 |
2 comments
LGTM overall, but curious: is this the only spot we need to hide these frames? (I guess we don't let them attach already, so maybe so?)
Also, is there desire to allow extensions to attach to the content frame (not the PDF Viewer frame) in the future?
return url.scheme() == extensions::kExtensionScheme &&nit: no need for extensions prefix
Related details
- Alex Rudenko
- Andrey Kosyakov
- Andy Phan
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Phan (Gerrit)
Andy Phan added 2 comments![]( "Open in Gerrit")
LGTM overall, but curious: is this the only spot we need to hide these frames? (I guess we don't let them attach already, so maybe so?)
Also, is there desire to allow extensions to attach to the content frame (not the PDF Viewer frame) in the future?
is this the only spot we need to hide these frames?
I'm unaware of any other spots. Based off the discussion in the [other comment](https://chromium-review.googlesource.com/c/chromium/src/+/5528139/comment/149355d7_a263d063/), from the CDP side, some clients may want to debug PDF internals.
I have a similar question, though, but for extensions. Outside of the APIs listed here and in https://crbug.com/333457293, are there any other extensions APIs that shouldn't see these frames?
Also, is there desire to allow extensions to attach to the content frame (not the PDF Viewer frame) in the future?
Another good question. I'll ask CSA about this.
return url.scheme() == extensions::kExtensionScheme &&nit: no need for extensions prefix
- Alex Rudenko
- Andrey Kosyakov
- Devlin Cronin
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
findit-for-me@appspot.gserviceaccount.com (Gerrit)
findit...@appspot.gserviceaccount.com voted Code-Coverage+1![]( "Open in Gerrit")
This change meets the code coverage requirements.
| Code-Coverage | +1 |
Andy Phan (Gerrit)
Andy Phan added 1 comment![]( "Open in Gerrit")
Andy PhanLGTM overall, but curious: is this the only spot we need to hide these frames? (I guess we don't let them attach already, so maybe so?)
Also, is there desire to allow extensions to attach to the content frame (not the PDF Viewer frame) in the future?
is this the only spot we need to hide these frames?
I'm unaware of any other spots. Based off the discussion in the [other comment](https://chromium-review.googlesource.com/c/chromium/src/+/5528139/comment/149355d7_a263d063/), from the CDP side, some clients may want to debug PDF internals.
I have a similar question, though, but for extensions. Outside of the APIs listed here and in https://crbug.com/333457293, are there any other extensions APIs that shouldn't see these frames?
Also, is there desire to allow extensions to attach to the content frame (not the PDF Viewer frame) in the future?
Another good question. I'll ask CSA about this.
Marking as resolved for submission.
Related details
- Alex Rudenko
- Andrey Kosyakov
- Devlin Cronin
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Phan (Gerrit)
Andy Phan voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
13 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/browser/extensions/api/debugger/debugger_api.cc
Insertions: 1, Deletions: 1.
The diff is too large to show. Please review the diff.
```
Change information
[OOPIF PDF] Hide PDF inner frames from chrome.debugger.getTargets()
Always hide the inner PDF extension frame and PDF content frame from
chrome.debugger.getTargets(). These frames are implementation details of
the PDF viewer, and aren't attachable anyway.
Keep the PDF embedder frame available for attachment.
- M chrome/browser/extensions/api/debugger/debugger_api.cc
- M chrome/browser/extensions/api/debugger/debugger_apitest.cc
Code-Coverage: +1 by findit...@appspot.gserviceaccount.com
Code-Review: +1 by Devlin Cronin
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Phan (Gerrit)
Andy Phan added 1 comment![]( "Open in Gerrit")
Andy PhanLGTM overall, but curious: is this the only spot we need to hide these frames? (I guess we don't let them attach already, so maybe so?)
Also, is there desire to allow extensions to attach to the content frame (not the PDF Viewer frame) in the future?
Andy Phanis this the only spot we need to hide these frames?
I'm unaware of any other spots. Based off the discussion in the [other comment](https://chromium-review.googlesource.com/c/chromium/src/+/5528139/comment/149355d7_a263d063/), from the CDP side, some clients may want to debug PDF internals.
I have a similar question, though, but for extensions. Outside of the APIs listed here and in https://crbug.com/333457293, are there any other extensions APIs that shouldn't see these frames?
Also, is there desire to allow extensions to attach to the content frame (not the PDF Viewer frame) in the future?
Another good question. I'll ask CSA about this.
Marking as resolved for submission.
Re-opening as a TODO.
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Phan (Gerrit)
Andy Phan added 1 comment![]( "Open in Gerrit")
Andy PhanLGTM overall, but curious: is this the only spot we need to hide these frames? (I guess we don't let them attach already, so maybe so?)
Also, is there desire to allow extensions to attach to the content frame (not the PDF Viewer frame) in the future?
Andy Phanis this the only spot we need to hide these frames?
I'm unaware of any other spots. Based off the discussion in the [other comment](https://chromium-review.googlesource.com/c/chromium/src/+/5528139/comment/149355d7_a263d063/), from the CDP side, some clients may want to debug PDF internals.
I have a similar question, though, but for extensions. Outside of the APIs listed here and in https://crbug.com/333457293, are there any other extensions APIs that shouldn't see these frames?
Also, is there desire to allow extensions to attach to the content frame (not the PDF Viewer frame) in the future?
Another good question. I'll ask CSA about this.
Andy PhanMarking as resolved for submission.
Re-opening as a TODO.
Resolving very old TODOs: there hasn't been any desire to allow extensions to attach to the content frame.
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |