Add `panelId` parameter to `Target.openDevTools` CDP command. [chromium/src : main]
Nikolay Vitkov (Gerrit)
Nikolay Vitkov voted and added 5 comments![]( "Open in Gerrit")
Votes added by Nikolay Vitkov
| Code-Review | +1 |
5 comments
LGTM, with some comments, I think Alex can double check if they are valid
tab_agent_host, nullptr, DevToolsOpenedByAction::kUnknown, panel_id);I think in my original CL, we should have used a new `DevToolsOpenedByAction`, to signify where this is coming from.
std::optional<std::string> panel_id = std::nullopt);Nit: Do we need explicitly add `std::nullopt` here and bellow?
if (panel_id.has_value()) {If we add a new `DevToolsToggleAction` we should move this there. My feeling is that this code block should not be hittable if we try to inspect a element,
just so we add a extra layer of safety.
class DevToolsProtocolTest_OpensDevTools : public DevToolsProtocolTest {Thanks for refactoring this!
Related details
- Alex Rudenko
- Dmitry Gozman
- Liviu Rau
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. |
Liviu Rau (Gerrit)
Liviu Rau voted and added 1 comment![]( "Open in Gerrit")
Votes added by Liviu Rau
| Commit-Queue | +1 |
1 comment
if (panel_id.has_value()) {If we add a new `DevToolsToggleAction` we should move this there. My feeling is that this code block should not be hittable if we try to inspect a element,
just so we add a extra layer of safety.
The names are not amazing here and the 2 action-typed arguments are confusing: DevToolsToggleAction action & DevToolsOpenedByAction toggled_by.
Related details
- Alex Rudenko
- Dmitry Gozman
- Nikolay Vitkov
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. |
Alex Rudenko (Gerrit)
Alex Rudenko added 2 comments![]( "Open in Gerrit")
std::optional<std::string> panel_id) {Let's introduce a struct called DevToolsOptions so that we can extend it later with additional options?
# The id/name of the panel we want DevTools to open in. Currently```suggestion
# The id of the panel we want DevTools to open initially. Currently
```
Related details
- Dmitry Gozman
- Liviu Rau
- Nikolay Vitkov
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. |
Liviu Rau (Gerrit)
Liviu Rau added 1 comment![]( "Open in Gerrit")
# The id/name of the panel we want DevTools to open in. Currently```suggestion
# The id of the panel we want DevTools to open initially. Currently
```
- Alex Rudenko
- Dmitry Gozman
- Nikolay Vitkov
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. |
Nikolay Vitkov (Gerrit)
Nikolay Vitkov added 2 comments![]( "Open in Gerrit")
tab_agent_host, nullptr, DevToolsOpenedByAction::kUnknown, panel_id);I think in my original CL, we should have used a new `DevToolsOpenedByAction`, to signify where this is coming from.
Done
if (panel_id.has_value()) {Liviu RauIf we add a new `DevToolsToggleAction` we should move this there. My feeling is that this code block should not be hittable if we try to inspect a element,
just so we add a extra layer of safety.
The names are not amazing here and the 2 action-typed arguments are confusing: DevToolsToggleAction action & DevToolsOpenedByAction toggled_by.
OK to not block this CL as I think it's not on its path we can move this before the switch and add comment explaining that the `DevToolsToggleAction` should take present over the `panel_id` to make sure things like inspect element are handled as expected, WDYT?
Maybe we can add a test for this as well?
Related details
- Alex Rudenko
- Dmitry Gozman
- Liviu Rau
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. |
Liviu Rau (Gerrit)
Liviu Rau added 3 comments![]( "Open in Gerrit")
Nit: Do we need explicitly add `std::nullopt` here and bellow?
Possible only here, but not below. I would rather not update all call sites.
if (panel_id.has_value()) {Liviu RauIf we add a new `DevToolsToggleAction` we should move this there. My feeling is that this code block should not be hittable if we try to inspect a element,
just so we add a extra layer of safety.
Nikolay VitkovThe names are not amazing here and the 2 action-typed arguments are confusing: DevToolsToggleAction action & DevToolsOpenedByAction toggled_by.
OK to not block this CL as I think it's not on its path we can move this before the switch and add comment explaining that the `DevToolsToggleAction` should take present over the `panel_id` to make sure things like inspect element are handled as expected, WDYT?
Maybe we can add a test for this as well?
Since from Target.OpenDevTools we come in with kShow, I think it is better to handle the panel_id in the that particular case statement. Updated.
std::optional<std::string> panel_id) {Let's introduce a struct called DevToolsOptions so that we can extend it later with additional options?
Where do I place this best? As I've done it here, I am fighting the build system atm, so I am either missing something builder config, or I added this in the wrong place.
Related details
- Alex Rudenko
- Dmitry Gozman
- Nikolay Vitkov
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. |
Liviu Rau (Gerrit)
Liviu Rau added 1 comment![]( "Open in Gerrit")
std::optional<std::string> panel_id) {Liviu RauLet's introduce a struct called DevToolsOptions so that we can extend it later with additional options?
Where do I place this best? As I've done it here, I am fighting the build system atm, so I am either missing something builder config, or I added this in the wrong place.
I think I found a good place for it now.
Related details
- Alex Rudenko
- Dmitry Gozman
- Nikolay Vitkov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nikolay Vitkov (Gerrit)
Nikolay Vitkov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nikolay Vitkov
| Code-Review | +1 |
1 comment
LGTM, differing to Alex in regards to the content::DevToolsOptions changes.
Related details
- Alex Rudenko
- Dmitry Gozman
- Liviu Rau
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")
struct DevToolsOptions {I think this needs to be marked as CONTENT_EXPORT. Not sure about specific guidance about adding this to the content public API so will defer to content/public/browser owners on this. I think Options could be also an inner struct for DevToolsManagerDelegate.
Related details
- Dmitry Gozman
- Liviu Rau
- Nikolay Vitkov
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. |
Liviu Rau (Gerrit)
Liviu Rau added 1 comment![]( "Open in Gerrit")
struct DevToolsOptions {I think this needs to be marked as CONTENT_EXPORT. Not sure about specific guidance about adding this to the content public API so will defer to content/public/browser owners on this. I think Options could be also an inner struct for DevToolsManagerDelegate.
Added CONTENT_EXPORT and I hit another issue. Will move this in DevToolsManagerDelegate.
Related details
- Alex Rudenko
- Dmitry Gozman
- Nikolay Vitkov
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. |
Alex Rudenko (Gerrit)
Alex Rudenko voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alex Rudenko
| Code-Review | +1 |
1 comment
struct DevToolsOptions {Liviu RauI think this needs to be marked as CONTENT_EXPORT. Not sure about specific guidance about adding this to the content public API so will defer to content/public/browser owners on this. I think Options could be also an inner struct for DevToolsManagerDelegate.
Added CONTENT_EXPORT and I hit another issue. Will move this in DevToolsManagerDelegate.
- Dmitry Gozman
- Liviu Rau
- Nikolay Vitkov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nikolay Vitkov (Gerrit)
Nikolay Vitkov voted Code-Review+1![]( "Open in Gerrit")
- Alex Rudenko
- Liviu Rau
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rakina Zata Amni (Gerrit)
Rakina Zata Amni voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Alex Rudenko
- Liviu Rau
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 Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Liviu Rau
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |