Componentize Constrained Window Views

Componentize Constrained Window Views

* Added ConstrainedWindowViewsClient to delegate browser/extensions dependent code.

BUG=410499
Committed: https://crrev.com/136691ad8af860f6bd89f17b9f3c9693c7d780bc
Cr-Commit-Position: refs/heads/master@{#301203}

[oshima, 2014-10-21 22:51:12]

Patchset #2 (id:20001) has been deleted

[oshima, 2014-10-21 22:51:16]

Patchset #1 (id:1) has been deleted

[oshima, 2014-10-22 00:03:18]

Patchset #2 (id:60001) has been deleted

[oshima, 2014-10-22 00:03:22]

Patchset #1 (id:40001) has been deleted

[oshima, 2014-10-22 00:03:41]

Patchset #1 (id:80001) has been deleted

[oshima, 2014-10-22 00:37:03]

Patchset #1 (id:100001) has been deleted

[oshima, 2014-10-22 00:38:10]

oshima@chromium.org changed reviewers:
+ blundell@chromium.org, msw@chromium.org

[blundell, 2014-10-22 13:49:54]

//components LGTM, but I defer to msw@ on reviewing the actual code going into
the new component.

https://codereview.chromium.org/658383003/diff/130001/components/constrained_...
File components/constrained_window.gypi (right):

https://codereview.chromium.org/658383003/diff/130001/components/constrained_...
components/constrained_window.gypi:7: 'target_name': 'constrained_window',
There should be a BUILD.gn file for this component too.

[oshima, 2014-10-22 17:07:37]

Patchset #3 (id:140001) has been deleted

[oshima, 2014-10-22 17:09:52]

https://codereview.chromium.org/658383003/diff/130001/components/constrained_...
File components/constrained_window.gypi (right):

https://codereview.chromium.org/658383003/diff/130001/components/constrained_...
components/constrained_window.gypi:7: 'target_name': 'constrained_window',
On 2014/10/22 13:49:54, blundell wrote:
> There should be a BUILD.gn file for this component too.

Done.

[msw, 2014-10-22 21:32:44]

https://codereview.chromium.org/658383003/diff/160001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_constrained_window_views_client.cc (right):

https://codereview.chromium.org/658383003/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_constrained_window_views_client.cc:8: #include
"components/constrained_window/constrained_window_views_client.h"
nit: not needed if included by the header.

https://codereview.chromium.org/658383003/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_constrained_window_views_client.cc:18: virtual
~ChromeConstrainedWindowViewsClient() {}
nit: virtual -> override (or final?) here and below.
see: http://codereview.chromium.org/671653002

https://codereview.chromium.org/658383003/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_constrained_window_views_client.cc:33: if
(browser) {
nit: return browser ? browser->GetWebContentsModalDialogHost() : nullptr;

https://codereview.chromium.org/658383003/diff/160001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_constrained_window_views_client.h (right):

https://codereview.chromium.org/658383003/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_constrained_window_views_client.h:11: // Creates
a ConstrainedWindowViewsClient for chrome environment.
nit: "the Chrome"

https://codereview.chromium.org/658383003/diff/160001/chrome/browser/ui/views...
File chrome/browser/ui/views/hung_renderer_view.cc (right):

https://codereview.chromium.org/658383003/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/hung_renderer_view.cc:234:
ChromeWebModalDialogManagerDelegate* manager = browser;
nit: Is |manager| necessary?

https://codereview.chromium.org/658383003/diff/160001/chrome/chrome_browser_u...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/658383003/diff/160001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:2777:
'<(DEPTH)/components/components.gyp:constrained_window',
This perhaps should be simply contingent on 'toolkit_views==1', not sure if the
'OS!="mac"' condition is necessary, but perhaps it doesn't matter much for now.
+CC andresantoso for thoughts.

https://codereview.chromium.org/658383003/diff/160001/components/BUILD.gn
File components/BUILD.gn (right):

https://codereview.chromium.org/658383003/diff/160001/components/BUILD.gn#new...
components/BUILD.gn:228: "//components/constrained_window:unit_tests",
Does this need to be contingent on |toolkit_views| too?

https://codereview.chromium.org/658383003/diff/160001/components/OWNERS
File components/OWNERS (right):

https://codereview.chromium.org/658383003/diff/160001/components/OWNERS#newco...
components/OWNERS:17: per-file constrained_window*=ben@chromium.org
I'm not so sure about these owners, maybe gbillock or others ought to be
represented here (finnur?), and likely the others inherited from c/b/ui will be
stripped down later...

https://codereview.chromium.org/658383003/diff/160001/components/constrained_...
File components/constrained_window.gypi (right):

https://codereview.chromium.org/658383003/diff/160001/components/constrained_...
components/constrained_window.gypi:12: '<(DEPTH)/skia/skia.gyp:skia',
Skia doesn't appear to be used directly, is there a transitive dependency
through views, and if so, would removing this explicit dependency entry be okay?

https://codereview.chromium.org/658383003/diff/160001/components/constrained_...
components/constrained_window.gypi:18:
'constrained_window/constrained_window_views.cc',
Shouldn't this list include constrained_window_views_client.h?

https://codereview.chromium.org/658383003/diff/160001/components/constrained_...
File components/constrained_window/BUILD.gn (right):

https://codereview.chromium.org/658383003/diff/160001/components/constrained_...
components/constrained_window/BUILD.gn:7: "constrained_window_views.cc",
Ditto: Shouldn't this list include constrained_window_views_client.h?

https://codereview.chromium.org/658383003/diff/160001/components/constrained_...
components/constrained_window/BUILD.gn:14: "//skia",
Ditto: Skia doesn't appear to be used directly, is there a transitive dependency
through views, and if so, would removing this explicit dependency entry be okay?

https://codereview.chromium.org/658383003/diff/160001/components/constrained_...
File components/constrained_window/constrained_window_views.cc (right):

https://codereview.chromium.org/658383003/diff/160001/components/constrained_...
components/constrained_window/constrained_window_views.cc:22:
ConstrainedWindowViewsClient* client_ = NULL;
nit: should this be |g_client|?

https://codereview.chromium.org/658383003/diff/160001/components/constrained_...
components/constrained_window/constrained_window_views.cc:127:
web_modal::ModalDialogHost* dialog_host) {
nit fix indent

https://codereview.chromium.org/658383003/diff/160001/components/constrained_...
components/constrained_window/constrained_window_views.cc:167:
ModalDialogHostObserver* dialog_host_observer =
Huh, what's the ownership story here? Is this intentionally leaked?

https://codereview.chromium.org/658383003/diff/160001/components/constrained_...
File components/constrained_window/constrained_window_views_client.h (right):

https://codereview.chromium.org/658383003/diff/160001/components/constrained_...
components/constrained_window/constrained_window_views_client.h:22: // Returns
an web contents that a constrained window should be modal to
nit: replace "an" with "the" or "a".

https://codereview.chromium.org/658383003/diff/160001/components/constrained_...
components/constrained_window/constrained_window_views_client.h:23: // in
embedder's context.
nit: "in the embedder's"

[Andre, 2014-10-22 21:45:02]

andresantoso@chromium.org changed reviewers:
+ andresantoso@chromium.org

[Andre, 2014-10-22 21:45:04]

https://codereview.chromium.org/658383003/diff/160001/chrome/chrome_browser_u...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/658383003/diff/160001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:2777:
'<(DEPTH)/components/components.gyp:constrained_window',
On 2014/10/22 21:32:43, msw wrote:
> This perhaps should be simply contingent on 'toolkit_views==1', not sure if
the
> 'OS!="mac"' condition is necessary, but perhaps it doesn't matter much for
now.
> +CC andresantoso for thoughts.

Yep, but we probably should do that when contrained_window_views compiles for
MacViews, which it does not yet right now.
So this looks right for now.

[oshima, 2014-10-22 22:37:55]

Patchset #4 (id:180001) has been deleted

[oshima, 2014-10-22 22:39:07]

oshima@chromium.org changed reviewers:
+ finnur@chromium.org, gbillock@chromium.org

[oshima, 2014-10-22 22:39:09]

+finnur, gbillock

are you ok to be the owner of this component ?

https://codereview.chromium.org/658383003/diff/160001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_constrained_window_views_client.cc (right):

https://codereview.chromium.org/658383003/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_constrained_window_views_client.cc:8: #include
"components/constrained_window/constrained_window_views_client.h"
On 2014/10/22 21:32:43, msw wrote:
> nit: not needed if included by the header.

Done.

https://codereview.chromium.org/658383003/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_constrained_window_views_client.cc:18: virtual
~ChromeConstrainedWindowViewsClient() {}
On 2014/10/22 21:32:43, msw wrote:
> nit: virtual -> override (or final?) here and below.
> see: http://codereview.chromium.org/671653002 

Oh, i didn't know about it. done.

https://codereview.chromium.org/658383003/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_constrained_window_views_client.cc:33: if
(browser) {
On 2014/10/22 21:32:43, msw wrote:
> nit: return browser ? browser->GetWebContentsModalDialogHost() : nullptr;

GetWebContentsModalDialogHost is private on Browser. Let me know if you want to
change Browser interface.

https://codereview.chromium.org/658383003/diff/160001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_constrained_window_views_client.h (right):

https://codereview.chromium.org/658383003/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_constrained_window_views_client.h:11: // Creates
a ConstrainedWindowViewsClient for chrome environment.
On 2014/10/22 21:32:43, msw wrote:
> nit: "the Chrome"

Done.

https://codereview.chromium.org/658383003/diff/160001/chrome/browser/ui/views...
File chrome/browser/ui/views/hung_renderer_view.cc (right):

https://codereview.chromium.org/658383003/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/hung_renderer_view.cc:234:
ChromeWebModalDialogManagerDelegate* manager = browser;
On 2014/10/22 21:32:43, msw wrote:
> nit: Is |manager| necessary?

See my comment above.

https://codereview.chromium.org/658383003/diff/160001/components/BUILD.gn
File components/BUILD.gn (right):

https://codereview.chromium.org/658383003/diff/160001/components/BUILD.gn#new...
components/BUILD.gn:228: "//components/constrained_window:unit_tests",
On 2014/10/22 21:32:43, msw wrote:
> Does this need to be contingent on |toolkit_views| too?

Done.

https://codereview.chromium.org/658383003/diff/160001/components/OWNERS
File components/OWNERS (right):

https://codereview.chromium.org/658383003/diff/160001/components/OWNERS#newco...
components/OWNERS:17: per-file constrained_window*=ben@chromium.org
On 2014/10/22 21:32:43, msw wrote:
> I'm not so sure about these owners, maybe gbillock or others ought to be
> represented here (finnur?), and likely the others inherited from c/b/ui will
be
> stripped down later...

Added gbillock and finnur, plus you as the current owner of c/b/ui/views

https://codereview.chromium.org/658383003/diff/160001/components/constrained_...
File components/constrained_window.gypi (right):

https://codereview.chromium.org/658383003/diff/160001/components/constrained_...
components/constrained_window.gypi:12: '<(DEPTH)/skia/skia.gyp:skia',
On 2014/10/22 21:32:43, msw wrote:
> Skia doesn't appear to be used directly, is there a transitive dependency
> through views, and if so, would removing this explicit dependency entry be
okay?

This is necessary for skia header included indirectly.

https://codereview.chromium.org/658383003/diff/160001/components/constrained_...
components/constrained_window.gypi:18:
'constrained_window/constrained_window_views.cc',
On 2014/10/22 21:32:43, msw wrote:
> Shouldn't this list include constrained_window_views_client.h?

Done.

https://codereview.chromium.org/658383003/diff/160001/components/constrained_...
File components/constrained_window/BUILD.gn (right):

https://codereview.chromium.org/658383003/diff/160001/components/constrained_...
components/constrained_window/BUILD.gn:7: "constrained_window_views.cc",
On 2014/10/22 21:32:43, msw wrote:
> Ditto: Shouldn't this list include constrained_window_views_client.h?

Done.

https://codereview.chromium.org/658383003/diff/160001/components/constrained_...
File components/constrained_window/constrained_window_views.cc (right):

https://codereview.chromium.org/658383003/diff/160001/components/constrained_...
components/constrained_window/constrained_window_views.cc:22:
ConstrainedWindowViewsClient* client_ = NULL;
On 2014/10/22 21:32:43, msw wrote:
> nit: should this be |g_client|?

Technically speaking, this is not global variable (file scoped).
I can change to g_client_ if you want. let me know.

https://codereview.chromium.org/658383003/diff/160001/components/constrained_...
components/constrained_window/constrained_window_views.cc:127:
web_modal::ModalDialogHost* dialog_host) {
On 2014/10/22 21:32:43, msw wrote:
> nit fix indent

Done.

https://codereview.chromium.org/658383003/diff/160001/components/constrained_...
components/constrained_window/constrained_window_views.cc:167:
ModalDialogHostObserver* dialog_host_observer =
On 2014/10/22 21:32:43, msw wrote:
> Huh, what's the ownership story here? Is this intentionally leaked?

it's owned by the widget and deleted in OnWidgetClosing()

https://codereview.chromium.org/658383003/diff/160001/components/constrained_...
File components/constrained_window/constrained_window_views_client.h (right):

https://codereview.chromium.org/658383003/diff/160001/components/constrained_...
components/constrained_window/constrained_window_views_client.h:22: // Returns
an web contents that a constrained window should be modal to
On 2014/10/22 21:32:44, msw wrote:
> nit: replace "an" with "the" or "a".

Done.

https://codereview.chromium.org/658383003/diff/160001/components/constrained_...
components/constrained_window/constrained_window_views_client.h:23: // in
embedder's context.
On 2014/10/22 21:32:44, msw wrote:
> nit: "in the embedder's"

Done.

[Finnur, 2014-10-23 09:05:51]

https://codereview.chromium.org/658383003/diff/160001/components/OWNERS
File components/OWNERS (right):

https://codereview.chromium.org/658383003/diff/160001/components/OWNERS#newco...
components/OWNERS:17: per-file constrained_window*=ben@chromium.org
I don't think I'd be an effective owner for this.

[msw, 2014-10-23 18:49:05]

lgtm, not that i want to be an owner of this either!
We really need to find new owners for some UI code!

https://codereview.chromium.org/658383003/diff/160001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_constrained_window_views_client.cc (right):

https://codereview.chromium.org/658383003/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_constrained_window_views_client.cc:33: if
(browser) {
On 2014/10/22 22:39:08, oshima wrote:
> On 2014/10/22 21:32:43, msw wrote:
> > nit: return browser ? browser->GetWebContentsModalDialogHost() : nullptr;
> 
> GetWebContentsModalDialogHost is private on Browser. Let me know if you want
to
> change Browser interface.

Ahhhh, got it! This is fine as-is.

https://codereview.chromium.org/658383003/diff/160001/chrome/browser/ui/views...
File chrome/browser/ui/views/hung_renderer_view.cc (right):

https://codereview.chromium.org/658383003/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/hung_renderer_view.cc:234:
ChromeWebModalDialogManagerDelegate* manager = browser;
On 2014/10/22 22:39:08, oshima wrote:
> On 2014/10/22 21:32:43, msw wrote:
> > nit: Is |manager| necessary?
> 
> See my comment above.

Acknowledged.

https://codereview.chromium.org/658383003/diff/160001/components/constrained_...
File components/constrained_window.gypi (right):

https://codereview.chromium.org/658383003/diff/160001/components/constrained_...
components/constrained_window.gypi:12: '<(DEPTH)/skia/skia.gyp:skia',
On 2014/10/22 22:39:09, oshima wrote:
> On 2014/10/22 21:32:43, msw wrote:
> > Skia doesn't appear to be used directly, is there a transitive dependency
> > through views, and if so, would removing this explicit dependency entry be
> okay?
> 
> This is necessary for skia header included indirectly.

Acknowledged.

https://codereview.chromium.org/658383003/diff/160001/components/constrained_...
File components/constrained_window/constrained_window_views.cc (right):

https://codereview.chromium.org/658383003/diff/160001/components/constrained_...
components/constrained_window/constrained_window_views.cc:22:
ConstrainedWindowViewsClient* client_ = NULL;
On 2014/10/22 22:39:09, oshima wrote:
> On 2014/10/22 21:32:43, msw wrote:
> > nit: should this be |g_client|?
> 
> Technically speaking, this is not global variable (file scoped).
> I can change to g_client_ if you want. let me know.

Hmm, maybe just |constrained_window_views_client| or something more descriptive
than |client|.

https://codereview.chromium.org/658383003/diff/160001/components/constrained_...
components/constrained_window/constrained_window_views.cc:167:
ModalDialogHostObserver* dialog_host_observer =
On 2014/10/22 22:39:09, oshima wrote:
> On 2014/10/22 21:32:43, msw wrote:
> > Huh, what's the ownership story here? Is this intentionally leaked?
> 
> it's owned by the widget and deleted in OnWidgetClosing()

Acknowledged.

[oshima, 2014-10-23 20:54:47]

https://codereview.chromium.org/658383003/diff/160001/components/OWNERS
File components/OWNERS (right):

https://codereview.chromium.org/658383003/diff/160001/components/OWNERS#newco...
components/OWNERS:17: per-file constrained_window*=ben@chromium.org
On 2014/10/23 09:05:50, Finnur wrote:
> I don't think I'd be an effective owner for this.

removed.

https://codereview.chromium.org/658383003/diff/160001/components/constrained_...
File components/constrained_window/constrained_window_views.cc (right):

https://codereview.chromium.org/658383003/diff/160001/components/constrained_...
components/constrained_window/constrained_window_views.cc:22:
ConstrainedWindowViewsClient* client_ = NULL;
On 2014/10/23 18:49:05, msw wrote:
> On 2014/10/22 22:39:09, oshima wrote:
> > On 2014/10/22 21:32:43, msw wrote:
> > > nit: should this be |g_client|?
> > 
> > Technically speaking, this is not global variable (file scoped).
> > I can change to g_client_ if you want. let me know.
> 
> Hmm, maybe just |constrained_window_views_client| or something more
descriptive
> than |client|.

Done.

[oshima, 2014-10-23 20:59:16]

based on git blame, you probably needs to be one of owners :)
I'll ask wittman/rfevang if they can be the owner in separate CL.

[oshima, 2014-10-23 20:59:22]

The CQ bit was checked by oshima@chromium.org

[oshima, 2014-10-23 20:59:25]

The CQ bit was unchecked by oshima@chromium.org

[oshima, 2014-10-23 21:00:17]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-10-23 21:01:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/658383003/220001

[commit-bot: I haz the power, 2014-10-23 21:15:05]

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

[commit-bot: I haz the power, 2014-10-23 21:15:09]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Andre, 2014-10-23 22:11:13]

andresantoso@chromium.org changed reviewers:
- andresantoso@chromium.org

[oshima, 2014-10-24 11:11:51]

Patchset #6 (id:240001) has been deleted

[oshima, 2014-10-24 11:12:01]

Patchset #5 (id:220001) has been deleted

[oshima, 2014-10-24 15:34:41]

oshima@chromium.org changed reviewers:
+ sky@chromium.org
- finnur@chromium.org

[oshima, 2014-10-24 15:34:42]

+sky for chrome/test/base change

[sky, 2014-10-24 18:04:22]

LGTM

https://codereview.chromium.org/658383003/diff/300001/chrome/test/base/browse...
File chrome/test/base/browser_with_test_window_test.cc (right):

https://codereview.chromium.org/658383003/diff/300001/chrome/test/base/browse...
chrome/test/base/browser_with_test_window_test.cc:125:
SetConstrainedWindowViewsClient(scoped_ptr<ConstrainedWindowViewsClient>());
I think there was a conversion from nullptr->scoped_ptr<X>?

[oshima, 2014-10-24 20:36:47]

https://codereview.chromium.org/658383003/diff/300001/chrome/test/base/browse...
File chrome/test/base/browser_with_test_window_test.cc (right):

https://codereview.chromium.org/658383003/diff/300001/chrome/test/base/browse...
chrome/test/base/browser_with_test_window_test.cc:125:
SetConstrainedWindowViewsClient(scoped_ptr<ConstrainedWindowViewsClient>());
On 2014/10/24 18:04:22, sky wrote:
> I think there was a conversion from nullptr->scoped_ptr<X>?

Hmm, it didn't work.

../../chrome/test/base/browser_with_test_window_test.cc:125:42: error: expected
')'
 
SetConstrainedWindowViewsClient(nullptr->scoped_ptr<ConstrainedWindowViewsClient>);

(I tried ..scoped_ptr<X>() but got the same error)



I may be doing something wrong, but it's actuall longer than above, so I'll
leave it as is.

[oshima, 2014-10-24 20:36:56]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-10-24 20:39:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/658383003/300001

[commit-bot: I haz the power, 2014-10-24 21:54:28]

Committed patchset #7 (id:300001)

[commit-bot: I haz the power, 2014-10-24 21:55:08]

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

[blundell, 2014-10-27 06:54:45]

https://codereview.chromium.org/658383003/diff/300001/chrome/test/base/browse...
File chrome/test/base/browser_with_test_window_test.cc (right):

https://codereview.chromium.org/658383003/diff/300001/chrome/test/base/browse...
chrome/test/base/browser_with_test_window_test.cc:125:
SetConstrainedWindowViewsClient(scoped_ptr<ConstrainedWindowViewsClient>());
On 2014/10/24 20:36:47, oshima wrote:
> On 2014/10/24 18:04:22, sky wrote:
> > I think there was a conversion from nullptr->scoped_ptr<X>?
> 
> Hmm, it didn't work.
> 
> ../../chrome/test/base/browser_with_test_window_test.cc:125:42: error:
expected
> ')'
>  
>
SetConstrainedWindowViewsClient(nullptr->scoped_ptr<ConstrainedWindowViewsClient>);
> 
> (I tried ..scoped_ptr<X>() but got the same error)
> 
> 
> 
> I may be doing something wrong, but it's actuall longer than above, so I'll
> leave it as is.

I think the idea is that you now can do
SetConstrainedWindowViewsClient(nullptr), and the conversion happens
automagically.