Refactor modality-specific behavior from ConstrainedWindowViews to WebContentsModalDialogManager

Refactor modality-specific behavior from ConstrainedWindowViews to WebContentsModalDialogManager

This CL moves the Views web contents modal dialog code towards the
interface described in the associated bug.  Similar changes for Gtk and
Cocoa will be made in follow-on CLs.

To that end, this CL contains the following detailed changes:

 - Introduce NativeWebContentsModalDialogManager for providing
   platform-specific UI functionality within WebContentsModalDialogManager, and
   implement NativeWebContentsModalDialogManagerViews.

 - Under Views, move to a model where WebContentsModalDialogManager listens for
   closing events rather than requiring notifications from the widget.  Continue
   using the existing model on Gtk and Cocoa, pending refactoring there.

 - Provide a mechanism for disabling mouse-driven dialog movement on Widget.

 - Remove NativeConstrainedWindowDelegate and subclasses as they're no longer
   necessary due to the above two items.

 - Update tests to account for closing notification to
   WebContentsModalDialogManager being done from the event loop, rather than
   synchronously.

 - Add temporary ConstrainedWindowViews factory function to minimize code churn
   at construction sites during this refactoring.

The next planned steps in this work are:

 - Refactor ConstrainedWindow subclasses under Gtk and Cocoa to listen for
   closing events rather than requiring notifications from the widget.

 - Refactor uses of WebContentsModalDialog interface to use platform-specific
   equivalents in platform-specific code and, in platform-independent code,
   invoke functionality via some other mechanism: platform_utils,
   WebContentsModalDialogManager, ???.  Remove WebContentsModalDialog interface.

 - Generalize WebContentsModalDialogManagerDelegate positioning interface and
   write WebContentsModalDialogManager::ShowDialog using the new interface.

BUG=157161
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180904

[Mike Wittman, 2013-01-23 00:39:27]

Hi Ben, please take a look at this next change for the Constrained Window API
refactor.  The main goal of this change is to refactor modality-specific
behavior from ConstrainedWindowViews to WebContentsModalDialogManager.  This
gets us much closer to the construction API you're looking for under Views.

To that end, this CL contains the following changes:

 - Introduce WebContentsModalDialogManagerUIDelegate for providing
   platform-specific UI functionality within WebContentsModalDialogManager, and
   implement WebContentsModalDialogManagerUIDelegateViews.

 - Under Views, move to a model where WebContentsModalDialogManager listens for
   closing events rather than requiring notifications from the widget.  Continue
   using the existing model on Gtk and Cocoa, pending refactoring there.

 - Provide a mechanism for disabling mouse-driven dialog movement on Widget.

 - Remove NativeConstrainedWindowDelegate and subclasses as they're no longer
   necessary due to the above two items.

 - Update tests to account for closing notification to
   WebContentsModalDialogManager being done from the event loop, rather than
   synchronously.

 - Add temporary ConstrainedWindowViews factory function to minimize code churn
   at construction sites during this refactoring.

The next planned steps in this work are:

 - Refactor ConstrainedWindow subclasses under Gtk and Cocoa to listen for
   closing events rather than requiring notifications from the widget.

 - Refactor uses of WebContentsModalDialog interface to use platform-specific
   equivalents in platform-specific code and, in platform-independent code,
   invoke functionality via some other mechanism: platform_utils,
   WebContentsModalDialogManager, ???.  Remove WebContentsModalDialog interface.

 - Generalize WebContentsModalDialogManagerDelegate positioning interface and
   write WebContentsModalDialogManager::ShowDialog using the new interface.

[tfarina, 2013-01-23 00:42:05]

On Tue, Jan 22, 2013 at 10:39 PM,  <wittman@chromium.org> wrote:
> Reviewers: Ben Goodger (Google),
>
> Message:
> Hi Ben, please take a look at this next change for the Constrained Window
> API
> refactor.  The main goal of this change is to refactor modality-specific
> behavior from ConstrainedWindowViews to WebContentsModalDialogManager.  This
> gets us much closer to the construction API you're looking for under Views.
>
> To that end, this CL contains the following changes:
>
Mike, can you include everything written below in the CL description
(so it gets saved in the commit message for future generations ;))?

