Display dialog when app install succeeds / fails on Athena

Displays dialog when app install succeeds / fails on Athena. In particular,
there is no Athena port of AppListService (crbug.com/417571)

In addition this CL:
- Fixes crashes in webstorePrivate.completeInstall() when the Javascript
requests the "App installed" bubble to be shown. In practice this is only
requested for extensions which should not be enabled (but are enabled) in
Athena.
- Moves ExtensionInstallUI and CrxInstallerError to
extensions/browser/install so that they can be used by code in Athena
- Refactors ExtensionInstallUI to remove the static methods

BUG=414341, 417571
TEST=Manual, see bug

Committed: https://crrev.com/a57a1f32435331e8207d4a871e2afea78712dd16
Cr-Commit-Position: refs/heads/master@{#300386}

[pkotwicz, 2014-10-09 00:41:40]

Patchset #1 (id:1) has been deleted

[pkotwicz, 2014-10-09 00:55:11]

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

[pkotwicz, 2014-10-09 00:55:11]

Oshima, can you please take a look?

This CL should make downloading from the web store not crash. There is still a
lot of work to do. For instance, right now installing an app from the web store
creates but does not show a Browser object.
(download_crx_util::CreateCrxInstaller())

I have replaced the AppListService with a stub version. We will eventually need
a proper implementation. A user may want to change an app's settings from the
app list for instance.

https://codereview.chromium.org/634313004/diff/20001/chrome/browser/ui/athena...
File chrome/browser/ui/athena/extensions/extension_install_ui_athena.cc (right):

https://codereview.chromium.org/634313004/diff/20001/chrome/browser/ui/athena...
chrome/browser/ui/athena/extensions/extension_install_ui_athena.cc:80: new
MessageDialogDelegate(error.message()), NULL, NULL);
We should eventually figure out whether these dialogs should be system modal and
pass in either context or the parent.
Finding the correct parent right now (the topmost WebActivity) is currently not
possible.

[oshima, 2014-10-09 19:44:48]

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

[oshima, 2014-10-09 19:44:48]

+mukai

https://codereview.chromium.org/634313004/diff/20001/chrome/browser/ui/athena...
File chrome/browser/ui/athena/extensions/extension_install_ui_athena.h (right):

https://codereview.chromium.org/634313004/diff/20001/chrome/browser/ui/athena...
chrome/browser/ui/athena/extensions/extension_install_ui_athena.h:2: #include
"chrome/browser/extensions/extension_install_ui.h"
we should not have athena code under c/b/ except very thin glue code.

can you move this to athena/extensions ?

[pkotwicz, 2014-10-09 20:47:46]

Patchset #2 (id:70001) has been deleted

[pkotwicz, 2014-10-09 20:49:25]

Moved to athena_extension_install_ui.* to athena/extensions/chrome/

[Jun Mukai, 2014-10-09 23:34:53]

