Move web modal dialog manager files into the constrained_window component.

Move web modal dialog manager files into the constrained_window component.

Currently, the CreateNativeWebModalManager() method declared inside the
web_modal component is defined outside of web_modal, which means linking it as a
shared component will fail. This is a layering violation.

This is now fixed for builds using Views, with constrained_window depending on
web_modal and web_modal providing definitions of all the symbols it declares.
Most unused implementations of CreateNativeWebModalManager() have been deleted,
with two remaining: create_native_web_modal_manager_cocoa.mm for Cocoa, and
test_create_native_web_modal_manager_cocoa.cc to prevent errors compiling
components_unittests, which isn't allowed to depend on the former file's
dependencies.

BUG=522654, 492442
Committed: https://crrev.com/7a65925409017abde6fcf3f4213f779707c0ab57
Cr-Commit-Position: refs/heads/master@{#412730}

[Patti Lor, 2016-06-22 02:55:38]

Patchset #2 (id:20001) has been deleted

[Patti Lor, 2016-06-22 23:57:11]

Patchset #3 (id:60001) has been deleted

[Patti Lor, 2016-06-28 04:06:08]

Patchset #7 (id:160001) has been deleted

[Patti Lor, 2016-06-29 00:23:34]

patricialor@chromium.org changed reviewers:
+ tapted@chromium.org

[Patti Lor, 2016-06-29 00:23:35]

Hey Trent, PTAL - this is the constrained_window CL to accompany the certificate
viewer stuff where I mentioned offline that two of the try bots are failing
because they can't see some Skia headers. I'm not able to replicate the error
locally and I can't tell which build target it is that has the failure out of
the zillion it's building, so I only have guesses right now (trying a build now
with use_ozone = true to see if that is the problem...) Hopefully you can help!
Tips on how to figure it out myself for next time would be much appreciated as
well! :)

[tapted, 2016-06-29 02:46:31]

On 2016/06/29 00:23:35, Patti Lor wrote:
> Hey Trent, PTAL - this is the constrained_window CL to accompany the
certificate
> viewer stuff where I mentioned offline that two of the try bots are failing
> because they can't see some Skia headers. I'm not able to replicate the error
> locally and I can't tell which build target it is that has the failure out of
> the zillion it's building, so I only have guesses right now (trying a build
now
> with use_ozone = true to see if that is the problem...) Hopefully you can
help!
> Tips on how to figure it out myself for next time would be much appreciated as
> well! :)

I think the problem is that web_modal has a skia dependency and that dependency
is exposed through its public headers like web_contents_modal_dialog_manager.h
(via web_contents_observer.h), so the skia dependency needs to be exported.

In web_modal/BUILD.gn, "//skia" needs to come out of

  deps = [
    "//base",
    "//content/public/browser",
    "//net",
    "//skia",
    "//ui/gfx",
    "//ui/gfx/geometry",
  ]

and put into

  public_deps = [
    # Skia headers are exposed in the public API.
    "//skia",
  ]

similar to how it's done in content/public/browser/BUILD.gn

The equivalent in web_modal.gypi should go something like the following (but
note it does not come out of 'dependencies' for gyp - it just gets mentioned
twice)

  'export_dependent_settings': [
    # Skia headers are exposed in the public API.
    '../skia/skia.gyp:skia',
  ],


This way, when web_modal_test_support depends on web_modal, it should get the
extra jazz it needs for skia stuff to compile.

[tapted, 2016-06-29 03:02:45]

https://codereview.chromium.org/2087643003/diff/180001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_views_mac_browsertest.mm
(right):

https://codereview.chromium.org/2087643003/diff/180001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_views_mac_browsertest.mm:44:
// constrained_window component is not used in Cocoa code (disallowed via
this comment should be updated

https://codereview.chromium.org/2087643003/diff/180001/components/constrained...
File components/constrained_window.gypi (right):