Thanks,

>  - Introduce WebContentsModalDialogManagerUIDelegate for providing
>    platform-specific UI functionality within WebContentsModalDialogManager,
> and
>    implement WebContentsModalDialogManagerUIDelegateViews.
>
>  - Under Views, move to a model where WebContentsModalDialogManager listens
> for
>    closing events rather than requiring notifications from the widget.
> Continue
>    using the existing model on Gtk and Cocoa, pending refactoring there.
>
>  - Provide a mechanism for disabling mouse-driven dialog movement on Widget.
>
>  - Remove NativeConstrainedWindowDelegate and subclasses as they're no
> longer
>    necessary due to the above two items.
>
>  - Update tests to account for closing notification to
>    WebContentsModalDialogManager being done from the event loop, rather than
>    synchronously.
>
>  - Add temporary ConstrainedWindowViews factory function to minimize code
> churn
>    at construction sites during this refactoring.
>
> The next planned steps in this work are:
>
>  - Refactor ConstrainedWindow subclasses under Gtk and Cocoa to listen for
>    closing events rather than requiring notifications from the widget.
>
>  - Refactor uses of WebContentsModalDialog interface to use
> platform-specific
>    equivalents in platform-specific code and, in platform-independent code,
>    invoke functionality via some other mechanism: platform_utils,
>    WebContentsModalDialogManager, ???.  Remove WebContentsModalDialog
> interface.
>
>  - Generalize WebContentsModalDialogManagerDelegate positioning interface
> and
>    write WebContentsModalDialogManager::ShowDialog using the new interface.
>
> Please review this at https://codereview.chromium.org/12045037/
>

-- 
Thiago

[Ben Goodger (Google), 2013-01-23 18:57:25]

I'm looking at this now...


On Tue, Jan 22, 2013 at 4:39 PM, <wittman@chromium.org> wrote:

