Run athena on chrome

Run athena on chrome

There are more work needs to be finished, but this should at least allow us to implement login UI

* Added chrome version of ExtensionsDelegate and AppActivity
* introduce use_athena gyp flag
* disabled features/code path that does not work yet on athena. Filed bugs to keep track of them.

BUG=397167
R=mukai@chromium.org, yoz@chromium.org

Committed: https://crrev.com/963c8527b331983a8f8ccb7d11aec2a50a6fcf77
Cr-Commit-Position: refs/heads/master@{#293023}

[oshima, 2014-08-28 22:21:22]

mukai-> everything
yoz -> everything except for gyp related changes

[oshima, 2014-08-28 22:22:31]

cc'ing hasimoto/skuhne

[Jun Mukai, 2014-08-28 22:36:48]

can you restructure the athena's directory?  athena/content/chrome and
athena/extensions/chrome look like a bit deep.
How about:
- keep athena/content and athena/extensions more like independent from the
implementation
- create athena/chrome for chrome specific files, which depends on
athena/content and athena/extensions
- for the existing files of athena/*/shell, create athena/shell or move to
athena/main

thoughts?

[Jun Mukai, 2014-08-28 22:50:46]

also, the main_parts could be chrome/browser/ui/athena?
There is chrome/browser/ui/ash

https://codereview.chromium.org/514293003/diff/1/athena/content/chrome/conten...
File athena/content/chrome/content_activity_factory.cc (right):

https://codereview.chromium.org/514293003/diff/1/athena/content/chrome/conten...
athena/content/chrome/content_activity_factory.cc:8: #include
"athena/content/app_activity.h"
This file looks like unrelated to chrome. Is hashimoto's cleanup finishing soon?
Why this needs to be in content/chrome?

https://codereview.chromium.org/514293003/diff/1/athena/extensions/chrome/ext...
File athena/extensions/chrome/extensions_delegate_impl.cc (right):

https://codereview.chromium.org/514293003/diff/1/athena/extensions/chrome/ext...
athena/extensions/chrome/extensions_delegate_impl.cc:46: app_id,
extensions::ExtensionRegistry::EVERYTHING);
DCHECK(extension)?  The code below simply assumes that |extension| exists.

https://codereview.chromium.org/514293003/diff/1/chrome/browser/chromeos/chro...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/514293003/diff/1/chrome/browser/chromeos/chro...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:623: #if
!defined(USE_ATHENA)
It's better to mark the crbug URL so that we can distinguish intentionally
introduced ones from the ones which will be fixed later.

https://codereview.chromium.org/514293003/diff/1/chrome/browser/chromeos/chro...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:631: #if
!defined(USE_ATHENA)
ditto

[oshima, 2014-08-28 23:08:28]

On 2014/08/28 22:36:48, Jun Mukai wrote:
> can you restructure the athena's directory?  athena/content/chrome and
> athena/extensions/chrome look like a bit deep.

hmm, do you think it's really deep?

> How about:
> - keep athena/content and athena/extensions more like independent from the
> implementation
> - create athena/chrome for chrome specific files, which depends on
> athena/content and athena/extensions
> - for the existing files of athena/*/shell, create athena/shell or move to
> athena/main

I first thought that way, but changed this way because each feature that needs
chrome will also
have non chrome (app_shell) version, and thought this is easier to follow
instead of
athena/chrome has unrelated files.

> thoughts?

[Jun Mukai, 2014-08-28 23:11:05]

On 2014/08/28 23:08:28, oshima wrote:
> On 2014/08/28 22:36:48, Jun Mukai wrote:
> > can you restructure the athena's directory?  athena/content/chrome and
> > athena/extensions/chrome look like a bit deep.
> 
> hmm, do you think it's really deep?
> 
> > How about:
> > - keep athena/content and athena/extensions more like independent from the
> > implementation
> > - create athena/chrome for chrome specific files, which depends on
> > athena/content and athena/extensions
> > - for the existing files of athena/*/shell, create athena/shell or move to
> > athena/main
> 
> I first thought that way, but changed this way because each feature that needs
> chrome will also
> have non chrome (app_shell) version, and thought this is easier to follow
> instead of
> athena/chrome has unrelated files.
> 
> > thoughts?

okay if you have considered the idea and chose the current one.

[oshima, 2014-08-28 23:59:04]

https://codereview.chromium.org/514293003/diff/1/athena/content/chrome/conten...
File athena/content/chrome/content_activity_factory.cc (right):

https://codereview.chromium.org/514293003/diff/1/athena/content/chrome/conten...
athena/content/chrome/content_activity_factory.cc:8: #include
"athena/content/app_activity.h"
On 2014/08/28 22:50:45, Jun Mukai wrote:
> This file looks like unrelated to chrome. Is hashimoto's cleanup finishing
soon?
> Why this needs to be in content/chrome?

apps/app_window currently requires chrome. Once his CL is landed, this file will
be merged to shell version.

https://codereview.chromium.org/514293003/diff/1/athena/extensions/chrome/ext...
File athena/extensions/chrome/extensions_delegate_impl.cc (right):

https://codereview.chromium.org/514293003/diff/1/athena/extensions/chrome/ext...
athena/extensions/chrome/extensions_delegate_impl.cc:46: app_id,
extensions::ExtensionRegistry::EVERYTHING);
On 2014/08/28 22:50:45, Jun Mukai wrote:
> DCHECK(extension)?  The code below simply assumes that |extension| exists.

Done.

https://codereview.chromium.org/514293003/diff/1/chrome/browser/chromeos/chro...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/514293003/diff/1/chrome/browser/chromeos/chro...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:623: #if
!defined(USE_ATHENA)
On 2014/08/28 22:50:45, Jun Mukai wrote:
> It's better to mark the crbug URL so that we can distinguish intentionally
> introduced ones from the ones which will be fixed later.

Done.

https://codereview.chromium.org/514293003/diff/1/chrome/browser/chromeos/chro...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:631: #if
!defined(USE_ATHENA)
On 2014/08/28 22:50:45, Jun Mukai wrote:
> ditto

Done.

[sadrul, 2014-08-29 00:05:20]

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

[sadrul, 2014-08-29 00:05:21]

https://codereview.chromium.org/514293003/diff/20001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/514293003/diff/20001/build/common.gypi#newcod...
build/common.gypi:2567: }],
Would you mind moving this into chrome.gyp so that we don't accidentally use
USE_ATHENA outside of chrome/?

[Jun Mukai, 2014-08-29 00:06:57]

lgtm with nits

https://codereview.chromium.org/514293003/diff/20001/athena/resource_manager/...
File athena/resource_manager/public/DEPS (right):

https://codereview.chromium.org/514293003/diff/20001/athena/resource_manager/...
athena/resource_manager/public/DEPS:5: 
remove blank lines

[Jun Mukai, 2014-08-29 00:08:25]

https://codereview.chromium.org/514293003/diff/20001/athena/resource_manager/...
File athena/resource_manager/public/resource_manager.h (right):

https://codereview.chromium.org/514293003/diff/20001/athena/resource_manager/...
athena/resource_manager/public/resource_manager.h:18: class ATHENA_EXPORT
ResourceManager {
Also adding ATHENA_EXPORT to
athena/resource_manager/public/resource_manager_delegate.h?
# sorry I missed this... :-/

[Mr4D (OOO till 08-26), 2014-08-29 00:09:59]

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

[Mr4D (OOO till 08-26), 2014-08-29 00:09:59]

Only a few nits from my side. lgtm.

https://codereview.chromium.org/514293003/diff/1/athena/extensions/chrome/ath...
File athena/extensions/chrome/athena_apps_client.cc (right):

https://codereview.chromium.org/514293003/diff/1/athena/extensions/chrome/ath...
athena/extensions/chrome/athena_apps_client.cc:62: // TODO(oshima): Figure out
what to do.
Indeed. this feels rather odd. Do we want to have dev tools on Athena?

https://codereview.chromium.org/514293003/diff/1/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/514293003/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:688:
main_parts->AddParts(CreateChromeBrowserMainExtraPartsAthena());
I leave this to you, but every other function uses new .. and you have created
this CreateChromeBrowser.. function. Would be nice to be uniform within the
module.

https://codereview.chromium.org/514293003/diff/1/chrome/browser/chromeos/chro...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/514293003/diff/1/chrome/browser/chromeos/chro...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:394: 
Sorry for this stupid nit but this empty line can go.

https://codereview.chromium.org/514293003/diff/1/chrome/browser/ui/views/athe...
File chrome/browser/ui/views/athena/OWNERS (left):

https://codereview.chromium.org/514293003/diff/1/chrome/browser/ui/views/athe...
chrome/browser/ui/views/athena/OWNERS:1: skuhne@chromium.org
Any reason for this?

[oshima, 2014-08-29 00:44:43]

https://codereview.chromium.org/514293003/diff/1/athena/extensions/chrome/ath...
File athena/extensions/chrome/athena_apps_client.cc (right):

https://codereview.chromium.org/514293003/diff/1/athena/extensions/chrome/ath...
athena/extensions/chrome/athena_apps_client.cc:62: // TODO(oshima): Figure out
what to do.
On 2014/08/29 00:09:58, Mr4D wrote:
> Indeed. this feels rather odd. Do we want to have dev tools on Athena?

We definitely need dev tools even on athena. Question is if we need dev window.
Maybe, or may not.

https://codereview.chromium.org/514293003/diff/1/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/514293003/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:688:
main_parts->AddParts(CreateChromeBrowserMainExtraPartsAthena());
On 2014/08/29 00:09:58, Mr4D wrote:
> I leave this to you, but every other function uses new .. and you have created
> this CreateChromeBrowser.. function. Would be nice to be uniform within the
> module.

I'm going to just follow the OWNERS recommendation. I did this way because
exposing class definition is technically unnecessary, and we're actually doing
the same thing if it's method ( in class A { B* Create(); }, we only uses
forward decl), but I'm fine with other way.

https://codereview.chromium.org/514293003/diff/1/chrome/browser/chromeos/chro...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/514293003/diff/1/chrome/browser/chromeos/chro...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:394: 
On 2014/08/29 00:09:59, Mr4D wrote:
> Sorry for this stupid nit but this empty line can go.

Done.

https://codereview.chromium.org/514293003/diff/1/chrome/browser/ui/views/athe...
File chrome/browser/ui/views/athena/OWNERS (left):

https://codereview.chromium.org/514293003/diff/1/chrome/browser/ui/views/athe...
chrome/browser/ui/views/athena/OWNERS:1: skuhne@chromium.org
On 2014/08/29 00:09:59, Mr4D wrote:
> Any reason for this?

I added you. Note that this is new file, but git just treated this as a copy of
random OWNERS file.

I also do not expect this directory will grow. This should stay almost as is.

https://codereview.chromium.org/514293003/diff/20001/athena/resource_manager/...
File athena/resource_manager/public/resource_manager.h (right):

https://codereview.chromium.org/514293003/diff/20001/athena/resource_manager/...
athena/resource_manager/public/resource_manager.h:18: class ATHENA_EXPORT
ResourceManager {
On 2014/08/29 00:08:25, Jun Mukai wrote:
> Also adding ATHENA_EXPORT to
> athena/resource_manager/public/resource_manager_delegate.h?
> # sorry I missed this... :-/

Done.

[hashimoto, 2014-08-29 06:05:46]

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

[hashimoto, 2014-08-29 06:05:46]

https://codereview.chromium.org/514293003/diff/20001/athena/extensions/chrome...
File athena/extensions/chrome/athena_apps_client.h (right):

https://codereview.chromium.org/514293003/diff/20001/athena/extensions/chrome...
athena/extensions/chrome/athena_apps_client.h:14: class AthenaAppsClient :
public apps::AppsClient {
To support apps.window API, Athena needs to implement its own AppsClient (and
NativeAppWindow and AppDelegate).
Can we reuse this class for the pure (non-Chrome) Athena?

If not, could you rename this class to something like ChromeAthenaAppsClient?

[Dmitry Polukhin, 2014-08-29 13:13:56]

FYI, this CL is a good starting point for OOBE/login work, thanks! On top of
this CL I created experimental CL that can show OOBE and login screens. Main
problems that Athena doesn't have ash::Shell instance that is used for showing
status area. Other problems I'm investigating now: edit fields don't work on
login screen (focus manager issue?) and so far cannot start Athena session for
Guest mode (doesn't require edit boxes but requires Athena specific code because
on Chrome we just restart browser for Guest mode).

[oshima, 2014-08-29 14:40:32]

https://codereview.chromium.org/514293003/diff/20001/athena/extensions/chrome...
File athena/extensions/chrome/athena_apps_client.h (right):

https://codereview.chromium.org/514293003/diff/20001/athena/extensions/chrome...
athena/extensions/chrome/athena_apps_client.h:14: class AthenaAppsClient :
public apps::AppsClient {
On 2014/08/29 06:05:46, hashimoto wrote:
> To support apps.window API, Athena needs to implement its own AppsClient (and
> NativeAppWindow and AppDelegate).
> Can we reuse this class for the pure (non-Chrome) Athena?
Yes, that's the plan. See my comment in CreateNativeAppWindow.

> If not, could you rename this class to something like ChromeAthenaAppsClient?

[oshima, 2014-08-29 17:17:43]

On 2014/08/28 22:50:46, Jun Mukai wrote:
> also, the main_parts could be chrome/browser/ui/athena?
> There is chrome/browser/ui/ash

I put here because _ash.cc is here. I'll follow OWNERS recommendation.


> 
>
https://codereview.chromium.org/514293003/diff/1/athena/content/chrome/conten...
> File athena/content/chrome/content_activity_factory.cc (right):
> 
>
https://codereview.chromium.org/514293003/diff/1/athena/content/chrome/conten...
> athena/content/chrome/content_activity_factory.cc:8: #include
> "athena/content/app_activity.h"
> This file looks like unrelated to chrome. Is hashimoto's cleanup finishing
soon?
> Why this needs to be in content/chrome?
> 
>
https://codereview.chromium.org/514293003/diff/1/athena/extensions/chrome/ext...
> File athena/extensions/chrome/extensions_delegate_impl.cc (right):
> 
>
https://codereview.chromium.org/514293003/diff/1/athena/extensions/chrome/ext...
> athena/extensions/chrome/extensions_delegate_impl.cc:46: app_id,
> extensions::ExtensionRegistry::EVERYTHING);
> DCHECK(extension)?  The code below simply assumes that |extension| exists.
> 
>
https://codereview.chromium.org/514293003/diff/1/chrome/browser/chromeos/chro...
> File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):
> 
>
https://codereview.chromium.org/514293003/diff/1/chrome/browser/chromeos/chro...
> chrome/browser/chromeos/chrome_browser_main_chromeos.cc:623: #if
> !defined(USE_ATHENA)
> It's better to mark the crbug URL so that we can distinguish intentionally
> introduced ones from the ones which will be fixed later.
> 
>
https://codereview.chromium.org/514293003/diff/1/chrome/browser/chromeos/chro...
> chrome/browser/chromeos/chrome_browser_main_chromeos.cc:631: #if
> !defined(USE_ATHENA)
> ditto

[oshima, 2014-08-29 17:19:24]

https://codereview.chromium.org/514293003/diff/20001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/514293003/diff/20001/build/common.gypi#newcod...
build/common.gypi:2567: }],
On 2014/08/29 00:05:20, sadrul wrote:
> Would you mind moving this into chrome.gyp so that we don't accidentally use
> USE_ATHENA outside of chrome/?

I will follow OWNERS recommendation here too.

[oshima, 2014-08-29 18:10:11]

yoz: ping?

[oshima, 2014-08-29 22:11:19]

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

[oshima, 2014-08-29 22:11:20]

sky@ -> owners review for chrome/

https://codereview.chromium.org/514293003/diff/20001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/514293003/diff/20001/build/common.gypi#newcod...
build/common.gypi:2567: }],
On 2014/08/29 17:19:24, oshima wrote:
> On 2014/08/29 00:05:20, sadrul wrote:
> > Would you mind moving this into chrome.gyp so that we don't accidentally use
> > USE_ATHENA outside of chrome/?
> 
> I will follow OWNERS recommendation here too.

sky@, I agree that this is probably better, but want to check your opinion.

https://codereview.chromium.org/514293003/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/athena/chrome_browser_main_extra_parts_athena.h
(right):

https://codereview.chromium.org/514293003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/athena/chrome_browser_main_extra_parts_athena.h:12:
ChromeBrowserMainExtraParts* CreateChromeBrowserMainExtraPartsAthena();
sky@, let me know if
1) you want to have class definition in this header, rather than in .cc
2) this should be c/b/ui/athena instead.

I looked at several files, and decision seems to be rather random. (there are
files in c/b/ui/ash that depend on views, w
Looks like many files in c/b/ui/ash depend on (either directly or indirectly)
views,
and there are very few files in c/b/ui/views/ash.

I can consolidate them if you agree. Please let me know.

[Yoyo Zhou, 2014-08-29 22:23:30]

LGTM, these are mostly just nits.

https://chromiumcodereview.appspot.com/514293003/diff/40001/athena/extensions...
File athena/extensions/chrome/athena_apps_client.cc (right):

https://chromiumcodereview.appspot.com/514293003/diff/40001/athena/extensions...
athena/extensions/chrome/athena_apps_client.cc:53: // No need to keep track of
KeepAlive count on ChromeOS
nit: end comments in .

https://chromiumcodereview.appspot.com/514293003/diff/40001/athena/extensions...
File athena/extensions/chrome/extensions_delegate_impl.cc (right):

https://chromiumcodereview.appspot.com/514293003/diff/40001/athena/extensions...
athena/extensions/chrome/extensions_delegate_impl.cc:46: app_id,
extensions::ExtensionRegistry::EVERYTHING);
EVERYTHING is probably not what you want. ENABLED?

https://chromiumcodereview.appspot.com/514293003/diff/40001/athena/extensions...
File athena/extensions/public/extensions_delegate.h (right):

https://chromiumcodereview.appspot.com/514293003/diff/40001/athena/extensions...
athena/extensions/public/extensions_delegate.h:53: // TODO(oshima): Add source
type.
What is meant by 'source type'?

https://chromiumcodereview.appspot.com/514293003/diff/40001/athena/resource_m...
File athena/resource_manager/DEPS (right):

https://chromiumcodereview.appspot.com/514293003/diff/40001/athena/resource_m...
athena/resource_manager/DEPS:18: 
nit: stray newlines

https://chromiumcodereview.appspot.com/514293003/diff/40001/athena/resource_m...
File athena/resource_manager/public/DEPS (right):

https://chromiumcodereview.appspot.com/514293003/diff/40001/athena/resource_m...
athena/resource_manager/public/DEPS:4: 
nit: stray newlines

https://chromiumcodereview.appspot.com/514293003/diff/40001/chrome/browser/ui...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://chromiumcodereview.appspot.com/514293003/diff/40001/chrome/browser/ui...
chrome/browser/ui/startup/startup_browser_creator.cc:564: #endif
nit: if we're going to have nested #ifdefs here then it would help for there to
be a comment // OS_CHROMEOS here.

https://chromiumcodereview.appspot.com/514293003/diff/40001/chrome/browser/ui...
File chrome/browser/ui/views/athena/chrome_browser_main_extra_parts_athena.cc
(right):

https://chromiumcodereview.appspot.com/514293003/diff/40001/chrome/browser/ui...
chrome/browser/ui/views/athena/chrome_browser_main_extra_parts_athena.cc:31: //
TODO(oshiam|polukhin): Start OOBE/Login process.
typo: oshima

https://chromiumcodereview.appspot.com/514293003/diff/40001/chrome/browser/ui...
File chrome/browser/ui/views/athena/chrome_browser_main_extra_parts_athena.h
(right):

https://chromiumcodereview.appspot.com/514293003/diff/40001/chrome/browser/ui...
chrome/browser/ui/views/athena/chrome_browser_main_extra_parts_athena.h:11: //
and ash cannot coexist, and has to be exclusive.
nit: s/has to/must/

[oshima, 2014-08-29 23:25:26]

https://chromiumcodereview.appspot.com/514293003/diff/40001/athena/extensions...
File athena/extensions/chrome/athena_apps_client.cc (right):

https://chromiumcodereview.appspot.com/514293003/diff/40001/athena/extensions...
athena/extensions/chrome/athena_apps_client.cc:53: // No need to keep track of
KeepAlive count on ChromeOS
On 2014/08/29 22:23:29, Yoyo Zhou wrote:
> nit: end comments in .

Done.

https://chromiumcodereview.appspot.com/514293003/diff/40001/athena/extensions...
File athena/extensions/chrome/extensions_delegate_impl.cc (right):

https://chromiumcodereview.appspot.com/514293003/diff/40001/athena/extensions...
athena/extensions/chrome/extensions_delegate_impl.cc:46: app_id,
extensions::ExtensionRegistry::EVERYTHING);
On 2014/08/29 22:23:29, Yoyo Zhou wrote:
> EVERYTHING is probably not what you want. ENABLED?

This is what ash is using for launcher
(ChromeLauncherController::GetExtensionForAppID).

I'll follow up on this.

https://chromiumcodereview.appspot.com/514293003/diff/40001/athena/extensions...
File athena/extensions/public/extensions_delegate.h (right):

https://chromiumcodereview.appspot.com/514293003/diff/40001/athena/extensions...
athena/extensions/public/extensions_delegate.h:53: // TODO(oshima): Add source
type.
On 2014/08/29 22:23:29, Yoyo Zhou wrote:
> What is meant by 'source type'?

There is c/b/ui/ash/launcher/chrome_launcher_types.h for tracking purpose. I
updated the comment.

https://chromiumcodereview.appspot.com/514293003/diff/40001/athena/resource_m...
File athena/resource_manager/DEPS (right):

https://chromiumcodereview.appspot.com/514293003/diff/40001/athena/resource_m...
athena/resource_manager/DEPS:18: 
On 2014/08/29 22:23:29, Yoyo Zhou wrote:
> nit: stray newlines

Done.

https://chromiumcodereview.appspot.com/514293003/diff/40001/athena/resource_m...
File athena/resource_manager/public/DEPS (right):

https://chromiumcodereview.appspot.com/514293003/diff/40001/athena/resource_m...
athena/resource_manager/public/DEPS:4: 
On 2014/08/29 22:23:30, Yoyo Zhou wrote:
> nit: stray newlines

Done.

https://chromiumcodereview.appspot.com/514293003/diff/40001/chrome/browser/ui...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://chromiumcodereview.appspot.com/514293003/diff/40001/chrome/browser/ui...
chrome/browser/ui/startup/startup_browser_creator.cc:564: #endif
On 2014/08/29 22:23:30, Yoyo Zhou wrote:
> nit: if we're going to have nested #ifdefs here then it would help for there
to
> be a comment // OS_CHROMEOS here.

Done.

https://chromiumcodereview.appspot.com/514293003/diff/40001/chrome/browser/ui...
File chrome/browser/ui/views/athena/chrome_browser_main_extra_parts_athena.cc
(right):

https://chromiumcodereview.appspot.com/514293003/diff/40001/chrome/browser/ui...
chrome/browser/ui/views/athena/chrome_browser_main_extra_parts_athena.cc:31: //
TODO(oshiam|polukhin): Start OOBE/Login process.
On 2014/08/29 22:23:30, Yoyo Zhou wrote:
> typo: oshima

Done.

https://chromiumcodereview.appspot.com/514293003/diff/40001/chrome/browser/ui...
File chrome/browser/ui/views/athena/chrome_browser_main_extra_parts_athena.h
(right):

https://chromiumcodereview.appspot.com/514293003/diff/40001/chrome/browser/ui...
chrome/browser/ui/views/athena/chrome_browser_main_extra_parts_athena.h:11: //
and ash cannot coexist, and has to be exclusive.
On 2014/08/29 22:23:30, Yoyo Zhou wrote:
> nit: s/has to/must/

Done.

[oshima, 2014-08-29 23:34:50]

oshima@chromium.org changed reviewers:
+ wtc@chromium.org

[oshima, 2014-08-29 23:34:51]

wtc -> net/base dependency in athena/extensions/chrome/DEPS

[wtc, 2014-08-30 20:53:32]

Patch set 5 LGTM.

[hashimoto, 2014-09-01 05:33:30]

https://codereview.chromium.org/514293003/diff/20001/athena/extensions/chrome...
File athena/extensions/chrome/athena_apps_client.h (right):

https://codereview.chromium.org/514293003/diff/20001/athena/extensions/chrome...
athena/extensions/chrome/athena_apps_client.h:14: class AthenaAppsClient :
public apps::AppsClient {
On 2014/08/29 14:40:32, oshima wrote:
> On 2014/08/29 06:05:46, hashimoto wrote:
> > To support apps.window API, Athena needs to implement its own AppsClient
(and
> > NativeAppWindow and AppDelegate).
> > Can we reuse this class for the pure (non-Chrome) Athena?
> Yes, that's the plan. See my comment in CreateNativeAppWindow.
> 
> > If not, could you rename this class to something like
ChromeAthenaAppsClient?

How are you going to remove chrome dependencies from GetLoadedBrowserContexts()
and IsCurrentChannelOlderThanDev()?
I guess it would make more sense to use separate implementations for pure Athena
and Athena on Chrome rather than sharing one implementation as the amount of
code shared between them would not be so large.

Anyways, I'm OK with this change as it would not be difficult to rename this
class even after this gets committed.

[sky, 2014-09-02 17:31:51]

Could you elaborate more on why this is needed? I thought athena was not going
to use chrome.

[Jun Mukai, 2014-09-02 17:43:08]

On 2014/09/02 17:31:51, sky wrote:
> Could you elaborate more on why this is needed? I thought athena was not going
> to use chrome.

As is described in the bug crbug.com/397167, the plan has changed slightly and
we want to run athena inside of chrome, at least for the time being (mainly for
sync and some profile-specific things IIRC).

[sky, 2014-09-02 19:10:27]

LGTM

https://codereview.chromium.org/514293003/diff/80001/chrome/browser/idle_chro...
File chrome/browser/idle_chromeos.cc (right):

https://codereview.chromium.org/514293003/diff/80001/chrome/browser/idle_chro...
chrome/browser/idle_chromeos.cc:13: #if defined(USE_ATHENA)
How about idle_athena with NOTIMPLEMENTEDs?

[oshima, 2014-09-02 19:33:40]

On 2014/09/01 05:33:30, hashimoto wrote:
>
https://codereview.chromium.org/514293003/diff/20001/athena/extensions/chrome...
> File athena/extensions/chrome/athena_apps_client.h (right):
> 
>
https://codereview.chromium.org/514293003/diff/20001/athena/extensions/chrome...
> athena/extensions/chrome/athena_apps_client.h:14: class AthenaAppsClient :
> public apps::AppsClient {
> On 2014/08/29 14:40:32, oshima wrote:
> > On 2014/08/29 06:05:46, hashimoto wrote:
> > > To support apps.window API, Athena needs to implement its own AppsClient
> (and
> > > NativeAppWindow and AppDelegate).
> > > Can we reuse this class for the pure (non-Chrome) Athena?
> > Yes, that's the plan. See my comment in CreateNativeAppWindow.
> > 
> > > If not, could you rename this class to something like
> ChromeAthenaAppsClient?
> 
> How are you going to remove chrome dependencies from
GetLoadedBrowserContexts()
> and IsCurrentChannelOlderThanDev()?
> I guess it would make more sense to use separate implementations for pure
Athena
> and Athena on Chrome rather than sharing one implementation as the amount of
> code shared between them would not be so large.

They will stay as is for the time being. This file will be kept dependent on
src/chrome/broser until profile manager is componentized.

ChromeNativeAppWindowViews and ChomeAppDelegate will be replaced with
AthenaNativeAppWindowViews and AthenaAppDelegate.




> 
> Anyways, I'm OK with this change as it would not be difficult to rename this
> class even after this gets committed.

[oshima, 2014-09-02 20:37:55]

https://codereview.chromium.org/514293003/diff/80001/chrome/browser/idle_chro...
File chrome/browser/idle_chromeos.cc (right):

https://codereview.chromium.org/514293003/diff/80001/chrome/browser/idle_chro...
chrome/browser/idle_chromeos.cc:13: #if defined(USE_ATHENA)
On 2014/09/02 19:10:26, sky wrote:
> How about idle_athena with NOTIMPLEMENTEDs?

Done.

[oshima, 2014-09-02 21:02:17]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-09-02 21:09:45]

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

[commit-bot: I haz the power, 2014-09-02 22:36:44]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[commit-bot: I haz the power, 2014-09-02 23:33:25]

Committed patchset #6 (id:100001) as 0f60797bcf3264389b06bc2aab33d6c547338b04

[commit-bot: I haz the power, 2014-09-10 03:21:55]

Patchset 6 (id:??) landed as
https://crrev.com/963c8527b331983a8f8ccb7d11aec2a50a6fcf77
Cr-Commit-Position: refs/heads/master@{#293023}