aura: Allow creating content_shell without views.

aura: Allow creating content_shell without views.

This builds content_shell with use_aura=1 and toolkit_views=0.

BUG=none
R=derat@chromium.org, jamescook@chromium.org, jochen@chromium.org

Previously, on this issue:
 * Landed in r229507
 * Speculatively reverted in r229521.
 * Relanded in r229524, since the revert didn't fix anything.
 * Reverted in r229525 because it broken win-blink bots.
 * Committed again in r229671 with an incorrect fix.
 * Reverted in r229683

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

[sadrul, 2013-10-17 05:09:11]

I heard there was interest in building content_shell without views. This patch
lets us do that ('use_aura=1' and 'toolkit_views=0'). It's a relatively small
amount of code, and there's opportunity to make this better by some trivial
refactoring.

[Daniel Erat, 2013-10-17 14:41:18]

Thanks for doing this!

https://codereview.chromium.org/26695007/diff/1/content/content_shell.gypi
File content/content_shell.gypi (right):

https://codereview.chromium.org/26695007/diff/1/content/content_shell.gypi#ne...
content/content_shell.gypi:236: '../ui/base/strings/ui_strings.gyp:ui_strings',
nit: is it possible to refactor these a bit further to remove some duplication?
e.g.

  use_aura==1:
    add aura and ui_strings deps
    exclude shell_{gtk,win}.cc
  toolkit_views==1:
    add webview, views, views_test_support, ui_resources deps
    exclude shell_aura.cc
  else (toolkit_views==0):
    exclude shell_views.cc

https://codereview.chromium.org/26695007/diff/1/content/shell/browser/shell.h
File content/shell/browser/shell.h (right):

https://codereview.chromium.org/26695007/diff/1/content/shell/browser/shell.h...
content/shell/browser/shell.h:270: static scoped_ptr<aura::RootWindow>
root_window_;
should this be a regular pointer instead?

https://codereview.chromium.org/26695007/diff/1/content/shell/browser/shell_a...
File content/shell/browser/shell_aura.cc (right):

https://codereview.chromium.org/26695007/diff/1/content/shell/browser/shell_a...
content/shell/browser/shell_aura.cc:160:
aura::client::SetFocusClient(root_window_.get(), new aura::FocusManager());
is there a reason not to make these be members of Shell?

[Daniel Erat, 2013-10-17 14:42:29]

(Or maybe I misunderstood and there's no plan to check this in immediately, in
which case you can keep the thanks and ignore my comments.) :-)

[James Cook, 2013-10-17 17:18:35]

https://codereview.chromium.org/26695007/diff/1/content/shell/browser/shell_a...
File content/shell/browser/shell_aura.cc (right):

https://codereview.chromium.org/26695007/diff/1/content/shell/browser/shell_a...
content/shell/browser/shell_aura.cc:135: void Shell::PlatformExit() {
Should this clean up root_window_?

I think in the other version MinimalShell owns the RootWindow and is deleted
here.

[sadrul, 2013-10-17 17:48:39]

On 2013/10/17 14:42:29, Daniel Erat wrote:
> (Or maybe I misunderstood and there's no plan to check this in immediately, in
> which case you can keep the thanks and ignore my comments.) :-)

I am happy to make the suggested changes; they should be mostly small/trivial.
But I do not really know if we want this in the tree or not. It may be useful
for anyone wanting to experiment with just content/, with as little of the rest
of the code as possible. But if this is not used regularly by someone (some
sub-team, for example), then there's a chance this will bitrot, and add
unnecessary maintenance burden. What do you guys think?

[James Cook, 2013-10-17 17:56:28]

On 2013/10/17 17:48:39, sadrul wrote:
> On 2013/10/17 14:42:29, Daniel Erat wrote:
> > (Or maybe I misunderstood and there's no plan to check this in immediately,
in
> > which case you can keep the thanks and ignore my comments.) :-)
> 
> I am happy to make the suggested changes; they should be mostly small/trivial.
> But I do not really know if we want this in the tree or not. It may be useful
> for anyone wanting to experiment with just content/, with as little of the
rest
> of the code as possible. But if this is not used regularly by someone (some
> sub-team, for example), then there's a chance this will bitrot, and add
> unnecessary maintenance burden. What do you guys think?

