New test cases for kiosk app crx file cache.

New test cases for kiosk app crx file cache.

Also fixes a bug in local_cache_extension.cc for app version update.

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

[jennyz, 2014-05-09 20:08:08]

dpolukhin@, please review the change in local_extension_cache.cc.

https://codereview.chromium.org/271263002/diff/1/chrome/browser/extensions/up...
File chrome/browser/extensions/updater/local_extension_cache.cc (left):

https://codereview.chromium.org/271263002/diff/1/chrome/browser/extensions/up...
chrome/browser/extensions/updater/local_extension_cache.cc:430: it =
cached_extensions_.insert(std::make_pair(id, info)).first;
std::map won't insert the element if the key exists. Therefore, the version is
not updated in cached_extensions_ for the app with |id|. This bug is exposed
during KioskAppManagerTest::UpdateApp.

[Dmitry Polukhin, 2014-05-09 20:35:26]

LGTM

Thanks for fixing bug in cache!

[xiyuan, 2014-05-09 20:49:15]

Thank you for adding tests and improve the test code. :)

https://codereview.chromium.org/271263002/diff/1/chrome/browser/chromeos/app_...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.h (right):

https://codereview.chromium.org/271263002/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_app_manager.h:215: void
GetKioskAppCrxCacheDir(base::FilePath* cache_dir);
nit: GetKioskAppCrxCacheDir -> GetCrxCacheDir

https://codereview.chromium.org/271263002/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_app_manager.h:217: bool
GetKioskAppExtension(const std::string& app_id,
nit: KioskApp feels redundant since it is in the interface of KioskAppManager.

GetKioskAppExtension -> GetCachedCrx

https://codereview.chromium.org/271263002/diff/1/chrome/browser/chromeos/app_...
File chrome/browser/chromeos/app_mode/kiosk_app_manager_browsertest.cc (right):

https://codereview.chromium.org/271263002/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_app_manager_browsertest.cc:53: const char
kCrxDownloadPath[] = "/webstore/downloads/";
This feels like part of KioskTestCommon (FakeCWS).

https://codereview.chromium.org/271263002/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_app_manager_browsertest.cc:151:
AppDataLoadWaiter(KioskAppManager* manager, int data_loaded_threshold)
Let's keep just this one and remove the one in line 145.

https://codereview.chromium.org/271263002/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_app_manager_browsertest.cc:161:
manager_->AddObserver(this);
This might need to go into ctor and RemoveObserver go to dtor.

We also need to test data_change_count_ >= data_loaded_threshold_ and bail out
if there is no need to wait.

https://codereview.chromium.org/271263002/diff/1/chrome/browser/chromeos/app_...
File chrome/browser/chromeos/app_mode/kiosk_test_common.h (right):

https://codereview.chromium.org/271263002/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_test_common.h:17: class KioskTestCommon {
Let's move more stuff into this class and make a FakeCWS (similar to FakeGaia)
out of it.

Things such as setup switches::kAppsGalleryURL,
switches::kAppsGalleryDownloadURL and switches::kAppsGalleryUpdateURL could all
be moved here. We need the test server's url to initialize them. This can be
done by passing the test server's url when it is available. 

Test suite class is better to have a FakeCWS as its member instead of deriving
from it.

[jennyz, 2014-05-13 00:31:20]

https://codereview.chromium.org/271263002/diff/1/chrome/browser/chromeos/app_...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.h (right):

https://codereview.chromium.org/271263002/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_app_manager.h:215: void
GetKioskAppCrxCacheDir(base::FilePath* cache_dir);
On 2014/05/09 20:49:15, xiyuan wrote:
> nit: GetKioskAppCrxCacheDir -> GetCrxCacheDir

Done.

https://codereview.chromium.org/271263002/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_app_manager.h:217: bool
GetKioskAppExtension(const std::string& app_id,
On 2014/05/09 20:49:15, xiyuan wrote:
> nit: KioskApp feels redundant since it is in the interface of KioskAppManager.
> 
> GetKioskAppExtension -> GetCachedCrx

Done.

https://codereview.chromium.org/271263002/diff/1/chrome/browser/chromeos/app_...
File chrome/browser/chromeos/app_mode/kiosk_app_manager_browsertest.cc (right):

https://codereview.chromium.org/271263002/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_app_manager_browsertest.cc:53: const char
kCrxDownloadPath[] = "/webstore/downloads/";
On 2014/05/09 20:49:15, xiyuan OOO 5-12 wrote:
> This feels like part of KioskTestCommon (FakeCWS).

Done.

https://codereview.chromium.org/271263002/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_app_manager_browsertest.cc:151:
AppDataLoadWaiter(KioskAppManager* manager, int data_loaded_threshold)
On 2014/05/09 20:49:15, xiyuan wrote:
> Let's keep just this one and remove the one in line 145.

Done.

https://codereview.chromium.org/271263002/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_app_manager_browsertest.cc:161:
manager_->AddObserver(this);
On 2014/05/09 20:49:15, xiyuan wrote:
> This might need to go into ctor and RemoveObserver go to dtor.
> 
> We also need to test data_change_count_ >= data_loaded_threshold_ and bail out
> if there is no need to wait.

Done.

https://codereview.chromium.org/271263002/diff/1/chrome/browser/chromeos/app_...
File chrome/browser/chromeos/app_mode/kiosk_test_common.h (right):

https://codereview.chromium.org/271263002/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_test_common.h:17: class KioskTestCommon {
On 2014/05/09 20:49:15, xiyuan OOO 5-12 wrote:
> Let's move more stuff into this class and make a FakeCWS (similar to FakeGaia)
> out of it.
> 
> Things such as setup switches::kAppsGalleryURL,
> switches::kAppsGalleryDownloadURL and switches::kAppsGalleryUpdateURL could
all
> be moved here. We need the test server's url to initialize them. This can be
> done by passing the test server's url when it is available. 
> 
> Test suite class is better to have a FakeCWS as its member instead of deriving
> from it.

Done.

[xiyuan, 2014-05-13 16:43:34]

https://codereview.chromium.org/271263002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/app_mode/fake_cws.h (right):

https://codereview.chromium.org/271263002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/fake_cws.h:14: using
net::test_server::HttpResponse;
"using" is not allowed in header file.

https://codereview.chromium.org/271263002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/fake_cws.h:25: void Init(EmbeddedTestServer*
embedded_test_server, bool setup_gallery_only);
We probably can setup everything all the time and get rid of
|setup_gallery_only|.

https://codereview.chromium.org/271263002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/fake_cws.h:26: void
SetupCrxDownloadAndUpdateUrls(EmbeddedTestServer* embedded_test_server);
No need to expose this if we setup everything all the time.

https://codereview.chromium.org/271263002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/fake_cws.h:33: std::string GetWebStoreDomain()
const;
nit: probably no need to expose this.

https://codereview.chromium.org/271263002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/app_mode/kiosk_app_manager_browsertest.cc (right):

https://codereview.chromium.org/271263002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_app_manager_browsertest.cc:153: if
(quit_)
Can we use |loaded_| and get rid of |quit_|?

https://codereview.chromium.org/271263002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_app_manager_browsertest.cc:229:
host_resolver()->AddRule(fake_cws_->GetWebStoreDomain(), "127.0.0.1");
We can just map "*" to 127.0.0.1. No need to just do it for cws domain.

https://codereview.chromium.org/271263002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/kiosk_browsertest.cc (right):

https://codereview.chromium.org/271263002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/kiosk_browsertest.cc:515: scoped_ptr<FakeCWS>
fake_cws_;
Member variable should be listed last. In this case, this should stay with other
member variables in private section.

[jennyz, 2014-05-13 18:36:11]

https://codereview.chromium.org/271263002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/app_mode/fake_cws.h (right):

https://codereview.chromium.org/271263002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/fake_cws.h:14: using
net::test_server::HttpResponse;
On 2014/05/13 16:43:35, xiyuan wrote:
> "using" is not allowed in header file.

Done.

https://codereview.chromium.org/271263002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/fake_cws.h:25: void Init(EmbeddedTestServer*
embedded_test_server, bool setup_gallery_only);
On 2014/05/13 16:43:35, xiyuan wrote:
> We probably can setup everything all the time and get rid of
> |setup_gallery_only|.

