[GreenDevFloaty]: Add WebUI for showing DevTools frontend content. [chromium/src : main]
Finnur Thorarinsson (Gerrit)
Finnur Thorarinsson voted and added 1 comment![]( "Open in Gerrit")
Votes added by Finnur Thorarinsson
| Commit-Queue | +1 |
1 comment
Mind taking a look from a feature perspective?
Related details
- Alex Rudenko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alex Rudenko
| Code-Review | +1 |
1 comment
LGTM (as a prototype, would need to revisit architecture if this is scheduled for launching)
Related details
- Finnur Thorarinsson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| 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")
rfh->ExecuteJavaScript(we would not probably want to do it in production (probably we would need to give the floaty its own CDP connection and maybe a channel to communicate with the main devtools windows)
Related details
- Finnur Thorarinsson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alex Rudenko
| Code-Review | +1 |
1 comment
GetFloatyRegistry()[backend_node_id_] = this;backend_node_id_ would duplicate between renderers so cannot be stored like this for production.
Related details
- Finnur Thorarinsson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Finnur Thorarinsson (Gerrit)
Finnur Thorarinsson added 3 comments![]( "Open in Gerrit")
Andrey, can you give OWNERS approval for:
c/b/ui/webui/devtools/
David, can you give OWNERS approval for:
c/b/r/render_view_context_menu.cc
c/b/ui/webui/BUILD.gn
Please note that this is purely to enable team testing of a prototype. This will not go as-is to production. See note in CL description.
GetFloatyRegistry()[backend_node_id_] = this;backend_node_id_ would duplicate between renderers so cannot be stored like this for production.
Yup, the intent with this is only internal team testing, but this will be addressed if we decide to put this into production. Thanks for the tip!
void RenderViewContextMenu::ExecInspectElementWithGemini() {Just a reminder that this function is guarded by a default-off flag, that we don't intend to turn on in production (ever). This is just for internal testing.
Related details
- Andrey Kosyakov
- David Pennington
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Finnur Thorarinsson (Gerrit)
Finnur Thorarinsson added 1 comment![]( "Open in Gerrit")
Looks like I over-committed on the owners. Switching from Andrey to David, who owns a superset of the files I am requesting owners approval.
Related details
- David Pennington
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Pennington (Gerrit)
David Pennington added 14 comments![]( "Open in Gerrit")
DevToolsFloaty() = default;do we need a default constructor at all? can we just delete it since this is a bag of static methods?
static void Restore(int backend_node_id);what happens if 0 is passed in? lets add some comments.
int backend_node_id = 0);what does a node id of 0 do. can we use std::optional to be more specific?
// Creates and shows the floating window.please put what the params do in the comment for ths method.
class DevToolsFloaty {pelase provide comments for what this class is supposed to be. This seems to be a collection of static methods, so does it need to exist as is.
#include "chrome/browser/profiles/profile.h"you dont need to include profile it can be forward declared.
class DevToolsFloatyWebContentsDelegate : public content::WebContentsDelegate {add comments to these anonymous classes.
GetFloatyRegistry()[backend_node_id_] = this;Finnur Thorarinssonbackend_node_id_ would duplicate between renderers so cannot be stored like this for production.
Yup, the intent with this is only internal team testing, but this will be addressed if we decide to put this into production. Thanks for the tip!
I dont see any code in this CL preventing Devtools floaty from being used in production, is there a plan to gate this code on a build flag or feature flag that is default disabled? if so lets add that in this CL with either a CHECK or preventing the static methods from running
inspected_contents->GetNativeView());i dont see this code anywhere else that special cases this for mac although i just did a quick search. is there any specific reason we need a mac implementation here?
const int offsetY = 175; // TODO(finnur): Remove.constexpr int
also lets not add new TODOs into the codebase without bugs associated.
widget->SetBounds(gfx::Rect(x + offsetX, y + offsetY, 400, 300));values here to constexpr ints
// TOOD(finnur): Send the actual node id.please add a bug.
})())",should this be checked by the security team before we execute arbitrary js strings. I dont see this as a common implementation in our codebase.
Related details
- Finnur Thorarinsson
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. |
Finnur Thorarinsson (Gerrit)
Finnur Thorarinsson added 14 comments![]( "Open in Gerrit")
do we need a default constructor at all? can we just delete it since this is a bag of static methods?
Nope. Gone.
what happens if 0 is passed in? lets add some comments.
Added CHECK.
what does a node id of 0 do. can we use std::optional to be more specific?
This is not necessary. Removed, thanks!
int y,Finnur Thorarinssonoptional: use gfx::Point
Done
please put what the params do in the comment for ths method.
Done
pelase provide comments for what this class is supposed to be. This seems to be a collection of static methods, so does it need to exist as is.
That is correct, it does not need to be a class, so I've converted it.
you dont need to include profile it can be forward declared.
Done
class DevToolsFloatyWebContentsDelegate : public content::WebContentsDelegate {add comments to these anonymous classes.
Done
Finnur Thorarinssonbackend_node_id_ would duplicate between renderers so cannot be stored like this for production.
David PenningtonYup, the intent with this is only internal team testing, but this will be addressed if we decide to put this into production. Thanks for the tip!
I dont see any code in this CL preventing Devtools floaty from being used in production, is there a plan to gate this code on a build flag or feature flag that is default disabled? if so lets add that in this CL with either a CHECK or preventing the static methods from running
I love this question, because it tells me you were looking to catch unintended consequences, which is great.
The function itself is not gated on a feature flag, but the feature flag is in effect because the changes to the context menu only apply if the feature flag is enabled. Therefore, it is not possible to reach this code without the flag.
https://chromium-review.googlesource.com/c/chromium/src/+/7548524/9/chrome/browser/renderer_context_menu/render_view_context_menu.cc#1647
However, it is probably prudent to CHECK on the flag also, so I've added that.
i dont see this code anywhere else that special cases this for mac although i just did a quick search. is there any specific reason we need a mac implementation here?
This is due to a failure on the bots, because the Mac bots don't like nullptr being passed in. I had left my mac at home, so I could not investigate properly, so I special-cased this so I could continue with the CL (Mac isn't my primary dev setup). But, I've added a bug and a comment.
constexpr int
also lets not add new TODOs into the codebase without bugs associated.
Added TODOs.
widget->SetBounds(gfx::Rect(x + offsetX, y + offsetY, 400, 300));values here to constexpr ints
Done
// TOOD(finnur): Send the actual node id.Finnur Thorarinssonplease add a bug.
Oops, did not mean to drop the bug reference. Thanks.
should this be checked by the security team before we execute arbitrary js strings. I dont see this as a common implementation in our codebase.
All of this code should be considered throw-away and will not be used for production. We're creating a series of prototypes that we are checking in behind a flag that we'll never turn on (except devs locally). This allows the wider team to play with the prototype concepts, collaborate and provide feedback -- with the ultimate goal of determining what is worth exploring further or launching into production. When a prototype is deemed worthy of production, we start by ripping it out of the codebase and then proceed with a formal project to launch the feature via the normal (formal) launch process (including Security).
You can look at the go/ link in the CL description for more details on the prototypes and the working procedure.
Related details
- David Pennington
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |