Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2415053002:
MacViews: Support ui::MODAL_TYPE_WINDOW with a null parent window. (Closed)
DescriptionMacViews: Support ui::MODAL_TYPE_WINDOW with a null parent window.
It sounds weird, but other platforms support this. The window simply
becomes unparented and isn't modal to anything.
Currently NativeWidgetMac will DCHECK in void NativeWidgetMac::
InitModalType() and crash in BridgedNativeWidget::ShowAsModalSheet()
when trying to attach the window to the parent as a dialog sheet.
It's straightforward to ignore the modal type on Mac when no parent is
given.
A use case for this is when extensions need to surface a dialog (e.g.
when permissions require upgrading). Usually there is a parent browser
window, but sometimes there is not. For example, background pages or app
shortcuts launched from the Windows desktop.
Add a test case for constrained_window::CreateBrowserModalDialogViews()
which is responsible for allowing this. There's an existing test --
ExtensionInstallPromptBrowserTest.NoParent -- but it doesn't have
coverage for actually showing the Widget - only the plumbing that leads
up to that step.
BUG=647574
Committed: https://crrev.com/82c125e09e98ceefc99fef084d0653fc959b2a97
Cr-Commit-Position: refs/heads/master@{#426302}
Patch Set 1 #Patch Set 2 : Allow context to be set #Patch Set 3 : don't git-conflict with myself #
Total comments: 7
Patch Set 4 : review comments, desktop widgets, cite bug #
Messages
Total messages: 34 (27 generated)
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== MacViews: Support ui::MODAL_TYPE_WINDOW with a null window? BUG=647574 ========== to ========== MacViews: Support ui::MODAL_TYPE_WINDOW with a null parent window. It sounds weird, but other platforms support this. The window simply becomes unparented and isn't modal to anything. Currently NativeWidgetMac will DCHECK in void NativeWidgetMac::InitModalType() and crash in BridgedNativeWidget::ShowAsModalSheet() when trying to attach the window to the parent as a dialog sheet. It's straightforward to ignore the modal type on Mac when no parent is given. A use case for this is when extensions need to surface a dialog (e.g. when permissions require upgrading). Usually there is a parent browser window, but sometimes there is not For example, background pages or app shortcuts launched from the Windows desktop). BUG=647574 ==========
tapted@chromium.org changed reviewers: + msw@chromium.org
Description was changed from ========== MacViews: Support ui::MODAL_TYPE_WINDOW with a null parent window. It sounds weird, but other platforms support this. The window simply becomes unparented and isn't modal to anything. Currently NativeWidgetMac will DCHECK in void NativeWidgetMac::InitModalType() and crash in BridgedNativeWidget::ShowAsModalSheet() when trying to attach the window to the parent as a dialog sheet. It's straightforward to ignore the modal type on Mac when no parent is given. A use case for this is when extensions need to surface a dialog (e.g. when permissions require upgrading). Usually there is a parent browser window, but sometimes there is not For example, background pages or app shortcuts launched from the Windows desktop). BUG=647574 ========== to ========== MacViews: Support ui::MODAL_TYPE_WINDOW with a null parent window. It sounds weird, but other platforms support this. The window simply becomes unparented and isn't modal to anything. Currently NativeWidgetMac will DCHECK in void NativeWidgetMac:: InitModalType() and crash in BridgedNativeWidget::ShowAsModalSheet() when trying to attach the window to the parent as a dialog sheet. It's straightforward to ignore the modal type on Mac when no parent is given. A use case for this is when extensions need to surface a dialog (e.g. when permissions require upgrading). Usually there is a parent browser window, but sometimes there is not For example, background pages or app shortcuts launched from the Windows desktop). BUG=647574 ==========
Description was changed from ========== MacViews: Support ui::MODAL_TYPE_WINDOW with a null parent window. It sounds weird, but other platforms support this. The window simply becomes unparented and isn't modal to anything. Currently NativeWidgetMac will DCHECK in void NativeWidgetMac:: InitModalType() and crash in BridgedNativeWidget::ShowAsModalSheet() when trying to attach the window to the parent as a dialog sheet. It's straightforward to ignore the modal type on Mac when no parent is given. A use case for this is when extensions need to surface a dialog (e.g. when permissions require upgrading). Usually there is a parent browser window, but sometimes there is not For example, background pages or app shortcuts launched from the Windows desktop). BUG=647574 ========== to ========== MacViews: Support ui::MODAL_TYPE_WINDOW with a null parent window. It sounds weird, but other platforms support this. The window simply becomes unparented and isn't modal to anything. Currently NativeWidgetMac will DCHECK in void NativeWidgetMac:: InitModalType() and crash in BridgedNativeWidget::ShowAsModalSheet() when trying to attach the window to the parent as a dialog sheet. It's straightforward to ignore the modal type on Mac when no parent is given. A use case for this is when extensions need to surface a dialog (e.g. when permissions require upgrading). Usually there is a parent browser window, but sometimes there is not For example, background pages or app shortcuts launched from the Windows desktop). Add a test case for constrained_window::CreateBrowserModalDialogViews() which is responsible for allowing this. There's an existing test -- ExtensionInstallPromptBrowserTest.NoParent -- but it doesn't have coverage for actually showing the Widget - only the plumbing that leads up to that step. BUG=647574 ==========
Hi Mike, please take a look. There is additional details/analysis in http://crbug.com/647574#c9 and #c10. Thanks!
Description was changed from ========== MacViews: Support ui::MODAL_TYPE_WINDOW with a null parent window. It sounds weird, but other platforms support this. The window simply becomes unparented and isn't modal to anything. Currently NativeWidgetMac will DCHECK in void NativeWidgetMac:: InitModalType() and crash in BridgedNativeWidget::ShowAsModalSheet() when trying to attach the window to the parent as a dialog sheet. It's straightforward to ignore the modal type on Mac when no parent is given. A use case for this is when extensions need to surface a dialog (e.g. when permissions require upgrading). Usually there is a parent browser window, but sometimes there is not For example, background pages or app shortcuts launched from the Windows desktop). Add a test case for constrained_window::CreateBrowserModalDialogViews() which is responsible for allowing this. There's an existing test -- ExtensionInstallPromptBrowserTest.NoParent -- but it doesn't have coverage for actually showing the Widget - only the plumbing that leads up to that step. BUG=647574 ========== to ========== MacViews: Support ui::MODAL_TYPE_WINDOW with a null parent window. It sounds weird, but other platforms support this. The window simply becomes unparented and isn't modal to anything. Currently NativeWidgetMac will DCHECK in void NativeWidgetMac:: InitModalType() and crash in BridgedNativeWidget::ShowAsModalSheet() when trying to attach the window to the parent as a dialog sheet. It's straightforward to ignore the modal type on Mac when no parent is given. A use case for this is when extensions need to surface a dialog (e.g. when permissions require upgrading). Usually there is a parent browser window, but sometimes there is not. For example, background pages or app shortcuts launched from the Windows desktop). Add a test case for constrained_window::CreateBrowserModalDialogViews() which is responsible for allowing this. There's an existing test -- ExtensionInstallPromptBrowserTest.NoParent -- but it doesn't have coverage for actually showing the Widget - only the plumbing that leads up to that step. BUG=647574 ==========
Description was changed from ========== MacViews: Support ui::MODAL_TYPE_WINDOW with a null parent window. It sounds weird, but other platforms support this. The window simply becomes unparented and isn't modal to anything. Currently NativeWidgetMac will DCHECK in void NativeWidgetMac:: InitModalType() and crash in BridgedNativeWidget::ShowAsModalSheet() when trying to attach the window to the parent as a dialog sheet. It's straightforward to ignore the modal type on Mac when no parent is given. A use case for this is when extensions need to surface a dialog (e.g. when permissions require upgrading). Usually there is a parent browser window, but sometimes there is not. For example, background pages or app shortcuts launched from the Windows desktop). Add a test case for constrained_window::CreateBrowserModalDialogViews() which is responsible for allowing this. There's an existing test -- ExtensionInstallPromptBrowserTest.NoParent -- but it doesn't have coverage for actually showing the Widget - only the plumbing that leads up to that step. BUG=647574 ========== to ========== MacViews: Support ui::MODAL_TYPE_WINDOW with a null parent window. It sounds weird, but other platforms support this. The window simply becomes unparented and isn't modal to anything. Currently NativeWidgetMac will DCHECK in void NativeWidgetMac:: InitModalType() and crash in BridgedNativeWidget::ShowAsModalSheet() when trying to attach the window to the parent as a dialog sheet. It's straightforward to ignore the modal type on Mac when no parent is given. A use case for this is when extensions need to surface a dialog (e.g. when permissions require upgrading). Usually there is a parent browser window, but sometimes there is not. For example, background pages or app shortcuts launched from the Windows desktop. Add a test case for constrained_window::CreateBrowserModalDialogViews() which is responsible for allowing this. There's an existing test -- ExtensionInstallPromptBrowserTest.NoParent -- but it doesn't have coverage for actually showing the Widget - only the plumbing that leads up to that step. BUG=647574 ==========
I totally agree, this fix matches hacky behavior on other platforms that we should probably try to fix in a better way. Maybe file a bug to ensure that we only ever allow dialogs to claim window-modality with a valid parent window. I worry that there isn't really a better solution than something like this hack, where dialog creation has some fallback behavior for bad users. lgtm with nits for now. https://codereview.chromium.org/2415053002/diff/40001/components/constrained_... File components/constrained_window/constrained_window_views_unittest.cc (right): https://codereview.chromium.org/2415053002/diff/40001/components/constrained_... components/constrained_window/constrained_window_views_unittest.cc:59: class TestConstrainedWindowViewsClient nit: add a simple class comment, event something like: "Dummy client that returns a null modal dialog host and host view." https://codereview.chromium.org/2415053002/diff/40001/components/constrained_... components/constrained_window/constrained_window_views_unittest.cc:90: if (!params->context) It's a bummer that we need this, but IIRC a null context (and a null parent) is not okay on any platform... Should we just set the ViewsTestBase::GetContext() as a backup/default in TestViewsDelegate::OnBeforeWidgetInit? Perhaps that's too heavy of a hammer? WDYT? https://codereview.chromium.org/2415053002/diff/40001/components/constrained_... components/constrained_window/constrained_window_views_unittest.cc:234: EXPECT_TRUE(destroyed); nit: this (and the supporting code) is probably unnecessary.
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
PTAL - staring at OnBeforeWidgetInit() for so long made me realise there's better coverage if we do views_delegate()->set_use_desktop_native_widgets(true) (and since they're what will be used for these dialogs on Desktop Linux and Windows) On 2016/10/18 23:47:15, msw wrote: > Maybe file a bug to ensure that we > only ever allow dialogs to claim window-modality with a valid parent window. Filed http://crbug.com/647574 and referenced it in contrained_window_views.h https://codereview.chromium.org/2415053002/diff/40001/components/constrained_... File components/constrained_window/constrained_window_views_unittest.cc (right): https://codereview.chromium.org/2415053002/diff/40001/components/constrained_... components/constrained_window/constrained_window_views_unittest.cc:59: class TestConstrainedWindowViewsClient On 2016/10/18 23:47:15, msw wrote: > nit: add a simple class comment, event something like: "Dummy client that > returns a null modal dialog host and host view." Done. https://codereview.chromium.org/2415053002/diff/40001/components/constrained_... components/constrained_window/constrained_window_views_unittest.cc:90: if (!params->context) On 2016/10/18 23:47:15, msw wrote: > It's a bummer that we need this, but IIRC a null context (and a null parent) is > not okay on any platform... Should we just set the ViewsTestBase::GetContext() > as a backup/default in TestViewsDelegate::OnBeforeWidgetInit? Perhaps that's too > heavy of a hammer? WDYT? It was certainly true that Weird Things would happen with a null context|parent when Metro mode was involved. Now things are mostly sane: - On ChromeOS, ChromeViewsDelegate::OnBeforeWidgetInit will DCHECK only if InitParams::child is set (in combination with a null parent and null context). But fall back to ash::Shell::GetPrimaryRootWindow() in all cases when parent and context are both null. - On other Aura platforms, ChromeViewsDelegate doesn't DCHECK and just uses a DesktopNativeWidgetAura when context is null - On Mac, there's just the one NativeWidget type - so widget.cc's CreateNativeWidget(..) will just invoke internal::NativeWidgetPrivate::CreateNativeWidget(delegate) most of the time I guess going forward (and, e.g., for Mus), the logic in OnBeforeWidgetInit() might move to native_widget_factory(), but currently ChromeViewsDelegate seems OK with the null-context+parent arrangement. But it's only *desktop* aura widgets that are OK with a null context. I had to add this because of the DCHECK in NativeWidgetAura::InitNativeWidget (I could set TestViewsDelegate::use_desktop_widgets, but then I couldn't run the test on ChromeOS). Hm. I guess this test has more appropriate coverage with use_desktop_widgets anyway, so I should do that. [Done.] So.. I suppose it's OK for TestViewsDelegate::OnBeforeWidgetInit() to fallback to GetContext(), since that's what ChromeOS does. However, maybe there are implications for Mus down the track? i.e. if we are OK with the GetContext() fallback, we should be able to scrap InitParams::context altogether and instead always get it from ViewsDelegate. Which is a much scarier change.. https://codereview.chromium.org/2415053002/diff/40001/components/constrained_... components/constrained_window/constrained_window_views_unittest.cc:234: EXPECT_TRUE(destroyed); On 2016/10/18 23:47:15, msw wrote: > nit: this (and the supporting code) is probably unnecessary. Done. (I think I added this to satisfy myself that `CloseNow` was working correctly, since Window-Modal "sheets" on Mac currently don't like being closed synchronously due to the way it interacts with OS window animations, but other DCHECKs should be hit if that goes awry).
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
still lgtm; thanks! https://codereview.chromium.org/2415053002/diff/40001/components/constrained_... File components/constrained_window/constrained_window_views_unittest.cc (right): https://codereview.chromium.org/2415053002/diff/40001/components/constrained_... components/constrained_window/constrained_window_views_unittest.cc:90: if (!params->context) On 2016/10/19 08:13:48, tapted wrote: > On 2016/10/18 23:47:15, msw wrote: > > It's a bummer that we need this, but IIRC a null context (and a null parent) > is > > not okay on any platform... Should we just set the ViewsTestBase::GetContext() > > as a backup/default in TestViewsDelegate::OnBeforeWidgetInit? Perhaps that's > too > > heavy of a hammer? WDYT? > > It was certainly true that Weird Things would happen with a null context|parent > when Metro mode was involved. Now things are mostly sane: > - On ChromeOS, ChromeViewsDelegate::OnBeforeWidgetInit will DCHECK only if > InitParams::child is set (in combination with a null parent and null context). > But fall back to ash::Shell::GetPrimaryRootWindow() in all cases when parent and > context are both null. > - On other Aura platforms, ChromeViewsDelegate doesn't DCHECK and just uses a > DesktopNativeWidgetAura when context is null > - On Mac, there's just the one NativeWidget type - so widget.cc's > CreateNativeWidget(..) will just invoke > internal::NativeWidgetPrivate::CreateNativeWidget(delegate) most of the time > > I guess going forward (and, e.g., for Mus), the logic in OnBeforeWidgetInit() > might move to native_widget_factory(), but currently ChromeViewsDelegate seems > OK with the null-context+parent arrangement. > > But it's only *desktop* aura widgets that are OK with a null context. I had to > add this because of the DCHECK in NativeWidgetAura::InitNativeWidget (I could > set TestViewsDelegate::use_desktop_widgets, but then I couldn't run the test on > ChromeOS). Hm. I guess this test has more appropriate coverage with > use_desktop_widgets anyway, so I should do that. [Done.] > > > So.. I suppose it's OK for TestViewsDelegate::OnBeforeWidgetInit() to fallback > to GetContext(), since that's what ChromeOS does. However, maybe there are > implications for Mus down the track? i.e. if we are OK with the GetContext() > fallback, we should be able to scrap InitParams::context altogether and instead > always get it from ViewsDelegate. Which is a much scarier change.. This is certainly fine as-is, it's a much more directed change.
The CQ bit was checked by tapted@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== MacViews: Support ui::MODAL_TYPE_WINDOW with a null parent window. It sounds weird, but other platforms support this. The window simply becomes unparented and isn't modal to anything. Currently NativeWidgetMac will DCHECK in void NativeWidgetMac:: InitModalType() and crash in BridgedNativeWidget::ShowAsModalSheet() when trying to attach the window to the parent as a dialog sheet. It's straightforward to ignore the modal type on Mac when no parent is given. A use case for this is when extensions need to surface a dialog (e.g. when permissions require upgrading). Usually there is a parent browser window, but sometimes there is not. For example, background pages or app shortcuts launched from the Windows desktop. Add a test case for constrained_window::CreateBrowserModalDialogViews() which is responsible for allowing this. There's an existing test -- ExtensionInstallPromptBrowserTest.NoParent -- but it doesn't have coverage for actually showing the Widget - only the plumbing that leads up to that step. BUG=647574 ========== to ========== MacViews: Support ui::MODAL_TYPE_WINDOW with a null parent window. It sounds weird, but other platforms support this. The window simply becomes unparented and isn't modal to anything. Currently NativeWidgetMac will DCHECK in void NativeWidgetMac:: InitModalType() and crash in BridgedNativeWidget::ShowAsModalSheet() when trying to attach the window to the parent as a dialog sheet. It's straightforward to ignore the modal type on Mac when no parent is given. A use case for this is when extensions need to surface a dialog (e.g. when permissions require upgrading). Usually there is a parent browser window, but sometimes there is not. For example, background pages or app shortcuts launched from the Windows desktop. Add a test case for constrained_window::CreateBrowserModalDialogViews() which is responsible for allowing this. There's an existing test -- ExtensionInstallPromptBrowserTest.NoParent -- but it doesn't have coverage for actually showing the Widget - only the plumbing that leads up to that step. BUG=647574 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== MacViews: Support ui::MODAL_TYPE_WINDOW with a null parent window. It sounds weird, but other platforms support this. The window simply becomes unparented and isn't modal to anything. Currently NativeWidgetMac will DCHECK in void NativeWidgetMac:: InitModalType() and crash in BridgedNativeWidget::ShowAsModalSheet() when trying to attach the window to the parent as a dialog sheet. It's straightforward to ignore the modal type on Mac when no parent is given. A use case for this is when extensions need to surface a dialog (e.g. when permissions require upgrading). Usually there is a parent browser window, but sometimes there is not. For example, background pages or app shortcuts launched from the Windows desktop. Add a test case for constrained_window::CreateBrowserModalDialogViews() which is responsible for allowing this. There's an existing test -- ExtensionInstallPromptBrowserTest.NoParent -- but it doesn't have coverage for actually showing the Widget - only the plumbing that leads up to that step. BUG=647574 ========== to ========== MacViews: Support ui::MODAL_TYPE_WINDOW with a null parent window. It sounds weird, but other platforms support this. The window simply becomes unparented and isn't modal to anything. Currently NativeWidgetMac will DCHECK in void NativeWidgetMac:: InitModalType() and crash in BridgedNativeWidget::ShowAsModalSheet() when trying to attach the window to the parent as a dialog sheet. It's straightforward to ignore the modal type on Mac when no parent is given. A use case for this is when extensions need to surface a dialog (e.g. when permissions require upgrading). Usually there is a parent browser window, but sometimes there is not. For example, background pages or app shortcuts launched from the Windows desktop. Add a test case for constrained_window::CreateBrowserModalDialogViews() which is responsible for allowing this. There's an existing test -- ExtensionInstallPromptBrowserTest.NoParent -- but it doesn't have coverage for actually showing the Widget - only the plumbing that leads up to that step. BUG=647574 Committed: https://crrev.com/82c125e09e98ceefc99fef084d0653fc959b2a97 Cr-Commit-Position: refs/heads/master@{#426302} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/82c125e09e98ceefc99fef084d0653fc959b2a97 Cr-Commit-Position: refs/heads/master@{#426302} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