> Reviewers: Ben Goodger (Google),
>
> Message:
> Hi Ben, please take a look at this next change for the Constrained Window
> API
> refactor.  The main goal of this change is to refactor modality-specific
> behavior from ConstrainedWindowViews to WebContentsModalDialogManager.
>  This
> gets us much closer to the construction API you're looking for under Views.
>
> To that end, this CL contains the following changes:
>
>  - Introduce WebContentsModalDialogManagerU**IDelegate for providing
>    platform-specific UI functionality within
> WebContentsModalDialogManager, and
>    implement WebContentsModalDialogManagerU**IDelegateViews.
>
>  - Under Views, move to a model where WebContentsModalDialogManager
> listens for
>    closing events rather than requiring notifications from the widget.
>  Continue
>    using the existing model on Gtk and Cocoa, pending refactoring there.
>
>  - Provide a mechanism for disabling mouse-driven dialog movement on
> Widget.
>
>  - Remove NativeConstrainedWindowDelegat**e and subclasses as they're no
> longer
>    necessary due to the above two items.
>
>  - Update tests to account for closing notification to
>    WebContentsModalDialogManager being done from the event loop, rather
> than
>    synchronously.
>
>  - Add temporary ConstrainedWindowViews factory function to minimize code
> churn
>    at construction sites during this refactoring.
>
> The next planned steps in this work are:
>
>  - Refactor ConstrainedWindow subclasses under Gtk and Cocoa to listen for
>    closing events rather than requiring notifications from the widget.
>
>  - Refactor uses of WebContentsModalDialog interface to use
> platform-specific
>    equivalents in platform-specific code and, in platform-independent code,
>    invoke functionality via some other mechanism: platform_utils,
>    WebContentsModalDialogManager, ???.  Remove WebContentsModalDialog
> interface.
>
>  - Generalize WebContentsModalDialogManagerD**elegate positioning
> interface and
>    write WebContentsModalDialogManager:**:ShowDialog using the new
> interface.
>
> Description:
> Refactor modality-specific behavior from ConstrainedWindowViews to
> WebContentsModalDialogManager
>
> This CL moves the Views web contents modal dialog code towards the
> interface described in the associated bug.  Similar changes for Gtk and
> Cocoa will be made in follow-on CLs.
>
> BUG=157161
>
>
> Please review this at
https://codereview.chromium.**org/12045037/<https://codereview.chromium.org/1...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>   M chrome/browser/ui/browser_**command_controller_**browsertest.cc
>   A
chrome/browser/ui/cocoa/web_**contents_modal_dialog_manager_**cocoa.mm<http://web_contents_modal_dialog_manager_cocoa.mm>
>   A chrome/browser/ui/gtk/web_**contents_modal_dialog_manager_**gtk.cc
>   M chrome/browser/ui/views/**autofill/autofill_dialog_**views.cc
>   M chrome/browser/ui/views/**collected_cookies_views.cc
>   M chrome/browser/ui/views/**constrained_web_dialog_**delegate_views.cc
>   M chrome/browser/ui/views/**constrained_window_views.h
>   M chrome/browser/ui/views/**constrained_window_views.cc
>   M chrome/browser/ui/views/**constrained_window_views_**browsertest.cc
>   M chrome/browser/ui/views/**extensions/media_galleries_**dialog_views.cc
>   M chrome/browser/ui/views/login_**prompt_views.cc
>   D chrome/browser/ui/views/**native_constrained_window_**aura.cc
>   D chrome/browser/ui/views/**native_constrained_window_win.**cc
>   M chrome/browser/ui/views/ssl_**client_certificate_selector.cc
>   M chrome/browser/ui/views/tab_**modal_confirm_dialog_views.cc
>   A chrome/browser/ui/views/web_**contents_modal_dialog_manager_**views.cc
>   M chrome/browser/ui/views/web_**intent_picker_views.cc
>   M chrome/browser/ui/web_**contents_modal_dialog_manager.**h
>   M chrome/browser/ui/web_**contents_modal_dialog_manager.**cc
>   A chrome/browser/ui/web_**contents_modal_dialog_manager_**ui_delegate.h
>   M chrome/browser/ui/web_**contents_modal_dialog_manager_**unittest.cc
>   M chrome/browser/ui/webui/print_**preview/print_preview_ui_**unittest.cc
>   M chrome/chrome_browser_ui.gypi
>   M ui/views/widget/widget.h
>   M ui/views/widget/widget.cc
>
>
>

[Ben Goodger (Google), 2013-01-23 19:10:46]

I really like where this is going!

Just nits.

https://codereview.chromium.org/12045037/diff/1/chrome/browser/ui/cocoa/web_c...
File chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_cocoa.mm (right):

https://codereview.chromium.org/12045037/diff/1/chrome/browser/ui/cocoa/web_c...
chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_cocoa.mm:8:
CreateUIDelegate(WebContentsModalDialogManager* manager) {
nit:

WCMDMUIDelegate*
    WCMDM::CreateUIDelegate(
        WCMDM* manager);

https://codereview.chromium.org/12045037/diff/1/chrome/browser/ui/gtk/web_con...
File chrome/browser/ui/gtk/web_contents_modal_dialog_manager_gtk.cc (right):

https://codereview.chromium.org/12045037/diff/1/chrome/browser/ui/gtk/web_con...
chrome/browser/ui/gtk/web_contents_modal_dialog_manager_gtk.cc:8:
CreateUIDelegate(WebContentsModalDialogManager* manager) {
etc

https://codereview.chromium.org/12045037/diff/1/chrome/browser/ui/views/web_c...
File chrome/browser/ui/views/web_contents_modal_dialog_manager_views.cc (right):

https://codereview.chromium.org/12045037/diff/1/chrome/browser/ui/views/web_c...
chrome/browser/ui/views/web_contents_modal_dialog_manager_views.cc:48: 
nit: remove line

https://codereview.chromium.org/12045037/diff/1/chrome/browser/ui/views/web_c...
chrome/browser/ui/views/web_contents_modal_dialog_manager_views.cc:56:
widget->GetNativeWindow()->SetProperty(aura::client::kConstrainedWindowKey,
Add a 

// TODO: remove once the new visual style is complete

https://codereview.chromium.org/12045037/diff/1/chrome/browser/ui/views/web_c...
chrome/browser/ui/views/web_contents_modal_dialog_manager_views.cc:69:
widget->GetNativeWindow()->SetProperty(ash::kConstrainedWindowKey, true);
this line seems redundant with above?

https://codereview.chromium.org/12045037/diff/1/chrome/browser/ui/web_content...
File chrome/browser/ui/web_contents_modal_dialog_manager_ui_delegate.h (right):

https://codereview.chromium.org/12045037/diff/1/chrome/browser/ui/web_content...
chrome/browser/ui/web_contents_modal_dialog_manager_ui_delegate.h:12: class
WebContentsModalDialogManagerUIDelegate {
the "UIDelegate" part here is a little confusing to me... well frankly our use
of a lot of these component words in our class names are confusing at this
point.

NativeWebContentsModalDialogManager

is a couple of characters shorter, and matches the intent (platform interface).
it's not vastly superior so I'm open to other suggestions... it's just
consistent with some other interfaces we use, e.g. NativeWidget, etc.

https://codereview.chromium.org/12045037/diff/1/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/12045037/diff/1/ui/views/widget/widget.cc#new...
ui/views/widget/widget.cc:1129: if (movement_disabled_ && (component ==
HTCAPTION || component == HTSYSMENU))
HTNOWHERE... will this cause events to get sent still? how about HTCLIENT?

https://codereview.chromium.org/12045037/diff/1/ui/views/widget/widget.cc#new...
ui/views/widget/widget.cc:1132: return component;
nit: no else after return

[Mike Wittman, 2013-01-23 21:52:30]

https://codereview.chromium.org/12045037/diff/1/chrome/browser/ui/cocoa/web_c...
File chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_cocoa.mm (right):

https://codereview.chromium.org/12045037/diff/1/chrome/browser/ui/cocoa/web_c...
chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_cocoa.mm:8:
CreateUIDelegate(WebContentsModalDialogManager* manager) {
On 2013/01/23 19:10:46, Ben Goodger (Google) wrote:
> nit:
> 
> WCMDMUIDelegate*
>     WCMDM::CreateUIDelegate(
>         WCMDM* manager);

Done.

https://codereview.chromium.org/12045037/diff/1/chrome/browser/ui/gtk/web_con...
File chrome/browser/ui/gtk/web_contents_modal_dialog_manager_gtk.cc (right):

https://codereview.chromium.org/12045037/diff/1/chrome/browser/ui/gtk/web_con...
chrome/browser/ui/gtk/web_contents_modal_dialog_manager_gtk.cc:8:
CreateUIDelegate(WebContentsModalDialogManager* manager) {
On 2013/01/23 19:10:46, Ben Goodger (Google) wrote:
> etc

Done.

https://codereview.chromium.org/12045037/diff/1/chrome/browser/ui/views/web_c...
File chrome/browser/ui/views/web_contents_modal_dialog_manager_views.cc (right):

https://codereview.chromium.org/12045037/diff/1/chrome/browser/ui/views/web_c...
chrome/browser/ui/views/web_contents_modal_dialog_manager_views.cc:48: 
On 2013/01/23 19:10:46, Ben Goodger (Google) wrote:
> nit: remove line

Done.

https://codereview.chromium.org/12045037/diff/1/chrome/browser/ui/views/web_c...
chrome/browser/ui/views/web_contents_modal_dialog_manager_views.cc:56:
widget->GetNativeWindow()->SetProperty(aura::client::kConstrainedWindowKey,
On 2013/01/23 19:10:46, Ben Goodger (Google) wrote:
> Add a 
> 
> // TODO: remove once the new visual style is complete

Done.

https://codereview.chromium.org/12045037/diff/1/chrome/browser/ui/views/web_c...
chrome/browser/ui/views/web_contents_modal_dialog_manager_views.cc:69:
widget->GetNativeWindow()->SetProperty(ash::kConstrainedWindowKey, true);
On 2013/01/23 19:10:46, Ben Goodger (Google) wrote:
> this line seems redundant with above?

For whatever reason, there are separate keys for Aura and Ash
(aura::client::kConstrainedWindowKey and ash::kConstrainedWindowKey), and
there's code elsewhere that checks for each of them.

I'll duplicate the TODO and leave this for now, unless you want some other
resolution.

https://codereview.chromium.org/12045037/diff/1/chrome/browser/ui/web_content...
File chrome/browser/ui/web_contents_modal_dialog_manager_ui_delegate.h (right):

https://codereview.chromium.org/12045037/diff/1/chrome/browser/ui/web_content...
chrome/browser/ui/web_contents_modal_dialog_manager_ui_delegate.h:12: class
WebContentsModalDialogManagerUIDelegate {
On 2013/01/23 19:10:46, Ben Goodger (Google) wrote:
> the "UIDelegate" part here is a little confusing to me... well frankly our use
> of a lot of these component words in our class names are confusing at this
> point.
> 
> NativeWebContentsModalDialogManager
> 
> is a couple of characters shorter, and matches the intent (platform
interface).
> it's not vastly superior so I'm open to other suggestions... it's just
> consistent with some other interfaces we use, e.g. NativeWidget, etc.

NativeWebContentsModalDialogManager sounds reasonable to me.  I wasn't sure how
to name this concept.

https://codereview.chromium.org/12045037/diff/1/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/12045037/diff/1/ui/views/widget/widget.cc#new...
ui/views/widget/widget.cc:1129: if (movement_disabled_ && (component ==
HTCAPTION || component == HTSYSMENU))
On 2013/01/23 19:10:46, Ben Goodger (Google) wrote:
> HTNOWHERE... will this cause events to get sent still? how about HTCLIENT?

Yes, I checked and we still get mouse events in Widget::OnMouseEvent for a
HTNOWHERE result.  It looks like HWNDMessageHandler::OnNCHitTest explicitly
checks for HTNOWHERE and doesn't propagate it back to Windows.

https://codereview.chromium.org/12045037/diff/1/ui/views/widget/widget.cc#new...
ui/views/widget/widget.cc:1132: return component;
On 2013/01/23 19:10:46, Ben Goodger (Google) wrote:
> nit: no else after return

Done.

[Ben Goodger (Google), 2013-01-23 22:37:26]

lgtm

[commit-bot: I haz the power, 2013-02-05 22:28:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wittman@chromium.org/12045037/28001

[commit-bot: I haz the power, 2013-02-05 22:28:54]

Failed to apply patch for ash/desktop_background/desktop_background_view.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file ash/desktop_background/desktop_background_view.cc
  Hunk #1 FAILED at 82.
  1 out of 1 hunk FAILED -- saving rejects to file
ash/desktop_background/desktop_background_view.cc.rej

Patch:       ash/desktop_background/desktop_background_view.cc
Index: ash/desktop_background/desktop_background_view.cc
diff --git a/ash/desktop_background/desktop_background_view.cc
b/ash/desktop_background/desktop_background_view.cc
index
3590853051e346c09f41427a6367db656d601813..0675bc998af5c5793c18bc31baca219f23d8e114
100644
--- a/ash/desktop_background/desktop_background_view.cc
+++ b/ash/desktop_background/desktop_background_view.cc
@@ -82,7 +82,7 @@ class ShowWallpaperAnimationObserver : public
ui::ImplicitAnimationObserver,
   }
 
   // Overridden from views::WidgetObserver.
-  virtual void OnWidgetClosing(views::Widget* widget) OVERRIDE {
+  virtual void OnWidgetDestroying(views::Widget* widget) OVERRIDE {
     desktop_widget_->RemoveObserver(this);
     desktop_widget_ = NULL;
   }

[commit-bot: I haz the power, 2013-02-05 22:52:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wittman@chromium.org/12045037/33002

[commit-bot: I haz the power, 2013-02-05 23:54:42]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-02-06 01:30:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wittman@chromium.org/12045037/44001

[commit-bot: I haz the power, 2013-02-06 06:50:25]

Change committed as 180904