https://codereview.chromium.org/2087643003/diff/180001/components/constrained...
components/constrained_window.gypi:27: 'sources': [
dependencies?

https://codereview.chromium.org/2087643003/diff/180001/components/constrained...
components/constrained_window.gypi:32: ['OS!="mac" or mac_views_browser==1', {
This is all views, so this condition probably belongs elsewhere (i.e. to prevent
the constrained_window being depended on entirely)

https://codereview.chromium.org/2087643003/diff/180001/components/constrained...
File components/constrained_window/constrained_window_views.cc (right):

https://codereview.chromium.org/2087643003/diff/180001/components/constrained...
components/constrained_window/constrained_window_views.cc:142:
web_modal::WebContentsModalDialogManager::CreateNativeWebModalManager(
Can this just call NativeWebContentsModalDialogManagerViews directly rather than
CreateNativeWebModalManager ?

https://codereview.chromium.org/2087643003/diff/180001/components/constrained...
File
components/constrained_window/native_web_contents_modal_dialog_manager_views.h
(right):

https://codereview.chromium.org/2087643003/diff/180001/components/constrained...
components/constrained_window/native_web_contents_modal_dialog_manager_views.h:65:
web_modal::SingleWebContentsDialogManagerDelegate* native_delegate() {
This should stay in the follow-up CL, along with the other stuff (except the
namespace / include guard change)

https://codereview.chromium.org/2087643003/diff/180001/components/constrained...
File components/constrained_window/web_contents_modal_dialog_manager_views.cc
(right):

https://codereview.chromium.org/2087643003/diff/180001/components/constrained...
components/constrained_window/web_contents_modal_dialog_manager_views.cc:12:
SingleWebContentsDialogManager*
Add

/ /TODO(..) This is a layering violation and should be deleted. Currently it's
needed by WebContentsModalDialogManager::ShowModalDialog() which is still used
by X and Y, but they
should use constrained_window::ShowModalDialog instead.

(or if it's easy, just make it TODONE in this CL, delete this file, and hide
WebContentsModalDialogManager::ShowModalDialog() with #ifdef MAC

[Patti Lor, 2016-06-29 08:16:46]

Patchset #8 (id:200001) has been deleted

[Patti Lor, 2016-07-01 04:16:07]

Patchset #9 (id:240001) has been deleted

[Patti Lor, 2016-07-04 00:38:18]

Patchset #10 (id:280001) has been deleted

[Patti Lor, 2016-07-05 03:58:01]

Patchset #10 (id:300001) has been deleted

[Patti Lor, 2016-07-05 03:58:14]

Patchset #9 (id:260001) has been deleted

[Patti Lor, 2016-07-07 03:52:43]

Patchset #10 (id:340001) has been deleted

[Patti Lor, 2016-07-13 03:09:41]

Hi Trent, PTAL! There's a consistently failing test in
linux_chromium_chromeos_rel_ng
ProxyAuthOnUserBoardScreenTest.ProxyAuthDialogOnUserBoardScreen which I can't
replicate locally and the output isn't very helpful, do you have any ideas?

https://codereview.chromium.org/2087643003/diff/180001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_views_mac_browsertest.mm
(right):

https://codereview.chromium.org/2087643003/diff/180001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_views_mac_browsertest.mm:44:
// constrained_window component is not used in Cocoa code (disallowed via
On 2016/06/29 03:02:45, tapted wrote:
> this comment should be updated

Done - as far as I can tell I think pretty much all of this comment is no longer
true so I have changed the code to use ShowWebModalDialogViews() like it says.

https://codereview.chromium.org/2087643003/diff/180001/components/constrained...
File components/constrained_window.gypi (right):

https://codereview.chromium.org/2087643003/diff/180001/components/constrained...
components/constrained_window.gypi:27: 'sources': [
On 2016/06/29 03:02:45, tapted wrote:
> dependencies?

Done, thanks!

https://codereview.chromium.org/2087643003/diff/180001/components/constrained...
components/constrained_window.gypi:32: ['OS!="mac" or mac_views_browser==1', {
On 2016/06/29 03:02:45, tapted wrote:
> This is all views, so this condition probably belongs elsewhere (i.e. to
prevent
> the constrained_window being depended on entirely)

Done, moved back into chrome_browser_ui.gypi.

https://codereview.chromium.org/2087643003/diff/180001/components/constrained...
File components/constrained_window/constrained_window_views.cc (right):

https://codereview.chromium.org/2087643003/diff/180001/components/constrained...
components/constrained_window/constrained_window_views.cc:142:
web_modal::WebContentsModalDialogManager::CreateNativeWebModalManager(
On 2016/06/29 03:02:45, tapted wrote:
> Can this just call NativeWebContentsModalDialogManagerViews directly rather
than
> CreateNativeWebModalManager ?

I don't think so - there are 4 separate implementations of
CreateNativeWebModalManager() which are included optionally depending on what
kind of build it is. There are two which had/have NOTREACHED() in them
(test_web_contents_modal_dialog_manager_views.cc and
shell_web_contents_modal_dialog_manager.cc), and two which create different
classes (NativeWebContentsModalDialogManagerViews from
web_contents_modal_dialog_manager_views.cc, and
SingleWebContentsDialogManagerViewsMac from
create_native_web_modal_manager_cocoa.mm) depending on whether it's a views or
cocoa build. Not sure if there is another way to have this without using
CreateNativeWebModalManager().

https://codereview.chromium.org/2087643003/diff/180001/components/constrained...
File
components/constrained_window/native_web_contents_modal_dialog_manager_views.h
(right):

https://codereview.chromium.org/2087643003/diff/180001/components/constrained...
components/constrained_window/native_web_contents_modal_dialog_manager_views.h:65:
web_modal::SingleWebContentsDialogManagerDelegate* native_delegate() {
On 2016/06/29 03:02:45, tapted wrote:
> This should stay in the follow-up CL, along with the other stuff (except the
> namespace / include guard change)

Done.

https://codereview.chromium.org/2087643003/diff/180001/components/constrained...
File components/constrained_window/web_contents_modal_dialog_manager_views.cc
(right):

https://codereview.chromium.org/2087643003/diff/180001/components/constrained...
components/constrained_window/web_contents_modal_dialog_manager_views.cc:12:
SingleWebContentsDialogManager*
On 2016/06/29 03:02:45, tapted wrote:
> Add
> 
> / /TODO(..) This is a layering violation and should be deleted. Currently it's
> needed by WebContentsModalDialogManager::ShowModalDialog() which is still used
> by X and Y, but they
> should use constrained_window::ShowModalDialog instead.
> 
> (or if it's easy, just make it TODONE in this CL, delete this file, and hide
> WebContentsModalDialogManager::ShowModalDialog() with #ifdef MAC

I think I already removed WebContentsModalDialogManager::ShowModalDialog() - or
not removed, moved it to constrained_window::ShowModalDialog() instead. See
https://codereview.chromium.org/2087643003/diff/180001/components/web_modal/w....
Is that what you meant?

[tapted, 2016-07-13 04:36:19]

I'm pretty sure the
ProxyAuthOnUserBoardScreenTest.ProxyAuthDialogOnUserBoardScreen is a flake

Call me over if you get stuck on anything below - there may still be something
I've missed. You can probably add BUG=522654,492442 as well

https://codereview.chromium.org/2087643003/diff/180001/components/constrained...
File components/constrained_window/constrained_window_views.cc (right):

https://codereview.chromium.org/2087643003/diff/180001/components/constrained...
components/constrained_window/constrained_window_views.cc:142:
web_modal::WebContentsModalDialogManager::CreateNativeWebModalManager(
On 2016/07/13 03:09:41, Patti Lor wrote:
> On 2016/06/29 03:02:45, tapted wrote:
> > Can this just call NativeWebContentsModalDialogManagerViews directly rather
> than
> > CreateNativeWebModalManager ?
> 
> I don't think so - there are 4 separate implementations of
> CreateNativeWebModalManager() which are included optionally depending on what
> kind of build it is. There are two which had/have NOTREACHED() in them
> (test_web_contents_modal_dialog_manager_views.cc and
> shell_web_contents_modal_dialog_manager.cc), and two which create different
> classes (NativeWebContentsModalDialogManagerViews from
> web_contents_modal_dialog_manager_views.cc, and
> SingleWebContentsDialogManagerViewsMac from
> create_native_web_modal_manager_cocoa.mm) depending on whether it's a views or
> cocoa build. Not sure if there is another way to have this without using
> CreateNativeWebModalManager().

The notreached ones shouldn't matter (I think they're only present to workaround
the layering bug). We still care about the Mac one, and you're right about it
being needed where it is SingleWebContentsDialogManagerViewsMac can't be moved
out of c/b/ui/cocoa since it depends on things in
chrome/browser/ui/cocoa/constrained_window/*

So when building with mac_views_browser=1 we shouldn't need the version in
create_native_web_modal_manager_cocoa.mm -- inlining should work.

Then - yep - the problem is that for the current release build, this needs to
invoke the thing in create_native_web_modal_manager_cocoa.mm.

To do this, I think the following will work:
 - in this folder, add show_modal_dialog_views.cc with
NativeWebContentsModalDialogManagerViews called directly - compile this on
not-mac and when mac_views_browser=1
 - also add show_modal_dialog_mac.cc, with this implementation - compile this on
mac when mac_views_browser=0  (and give it a comment saying it's a layering
violation).

I think then the other implementations of
WebContentsModalDialogManager::CreateNativeWebModalManager can be deleted since
it's only needed when show_modal_dialog_mac.cc is used, which will always be
able to call the version in create_native_web_modal_manager_cocoa.mm

https://codereview.chromium.org/2087643003/diff/180001/components/constrained...
File components/constrained_window/web_contents_modal_dialog_manager_views.cc
(right):

https://codereview.chromium.org/2087643003/diff/180001/components/constrained...
components/constrained_window/web_contents_modal_dialog_manager_views.cc:12:
SingleWebContentsDialogManager*
On 2016/07/13 03:09:41, Patti Lor wrote:
> On 2016/06/29 03:02:45, tapted wrote:
> > Add
> > 
> > / /TODO(..) This is a layering violation and should be deleted. Currently
it's
> > needed by WebContentsModalDialogManager::ShowModalDialog() which is still
used
> > by X and Y, but they
> > should use constrained_window::ShowModalDialog instead.
> > 
> > (or if it's easy, just make it TODONE in this CL, delete this file, and hide
> > WebContentsModalDialogManager::ShowModalDialog() with #ifdef MAC
> 
> I think I already removed WebContentsModalDialogManager::ShowModalDialog() -
or
> not removed, moved it to constrained_window::ShowModalDialog() instead. See
>
https://codereview.chromium.org/2087643003/diff/180001/components/web_modal/w....
> Is that what you meant?

There's still a problem - even though this has been moved from
chrome/browser/ui/views to components/contrained_window, it's still defining a
symbol in the 'web_modal' namespace, which is backward.

That is, web_modal should be at the "bottom" of the layer cake. So it should be
able to be linked into an .so or .dll library in isolation, without needing to
link against constrained_window or chrome.dll to provide missing symbols
("backreferences"). We actually don't allow backreferences (dlls don't support
them), so the only way it works now is by having web_modal declare itself as a
static_library (".a" rather than .so or .dll)

So we should still have the comment, but this file isn't compiled for Cocoa
builds -- if that's the only reason we need it we might be able to delete it
instead, but this may depend on the other comment.

[Patti Lor, 2016-07-15 06:12:03]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-15 06:12:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-15 06:40:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-15 06:40:41]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Patti Lor, 2016-07-18 00:40:39]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-18 00:40:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Patti Lor, 2016-07-18 00:45:38]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-18 00:45:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-18 00:59:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-18 00:59:15]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Patti Lor, 2016-07-18 01:23:35]

Patchset #13 (id:420001) has been deleted

[Patti Lor, 2016-07-18 01:30:46]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-18 01:30:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-18 01:54:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-18 01:54:26]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Patti Lor, 2016-07-19 05:56:49]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-19 05:57:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-19 06:02:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-19 06:02:13]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Patti Lor, 2016-07-19 06:04:14]

Patchset #14 (id:450001) has been deleted

[Patti Lor, 2016-07-19 06:06:37]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-19 06:07:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Patti Lor, 2016-07-19 06:49:32]

Description was changed from

==========
Move web modal dialog manager files into the constrained_window component.

Move native_web_contents_modal_dialog_manager_views* and
web_contents_modal_dialog_manager_views.cc into components/constrained_window.
Additionally, move web_contents_modal_dialog_manager_.*'s ShowModalDialog()
method into constrained_window_views.*.
==========

to

==========
Move web modal dialog manager files into the constrained_window component.

Move native_web_contents_modal_dialog_manager_views* and
web_contents_modal_dialog_manager_views.cc into components/constrained_window.
Additionally, move web_contents_modal_dialog_manager.*'s ShowModalDialog()
method into constrained_window_views.*. Delete most implementations of
CreateNativeWebModalManager(), with two remaining:
create_native_web_modal_manager_cocoa.mm, for Cocoa browser builds, and
test_create_native_web_modal_manager_cocoa.cc, used only to prevent compile
errors in components_unittests, which isn't allowed to depend on the former
file.

BUG=522654,492442
==========

[commit-bot: I haz the power, 2016-07-19 07:23:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-19 07:23:31]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Patti Lor, 2016-07-19 07:39:08]

Thanks, PTAL.

https://codereview.chromium.org/2087643003/diff/180001/components/constrained...
File components/constrained_window/constrained_window_views.cc (right):

https://codereview.chromium.org/2087643003/diff/180001/components/constrained...
components/constrained_window/constrained_window_views.cc:142:
web_modal::WebContentsModalDialogManager::CreateNativeWebModalManager(
On 2016/07/13 04:36:19, tapted wrote:
> On 2016/07/13 03:09:41, Patti Lor wrote:
> > On 2016/06/29 03:02:45, tapted wrote:
> > > Can this just call NativeWebContentsModalDialogManagerViews directly
rather
> > than
> > > CreateNativeWebModalManager ?
> > 
> > I don't think so - there are 4 separate implementations of
> > CreateNativeWebModalManager() which are included optionally depending on
what
> > kind of build it is. There are two which had/have NOTREACHED() in them
> > (test_web_contents_modal_dialog_manager_views.cc and
> > shell_web_contents_modal_dialog_manager.cc), and two which create different
> > classes (NativeWebContentsModalDialogManagerViews from
> > web_contents_modal_dialog_manager_views.cc, and
> > SingleWebContentsDialogManagerViewsMac from
> > create_native_web_modal_manager_cocoa.mm) depending on whether it's a views
or
> > cocoa build. Not sure if there is another way to have this without using
> > CreateNativeWebModalManager().
> 
> The notreached ones shouldn't matter (I think they're only present to
workaround
> the layering bug). We still care about the Mac one, and you're right about it
> being needed where it is SingleWebContentsDialogManagerViewsMac can't be moved
> out of c/b/ui/cocoa since it depends on things in
> chrome/browser/ui/cocoa/constrained_window/*
> 
> So when building with mac_views_browser=1 we shouldn't need the version in
> create_native_web_modal_manager_cocoa.mm -- inlining should work.
> 
> Then - yep - the problem is that for the current release build, this needs to
> invoke the thing in create_native_web_modal_manager_cocoa.mm.
> 
> To do this, I think the following will work:
>  - in this folder, add show_modal_dialog_views.cc with
> NativeWebContentsModalDialogManagerViews called directly - compile this on
> not-mac and when mac_views_browser=1
>  - also add show_modal_dialog_mac.cc, with this implementation - compile this
on
> mac when mac_views_browser=0  (and give it a comment saying it's a layering
> violation).
> 
> I think then the other implementations of
> WebContentsModalDialogManager::CreateNativeWebModalManager can be deleted
since
> it's only needed when show_modal_dialog_mac.cc is used, which will always be
> able to call the version in create_native_web_modal_manager_cocoa.mm

Done (including deleting the other implementations, except one for tests only -
see description), let me know if the comment (in show_modal_dialog_cocoa.cc)
makes sense!

https://codereview.chromium.org/2087643003/diff/180001/components/constrained...
File components/constrained_window/web_contents_modal_dialog_manager_views.cc
(right):

https://codereview.chromium.org/2087643003/diff/180001/components/constrained...
components/constrained_window/web_contents_modal_dialog_manager_views.cc:12:
SingleWebContentsDialogManager*
On 2016/07/13 04:36:19, tapted wrote:
> On 2016/07/13 03:09:41, Patti Lor wrote:
> > On 2016/06/29 03:02:45, tapted wrote:
> > > Add
> > > 
> > > / /TODO(..) This is a layering violation and should be deleted. Currently
> it's
> > > needed by WebContentsModalDialogManager::ShowModalDialog() which is still
> used
> > > by X and Y, but they
> > > should use constrained_window::ShowModalDialog instead.
> > > 
> > > (or if it's easy, just make it TODONE in this CL, delete this file, and
hide
> > > WebContentsModalDialogManager::ShowModalDialog() with #ifdef MAC
> > 
> > I think I already removed WebContentsModalDialogManager::ShowModalDialog() -
> or
> > not removed, moved it to constrained_window::ShowModalDialog() instead. See
> >
>
https://codereview.chromium.org/2087643003/diff/180001/components/web_modal/w....
> > Is that what you meant?
> 
> There's still a problem - even though this has been moved from
> chrome/browser/ui/views to components/contrained_window, it's still defining a
> symbol in the 'web_modal' namespace, which is backward.
> 
> That is, web_modal should be at the "bottom" of the layer cake. So it should
be
> able to be linked into an .so or .dll library in isolation, without needing to
> link against constrained_window or chrome.dll to provide missing symbols
> ("backreferences"). We actually don't allow backreferences (dlls don't support
> them), so the only way it works now is by having web_modal declare itself as a
> static_library (".a" rather than .so or .dll)
> 
> So we should still have the comment, but this file isn't compiled for Cocoa
> builds -- if that's the only reason we need it we might be able to delete it
> instead, but this may depend on the other comment.

This file was deleted, but I think ShowModalDialog() is needed for other
platforms as well, not sure if that part of your first comment still applies.

[tapted, 2016-07-19 09:29:41]

lgtm % nits and the following:

can you start the CL description with a short summary of the status-quo?

Something about how the web_modal component currently relies on a layering
violation and can't link as a shared component. Then we can have a third para
about how the layering is fixed (for views at least), with the
constrained_window depending on web_modal and web_modal providing definitions of
all the symbols it declares.

We should verify too that we can change the 'static_library' in
web_modal/BUILD.gn and the 'source_set' in constrained_window/BUILD.gn into
`component`, then build a component build successfully (except on Cocoa where it
should fail to link, but mac_views_browser=1 should be OK).

https://codereview.chromium.org/2087643003/diff/470001/components/constrained...
File components/constrained_window.gypi (right):

https://codereview.chromium.org/2087643003/diff/470001/components/constrained...
components/constrained_window.gypi:32: ["OS=='macosx' and mac_views_browser==0",
{
you need to swap the quote types here and use "mac" instead of 'macosx', also it
needs to be indented another step

https://codereview.chromium.org/2087643003/diff/470001/components/constrained...
File components/constrained_window/show_modal_dialog_cocoa.cc (right):

https://codereview.chromium.org/2087643003/diff/470001/components/constrained...
components/constrained_window/show_modal_dialog_cocoa.cc:18: //
components/constrained_window/*.
add something like "For this to work at all, CreateNativeWebModalManager is
declared in the web_modal component, but not defined."

https://codereview.chromium.org/2087643003/diff/470001/components/constrained...
File components/constrained_window/test_create_native_web_modal_manager_cocoa.cc
(right):

https://codereview.chromium.org/2087643003/diff/470001/components/constrained...
components/constrained_window/test_create_native_web_modal_manager_cocoa.cc:7:
#include "components/web_modal/single_web_contents_dialog_manager.h"
These 3 includes should be unnecessary

https://codereview.chromium.org/2087643003/diff/470001/components/web_modal/w...
File components/web_modal/web_contents_modal_dialog_manager.h (right):

https://codereview.chromium.org/2087643003/diff/470001/components/web_modal/w...
components/web_modal/web_contents_modal_dialog_manager.h:33: static
SingleWebContentsDialogManager* CreateNativeWebModalManager(
nit: add a note here that this is not defined in the web_modal component

[Patti Lor, 2016-07-20 01:21:13]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-20 01:21:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 04:01:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-20 04:01:11]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Patti Lor, 2016-07-21 04:53:38]

Description was changed from

==========
Move web modal dialog manager files into the constrained_window component.

Move native_web_contents_modal_dialog_manager_views* and
web_contents_modal_dialog_manager_views.cc into components/constrained_window.
Additionally, move web_contents_modal_dialog_manager.*'s ShowModalDialog()
method into constrained_window_views.*. Delete most implementations of
CreateNativeWebModalManager(), with two remaining:
create_native_web_modal_manager_cocoa.mm, for Cocoa browser builds, and
test_create_native_web_modal_manager_cocoa.cc, used only to prevent compile
errors in components_unittests, which isn't allowed to depend on the former
file.

BUG=522654,492442
==========

to

==========
Move web modal dialog manager files into the constrained_window component.

Currently, the CreateNativeWebModalManager() method declared inside the
web_modal component is defined outside of web_modal, which means linking it as a
shared component will fail. This is a layering violation.

This is now fixed for builds using Views, with constrained_window depending on
web_modal and web_modal providing definitions of all the symbols it declares.
Most unused implementations of CreateNativeWebModalManager() have been deleted,
with two remaining: create_native_web_modal_manager_cocoa.mm for Cocoa, and
test_create_native_web_modal_manager_cocoa.cc to prevent errors compiling
components_unittests, which isn't allowed to depend on the former file's
dependencies.

BUG=522654,492442
==========

[Patti Lor, 2016-07-28 00:24:40]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-28 00:24:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-28 01:12:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-28 01:12:05]

Dry run: This issue passed the CQ dry run.

[Patti Lor, 2016-07-28 01:29:31]

Hi msw, please take a look - this was the constrained_window refactor you asked
for as a precursor for this CL: https://codereview.chromium.org/1779383002
(MacViews: Remove constrained window dependencies for certificate viewer.)

https://codereview.chromium.org/2087643003/diff/470001/components/constrained...
File components/constrained_window.gypi (right):

https://codereview.chromium.org/2087643003/diff/470001/components/constrained...
components/constrained_window.gypi:32: ["OS=='macosx' and mac_views_browser==0",
{
On 2016/07/19 09:29:41, tapted wrote:
> you need to swap the quote types here and use "mac" instead of 'macosx', also
it
> needs to be indented another step

Done, thanks!

https://codereview.chromium.org/2087643003/diff/470001/components/constrained...
File components/constrained_window/show_modal_dialog_cocoa.cc (right):

https://codereview.chromium.org/2087643003/diff/470001/components/constrained...
components/constrained_window/show_modal_dialog_cocoa.cc:18: //
components/constrained_window/*.
On 2016/07/19 09:29:41, tapted wrote:
> add something like "For this to work at all, CreateNativeWebModalManager is
> declared in the web_modal component, but not defined."

Done.

https://codereview.chromium.org/2087643003/diff/470001/components/constrained...
File components/constrained_window/test_create_native_web_modal_manager_cocoa.cc
(right):

https://codereview.chromium.org/2087643003/diff/470001/components/constrained...
components/constrained_window/test_create_native_web_modal_manager_cocoa.cc:7:
#include "components/web_modal/single_web_contents_dialog_manager.h"
On 2016/07/19 09:29:41, tapted wrote:
> These 3 includes should be unnecessary

Done.

https://codereview.chromium.org/2087643003/diff/470001/components/web_modal/w...
File components/web_modal/web_contents_modal_dialog_manager.h (right):

https://codereview.chromium.org/2087643003/diff/470001/components/web_modal/w...
components/web_modal/web_contents_modal_dialog_manager.h:33: static
SingleWebContentsDialogManager* CreateNativeWebModalManager(
On 2016/07/19 09:29:41, tapted wrote:
> nit: add a note here that this is not defined in the web_modal component

Done.

[Patti Lor, 2016-07-31 23:55:14]

patricialor@chromium.org changed reviewers:
+ msw@chromium.org

[Patti Lor, 2016-07-31 23:55:15]

Sorry, I seem to not have added you - please see the message just before this
one!

[Patti Lor, 2016-08-09 08:05:12]

Ping @msw? Thanks.

[msw, 2016-08-09 17:41:27]

lgtm

[Patti Lor, 2016-08-10 00:20:16]

patricialor@chromium.org changed reviewers:
+ brettw@chromium.org

[Patti Lor, 2016-08-10 00:20:17]

Hi brettw@, PTAL for OWNER's review on:

chrome/browser/chromeos/login/ui/captive_portal_window_proxy.cc
components/constrained_window.gypi
components/web_modal.gypi & components/web_modal/*
extensions/*

Thanks very much!

[Patti Lor, 2016-08-10 01:19:21]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 01:20:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 02:38:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 02:38:05]

Dry run: This issue passed the CQ dry run.

[brettw, 2016-08-17 17:49:55]

rs lgtm

https://codereview.chromium.org/2087643003/diff/530001/components/constrained...
File components/constrained_window.gypi (right):

https://codereview.chromium.org/2087643003/diff/530001/components/constrained...
components/constrained_window.gypi:1: # Copyright 2014 The Chromium Authors. All
rights reserved.
Just revert this file, we don't need to keep the GYP build in sync any more.

https://codereview.chromium.org/2087643003/diff/530001/components/web_modal.gypi
File components/web_modal.gypi (right):

https://codereview.chromium.org/2087643003/diff/530001/components/web_modal.g...
components/web_modal.gypi:1: # Copyright (c) 2013 The Chromium Authors. All
rights reserved.
Ditto, just revert this file.

https://codereview.chromium.org/2087643003/diff/530001/components/web_modal/w...
File components/web_modal/web_contents_modal_dialog_manager.h (right):

https://codereview.chromium.org/2087643003/diff/530001/components/web_modal/w...
components/web_modal/web_contents_modal_dialog_manager.h:11: #include
"base/macros.h"
You should include build/build_config.h for OS_MACOSX definition you use below
(in practice somebody is probably already doing this for you which is why you
didn't notice).

[Patti Lor, 2016-08-18 01:24:53]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 01:25:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-18 02:27:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-18 02:27:29]

Dry run: This issue passed the CQ dry run.

[Patti Lor, 2016-08-18 03:05:49]

Thanks for the review!

https://codereview.chromium.org/2087643003/diff/530001/components/constrained...
File components/constrained_window.gypi (right):

https://codereview.chromium.org/2087643003/diff/530001/components/constrained...
components/constrained_window.gypi:1: # Copyright 2014 The Chromium Authors. All
rights reserved.
On 2016/08/17 17:49:55, brettw (ping on IM after 24h) wrote:
> Just revert this file, we don't need to keep the GYP build in sync any more.

Done.

https://codereview.chromium.org/2087643003/diff/530001/components/web_modal.gypi
File components/web_modal.gypi (right):

https://codereview.chromium.org/2087643003/diff/530001/components/web_modal.g...
components/web_modal.gypi:1: # Copyright (c) 2013 The Chromium Authors. All
rights reserved.
On 2016/08/17 17:49:55, brettw (ping on IM after 24h) wrote:
> Ditto, just revert this file.

Done.

https://codereview.chromium.org/2087643003/diff/530001/components/web_modal/w...
File components/web_modal/web_contents_modal_dialog_manager.h (right):

https://codereview.chromium.org/2087643003/diff/530001/components/web_modal/w...
components/web_modal/web_contents_modal_dialog_manager.h:11: #include
"base/macros.h"
On 2016/08/17 17:49:55, brettw (ping on IM after 24h) wrote:
> You should include build/build_config.h for OS_MACOSX definition you use below
> (in practice somebody is probably already doing this for you which is why you
> didn't notice).

Done, thanks for picking that up!

[Patti Lor, 2016-08-18 03:06:01]

The CQ bit was checked by patricialor@chromium.org

[Patti Lor, 2016-08-18 03:06:02]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org,
msw@chromium.org, brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/2087643003/#ps550001
(title: "Revert changes to gyp files, IWYU for #ifdef (OS_MACOSX).")

[commit-bot: I haz the power, 2016-08-18 03:06:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-18 03:57:33]

Committed patchset #18 (id:550001)

[commit-bot: I haz the power, 2016-08-18 04:01:13]

Description was changed from

==========
Move web modal dialog manager files into the constrained_window component.

Currently, the CreateNativeWebModalManager() method declared inside the
web_modal component is defined outside of web_modal, which means linking it as a
shared component will fail. This is a layering violation.

This is now fixed for builds using Views, with constrained_window depending on
web_modal and web_modal providing definitions of all the symbols it declares.
Most unused implementations of CreateNativeWebModalManager() have been deleted,
with two remaining: create_native_web_modal_manager_cocoa.mm for Cocoa, and
test_create_native_web_modal_manager_cocoa.cc to prevent errors compiling
components_unittests, which isn't allowed to depend on the former file's
dependencies.

BUG=522654,492442
==========

to

==========
Move web modal dialog manager files into the constrained_window component.

Currently, the CreateNativeWebModalManager() method declared inside the
web_modal component is defined outside of web_modal, which means linking it as a
shared component will fail. This is a layering violation.

This is now fixed for builds using Views, with constrained_window depending on
web_modal and web_modal providing definitions of all the symbols it declares.
Most unused implementations of CreateNativeWebModalManager() have been deleted,
with two remaining: create_native_web_modal_manager_cocoa.mm for Cocoa, and
test_create_native_web_modal_manager_cocoa.cc to prevent errors compiling
components_unittests, which isn't allowed to depend on the former file's
dependencies.

BUG=522654,492442

Committed: https://crrev.com/7a65925409017abde6fcf3f4213f779707c0ab57
Cr-Commit-Position: refs/heads/master@{#412730}
==========

[commit-bot: I haz the power, 2016-08-18 04:01:15]

Patchset 18 (id:??) landed as
https://crrev.com/7a65925409017abde6fcf3f4213f779707c0ab57
Cr-Commit-Position: refs/heads/master@{#412730}