Make content_shell and content_browsertests working with aura

Make content_shell and content_browsertests working with aura

BUG=138525
TEST=content_browsertests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148130

[Hongbo Min, 2012-07-23 10:18:35]

Please review it. Thanks.

[jam, 2012-07-23 14:39:46]

Adding Ben to review the Aura changes.

Please send this through the trybots before the review, i.e. it might catch
issues with DEPS files that you'll need to update

http://codereview.chromium.org/10802064/diff/7/content/shell/shell.h
File content/shell/shell.h (right):

http://codereview.chromium.org/10802064/diff/7/content/shell/shell.h#newcode34
content/shell/shell.h:34: class StackingClient;
nit: no indentation here and above

http://codereview.chromium.org/10802064/diff/7/content/shell/shell_aura.cc
File content/shell/shell_aura.cc (right):

http://codereview.chromium.org/10802064/diff/7/content/shell/shell_aura.cc#ne...
content/shell/shell_aura.cc:16: #include "ui/aura/window.h"
nit: order

http://codereview.chromium.org/10802064/diff/7/content/shell/shell_aura.cc#ne...
content/shell/shell_aura.cc:340: scoped_ptr<aura::client::StackingClient>
Shell::stacking_client_;
only pod are allowed as statics per chrome style guide. so you can make this a
naked pointer and delete it manually

[jochen (gone - plz use gerrit), 2012-07-23 15:21:46]

it feels like moving the ShellWindowDelegateView and ShellViewDelegateAura into
separate files would make this much cleaner, wdyt?

http://codereview.chromium.org/10802064/diff/7/content/shell/shell_aura.cc
File content/shell/shell_aura.cc (left):

http://codereview.chromium.org/10802064/diff/7/content/shell/shell_aura.cc#ol...
content/shell/shell_aura.cc:4: 
please keep this empty line

http://codereview.chromium.org/10802064/diff/7/content/shell/shell_aura.cc
File content/shell/shell_aura.cc (right):

http://codereview.chromium.org/10802064/diff/7/content/shell/shell_aura.cc#ne...
content/shell/shell_aura.cc:30: namespace views{
space between views and {

http://codereview.chromium.org/10802064/diff/7/content/shell/shell_aura.cc#ne...
content/shell/shell_aura.cc:38: }
empty lines between methods.

This class is sufficiently complex that you could consider separating the
declaration and implementation

http://codereview.chromium.org/10802064/diff/7/content/shell/shell_aura.cc#ne...
content/shell/shell_aura.cc:122: ShellWindowDelegateView(content::Shell* shell)
explicit

http://codereview.chromium.org/10802064/diff/7/content/shell/shell_aura.cc#ne...
content/shell/shell_aura.cc:127: ~ShellWindowDelegateView() {}
virtual

http://codereview.chromium.org/10802064/diff/7/content/shell/shell_aura.cc#ne...
content/shell/shell_aura.cc:130: void SetAddressBarURL(const GURL& url);
please separate the declaration and implementation for all methods

[jam, 2012-07-23 22:55:55]

http://codereview.chromium.org/10802064/diff/7/content/shell/shell_aura.cc
File content/shell/shell_aura.cc (right):

http://codereview.chromium.org/10802064/diff/7/content/shell/shell_aura.cc#ne...
content/shell/shell_aura.cc:130: void SetAddressBarURL(const GURL& url);
On 2012/07/23 15:21:47, jochen wrote:
> please separate the declaration and implementation for all methods

I actually prefer when they're inline if the class is defined in a cc file :) I
find that separating them takes more scrolling to read the class, and having the
functions right beside each other isn't always helpful for readability (i.e.
method names, without looking at the code, aren't that useful).

[Hongbo Min, 2012-07-24 05:23:01]

Sorry, I have no right to use trybot. 

The DEPS file for content/shell has to be updated because content_shell aura
also depends on views and aura.

On 2012/07/23 14:39:46, John Abd-El-Malek wrote:
> Adding Ben to review the Aura changes.
> 
> Please send this through the trybots before the review, i.e. it might catch
> issues with DEPS files that you'll need to update
> 
> http://codereview.chromium.org/10802064/diff/7/content/shell/shell.h
> File content/shell/shell.h (right):
> 
> http://codereview.chromium.org/10802064/diff/7/content/shell/shell.h#newcode34
> content/shell/shell.h:34: class StackingClient;
> nit: no indentation here and above
> 
> http://codereview.chromium.org/10802064/diff/7/content/shell/shell_aura.cc
> File content/shell/shell_aura.cc (right):
> 
>
http://codereview.chromium.org/10802064/diff/7/content/shell/shell_aura.cc#ne...
> content/shell/shell_aura.cc:16: #include "ui/aura/window.h"
> nit: order
> 
>
http://codereview.chromium.org/10802064/diff/7/content/shell/shell_aura.cc#ne...
> content/shell/shell_aura.cc:340: scoped_ptr<aura::client::StackingClient>
> Shell::stacking_client_;
> only pod are allowed as statics per chrome style guide. so you can make this a
> naked pointer and delete it manually

[jam, 2012-07-24 15:34:52]

lgtm

http://codereview.chromium.org/10802064/diff/9002/content/shell/DEPS
File content/shell/DEPS (right):

http://codereview.chromium.org/10802064/diff/9002/content/shell/DEPS#newcode16
content/shell/DEPS:16: "+ui/aura",
nit: order

[commit-bot: I haz the power, 2012-07-24 15:35:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hongbo.min@intel.com/10802064/9002

[commit-bot: I haz the power, 2012-07-24 17:07:39]

Change committed as 148130

[Hongbo Min, 2012-07-25 06:13:40]

On 2012/07/24 17:07:39, I haz the power (commit-bot) wrote:
> Change committed as 148130

Reopen it. Need to
1) make sure win aura build passes
2) make sure no crash happend when running content_browsertest with chromeos
config

