Moved IS_EPHEMERAL flag to extension prefs

Moved IS_EPHEMERAL flag to extension prefs

This patch moves the IS_EPHEMERAL flag from the Extension class to
extension prefs. This is because an Extension object is immutable and
this flag needs to be changed after creation to quickly promote a
running ephemeral app to being installed. Otherwise the install must
be delayed until the ephemeral app is closed.

BUG=374018
TBR=sky@chromium.org
TEST=All automated tests

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

[tmdiep, 2014-05-15 02:18:42]

benwells: Before I went on leave, we discussed moving the IS_EPHEMERAL flag to
prefs. Otherwise we need to wait until an ephemeral app is closed before it can
be promoted to installed. Can chat in person if this doesn't make sense.

This patch is still WIP (mostly finished, but need to write more tests), but I
wanted to get your opinion on the approach.

Sorry for the huge patch. Best to look at extension_prefs, e/b/extension_util
and c/b/e/extension_ui_util first. Everything else is a result of the
refactoring.

Thanks!

https://codereview.chromium.org/282103003/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_browsertest.cc (right):

https://codereview.chromium.org/282103003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_browsertest.cc:471: // TODO(tmdiep): remove
this hack
TODO in this patch before committing.

https://codereview.chromium.org/282103003/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_ui_util.cc (right):

https://codereview.chromium.org/282103003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_ui_util.cc:18: bool IsBlockedByPolicy(const
Extension* app, content::BrowserContext* context) {
There is some deviation from previous behavior here.

Previously, only app_launcher_handler.cc (for NTP) and
extension_app_model_builder.cc (for app launcher) checked the HideWebStoreIcon
pref. But if the web store icon is hidden there, I think it makes sense to also
hide it in other locations, e.g. extension_app_provider.cc (autocomplete),
app_search_provider.cc (search in app launcher), recommended_apps.cc (app
launcher), etc.

If I should just conform to the previous behavior, I can fix this patch.

[benwells, 2014-05-16 03:39:08]

from a quick glance the approach lg. I haven't done more than glance though.

Let me know when it's ready for a full review.

[benwells, 2014-05-19 02:06:53]

https://codereview.chromium.org/282103003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/282103003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1700: bool is_ephemeral,
Could we make ephemerality less central?

Do you have a feel for what the change would look like if hExtensionService had
no notion of is_ephemeral, prefs OnExtensionInstalled didn't take is_ephemeral,
and the prefs were fiddled by the EphemeralAppService?

https://codereview.chromium.org/282103003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/extension_sync_service.cc (right):

https://codereview.chromium.org/282103003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/extension_sync_service.cc:61: bool CanSyncApp(const
Extension* extension, Profile* profile) {
Not: CanSyncApp -> ShouldSyncApp

https://codereview.chromium.org/282103003/diff/100001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_test_util.h (right):

https://codereview.chromium.org/282103003/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_test_util.h:5: #ifndef
CHROME_BROWSER_UI_APP_LIST_APP_LIST_TEST_UTIL_H_
Is this part of your change?

[tmdiep, 2014-05-19 02:51:16]

https://codereview.chromium.org/282103003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/282103003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1700: bool is_ephemeral,
On 2014/05/19 02:06:53, benwells wrote:
> Could we make ephemerality less central?
> 
> Do you have a feel for what the change would look like if hExtensionService
had
> no notion of is_ephemeral, prefs OnExtensionInstalled didn't take
is_ephemeral,
> and the prefs were fiddled by the EphemeralAppService?

That would be quite nice actually. One problem is that EphemeralAppService is in
c/b/apps. Looking at the patch, there is code everywhere that needs to do
something differently for ephemeral apps in e/b and c/b/e. So that messes up the
dependencies.

Will think about this some more and see if I can come up with a nice solution.

https://codereview.chromium.org/282103003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/extension_sync_service.cc (right):

https://codereview.chromium.org/282103003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/extension_sync_service.cc:61: bool CanSyncApp(const
Extension* extension, Profile* profile) {
On 2014/05/19 02:06:53, benwells wrote:
> Not: CanSyncApp -> ShouldSyncApp

Done.

https://codereview.chromium.org/282103003/diff/100001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_test_util.h (right):

https://codereview.chromium.org/282103003/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_test_util.h:5: #ifndef
CHROME_BROWSER_UI_APP_LIST_APP_LIST_TEST_UTIL_H_
On 2014/05/19 02:06:53, benwells wrote:
> Is this part of your change?

Yes. I added a test for ephemeral apps to
extension_app_model_builder_unittest.cc and app_search_provider_unittest.cc, and
saw that they duplicated a chunk of code in order to use a common set of test
files. So refactored that bit into this class to avoid making the same change in
2 different places.

[tmdiep, 2014-05-19 06:42:47]

On 2014/05/19 02:06:53, benwells wrote:
> Could we make ephemerality less central?
> 
> Do you have a feel for what the change would look like if hExtensionService
had
> no notion of is_ephemeral, prefs OnExtensionInstalled didn't take
is_ephemeral,
> and the prefs were fiddled by the EphemeralAppService?

Ok I had a think about this, and this is the best I can do to make ephemerality
less central:

- I can remove ephemerality out of extensions/*.
- The is_ephemeral flag (& other related flags) can be moved into
EphemeralAppService.
- But if the flag is contained within EphemeralAppService, I will need to move
this service out of c/b/a and into c/b/e. Unfortunately too many things in c/b/e
need to query this flag to do something different for ephemeral apps (vs regular
installed apps & extensions).

Hopefully this is a reasonable compromise?

[benwells, 2014-05-20 03:07:44]

On 2014/05/19 06:42:47, tmdiep wrote:
> On 2014/05/19 02:06:53, benwells wrote:
> > Could we make ephemerality less central?
> > 
> > Do you have a feel for what the change would look like if hExtensionService
> had
> > no notion of is_ephemeral, prefs OnExtensionInstalled didn't take
> is_ephemeral,
> > and the prefs were fiddled by the EphemeralAppService?
> 
> Ok I had a think about this, and this is the best I can do to make
ephemerality
> less central:
> 
> - I can remove ephemerality out of extensions/*.
> - The is_ephemeral flag (& other related flags) can be moved into
> EphemeralAppService.
> - But if the flag is contained within EphemeralAppService, I will need to move
> this service out of c/b/a and into c/b/e. Unfortunately too many things in
c/b/e
> need to query this flag to do something different for ephemeral apps (vs
regular
> installed apps & extensions).
> 
> Hopefully this is a reasonable compromise?

After discussing I'm OK with this as is. I had asked another question and had a
minor nit, if you answer / address those I'll finish the review.

[tmdiep, 2014-05-20 03:22:34]

On 2014/05/20 03:07:44, benwells wrote:
> After discussing I'm OK with this as is. I had asked another question and had
a
> minor nit, if you answer / address those I'll finish the review.

Yep, I fix the nit in patch set 4 and answered the question in comment #4.

[benwells, 2014-05-20 03:49:18]

lgtm

[tmdiep, 2014-05-21 06:23:04]

The CQ bit was checked by tmdiep@chromium.org

[commit-bot: I haz the power, 2014-05-21 06:25:04]

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

[commit-bot: I haz the power, 2014-05-21 20:02:54]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-05-21 20:17:44]

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

[commit-bot: I haz the power, 2014-05-21 20:17:45]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[tmdiep, 2014-05-22 00:08:21]

The CQ bit was checked by tmdiep@chromium.org

[commit-bot: I haz the power, 2014-05-22 00:12:00]

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

[commit-bot: I haz the power, 2014-05-22 06:24:19]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-22 11:12:28]

Change committed as 272157