components: Create web_modal component.

components: Create web_modal component.

Notable changes:
 * Move WebContentsModalDialog related classes into components/web_modal/
 * Use namespace web_modal
 * Move the logic for sending the Chrome IPC message out of
   WebContentsModalDialogManager, and into ChromeModalDialogManager.
   Have Browser and ShellWindow subclass ChromeModalDialogManager,
   instead of directly from WebContentsModalDialogManager, to be able
   to send the IPC.
 * Introduce WebContentsModalDialogManagerDelegate::IsWebContentsVisibile.
   This is necessary to remove the dependency on platform_util.

BUG=241278
R=ben@chromium.org, wittman@chromium.org

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

[sadrul, 2013-05-09 20:26:38]

I will update this CL once http://crrev.com/13674013 and
http://crrev.com/13467040 land. But I think this can be reviewed in parallel.

https://codereview.chromium.org/14969012/diff/7001/chrome/browser/ui/views/we...
File chrome/browser/ui/views/web_contents_modal_dialog_manager_views.cc (right):

https://codereview.chromium.org/14969012/diff/7001/chrome/browser/ui/views/we...
chrome/browser/ui/views/web_contents_modal_dialog_manager_views.cc:34: using
web_modal::NativeWebContentsModalDialogManagerDelegate;
TheseAreSomeReallyLongNamesDelegate.

Any objections to shortening 'WebContentsModalDialog' to 'WebModal' at some
point? (i.e. 'NativeWebContentsModalDialogManager' becomes
'NativeWebModalManager' etc.)

[Ben Goodger (Google), 2013-05-09 21:29:06]

For context, Mike, we need to use web modal dialogs from a non-Chrome
component, so it seemed like this was a good time to move this code out
especially since you have kept it nicely self-contained.


On Thu, May 9, 2013 at 1:26 PM, <sadrul@chromium.org> wrote:

