ExtensionDelegate to abstract extension implementation

ExtensionDelegate to abstract extension implementation

This is necessary to switch app shell / chrome implementation.

BUG=397167
TEST= no functional change.
R=mukai@chromium.org, yoz@chromium.org

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

[oshima, 2014-08-20 19:30:39]

mukai-> everything
yoz -> extensions dependency in DEPS

[Yoyo Zhou, 2014-08-20 20:37:28]

https://codereview.chromium.org/487843003/diff/140001/athena/extensions/exten...
File athena/extensions/extension_delegate_impl.cc (right):

https://codereview.chromium.org/487843003/diff/140001/athena/extensions/exten...
athena/extensions/extension_delegate_impl.cc:22: if
(extension_system_->extension())
I know in the current implementation the extension doesn't change, but this
seems a little bit fragile. Why not build the ExtensionSet on demand when we
call GetExtensionSet?

https://codereview.chromium.org/487843003/diff/140001/athena/extensions/exten...
athena/extensions/extension_delegate_impl.cc:25: virtual
~ShellExtensionDelegate() {
nit: line breaks between function definitions

https://codereview.chromium.org/487843003/diff/140001/athena/extensions/publi...
File athena/extensions/public/extension_delegate.h (right):

https://codereview.chromium.org/487843003/diff/140001/athena/extensions/publi...
athena/extensions/public/extension_delegate.h:23: class ATHENA_EXPORT
ExtensionDelegate {
nit: We would generally call this ExtensionsDelegate. (Extensions refers to the
system at large.)

https://codereview.chromium.org/487843003/diff/140001/athena/extensions/publi...
athena/extensions/public/extension_delegate.h:26: static void Shutdown();
Document when this gets called.

https://codereview.chromium.org/487843003/diff/140001/athena/extensions/publi...
athena/extensions/public/extension_delegate.h:37: virtual
extensions::ExtensionSet* GetExtensionSet() = 0;
I'd prefer something like GetInstalledExtensions.

https://codereview.chromium.org/487843003/diff/140001/athena/extensions/publi...
athena/extensions/public/extension_delegate.h:39: // Luanch an applicatino
specified by |app_id|.
typo: Launch,  application

https://codereview.chromium.org/487843003/diff/140001/athena/extensions/publi...
athena/extensions/public/extension_delegate.h:40: virtual void Launch(const
std::string& app_id) = 0;
LaunchApp would be clearer.

[Jun Mukai, 2014-08-20 20:38:21]

lgtm from my side, if yoyo's comments are addressed.

[oshima, 2014-08-20 21:39:22]

https://codereview.chromium.org/487843003/diff/140001/athena/extensions/exten...
File athena/extensions/extension_delegate_impl.cc (right):

https://codereview.chromium.org/487843003/diff/140001/athena/extensions/exten...
athena/extensions/extension_delegate_impl.cc:22: if
(extension_system_->extension())
On 2014/08/20 20:37:27, Yoyo Zhou wrote:
> I know in the current implementation the extension doesn't change, but this
> seems a little bit fragile. Why not build the ExtensionSet on demand when we
> call GetExtensionSet?

Done.

https://codereview.chromium.org/487843003/diff/140001/athena/extensions/exten...
athena/extensions/extension_delegate_impl.cc:25: virtual
~ShellExtensionDelegate() {
On 2014/08/20 20:37:27, Yoyo Zhou wrote:
> nit: line breaks between function definitions

Done.

https://codereview.chromium.org/487843003/diff/140001/athena/extensions/publi...
File athena/extensions/public/extension_delegate.h (right):

https://codereview.chromium.org/487843003/diff/140001/athena/extensions/publi...
athena/extensions/public/extension_delegate.h:23: class ATHENA_EXPORT
ExtensionDelegate {
On 2014/08/20 20:37:28, Yoyo Zhou wrote:
> nit: We would generally call this ExtensionsDelegate. (Extensions refers to
the
> system at large.)

Done.

https://codereview.chromium.org/487843003/diff/140001/athena/extensions/publi...
athena/extensions/public/extension_delegate.h:37: virtual
extensions::ExtensionSet* GetExtensionSet() = 0;
On 2014/08/20 20:37:27, Yoyo Zhou wrote:
> I'd prefer something like GetInstalledExtensions.

Done.

https://codereview.chromium.org/487843003/diff/140001/athena/extensions/publi...
athena/extensions/public/extension_delegate.h:39: // Luanch an applicatino
specified by |app_id|.
On 2014/08/20 20:37:27, Yoyo Zhou wrote:
> typo: Launch,  application

Done.

https://codereview.chromium.org/487843003/diff/140001/athena/extensions/publi...
athena/extensions/public/extension_delegate.h:40: virtual void Launch(const
std::string& app_id) = 0;
On 2014/08/20 20:37:28, Yoyo Zhou wrote:
> LaunchApp would be clearer.

Done.

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

yoz: ping?

[Yoyo Zhou, 2014-08-21 22:40:29]

https://codereview.chromium.org/487843003/diff/180001/athena/content/DEPS
File athena/content/DEPS (right):

https://codereview.chromium.org/487843003/diff/180001/athena/content/DEPS#new...
athena/content/DEPS:22: # Remove this once app shell uses app_wnidow.h
(crbug.com/403726)
typo: window

https://codereview.chromium.org/487843003/diff/180001/athena/extensions/publi...
File athena/extensions/public/extensions_delegate.h (right):

https://codereview.chromium.org/487843003/diff/180001/athena/extensions/publi...
athena/extensions/public/extensions_delegate.h:39: // Returns the set of
extensions that are currently installed.
I find the ownership here a little unintuitive, so it's probably worth a comment
at least. Alternatively, you could create a new ExtensionSet and say that
ownership goes to the caller; I think that's somewhat better. That should be
commented too.

[Yoyo Zhou, 2014-08-21 22:40:37]

(LGTM other than that)

[oshima, 2014-08-21 23:22:58]

https://codereview.chromium.org/487843003/diff/180001/athena/content/DEPS
File athena/content/DEPS (right):

https://codereview.chromium.org/487843003/diff/180001/athena/content/DEPS#new...
athena/content/DEPS:22: # Remove this once app shell uses app_wnidow.h
(crbug.com/403726)
On 2014/08/21 22:40:29, Yoyo Zhou wrote:
> typo: window

Done.

https://codereview.chromium.org/487843003/diff/180001/athena/extensions/publi...
File athena/extensions/public/extensions_delegate.h (right):

https://codereview.chromium.org/487843003/diff/180001/athena/extensions/publi...
athena/extensions/public/extensions_delegate.h:39: // Returns the set of
extensions that are currently installed.
On 2014/08/21 22:40:29, Yoyo Zhou wrote:
> I find the ownership here a little unintuitive, so it's probably worth a
comment
> at least. Alternatively, you could create a new ExtensionSet and say that
> ownership goes to the caller; I think that's somewhat better. That should be
> commented too.

Changed to const ExtensionSet& I think that's better.

[oshima, 2014-08-21 23:23:15]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-08-21 23:29:29]

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

[commit-bot: I haz the power, 2014-08-21 23:43:38]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/44958)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/50142)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-21 23:49:06]

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

