Add option to install an ephemeral app to ChromeOS shelf context menu

Add option to install an ephemeral app to ChromeOS shelf context menu

This is the ChromeOS implementation for adding an option to install an
ephemeral app in the shelf context menu. Includes some refactoring of
the WebstoreStandaloneInstallers to reuse common code.

BUG=339253
TEST=browser_tests (LauncherPlatformAppBrowserTest.InstallEphemeralApp)
Manual: Enable the ephemeral apps experimental feature and launch an
ephemeral app. Right-click on the app's icon in the shelf. There should
be an "Install app" item, which invoked, will install the app and make
it visible in the app launcher and NTP.

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

[tmdiep, 2014-05-28 03:25:42]

This is the ChromeOS equivalent of the "Install app" item in the Windows
jumplist (please see bug for more details).

skuhne: for ash.
tapted: for installers and general review (since you reviewed the Windows
patch). Let me know if I should get someone else to review the installers.

Thanks in advance.

https://codereview.chromium.org/298303002/diff/20001/chrome/browser/ui/ash/la...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc
(right):

https://codereview.chromium.org/298303002/diff/20001/chrome/browser/ui/ash/la...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:669:
service->PromoteEphemeralApp(ephemeral_app, false);
PromoteEphemeralApp() will be available when this patch makes it through the CQ:
https://codereview.chromium.org/297263003

[tapted, 2014-05-28 06:28:46]

the webstore installer stuff isn't too familiar to me - it might be good to get
an opinion from asargent@

https://codereview.chromium.org/298303002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/webstore_install_prompt.cc (right):

https://codereview.chromium.org/298303002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/webstore_install_prompt.cc:37:
set_install_source(WebstoreInstaller::INSTALL_SOURCE_OTHER);
since it's a separate constructor, maybe DCHECK(parent_window)

(I'd also be fine with getting rid of the 3-arg constructor)

https://codereview.chromium.org/298303002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/webstore_install_prompt.cc:70: return
make_scoped_ptr(new ExtensionInstallPrompt(NULL));
I'm not sure this is safe.. it looks like passing a NULL WebContents will also
result in the profile being NULL, and I'm pretty sure the install UI needs a
profile :).

Note there are comments like