https://codereview.chromium.org/634313004/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/634313004/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:482:
AppListService* app_list_service = AppListService::Get(
This warns me a bit.  Right now Athena doesn't use AppListService at all. This
is still safe, we'll have AppListServiceAsh here, and AppListServiceAsh does
nothing for EnableAppList (because it's enabled by default).

I am not sure what's the best approach here.  This code looks good, but simply
ifndef ENABLE_ATHENA might be acceptable for the time being.

https://codereview.chromium.org/634313004/diff/140001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

https://codereview.chromium.org/634313004/diff/140001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_shell_delegate.cc:103:
app_list::AppListViewDelegate* ChromeShellDelegate::GetAppListViewDelegate() {
Who invokes this?

[pkotwicz, 2014-10-10 00:51:31]

mukai@ can you please take another look?

https://codereview.chromium.org/634313004/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/634313004/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:482:
AppListService* app_list_service = AppListService::Get(
I changed the gyp includes such that Athena uses app_list_service_disabled.cc

I am unsure what the long term direction should be. We could send people to the
home card when they install an app. (I don't think we should, but we could)

Using app_List_service_disabled.cc is the most minimal change possible. We need
to make this change in order to componentize the webstore private API anyways

https://codereview.chromium.org/634313004/diff/140001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

https://codereview.chromium.org/634313004/diff/140001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_shell_delegate.cc:103:
app_list::AppListViewDelegate* ChromeShellDelegate::GetAppListViewDelegate() {
This is to ensure that chrome still compiles. I exclude AppListServiceAsh from
chrome Athena

[Jun Mukai, 2014-10-10 01:01:08]

https://codereview.chromium.org/634313004/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/634313004/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:482:
AppListService* app_list_service = AppListService::Get(
On 2014/10/10 00:51:31, pkotwicz wrote:
> I changed the gyp includes such that Athena uses app_list_service_disabled.cc
> 
> I am unsure what the long term direction should be. We could send people to
the
> home card when they install an app. (I don't think we should, but we could)
> 
> Using app_List_service_disabled.cc is the most minimal change possible. We
need
> to make this change in order to componentize the webstore private API anyways

That seems a right direction to me. Eventually I will have to put AppListService
for athena, but it's not gonna happen soon. Thanks for doing this.

https://codereview.chromium.org/634313004/diff/140001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

https://codereview.chromium.org/634313004/diff/140001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_shell_delegate.cc:103:
app_list::AppListViewDelegate* ChromeShellDelegate::GetAppListViewDelegate() {
On 2014/10/10 00:51:31, pkotwicz wrote:
> This is to ensure that chrome still compiles. I exclude AppListServiceAsh from
> chrome Athena

Theoretically ChromeShellDelegate is only called from ash to bring chrome
features. This means in Athena this method should not be called.

So I think it's okay to leave as is, and if it crashes, fix the caller, not
here.

[pkotwicz, 2014-10-10 01:32:32]

https://codereview.chromium.org/634313004/diff/140001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

https://codereview.chromium.org/634313004/diff/140001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_shell_delegate.cc:103:
app_list::AppListViewDelegate* ChromeShellDelegate::GetAppListViewDelegate() {
The problem is that Athena still compiles Ash. I have volunteered to make Chrome
using Athena not compile Ash. This CL should not be blocked on that refactor
though

[Jun Mukai, 2014-10-10 01:41:20]

lgtm

https://codereview.chromium.org/634313004/diff/140001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

https://codereview.chromium.org/634313004/diff/140001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_shell_delegate.cc:103:
app_list::AppListViewDelegate* ChromeShellDelegate::GetAppListViewDelegate() {
On 2014/10/10 01:32:32, pkotwicz wrote:
> The problem is that Athena still compiles Ash. I have volunteered to make
Chrome
> using Athena not compile Ash. This CL should not be blocked on that refactor
> though

Chatted with oshima offline and he suggested to exclude this file itself (and
ash_init) from athena-on-chrome build, they are actually not used in Athena.

If we can now simply exclude those files, I think it's a better way to go. If it
needs additional hack, then, this CL is fine.

[oshima, 2014-10-10 01:53:07]

On 2014/10/10 01:41:20, Jun Mukai wrote:
> lgtm
> 
>
https://codereview.chromium.org/634313004/diff/140001/chrome/browser/ui/ash/c...
> File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):
> 
>
https://codereview.chromium.org/634313004/diff/140001/chrome/browser/ui/ash/c...
> chrome/browser/ui/ash/chrome_shell_delegate.cc:103:
> app_list::AppListViewDelegate* ChromeShellDelegate::GetAppListViewDelegate() {
> On 2014/10/10 01:32:32, pkotwicz wrote:
> > The problem is that Athena still compiles Ash. I have volunteered to make
> Chrome
> > using Athena not compile Ash. This CL should not be blocked on that refactor
> > though
> 
> Chatted with oshima offline and he suggested to exclude this file itself (and
> ash_init) from athena-on-chrome build, they are actually not used in Athena.
> 
> If we can now simply exclude those files, I think it's a better way to go. If
it
> needs additional hack, then, this CL is fine.

I think if you move ShouldOpenAshOnStartup to ash_util, you should be able to
exclude ash_init.

[pkotwicz, 2014-10-10 03:28:57]

Patchset #3 (id:250001) has been deleted

[pkotwicz, 2014-10-10 13:54:03]

Patchset #3 (id:350001) has been deleted

[pkotwicz, 2014-10-10 13:58:37]

Patchset #3 (id:380001) has been deleted

[pkotwicz, 2014-10-10 14:00:23]

Patchset #3 (id:410001) has been deleted

[pkotwicz, 2014-10-10 14:00:47]

Oshima, can you please take another look?

[oshima, 2014-10-10 15:36:16]

https://codereview.chromium.org/634313004/diff/430001/athena/extensions/chrom...
File athena/extensions/chrome/athena_extension_install_ui.cc (right):

https://codereview.chromium.org/634313004/diff/430001/athena/extensions/chrom...
athena/extensions/chrome/athena_extension_install_ui.cc:60: }
Having these in athena ain't great. let's dicuss offline.

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

https://codereview.chromium.org/634313004/diff/430001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:2459:
'browser/ui/app_list/app_list_service_disabled.cc',
why this change?

https://codereview.chromium.org/634313004/diff/470001/athena/extensions/chrom...
File athena/extensions/chrome/athena_extension_install_ui.h (right):

https://codereview.chromium.org/634313004/diff/470001/athena/extensions/chrom...
athena/extensions/chrome/athena_extension_install_ui.h:10: 
namespace athena

[pkotwicz, 2014-10-10 16:43:22]

https://codereview.chromium.org/634313004/diff/430001/athena/extensions/chrom...
File athena/extensions/chrome/athena_extension_install_ui.cc (right):

https://codereview.chromium.org/634313004/diff/430001/athena/extensions/chrom...
athena/extensions/chrome/athena_extension_install_ui.cc:60: }
This is not the only CL needed in order to get installs not to crash in Athena.
There's a lot more to do.

It is suboptimal that we are not passing in a parent window to
ExtensionInstallPrompt()
It is suboptimal that we are not indicating that an install has completed
There are a number of cases where installing apps still crashes.

I would rather address those issues prior to fixing dependency issues.

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

https://codereview.chromium.org/634313004/diff/430001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:2459:
'browser/ui/app_list/app_list_service_disabled.cc',
This is to replace the AppListService in Athena with a stub implementation.
Otherwise, attempting to access to AppListService webstore_private_api.cc
crashes

[oshima, 2014-10-10 17:06:11]

lgtm

https://codereview.chromium.org/634313004/diff/430001/athena/extensions/chrom...
File athena/extensions/chrome/athena_extension_install_ui.cc (right):

https://codereview.chromium.org/634313004/diff/430001/athena/extensions/chrom...
athena/extensions/chrome/athena_extension_install_ui.cc:60: }
On 2014/10/10 16:43:21, pkotwicz wrote:
> This is not the only CL needed in order to get installs not to crash in
Athena.
> There's a lot more to do.
> 
> It is suboptimal that we are not passing in a parent window to
> ExtensionInstallPrompt()
> It is suboptimal that we are not indicating that an install has completed
> There are a number of cases where installing apps still crashes.
> 
> I would rather address those issues prior to fixing dependency issues.
This is structural issue than dependency. I'm ok as short term solution, but
please refactor and move the code (for ExtensionInstallUI) back to chrome/

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

https://codereview.chromium.org/634313004/diff/430001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:2459:
'browser/ui/app_list/app_list_service_disabled.cc',
On 2014/10/10 16:43:21, pkotwicz wrote:
> This is to replace the AppListService in Athena with a stub implementation.
> Otherwise, attempting to access to AppListService webstore_private_api.cc
> crashes

Done.

https://codereview.chromium.org/634313004/diff/430001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:2459:
'browser/ui/app_list/app_list_service_disabled.cc',
On 2014/10/10 16:43:21, pkotwicz wrote:
> This is to replace the AppListService in Athena with a stub implementation.
> Otherwise, attempting to access to AppListService webstore_private_api.cc
> crashes

Can you add TODO(mukai): replace with app_list_service_athena?

[pkotwicz, 2014-10-10 23:25:43]

Patchset #5 (id:630001) has been deleted

[pkotwicz, 2014-10-10 23:39:24]

Patchset #5 (id:770001) has been deleted

[pkotwicz, 2014-10-10 23:44:59]

Oshima, can you please take another look?
I have:
- split the CL into two. Part #1 is at
https://codereview.chromium.org/643103003/
- removed the static methods from ExtensionInstallUI
- created factory files for creating ExtensionInstallUI objects

[oshima, 2014-10-11 02:10:27]

https://codereview.chromium.org/634313004/diff/800001/athena/extensions/chrom...
File athena/extensions/chrome/public/extension_install_ui_factory.h (right):

https://codereview.chromium.org/634313004/diff/800001/athena/extensions/chrom...
athena/extensions/chrome/public/extension_install_ui_factory.h:13:
ExtensionInstallUI* CreateExtensionInstallUI(Profile* profile);
how about move this to ExtensionsDelegate?

[pkotwicz, 2014-10-12 02:05:32]

https://codereview.chromium.org/634313004/diff/800001/athena/extensions/chrom...
File athena/extensions/chrome/public/extension_install_ui_factory.h (right):

https://codereview.chromium.org/634313004/diff/800001/athena/extensions/chrom...
athena/extensions/chrome/public/extension_install_ui_factory.h:13:
ExtensionInstallUI* CreateExtensionInstallUI(Profile* profile);
That would be difficult because ExtensionsDelegate cannot depend on chrome and
both ExtensionInstallUI and Profile are defined in Chrome. Switching to using
BrowserContext instead of Profile is non-trivial and maybe not possible.

I suspect that in practice moving this to ExtensionsDelegate is OK because we
can forward declare ExtensionInstallUI and Profile, but that's sooo ugly.

[oshima, 2014-10-13 18:01:04]

On 2014/10/12 02:05:32, pkotwicz wrote:
>
https://codereview.chromium.org/634313004/diff/800001/athena/extensions/chrom...
> File athena/extensions/chrome/public/extension_install_ui_factory.h (right):
> 
>
https://codereview.chromium.org/634313004/diff/800001/athena/extensions/chrom...
> athena/extensions/chrome/public/extension_install_ui_factory.h:13:
> ExtensionInstallUI* CreateExtensionInstallUI(Profile* profile);
> That would be difficult because ExtensionsDelegate cannot depend on chrome and
> both ExtensionInstallUI and Profile are defined in Chrome. Switching to using
> BrowserContext instead of Profile is non-trivial and maybe not possible.
> 
> I suspect that in practice moving this to ExtensionsDelegate is OK because we
> can forward declare ExtensionInstallUI and Profile, but that's sooo ugly.

You don't need Profile (you can use BrowserContext that ExtensionsDelegate has).
I understand ExtensinoInstallUI is problematic. Can we move it to
extensinos/componentns?

[pkotwicz, 2014-10-14 03:19:38]

Patchset #6 (id:870001) has been deleted

[pkotwicz, 2014-10-14 03:20:22]

Oshima, can you please take another look? Thank you for checking with rockot@
about ExtensionsInstallUI

[oshima, 2014-10-14 16:57:59]

looks nicer, thanks.

lgtm with a nit, and please ask rockot@ for owners review.

https://codereview.chromium.org/634313004/diff/890001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/634313004/diff/890001/chrome/browser/DEPS#new...
chrome/browser/DEPS:5: "+athena/extensions/public",
can you move this to c/b/ui/extensions ?

[pkotwicz, 2014-10-14 21:44:50]

pkotwicz@chromium.org changed reviewers:
+ rockot@chromium.org

[pkotwicz, 2014-10-14 21:44:51]

rockot@ for chrome/browser/extensions OWNERS

https://codereview.chromium.org/634313004/diff/890001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/634313004/diff/890001/chrome/browser/DEPS#new...
chrome/browser/DEPS:5: "+athena/extensions/public",
chrome/browser/ui/extensions/DEPS does not exist so its probably best to leave
this here for now

[oshima, 2014-10-14 22:08:29]

On 2014/10/14 21:44:51, pkotwicz wrote:
> rockot@ for chrome/browser/extensions OWNERS
> 
> https://codereview.chromium.org/634313004/diff/890001/chrome/browser/DEPS
> File chrome/browser/DEPS (right):
> 
>
https://codereview.chromium.org/634313004/diff/890001/chrome/browser/DEPS#new...
> chrome/browser/DEPS:5: "+athena/extensions/public",
> chrome/browser/ui/extensions/DEPS does not exist so its probably best to leave
> this here for now

I don't want arbitrary chrome code to depend on athena. If the owner does not
like new DEPS file, please add move it to chrome/browser/ui/DEPS at least.

[Ken Rockot(use gerrit already), 2014-10-14 22:18:58]

https://codereview.chromium.org/634313004/diff/890001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/634313004/diff/890001/chrome/browser/DEPS#new...
chrome/browser/DEPS:5: "+athena/extensions/public",
On 2014/10/14 21:44:51, pkotwicz wrote:
> chrome/browser/ui/extensions/DEPS does not exist so its probably best to leave
> this here for now

It seems supremely weird to see athena listed anywhere in chrome DEPS (or really
in any deps outside of athena itself.)

But in this case especially, I don't even see where a real dependency on
athena/extensions/public is being introduced. Why is this needed?

https://codereview.chromium.org/634313004/diff/890001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/extension_install_ui_default.cc (right):

https://codereview.chromium.org/634313004/diff/890001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/extension_install_ui_default.cc:142: //
ExtensionInstallUIDefault --------------------------------------------------
totally optional nit: Could you remove this and other // Foo ------- comments
while you're here? They're unnecessary and should be discouraged.

[Ken Rockot(use gerrit already), 2014-10-14 22:24:31]

Also please update the CL description. Right now it explains the motivation and
one result of the change, but it doesn't really explain the change itself.

[pkotwicz, 2014-10-14 22:29:08]

rockot@ I will upload a new CL once you have sent all of your comments (I assume
that you have more)

https://codereview.chromium.org/634313004/diff/890001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/634313004/diff/890001/chrome/browser/DEPS#new...
chrome/browser/DEPS:5: "+athena/extensions/public",
This is needed for the include in
chrome/browser/ui/extensions/extension_install_ui_factory.cc

[oshima, 2014-10-14 22:31:09]

https://codereview.chromium.org/634313004/diff/890001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/634313004/diff/890001/chrome/browser/DEPS#new...
chrome/browser/DEPS:5: "+athena/extensions/public",
On 2014/10/14 22:18:58, Ken Rockot wrote:
> On 2014/10/14 21:44:51, pkotwicz wrote:
> > chrome/browser/ui/extensions/DEPS does not exist so its probably best to
leave
> > this here for now
> 
> It seems supremely weird to see athena listed anywhere in chrome DEPS (or
really
> in any deps outside of athena itself.)
> 
> But in this case especially, I don't even see where a real dependency on
> athena/extensions/public is being introduced. Why is this needed?

Chrome is starting point, so we need minimum athena dependency inside chrome to
inject athena shell.

We can create a bridge class in chrome/browser/ui/views/athena, and avoid
direct //athena dependency, if you're ok with it. That allow us to control
the //athena dependency in //chrome more easily.

[Ken Rockot(use gerrit already), 2014-10-14 22:39:11]

On 2014/10/14 22:31:09, oshima wrote:
> https://codereview.chromium.org/634313004/diff/890001/chrome/browser/DEPS
> File chrome/browser/DEPS (right):
> 
>
https://codereview.chromium.org/634313004/diff/890001/chrome/browser/DEPS#new...
> chrome/browser/DEPS:5: "+athena/extensions/public",
> On 2014/10/14 22:18:58, Ken Rockot wrote:
> > On 2014/10/14 21:44:51, pkotwicz wrote:
> > > chrome/browser/ui/extensions/DEPS does not exist so its probably best to
> leave
> > > this here for now
> > 
> > It seems supremely weird to see athena listed anywhere in chrome DEPS (or
> really
> > in any deps outside of athena itself.)
> > 
> > But in this case especially, I don't even see where a real dependency on
> > athena/extensions/public is being introduced. Why is this needed?
> 
> Chrome is starting point, so we need minimum athena dependency inside chrome
to
> inject athena shell.
> 
> We can create a bridge class in chrome/browser/ui/views/athena, and avoid
> direct //athena dependency, if you're ok with it. That allow us to control
> the //athena dependency in //chrome more easily.

I think either approach is equally OK as long as it's meant to be temporary.
Either way, please file a bug to remove the dependency if there isn't one
already.

I'm missing historical context for any decision to introduce DEPS from chrome
out to athena, so I don't mean to nit pick on something that's probably already
been
discussed at length. In general though I don't see it making sense for chrome to
embed athena unless it's very temporary.

[Ken Rockot(use gerrit already), 2014-10-14 22:40:25]

On 2014/10/14 22:29:08, pkotwicz wrote:
> rockot@ I will upload a new CL once you have sent all of your comments (I
assume
> that you have more)
> 
> https://codereview.chromium.org/634313004/diff/890001/chrome/browser/DEPS
> File chrome/browser/DEPS (right):
> 
>
https://codereview.chromium.org/634313004/diff/890001/chrome/browser/DEPS#new...
> chrome/browser/DEPS:5: "+athena/extensions/public",
> This is needed for the include in
> chrome/browser/ui/extensions/extension_install_ui_factory.cc

Woops, yeah. I see. Consider this resolved / merged into the greater discussion
about athena deps in chrome

[pkotwicz, 2014-10-14 23:32:33]

rockot@ can you please take another look? I have added
chrome/browser/ui/athena/extensions/extension_install_ui_factory_athena.cc
This way I can confine the dependency on athena to chrome/browser/ui/athena

I did not create
chrome/browser/ui/athena/extensions/extension_install_ui_athena.cc (I did so in
patch set #1 and Oshima did not like it)

[pkotwicz, 2014-10-14 23:32:37]

rockot@ can you please take another look? I have added
chrome/browser/ui/athena/extensions/extension_install_ui_factory_athena.cc
This way I can confine the dependency on athena to chrome/browser/ui/athena

I did not create
chrome/browser/ui/athena/extensions/extension_install_ui_athena.cc (I did so in
patch set #1 and Oshima did not like it)

[Ken Rockot(use gerrit already), 2014-10-15 22:20:08]

Apart from the comments below everything else looks fine.

https://codereview.chromium.org/634313004/diff/930001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/extension_install_ui_factory.h (right):

https://codereview.chromium.org/634313004/diff/930001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/extension_install_ui_factory.h:15:
extensions::ExtensionInstallUI* CreateExtensionInstallUI(
Please return a scoped_ptr to enforce ownership transfer.

Though regarding the existence of "non-Android non-Athena" sources in the GYP, I
would really rather this just go away altogether. As far as I understand this,
you need some way of overriding ExtensionInstallUI creation within existing
//chrome code.

Here is my proposal:

We add a CreateInstallUI interface to ExtensionsBrowserClient. TestEBC and
ShellEBC implementations can return a dummy implementation of ExtensionInstallUI
for now (let's call it NullExtensionInstallUI). Then the ChromeEBC
implementation can be:

scoped_ptr<ExtensionInstallUI> ChromeExtensionsBrowserClient::CreateInstallUI(
    BrowserContext* context) {
#if defined(USE_ATHENA)
  ExtensionInstallUI* ui = athena::ExtensionsDelegate::Get(context)
      ->CreateExtensionInstallUI();
#else
  ExtensionInstallUI* ui = new ExtensionInstallUIDefault(context);
#endif
  return make_scoped_ptr(ui);
}

What do you think of this?

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

https://codereview.chromium.org/634313004/diff/930001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:2461:
'chrome_browser_ui_non_athena_non_android_sources': [
This is extremely unfortunate. I would like this to not be necessary.

[pkotwicz, 2014-10-15 23:08:53]

https://codereview.chromium.org/634313004/diff/930001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/extension_install_ui_factory.h (right):

https://codereview.chromium.org/634313004/diff/930001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/extension_install_ui_factory.h:15:
extensions::ExtensionInstallUI* CreateExtensionInstallUI(
To double check, with your proposal,
chrome/browser/extensions/chrome_extensions_browser_client.cc will depend on
Athena.

This will necessitate either:
Adding athena/extensions/public to chrome/browser/DEPS
OR
Creating chrome/browser/extensions/DEPS (And adding athena/extensions/public as
a dependency there)

[Ken Rockot(use gerrit already), 2014-10-15 23:40:21]

Ugh. You're right, that's probably just as bad. What about adding an
AthenaExtensionsBrowserClient that inherits ChromeExtensionsBrowserClient
and overrides CreateInstallUI?

On Wed, Oct 15, 2014 at 4:08 PM, <pkotwicz@chromium.org> wrote:

>
> https://codereview.chromium.org/634313004/diff/930001/
> chrome/browser/ui/extensions/extension_install_ui_factory.h
> File chrome/browser/ui/extensions/extension_install_ui_factory.h
> (right):
>
> https://codereview.chromium.org/634313004/diff/930001/
> chrome/browser/ui/extensions/extension_install_ui_factory.h#newcode15
> chrome/browser/ui/extensions/extension_install_ui_factory.h:15:
> extensions::ExtensionInstallUI* CreateExtensionInstallUI(
> To double check, with your proposal,
> chrome/browser/extensions/chrome_extensions_browser_client.cc will
> depend on Athena.
>
> This will necessitate either:
> Adding athena/extensions/public to chrome/browser/DEPS
> OR
> Creating chrome/browser/extensions/DEPS (And adding
> athena/extensions/public as a dependency there)
>
> https://codereview.chromium.org/634313004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2014-10-15 23:49:05]

On 2014/10/15 23:40:21, Ken Rockot wrote:
> Ugh. You're right, that's probably just as bad. What about adding an
> AthenaExtensionsBrowserClient that inherits ChromeExtensionsBrowserClient
> and overrides CreateInstallUI?
> 
> On Wed, Oct 15, 2014 at 4:08 PM, <mailto:pkotwicz@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.org/634313004/diff/930001/
> > chrome/browser/ui/extensions/extension_install_ui_factory.h
> > File chrome/browser/ui/extensions/extension_install_ui_factory.h
> > (right):
> >
> > https://codereview.chromium.org/634313004/diff/930001/
> > chrome/browser/ui/extensions/extension_install_ui_factory.h#newcode15
> > chrome/browser/ui/extensions/extension_install_ui_factory.h:15:
> > extensions::ExtensionInstallUI* CreateExtensionInstallUI(
> > To double check, with your proposal,
> > chrome/browser/extensions/chrome_extensions_browser_client.cc will
> > depend on Athena.
> >
> > This will necessitate either:
> > Adding athena/extensions/public to chrome/browser/DEPS
> > OR
> > Creating chrome/browser/extensions/DEPS (And adding
> > athena/extensions/public as a dependency there)
> >
> > https://codereview.chromium.org/634313004/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Actually I just realized you don't control the EBC creation, you just rely on
Chrome's browser process impl. Hmm.

[pkotwicz, 2014-10-15 23:58:55]

After some thought, I think that
"chrome_browser_ui_non_athena_non_android_sources" is not that ugly. We need to
find a better name, yes
We want-to / must-before-we-ship exclude browser.cc from Athena. So browser.cc
belongs to chrome_browser_ui_non_athena_non_android_sources

[Ken Rockot(use gerrit already), 2014-10-16 00:51:05]

OK. I'm a little sad that there isn't a better solution, but LGTM as-is.
Please do change CreateExtensionInstallUI to return a scoped_ptr though.

On Wed, Oct 15, 2014 at 4:58 PM, <pkotwicz@chromium.org> wrote:

> After some thought, I think that
> "chrome_browser_ui_non_athena_non_android_sources" is not that ugly. We
> need to
> find a better name, yes
> We want-to / must-before-we-ship exclude browser.cc from Athena. So
> browser.cc
> belongs to chrome_browser_ui_non_athena_non_android_sources
>
> https://codereview.chromium.org/634313004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2014-10-16 00:51:42]

On 2014/10/16 00:51:05, Ken Rockot wrote:
> OK. I'm a little sad that there isn't a better solution, but LGTM as-is.
> Please do change CreateExtensionInstallUI to return a scoped_ptr though.
> 
> On Wed, Oct 15, 2014 at 4:58 PM, <mailto:pkotwicz@chromium.org> wrote:
> 
> > After some thought, I think that
> > "chrome_browser_ui_non_athena_non_android_sources" is not that ugly. We
> > need to
> > find a better name, yes
> > We want-to / must-before-we-ship exclude browser.cc from Athena. So
> > browser.cc
> > belongs to chrome_browser_ui_non_athena_non_android_sources
> >
> > https://codereview.chromium.org/634313004/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Oops - real lgtm. Also please update the CL description as mentioned earlier.

[pkotwicz, 2014-10-16 15:18:40]

miket@ for chrome/browser/ui/extensions
sky@ for the remainder of chrome/browser/ui

[pkotwicz, 2014-10-16 15:35:28]

pkotwicz@chromium.org changed reviewers:
+ benwells@chromium.org, sky@chromium.org

[pkotwicz, 2014-10-16 15:36:25]

benwells@ can you please take a look at chrome/browser/ui/extensions ?
sky@ can you still look at chrome/browser/ui ?

[pkotwicz, 2014-10-16 15:36:29]

benwells@ can you please take a look at chrome/browser/ui/extensions ?
sky@ can you still look at chrome/browser/ui ?

[sky, 2014-10-16 16:46:52]

LGTM

[benwells, 2014-10-16 22:10:31]

c/b/ui/extensions lgtm.

I'm finding the delegation of extensions stuff in athena a bit confusing. Is
there a design doc somewhere?

[pkotwicz, 2014-10-16 22:44:15]

There is no design doc. I am trying to make things more sane step by step.
https://codereview.chromium.org/635233008/ is probably the last crazy CL for now
(though there is still a long way to go)
My current task is to remove the usage of the Browser object in extension
installation code. To that end, I am currently working on fixing
crbug.com/422814 I am hoping that fixing that bug will allow me to consolidate
ExtensionInstallUI::GetDefaultInstallDialogParent() and
download_crx_util::CreateCrxInstaller()

rockot@ may have some more information. He promised that he would let me know
when there was something useful I could do towards the goal of componentizing
the extension install code

[pkotwicz, 2014-10-20 18:31:06]

Patchset #10 (id:970001) has been deleted

[pkotwicz, 2014-10-20 18:31:35]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-10-20 18:32:47]

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

[commit-bot: I haz the power, 2014-10-20 18:52:50]

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

[commit-bot: I haz the power, 2014-10-20 18:52:52]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pkotwicz, 2014-10-20 20:42:23]

The CQ bit was checked by pkotwicz@chromium.org

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

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

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

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

[commit-bot: I haz the power, 2014-10-20 21:24:29]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pkotwicz, 2014-10-20 22:12:52]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-10-20 22:13:33]

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

[commit-bot: I haz the power, 2014-10-21 00:24:52]

Committed patchset #12 (id:1020001)

[commit-bot: I haz the power, 2014-10-21 00:25:33]

Patchset 12 (id:??) landed as
https://crrev.com/a57a1f32435331e8207d4a871e2afea78712dd16
Cr-Commit-Position: refs/heads/master@{#300386}