> Reviewers: Mike Wittman, Ben Goodger (Google),
>
> Message:
> I will update this CL once http://crrev.com/13674013 and
> http://crrev.com/13467040 land. But I think this can be reviewed in
> parallel.
>
>
> https://codereview.chromium.**org/14969012/diff/7001/chrome/**
>
browser/ui/views/web_contents_**modal_dialog_manager_views.cc<https://codereview.chromium.org/14969012/diff/7001/chrome/browser/ui/views/web_contents_modal_dialog_manager_views.cc>
> File chrome/browser/ui/views/web_**contents_modal_dialog_manager_**
> views.cc
> (right):
>
> https://codereview.chromium.**org/14969012/diff/7001/chrome/**
>
browser/ui/views/web_contents_**modal_dialog_manager_views.cc#**newcode34<https://codereview.chromium.org/14969012/diff/7001/chrome/browser/ui/views/web_contents_modal_dialog_manager_views.cc#newcode34>
> chrome/browser/ui/views/web_**contents_modal_dialog_manager_**views.cc:34:
> using web_modal::**NativeWebContentsModalDialogMa**nagerDelegate;
> TheseAreSomeReallyLongNamesDel**egate.
>
> Any objections to shortening 'WebContentsModalDialog' to 'WebModal' at
> some point? (i.e. '**NativeWebContentsModalDialogMa**nager' becomes
> 'NativeWebModalManager' etc.)
>
> Description:
> components: Create web_modal component.
>
> Notable changes:
>  * Move WebContentsModalDialog related classes into components/web_modal/
>  * Use namespace web_modal
>  * Move the logic for sending the Chrome IPC message out of
>    WebContentsModalDialogManager, and into ChromeModalDialogManager.
>    Have Browser and ShellWindow subclass ChromeModalDialogManager,
>    instead of directly from WebContentsModalDialogManager, to be able
>    to send the IPC.
>
> BUG=TBD
>
> Please review this at
https://codereview.chromium.**org/14969012/<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/DEPS
>   M chrome/browser/chromeos/login/**simple_web_view_dialog.cc
>   M chrome/browser/chromeos/login/**webui_login_view.cc
>   M chrome/browser/extensions/api/**media_galleries/media_**
> galleries_api.cc
>   M chrome/browser/extensions/**platform_app_browsertest.cc
>   M chrome/browser/managed_mode/**managed_mode_browsertest.cc
>   M chrome/browser/repost_form_**warning_browsertest.cc
>   M chrome/browser/ssl/ssl_**browser_tests.cc
>   M chrome/browser/ui/browser.h
>   M chrome/browser/ui/browser.cc
>   M chrome/browser/ui/browser_**commands.cc
>   M chrome/browser/ui/browser_tab_**contents.cc
>   M chrome/browser/ui/browser_**window.h
>   A chrome/browser/ui/chrome_**modal_dialog_manager_delegate.**h
>   A chrome/browser/ui/chrome_**modal_dialog_manager_delegate.**cc
>   M chrome/browser/ui/cocoa/**browser_window_cocoa.h
>   M
chrome/browser/ui/cocoa/browse**r_window_cocoa.mm<http://browser_window_cocoa.mm>
>   M
chrome/browser/ui/cocoa/browse**r_window_controller.mm<http://browser_window_controller.mm>
>   M
chrome/browser/ui/cocoa/certif**icate_viewer_mac_browsertest.**mm<http://certificate_viewer_mac_browsertest.mm>
>   M
chrome/browser/ui/cocoa/constr**ained_web_dialog_delegate_mac.**mm<http://constrained_web_dialog_delegate_mac.mm>
>   M chrome/browser/ui/cocoa/**constrained_window/**
> constrained_window_mac.h
>   M chrome/browser/ui/cocoa/**constrained_window/constrained**
> _window_mac.mm <http://constrained_window_mac.mm>
>   M chrome/browser/ui/cocoa/**content_settings/collected_**
> cookies_mac_browsertest.mm <http://collected_cookies_mac_browsertest.mm>
>   M chrome/browser/ui/cocoa/**extensions/media_galleries_**
>
dialog_cocoa_browsertest.mm<http://media_galleries_dialog_cocoa_browsertest.mm>
>   M chrome/browser/ui/cocoa/**extensions/native_app_window_**cocoa.h
>   M
chrome/browser/ui/cocoa/**extensions/native_app_window_**cocoa.mm<http://native_app_window_cocoa.mm>
>   M chrome/browser/ui/cocoa/ssl_**client_certificate_selector_**
>
cocoa_browsertest.mm<http://ssl_client_certificate_selector_cocoa_browsertest.mm>
>   M
chrome/browser/ui/cocoa/tabs/t**ab_strip_controller.mm<http://tab_strip_controller.mm>
>   M
chrome/browser/ui/cocoa/web_**contents_modal_dialog_manager_**cocoa.mm<http://web_contents_modal_dialog_manager_cocoa.mm>
>   M chrome/browser/ui/extensions/**native_app_window.h
>   M chrome/browser/ui/extensions/**shell_window.h
>   M chrome/browser/ui/extensions/**shell_window.cc
>   M chrome/browser/ui/gtk/browser_**window_gtk.h
>   M chrome/browser/ui/gtk/browser_**window_gtk.cc
>   M chrome/browser/ui/gtk/**collected_cookies_gtk.cc
>   M chrome/browser/ui/gtk/**constrained_web_dialog_**delegate_gtk.cc
>   M chrome/browser/ui/gtk/**extensions/media_galleries_**dialog_gtk.cc
>   M chrome/browser/ui/gtk/**extensions/native_app_window_**gtk.h
>   M chrome/browser/ui/gtk/**extensions/native_app_window_**gtk.cc
>   M chrome/browser/ui/gtk/login_**prompt_gtk.cc
>   M chrome/browser/ui/gtk/ssl_**client_certificate_selector.cc
>   M chrome/browser/ui/gtk/tab_**modal_confirm_dialog_gtk.cc
>   M chrome/browser/ui/gtk/web_**contents_modal_dialog_manager_**gtk.cc
>   D chrome/browser/ui/native_web_**contents_modal_dialog.h
>   D chrome/browser/ui/native_web_**contents_modal_dialog_manager.**h
>   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/**extensions/native_app_window_**views.h
>   M chrome/browser/ui/views/**extensions/native_app_window_**views.cc
>   M chrome/browser/ui/views/frame/**browser_view.h
>   M chrome/browser/ui/views/frame/**browser_view.cc
>   M chrome/browser/ui/views/frame/**browser_view_layout.h
>   M chrome/browser/ui/views/frame/**browser_view_layout.cc
>   M chrome/browser/ui/views/login_**prompt_views.cc
>   M chrome/browser/ui/views/ssl_**client_certificate_selector.cc
>   M chrome/browser/ui/views/tab_**contents/chrome_web_contents_**
> view_delegate_views.cc
>   M chrome/browser/ui/views/tab_**modal_confirm_dialog_views.cc
>   M chrome/browser/ui/views/web_**contents_modal_dialog_manager_**views.cc
>   D chrome/browser/ui/web_**contents_modal_dialog_host.h
>   D chrome/browser/ui/web_**contents_modal_dialog_host.cc
>   D chrome/browser/ui/web_**contents_modal_dialog_manager.**h
>   D chrome/browser/ui/web_**contents_modal_dialog_manager.**cc
>   D chrome/browser/ui/web_**contents_modal_dialog_manager_**delegate.h
>   D chrome/browser/ui/web_**contents_modal_dialog_manager_**delegate.cc
>   D chrome/browser/ui/web_**contents_modal_dialog_manager_**unittest.cc
>   M chrome/browser/ui/webui/**certificate_viewer_webui.cc
>   M chrome/browser/ui/webui/**constrained_web_dialog_**delegate_base.h
>   M chrome/browser/ui/webui/**constrained_web_dialog_**delegate_base.cc
>   M chrome/browser/ui/webui/**constrained_web_dialog_ui.h
>   M chrome/browser/ui/webui/**constrained_web_dialog_ui_**browsertest.cc
>   M chrome/browser/ui/webui/print_**preview/print_preview_ui_**unittest.cc
>   M chrome/chrome_browser.gypi
>   M chrome/chrome_browser_ui.gypi
>   M chrome/chrome_tests_unit.gypi
>   M chrome/test/base/test_browser_**window.h
>   M chrome/test/base/test_browser_**window.cc
>   M components/components.gyp
>   M components/components_tests.**gypi
>   A components/web_modal.gypi
>   A + components/web_modal/DEPS
>   A + components/web_modal/native_**web_contents_modal_dialog.h
>   A + components/web_modal/native_**web_contents_modal_dialog_**manager.h
>   A + components/web_modal/web_**contents_modal_dialog_host.h
>   A + components/web_modal/web_**contents_modal_dialog_host.cc
>   A + components/web_modal/web_**contents_modal_dialog_manager.**h
>   A + components/web_modal/web_**contents_modal_dialog_manager.**cc
>   A + components/web_modal/web_**contents_modal_dialog_manager_**
> delegate.h
>   A + components/web_modal/web_**contents_modal_dialog_manager_**
> delegate.cc
>   A + components/web_modal/web_**contents_modal_dialog_manager_**
> unittest.cc
>
>
>

[Mike Wittman, 2013-05-09 21:48:10]

On 2013/05/09 21:29:06, Ben Goodger (Google) wrote:
> For context, Mike, we need to use web modal dialogs from a non-Chrome
> component, so it seemed like this was a good time to move this code out
> especially since you have kept it nicely self-contained.

SGTM.  I was planning to do this, but wound up helping with new dialog style
appearance issues instead.  Beyond the two CLs mentioned I still have a couple
bug fixes which will touch WebContentsModalDialogManager, so Sadrul and I can
coordinate on how to land.

[Mike Wittman, 2013-05-10 00:03:22]

Looks good overall, just a couple small issues/questions.

https://codereview.chromium.org/14969012/diff/7001/chrome/browser/ui/views/we...
File chrome/browser/ui/views/web_contents_modal_dialog_manager_views.cc (right):

https://codereview.chromium.org/14969012/diff/7001/chrome/browser/ui/views/we...
chrome/browser/ui/views/web_contents_modal_dialog_manager_views.cc:34: using
web_modal::NativeWebContentsModalDialogManagerDelegate;
On 2013/05/09 20:26:38, sadrul wrote:
> TheseAreSomeReallyLongNamesDelegate.
> 
> Any objections to shortening 'WebContentsModalDialog' to 'WebModal' at some
> point? (i.e. 'NativeWebContentsModalDialogManager' becomes
> 'NativeWebModalManager' etc.)

If they're consistently prefixed by the web_modal namespace, how about just
removing the "WebContentsModal" part of the names?

https://codereview.chromium.org/14969012/diff/11001/chrome/browser/ui/chrome_...
File chrome/browser/ui/chrome_modal_dialog_manager_delegate.h (right):

https://codereview.chromium.org/14969012/diff/11001/chrome/browser/ui/chrome_...
chrome/browser/ui/chrome_modal_dialog_manager_delegate.h:12: class
ChromeModalDialogManagerDelegate
How about ChromeWebContentsModalDialogManagerDelegate, or
ChromeWebModalDialogManagerDelegate?

ChromeModalDialogManagerDelegate gives the impression that the dialog is
Chrome-modal.

https://codereview.chromium.org/14969012/diff/11001/components/web_modal/web_...
File components/web_modal/web_contents_modal_dialog_manager_unittest.cc (right):

https://codereview.chromium.org/14969012/diff/11001/components/web_modal/web_...
components/web_modal/web_contents_modal_dialog_manager_unittest.cc:36: return
new TestNativeWebContentsModalDialogManager();
This can probably return a new NativeWebContentsModalDialogManagerCloseTest. 
The refactoring in https://codereview.chromium.org/13467040 is moving to just
one stub native manager class for all tests.

[sadrul, 2013-05-10 02:59:18]

Thanks for the quick review!

> > 
> > Any objections to shortening 'WebContentsModalDialog' to 'WebModal' at some
> > point? (i.e. 'NativeWebContentsModalDialogManager' becomes
> > 'NativeWebModalManager' etc.)
> 
> If they're consistently prefixed by the web_modal namespace, how about just
> removing the "WebContentsModal" part of the names?

That sounds good to me. I don't plan on doing this renaming in this CL though. I
will wait until all the in-progress CLs land to minimize the risk of
merge-conflicts.

https://codereview.chromium.org/14969012/diff/11001/chrome/browser/ui/chrome_...
File chrome/browser/ui/chrome_modal_dialog_manager_delegate.h (right):

https://codereview.chromium.org/14969012/diff/11001/chrome/browser/ui/chrome_...
chrome/browser/ui/chrome_modal_dialog_manager_delegate.h:12: class
ChromeModalDialogManagerDelegate
On 2013/05/10 00:03:22, Mike Wittman wrote:
> How about ChromeWebContentsModalDialogManagerDelegate, or
> ChromeWebModalDialogManagerDelegate?
> 
> ChromeModalDialogManagerDelegate gives the impression that the dialog is
> Chrome-modal.

Renamed to ChromeWebModalDialogManagerDelegate.

https://codereview.chromium.org/14969012/diff/11001/components/web_modal/web_...
File components/web_modal/web_contents_modal_dialog_manager_unittest.cc (right):

https://codereview.chromium.org/14969012/diff/11001/components/web_modal/web_...
components/web_modal/web_contents_modal_dialog_manager_unittest.cc:36: return
new TestNativeWebContentsModalDialogManager();
On 2013/05/10 00:03:22, Mike Wittman wrote:
> This can probably return a new NativeWebContentsModalDialogManagerCloseTest. 
> The refactoring in https://codereview.chromium.org/13467040 is moving to just
> one stub native manager class for all tests.

Ah, cool. I didn't actually notice NWCMDMCloseTest. Changed for now to return
that instead of this new TNWCMDM.

[Mike Wittman, 2013-05-15 21:42:36]

https://codereview.chromium.org/14969012/diff/18001/chrome/browser/ssl/ssl_br...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/14969012/diff/18001/chrome/browser/ssl/ssl_br...
chrome/browser/ssl/ssl_browser_tests.cc:1294: IN_PROC_BROWSER_TEST_F(SSLUITest,
DISABLED_TestGoodFrameNavigation) {
Why disable this test?

[sadrul, 2013-05-15 21:45:34]

https://codereview.chromium.org/14969012/diff/18001/chrome/browser/ssl/ssl_br...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/14969012/diff/18001/chrome/browser/ssl/ssl_br...
chrome/browser/ssl/ssl_browser_tests.cc:1294: IN_PROC_BROWSER_TEST_F(SSLUITest,
DISABLED_TestGoodFrameNavigation) {
On 2013/05/15 21:42:37, Mike Wittman wrote:
> Why disable this test?

This CL is not disabling the test (the patchset delta includes ToT merge, and
the test has been disabled in ToT)

[Mike Wittman, 2013-05-15 22:05:13]

On 2013/05/15 21:45:34, sadrul wrote:
>
https://codereview.chromium.org/14969012/diff/18001/chrome/browser/ssl/ssl_br...
> File chrome/browser/ssl/ssl_browser_tests.cc (right):
> 
>
https://codereview.chromium.org/14969012/diff/18001/chrome/browser/ssl/ssl_br...
> chrome/browser/ssl/ssl_browser_tests.cc:1294:
IN_PROC_BROWSER_TEST_F(SSLUITest,
> DISABLED_TestGoodFrameNavigation) {
> On 2013/05/15 21:42:37, Mike Wittman wrote:
> > Why disable this test?
> 
> This CL is not disabling the test (the patchset delta includes ToT merge, and
> the test has been disabled in ToT)

LGTM.  Thanks for taking this on, and being flexible on the logistics!

[sadrul, 2013-05-15 23:17:45]

On 2013/05/15 22:05:13, Mike Wittman wrote:
> On 2013/05/15 21:45:34, sadrul wrote:
> >
>
https://codereview.chromium.org/14969012/diff/18001/chrome/browser/ssl/ssl_br...
> > File chrome/browser/ssl/ssl_browser_tests.cc (right):
> > 
> >
>
https://codereview.chromium.org/14969012/diff/18001/chrome/browser/ssl/ssl_br...
> > chrome/browser/ssl/ssl_browser_tests.cc:1294:
> IN_PROC_BROWSER_TEST_F(SSLUITest,
> > DISABLED_TestGoodFrameNavigation) {
> > On 2013/05/15 21:42:37, Mike Wittman wrote:
> > > Why disable this test?
> > 
> > This CL is not disabling the test (the patchset delta includes ToT merge,
and
> > the test has been disabled in ToT)
> 
> LGTM.  Thanks for taking this on, and being flexible on the logistics!

Thanks!

Looks like the bots are pretty happy with the change too. I will land after I
get OWNERS stamp.

[Ben Goodger (Google), 2013-05-15 23:50:30]

lgtm

[sadrul, 2013-05-16 04:38:03]

Committed patchset #7 manually as r200448 (presubmit successful).