Clear the ephemeral app cache from Clear Browsing Data

Clear the ephemeral app cache from Clear Browsing Data

Selecting the "Cached images and files" in the Clear Browsing Data
dialog will now remove all idle ephemeral apps.

BUG=392007
TEST=browser_tests

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

[tmdiep, 2014-07-10 23:25:25]

The ephemeral app cache will now be cleared as part of the BrowsingDataRemover.
I didn't take into account the time period as it didn't seem necessary and not
all data removed in this class does.

tapted: Please review c/b/apps.
markusheintz: Please review c/b/browsing_data.
Thanks.

[tapted, 2014-07-11 01:16:36]

https://codereview.chromium.org/383703002/diff/1/chrome/browser/apps/ephemera...
File chrome/browser/apps/ephemeral_app_service.cc (right):

https://codereview.chromium.org/383703002/diff/1/chrome/browser/apps/ephemera...
chrome/browser/apps/ephemeral_app_service.cc:93:
!service->GetInstalledExtension(extension_id)) {
I think GetInstalledExtension(..) should return true for everything in
GenerateInstalledExtensionsSet - maybe DCHECK? (also note GetInstalledExtension
is marked deprecated)

unless uninstalling one extension can cascade and trigger an uninstall of
another..

https://codereview.chromium.org/383703002/diff/1/chrome/browser/apps/ephemera...
chrome/browser/apps/ephemeral_app_service.cc:231: if
(!service->GetInstalledExtension(*id))
I don't think I've wrapped my head around this bit. Can you add a comment?  My
interpretation: since IsExtensionIdle returns `false` for extensions that it
can't find we can end up here with an extension that's been uninstalled. But
then I'm not sure why you don't need to clear out something else, or do
`--app_count` before continuing. And I'm not sure why it would have appeared in
the set generated by GenerateInstalledExtensionsSet in the first place.

Also would this apply to things uninstalled by sync as well as clear browsing
data?

(also GetInstalledExtension is marked deprecated - use the alternative?)

https://codereview.chromium.org/383703002/diff/1/chrome/browser/apps/ephemera...
File chrome/browser/apps/ephemeral_app_service_browsertest.cc (right):

https://codereview.chromium.org/383703002/diff/1/chrome/browser/apps/ephemera...
chrome/browser/apps/ephemeral_app_service_browsertest.cc:146:
ephemeral_service->ClearCachedApps();
before this, perhaps EXPECT_EQ(2, ephemeral_service->ephemeral_app_count());

https://codereview.chromium.org/383703002/diff/1/chrome/browser/apps/ephemera...
chrome/browser/apps/ephemeral_app_service_browsertest.cc:149:
ExtensionSystem::Get(browser()->profile())->extension_service();
nit: I think you're inheriting an `extension_service()` helper function from
ExtensionBrowserTest, which can simplify this

[tmdiep, 2014-07-11 02:28:31]

https://codereview.chromium.org/383703002/diff/1/chrome/browser/apps/ephemera...
File chrome/browser/apps/ephemeral_app_service.cc (right):

https://codereview.chromium.org/383703002/diff/1/chrome/browser/apps/ephemera...
chrome/browser/apps/ephemeral_app_service.cc:93:
!service->GetInstalledExtension(extension_id)) {
On 2014/07/11 01:16:36, tapted wrote:
> I think GetInstalledExtension(..) should return true for everything in
> GenerateInstalledExtensionsSet - maybe DCHECK? (also note
GetInstalledExtension
> is marked deprecated)
> 
> unless uninstalling one extension can cascade and trigger an uninstall of
> another..

There is a CHECK in ExtensionService::UninstallExtension() which makes me
nervous, so this is just a sanity check. If prefs->IsEphemeralApp() returns
true, there *should* be an extension in the registry, so I made this a DCHECK.

SharedModuleService::OnExtensionUninstalled() is a current case of cascading
uninstalls. That shouldn't apply here, but just in case there will be
dependencies between ephemeral apps in the future...

https://codereview.chromium.org/383703002/diff/1/chrome/browser/apps/ephemera...
chrome/browser/apps/ephemeral_app_service.cc:231: if
(!service->GetInstalledExtension(*id))
On 2014/07/11 01:16:36, tapted wrote:
> I don't think I've wrapped my head around this bit. Can you add a comment?  My
> interpretation: since IsExtensionIdle returns `false` for extensions that it
> can't find we can end up here with an extension that's been uninstalled. But
> then I'm not sure why you don't need to clear out something else, or do
> `--app_count` before continuing. And I'm not sure why it would have appeared
in
> the set generated by GenerateInstalledExtensionsSet in the first place.

Same reason as above - in case of cascading uninstalls.

Good point about the app count. I don't really need to maintain it here, 
OnExtensionUninstalled() will handle it. 

> Also would this apply to things uninstalled by sync as well as clear browsing
> data?

Ephemeral apps don't really get synced. But suppose you have an ephemeral app on
device A, you installed and uninstalled it on device B.
- If both the install and uninstall messages got through - the app gets promoted
and uninstalled (this case is half tested in browser tests - the install from
sync part).
- If only the uninstall message got through for some reason, I think the
ephemeral app would be removed, which is fine. I'll need to add a browser test
for this.

> (also GetInstalledExtension is marked deprecated - use the alternative?)

Done.

[tmdiep, 2014-07-11 03:11:43]

Oops, got distracted and forgot about the tests!

https://codereview.chromium.org/383703002/diff/1/chrome/browser/apps/ephemera...
File chrome/browser/apps/ephemeral_app_service_browsertest.cc (right):

https://codereview.chromium.org/383703002/diff/1/chrome/browser/apps/ephemera...
chrome/browser/apps/ephemeral_app_service_browsertest.cc:146:
ephemeral_service->ClearCachedApps();
On 2014/07/11 01:16:36, tapted wrote:
> before this, perhaps EXPECT_EQ(2, ephemeral_service->ephemeral_app_count());

Done.

https://codereview.chromium.org/383703002/diff/1/chrome/browser/apps/ephemera...
chrome/browser/apps/ephemeral_app_service_browsertest.cc:149:
ExtensionSystem::Get(browser()->profile())->extension_service();
On 2014/07/11 01:16:36, tapted wrote:
> nit: I think you're inheriting an `extension_service()` helper function from
> ExtensionBrowserTest, which can simplify this

Updated to use ExtensionRegistry since the function is deprecated.

[tapted, 2014-07-11 04:58:57]

lgtm

[tmdiep, 2014-07-15 11:13:04]

+mkwst for browsing_data_remover. Thanks in advance.

[Mike West, 2014-07-15 11:35:55]

https://codereview.chromium.org/383703002/diff/40001/chrome/browser/browsing_...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/383703002/diff/40001/chrome/browser/browsing_...
chrome/browser/browsing_data/browsing_data_remover.cc:611:
EphemeralAppService::Get(profile_)->ClearCachedApps();
Do ephemeral apps show up anywhere other than cache? If they're tied to an
origin, it might make sense to clear them when clearing cookies/site data.

[tmdiep, 2014-07-15 22:26:58]

https://codereview.chromium.org/383703002/diff/40001/chrome/browser/browsing_...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/383703002/diff/40001/chrome/browser/browsing_...
chrome/browser/browsing_data/browsing_data_remover.cc:611:
EphemeralAppService::Get(profile_)->ClearCachedApps();
On 2014/07/15 11:35:55, Mike West wrote:
> Do ephemeral apps show up anywhere other than cache? If they're tied to an
> origin, it might make sense to clear them when clearing cookies/site data.

I don't think any other existing category applies to them. They are Chrome V2
apps only (not hosted apps) not associated with any site.

[Mike West, 2014-07-16 07:59:14]

Understood. I think I confused this with a different feature.

LGTM.

[tmdiep, 2014-07-16 08:15:59]

On 2014/07/16 07:59:14, Mike West wrote:
> Understood. I think I confused this with a different feature.
> 
> LGTM.

No problem. I suspect you were thinking of Fizzy apps and serviceworkers, which
are also sometimes referred to as ephemeral apps. These do involve site data.

[tmdiep, 2014-07-16 08:16:14]

The CQ bit was checked by tmdiep@chromium.org

[commit-bot: I haz the power, 2014-07-16 08:17:53]

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

[commit-bot: I haz the power, 2014-07-16 21:52:26]

Change committed as 283529