kiosk: enable devtools in kiosk mode. [chromium/src : main]

Irina Fedorova (Gerrit)

unread,

Feb 8, 2023, 6:40:52 PM2/8/23

to Polina Bondarenko, asvitkine...@chromium.org, chromeos-kio...@google.com, devtools...@chromium.org, oshima...@chromium.org, Tricium, Ben Franz, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Ben Franz, Polina Bondarenko.

View Change

11 comments:

Commit Message:
- Patch Set #4, Line 11: .
  Could you, please, add more details on how it works with web / chrome apps? And that the window is resizable.
Patchset:
- Patch Set #4:
  Hey Polina,
  Thank you for this CL :)
File ash/wm/workspace/workspace_window_resizer.cc:
- Patch Set #4, Line 549: .
  Maybe we can add mode details why we want to create a resizer only for non-fullscreen kiosk windows and mention devtools?
File chrome/browser/ash/app_mode/kiosk_troubleshooting_tools_browsertest.cc:
- Patch Set #4, Line 54: }
  nit: add empty line
- Patch Set #4, Line 91: DisableDevTools
  Can you also add a test / modify this one to check what happens if we do not manually disable dev tools policy? This will be a check for a default value
- Patch Set #4, Line 112: 1
  Why this value in the previous test is `0`? IIUC because in kiosk-side we allow to open this window, but devtools itself will close it, right? If so, could you, please, add a comment here?
File chrome/browser/chromeos/app_mode/app_session_browser_window_handler.h:
- Patch Set #4, Line 52: controller
  nit: kiosk_troubleshooting_controller
File chrome/browser/chromeos/app_mode/app_session_unittest.cc:
- Patch Set #4, Line 721: AppSessionTroubleshootingTest
  Why did you delete these tests? I understand that new `KioskTroubleshootingToolsTest` cover most of it, but these tests run in both Ash and Lacros and does not depend of web kiosk / chrome app kiosk. And `KioskTroubleshootingToolsTest` covers only web ash kiosk.
  Also in browser tests we more rely on metrics and do not explicitly check `AreKioskTroubleshootingToolsEnabled`.
  I think it make sense to have both unit and browser tests.
File chrome/browser/devtools/devtools_window_testing.cc:
- Patch Set #4, Line 117: The window might be removed upon creation.
  Can you, please, describe why?
File chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:
- Patch Set #4, Line 316: DevToolsWindow
  Would it be safer to check if it's a kiosk session here? IIUC we register this accelerator only for kiosk, but in case it will be somehow registered for user session and this new logic will be executed even if it was not intended.
  And another question: is it true that we do not check for troubleshooting policy here because we rely on devtools policy?
File chrome/browser/ui/views/frame/browser_view.cc:
- Patch Set #4, Line 4393: IDC_DEV_TOOLS
  Are we sure if we actually need this?

To view, visit change 4232543. To unsubscribe, or for help writing mail filters, visit settings.

Ben Franz (Gerrit)

unread,

Feb 9, 2023, 7:08:54 AM2/9/23

to Polina Bondarenko, asvitkine...@chromium.org, chromeos-kio...@google.com, devtools...@chromium.org, oshima...@chromium.org, Tricium, Irina Fedorova, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Polina Bondarenko.

View Change

5 comments:

File chrome/browser/chromeos/app_mode/app_session.cc:
- Patch Set #4, Line 275:
```
     std::make_unique<KioskTroubleshootingController>(
          profile_->GetPrefs(),
          base::BindOnce(&AppSession::ShutdownAppSession,
                         weak_ptr_factory_.GetWeakPtr())));
```
  Interesting. With this ownership, it feels almost like we could create the `KioskTroubleshootingController` inside the `AppSessionBrowserWindowHandler`, as they share the same callback.
  Happy to see where this is going though.
File chrome/browser/chromeos/app_mode/app_session_browser_window_handler.cc:
- Patch Set #4, Line 29: KioskTroubleshootingController
  nit: this ownership model also feels surprising to me, but happy to see how this evolves as we move forward.
- Patch Set #4, Line 60: IsNewBrowserWindowAllowed
  I think i'd move the dev tools check first because it's more specific and we'd want to make sure we get the right UMA metrics. Can they overlap?
- Patch Set #4, Line 71: AreDevtoolsBrowserAllowed
  nit: Hmm, on first glance it doesn't feel obvious that we're checking in here whether the browser is dev tools, sounds like we're only checking in general if dev tools are allowed. Could we either find a better name for this method or move out the part that checks whether the browser is dev tools?
- Patch Set #4, Line 165: AreKioskTroubleshootingToolsEnabled
  I might have misunderstood this: didn't you say we were now looking at a dev tools policy?

To view, visit change 4232543. To unsubscribe, or for help writing mail filters, visit settings.

Polina Bondarenko (Gerrit)

unread,

Feb 10, 2023, 2:59:34 AM2/10/23

to asvitkine...@chromium.org, chromeos-kio...@google.com, devtools...@chromium.org, oshima...@chromium.org

Attention is currently required from: Polina Bondarenko.

Polina Bondarenko uploaded patch set #7 to this change.

View Change

kiosk: enable devtools in kiosk mode.

When kiosk troubleshooting is enabled and devtools are allowed by
policy, devtools should be allowed to be open by a shortcut
(Shift+Ctrl+I).

This works for both web and chrome app kiosks. The devtools window is created movable and resizable.
Note: setting KioskTroubleshootingToolsEnabled policy to true is not sufficient to open devtools in kiosk mode. DeveloperToolsAvailability user policy should allow opening of devtools for the session.

Bug: b/265405355
Bug: b/265405666

Test: browser_tests
Test: ash_unittests
Test: unit_tests
Change-Id: I4e46685daa605a78741552ea7c16bd1acb5f391b
---
M ash/wm/workspace/workspace_window_resizer.cc
M ash/wm/workspace/workspace_window_resizer_unittest.cc
M chrome/browser/app_mode/app_mode_utils.cc
A chrome/browser/ash/app_mode/kiosk_troubleshooting_tools_browsertest.cc
M chrome/browser/chromeos/app_mode/app_session.cc
M chrome/browser/chromeos/app_mode/app_session.h
M chrome/browser/chromeos/app_mode/app_session_browser_window_handler.cc
M chrome/browser/chromeos/app_mode/app_session_browser_window_handler.h
M chrome/browser/chromeos/app_mode/app_session_unittest.cc
M chrome/browser/devtools/devtools_window.cc
M chrome/browser/devtools/devtools_window.h
M chrome/browser/devtools/devtools_window_testing.cc
M chrome/browser/ui/views/apps/chrome_native_app_window_views.cc
M chrome/browser/ui/views/frame/browser_view.cc
M chrome/test/BUILD.gn
M tools/metrics/histograms/enums.xml
16 files changed, 347 insertions(+), 83 deletions(-)

To view, visit change 4232543. To unsubscribe, or for help writing mail filters, visit settings.

Polina Bondarenko (Gerrit)

unread,

Feb 10, 2023, 3:00:11 AM2/10/23

to asvitkine...@chromium.org, chromeos-kio...@google.com, devtools...@chromium.org, oshima...@chromium.org, Tricium, Irina Fedorova, Ben Franz, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Ben Franz, Irina Fedorova.

View Change

14 comments:

Patchset:
- Patch Set #7:
  Thanks for reviewing! Fixed/responded to your comments
File ash/wm/workspace/workspace_window_resizer.cc:

- Patch Set #4, Line 549: .

- Maybe we can add mode details why we want to create a resizer only for non-fullscreen kiosk windows […]
  Done
File chrome/browser/ash/app_mode/kiosk_troubleshooting_tools_browsertest.cc:

- Patch Set #4, Line 54: }
  nit: add empty line

- Done

- Patch Set #4, Line 91: DisableDevTools

- Can you also add a test / modify this one to check what happens if we do not manually disable dev to […]
  Done

- Patch Set #4, Line 112: 1

- Why this value in the previous test is `0`? IIUC because in kiosk-side we allow to open this window, […]
  Done
File chrome/browser/chromeos/app_mode/app_session.cc:

Patch Set #4, Line 275:

     std::make_unique<KioskTroubleshootingController>(
          profile_->GetPrefs(),
          base::BindOnce(&AppSession::ShutdownAppSession,
                         weak_ptr_factory_.GetWeakPtr())));

- Interesting. […]
  Correct. I think it has to be refactored at some point.
  However, there may be a race condition when both KioskTroubleshootingController and AppSessionBrowserWindowHandler want to call this callback during shutdown. It should be handled carefully. Current implementation is safe in this case.
File chrome/browser/chromeos/app_mode/app_session_browser_window_handler.h:

- Patch Set #4, Line 52: controller
  nit: kiosk_troubleshooting_controller

- Done
File chrome/browser/chromeos/app_mode/app_session_browser_window_handler.cc:
- Patch Set #4, Line 60: IsNewBrowserWindowAllowed
  I think i'd move the dev tools check first because it's more specific and we'd want to make sure we […]
  No, they do not overlap. But I agree, moved.

- Patch Set #4, Line 71: AreDevtoolsBrowserAllowed

- nit: Hmm, on first glance it doesn't feel obvious that we're checking in here whether the browser is […]
  Fixed to IsDevToolsAllowedBrowser.

- Patch Set #4, Line 165: AreKioskTroubleshootingToolsEnabled
  I might have misunderstood this: didn't you say we were now looking at a dev tools policy?

- No, this makes two checks:
  1. it's an indeed devtools browser.
  2. the policy KioskRemoteTroubleshootingEnabled is true.
  The devtools policy availability checks are hidden inside devtools window logic. If it's disabled, there is no devtools browser being created. There is a related browser test: KioskTroubleshootingToolsTest.DevToolsDisallowedNoShow
File chrome/browser/chromeos/app_mode/app_session_unittest.cc:

- Patch Set #4, Line 721: AppSessionTroubleshootingTest

- Why did you delete these tests? I understand that new `KioskTroubleshootingToolsTest` cover most of […]
  Yeah, browser agnostic tests sound like a good reason to keep the tests. Done.
  However, I disagree that we need explicitly check AreKioskTroubleshootingToolsEnabled, since these are AppSession tests and we presume troubleshooting tools are controlled by policy. Let's focus on the behavior enabled by policy value, not the exact value returned by AreKioskTroubleshootingToolsEnabled() method.
File chrome/browser/devtools/devtools_window_testing.cc:

- Patch Set #4, Line 117: The window might be removed upon creation.
  Can you, please, describe why?

- Extended the comment.
  This is the case when devtools availability policy allows devtools to be open, but the kiosk troubleshooting tools policy is disabled.
File chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:

- Patch Set #4, Line 316: DevToolsWindow

- Would it be safer to check if it's a kiosk session here? IIUC we register this accelerator only for […]
  If we decide to register this accelerator for normal user session, we do not need am additional kiosk check, since it is expected to be precessed with no issue. The action is standard and intended in this case.
  Yes, we rely on a devtools availability policy, but also close the browser in AppSessionBrowserWindowHandler if kiosk troubleshooting tools are disabled. This additional check is performed ONLY in kiosk mode.
File chrome/browser/ui/views/frame/browser_view.cc:

- Patch Set #4, Line 4393: IDC_DEV_TOOLS
  Are we sure if we actually need this?

- Done

To view, visit change 4232543. To unsubscribe, or for help writing mail filters, visit settings.

Polina Bondarenko (Gerrit)

unread,

Feb 10, 2023, 3:02:33 AM2/10/23

to Danil Somsikov, Ahmed Fakhry, Dana Fried, Alan Cutter, asvitkine...@chromium.org, chromeos-kio...@google.com, devtools...@chromium.org, oshima...@chromium.org, Irina Fedorova, Ben Franz

Attention is currently required from: Ahmed Fakhry, Alan Cutter, Ben Franz, Dana Fried, Danil Somsikov, Irina Fedorova.

Polina Bondarenko would like Danil Somsikov, Ahmed Fakhry, Dana Fried and Alan Cutter to review this change.

To view, visit change 4232543. To unsubscribe, or for help writing mail filters, visit settings.

Polina Bondarenko (Gerrit)

unread,

Feb 10, 2023, 3:03:58 AM2/10/23

to asvitkine...@chromium.org, chromeos-kio...@google.com, devtools...@chromium.org, oshima...@chromium.org, Danil Somsikov, Ahmed Fakhry, Dana Fried, Alan Cutter, Tricium, Irina Fedorova, Ben Franz, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Ahmed Fakhry, Alan Cutter, Ben Franz, Dana Fried, Danil Somsikov, Irina Fedorova.

View Change

1 comment:

Commit Message:

- Patch Set #4, Line 11: .

- Could you, please, add more details on how it works with web / chrome apps? And that the window is r […]
  Done

To view, visit change 4232543. To unsubscribe, or for help writing mail filters, visit settings.

Danil Somsikov (Gerrit)

unread,

Feb 10, 2023, 3:36:21 AM2/10/23