[Hongbo Min, 2012-07-25 15:50:56]

On 2012/07/25 06:13:40, Hongbo wrote:
> On 2012/07/24 17:07:39, I haz the power (commit-bot) wrote:
> > Change committed as 148130
> 
> Reopen it. Need to
> 1) make sure win aura build passes
> 2) make sure no crash happend when running content_browsertest with chromeos
> config

Hi, John

The latest patch solves the above two issues. It can now pass win aura buid and
content_browsertests is enabled for chromeos config building.

One thing you may notice is, both content_shell and content_browsertests now
have an dependency on  '../chrome/chrome_resources.gyp:packed_resources' in case
of use_aura==1. The reason is, some of UI controls in views toolkit need
localized string which are finally packed by that packed_resources target.

I am not sure whether it is reason for these two executable binaries to depend
on the target defined chrome module. Could you  pls have a look? Thanks.

[jam, 2012-07-25 16:29:01]

Thanks for doing this.

On 2012/07/25 15:50:56, Hongbo wrote:
> On 2012/07/25 06:13:40, Hongbo wrote:
> > On 2012/07/24 17:07:39, I haz the power (commit-bot) wrote:
> > > Change committed as 148130
> > 
> > Reopen it. Need to
> > 1) make sure win aura build passes
> > 2) make sure no crash happend when running content_browsertest with chromeos
> > config
> 
> Hi, John
> 
> The latest patch solves the above two issues. It can now pass win aura buid
and
> content_browsertests is enabled for chromeos config building.
> 
> One thing you may notice is, both content_shell and content_browsertests now
> have an dependency on  '../chrome/chrome_resources.gyp:packed_resources' in
case
> of use_aura==1. The reason is, some of UI controls in views toolkit need
> localized string which are finally packed by that packed_resources target.
> 
> I am not sure whether it is reason for these two executable binaries to depend
> on the target defined chrome module. Could you  pls have a look? Thanks.

It seems wrong that content depends on chrome targets. Not only might this make
the build slower since it depends on chrome, but it also means that it won't be
possible to build this in a webkit checkout that doesn't have chrome. I don't
know how that resource target works, but can't we have a small one just for
non-chrome uses? Are the grd files in chrome or in views?

http://codereview.chromium.org/10802064/diff/24001/content/test/content_brows...
File content/test/content_browser_test.cc (right):

