Install and launch kiosk app from cached crx file at start up.

Install and launch kiosk app from cached crx file at start up.

BUG=369552, 379769
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283357

[xiyuan, 2014-05-29 19:49:01]

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

https://codereview.chromium.org/300843013/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/startup_app_launcher.cc:343:
update_check_pending_ = false;
We probably could consolidate |crx_cache_pending_| and |update_check_pending_|
into one flag because we only need to know whether the cached crx is ready to
use.

https://codereview.chromium.org/300843013/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/startup_app_launcher.cc:426: crx_installer_ =
CreateCrxInstaller(profile_);
Sorry that I did not mention this earlier. We should also consider following
AppPackUpdater and install cached crx by creating our own ExternalLoader.

https://codereview.chromium.org/300843013/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/startup_app_launcher.cc:435:
scoped_refptr<extensions::CrxInstaller> StartupAppLauncher::CreateCrxInstaller(
Consider merge this into BeginInstall.

https://codereview.chromium.org/300843013/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/startup_app_launcher.cc:446:
installer->set_install_cause(extension_misc::INSTALL_CAUSE_USER_DOWNLOAD);
How about use INSTALL_CAUSE_EXTERNAL_FILE? INSTALL_CAUSE_USER_DOWNLOAD triggers
many histograms.

https://codereview.chromium.org/300843013/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/startup_app_launcher.cc:451:
tracker->AddObserver(this);
This should be moved out of CreateCrxInstaller.

https://codereview.chromium.org/300843013/diff/1/chrome/browser/chromeos/exte...
File chrome/browser/chromeos/extensions/external_cache.cc (right):

https://codereview.chromium.org/300843013/diff/1/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/external_cache.cc:189: extensions_status_[id]
!= DOWNLOAD_FAILED;
Not sure if we should change this. Other call sites might use the function as a
signal of whether to use the cached extension. The code here changes that
expectation.

https://codereview.chromium.org/300843013/diff/1/chrome/browser/chromeos/exte...
File chrome/browser/chromeos/extensions/external_cache.h (right):

https://codereview.chromium.org/300843013/diff/1/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/external_cache.h:107: bool
IsExtensionPendingForUpdateCheck(const std::string& id);
Move this under IsExtensionPending so that it has the same location as in cc
file.

https://codereview.chromium.org/300843013/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/app_mode/kiosk_app_launch_error.h (right):

https://codereview.chromium.org/300843013/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_app_launch_error.h:25: UNABLE_TO_LAUNCH, 
       // Unable to launch app.
Append to the end so that existing enums still have the same numeric number.

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

https://codereview.chromium.org/300843013/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/kiosk_browsertest.cc:282: if
(!test_app_version().empty())
nit: {} since the branch is more than one line

[jennyz, 2014-07-07 21:11:13]

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

https://codereview.chromium.org/300843013/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/startup_app_launcher.cc:343:
update_check_pending_ = false;
On 2014/05/29 19:49:01, xiyuan (OOO until July 6) wrote:
> We probably could consolidate |crx_cache_pending_| and |update_check_pending_|
> into one flag because we only need to know whether the cached crx is ready to
> use.

Done.

https://codereview.chromium.org/300843013/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/startup_app_launcher.cc:426: crx_installer_ =
CreateCrxInstaller(profile_);
On 2014/05/29 19:49:01, xiyuan (OOO until July 6) wrote:
> Sorry that I did not mention this earlier. We should also consider following
> AppPackUpdater and install cached crx by creating our own ExternalLoader.

Removed CrxInstaller from StartupAppLauncher, used KioskAppExternalLoader now.

https://codereview.chromium.org/300843013/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/startup_app_launcher.cc:435:
scoped_refptr<extensions::CrxInstaller> StartupAppLauncher::CreateCrxInstaller(
On 2014/05/29 19:49:01, xiyuan (OOO until July 6) wrote:
> Consider merge this into BeginInstall.

This function has been removed.

https://codereview.chromium.org/300843013/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/startup_app_launcher.cc:446:
installer->set_install_cause(extension_misc::INSTALL_CAUSE_USER_DOWNLOAD);
On 2014/05/29 19:49:01, xiyuan (OOO until July 6) wrote:
> How about use INSTALL_CAUSE_EXTERNAL_FILE? INSTALL_CAUSE_USER_DOWNLOAD
triggers
> many histograms.

Function has been removed.

https://codereview.chromium.org/300843013/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/startup_app_launcher.cc:451:
tracker->AddObserver(this);
On 2014/05/29 19:49:01, xiyuan (OOO until July 6) wrote:
> This should be moved out of CreateCrxInstaller.

Done.

https://codereview.chromium.org/300843013/diff/1/chrome/browser/chromeos/exte...
File chrome/browser/chromeos/extensions/external_cache.cc (right):

https://codereview.chromium.org/300843013/diff/1/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/external_cache.cc:189: extensions_status_[id]
!= DOWNLOAD_FAILED;
On 2014/05/29 19:49:01, xiyuan (OOO until July 6) wrote:
> Not sure if we should change this. Other call sites might use the function as
a
> signal of whether to use the cached extension. The code here changes that
> expectation.

removed the change from this function.

https://codereview.chromium.org/300843013/diff/1/chrome/browser/chromeos/exte...
File chrome/browser/chromeos/extensions/external_cache.h (right):

https://codereview.chromium.org/300843013/diff/1/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/external_cache.h:107: bool
IsExtensionPendingForUpdateCheck(const std::string& id);
On 2014/05/29 19:49:01, xiyuan (OOO until July 6) wrote:
> Move this under IsExtensionPending so that it has the same location as in cc
> file.

Since this one is not virtual, so I won't put it under IsExtensionPending,
unless we add it to the ExtensionDownloaderDelegate. Moved its location is cc to
keep the order consistent with .h.

https://codereview.chromium.org/300843013/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/app_mode/kiosk_app_launch_error.h (right):

https://codereview.chromium.org/300843013/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_app_launch_error.h:25: UNABLE_TO_LAUNCH, 
       // Unable to launch app.
On 2014/05/29 19:49:01, xiyuan (OOO until July 6) wrote:
> Append to the end so that existing enums still have the same numeric number.

Done.

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

https://codereview.chromium.org/300843013/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/kiosk_browsertest.cc:282: if
(!test_app_version().empty())
On 2014/05/29 19:49:01, xiyuan (OOO until July 6) wrote:
> nit: {} since the branch is more than one line

Done.

[xiyuan, 2014-07-08 21:51:00]

https://codereview.chromium.org/300843013/diff/170001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/kiosk_app_external_loader.h (right):

https://codereview.chromium.org/300843013/diff/170001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/kiosk_app_external_loader.h:23: void
SetCurrentAppExtensions(scoped_ptr<base::DictionaryValue> prefs);
nit: #include "base/memory/scoped_ptr.h"

https://codereview.chromium.org/300843013/diff/170001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.h (right):

https://codereview.chromium.org/300843013/diff/170001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/kiosk_app_manager.h:157: bool
GetCachedCrx(const std::string& app_id,
nit: It seems we no longer need to expose crx path directly. The function is
used for testing existence of cached crx.

How about change this to:
bool HasCachedCrx(const std::string& app_id) const;

https://codereview.chromium.org/300843013/diff/170001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/kiosk_app_manager.h:167: bool
IsExtensionPendingForUpdateCheck(const std::string& id);
We can probably merge this with IsExtensionPendingForCache. The outside world
only needs to know whether the crx of given |id| is being downloaded or not.

https://codereview.chromium.org/300843013/diff/170001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/kiosk_app_manager.h:177: void
InstallKioskApp(const std::string& id);
nit: InstallKioskApp -> InstallFromCache to be more clear about what the code
does.

https://codereview.chromium.org/300843013/diff/170001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/startup_app_launcher.cc (right):

https://codereview.chromium.org/300843013/diff/170001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/startup_app_launcher.cc:100: base::FilePath
crx_path;
The network might take sometime to be up and initial download attempts from
ExternalCache might fail. This feels like a good place to start/retry them.

Think we can do
KioskAppManager::Get()->UpdateExternalCache(); (which could just call
UpdateAppData()) here and start to observe the load status. And call
BeginInstall when the crx is loaded in the observing method. 

MaybeInstall could probably be removed.

https://codereview.chromium.org/300843013/diff/170001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/startup_app_launcher.cc:275:
DCHECK(extension_id == app_id_);
nit: Remove the DCHECK since "if" below handles the case.

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

https://codereview.chromium.org/300843013/diff/170001/chrome/browser/chromeos...
chrome/browser/chromeos/login/kiosk_browsertest.cc:296:
SetupTestAppUpdateCheck();
These three lines don't feel belong here. Maybe move them to Setup() just below
we setup the default |test_app_id_|? This way, we could probably also get rid of
code such as line 549-550.

https://codereview.chromium.org/300843013/diff/170001/chrome/browser/extensio...
File chrome/browser/extensions/external_provider_impl.cc (right):

https://codereview.chromium.org/300843013/diff/170001/chrome/browser/extensio...
chrome/browser/extensions/external_provider_impl.cc:402: // No external app
install in app mode.
nit: Update the comment.

[xiyuan, 2014-07-09 16:27:50]

We might want to add a couple more test cases for the scenarios support by this
CL:
- network offline, has cached crx, first time run the app, and we should be able
to run it;
- network offline, app v1.0 has run before, has cached v2.0 crx and v2.0 should
be installed and launched;

[jennyz, 2014-07-12 00:03:21]

https://codereview.chromium.org/300843013/diff/170001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/kiosk_app_external_loader.h (right):

https://codereview.chromium.org/300843013/diff/170001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/kiosk_app_external_loader.h:23: void
SetCurrentAppExtensions(scoped_ptr<base::DictionaryValue> prefs);
On 2014/07/08 21:50:59, xiyuan wrote:
> nit: #include "base/memory/scoped_ptr.h"

Done.

https://codereview.chromium.org/300843013/diff/170001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.h (right):

https://codereview.chromium.org/300843013/diff/170001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/kiosk_app_manager.h:157: bool
GetCachedCrx(const std::string& app_id,
On 2014/07/08 21:50:59, xiyuan wrote:
> nit: It seems we no longer need to expose crx path directly. The function is
> used for testing existence of cached crx.
> 
> How about change this to:
> bool HasCachedCrx(const std::string& app_id) const;

This function is still needed in KioskAppManagerTest for verifying the cached
crx's version and file path. So I added a HasCachedCrx, and moved the
GetCachedCrx to private functions.

https://codereview.chromium.org/300843013/diff/170001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/kiosk_app_manager.h:167: bool
IsExtensionPendingForUpdateCheck(const std::string& id);
On 2014/07/08 21:50:59, xiyuan wrote:
> We can probably merge this with IsExtensionPendingForCache. The outside world
> only needs to know whether the crx of given |id| is being downloaded or not.

We don't need both functions now, removed.

https://codereview.chromium.org/300843013/diff/170001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/kiosk_app_manager.h:177: void
InstallKioskApp(const std::string& id);
On 2014/07/08 21:50:59, xiyuan wrote:
> nit: InstallKioskApp -> InstallFromCache to be more clear about what the code
> does.

Done.

https://codereview.chromium.org/300843013/diff/170001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/startup_app_launcher.cc (right):

https://codereview.chromium.org/300843013/diff/170001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/startup_app_launcher.cc:100: base::FilePath
crx_path;
On 2014/07/08 21:50:59, xiyuan wrote:
> The network might take sometime to be up and initial download attempts from
> ExternalCache might fail. This feels like a good place to start/retry them.
> 
> Think we can do
> KioskAppManager::Get()->UpdateExternalCache(); (which could just call
> UpdateAppData()) here and start to observe the load status. And call
> BeginInstall when the crx is loaded in the observing method. 
> 
> MaybeInstall could probably be removed.

Done.

https://codereview.chromium.org/300843013/diff/170001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/startup_app_launcher.cc:275:
DCHECK(extension_id == app_id_);
On 2014/07/08 21:50:59, xiyuan wrote:
> nit: Remove the DCHECK since "if" below handles the case.

Done.

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

https://codereview.chromium.org/300843013/diff/170001/chrome/browser/chromeos...
chrome/browser/chromeos/login/kiosk_browsertest.cc:296:
SetupTestAppUpdateCheck();
On 2014/07/08 21:51:00, xiyuan wrote:
> These three lines don't feel belong here. Maybe move them to Setup() just
below
> we setup the default |test_app_id_|? This way, we could probably also get rid
of
> code such as line 549-550.
For some reason, when I moved them to Setup(), KioskTest.LaunchAppUserCancel
starts to fail. I will leave them here, and take a detailed look at
LaunchAppUserCancel later.

https://codereview.chromium.org/300843013/diff/170001/chrome/browser/extensio...
File chrome/browser/extensions/external_provider_impl.cc (right):

https://codereview.chromium.org/300843013/diff/170001/chrome/browser/extensio...
chrome/browser/extensions/external_provider_impl.cc:402: // No external app
install in app mode.
On 2014/07/08 21:51:00, xiyuan wrote:
> nit: Update the comment.

Done.

[xiyuan, 2014-07-14 18:20:55]

Mostly good.

https://codereview.chromium.org/300843013/diff/250001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/startup_app_launcher.cc (right):

https://codereview.chromium.org/300843013/diff/250001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/startup_app_launcher.cc:248:
->GetInstalledExtension(app_id_);
nit: DCHECK(extension);

https://codereview.chromium.org/300843013/diff/250001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/startup_app_launcher.cc:283: // Check if the
app is offline enabled.
The code inside this branch looks like MaybeLaunchApp().

https://codereview.chromium.org/300843013/diff/250001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/startup_app_launcher.cc:330: } else if
(extensions::ExtensionSystem::Get(profile_)
Why do we need this "else" branch?

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

https://codereview.chromium.org/300843013/diff/250001/chrome/browser/chromeos...
chrome/browser/chromeos/login/kiosk_browsertest.cc:529:
set_test_crx_file(test_app_id() + ".crx");
Move this into a setup function?

[jennyz, 2014-07-14 22:33:01]

https://codereview.chromium.org/300843013/diff/250001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/startup_app_launcher.cc (right):

https://codereview.chromium.org/300843013/diff/250001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/startup_app_launcher.cc:248:
->GetInstalledExtension(app_id_);
On 2014/07/14 18:20:54, xiyuan wrote:
> nit: DCHECK(extension);

Done.

https://codereview.chromium.org/300843013/diff/250001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/startup_app_launcher.cc:283: // Check if the
app is offline enabled.
On 2014/07/14 18:20:54, xiyuan wrote:
> The code inside this branch looks like MaybeLaunchApp().

Done.

https://codereview.chromium.org/300843013/diff/250001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/startup_app_launcher.cc:330: } else if
(extensions::ExtensionSystem::Get(profile_)
On 2014/07/14 18:20:54, xiyuan wrote:
> Why do we need this "else" branch?
As we discussed, we won't launch the app if the app is installed but cache is
missing, consider this as an error case now.

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

https://codereview.chromium.org/300843013/diff/250001/chrome/browser/chromeos...
chrome/browser/chromeos/login/kiosk_browsertest.cc:529:
set_test_crx_file(test_app_id() + ".crx");
On 2014/07/14 18:20:55, xiyuan wrote:
> Move this into a setup function?

Done.

[xiyuan, 2014-07-15 02:32:51]

LGTM

https://codereview.chromium.org/300843013/diff/270001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/startup_app_launcher.cc (right):

https://codereview.chromium.org/300843013/diff/270001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/startup_app_launcher.cc:307: 
nit: reset |wait_for_crx_update_|.

https://codereview.chromium.org/300843013/diff/270001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/kiosk_browsertest.cc (left):

https://codereview.chromium.org/300843013/diff/270001/chrome/browser/chromeos...
chrome/browser/chromeos/login/kiosk_browsertest.cc:918:
SetupAppProfile("chromeos/app_mode/offline_enabled_app_profile");
We can delete chrome/test/data/chromeos/app_mode/offline_enabled_app_profile
now.

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

https://codereview.chromium.org/300843013/diff/270001/chrome/browser/chromeos...
chrome/browser/chromeos/login/kiosk_browsertest.cc:592:
set_test_crx_file(test_app_id() + ".crx");
Are these two lines necessary?

https://codereview.chromium.org/300843013/diff/270001/chrome/browser/chromeos...
chrome/browser/chromeos/login/kiosk_browsertest.cc:647:
set_test_crx_file(test_app_id() + ".crx");
Are these two lines necessary?

https://codereview.chromium.org/300843013/diff/270001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/extensions/chromeos/kiosk_apps_handler.h (right):

https://codereview.chromium.org/300843013/diff/270001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/chromeos/kiosk_apps_handler.h:64: void
HandleKioskDataFailure(const std::string& app_id);
nit: Handlexxx sounds like a JS callback. Let's use different names,
e.g.
HandleKioskDataChange -> UpdateApp
HandleKioskDataFailure -> ShowError

[jennyz, 2014-07-15 05:29:50]

asargent@, would you please review:
chrome/browser/extensions/external_provider_impl.cc
chrome/browser/extensions/external_provider_impl_chromeos_unittest.cc
chrome/browser/chromeos/extensions/external_cache.cc

xiyuan@ already reviewed and lgtmed, needs owners approval. Thanks!

[jennyz, 2014-07-15 16:36:41]

https://codereview.chromium.org/300843013/diff/270001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/startup_app_launcher.cc (right):

https://codereview.chromium.org/300843013/diff/270001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/startup_app_launcher.cc:307: 
On 2014/07/15 02:32:51, xiyuan wrote:
> nit: reset |wait_for_crx_update_|.

Done.

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

https://codereview.chromium.org/300843013/diff/270001/chrome/browser/chromeos...
chrome/browser/chromeos/login/kiosk_browsertest.cc:592:
set_test_crx_file(test_app_id() + ".crx");
On 2014/07/15 02:32:51, xiyuan wrote:
> Are these two lines necessary?
woops, removed.

https://codereview.chromium.org/300843013/diff/270001/chrome/browser/chromeos...
chrome/browser/chromeos/login/kiosk_browsertest.cc:647:
set_test_crx_file(test_app_id() + ".crx");
On 2014/07/15 02:32:51, xiyuan wrote:
> Are these two lines necessary?

Done.

https://codereview.chromium.org/300843013/diff/270001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/extensions/chromeos/kiosk_apps_handler.h (right):

https://codereview.chromium.org/300843013/diff/270001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/chromeos/kiosk_apps_handler.h:64: void
HandleKioskDataFailure(const std::string& app_id);
On 2014/07/15 02:32:51, xiyuan wrote:
> nit: Handlexxx sounds like a JS callback. Let's use different names,
> e.g.
> HandleKioskDataChange -> UpdateApp
> HandleKioskDataFailure -> ShowError

Done.

[jennyz, 2014-07-15 16:39:25]

zel, would you please review:
chrome/browser/extensions/external_provider_impl.cc
chrome/browser/extensions/external_provider_impl_chromeos_unittest.cc
chrome/browser/chromeos/extensions/external_cache.cc

xiyuan@ already reviewed and lgtmed, needs owners approval. Thanks!

[jennyz, 2014-07-15 17:27:13]

asargent@, zel thinks you should review these files.

[jennyz, 2014-07-15 18:34:24]

https://codereview.chromium.org/300843013/diff/270001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/kiosk_browsertest.cc (left):

https://codereview.chromium.org/300843013/diff/270001/chrome/browser/chromeos...
chrome/browser/chromeos/login/kiosk_browsertest.cc:918:
SetupAppProfile("chromeos/app_mode/offline_enabled_app_profile");
On 2014/07/15 02:32:51, xiyuan wrote:
> We can delete chrome/test/data/chromeos/app_mode/offline_enabled_app_profile
> now.

Done.

[asargent_no_longer_on_chrome, 2014-07-15 19:32:09]

chrome/browser/extensions lgtm

[jennyz, 2014-07-15 20:43:44]

The CQ bit was checked by jennyz@chromium.org

[commit-bot: I haz the power, 2014-07-15 20:45:01]

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

[commit-bot: I haz the power, 2014-07-16 05:27:41]

Change committed as 283357

[falken, 2014-07-16 08:11:21]

On 2014/07/16 05:27:41, I haz the power (commit-bot) wrote:
> Change committed as 283357

There are multiple flaky failures on chromium.webkit's Linux Chromium OS bots
that look related to this:
http://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Tes...
http://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Tes...
http://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Tes...

Excerpt from log:
../../chrome/browser/chromeos/login/kiosk_browsertest.cc:453: Failure
Value of: base::CopyFile(test_data_dir.Append(chrome::kPreferencesFilename),
app_profile_dir.Append(chrome::kPreferencesFilename))
  Actual: false
Expected: true

I don't see the failure on chromium waterfall yet, but I think it's just because
it's a flaky failure. E.g., these two builds had different results:
http://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Tes...
(failed)
http://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Tes...
(passed)

[xiyuan, 2014-07-16 16:37:24]

On 2014/07/16 08:11:21, falken wrote:
> On 2014/07/16 05:27:41, I haz the power (commit-bot) wrote:
> > Change committed as 283357
> 
> There are multiple flaky failures on chromium.webkit's Linux Chromium OS bots
> that look related to this:
>
http://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Tes...
>
http://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Tes...
>
http://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Tes...
> 
> Excerpt from log:
> ../../chrome/browser/chromeos/login/kiosk_browsertest.cc:453: Failure
> Value of: base::CopyFile(test_data_dir.Append(chrome::kPreferencesFilename),
> app_profile_dir.Append(chrome::kPreferencesFilename))
>   Actual: false
> Expected: true
> 
> I don't see the failure on chromium waterfall yet, but I think it's just
because
> it's a flaky failure. E.g., these two builds had different results:
>
http://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Tes...
> (failed)
>
http://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Tes...
> (passed)

This is strange. It looks like the bot does not get the correct revision. This
CL (r283357) deletes the failed line (kiosk_browsertest.cc:453). The failure
looks like the test data file deletion is picked up but not the test code
itself.

[jennyz, 2014-07-16 16:41:27]

Yes, xiyuan is right. The flaky failure is not caused by this cl (r283357).
Actually this cl removed the code(SetupAppProfile) which causes the
failure. The bot got revision 283345, which is earlier than the revision of
this cl.

Thanks,

Jenny


On Wed, Jul 16, 2014 at 9:37 AM, <xiyuan@chromium.org> wrote:

> On 2014/07/16 08:11:21, falken wrote:
>
>> On 2014/07/16 05:27:41, I haz the power (commit-bot) wrote:
>> > Change committed as 283357
>>
>
>  There are multiple flaky failures on chromium.webkit's Linux Chromium OS
>> bots
>> that look related to this:
>>
>
> http://build.chromium.org/p/chromium.webkit/builders/
> Linux%20ChromiumOS%20Tests%20%28dbg%29%281%29/builds/272
>
> http://build.chromium.org/p/chromium.webkit/builders/
> Linux%20ChromiumOS%20Tests%20%28dbg%29%282%29/builds/304
>
> http://build.chromium.org/p/chromium.webkit/builders/
> Linux%20ChromiumOS%20Tests%20%28dbg%29%283%29/builds/264
>
>  Excerpt from log:
>> ../../chrome/browser/chromeos/login/kiosk_browsertest.cc:453: Failure
>> Value of: base::CopyFile(test_data_dir.Append(chrome::
>> kPreferencesFilename),
>> app_profile_dir.Append(chrome::kPreferencesFilename))
>>    Actual: false
>> Expected: true
>>
>
>  I don't see the failure on chromium waterfall yet, but I think it's just
>>
> because
>
>> it's a flaky failure. E.g., these two builds had different results:
>>
>
> http://build.chromium.org/p/chromium.webkit/builders/
> Linux%20ChromiumOS%20Tests%20%28dbg%29%282%29/builds/304
>
>> (failed)
>>
>
> http://build.chromium.org/p/chromium.webkit/builders/
> Linux%20ChromiumOS%20Tests%20%28dbg%29%282%29/builds/305
>
>> (passed)
>>
>
> This is strange. It looks like the bot does not get the correct revision.
> This
> CL (r283357) deletes the failed line (kiosk_browsertest.cc:453). The
> failure
> looks like the test data file deletion is picked up but not the test code
> itself.
>
> https://codereview.chromium.org/300843013/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.