Minimal athena shell main

Minimal athena shell main

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

[oshima, 2014-05-07 22:08:03]

not ready for review yet, but I want to hear your opinion.

https://codereview.chromium.org/270763002/diff/60001/apps/shell/app/shell_mai...
File apps/shell/app/shell_main.cc (right):

https://codereview.chromium.org/270763002/diff/60001/apps/shell/app/shell_mai...
apps/shell/app/shell_main.cc:19: class AppShellDelegate : public
apps::ShellDelegate {
I'm not sure where this code should live. Maybe in apps/shell/browser?

https://codereview.chromium.org/270763002/diff/60001/apps/shell/app/shell_mai...
apps/shell/app/shell_main.cc:33:
extensions::ExtensionSystem::Get(browser_context));
Not sure if this is right thing to do either....

https://codereview.chromium.org/270763002/diff/60001/apps/shell/common/shell_...
File apps/shell/common/shell_delegate.h (right):

https://codereview.chromium.org/270763002/diff/60001/apps/shell/common/shell_...
apps/shell/common/shell_delegate.h:16: class ShellDelegate {
This isn't great name. Any suggestion? Maybe we should just use a callback and
pass it around?

[James Cook, 2014-05-07 23:42:45]

Overall this seems fine. I like the idea of a startup callback instead of a new
delegate interface.

https://codereview.chromium.org/270763002/diff/60001/apps/shell/app/shell_mai...
File apps/shell/app/shell_main.cc (right):

https://codereview.chromium.org/270763002/diff/60001/apps/shell/app/shell_mai...
apps/shell/app/shell_main.cc:19: class AppShellDelegate : public
apps::ShellDelegate {
On 2014/05/07 22:08:03, oshima wrote:
> I'm not sure where this code should live. Maybe in apps/shell/browser?

The final code should live in apps/shell/browser, since ExtensionSystem (and
ShellExtensionSystem) are browser-process concepts.  But see later comment about
just using a callback.

https://codereview.chromium.org/270763002/diff/60001/apps/shell/app/shell_mai...
apps/shell/app/shell_main.cc:33:
extensions::ExtensionSystem::Get(browser_context));
On 2014/05/07 22:08:03, oshima wrote:
> Not sure if this is right thing to do either....

The cast is fine.  It's just convenient to put LoadAndLaunchApp in the
ShellExtensionSystem class.  And this is the right way to look up the
ShellExtensionSystem.

https://codereview.chromium.org/270763002/diff/60001/apps/shell/common/shell_...
File apps/shell/common/shell_delegate.h (right):

https://codereview.chromium.org/270763002/diff/60001/apps/shell/common/shell_...
apps/shell/common/shell_delegate.h:16: class ShellDelegate {
On 2014/05/07 22:08:03, oshima wrote:
> This isn't great name. Any suggestion? Maybe we should just use a callback and
> pass it around?

I like the idea of just passing around a callback.  startup_callback or
something.

https://codereview.chromium.org/270763002/diff/60001/athena/main/DEPS
File athena/main/DEPS (right):

https://codereview.chromium.org/270763002/diff/60001/athena/main/DEPS#newcode1
athena/main/DEPS:1: include_rules = [
The typical structure for new executables is:

foo/app
foo/browser
foo/common
foo/renderer

So this directory might want to be athena/app/

https://codereview.chromium.org/270763002/diff/60001/build/all.gyp
File build/all.gyp (right):

https://codereview.chromium.org/270763002/diff/60001/build/all.gyp#newcode205
build/all.gyp:205: '../athena/main/athena_main.gyp:*',
You might want to wait on this until extensions and app_shell are completely cut
off from chrome. Otherwise you'll be adding another chrome-size link step to all
the builders.

[oshima, 2014-05-08 21:12:37]

uploaded new patch. PTAL.

https://codereview.chromium.org/270763002/diff/60001/apps/shell/app/shell_mai...
File apps/shell/app/shell_main.cc (right):

https://codereview.chromium.org/270763002/diff/60001/apps/shell/app/shell_mai...
apps/shell/app/shell_main.cc:19: class AppShellDelegate : public
apps::ShellDelegate {
On 2014/05/07 23:42:45, James Cook wrote:
> On 2014/05/07 22:08:03, oshima wrote:
> > I'm not sure where this code should live. Maybe in apps/shell/browser?
> 
> The final code should live in apps/shell/browser, since ExtensionSystem (and
> ShellExtensionSystem) are browser-process concepts.  But see later comment
about
> just using a callback.

moved to apps/shell/browser/shell_app_starter.

https://codereview.chromium.org/270763002/diff/60001/apps/shell/common/shell_...
File apps/shell/common/shell_delegate.h (right):

https://codereview.chromium.org/270763002/diff/60001/apps/shell/common/shell_...
apps/shell/common/shell_delegate.h:16: class ShellDelegate {
On 2014/05/07 23:42:45, James Cook wrote:
> On 2014/05/07 22:08:03, oshima wrote:
> > This isn't great name. Any suggestion? Maybe we should just use a callback
and
> > pass it around?
> 
> I like the idea of just passing around a callback.  startup_callback or
> something.

done

https://codereview.chromium.org/270763002/diff/60001/athena/main/DEPS
File athena/main/DEPS (right):

https://codereview.chromium.org/270763002/diff/60001/athena/main/DEPS#newcode1
athena/main/DEPS:1: include_rules = [
On 2014/05/07 23:42:45, James Cook wrote:
> The typical structure for new executables is:
> 
> foo/app
> foo/browser
> foo/common
> foo/renderer
> 
> So this directory might want to be athena/app/

Discussed offline, this directory will not have dependency to browser nor
renderer, so
we keep this as is.

[James Cook, 2014-05-08 21:27:24]

I hate to say this, but what do you think of going back to using a delegate? It
could be called ShellBrowserMainDelegate (as opposed to ShellMainDelegate) which
would make clear that it is only used in the browser process.

If you think that's too much work (or are too annoyed at me for asking you to
change it before :-) then keep the callback approach.

https://codereview.chromium.org/270763002/diff/160001/apps/shell/app/shell_ma...
File apps/shell/app/shell_main_delegate.h (right):

https://codereview.chromium.org/270763002/diff/160001/apps/shell/app/shell_ma...
apps/shell/app/shell_main_delegate.h:24: explicit
ShellMainDelegate(StartCallback* start_callback);
Please add a comment saying that StartCallback will only be used in the browser
process (since ShellMainDelegate is code used in all processes).

https://codereview.chromium.org/270763002/diff/160001/apps/shell/browser/shel...
File apps/shell/browser/shell_app_starter.cc (right):

https://codereview.chromium.org/270763002/diff/160001/apps/shell/browser/shel...
apps/shell/browser/shell_app_starter.cc:18: CommandLine* command_line =
CommandLine::ForCurrentProcess();
I know you just moved this code, but can you change this to base::CommandLine or
add "using base::CommandLine" above?  I think we're trying to move everything
into the base:: namespace.

https://codereview.chromium.org/270763002/diff/160001/apps/shell/browser/shel...
File apps/shell/browser/shell_app_starter.h (right):

https://codereview.chromium.org/270763002/diff/160001/apps/shell/browser/shel...
apps/shell/browser/shell_app_starter.h:5: #include
"apps/shell/common/start_callback.h"
move this below #define below

https://codereview.chromium.org/270763002/diff/160001/apps/shell/browser/shel...
apps/shell/browser/shell_app_starter.h:12: StartCallback CreateDefaultStarter();
Please add a comment about what this function does/why you want to call it.

https://codereview.chromium.org/270763002/diff/160001/apps/shell/browser/shel...
File apps/shell/browser/shell_browser_main_parts.h (right):

https://codereview.chromium.org/270763002/diff/160001/apps/shell/browser/shel...
apps/shell/browser/shell_browser_main_parts.h:100: StartCallback*
start_callback_;  // not owned
nit: "not owned" -> "Not owned."

https://codereview.chromium.org/270763002/diff/160001/apps/shell/browser/shel...
File apps/shell/browser/shell_content_browser_client.h (right):

https://codereview.chromium.org/270763002/diff/160001/apps/shell/browser/shel...
apps/shell/browser/shell_content_browser_client.h:68: StartCallback*
start_callback_;  // now owned
"no*w* owned" -> "Not owned."

[oshima, 2014-05-08 23:57:02]

changed to use ShellBrowserMainDelegate. PTAL.

https://codereview.chromium.org/270763002/diff/160001/apps/shell/browser/shel...
File apps/shell/browser/shell_app_starter.cc (right):

https://codereview.chromium.org/270763002/diff/160001/apps/shell/browser/shel...
apps/shell/browser/shell_app_starter.cc:18: CommandLine* command_line =
CommandLine::ForCurrentProcess();
On 2014/05/08 21:27:24, James Cook wrote:
> I know you just moved this code, but can you change this to base::CommandLine
or
> add "using base::CommandLine" above?  I think we're trying to move everything
> into the base:: namespace.

Done.

https://codereview.chromium.org/270763002/diff/160001/apps/shell/browser/shel...
File apps/shell/browser/shell_browser_main_parts.h (right):

https://codereview.chromium.org/270763002/diff/160001/apps/shell/browser/shel...
apps/shell/browser/shell_browser_main_parts.h:100: StartCallback*
start_callback_;  // not owned
On 2014/05/08 21:27:24, James Cook wrote:
> nit: "not owned" -> "Not owned."

Done.

https://codereview.chromium.org/270763002/diff/160001/apps/shell/browser/shel...
File apps/shell/browser/shell_content_browser_client.h (right):

https://codereview.chromium.org/270763002/diff/160001/apps/shell/browser/shel...
apps/shell/browser/shell_content_browser_client.h:68: StartCallback*
start_callback_;  // now owned
On 2014/05/08 21:27:24, James Cook wrote:
> "no*w* owned" -> "Not owned."

Done.

[James Cook, 2014-05-09 15:56:00]

LGTM with nits

https://codereview.chromium.org/270763002/diff/200001/apps/shell/browser/shel...
File apps/shell/browser/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/270763002/diff/200001/apps/shell/browser/shel...
apps/shell/browser/shell_browser_main_parts.cc:109: if (parameters_.ui_task) {
Just to double-check: Does app_shell_browsertests run OK with this change?

https://codereview.chromium.org/270763002/diff/220001/apps/shell/app_shell.gyp
File apps/shell/app_shell.gyp (right):

https://codereview.chromium.org/270763002/diff/220001/apps/shell/app_shell.gy...
apps/shell/app_shell.gyp:102: 'browser/shell_app_window.cc',
remove this line

https://codereview.chromium.org/270763002/diff/220001/apps/shell/browser/defa...
File apps/shell/browser/default_shell_browser_main_delegate.cc (right):

https://codereview.chromium.org/270763002/diff/220001/apps/shell/browser/defa...
apps/shell/browser/default_shell_browser_main_delegate.cc:13:
DefaultShellBrowserMainDelegate::DefaultShellBrowserMainDelegate() {
nit: blank line above

https://codereview.chromium.org/270763002/diff/220001/apps/shell/browser/defa...
File apps/shell/browser/default_shell_browser_main_delegate.h (right):

https://codereview.chromium.org/270763002/diff/220001/apps/shell/browser/defa...
apps/shell/browser/default_shell_browser_main_delegate.h:9: #include
"base/basictypes.h"
You can use "base/macros.h"

[oshima, 2014-05-09 17:54:23]

changed to use CreateShellBrowserMainDelegate() as discussed offline.
app_shell_browsertest works and passes with this CL.

https://codereview.chromium.org/270763002/diff/220001/apps/shell/app_shell.gyp
File apps/shell/app_shell.gyp (right):

https://codereview.chromium.org/270763002/diff/220001/apps/shell/app_shell.gy...
apps/shell/app_shell.gyp:102: 'browser/shell_app_window.cc',
On 2014/05/09 15:56:00, James Cook wrote:
> remove this line

Done.

https://codereview.chromium.org/270763002/diff/220001/apps/shell/browser/defa...
File apps/shell/browser/default_shell_browser_main_delegate.cc (right):

https://codereview.chromium.org/270763002/diff/220001/apps/shell/browser/defa...
apps/shell/browser/default_shell_browser_main_delegate.cc:13:
DefaultShellBrowserMainDelegate::DefaultShellBrowserMainDelegate() {
On 2014/05/09 15:56:00, James Cook wrote:
> nit: blank line above

Done.

https://codereview.chromium.org/270763002/diff/220001/apps/shell/browser/defa...
File apps/shell/browser/default_shell_browser_main_delegate.h (right):

https://codereview.chromium.org/270763002/diff/220001/apps/shell/browser/defa...
apps/shell/browser/default_shell_browser_main_delegate.h:9: #include
"base/basictypes.h"
On 2014/05/09 15:56:00, James Cook wrote:
> You can use "base/macros.h"

Done.

[James Cook, 2014-05-09 19:55:42]

LGTM

[oshima, 2014-05-09 20:10:23]

The CQ bit was checked by oshima@chromium.org

[oshima, 2014-05-09 20:10:25]

The CQ bit was unchecked by oshima@chromium.org

[oshima, 2014-05-09 20:11:57]

+brettw for content/public/app dependency in athena/main/DEPS

[brettw, 2014-05-09 20:27:38]

lgtm

[oshima, 2014-05-09 20:30:22]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-05-09 20:32:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/oshima@chromium.org/270763002/260001

[commit-bot: I haz the power, 2014-05-11 00:06:21]

Change committed as 269643