to Polina Bondarenko, asvitkine...@chromium.org, chromeos-kio...@google.com, devtools...@chromium.org, oshima...@chromium.org, Ahmed Fakhry, Dana Fried, Alan Cutter, Tricium, Irina Fedorova, Ben Franz, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Ahmed Fakhry, Alan Cutter, Ben Franz, Dana Fried, Irina Fedorova, Polina Bondarenko.

View Change

2 comments:

File chrome/browser/chromeos/app_mode/app_session_browser_window_handler.cc:
- Patch Set #7, Line 60: if (IsDevToolsAllowedBrowser(browser)) {
  Do we want to check the URL here as well?
File chrome/browser/devtools/devtools_window.cc:
- Patch Set #7, Line 565: bool DevToolsWindow::IsDevToolsWindow(DevToolsWindow* window) {
  Do we really need this overload? It seems like we could do the same with `IsDevToolsWindow(window->main_web_contents_)` in the `chrome/browser/devtools/devtools_window_testing.cc`

To view, visit change 4232543. To unsubscribe, or for help writing mail filters, visit settings.

Irina Fedorova (Gerrit)

unread,

Feb 10, 2023, 4:33:10 AM2/10/23

to Polina Bondarenko, asvitkine...@chromium.org, chromeos-kio...@google.com, devtools...@chromium.org, oshima...@chromium.org, Danil Somsikov, Ahmed Fakhry, Dana Fried, Alan Cutter, Tricium, Ben Franz, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Ahmed Fakhry, Alan Cutter, Ben Franz, Dana Fried, Polina Bondarenko.

Patch set 7:Code-Review +1

View Change

2 comments:

Patchset:
- Patch Set #7:
  Thank you, Polina!
File chrome/browser/chromeos/app_mode/app_session_browser_window_handler.cc:

- Patch Set #4, Line 60: IsNewBrowserWindowAllowed

- No, they do not overlap. But I agree, moved.
  Sorry, I would not move it above because later we will add another troubleshooting window and we should check for the original app window first and only after that open all troubleshooting tools. However, for this devtool window it does not mate so I can change it back when adding a new functionality.

To view, visit change 4232543. To unsubscribe, or for help writing mail filters, visit settings.

Irina Fedorova (Gerrit)

unread,

Feb 10, 2023, 5:45:09 AM2/10/23

to Polina Bondarenko, asvitkine...@chromium.org, chromeos-kio...@google.com, devtools...@chromium.org, oshima...@chromium.org, Danil Somsikov, Ahmed Fakhry, Dana Fried, Alan Cutter, Tricium, Ben Franz, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Ahmed Fakhry, Alan Cutter, Ben Franz, Dana Fried, Polina Bondarenko.

View Change

1 comment:

File chrome/browser/chromeos/app_mode/app_session.cc:

Patch Set #4, Line 275:

     std::make_unique<KioskTroubleshootingController>(
          profile_->GetPrefs(),
          base::BindOnce(&AppSession::ShutdownAppSession,
                         weak_ptr_factory_.GetWeakPtr())));

- Correct. I think it has to be refactored at some point. […]
```
Can we pass 
```
  base::BindRepeating(&AppSession::ShutdownAppSession,
                            weak_ptr_factory_.GetWeakPtr()),
```
```
  one more time here then?
  and create `KioskTroubleshootingController` inside `AppSessionBrowserWindowHandler`?

To view, visit change 4232543. To unsubscribe, or for help writing mail filters, visit settings.

Dana Fried (Gerrit)

unread,

Feb 10, 2023, 1:35:39 PM2/10/23

to Polina Bondarenko, asvitkine...@chromium.org, chromeos-kio...@google.com, devtools...@chromium.org, oshima...@chromium.org, Irina Fedorova, Danil Somsikov, Ahmed Fakhry, Alan Cutter, Tricium, Ben Franz, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Ahmed Fakhry, Alan Cutter, Ben Franz, Polina Bondarenko.

Patch set 7:Code-Review +1

View Change

1 comment:

Patchset:
- Patch Set #7:
  +1 for c/b/ui only.

To view, visit change 4232543. To unsubscribe, or for help writing mail filters, visit settings.

Alan Cutter (Gerrit)

unread,

Feb 13, 2023, 1:26:48 AM2/13/23

