Add a SlimWebView component to glic [chromium/src : main]
Aldo Culquicondor (Gerrit)
Aldo Culquicondor added 1 comment![]( "Open in Gerrit")
Kevin for components/guest_view
David for glic and resources/
Related details
- David Trainor
- Kevin McNee
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin McNee (Gerrit)
Kevin McNee added 7 comments![]( "Open in Gerrit")
build_webui("build") {Optional: I think these resources could live alongside the rest of the implementation in components. We just wouldn't be able to assert on the extensions build flag.
iframeElement: HTMLIFrameElement;Optional: Depending on how closely you want to stick to acting like a <webview>, make the internal iframe private and encapsulate it in a closed shadow dom.
// GuestViewInternalCustomBindings::RegisterView().nit: This is referring to the extensions impl.
import("//extensions/buildflags/buildflags.gni")Both of these are layering violations.
"//components/guest_view/common:mojom",This is a layering violation. content can't depend on this.
WebUIExtension::Install(frame_, enabled_bindings_);If the bindings checks are all being done within this function, perhaps make that clearer by renaming this to MaybeInstall or something like that.
guest_view::SlimWebViewBindings::Install(isolate, context, chrome,As mentioned above, content can't depend on guest view. This will need to be done through a delegate of some sort. ContentRendererClient perhaps?
Related details
- Aldo Culquicondor
- David Trainor
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. |
David Trainor (Gerrit)
David Trainor added 1 comment![]( "Open in Gerrit")
Adding harringtond@ - he's a closer owner to this and probably has better review context for the same files (c/b/glic and c/b/r/glic). Let me know if you need my review on anything else though!
Related details
- Aldo Culquicondor
- Dan Harrington
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. |
Dan Harrington (Gerrit)
Dan Harrington added 4 comments![]( "Open in Gerrit")
}
// <if expr="enable_extensions_core">
type WebViewType = chrome.webviewTag.WebView;
// </if>
// <if expr="not enable_extensions_core">
type WebViewType = SlimWebViewElement;
// </if>let's move this to webview.ts
// <if expr="enable_extensions_core">
const webview =
document.createElement('webview') as chrome.webviewTag.WebView;
// </if>
// <if expr="not enable_extensions_core">
const webview = document.createElement('slim-webview');
// </if>suggest moving to webview.ts, as a createWebview() function.
// <if expr="enable_extensions_core">
this.webview =
document.createElement('webview') as chrome.webviewTag.WebView;
// </if>
// <if expr="not enable_extensions_core">
this.webview = document.createElement('slim-webview');
// </if>let's have a createWebview function
// <if expr="enable_extensions_core">i really don't like the preprocessor if statements mixed in with code. Here, could we do something like:
getFullWebview(): chrome.webviewTag.WebView | undefined {
// <if expr=...>
...
// </if>
}
then later:
const fullWebview = this.getFullWebview();
if (fullWebview) {
// register the header injector
}
This should lead to fewer "// if" blocks.
Related details
- Aldo Culquicondor
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. |
Aldo Culquicondor (Gerrit)
Aldo Culquicondor added 11 comments![]( "Open in Gerrit")
// <if expr="enable_extensions_core">
type WebViewType = chrome.webviewTag.WebView;
// </if>
// <if expr="not enable_extensions_core">
type WebViewType = SlimWebViewElement;
// </if>let's move this to webview.ts
webview.ts isn't in a common location.
But I decided to add a shared/web_view_type.ts library.
// <if expr="enable_extensions_core">
const webview =
document.createElement('webview') as chrome.webviewTag.WebView;
// </if>
// <if expr="not enable_extensions_core">
const webview = document.createElement('slim-webview');
// </if>suggest moving to webview.ts, as a createWebview() function.
Done
// <if expr="enable_extensions_core">
this.webview =
document.createElement('webview') as chrome.webviewTag.WebView;
// </if>
// <if expr="not enable_extensions_core">
this.webview = document.createElement('slim-webview');
// </if>let's have a createWebview function
Done
i really don't like the preprocessor if statements mixed in with code. Here, could we do something like:
getFullWebview(): chrome.webviewTag.WebView | undefined {
// <if expr=...>
...
// </if>
}then later:
const fullWebview = this.getFullWebview();
if (fullWebview) {
// register the header injector
}This should lead to fewer "// if" blocks.
Now only the web_view_type.js has preprocessors. Everything here is pure ts.
build_webui("build") {Optional: I think these resources could live alongside the rest of the implementation in components. We just wouldn't be able to assert on the extensions build flag.
I initially had them there, but I ran into some problems trying to load the files. I ended up following this https://chromium.googlesource.com/chromium/src/+/HEAD/docs/webui/webui_code_sharing.md
I should still be able to move them now that I know all the necessary pieces, if you think that's best.
iframeElement: HTMLIFrameElement;Optional: Depending on how closely you want to stick to acting like a <webview>, make the internal iframe private and encapsulate it in a closed shadow dom.
I'll leave it private for now. I think once I'm properly rendering using the Lit APIs, it will be shadowed by default https://lit.dev/docs/components/shadow-dom/
// GuestViewInternalCustomBindings::RegisterView().nit: This is referring to the extensions impl.
Fixed
import("//extensions/buildflags/buildflags.gni")Both of these are layering violations.
Moved to chrome/renderer
"//components/guest_view/common:mojom",This is a layering violation. content can't depend on this.
Done
WebUIExtension::Install(frame_, enabled_bindings_);If the bindings checks are all being done within this function, perhaps make that clearer by renaming this to MaybeInstall or something like that.
Reverted, since I'm no longer modifying this file.
guest_view::SlimWebViewBindings::Install(isolate, context, chrome,As mentioned above, content can't depend on guest view. This will need to be done through a delegate of some sort. ContentRendererClient perhaps?
- Dan Harrington
- Kevin McNee
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. |
Dan Harrington (Gerrit)
Dan Harrington voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dan Harrington
| Code-Review | +1 |
1 comment
}
// <if expr="enable_extensions_core">
type WebViewType = chrome.webviewTag.WebView;
// </if>
// <if expr="not enable_extensions_core">
type WebViewType = SlimWebViewElement;
// </if>Aldo Culquicondorlet's move this to webview.ts
webview.ts isn't in a common location.
But I decided to add a shared/web_view_type.ts library.
- Aldo Culquicondor
- Kevin McNee
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. |
Kevin McNee (Gerrit)
Kevin McNee added 5 comments![]( "Open in Gerrit")
if (chrome.webviewTag) {This only exists as a ts definition and would be undefined at runtime [1]. Check for window.WebView instead. (Assuming this is feature detection of the full webview tag)
return 'request' in webview;nit: Similarly, I think something like `webview.constructor === window.WebView` would be more direct.
build_webui("build") {Aldo CulquicondorOptional: I think these resources could live alongside the rest of the implementation in components. We just wouldn't be able to assert on the extensions build flag.
I initially had them there, but I ran into some problems trying to load the files. I ended up following this https://chromium.googlesource.com/chromium/src/+/HEAD/docs/webui/webui_code_sharing.md
I should still be able to move them now that I know all the necessary pieces, if you think that's best.
No strong preference. Either is fine with me.
#include "v8/include/v8-forward.h"nit: Includes appear unused.
static base::NoDestructor<This is being stored in a way that's not scoped to the render frame.
We can probably do the same transformation as for the webui case here [1] and bind it on demand.
Related details
- Aldo Culquicondor
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. |