http://codereview.chromium.org/10802064/diff/24001/content/test/content_brows...
content/test/content_browser_test.cc:49:
chromeos::DBusThreadManager::InitializeForTesting(mock_dbus_thread_manager);
a CrOS person should maybe look at this, but are you sure that you want to use
this mock dbus manager?

http://code.google.com/searchframe#OAMlx_jo-ck/src/ash/shell/content_client/s...

used the real one. perhaps we want to move that line to somewhere in
ContentShell itself, so that it's used by that example and also browser_tests

[jam, 2012-07-25 16:36:39]

On 2012/07/25 16:29:01, John Abd-El-Malek wrote:
> Thanks for doing this.
> 
> On 2012/07/25 15:50:56, Hongbo wrote:
> > On 2012/07/25 06:13:40, Hongbo wrote:
> > > On 2012/07/24 17:07:39, I haz the power (commit-bot) wrote:
> > > > Change committed as 148130
> > > 
> > > Reopen it. Need to
> > > 1) make sure win aura build passes
> > > 2) make sure no crash happend when running content_browsertest with
chromeos
> > > config
> > 
> > Hi, John
> > 
> > The latest patch solves the above two issues. It can now pass win aura buid
> and
> > content_browsertests is enabled for chromeos config building.
> > 
> > One thing you may notice is, both content_shell and content_browsertests now
> > have an dependency on  '../chrome/chrome_resources.gyp:packed_resources' in
> case
> > of use_aura==1. The reason is, some of UI controls in views toolkit need
> > localized string which are finally packed by that packed_resources target.
> > 
> > I am not sure whether it is reason for these two executable binaries to
depend
> > on the target defined chrome module. Could you  pls have a look? Thanks.
> 
> It seems wrong that content depends on chrome targets. Not only might this
make
> the build slower since it depends on chrome, but it also means that it won't
be
> possible to build this in a webkit checkout that doesn't have chrome. I don't
> know how that resource target works, but can't we have a small one just for
> non-chrome uses? Are the grd files in chrome or in views?

Perhaps we just need a new target, maybe in ui's gyp, that's like
packed_resources but with only the /ui entries from below?

http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/chrome_resources.gy...

[jochen (gone - plz use gerrit), 2012-07-25 17:49:37]

Can you please create a new CL so it's clear that it's a fresh review (and not
yet landed as the description would suggest)?

For the resources, what about an approach similar to
ShellMainDelegate::InitializeResourceBundle ?

[jam, 2012-07-25 17:51:40]

On 2012/07/25 17:49:37, jochen wrote:
> Can you please create a new CL so it's clear that it's a fresh review (and not
> yet landed as the description would suggest)?

I prefer that reverted changes are uploaded to the same rietveld change :) That
way, you can diff to see what changed since the last review (which is a big time
saver). Also, the review comments are in one place.

[jochen (gone - plz use gerrit), 2012-07-25 17:53:22]

On 2012/07/25 17:51:40, John Abd-El-Malek wrote:
> On 2012/07/25 17:49:37, jochen wrote:
> > Can you please create a new CL so it's clear that it's a fresh review (and
not
> > yet landed as the description would suggest)?
> 
> I prefer that reverted changes are uploaded to the same rietveld change :)
That
> way, you can diff to see what changed since the last review (which is a big
time
> saver). Also, the review comments are in one place.

You can achieve the same by first uploading the old version and then the new
version.

Also, the tools will think that the CL is already approved.

[Hongbo Min, 2012-07-26 13:39:30]

On 2012/07/25 17:53:22, jochen wrote:
> On 2012/07/25 17:51:40, John Abd-El-Malek wrote:
> > On 2012/07/25 17:49:37, jochen wrote:
> > > Can you please create a new CL so it's clear that it's a fresh review (and
> not
> > > yet landed as the description would suggest)?
> > 
> > I prefer that reverted changes are uploaded to the same rietveld change :)
> That
> > way, you can diff to see what changed since the last review (which is a big
> time
> > saver). Also, the review comments are in one place.
> 
> You can achieve the same by first uploading the old version and then the new
> version.
> 
> Also, the tools will think that the CL is already approved.

Closed it and create a new CL at http://codereview.chromium.org/10824043.
Thanks!