Extensions: fix GlicPrivate.invoke failed in webview [chromium/src : main]
Yuheng Huang (Gerrit)
Yuheng Huang voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Devlin Cronin
- Justin DeWitt
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin added 2 comments![]( "Open in Gerrit")
if (!source_frame.document_id.is_empty()) {As I mentioned in the associated bug, this is a *public-facing* API that's used today by lots and lots of extensions. Changing this behavior risks breaking them, since some could potentially be relying on this being undefined in various contexts. For instance, something like the code below:
```
if (sender.documentId) {
// Sender is in a tab; otherwise documentId is undefined.
executeScript(getTabWithDocumentId(sender.documentId));
}
```
I'm amenable to changing this for all extensions, but it is something we need to a) discuss, b) communicate, and c) test (as part of the public API).
WDYT about:
1) Filing a public bug and starting the discussion to always populate documentId when there's an associated frame?
2) As a *temporary* hack, populating documentId unconditionally if the connection is a webview to a component extension? (I think Justin mentioned this already in the bug?)
The reason I say 2) should be temporary is because it a) adds technical debt and b) makes these APIs (which are already complicated and nuanced) harder to reason about, so we should strive to remove it rather than introducing another possible path for confusion.
Related details
- Justin DeWitt
- Yuheng Huang
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. |
Justin DeWitt (Gerrit)
Justin DeWitt added 2 comments![]( "Open in Gerrit")
// Get the outer web contents to handle cases where the frame is inside aThis is very general, I think the code should be more tailored to this case. For example:
```
if (auto* guest_view = WebViewGuest::FromRenderFrameHost(rfh) &&
contextual_tasks::GetWebUiInterface(guest_view->embedder_web_contents()) {
web_contents = guest_view->embedder_web_contents();
}
```
while (web_contents->GetOuterWebContents()) {is this loop just web_contents->GetOutermostWebContents()
Related details
- Yuheng Huang
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. |
Justin DeWitt (Gerrit)
Justin DeWitt added 2 comments![]( "Open in Gerrit")
// Navigate the active tab to chrome://glic/.we want this to work in contextual tasks, not chrome://glic
while (document.body.firstChild) {
document.body.removeChild(document.body.firstChild);
}why?
Yuheng Huang (Gerrit)
Yuheng Huang added 1 comment![]( "Open in Gerrit")
if (!source_frame.document_id.is_empty()) {As I mentioned in the associated bug, this is a *public-facing* API that's used today by lots and lots of extensions. Changing this behavior risks breaking them, since some could potentially be relying on this being undefined in various contexts. For instance, something like the code below:
```
if (sender.documentId) {
// Sender is in a tab; otherwise documentId is undefined.
executeScript(getTabWithDocumentId(sender.documentId));
}
```I'm amenable to changing this for all extensions, but it is something we need to a) discuss, b) communicate, and c) test (as part of the public API).
WDYT about:
1) Filing a public bug and starting the discussion to always populate documentId when there's an associated frame?
2) As a *temporary* hack, populating documentId unconditionally if the connection is a webview to a component extension? (I think Justin mentioned this already in the bug?)The reason I say 2) should be temporary is because it a) adds technical debt and b) makes these APIs (which are already complicated and nuanced) harder to reason about, so we should strive to remove it rather than introducing another possible path for confusion.
Thanks Devlin. That makes sense. Can we land this before the API change discussion if 2 is addressed?
Related details
- Devlin Cronin
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. |
Devlin Cronin (Gerrit)
Devlin Cronin added 1 comment![]( "Open in Gerrit")
if (!source_frame.document_id.is_empty()) {Yuheng HuangAs I mentioned in the associated bug, this is a *public-facing* API that's used today by lots and lots of extensions. Changing this behavior risks breaking them, since some could potentially be relying on this being undefined in various contexts. For instance, something like the code below:
```
if (sender.documentId) {
// Sender is in a tab; otherwise documentId is undefined.
executeScript(getTabWithDocumentId(sender.documentId));
}
```I'm amenable to changing this for all extensions, but it is something we need to a) discuss, b) communicate, and c) test (as part of the public API).
WDYT about:
1) Filing a public bug and starting the discussion to always populate documentId when there's an associated frame?
2) As a *temporary* hack, populating documentId unconditionally if the connection is a webview to a component extension? (I think Justin mentioned this already in the bug?)The reason I say 2) should be temporary is because it a) adds technical debt and b) makes these APIs (which are already complicated and nuanced) harder to reason about, so we should strive to remove it rather than introducing another possible path for confusion.
Thanks Devlin. That makes sense. Can we land this before the API change discussion if 2 is addressed?
Yes, as long as a) it's intended as temporary (I'd like if we can start the discussion for 1] before landing this) and b) it has no public-facing API implications.
Related details
- Yuheng Huang
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. |