New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 912998 link

Starred by 1 user
Status: Fixed
Owner:
steve...@chromium.org
Closed: Dec 10
Cc:
█ updowndota@chromium.org
tbarzic@chromium.org
hansberry@chromium.org
jamescook@chromium.org
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug

Blocking:
issue 888711



Sign in to add a comment

Clean up Chrome OS System Dialogs

Project Member Reported by steve...@chromium.org, Dec 7 
We still have a few system dialogs in Chrome OS that do not use SystemWebDialogDelegate. Fixing this will allow us to clean up chrome_web_dialog_view.cc and make sure that system dialogs are handled consistently in Chrome OS.

Known issues:
* EnrollmentDialogView which is the sole consumer of SystemTrayClient::CreateUnownedDialogWidget, which replicates logic in chrome_web_dialog_view.cc.
* SetTimeDialog
* multidevice_setup_dialog.cc and assistant_optin_ui.cc call chrome::ShowWebDialogInContainer in similar but slightly different ways. We should standardize these.
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 10 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/061816c5c3c86061fe4f61526b7be9756ebc4f31

commit 061816c5c3c86061fe4f61526b7be9756ebc4f31
Author: Steven Bennetts <stevenjb@chromium.org>
Date: Mon Dec 10 18:35:33 2018

SetTimeDialog: Use SystemWebDialogDelegate

This makes the SetTimeDialog consistent with other system dialogs
when opened from the system tray, and simplifies the class.

This CL also:
* Removes the unused |use_minimal_style| arg to
  SystemWebDialogDelegate::ShowSystemDialog()
* Adds a |parent| argument to ShowSystemDialog() to allow the time
  dialog to be parented when opened from the Settings UI, instead of
  AlwaysOnTop or modal.

Bug:  912998 
Change-Id: I6f075773a04c862f58bd4be16e0878d7ea8f1d4f
Reviewed-on: https://chromium-review.googlesource.com/c/1368281
Reviewed-by: Michael Giuffrida <michaelpg@chromium.org>
Reviewed-by: Toni Baržić <tbarzic@chromium.org>
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615182}
[modify] https://crrev.com/061816c5c3c86061fe4f61526b7be9756ebc4f31/chrome/browser/chromeos/set_time_dialog.cc
[modify] https://crrev.com/061816c5c3c86061fe4f61526b7be9756ebc4f31/chrome/browser/chromeos/set_time_dialog.h
[modify] https://crrev.com/061816c5c3c86061fe4f61526b7be9756ebc4f31/chrome/browser/ui/ash/system_tray_client.cc
[modify] https://crrev.com/061816c5c3c86061fe4f61526b7be9756ebc4f31/chrome/browser/ui/webui/chromeos/system_web_dialog_delegate.cc
[modify] https://crrev.com/061816c5c3c86061fe4f61526b7be9756ebc4f31/chrome/browser/ui/webui/chromeos/system_web_dialog_delegate.h
[modify] https://crrev.com/061816c5c3c86061fe4f61526b7be9756ebc4f31/chrome/browser/ui/webui/settings/chromeos/date_time_handler.cc
[modify] https://crrev.com/061816c5c3c86061fe4f61526b7be9756ebc4f31/chrome/browser/ui/webui/signin/inline_login_handler_dialog_chromeos.cc
Project Member

Comment 2 by bugdroid1@chromium.org, Dec 10 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/70e4cc800a1c28c6e55fb1ceead7afca5f3d1576

commit 70e4cc800a1c28c6e55fb1ceead7afca5f3d1576
Author: Steven Bennetts <stevenjb@chromium.org>
Date: Mon Dec 10 19:27:26 2018

Clean up EnrollmentDialogView

This eliminates the unused 'owning_window' case from the enrollment
dialog, folds in the remaning call to
SystemTrayClient::CreateUnownedDialogWidget, and uses ash_util
instead of duplicating logic. It also moves GetDialogParentContainerId
to browser_dialogs.h.

Bug:  912998 
Change-Id: I53bc9ddfe9034a40bfd64ce35a41a21c1639b16e
Reviewed-on: https://chromium-review.googlesource.com/c/1368456
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615200}
[modify] https://crrev.com/70e4cc800a1c28c6e55fb1ceead7afca5f3d1576/chrome/browser/chromeos/login/ui/login_web_dialog.cc
[modify] https://crrev.com/70e4cc800a1c28c6e55fb1ceead7afca5f3d1576/chrome/browser/ui/ash/ash_util.cc
[modify] https://crrev.com/70e4cc800a1c28c6e55fb1ceead7afca5f3d1576/chrome/browser/ui/ash/ash_util.h
[modify] https://crrev.com/70e4cc800a1c28c6e55fb1ceead7afca5f3d1576/chrome/browser/ui/ash/network/enrollment_dialog_view.cc
[modify] https://crrev.com/70e4cc800a1c28c6e55fb1ceead7afca5f3d1576/chrome/browser/ui/ash/network/enrollment_dialog_view.h
[modify] https://crrev.com/70e4cc800a1c28c6e55fb1ceead7afca5f3d1576/chrome/browser/ui/ash/network/network_connect_delegate_chromeos.cc
[modify] https://crrev.com/70e4cc800a1c28c6e55fb1ceead7afca5f3d1576/chrome/browser/ui/ash/system_tray_client.cc
[modify] https://crrev.com/70e4cc800a1c28c6e55fb1ceead7afca5f3d1576/chrome/browser/ui/ash/system_tray_client.h

Comment 3 by steve...@chromium.org, Dec 10

Status: Fixed (was: Started)
►

Sign in to add a comment