Webium: Add wpt test support for webui browser and a basic test [chromium/src : main]
Liang Zhao (Gerrit)
Message from Liang Zhao![]( "Open in Gerrit")
Set Ready For Review
Related details
- Liang Zhao
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liang Zhao (Gerrit)
Liang Zhao added 1 comment![]( "Open in Gerrit")
Keren, could you review the change first before I add other code owners?
Note that the test failure of ActorMouseMoveToolBrowserTest.MouseMoveTool_Events/Disabled should be unrelated to this change. I could not repro it and previous test run with the same code change didn't fail the test.
Related details
- Keren Zhu
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keren Zhu (Gerrit)
Keren Zhu added 1 comment![]( "Open in Gerrit")
virtual std::string GetTargetType(WebContents* web_contents);Can we update GetTargetType() to return kBrowser for the top-chrome UI? If not, do you know what will break?
Related details
- Liang Zhao
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liang Zhao (Gerrit)
Liang Zhao added 1 comment![]( "Open in Gerrit")
virtual std::string GetTargetType(WebContents* web_contents);Can we update GetTargetType() to return kBrowser for the top-chrome UI? If not, do you know what will break?
I thought about that, but decided not to do that. We probably should add code owners for comments.
Hiding "Browser" targets should be doable with modifying GetTargetType().
Getting RenderFrameDevToolsAgentHost::GetType() and RenderFrameDevToolsAgentHost::GetParentId() to work as expected would require different handling of guest views in them and the some dependency on what GetTargetType() would return for guest view contents and use that knowledge to differentiate tab and guest view contents, like if GetTargetType() returns Page, then it is Page and not guest view. But if the guest view happens to be used to host pages from an extension, then it would be hard to control what would be returned.
Introducing a new target type for WebContents (not for main frame of it) with Browser and Tab type seems more reliable to me.
Related details
- Keren Zhu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liang Zhao (Gerrit)
Liang Zhao added 1 comment![]( "Open in Gerrit")
virtual std::string GetTargetType(WebContents* web_contents);Liang ZhaoCan we update GetTargetType() to return kBrowser for the top-chrome UI? If not, do you know what will break?
I thought about that, but decided not to do that. We probably should add code owners for comments.
Hiding "Browser" targets should be doable with modifying GetTargetType().
Getting RenderFrameDevToolsAgentHost::GetType() and RenderFrameDevToolsAgentHost::GetParentId() to work as expected would require different handling of guest views in them and the some dependency on what GetTargetType() would return for guest view contents and use that knowledge to differentiate tab and guest view contents, like if GetTargetType() returns Page, then it is Page and not guest view. But if the guest view happens to be used to host pages from an extension, then it would be hard to control what would be returned.
Introducing a new target type for WebContents (not for main frame of it) with Browser and Tab type seems more reliable to me.
Thought about this overnight, it might be doable.
Existing code does not return Tab or Browser type from GetTargetType(), so we can change GetTargetType() to return them for webui browser ui and tab, and update the current caller to use the return value of GetTargetType() the same way as GetLogicTargetType(), and map the logic Browser and Tab type to Page when using it as the main frame target type.
I'll update the code to do that.
Liang Zhao (Gerrit)
Liang Zhao added 1 comment![]( "Open in Gerrit")
virtual std::string GetTargetType(WebContents* web_contents);Liang ZhaoCan we update GetTargetType() to return kBrowser for the top-chrome UI? If not, do you know what will break?
Liang ZhaoI thought about that, but decided not to do that. We probably should add code owners for comments.
Hiding "Browser" targets should be doable with modifying GetTargetType().
Getting RenderFrameDevToolsAgentHost::GetType() and RenderFrameDevToolsAgentHost::GetParentId() to work as expected would require different handling of guest views in them and the some dependency on what GetTargetType() would return for guest view contents and use that knowledge to differentiate tab and guest view contents, like if GetTargetType() returns Page, then it is Page and not guest view. But if the guest view happens to be used to host pages from an extension, then it would be hard to control what would be returned.
Introducing a new target type for WebContents (not for main frame of it) with Browser and Tab type seems more reliable to me.
Thought about this overnight, it might be doable.
Existing code does not return Tab or Browser type from GetTargetType(), so we can change GetTargetType() to return them for webui browser ui and tab, and update the current caller to use the return value of GetTargetType() the same way as GetLogicTargetType(), and map the logic Browser and Tab type to Page when using it as the main frame target type.
I'll update the code to do that.
Merged logic target type into GetTargetType()
Related details
- Keren Zhu
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keren Zhu (Gerrit)
Keren Zhu added 9 comments![]( "Open in Gerrit")
parameter for webui browser to report logic target type for WebContentsnit: "logic" is a noun. This should be "logical".
web_contents->GetUserData(
webui_browser::GetWebShellWebContentsUserDataKey())) {Instead of using the user data as a proxy, can we add a helper function for this check? e.g. webui_browser::IsBrowserUIWebContents()?
class WebUIBrowserTest : public InProcessBrowserTest,Can we make a new test harness for devtools? It can be a subclass of WebUIBrowserTest.
// Verify DevTools target types.Can we make a new test for devtools targets? Each test should focus on one capability / feature.
// Only expect targets for tab WebContents and its main frame.
EXPECT_EQ(hosts.size(), 2U);
EXPECT_EQ(tab_count, 1);
EXPECT_EQ(page_count, 1);q: looks like the browser ui WebContents is not a DevTools host. Can it still be inspected by DevTools?
const base::FeatureParam<bool> kWebiumReportLogicDevToolsTargetType{Can we add a description of this parameter?
if (delegate && (delegate->GetTargetType(wc) == kTypeBrowser)) {If I understanding correctly this is excluding the browser ui from DevTools target list. Will the browser ui still be inspectable?
if ((type == kTypeTab) || (type == kTypeBrowser)) {
return kTypePage;I have trouble understanding this overriding. Can you explain the logic?
virtual std::string GetTargetType(WebContents* web_contents);Liang ZhaoCan we update GetTargetType() to return kBrowser for the top-chrome UI? If not, do you know what will break?
Liang ZhaoI thought about that, but decided not to do that. We probably should add code owners for comments.
Hiding "Browser" targets should be doable with modifying GetTargetType().
Getting RenderFrameDevToolsAgentHost::GetType() and RenderFrameDevToolsAgentHost::GetParentId() to work as expected would require different handling of guest views in them and the some dependency on what GetTargetType() would return for guest view contents and use that knowledge to differentiate tab and guest view contents, like if GetTargetType() returns Page, then it is Page and not guest view. But if the guest view happens to be used to host pages from an extension, then it would be hard to control what would be returned.
Introducing a new target type for WebContents (not for main frame of it) with Browser and Tab type seems more reliable to me.
Liang ZhaoThought about this overnight, it might be doable.
Existing code does not return Tab or Browser type from GetTargetType(), so we can change GetTargetType() to return them for webui browser ui and tab, and update the current caller to use the return value of GetTargetType() the same way as GetLogicTargetType(), and map the logic Browser and Tab type to Page when using it as the main frame target type.
I'll update the code to do that.
Merged logic target type into GetTargetType()
Thank you for working on this! I am not too familiar with DevTools and it took me more time than expected to review.
Hiding "Browser" targets should be doable with modifying GetTargetType().
It is not obvious to me why we want to hide "Browser" targets. Can you help me understand it?
Related details
- Dmitry Gozman
- Liang Zhao
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
1 comment
third_party/blink/web_tests/VirtualTestSuites LGTM
Related details
- Dmitry Gozman
- Liang Zhao
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liang Zhao (Gerrit)
Liang Zhao added 9 comments![]( "Open in Gerrit")
parameter for webui browser to report logic target type for WebContentsnit: "logic" is a noun. This should be "logical".
Done
web_contents->GetUserData(
webui_browser::GetWebShellWebContentsUserDataKey())) {Instead of using the user data as a proxy, can we add a helper function for this check? e.g. webui_browser::IsBrowserUIWebContents()?
Done
Can we make a new test harness for devtools? It can be a subclass of WebUIBrowserTest.
Done
Can we make a new test for devtools targets? Each test should focus on one capability / feature.
Done
// Only expect targets for tab WebContents and its main frame.
EXPECT_EQ(hosts.size(), 2U);
EXPECT_EQ(tab_count, 1);
EXPECT_EQ(page_count, 1);q: looks like the browser ui WebContents is not a DevTools host. Can it still be inspected by DevTools?
Yes, it can still be inspected, like by right click on the context menu. That's directly creating agent host from a WebContents. This change only makes not visible from enumeration.
const base::FeatureParam<bool> kWebiumReportLogicDevToolsTargetType{Can we add a description of this parameter?
Done
if (delegate && (delegate->GetTargetType(wc) == kTypeBrowser)) {If I understanding correctly this is excluding the browser ui from DevTools target list. Will the browser ui still be inspectable?
It is still inspectable, like from context menu.
if ((type == kTypeTab) || (type == kTypeBrowser)) {
return kTypePage;I have trouble understanding this overriding. Can you explain the logic?
Added comment: // kTypeTab is the type for WebContentsDevToolsAgentHost and kTypeBrowser is for BrowserDevToolsAgentHost. Replace these logic types with kTypePage which is a proper for RenderFrameDevToolsAgentHost.
virtual std::string GetTargetType(WebContents* web_contents);Liang ZhaoCan we update GetTargetType() to return kBrowser for the top-chrome UI? If not, do you know what will break?
Liang ZhaoI thought about that, but decided not to do that. We probably should add code owners for comments.
Hiding "Browser" targets should be doable with modifying GetTargetType().
Getting RenderFrameDevToolsAgentHost::GetType() and RenderFrameDevToolsAgentHost::GetParentId() to work as expected would require different handling of guest views in them and the some dependency on what GetTargetType() would return for guest view contents and use that knowledge to differentiate tab and guest view contents, like if GetTargetType() returns Page, then it is Page and not guest view. But if the guest view happens to be used to host pages from an extension, then it would be hard to control what would be returned.
Introducing a new target type for WebContents (not for main frame of it) with Browser and Tab type seems more reliable to me.
Liang ZhaoThought about this overnight, it might be doable.
Existing code does not return Tab or Browser type from GetTargetType(), so we can change GetTargetType() to return them for webui browser ui and tab, and update the current caller to use the return value of GetTargetType() the same way as GetLogicTargetType(), and map the logic Browser and Tab type to Page when using it as the main frame target type.
I'll update the code to do that.
Keren ZhuMerged logic target type into GetTargetType()
Thank you for working on this! I am not too familiar with DevTools and it took me more time than expected to review.
Hiding "Browser" targets should be doable with modifying GetTargetType().
It is not obvious to me why we want to hide "Browser" targets. Can you help me understand it?
Browser means BrowserDevToolsAgentHost with different supported methods. As the webui browser web page is not real BrowserDevToolsAgentHost that supports those operations, we just hide it. The real BrowserDevToolsAgentHost is still there and reported.
Related details
- Dmitry Gozman
- Keren Zhu
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keren Zhu (Gerrit)
Keren Zhu added 3 comments![]( "Open in Gerrit")
// Only expect targets for tab WebContents and its main frame.
EXPECT_EQ(hosts.size(), 2U);
EXPECT_EQ(tab_count, 1);
EXPECT_EQ(page_count, 1);Liang Zhaoq: looks like the browser ui WebContents is not a DevTools host. Can it still be inspected by DevTools?
Yes, it can still be inspected, like by right click on the context menu. That's directly creating agent host from a WebContents. This change only makes not visible from enumeration.
Does it mean that it does not show up from chrome://inspect?
if ((type == kTypeTab) || (type == kTypeBrowser)) {
return kTypePage;Liang ZhaoI have trouble understanding this overriding. Can you explain the logic?
Added comment: // kTypeTab is the type for WebContentsDevToolsAgentHost and kTypeBrowser is for BrowserDevToolsAgentHost. Replace these logic types with kTypePage which is a proper for RenderFrameDevToolsAgentHost.
Make sense to me. I was not aware that the difference between WebContentsDevToolsAgentHost and BrowserDevToolsAgentHost.
virtual std::string GetTargetType(WebContents* web_contents);Liang ZhaoCan we update GetTargetType() to return kBrowser for the top-chrome UI? If not, do you know what will break?
Liang ZhaoI thought about that, but decided not to do that. We probably should add code owners for comments.
Hiding "Browser" targets should be doable with modifying GetTargetType().
Getting RenderFrameDevToolsAgentHost::GetType() and RenderFrameDevToolsAgentHost::GetParentId() to work as expected would require different handling of guest views in them and the some dependency on what GetTargetType() would return for guest view contents and use that knowledge to differentiate tab and guest view contents, like if GetTargetType() returns Page, then it is Page and not guest view. But if the guest view happens to be used to host pages from an extension, then it would be hard to control what would be returned.
Introducing a new target type for WebContents (not for main frame of it) with Browser and Tab type seems more reliable to me.
Liang ZhaoThought about this overnight, it might be doable.
Existing code does not return Tab or Browser type from GetTargetType(), so we can change GetTargetType() to return them for webui browser ui and tab, and update the current caller to use the return value of GetTargetType() the same way as GetLogicTargetType(), and map the logic Browser and Tab type to Page when using it as the main frame target type.
I'll update the code to do that.
Keren ZhuMerged logic target type into GetTargetType()
Liang ZhaoThank you for working on this! I am not too familiar with DevTools and it took me more time than expected to review.
Hiding "Browser" targets should be doable with modifying GetTargetType().
It is not obvious to me why we want to hide "Browser" targets. Can you help me understand it?
Browser means BrowserDevToolsAgentHost with different supported methods. As the webui browser web page is not real BrowserDevToolsAgentHost that supports those operations, we just hide it. The real BrowserDevToolsAgentHost is still there and reported.
Can you check if my understanding is correct? kTypeBrowser type DevTools host is the entry point for chrome driver. It reports other inspactable targets in the browser (e.g. pages, service workers, etc). Therefore the browser ui WebContents shouldn't be reported as the Browser type.
WDYT of introducing a new kTypeBrowserUI type? The overriding of kTypeTab and kTypeBrowserUI to kPage in `RenderFrameDevToolsAgentHost::GetType()` will become reasonable, since both Tab and BrowserUI are web pages.
Related details
- Dmitry Gozman
- Liang Zhao
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liang Zhao (Gerrit)
Liang Zhao added 2 comments![]( "Open in Gerrit")
// Only expect targets for tab WebContents and its main frame.
EXPECT_EQ(hosts.size(), 2U);
EXPECT_EQ(tab_count, 1);
EXPECT_EQ(page_count, 1);Liang Zhaoq: looks like the browser ui WebContents is not a DevTools host. Can it still be inspected by DevTools?
Keren ZhuYes, it can still be inspected, like by right click on the context menu. That's directly creating agent host from a WebContents. This change only makes not visible from enumeration.
Does it mean that it does not show up from chrome://inspect?
That is correct. It will not show up from chrome://inspect.
virtual std::string GetTargetType(WebContents* web_contents);Liang ZhaoCan we update GetTargetType() to return kBrowser for the top-chrome UI? If not, do you know what will break?
Liang ZhaoI thought about that, but decided not to do that. We probably should add code owners for comments.
Hiding "Browser" targets should be doable with modifying GetTargetType().
Getting RenderFrameDevToolsAgentHost::GetType() and RenderFrameDevToolsAgentHost::GetParentId() to work as expected would require different handling of guest views in them and the some dependency on what GetTargetType() would return for guest view contents and use that knowledge to differentiate tab and guest view contents, like if GetTargetType() returns Page, then it is Page and not guest view. But if the guest view happens to be used to host pages from an extension, then it would be hard to control what would be returned.
Introducing a new target type for WebContents (not for main frame of it) with Browser and Tab type seems more reliable to me.
Liang ZhaoThought about this overnight, it might be doable.
Existing code does not return Tab or Browser type from GetTargetType(), so we can change GetTargetType() to return them for webui browser ui and tab, and update the current caller to use the return value of GetTargetType() the same way as GetLogicTargetType(), and map the logic Browser and Tab type to Page when using it as the main frame target type.
I'll update the code to do that.
Keren ZhuMerged logic target type into GetTargetType()
Liang ZhaoThank you for working on this! I am not too familiar with DevTools and it took me more time than expected to review.
Hiding "Browser" targets should be doable with modifying GetTargetType().
It is not obvious to me why we want to hide "Browser" targets. Can you help me understand it?
Keren ZhuBrowser means BrowserDevToolsAgentHost with different supported methods. As the webui browser web page is not real BrowserDevToolsAgentHost that supports those operations, we just hide it. The real BrowserDevToolsAgentHost is still there and reported.
Can you check if my understanding is correct? kTypeBrowser type DevTools host is the entry point for chrome driver. It reports other inspactable targets in the browser (e.g. pages, service workers, etc). Therefore the browser ui WebContents shouldn't be reported as the Browser type.
WDYT of introducing a new kTypeBrowserUI type? The overriding of kTypeTab and kTypeBrowserUI to kPage in `RenderFrameDevToolsAgentHost::GetType()` will become reasonable, since both Tab and BrowserUI are web pages.
- Dmitry Gozman
- Keren Zhu
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
1 comment
third_party/blink/web_tests/VirtualTestSuites still LGTM
Related details
- Dmitry Gozman
- Keren Zhu
- Liang Zhao
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dmitry Gozman (Gerrit)
Dmitry Gozman added 1 comment![]( "Open in Gerrit")
It seems to me that the most logical way to report CDP targets in this case is:
- browser_ui target
- tab target for each tab
- basically, nothing changes compared to non-webui browser, except for an additional target being reported
Overall, the treatment for browser_ui target should be very similar to tab targets. For example, exclude it by default in the target filter, do not report it to extensions, show it on chrome://inspect page, etc.
I don't think we need a feature for this - we should figure out the behavior once and stick to it.
Adding caseq@ who worked on tab targets for his input.
Related details
- Andrey Kosyakov
- Keren Zhu
- Liang Zhao
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrey Kosyakov (Gerrit)
Andrey Kosyakov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Andrey Kosyakov
| Code-Review | -1 |
1 comment
+1 to Dmitry says, I don't think this is a step in the right direction. I don't think working around undesired chromedriver logic by adding quirks to chrome is the right way to solve it. +ale...@chromium.org who may help us solve this in the chromedriver land. From what I can see, right now dom UI pages are consistently reported as a tab target and a page target, same as regular pages. I think this is correct behavior. If you're observing a different behavior, please describe the scenario in details.
Related details
- Alex N. Jose
- Keren Zhu
- Liang Zhao
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. |
Liang Zhao (Gerrit)
Liang Zhao added 1 comment![]( "Open in Gerrit")
+1 to Dmitry says, I don't think this is a step in the right direction. I don't think working around undesired chromedriver logic by adding quirks to chrome is the right way to solve it. +ale...@chromium.org who may help us solve this in the chromedriver land. From what I can see, right now dom UI pages are consistently reported as a tab target and a page target, same as regular pages. I think this is correct behavior. If you're observing a different behavior, please describe the scenario in details.
Thanks for the advice.
I also thought about what the desired behavior should be, but felt that we might want different behavior for different audiences and therefore a feature parameter to toggle.
For normal web developers, we probably don't want to expose our internal browser_ui targets which is part of the chrome browser itself.
For chrome developers, we would want to provide the "physical" targets without any alteration.
Currently, chrome://inspect/#pages shows chrome://webui-browser/ as the top with sites in tabs listed as children. The top Page (chrome://webui-browser/) is what we considered browser UI and should not be interested by normal web developer. For normal browser, the tabs are listed as top level pages (with possible first level child frame targets).
For wpt test, as long as browser_ui frame are not reported as normal Page and browser_ui WebContents is not reported as normal Tab, wpt tests will work. It seems the suggestion is to hide browser_ui WebContents target and report its frame target as of browser_ui type. That would work for WPT.
So, it is actually more about how to handle browser_ui frame targets.
Related details
- Alex N. Jose
- Andrey Kosyakov
- Keren Zhu
Liang Zhao (Gerrit)
Liang Zhao added 1 comment![]( "Open in Gerrit")
Liang Zhao+1 to Dmitry says, I don't think this is a step in the right direction. I don't think working around undesired chromedriver logic by adding quirks to chrome is the right way to solve it. +ale...@chromium.org who may help us solve this in the chromedriver land. From what I can see, right now dom UI pages are consistently reported as a tab target and a page target, same as regular pages. I think this is correct behavior. If you're observing a different behavior, please describe the scenario in details.
Thanks for the advice.
I also thought about what the desired behavior should be, but felt that we might want different behavior for different audiences and therefore a feature parameter to toggle.
For normal web developers, we probably don't want to expose our internal browser_ui targets which is part of the chrome browser itself.
For chrome developers, we would want to provide the "physical" targets without any alteration.
Currently, chrome://inspect/#pages shows chrome://webui-browser/ as the top with sites in tabs listed as children. The top Page (chrome://webui-browser/) is what we considered browser UI and should not be interested by normal web developer. For normal browser, the tabs are listed as top level pages (with possible first level child frame targets).
For wpt test, as long as browser_ui frame are not reported as normal Page and browser_ui WebContents is not reported as normal Tab, wpt tests will work. It seems the suggestion is to hide browser_ui WebContents target and report its frame target as of browser_ui type. That would work for WPT.
So, it is actually more about how to handle browser_ui frame targets.
I assume that dom UI page refers to Elements panel when inspecting the site in tab, the current DevTools UI is working as expected and this change would not change that behavior. That's the behavior of inspecting a specific WebContents, not related to target enumeration behavior.
Andrey Kosyakov (Gerrit)
Andrey Kosyakov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Andrey Kosyakov
| Code-Review | +0 |
1 comment
Liang Zhao+1 to Dmitry says, I don't think this is a step in the right direction. I don't think working around undesired chromedriver logic by adding quirks to chrome is the right way to solve it. +ale...@chromium.org who may help us solve this in the chromedriver land. From what I can see, right now dom UI pages are consistently reported as a tab target and a page target, same as regular pages. I think this is correct behavior. If you're observing a different behavior, please describe the scenario in details.
Liang ZhaoThanks for the advice.
I also thought about what the desired behavior should be, but felt that we might want different behavior for different audiences and therefore a feature parameter to toggle.
For normal web developers, we probably don't want to expose our internal browser_ui targets which is part of the chrome browser itself.
For chrome developers, we would want to provide the "physical" targets without any alteration.
Currently, chrome://inspect/#pages shows chrome://webui-browser/ as the top with sites in tabs listed as children. The top Page (chrome://webui-browser/) is what we considered browser UI and should not be interested by normal web developer. For normal browser, the tabs are listed as top level pages (with possible first level child frame targets).
For wpt test, as long as browser_ui frame are not reported as normal Page and browser_ui WebContents is not reported as normal Tab, wpt tests will work. It seems the suggestion is to hide browser_ui WebContents target and report its frame target as of browser_ui type. That would work for WPT.
So, it is actually more about how to handle browser_ui frame targets.
I assume that dom UI page refers to Elements panel when inspecting the site in tab, the current DevTools UI is working as expected and this change would not change that behavior. That's the behavior of inspecting a specific WebContents, not related to target enumeration behavior.
I think we already have precedent for exposing our guts (i.e. dom UI pages like settings), I don't think webium should be different. Even if we decide that it should, this should be then implemented just as hiding certain targets rather than changing their types.
I played with it a bit now and I can see the problem. So my suggestion is:
- keep the dedicated type for the briwser_ui;
- make sure regular pages are reported as `page` rather than a `webview`;
- do so unconditoinally (I doubt there's a merit of switching target types upon a feature flag, and it sounds very confusing).
Does this make sense?
Related details
- Alex N. Jose
- Keren Zhu
- Liang Zhao
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. |
Liang Zhao (Gerrit)
Liang Zhao added 1 comment![]( "Open in Gerrit")
Liang Zhao+1 to Dmitry says, I don't think this is a step in the right direction. I don't think working around undesired chromedriver logic by adding quirks to chrome is the right way to solve it. +ale...@chromium.org who may help us solve this in the chromedriver land. From what I can see, right now dom UI pages are consistently reported as a tab target and a page target, same as regular pages. I think this is correct behavior. If you're observing a different behavior, please describe the scenario in details.
Liang ZhaoThanks for the advice.
I also thought about what the desired behavior should be, but felt that we might want different behavior for different audiences and therefore a feature parameter to toggle.
For normal web developers, we probably don't want to expose our internal browser_ui targets which is part of the chrome browser itself.
For chrome developers, we would want to provide the "physical" targets without any alteration.
Currently, chrome://inspect/#pages shows chrome://webui-browser/ as the top with sites in tabs listed as children. The top Page (chrome://webui-browser/) is what we considered browser UI and should not be interested by normal web developer. For normal browser, the tabs are listed as top level pages (with possible first level child frame targets).
For wpt test, as long as browser_ui frame are not reported as normal Page and browser_ui WebContents is not reported as normal Tab, wpt tests will work. It seems the suggestion is to hide browser_ui WebContents target and report its frame target as of browser_ui type. That would work for WPT.
So, it is actually more about how to handle browser_ui frame targets.
Andrey KosyakovI assume that dom UI page refers to Elements panel when inspecting the site in tab, the current DevTools UI is working as expected and this change would not change that behavior. That's the behavior of inspecting a specific WebContents, not related to target enumeration behavior.
I think we already have precedent for exposing our guts (i.e. dom UI pages like settings), I don't think webium should be different. Even if we decide that it should, this should be then implemented just as hiding certain targets rather than changing their types.
I played with it a bit now and I can see the problem. So my suggestion is:
- keep the dedicated type for the briwser_ui;
- make sure regular pages are reported as `page` rather than a `webview`;
- do so unconditoinally (I doubt there's a merit of switching target types upon a feature flag, and it sounds very confusing).
Does this make sense?
That sounds good to me. Let me update the code and play around it to see whether I understand the suggestion correctly and whether there is any other issues with the design.
Specifically, I plan to hide browser ui WebContents target and expose its frame target as of browser_ui type. While there is a need to debug browser ui frame, we don't have a scenario that requires interacting with browser ui WebContents target. If we really want to expose browser ui WebContents, it would have to be of another new type so that we can differentiate it from browser ui frame target and normal tab WebContents target.
The end result is that chrome://inspect#page would show the same site list as normal browser, and the browser ui frame target (chrome://webui-browser/) will be shown in chrome://inspect/#other. We might have to make some change to make "Inspect" from chrome://inspect/#other for the new browser_ui works. It currently doesn't seem to work.
I will also remove the feature parameter.
Related details
- Alex N. Jose
- Andrey Kosyakov
- Keren Zhu
Liang Zhao (Gerrit)
Liang Zhao added 1 comment![]( "Open in Gerrit")
Liang Zhao+1 to Dmitry says, I don't think this is a step in the right direction. I don't think working around undesired chromedriver logic by adding quirks to chrome is the right way to solve it. +ale...@chromium.org who may help us solve this in the chromedriver land. From what I can see, right now dom UI pages are consistently reported as a tab target and a page target, same as regular pages. I think this is correct behavior. If you're observing a different behavior, please describe the scenario in details.
Liang ZhaoThanks for the advice.
I also thought about what the desired behavior should be, but felt that we might want different behavior for different audiences and therefore a feature parameter to toggle.
For normal web developers, we probably don't want to expose our internal browser_ui targets which is part of the chrome browser itself.
For chrome developers, we would want to provide the "physical" targets without any alteration.
Currently, chrome://inspect/#pages shows chrome://webui-browser/ as the top with sites in tabs listed as children. The top Page (chrome://webui-browser/) is what we considered browser UI and should not be interested by normal web developer. For normal browser, the tabs are listed as top level pages (with possible first level child frame targets).
For wpt test, as long as browser_ui frame are not reported as normal Page and browser_ui WebContents is not reported as normal Tab, wpt tests will work. It seems the suggestion is to hide browser_ui WebContents target and report its frame target as of browser_ui type. That would work for WPT.
So, it is actually more about how to handle browser_ui frame targets.
Andrey KosyakovI assume that dom UI page refers to Elements panel when inspecting the site in tab, the current DevTools UI is working as expected and this change would not change that behavior. That's the behavior of inspecting a specific WebContents, not related to target enumeration behavior.
Liang ZhaoI think we already have precedent for exposing our guts (i.e. dom UI pages like settings), I don't think webium should be different. Even if we decide that it should, this should be then implemented just as hiding certain targets rather than changing their types.
I played with it a bit now and I can see the problem. So my suggestion is:
- keep the dedicated type for the briwser_ui;
- make sure regular pages are reported as `page` rather than a `webview`;
- do so unconditoinally (I doubt there's a merit of switching target types upon a feature flag, and it sounds very confusing).
Does this make sense?
That sounds good to me. Let me update the code and play around it to see whether I understand the suggestion correctly and whether there is any other issues with the design.
Specifically, I plan to hide browser ui WebContents target and expose its frame target as of browser_ui type. While there is a need to debug browser ui frame, we don't have a scenario that requires interacting with browser ui WebContents target. If we really want to expose browser ui WebContents, it would have to be of another new type so that we can differentiate it from browser ui frame target and normal tab WebContents target.
The end result is that chrome://inspect#page would show the same site list as normal browser, and the browser ui frame target (chrome://webui-browser/) will be shown in chrome://inspect/#other. We might have to make some change to make "Inspect" from chrome://inspect/#other for the new browser_ui works. It currently doesn't seem to work.
I will also remove the feature parameter.
It seems that introducing a new type and treat it as inspectable as Page requires more changes. If I report browser_ui as the type for the frame, I am not able to inpsect it, the DevTools window will open, but the Element panel is empty and the page url is not shown in the DevTools window title.
Searching DevTools code, it seems that there is a concept of [IsPageTarget](https://source.chromium.org/chromium/chromium/src/+/main:third_party/devtools-frontend/src/front_end/third_party/puppeteer/package/lib/es5-iife/puppeteer-core-browser.js;drc=8fa26aeceaa8bd2e93e399640d38775275cab0dd;l=22652) and all possible target types are all declared in DevTools front end code.
Do we know how to update DevTools front end code to add a new browser_ui type that should be a Page target?
Besides that, I've updated the code to hide browser ui WebContents target and reports browser ui frame target as Page target. It seems that it works reasonable in chrome://inspect and wpt test. With the current code, in chrome://inspect, the browser ui is listed as another top level Page target along with other pages in tabs. Does the current code looks close to what it should be or even OK to go in?
Related details
- Alex N. Jose
- Andrey Kosyakov
- Keren Zhu
- Mason Freed
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. |
Mason Freed (Gerrit)
Mason Freed voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
third_party/blink/web_tests/VirtualTestSuites still LGTM
- Alex N. Jose
- Andrey Kosyakov
- Keren Zhu
- Liang Zhao
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. |
Keren Zhu (Gerrit)
Keren Zhu added 2 comments![]( "Open in Gerrit")
The `IsPageTarget` code is in puppeteer, https://github.com/puppeteer/puppeteer/blame/a95d18496acb5a7cfe1d1752e7e0a17683e60fef/packages/puppeteer-core/src/cdp/Browser.ts#L185. I suspect that this is not the actual code that controls the DevTools behavior.
in chrome://inspect, the browser ui is listed as another top level Page target along with other pages in tabs
This looks good to me.
if (delegate && (delegate->GetTargetType(wc) == kTypeBrowserUI)) {
continue;
}what will happen if kTypeBrowserUI WebContents are included here?
Related details
- Alex N. Jose
- Andrey Kosyakov
- Liang Zhao
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. |
Liang Zhao (Gerrit)
Liang Zhao added 2 comments![]( "Open in Gerrit")
If it feels right that the external behavior of reporting browser ui main frame as another Page, then the only issue is whether we internally report it Page or report as a new "browser_ui" type and update CDP and DevTools front end to handle this new type the same way as Page target.
if (delegate && (delegate->GetTargetType(wc) == kTypeBrowserUI)) {
continue;
}what will happen if kTypeBrowserUI WebContents are included here?
Then the WebContents target will be reported as a Tab target and confuses chrome test driver and WPT test would not work. For WPT test, the test driver wants to close all "other" tabs and drive test tab to test page urls.
Related details
- Alex N. Jose
- Andrey Kosyakov
- Keren Zhu
Liang Zhao (Gerrit)
Liang Zhao added 1 comment![]( "Open in Gerrit")
Looks like the DevTools front end change needed is in [attachedToTarget](https://source.chromium.org/chromium/chromium/src/+/main:third_party/devtools-frontend/src/front_end/core/sdk/ChildTargetManager.ts;drc=a8b1f4dcc18f99034e11c79aa767347f03f512aa;l=179). If I add code to assign type with Type.FRAME for "browser_ui" there, inspecting the browser ui works.
@ca...@chromium.org, do you think that I should first modify DevTools front end code code to treat the new "browser_ui" as a Type.FRAME type, and then continue with this change and report the frame target as browser_ui, or get the current CL in and then add formal support for the new "browser_ui" type in separate CL?
Andrey Kosyakov (Gerrit)
Andrey Kosyakov added 3 comments![]( "Open in Gerrit")
As long as this is necessary to prevent a regression in the front-end, let's land that first. I was under impression that just adding another case label where we compute the capabilities would be sufficient, but it looks like we have some [additional magic](https://source.chromium.org/chromium/chromium/src/+/main:third_party/devtools-frontend/src/front_end/core/sdk/ChildTargetManager.ts;l=156;drc=2de29efe573e2d6cf3831c516cb47772c57e135a;bpv=1;bpt=1) there, so please check with d...@chromium.org, I'm a bit out of date re that logic.
if (should_handle_webui_browser &&Do we actually need this check here? Perhaps `webui_browser::IsBrowserUIWebContents()` would be sufficient?
return should_handle_webui_browser ? DevToolsAgentHost::kTypeTabHmm, that's a bit unexpected. From what I could see, the tab targets were correctly reported with WebUI browser, it's the page targets that were reported as views, isn't that so?
We shouldn't really return kTypeTab here, since this is only called from RenderFrameDevToolsAgentHost::GetTargetType(), whereas the real tab targets only correspond to WebContentsDevToolsAgnetHost (so kTypeTab is hard-coded there).
Related details
- Alex N. Jose
- Keren Zhu
- Liang Zhao
Keren Zhu (Gerrit)
Keren Zhu added 1 comment![]( "Open in Gerrit")
if (delegate && (delegate->GetTargetType(wc) == kTypeBrowserUI)) {
continue;
}Liang Zhaowhat will happen if kTypeBrowserUI WebContents are included here?
Then the WebContents target will be reported as a Tab target and confuses chrome test driver and WPT test would not work. For WPT test, the test driver wants to close all "other" tabs and drive test tab to test page urls.
Thanks for the explanation. This is a very important information. Can we add a comment? This can save people a lot of time.
Related details
- Alex N. Jose
- Liang Zhao
Liang Zhao (Gerrit)
Liang Zhao added 4 comments![]( "Open in Gerrit")
Will get front end change in first. There are indeed 2 ways to get it to work, using url pattern or type. Since we have a new type, using type would be better in this case.
if (should_handle_webui_browser &&Do we actually need this check here? Perhaps `webui_browser::IsBrowserUIWebContents()` would be sufficient?
You are right, IsBrowserUIWebContents() should be sufficient. If we really want to check whether the feature is enabled before checking the WebContents, we can check the feature inside that function. Will change in next update.
return should_handle_webui_browser ? DevToolsAgentHost::kTypeTabHmm, that's a bit unexpected. From what I could see, the tab targets were correctly reported with WebUI browser, it's the page targets that were reported as views, isn't that so?
We shouldn't really return kTypeTab here, since this is only called from RenderFrameDevToolsAgentHost::GetTargetType(), whereas the real tab targets only correspond to WebContentsDevToolsAgnetHost (so kTypeTab is hard-coded there).
This change is indeed something that is not clean. Returning kTypeTab is as a "logical" type that requires the caller to also make some adjustments. Without this change, the frame of the webui browser Tab is reported as kTypeGuest, parenting to the browser_ui WebContents.
This is actually not a surprise, as the webui browser really uses a guest WebContents for the tabs.
After thinking about it more, I think that we don't need this change. We can change the caller to check whether parent WebContents's target type is browser_ui to make the differentiation. If the parent WebContents is browser_ui, we will let the delegate to decide the type and if it is Page instead of WebView, we will set its parentId as empty.
Will make the change in the next update to the CL.
if (delegate && (delegate->GetTargetType(wc) == kTypeBrowserUI)) {
continue;
}Liang Zhaowhat will happen if kTypeBrowserUI WebContents are included here?
Keren ZhuThen the WebContents target will be reported as a Tab target and confuses chrome test driver and WPT test would not work. For WPT test, the test driver wants to close all "other" tabs and drive test tab to test page urls.
Thanks for the explanation. This is a very important information. Can we add a comment? This can save people a lot of time.
- Alex N. Jose
- Andrey Kosyakov
- Keren Zhu
Liang Zhao (Gerrit)
Liang Zhao added 4 comments![]( "Open in Gerrit")
Front end change is in and in main now: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/7018087.
Liang ZhaoDo we actually need this check here? Perhaps `webui_browser::IsBrowserUIWebContents()` would be sufficient?
You are right, IsBrowserUIWebContents() should be sufficient. If we really want to check whether the feature is enabled before checking the WebContents, we can check the feature inside that function. Will change in next update.
Done
return should_handle_webui_browser ? DevToolsAgentHost::kTypeTabLiang ZhaoHmm, that's a bit unexpected. From what I could see, the tab targets were correctly reported with WebUI browser, it's the page targets that were reported as views, isn't that so?
We shouldn't really return kTypeTab here, since this is only called from RenderFrameDevToolsAgentHost::GetTargetType(), whereas the real tab targets only correspond to WebContentsDevToolsAgnetHost (so kTypeTab is hard-coded there).
This change is indeed something that is not clean. Returning kTypeTab is as a "logical" type that requires the caller to also make some adjustments. Without this change, the frame of the webui browser Tab is reported as kTypeGuest, parenting to the browser_ui WebContents.
This is actually not a surprise, as the webui browser really uses a guest WebContents for the tabs.
After thinking about it more, I think that we don't need this change. We can change the caller to check whether parent WebContents's target type is browser_ui to make the differentiation. If the parent WebContents is browser_ui, we will let the delegate to decide the type and if it is Page instead of WebView, we will set its parentId as empty.
Will make the change in the next update to the CL.
Done
if (delegate && (delegate->GetTargetType(wc) == kTypeBrowserUI)) {
continue;
}Liang Zhaowhat will happen if kTypeBrowserUI WebContents are included here?
Keren ZhuThen the WebContents target will be reported as a Tab target and confuses chrome test driver and WPT test would not work. For WPT test, the test driver wants to close all "other" tabs and drive test tab to test page urls.
Liang ZhaoThanks for the explanation. This is a very important information. Can we add a comment? This can save people a lot of time.
Will do in the next update of the change.
- Alex N. Jose
- Andrey Kosyakov
- Keren Zhu
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrey Kosyakov (Gerrit)
Andrey Kosyakov added 5 comments![]( "Open in Gerrit")
mostly looks good, but let's try to deal with layering violations.
"//chrome/browser/ui/webui_browser:feature_flag",no longer needed?
(delegate->GetTargetType(contents) != kTypeBrowserUI)) {nit: can this clause ever be false? can we instead CHECK() this?
if (!type.empty() && (type != kTypeBrowserUI) &&Let's try to have this logic in the delegate instead. `kTypeBrowserUI` does not belong to content/ layer because it's a notion that only exists in a certain embedder (chrome). Compare this to extension-specific types that are being [declared in the delegate as well](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/devtools/chrome_devtools_manager_delegate.cc;l=69;drc=7692ccdc59f703ce9e329a28e8edd8c7384327f8;bpv=1;bpt=1).
if (delegate && (delegate->GetTargetType(wc) == kTypeBrowserUI)) {Similarly to a previous comment, this is a layering violation as well. Is there any other way we can differentiate this one? FWIW, since I wouldn't expect the browser UI to ever navigate, perhaps we can entirely skip creating a WebCotentsDevToolsAgentHost for it (which still leaves a problem of differentiating it from regular WCs in the content layer)?
Related details
- Alex N. Jose
- Keren Zhu
- Liang Zhao
- Mason Freed
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. |
Liang Zhao (Gerrit)
Liang Zhao added 3 comments![]( "Open in Gerrit")
"//chrome/browser/ui/webui_browser:feature_flag",no longer needed?
It is still needed, it is named as feature_flag, but really is for webui_browser.h. Let me rename the build target as headers so that it is not confusing.
(delegate->GetTargetType(contents) != kTypeBrowserUI)) {nit: can this clause ever be false? can we instead CHECK() this?
We currently don't have other guest contents besides the ones for tabs in browser ui top frame, but it is possible for browser ui top frame to use guest contents for things other than tab and that guest contents should be considered as part of browser UI. In that case, we would want to report that guest view as a webview child to the browser UI main frame target.
I actually don't think that we should CHECK.
if (!type.empty() && (type != kTypeBrowserUI) &&Let's try to have this logic in the delegate instead. `kTypeBrowserUI` does not belong to content/ layer because it's a notion that only exists in a certain embedder (chrome). Compare this to extension-specific types that are being [declared in the delegate as well](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/devtools/chrome_devtools_manager_delegate.cc;l=69;drc=7692ccdc59f703ce9e329a28e8edd8c7384327f8;bpv=1;bpt=1).
I have been struggling how to make the layering work and really need advice.
Currently, the type for guest content is decided in content layer without further check with delegate, but for webui browser, we want delegate to override content layer's classification for the guest contents.
For it to work, we would have to move the whole logic to delegates and always let delegate to decide, or have a contract between content layer and delegate for delegate to indicate that it wants to override the content layer classification.
If we move the whole logic to delegates, we would also have to add duplicated code into DevToolsManagerDelegateAndroid to handle the guest contents. And we are introducing a breaking change for other embedders. They also have to update they delegates to have duplicated code to handle the guest content that content layer should be able to handle by default.
For indicating that delegate want to override type for guest content, I struggled with how to express that "delegate want to override" intent. Returning a "logical kTypeTab" from delegate and documenting it in previous versions of this CL was that attempt.
Though not that obvious, we have similar layering issue in GetParentId(),
If we document the implication of returning kTypeBrowserUI from delegate (treat non browser ui children as top level target with type from delegate), would that mitigate layering concern?
What do you think is the best approach?
Related details
- Alex N. Jose
- Andrey Kosyakov
- Keren Zhu
- Mason Freed
Liang Zhao (Gerrit)
Liang Zhao added 1 comment![]( "Open in Gerrit")
if (delegate && (delegate->GetTargetType(wc) == kTypeBrowserUI)) {Similarly to a previous comment, this is a layering violation as well. Is there any other way we can differentiate this one? FWIW, since I wouldn't expect the browser UI to ever navigate, perhaps we can entirely skip creating a WebCotentsDevToolsAgentHost for it (which still leaves a problem of differentiating it from regular WCs in the content layer)?
I feel that we might need a new contract between content layer and delegate to solve the layering issue.
In previous versions of this CL, I tried:
- Adding a new method like GetLogicalTargetType() that can return kTypeBrowser and kTypeTab and documents the implications of returning them
- Allowing GetTargetType to return these "logical" types and document their implications
Both don't feel great.
Liang Zhao (Gerrit)
Liang Zhao added 1 comment![]( "Open in Gerrit")
Liang Zhaono longer needed?
It is still needed, it is named as feature_flag, but really is for webui_browser.h. Let me rename the build target as headers so that it is not confusing.
updated build target name to headers
Related details
- Alex N. Jose
- Andrey Kosyakov
- Keren Zhu
- Mason Freed
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. |
Liang Zhao (Gerrit)
Liang Zhao added 1 comment![]( "Open in Gerrit")
if (delegate && (delegate->GetTargetType(wc) == kTypeBrowserUI)) {Liang ZhaoSimilarly to a previous comment, this is a layering violation as well. Is there any other way we can differentiate this one? FWIW, since I wouldn't expect the browser UI to ever navigate, perhaps we can entirely skip creating a WebCotentsDevToolsAgentHost for it (which still leaves a problem of differentiating it from regular WCs in the content layer)?
I feel that we might need a new contract between content layer and delegate to solve the layering issue.
In previous versions of this CL, I tried:
- Adding a new method like GetLogicalTargetType() that can return kTypeBrowser and kTypeTab and documents the implications of returning them
- Allowing GetTargetType to return these "logical" types and document their implicationsBoth don't feel great.
@ca...@chromium.org, thought about this more, would add a new delegate method work better:
```
// Returns classification override for the `web_contents` about whether it
// should be treated as a Tab target.
// Returns:
// - std::nullopt if the delegate doesn't want to override the default
// behavior, which means a kTypeTab target for WebContents and a frame
// target for its main frame will be reported when enumerating all
// targets.
// - true if the target should be treated as a Tab with no parent even if
// it is an inner WebContents that might be treated as kTypeGuest by
// default. A kTypeTab target for WebContents and a frame target with
// type determined by GetTargetType will be reported when enumerating all
// targets.
// - false if the target should not be treated as a Tab, which means no
// target is reported for the WebContents when enumerating all targets.
// Its main frame target will be reported as normal.
virtual std::optional<bool> ShouldReportAsTabTarget(WebContents* web_contents);
```
We will return true for "tabs", false for browser ui, and std::nullopt otherwise.
For whether we want to prevent creating WebCotentsDevToolsAgentHost entirely, as DevToolsAgentHost::GetOrCreateForTab() is used as GetOrCreateForWebContents when open DevTools Window for a WebContents, if we return null from DevToolsAgentHost::GetOrCreateForTab(), things would be broken.
Mason Freed (Gerrit)
Mason Freed voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
third_party/blink/web_tests/VirtualTestSuites still LGTM
- Alex N. Jose
- Andrey Kosyakov
- Keren Zhu
- Liang Zhao
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. |
Andrey Kosyakov (Gerrit)
Andrey Kosyakov added 1 comment![]( "Open in Gerrit")
Yep, this sounds pretty reasonable to me, let's proceed with this approach!
Related details
- Alex N. Jose
- Keren Zhu
- Liang Zhao
Liang Zhao (Gerrit)
Liang Zhao added 2 comments![]( "Open in Gerrit")
Liang ZhaoLet's try to have this logic in the delegate instead. `kTypeBrowserUI` does not belong to content/ layer because it's a notion that only exists in a certain embedder (chrome). Compare this to extension-specific types that are being [declared in the delegate as well](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/devtools/chrome_devtools_manager_delegate.cc;l=69;drc=7692ccdc59f703ce9e329a28e8edd8c7384327f8;bpv=1;bpt=1).
I have been struggling how to make the layering work and really need advice.
Currently, the type for guest content is decided in content layer without further check with delegate, but for webui browser, we want delegate to override content layer's classification for the guest contents.
For it to work, we would have to move the whole logic to delegates and always let delegate to decide, or have a contract between content layer and delegate for delegate to indicate that it wants to override the content layer classification.
If we move the whole logic to delegates, we would also have to add duplicated code into DevToolsManagerDelegateAndroid to handle the guest contents. And we are introducing a breaking change for other embedders. They also have to update they delegates to have duplicated code to handle the guest content that content layer should be able to handle by default.
For indicating that delegate want to override type for guest content, I struggled with how to express that "delegate want to override" intent. Returning a "logical kTypeTab" from delegate and documenting it in previous versions of this CL was that attempt.
Though not that obvious, we have similar layering issue in GetParentId(),
If we document the implication of returning kTypeBrowserUI from delegate (treat non browser ui children as top level target with type from delegate), would that mitigate layering concern?
What do you think is the best approach?
Updated the code with a ShouldReportAsTabTarget() delegate, which should have addressed the layering concern.
if (delegate && (delegate->GetTargetType(wc) == kTypeBrowserUI)) {- Alex N. Jose
- Andrey Kosyakov
- Keren Zhu
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liang Zhao (Gerrit)
Liang Zhao added 1 comment![]( "Open in Gerrit")
@ag...@chromium.org, could you please review the change to chrome/browser/ui/toasts/BUILD.gn?
@tl...@chromium.org, could you please review the changes to chrome/browser/ui/views/side_panel/BUILD.gn and WebUI browser code?
Related details
- Alex N. Jose
- Alison Gale
- Andrey Kosyakov
- Keren Zhu
- Mason Freed
- Tom Lukaszewicz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrey Kosyakov (Gerrit)
Andrey Kosyakov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Andrey Kosyakov
| Code-Review | +1 |
1 comment
Thanks, lgtm! That new delegate method makes it pretty neat!
Related details
- Alex N. Jose
- Alison Gale
- Keren Zhu
- Liang Zhao
- Mason Freed
- Tom Lukaszewicz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
1 comment
third_party/blink/web_tests/VirtualTestSuites still LGTM
- Alex N. Jose
- Alison Gale
- Keren Zhu
- Liang Zhao
- Tom Lukaszewicz
Liang Zhao (Gerrit)
Liang Zhao added 1 comment![]( "Open in Gerrit")
@dgo...@chromium.org, could you please take another look at the change, especially for the files under content/public/browser?
Related details
- Alison Gale
- Dmitry Gozman
- Keren Zhu
- Tom Lukaszewicz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Lukaszewicz (Gerrit)
Tom Lukaszewicz voted and added 5 comments![]( "Open in Gerrit")
Votes added by Tom Lukaszewicz
| Code-Review | +1 |
5 comments
lgtm! Just a few nits
if (base::Contains(AllTabContentses(), web_contents)) {nit: Prefer `tabs::TabInterface::MaybeGetFromContents(web_contents)`
return IsWebUIBrowserEnabled() &&
(WebUIBrowserWindow::FromWebShellWebContents(web_contents) != nullptr);nit: This can just be
```
return IsWebUIBrowserEnabled() &&
WebUIBrowserWindow::FromWebShellWebContents(web_contents);
```
// For now this is disabled on CrOS since BrowserStatusMonitor/
// AppServiceInstanceRegistryHelper aren't happy with our shutdown deletion
// order of native windows vs. Browser and aren't tracking the switch over
// of views on child guest contents properly.nit: Can we raise a bug for this and tag it as a TODO in this comment?
return "";trivial nit: We typically prefer `std::string()` instead of the `""` literal (here and elsewhere in this method I suppose)
Related details
- Alison Gale
- Dmitry Gozman
- Keren Zhu
- Liang Zhao
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. |