We might want this some day for app_shell. But at present we're a long way from
having something useful running there.  Dan, WDYT?

[rjkroege, 2013-10-17 20:44:51]

On 2013/10/17 17:56:28, James Cook wrote:
> On 2013/10/17 17:48:39, sadrul wrote:
> > On 2013/10/17 14:42:29, Daniel Erat wrote:
> > > (Or maybe I misunderstood and there's no plan to check this in
immediately,
> in
> > > which case you can keep the thanks and ignore my comments.) :-)
> > 
> > I am happy to make the suggested changes; they should be mostly
small/trivial.
> > But I do not really know if we want this in the tree or not. It may be
useful
> > for anyone wanting to experiment with just content/, with as little of the
> rest
> > of the code as possible. But if this is not used regularly by someone (some
> > sub-team, for example), then there's a chance this will bitrot, and add
> > unnecessary maintenance burden. What do you guys think?
> 
> We might want this some day for app_shell. But at present we're a long way
from
> having something useful running there.  Dan, WDYT?

The ability to suppress toolkit views from content_shell would be very useful
for embedded ozone=1 targets. Maybe this could land now for that and get used in
app_shell at the appropriate time?

[Daniel Erat, 2013-10-17 22:57:22]

On 2013/10/17 20:44:51, rjkroege wrote:
> On 2013/10/17 17:56:28, James Cook wrote:
> > On 2013/10/17 17:48:39, sadrul wrote:
> > > On 2013/10/17 14:42:29, Daniel Erat wrote:
> > > > (Or maybe I misunderstood and there's no plan to check this in
> immediately,
> > in
> > > > which case you can keep the thanks and ignore my comments.) :-)
> > > 
> > > I am happy to make the suggested changes; they should be mostly
> small/trivial.
> > > But I do not really know if we want this in the tree or not. It may be
> useful
> > > for anyone wanting to experiment with just content/, with as little of the
> > rest
> > > of the code as possible. But if this is not used regularly by someone
(some
> > > sub-team, for example), then there's a chance this will bitrot, and add
> > > unnecessary maintenance burden. What do you guys think?
> > 
> > We might want this some day for app_shell. But at present we're a long way
> from
> > having something useful running there.  Dan, WDYT?
> 
> The ability to suppress toolkit views from content_shell would be very useful
> for embedded ozone=1 targets. Maybe this could land now for that and get used
in
> app_shell at the appropriate time?

Makes sense to me. I think we're still unsure whether app_shell will need Views
or not.

[James Cook, 2013-10-17 23:00:42]

On 2013/10/17 22:57:22, Daniel Erat wrote:
> On 2013/10/17 20:44:51, rjkroege wrote:
> > On 2013/10/17 17:56:28, James Cook wrote:
> > > On 2013/10/17 17:48:39, sadrul wrote:
> > > > On 2013/10/17 14:42:29, Daniel Erat wrote:
> > > > > (Or maybe I misunderstood and there's no plan to check this in
> > immediately,
> > > in
> > > > > which case you can keep the thanks and ignore my comments.) :-)
> > > > 
> > > > I am happy to make the suggested changes; they should be mostly
> > small/trivial.
> > > > But I do not really know if we want this in the tree or not. It may be
> > useful
> > > > for anyone wanting to experiment with just content/, with as little of
the
> > > rest
> > > > of the code as possible. But if this is not used regularly by someone
> (some
> > > > sub-team, for example), then there's a chance this will bitrot, and add
> > > > unnecessary maintenance burden. What do you guys think?
> > > 
> > > We might want this some day for app_shell. But at present we're a long way
> > from
> > > having something useful running there.  Dan, WDYT?
> > 
> > The ability to suppress toolkit views from content_shell would be very
useful
> > for embedded ozone=1 targets. Maybe this could land now for that and get
used
> in
> > app_shell at the appropriate time?
> 
> Makes sense to me. I think we're still unsure whether app_shell will need
Views
> or not.