ExtensionInstallUIDefault::ExtensionInstallUIDefault(Profile* profile)
    : ExtensionInstallUI(profile),
      previous_using_system_theme_(false),
      use_app_installed_bubble_(false) {
  // |profile| can be NULL during tests.
  if (profile) {


Which is why it might seem to work..

[edit:] And it looks like the StartupInstaller relied on it..... ick. Maybe that
means it is skipping any install dialogs completely, which might not be good
here.

https://codereview.chromium.org/298303002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/webstore_install_prompt.cc:89: *error = "";
nit: "" -> std::string()  (doesn't malloc a 1-byte array)

https://codereview.chromium.org/298303002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/webstore_install_prompt.cc:97: *error = "";
std::string()

https://codereview.chromium.org/298303002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/webstore_install_prompt.h (right):

https://codereview.chromium.org/298303002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/webstore_install_prompt.h:11: #include
"webstore_standalone_installer.h"
this should have the full path

https://codereview.chromium.org/298303002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/webstore_install_prompt.h:17: namespace extensions {
Things under chrome/browser don't typically go in a namespace
(WebstoreStandaloneInstaller is, but I think that's a bug)

https://codereview.chromium.org/298303002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/webstore_install_prompt.h:32: typedef
WebstoreStandaloneInstaller::Callback Callback;
I.. think this line is redundant -- inheriting from WebstoreStandaloneInstaller
brings its nested types in. (Although `Callback` is possibly a bad name anyway,
since it looks so much like base::Callback - I'd maybe favour something like
`InstalledCallback`, but that's orthogonal).

If removing this line works, I'd go with that -- duplicating the dec here means
information about what parameters the callback takes is harder to find.

https://codereview.chromium.org/298303002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/webstore_install_prompt.h:43: Profile* profile,
nit: indenting is off

https://codereview.chromium.org/298303002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/webstore_install_prompt.h:51: //
extensions::WebstoreStartupInstaller overrides:
Startup -> Standalone?

https://codereview.chromium.org/298303002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/webstore_install_prompt.h:58: const OVERRIDE;
nit: const/OVERRIDE shouldn't appear on a line by itself. `git cl format` should
fix this properly now.

https://codereview.chromium.org/298303002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/webstore_install_prompt.h:75:
scoped_ptr<content::WebContents> dummy_web_contents_;
nit: #include scoped_ptr for this

https://codereview.chromium.org/298303002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/webstore_install_prompt.h:77: gfx::NativeWindow
parent_window_;
nit: #include native_widget_types.h

https://codereview.chromium.org/298303002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/webstore_install_prompt.h:79:
DISALLOW_IMPLICIT_CONSTRUCTORS(WebstoreInstallPrompt);
nit: I think this is only if you don't define *any* constructors, otherwise the
compiler hides the implicit default constructor for you. So
DISALLOW_COPY_AND_ASSIGN is probably better

https://codereview.chromium.org/298303002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/webstore_startup_installer.cc (right):

https://codereview.chromium.org/298303002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/webstore_startup_installer.cc:16:
set_install_source(WebstoreInstaller::INSTALL_SOURCE_INLINE);
hm - would it be better to force child classes to pass the install source to the
WebstoreInstallPrompt constructor?

https://codereview.chromium.org/298303002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/webstore_startup_installer.h (left):

https://codereview.chromium.org/298303002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/webstore_startup_installer.h:9: #include
"webstore_standalone_installer.h"
:o

https://codereview.chromium.org/298303002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/webstore_startup_installer.h (right):

https://codereview.chromium.org/298303002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/webstore_startup_installer.h:8: #include
"webstore_install_prompt.h"
this should be the full path.

[tmdiep, 2014-05-28 08:39:27]

Thanks for the review! Comments addressed in patch set 3.

https://codereview.chromium.org/298303002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/webstore_install_prompt.cc (right):

https://codereview.chromium.org/298303002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/webstore_install_prompt.cc:70: return
make_scoped_ptr(new ExtensionInstallPrompt(NULL));
On 2014/05/28 06:28:46, tapted wrote:
> I'm not sure this is safe.. it looks like passing a NULL WebContents will also
> result in the profile being NULL, and I'm pretty sure the install UI needs a
> profile :).
> 
> Note there are comments like
> 
> ExtensionInstallUIDefault::ExtensionInstallUIDefault(Profile* profile)
>     : ExtensionInstallUI(profile),
>       previous_using_system_theme_(false),
>       use_app_installed_bubble_(false) {
>   // |profile| can be NULL during tests.
>   if (profile) {
> 
> 
> Which is why it might seem to work..
> 
> [edit:] And it looks like the StartupInstaller relied on it..... ick. Maybe
that
> means it is skipping any install dialogs completely, which might not be good
> here.

Ugh. Using the ExtensionInstallPrompt(content::WebContents*) with NULL does show
a prompt, but I think it has been able to do without it so far.

I changed the call to always use the 3-arg constructor so that profile and
navigator are never NULL. A navigator is required to invoke links, but only
inline install dialogs contain links, so the startup installer has survived so
far without it. The native window arg seems to be only used to parent the window
and can be NULL.

https://codereview.chromium.org/298303002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/webstore_install_prompt.h (right):

https://codereview.chromium.org/298303002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/webstore_install_prompt.h:58: const OVERRIDE;
On 2014/05/28 06:28:46, tapted wrote:
> nit: const/OVERRIDE shouldn't appear on a line by itself. `git cl format`
should
> fix this properly now. 

git cl format put the "const OVERRIDE" in a new line. Maybe it prefers the
function name to fit on the first line?

https://codereview.chromium.org/298303002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/webstore_startup_installer.cc (right):

https://codereview.chromium.org/298303002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/webstore_startup_installer.cc:16:
set_install_source(WebstoreInstaller::INSTALL_SOURCE_INLINE);
On 2014/05/28 06:28:46, tapted wrote:
> hm - would it be better to force child classes to pass the install source to
the
> WebstoreInstallPrompt constructor?

WebstoreInstallPrompt is instantiated, as well as inherited from. If
instantiated, the source would be INSTALL_SOURCE_OTHER and I prefer callers not
worry about this parameter.

[tmdiep, 2014-05-28 08:40:26]

asargent: Can you please look over the refactoring of the installers, thanks!

[tapted, 2014-05-28 12:57:32]

lgtm but wait for Antony (and you'll need skuhne@ for ash/launcher OWNERS)

https://codereview.chromium.org/298303002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/webstore_install_prompt.h (right):

https://codereview.chromium.org/298303002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/webstore_install_prompt.h:58: const OVERRIDE;
On 2014/05/28 08:39:27, tmdiep wrote:
> On 2014/05/28 06:28:46, tapted wrote:
> > nit: const/OVERRIDE shouldn't appear on a line by itself. `git cl format`
> should
> > fix this properly now. 
> 
> git cl format put the "const OVERRIDE" in a new line. Maybe it prefers the
> function name to fit on the first line?

probably an accident/clang-format bug - clang-format was changed recently to
ensure 'OVERRIDE' didn't appear on a line by itself to match the "const
shouldn't appear on a line by itself" style rule. But I guess nobody specified
what to do if there are both (of if they did, it's OK)...  I'll defer to
clang-format :)

[Mr4D (OOO till 08-26), 2014-05-28 15:42:15]

Please see my comments!

https://codereview.chromium.org/298303002/diff/80001/chrome/browser/ui/ash/la...
File chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc
(right):

https://codereview.chromium.org/298303002/diff/80001/chrome/browser/ui/ash/la...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:162:
parent_window ? parent_window->GetNativeWindow() : NULL,
I am not quite sure that I understand why you need this window. It feels like
that you are using a "random" window as a parent (which might even be hidden).
You might therefore be better off to use e.g. the shelf's window instead?

https://codereview.chromium.org/298303002/diff/80001/chrome/browser/ui/ash/la...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/298303002/diff/80001/chrome/browser/ui/ash/la...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:585: return;
How can you have a shelfID without the application being installed?

Am I right that either we "promote a Chrome app" via shelf - or the user "tries"
an app via chrome store, and after doing so he can install the app for real? If
so - is the app in the shelf marked somehow so that the user knows that? It
feels rather confusing somehow.

[tmdiep, 2014-05-28 23:22:20]

Thanks for the comments.

https://codereview.chromium.org/298303002/diff/80001/chrome/browser/ui/ash/la...
File chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc
(right):

https://codereview.chromium.org/298303002/diff/80001/chrome/browser/ui/ash/la...
chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc:162:
parent_window ? parent_window->GetNativeWindow() : NULL,
On 2014/05/28 15:42:15, Mr4D wrote:
> I am not quite sure that I understand why you need this window. It feels like
> that you are using a "random" window as a parent (which might even be hidden).
> You might therefore be better off to use e.g. the shelf's window instead?

The purpose of the window is for positioning the install dialog. Ideally, I
would use the foreground window of the app. If there is no visible window, we
can just pass in NULL for the parent and it will center the dialog on the
screen.

Good point about hidden windows; I'll refactor the parent window selection.

https://codereview.chromium.org/298303002/diff/80001/chrome/browser/ui/ash/la...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/298303002/diff/80001/chrome/browser/ui/ash/la...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:585: return;
On 2014/05/28 15:42:15, Mr4D wrote:
> How can you have a shelfID without the application being installed?
> 
> Am I right that either we "promote a Chrome app" via shelf - or the user
"tries"
> an app via chrome store, and after doing so he can install the app for real?
If
> so - is the app in the shelf marked somehow so that the user knows that? It
> feels rather confusing somehow.

Yes, that's about right - Ephemeral apps are technically installed in extension
system (albiet transiently), but we hide them from UI such as the app launcher
and NTP to give the impression of not being permanently installed. But we give
the user options to install it for real - exact UI for this is still under
discussion. Feel free to express your opinions on this bug: crbug.com/375998. I
agree that it would be useful to incorporate something that would clearly
differentiate ephemeral apps from regular installed apps.

[asargent_no_longer_on_chrome, 2014-05-28 23:33:03]

webstore installer changes lgtm w/ a question about whether we can come up with
a better name

https://codereview.chromium.org/298303002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/webstore_install_prompt.h (right):

https://codereview.chromium.org/298303002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/webstore_install_prompt.h:23: // the user confirms. No
post-install UI is shown.
slight nit: the name WebstoreInstallPrompt seems a little misleading given the
full list of responsibilities mentioned here. Would something like
"WebstoreInstallWithPrompt" or "WebstorePromptingInstaller" be better?

[Mr4D (OOO till 08-26), 2014-05-29 00:15:38]

The only thing - which escapes the scope of this CL - what I would like to see
is to get an icon which somehow shows that the app is of temporary nature.

lgtm

[tmdiep, 2014-05-29 00:16:36]

On 2014/05/28 23:33:03, Antony Sargent wrote:
> webstore installer changes lgtm w/ a question about whether we can come up
with
> a better name
> 
>
https://codereview.chromium.org/298303002/diff/80001/chrome/browser/extension...
> File chrome/browser/extensions/webstore_install_prompt.h (right):
> 
>
https://codereview.chromium.org/298303002/diff/80001/chrome/browser/extension...
> chrome/browser/extensions/webstore_install_prompt.h:23: // the user confirms.
No
> post-install UI is shown.
> slight nit: the name WebstoreInstallPrompt seems a little misleading given the
> full list of responsibilities mentioned here. Would something like
> "WebstoreInstallWithPrompt" or "WebstorePromptingInstaller" be better?

+benwells

I'm glad you raised the issue of the name! We can move this to email if this is
the wrong forum.

Ben raised the point that there are many classes named something-installer and
it might be confusing to someone new to this part of the code.
WebstoreStandaloneInstaller, WebstoreInstaller and CrxInstaller form a pipeline
and have very specific roles, so perhaps it is a good idea to rename them to
reflect this. I'd be interested to hear your opinion.

I'm happy to change WebstoreInstallPrompt to WebstoreInstallWithPrompt. My
thoughts for renaming the other installers:
WebstoreStandaloneInstaller -> WebstoreInstallWithPromptBase?
WebstoreInstaller -> WebstoreExtensionFetcher, or WebstoreExtensionDownloader?
CrxInstaller - Can't think of a better name for this; it unpacks, validates,
prompts if required. So probably can keep as is.

[tmdiep, 2014-05-29 02:00:27]

Patch set 4 - Addresses Skuhne's review comments (parent window of the install
dialog)
Patch set 5 - Addresses asargent's review comments (rename to
WebstoreInstallWithPrompt)

[asargent_no_longer_on_chrome, 2014-05-29 17:28:46]

still lgtm, in case you were waiting for me

Also, I like your renaming suggestions for the other classes too (maybe best to
do that in a separate CL from this one?)

[tmdiep, 2014-05-29 22:29:34]

The CQ bit was checked by tmdiep@chromium.org

[commit-bot: I haz the power, 2014-05-29 22:30:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tmdiep@chromium.org/298303002/140001

[tmdiep, 2014-05-29 23:28:10]

On 2014/05/29 17:28:46, Antony Sargent wrote:
> Also, I like your renaming suggestions for the other classes too (maybe best
to
> do that in a separate CL from this one?)

Yes, I was planning to do this in a separate patch.

[commit-bot: I haz the power, 2014-05-30 02:07:50]

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

[commit-bot: I haz the power, 2014-05-30 07:19:40]

Change committed as 273749