[commit-bot: I haz the power, 2014-08-21 23:49:07]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/44961)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Jun Mukai, 2014-08-22 16:53:55]

https://codereview.chromium.org/487843003/diff/300001/athena/main/athena_laun...
File athena/main/athena_launcher.cc (right):

https://codereview.chromium.org/487843003/diff/300001/athena/main/athena_laun...
athena/main/athena_launcher.cc:145: void
StartAthenaSession(athena::ActivityFactory* activity_factory,
So far, StartAthenaSession() is used for tests only. How about to rename as
StartAthenaSessionForTest()?

Creating a delegate ForTest within seemingly-normal method doesn't look great.

[oshima, 2014-08-22 17:06:23]

https://codereview.chromium.org/487843003/diff/300001/athena/main/athena_laun...
File athena/main/athena_launcher.cc (right):

https://codereview.chromium.org/487843003/diff/300001/athena/main/athena_laun...
athena/main/athena_launcher.cc:145: void
StartAthenaSession(athena::ActivityFactory* activity_factory,
On 2014/08/22 16:53:55, Jun Mukai wrote:
> So far, StartAthenaSession() is used for tests only. How about to rename as
> StartAthenaSessionForTest()?
> 
> Creating a delegate ForTest within seemingly-normal method doesn't look great.

You're right. I moved it to athena_test_helper.

[Jun Mukai, 2014-08-22 17:33:10]

lgtm

[oshima, 2014-08-22 17:42:59]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-08-22 17:44:25]

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

[commit-bot: I haz the power, 2014-08-22 17:55:31]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/45252)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/50436)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-22 17:58:09]

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

[commit-bot: I haz the power, 2014-08-22 17:58:10]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/45256)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[oshima, 2014-08-22 19:44:57]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-08-22 19:46:02]

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

[commit-bot: I haz the power, 2014-08-22 19:59:44]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/45321)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/50506)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[oshima, 2014-08-22 21:38:59]

Committed patchset #11 manually as 291509 (presubmit successful).