SGTM

[sadrul, 2013-10-18 13:05:19]

Addressed the comments, and cleaned up the leaks.

+jochen@ for content/shell/OWNERS

https://codereview.chromium.org/26695007/diff/1/content/content_shell.gypi
File content/content_shell.gypi (right):

https://codereview.chromium.org/26695007/diff/1/content/content_shell.gypi#ne...
content/content_shell.gypi:236: '../ui/base/strings/ui_strings.gyp:ui_strings',
On 2013/10/17 14:41:19, Daniel Erat wrote:
> nit: is it possible to refactor these a bit further to remove some
duplication?
> e.g.
> 
>   use_aura==1:
>     add aura and ui_strings deps
>     exclude shell_{gtk,win}.cc
>   toolkit_views==1:
>     add webview, views, views_test_support, ui_resources deps
>     exclude shell_aura.cc
>   else (toolkit_views==0):
>     exclude shell_views.cc

Done.

[Daniel Erat, 2013-10-18 14:46:00]

lgtm

[James Cook, 2013-10-18 16:31:38]

lgtm

[jochen (gone - plz use gerrit), 2013-10-18 17:01:14]

lgtm

[sadrul, 2013-10-19 02:04:39]

Committed patchset #3 manually as r229507 (presubmit successful).

[pfeldman, 2013-10-19 07:07:05]

This has been rolled out due to
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20Builder/b....

[sadrul, 2013-10-20 06:08:39]

Committed patchset #6 manually as r229671 (presubmit successful).

[pfeldman, 2013-10-20 06:50:17]

not lgtm. Please don't land this again, it breaks webkit builder.

http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20Builder%2...

[sadrul, 2013-10-20 06:50:59]

On 2013/10/20 06:50:17, pfeldman wrote:
> not lgtm. Please don't land this again, it breaks webkit builder.
> 
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%2520Win%2520Build...

Is there a trybot with this configuration?

[pfeldman, 2013-10-20 06:51:20]

On 2013/10/20 06:50:17, pfeldman wrote:
> not lgtm. Please don't land this again, it breaks webkit builder.
> 
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%2520Win%2520Build...

http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20Builder%2...

[sadrul, 2013-10-20 06:52:40]

On 2013/10/20 06:50:59, sadrul wrote:
> On 2013/10/20 06:50:17, pfeldman wrote:
> > not lgtm. Please don't land this again, it breaks webkit builder.
> > 
> >
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%252520Win%252520B...
> 
> Is there a trybot with this configuration?

I expected
https://codereview.chromium.org/26695007/diff2/267001:367001/content/shell/br...
to fix the issue of TOOLKIT_VIEWS not being defined on windows.

[pfeldman, 2013-10-20 06:52:41]

On 2013/10/20 06:51:20, pfeldman wrote:
> On 2013/10/20 06:50:17, pfeldman wrote:
> > not lgtm. Please don't land this again, it breaks webkit builder.
> > 
> >
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%252520Win%252520B...
> 
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%2520Win%2520Build...

Argh. This thing is so great at escaping...
http://build.chromium.org/p/chromium.webkit/builders/WebKit Win Builder
(deps)/builds/74852

[sadrul, 2013-10-20 07:47:50]

I think I understand the problem now. The blink win bots are probably building
non-aura, and shell_views.cc should not be used when not on aura. I have made
the necessary change in content_shell.gypi to exclude both shell_aura and
shell_views when use_aura==0.

[pfeldman, 2013-10-20 18:33:28]

Daniel, James, jochen@:

As sadrul@ explained me on IRC, updated change introduces following behavior:

"when both use_aura and toolkit_views is set, shell_views is built. when just
use_aura is set, shell_aura is built."

Could you confirm that this is intended behavior and you are still happy with
it? It introduces fair amount of indirection in the build config and I would not
land it this way unless necessary. It has already been reverted twice.

[sadrul, 2013-10-20 19:10:43]

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

[Daniel Erat, 2013-10-21 03:36:49]

lgtm

[James Cook, 2013-10-21 16:08:15]

lgtm