to Polina Bondarenko, asvitkine...@chromium.org, chromeos-kio...@google.com, devtools...@chromium.org, oshima...@chromium.org, Dana Fried, Irina Fedorova, Danil Somsikov, Ahmed Fakhry, Tricium, Ben Franz, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Ahmed Fakhry, Ben Franz, Polina Bondarenko.

View Change

2 comments:

File chrome/browser/chromeos/app_mode/app_session_browser_window_handler.cc:
- Patch Set #7, Line 56: false
  Nit: Add `/*is_closing=*/` to these function calls.
File chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:
- Patch Set #7, Line 60: {ui::VKEY_I, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN, IDC_DEV_TOOLS}};
  There's a few more shortcuts that open devtools, WDYT of including all of them?
  https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/accelerator_table.cc;l=216?q=IDC_DEV_TOOLS

To view, visit change 4232543. To unsubscribe, or for help writing mail filters, visit settings.

Ahmed Fakhry (Gerrit)

unread,

Feb 14, 2023, 12:21:12 PM2/14/23

to asvitkine...@chromium.org, chromeos-kio...@google.com, devtools...@chromium.org, oshima...@chromium.org, Xiaoqian Dai, Polina Bondarenko, Dana Fried, Irina Fedorova, Danil Somsikov, Alan Cutter, Ben Franz

Attention is currently required from: Ben Franz, Polina Bondarenko, Xiaoqian Dai.

Polina Bondarenko has uploaded this change for review.

To view, visit change 4232543. To unsubscribe, or for help writing mail filters, visit settings.

Ahmed Fakhry (Gerrit)

unread,

Feb 14, 2023, 12:21:18 PM2/14/23

to Polina Bondarenko, asvitkine...@chromium.org, chromeos-kio...@google.com, devtools...@chromium.org, oshima...@chromium.org, Xiaoqian Dai, Dana Fried, Irina Fedorova, Danil Somsikov, Alan Cutter, Tricium, Ben Franz, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Ben Franz, Polina Bondarenko, Xiaoqian Dai.

View Change

1 comment:

File ash/wm/workspace/workspace_window_resizer.cc:
- Patch Set #7, Line 555: IsFullscreen()
  Is there a better check than this? Assuming that a non-fullscreen window in kiosk mode is a devtools window seems not very robust.
  @xd...@chromium.org WDYT?

To view, visit change 4232543. To unsubscribe, or for help writing mail filters, visit settings.

Xiaoqian Dai (Gerrit)

unread,

Feb 14, 2023, 1:31:25 PM2/14/23

to Polina Bondarenko, asvitkine...@chromium.org, chromeos-kio...@google.com, devtools...@chromium.org, oshima...@chromium.org, Xiaoqian Dai, Dana Fried, Irina Fedorova, Danil Somsikov, Ahmed Fakhry, Alan Cutter, Tricium, Ben Franz, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Ahmed Fakhry, Ben Franz, Polina Bondarenko.

View Change

1 comment:

File ash/wm/workspace/workspace_window_resizer.cc:

- Patch Set #7, Line 555: IsFullscreen()

- Is there a better check than this? Assuming that a non-fullscreen window in kiosk mode is a devtools […]
  Agreed. The logic seems fragile to me. potentially we can use `ShellDelegate` to query if the window is a browser window and if it's `is_type_devtools`?

To view, visit change 4232543. To unsubscribe, or for help writing mail filters, visit settings.

Polina Bondarenko (Gerrit)

unread,

Feb 14, 2023, 1:54:45 PM2/14/23

to asvitkine...@chromium.org, chromeos-kio...@google.com, devtools...@chromium.org, oshima...@chromium.org, Xiaoqian Dai, Dana Fried, Irina Fedorova, Danil Somsikov, Ahmed Fakhry, Alan Cutter, Tricium, Ben Franz, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Ahmed Fakhry, Alan Cutter, Ben Franz, Danil Somsikov, Xiaoqian Dai.

View Change

3 comments:

File ash/wm/workspace/workspace_window_resizer.cc:

- Patch Set #7, Line 555: IsFullscreen()

- Agreed. The logic seems fragile to me. […]
  Why resizing and moving around a non-fullscreen window is bad?
  We plan a few more windows for troubleshooting which are not going to be fullscreen too.
File chrome/browser/chromeos/app_mode/app_session_browser_window_handler.cc:
- Patch Set #7, Line 60: if (IsDevToolsAllowedBrowser(browser)) {

- Do we want to check the URL here as well?