yes, changed to always mock all urls. This makes several KioskAppManagerTest
cases failed, and I fixed them, PTAL.

https://codereview.chromium.org/271263002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/fake_cws.h:26: void
SetupCrxDownloadAndUpdateUrls(EmbeddedTestServer* embedded_test_server);
On 2014/05/13 16:43:35, xiyuan wrote:
> No need to expose this if we setup everything all the time.

Done.

https://codereview.chromium.org/271263002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/fake_cws.h:33: std::string GetWebStoreDomain()
const;
On 2014/05/13 16:43:35, xiyuan wrote:
> nit: probably no need to expose this.

Done.

https://codereview.chromium.org/271263002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/app_mode/kiosk_app_manager_browsertest.cc (right):

https://codereview.chromium.org/271263002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_app_manager_browsertest.cc:153: if
(quit_)
On 2014/05/13 16:43:35, xiyuan wrote:
> Can we use |loaded_| and get rid of |quit_|?

In OnKioskAppDataLoadFailure case, loaded will be false, and we still want to
quit, so we can't only rely on |loaded_| to quit.

https://codereview.chromium.org/271263002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_app_manager_browsertest.cc:229:
host_resolver()->AddRule(fake_cws_->GetWebStoreDomain(), "127.0.0.1");
On 2014/05/13 16:43:35, xiyuan wrote:
> We can just map "*" to 127.0.0.1. No need to just do it for cws domain.

Done.

https://codereview.chromium.org/271263002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/kiosk_browsertest.cc (right):

https://codereview.chromium.org/271263002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/kiosk_browsertest.cc:515: scoped_ptr<FakeCWS>
fake_cws_;
On 2014/05/13 16:43:35, xiyuan wrote:
> Member variable should be listed last. In this case, this should stay with
other
> member variables in private section.

Done.

[xiyuan, 2014-05-13 18:41:17]

LGTM!

[jennyz, 2014-05-13 20:22:27]

The CQ bit was checked by jennyz@chromium.org

[commit-bot: I haz the power, 2014-05-13 20:23:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jennyz@chromium.org/271263002/80001

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

Change committed as 270261