- Do you mean that the URL starts with devtools:// ?
  Is it a valid scenario that devtools type browser's URL starts not with devtools:// ?
File chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:

- Patch Set #7, Line 60: {ui::VKEY_I, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN, IDC_DEV_TOOLS}};

- There's a few more shortcuts that open devtools, WDYT of including all of them? […]
  I plan to extend the list in coming CLs.

To view, visit change 4232543. To unsubscribe, or for help writing mail filters, visit settings.

Xiaoqian Dai (Gerrit)

unread,

Feb 14, 2023, 6:01:59 PM2/14/23

to Polina Bondarenko, asvitkine...@chromium.org, chromeos-kio...@google.com, devtools...@chromium.org, oshima...@chromium.org, Xiaoqian Dai, Dana Fried, Irina Fedorova, Danil Somsikov, Ahmed Fakhry, Alan Cutter, Tricium, Ben Franz, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Ahmed Fakhry, Alan Cutter, Ben Franz, Danil Somsikov, Polina Bondarenko.

View Change

1 comment:

File ash/wm/workspace/workspace_window_resizer.cc:

- Patch Set #7, Line 555: IsFullscreen()

- Why resizing and moving around a non-fullscreen window is bad? […]
  So from the comment, it looks like it's intentionally that we disabled window moving and resizing when running in the single app mode (kiosk or forced app mode). If we exclude non-fullscreen windows here, I'm not sure if that could potentially mean other windows in single app mode has a way to exit kiosk mode or forced app mode and view system UIs, which seems a bit unsafe to me.

To view, visit change 4232543. To unsubscribe, or for help writing mail filters, visit settings.

Alan Cutter (Gerrit)

unread,

Feb 14, 2023, 10:19:14 PM2/14/23

to Polina Bondarenko, asvitkine...@chromium.org, chromeos-kio...@google.com, devtools...@chromium.org, oshima...@chromium.org, Xiaoqian Dai, Dana Fried, Irina Fedorova, Danil Somsikov, Ahmed Fakhry, Tricium, Ben Franz, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Ahmed Fakhry, Ben Franz, Danil Somsikov, Polina Bondarenko.

Patch set 7:Code-Review +1

View Change

2 comments:

Patchset:
- Patch Set #7:
  chrome/browser/ui/views/apps/chrome_native_app_window_views.cc lgtm after comments addressed.
File chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:

- Patch Set #7, Line 60: {ui::VKEY_I, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN, IDC_DEV_TOOLS}};

- I plan to extend the list in coming CLs.
  Let's add a TODO here if there's more work intended.

To view, visit change 4232543. To unsubscribe, or for help writing mail filters, visit settings.

Polina Bondarenko (Gerrit)

unread,

Feb 15, 2023, 7:04:57 AM2/15/23

to asvitkine...@chromium.org, chromeos-kio...@google.com, devtools...@chromium.org, oshima...@chromium.org, Alan Cutter, Xiaoqian Dai, Dana Fried, Irina Fedorova, Danil Somsikov, Ahmed Fakhry, Tricium, Ben Franz, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Ahmed Fakhry, Alan Cutter, Ben Franz, Dana Fried, Danil Somsikov, Irina Fedorova, Xiaoqian Dai.

View Change

6 comments:

Patchset:
- Patch Set #8:

- Thanks for reviewing! Fixed/responded to your comments

File ash/wm/workspace/workspace_window_resizer.cc:

- Patch Set #7, Line 555: IsFullscreen()

- So from the comment, it looks like it's intentionally that we disabled window moving and resizing wh […]
  What do you think about kResizeBehaviorCanResize window property? (Updated the patch)
  I'd like to not spam this part of code with devtools-specific code and would like to formalize it somehow.
  The window is resizable only for devtools windows in kiosk mode. See changes in chrome/browser/ui/views/frame/browser_view.cc
  Looks like it is safe.
File chrome/browser/chromeos/app_mode/app_session.cc:

Patch Set #4, Line 275:

     std::make_unique<KioskTroubleshootingController>(
          profile_->GetPrefs(),
          base::BindOnce(&AppSession::ShutdownAppSession,
                         weak_ptr_factory_.GetWeakPtr())));

- Can we pass […]
  Added TODO.
File chrome/browser/chromeos/app_mode/app_session_browser_window_handler.cc:
- Patch Set #7, Line 56: false

- Nit: Add `/*is_closing=*/` to these function calls.

- Done
File chrome/browser/devtools/devtools_window.cc:

- Patch Set #7, Line 565: bool DevToolsWindow::IsDevToolsWindow(DevToolsWindow* window) {

- Do we really need this overload? It seems like we could do the same with `IsDevToolsWindow(window->m […]
  Done
File chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:

- Patch Set #7, Line 60: {ui::VKEY_I, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN, IDC_DEV_TOOLS}};

- Let's add a TODO here if there's more work intended.
  Done

To view, visit change 4232543. To unsubscribe, or for help writing mail filters, visit settings.

Polina Bondarenko (Gerrit)

unread,

Feb 15, 2023, 7:05:31 AM2/15/23

to asvitkine...@chromium.org, chromeos-kio...@google.com, devtools...@chromium.org, oshima...@chromium.org, Alan Cutter, Xiaoqian Dai, Dana Fried, Irina Fedorova, Danil Somsikov, Ahmed Fakhry, Tricium, Ben Franz, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Ahmed Fakhry, Alan Cutter, Ben Franz, Dana Fried, Danil Somsikov, Irina Fedorova, Xiaoqian Dai.

Patch set 8:Quick-Run +1

View Change

Irina Fedorova (Gerrit)

unread,

Feb 15, 2023, 7:08:11 AM2/15/23

to Polina Bondarenko, asvitkine...@chromium.org, chromeos-kio...@google.com, devtools...@chromium.org, oshima...@chromium.org, Alan Cutter, Xiaoqian Dai, Dana Fried, Danil Somsikov, Ahmed Fakhry, Tricium, Ben Franz, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Ahmed Fakhry, Alan Cutter, Ben Franz, Dana Fried, Danil Somsikov, Polina Bondarenko, Xiaoqian Dai.

Patch set 8:Code-Review +1

View Change

To view, visit change 4232543. To unsubscribe, or for help writing mail filters, visit settings.

Danil Somsikov (Gerrit)

unread,

Feb 15, 2023, 8:11:31 AM2/15/23

to Polina Bondarenko, asvitkine...@chromium.org, chromeos-kio...@google.com, devtools...@chromium.org, oshima...@chromium.org, Irina Fedorova, Alan Cutter, Xiaoqian Dai, Dana Fried, Ahmed Fakhry, Tricium, Ben Franz, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Ahmed Fakhry, Alan Cutter, Ben Franz, Dana Fried, Polina Bondarenko, Xiaoqian Dai.

View Change

1 comment:

File chrome/browser/chromeos/app_mode/app_session_browser_window_handler.cc:
- Patch Set #7, Line 60: if (IsDevToolsAllowedBrowser(browser)) {
  Do you mean that the URL starts with devtools:// ? […]
  I mean that you seem to conclude that everything that is not settings is devtools. And this might be dangerous. I think for your purposes checking for devtools:// scheme would be enough.

To view, visit change 4232543. To unsubscribe, or for help writing mail filters, visit settings.

Danil Somsikov (Gerrit)

unread,

Feb 15, 2023, 8:17:20 AM2/15/23

to Polina Bondarenko, asvitkine...@chromium.org, chromeos-kio...@google.com, devtools...@chromium.org, oshima...@chromium.org, Irina Fedorova, Alan Cutter, Xiaoqian Dai, Dana Fried, Ahmed Fakhry, Tricium, Ben Franz, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Ahmed Fakhry, Alan Cutter, Ben Franz, Dana Fried, Polina Bondarenko, Xiaoqian Dai.

Patch set 8:Code-Review +1

View Change

1 comment:

File chrome/browser/chromeos/app_mode/app_session_browser_window_handler.cc:

- Patch Set #7, Line 60: if (IsDevToolsAllowedBrowser(browser)) {

- I mean that you seem to conclude that everything that is not settings is devtools. […]
  As discussed offline, the check for is_type_devtools() should probably suffice.

To view, visit change 4232543. To unsubscribe, or for help writing mail filters, visit settings.

Polina Bondarenko (Gerrit)

unread,

Feb 15, 2023, 8:20:25 AM2/15/23

to asvitkine...@chromium.org, chromeos-kio...@google.com, devtools...@chromium.org, oshima...@chromium.org, Danil Somsikov, Irina Fedorova, Alan Cutter, Xiaoqian Dai, Dana Fried, Ahmed Fakhry, Tricium, Ben Franz, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Ahmed Fakhry, Alan Cutter, Ben Franz, Dana Fried, Danil Somsikov, Irina Fedorova, Xiaoqian Dai.

Patch set 9:Auto-Submit +1Commit-Queue +2

View Change

1 comment:

Patchset:
- Patch Set #9:
  Thanks for reviewing! I move the resizer's changes to a separate CL

To view, visit change 4232543. To unsubscribe, or for help writing mail filters, visit settings.

Irina Fedorova (Gerrit)

unread,

Feb 15, 2023, 8:20:52 AM2/15/23

to Polina Bondarenko, asvitkine...@chromium.org, chromeos-kio...@google.com, devtools...@chromium.org, oshima...@chromium.org, Chromium LUCI CQ, Danil Somsikov, Alan Cutter, Xiaoqian Dai, Dana Fried, Ahmed Fakhry, Tricium, Ben Franz, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Ahmed Fakhry, Alan Cutter, Ben Franz, Dana Fried, Danil Somsikov, Polina Bondarenko, Xiaoqian Dai.

Patch set 9:Code-Review +1

View Change

To view, visit change 4232543. To unsubscribe, or for help writing mail filters, visit settings.

Polina Bondarenko (Gerrit)

unread,

Feb 15, 2023, 8:21:46 AM2/15/23

to asvitkine...@chromium.org, chromeos-kio...@google.com, devtools...@chromium.org, oshima...@chromium.org, Irina Fedorova, Chromium LUCI CQ, Danil Somsikov, Alan Cutter, Xiaoqian Dai, Dana Fried, Ahmed Fakhry, Tricium, Ben Franz, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Ahmed Fakhry, Alan Cutter, Ben Franz, Dana Fried, Danil Somsikov, Xiaoqian Dai.

Patch set 9:Commit-Queue +2

View Change

To view, visit change 4232543. To unsubscribe, or for help writing mail filters, visit settings.

Chromium LUCI CQ (Gerrit)

unread,

Feb 15, 2023, 9:22:41 AM2/15/23

to Polina Bondarenko, asvitkine...@chromium.org, chromeos-kio...@google.com, devtools...@chromium.org, oshima...@chromium.org, Irina Fedorova, Danil Somsikov, Alan Cutter, Xiaoqian Dai, Dana Fried, Ahmed Fakhry, Tricium, Ben Franz, Chromium Metrics Reviews, chromium...@chromium.org

Chromium LUCI CQ submitted this change.

View Change

Approvals:
  Polina Bondarenko: Send CL to CQ automatically after approval; Commit
  Irina Fedorova: Looks good to me

kiosk: enable devtools in kiosk mode.

When kiosk troubleshooting is enabled and devtools are allowed by
policy, devtools should be allowed to be open by a shortcut
(Shift+Ctrl+I).

This works for both web and chrome app kiosks. The devtools window is created movable and resizable.
Note: setting KioskTroubleshootingToolsEnabled policy to true is not sufficient to open devtools in kiosk mode. DeveloperToolsAvailability user policy should allow opening of devtools for the session.

Bug: b/265405355
Bug: b/265405666

Test: browser_tests
Test: ash_unittests
Test: unit_tests
Change-Id: I4e46685daa605a78741552ea7c16bd1acb5f391b

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4232543
Auto-Submit: Polina Bondarenko <pb...@chromium.org>
Commit-Queue: Polina Bondarenko <pb...@chromium.org>
Reviewed-by: Irina Fedorova <irfed...@google.com>
Cr-Commit-Position: refs/heads/main@{#1105630}
---


M chrome/browser/app_mode/app_mode_utils.cc
A chrome/browser/ash/app_mode/kiosk_troubleshooting_tools_browsertest.cc
M chrome/browser/chromeos/app_mode/app_session.cc
M chrome/browser/chromeos/app_mode/app_session.h
M chrome/browser/chromeos/app_mode/app_session_browser_window_handler.cc
M chrome/browser/chromeos/app_mode/app_session_browser_window_handler.h
M chrome/browser/chromeos/app_mode/app_session_unittest.cc
M chrome/browser/devtools/devtools_window.cc

M chrome/browser/devtools/devtools_window_testing.cc
M chrome/browser/ui/views/apps/chrome_native_app_window_views.cc
M chrome/browser/ui/views/frame/browser_view.cc
M chrome/test/BUILD.gn
M tools/metrics/histograms/enums.xml

13 files changed, 334 insertions(+), 80 deletions(-)

To view, visit change 4232543. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward