Update cached kiosk app crx from usb stick.

Update cached kiosk app crx from usb stick.

BUG=372857
Committed: https://crrev.com/ce634e9b901c485341182c546dd4d4c07bc59f5a
Cr-Commit-Position: refs/heads/master@{#292758}

[jennyz, 2014-08-21 22:33:20]

I only had one simple pass thru browser test for the successful usb stick update
case. I would like to have you to take a quick look at the code, to see if you
have any major concerns before I add more test cases. Thanks!

[xiyuan, 2014-08-23 18:11:58]

Rough thoughts of getting rid of ref-counting for the updater.
Let's sync up and discuss on Monday.

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

https://codereview.chromium.org/491403003/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:78: const char
KioskAppManager::kCrxUnpackDir[] = "kiosk/unpack";
We should probably use "/tmp"

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

https://codereview.chromium.org/491403003/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_app_update_service.cc:47: }
nit: drop {}

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

https://codereview.chromium.org/491403003/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_external_update_validator.h:22: : public
base::RefCountedThreadSafe<KioskExternalUpdateValidatorDelegate> {
KioskExternalUpdateValidatorDelegate probably should not be ref counted. We need
to have a way for it clear its reference in KioskExternalUpdateValidator when it
is gone.

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

https://codereview.chromium.org/491403003/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:92:
base::Bind(&KioskExternalUpdater::LoadExternalUpdateManifest,
We can break LoadExternalUpdateManifest up into two function. The first part can
be either a static member function or in anonymous namespace. It takes the
manifest json file path as input, runs on background thread to read the file and
returns a DictionaryValue. The 2nd part takes the DictionaryValue and generate
ExternalUpdate jobs. This is needed because KioskAppManager is not thread safe
and should only be accessed from UI thread.

And here we can use PostTaskAndReply and WeakPtr of the updater. Something like:

base::DictionaryValue* parsed_manifest = new base::DictionaryValue;
backend_task_runner_->PostTaskAndReply(
    FROM_HERE,
    base::Bind(<json file loading helper>, base::FilePath(...),
parsed_manifest),
    base::Bind(<parsed_manifest processing>,
              weak_ptr_factor_.GetWeakPtr(),
              base::Owned(parsed_manifest));

https://codereview.chromium.org/491403003/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:135: // Cleans upt he
temp_dir for unpacking.
nit: upt he -> up the

https://codereview.chromium.org/491403003/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:263: if
(!external_updates_.size()) {
nit: if (external_updates_.empty())

https://codereview.chromium.org/491403003/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:278:
scoped_refptr<KioskExternalUpdateValidator> crx_validator =
To not ref-counting updater, we should keep track of running validators (e.g. in
a std::vector<scoped_refptr<> >) and clear the reference when updater goes away.
And it is probably better to do the validation jobs one by one.

https://codereview.chromium.org/491403003/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:402: void
KioskExternalUpdater::ShowKioskUpdateProgressOnUI(
nit: OnUI is misleading. Other places uses this surfix to indicate the function
should be running on UI rather than post task to UI.

https://codereview.chromium.org/491403003/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:424: /// This should
be done on UI thread???
Yes, this has to be called on UI.

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

https://codereview.chromium.org/491403003/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_external_updater.h:43: ExternalUpdate();
ctor before members

https://codereview.chromium.org/491403003/diff/1/chrome/browser/chromeos/ui/k...
File chrome/browser/chromeos/ui/kiosk_external_update_notification.cc (right):

https://codereview.chromium.org/491403003/diff/1/chrome/browser/chromeos/ui/k...
chrome/browser/chromeos/ui/kiosk_external_update_notification.cc:43: Close();
Suggest to not call Close() here. It does not feel right because we should be
closing when the dtor is called. Instead, we should just inform owner that we
are going away. And Close() does not need to inform owner.

https://codereview.chromium.org/491403003/diff/1/chrome/browser/chromeos/ui/k...
chrome/browser/chromeos/ui/kiosk_external_update_notification.cc:64: virtual
void OnPaint(gfx::Canvas* canvas) OVERRIDE {
nit: // views::WidgetDelegateView:

[jennyz, 2014-08-27 00:58:42]

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

https://codereview.chromium.org/491403003/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:78: const char
KioskAppManager::kCrxUnpackDir[] = "kiosk/unpack";
On 2014/08/23 18:11:57, xiyuan wrote:
> We should probably use "/tmp"

Done.

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

https://codereview.chromium.org/491403003/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_app_update_service.cc:47: }
On 2014/08/23 18:11:57, xiyuan wrote:
> nit: drop {}

Done.

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

https://codereview.chromium.org/491403003/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_external_update_validator.h:22: : public
base::RefCountedThreadSafe<KioskExternalUpdateValidatorDelegate> {
On 2014/08/23 18:11:57, xiyuan wrote:
> KioskExternalUpdateValidatorDelegate probably should not be ref counted. We
need
> to have a way for it clear its reference in KioskExternalUpdateValidator when
it
> is gone.

Changed, KioskExternalUpdateValidator is no longer ref counted. It will be
passed to KioskExternalUpdateValidator as a WeakPtr.

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

https://codereview.chromium.org/491403003/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:135: // Cleans upt he
temp_dir for unpacking.
On 2014/08/23 18:11:57, xiyuan wrote:
> nit: upt he -> up the

Done.

https://codereview.chromium.org/491403003/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:263: if
(!external_updates_.size()) {
On 2014/08/23 18:11:57, xiyuan wrote:
> nit: if (external_updates_.empty())

Done.

https://codereview.chromium.org/491403003/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:278:
scoped_refptr<KioskExternalUpdateValidator> crx_validator =
On 2014/08/23 18:11:57, xiyuan wrote:
> To not ref-counting updater, we should keep track of running validators (e.g.
in
> a std::vector<scoped_refptr<> >) and clear the reference when updater goes
away.
> And it is probably better to do the validation jobs one by one.

KioskExternalUpdater is not ref counted, and the validation jobs are done one by
one now.

https://codereview.chromium.org/491403003/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:402: void
KioskExternalUpdater::ShowKioskUpdateProgressOnUI(
On 2014/08/23 18:11:57, xiyuan wrote:
> nit: OnUI is misleading. Other places uses this surfix to indicate the
function
> should be running on UI rather than post task to UI. 

Renamed.

https://codereview.chromium.org/491403003/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:424: /// This should
be done on UI thread???
On 2014/08/23 18:11:58, xiyuan wrote:
> Yes, this has to be called on UI.

Acknowledged.

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

https://codereview.chromium.org/491403003/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_external_updater.h:43: ExternalUpdate();
On 2014/08/23 18:11:58, xiyuan wrote:
> ctor before members

Done.

https://codereview.chromium.org/491403003/diff/1/chrome/browser/chromeos/ui/k...
File chrome/browser/chromeos/ui/kiosk_external_update_notification.cc (right):

https://codereview.chromium.org/491403003/diff/1/chrome/browser/chromeos/ui/k...
chrome/browser/chromeos/ui/kiosk_external_update_notification.cc:43: Close();
On 2014/08/23 18:11:58, xiyuan wrote:
> Suggest to not call Close() here. It does not feel right because we should be
> closing when the dtor is called. Instead, we should just inform owner that we
> are going away. And Close() does not need to inform owner.

Done.

https://codereview.chromium.org/491403003/diff/1/chrome/browser/chromeos/ui/k...
chrome/browser/chromeos/ui/kiosk_external_update_notification.cc:64: virtual
void OnPaint(gfx::Canvas* canvas) OVERRIDE {
On 2014/08/23 18:11:58, xiyuan wrote:
> nit: // views::WidgetDelegateView:

Done.

[xiyuan, 2014-08-27 16:17:04]

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

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_external_update_validator.cc:71:
LOG(ERROR) << "Can't find minimum browser version for app " << app_id_;
nit: minimum_browser_version.clear() to make sure we don't use it uninitialized.

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

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_external_update_validator.h:45:
base::WeakPtr<KioskExternalUpdateValidatorDelegate> delegate);
nit: use const reference, i.e.
const base::WeakPtr<KioskExternalUpdateValidatorDelegate>& delegate

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

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:63: void
CacheExternalCrx(const base::FilePath& external_file,
nit: Use a function name that reflects what it does better, e.g.
CopyCrxAndDeleteTemp

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:181: base::FilePath
new_cache_file = crx_cache_dir_.AppendASCII(filename);
We should not assume the filename in ExternalCache's LocalExtensionCache. We
should consider exposing the LocalExtensionCache and pass in ExternalCache
instance to th updater and call PutExtension instead.

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:259: if
(!base::PathExists(external_crx)) {
The code now runs on UI thread and file access is not allowed. This could be the
job of the validator.

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:266: LOG(ERROR) <<
"Can't find external version in manifest " << app_id;
We don't need to require the update manifest to have a version key. If it has
the version key, we can use it here to speed things up but it is okay to not
have it and we fallback to use the version inside crx.

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:271: switch
(cached_version.CompareTo(external_version)) {
nit: suggest to change to use "if", switch on int without default is not good.
i.e. 
if (cached_version.CompareTo(external_version) >= 0)
  continue;

Also, consider merging this with ShouldDoExternalUpdate. The code here
essentially the same as calling that with empty minimum_browser_version.

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:383: switch
(existing_version.CompareTo(external_version)) {
nit: change this to "if"

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:399: if
(min_browser_version.empty()) {
This should not be a requirement. Many apps do not have minimum browser version.

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:433:
content::BrowserThread::PostTask(
Why do we need to PostTask to UI when we are already on UI thread?

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:454:
content::BrowserThread::PostTask(
Do we need PostTask here?

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:482:
content::BrowserThread::PostTask(
Do we need PostTask here?

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:511: updated_apps =
updated_apps + base::UTF8ToUTF16(", ") + app_name;
nit: use base::ASCIIToUTF16 for this

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:515: failed_apps =
app_name + base::UTF8ToUTF16(": ") + it->second.error;
nit: ASCIIToUTF16

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:518:
base::UTF8ToUTF16(": ") + it->second.error;
nit: ASCIIToUTF16

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:530: message =
message + base::UTF8ToUTF16("\n") + success_app_msg;
nit: ASCIIToUTF16

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:538: message =
message + base::UTF8ToUTF16("\n") + failed_app_msg;
nit: ASCIIToUTF16

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/ui/kiosk_external_update_notification.cc (right):

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/ui/kiosk_external_update_notification.cc:58:
InformOwnerForDismiss();
This is not necessary. Close would cause this view class being destroyed and
causing InformOwnerForDismiss being called.

We don't need to do it here and having this could easily causing infinite
recursion.

[jennyz, 2014-08-28 22:27:44]

Patchset #5 (id:80001) has been deleted

[jennyz, 2014-08-28 22:37:25]

Patchset #5 (id:100001) has been deleted

[jennyz, 2014-08-28 22:41:07]

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

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_external_update_validator.cc:71:
LOG(ERROR) << "Can't find minimum browser version for app " << app_id_;
On 2014/08/27 16:17:02, xiyuan wrote:
> nit: minimum_browser_version.clear() to make sure we don't use it
uninitialized.

Done.

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

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_external_update_validator.h:45:
base::WeakPtr<KioskExternalUpdateValidatorDelegate> delegate);
On 2014/08/27 16:17:02, xiyuan wrote:
> nit: use const reference, i.e.
> const base::WeakPtr<KioskExternalUpdateValidatorDelegate>& delegate

Done.

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

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:63: void
CacheExternalCrx(const base::FilePath& external_file,
On 2014/08/27 16:17:03, xiyuan wrote:
> nit: Use a function name that reflects what it does better, e.g.
> CopyCrxAndDeleteTemp

Done.

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:181: base::FilePath
new_cache_file = crx_cache_dir_.AppendASCII(filename);
On 2014/08/27 16:17:03, xiyuan wrote:
> We should not assume the filename in ExternalCache's LocalExtensionCache. We
> should consider exposing the LocalExtensionCache and pass in ExternalCache
> instance to th updater and call PutExtension instead.

Changed to use LocalExtensionCache::PutExtension to install the updated crx file
into the local cache.

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:259: if
(!base::PathExists(external_crx)) {
On 2014/08/27 16:17:03, xiyuan wrote:
> The code now runs on UI thread and file access is not allowed. This could be
the
> job of the validator.

Yes, removed the check here, if the file does not exist, it should fail the
validator.

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:266: LOG(ERROR) <<
"Can't find external version in manifest " << app_id;
On 2014/08/27 16:17:03, xiyuan wrote:
> We don't need to require the update manifest to have a version key. If it has
> the version key, we can use it here to speed things up but it is okay to not
> have it and we fallback to use the version inside crx.

Done.

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:271: switch
(cached_version.CompareTo(external_version)) {
On 2014/08/27 16:17:03, xiyuan wrote:
> nit: suggest to change to use "if", switch on int without default is not good.
> i.e. 
> if (cached_version.CompareTo(external_version) >= 0)
>   continue;
> 
> Also, consider merging this with ShouldDoExternalUpdate. The code here
> essentially the same as calling that with empty minimum_browser_version.

Done.

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:383: switch
(existing_version.CompareTo(external_version)) {
On 2014/08/27 16:17:03, xiyuan wrote:
> nit: change this to "if"

Done.

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:399: if
(min_browser_version.empty()) {
On 2014/08/27 16:17:03, xiyuan wrote:
> This should not be a requirement. Many apps do not have minimum browser
version.

Done.

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:433:
content::BrowserThread::PostTask(
On 2014/08/27 16:17:02, xiyuan wrote:
> Why do we need to PostTask to UI when we are already on UI thread?

Removed the PostTask, making direct call now.

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:454:
content::BrowserThread::PostTask(
On 2014/08/27 16:17:03, xiyuan wrote:
> Do we need PostTask here?
No need to PostTask, making direct call.

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:482:
content::BrowserThread::PostTask(
On 2014/08/27 16:17:03, xiyuan wrote:
> Do we need PostTask here?

Making direct call now.

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:511: updated_apps =
updated_apps + base::UTF8ToUTF16(", ") + app_name;
On 2014/08/27 16:17:02, xiyuan wrote:
> nit: use base::ASCIIToUTF16 for this

Done.

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:515: failed_apps =
app_name + base::UTF8ToUTF16(": ") + it->second.error;
On 2014/08/27 16:17:02, xiyuan wrote:
> nit: ASCIIToUTF16

Done.

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:518:
base::UTF8ToUTF16(": ") + it->second.error;
On 2014/08/27 16:17:03, xiyuan wrote:
> nit: ASCIIToUTF16

Done.

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:530: message =
message + base::UTF8ToUTF16("\n") + success_app_msg;
On 2014/08/27 16:17:03, xiyuan wrote:
> nit: ASCIIToUTF16

Done.

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:538: message =
message + base::UTF8ToUTF16("\n") + failed_app_msg;
On 2014/08/27 16:17:03, xiyuan wrote:
> nit: ASCIIToUTF16

Done.

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/ui/kiosk_external_update_notification.cc (right):

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/ui/kiosk_external_update_notification.cc:58:
InformOwnerForDismiss();
On 2014/08/27 16:17:03, xiyuan wrote:
> This is not necessary. Close would cause this view class being destroyed and
> causing InformOwnerForDismiss being called.
> 
> We don't need to do it here and having this could easily causing infinite
> recursion.

In our case, both notification and view should go away whenever user dismisses
the notification. We don't really re-use them after they have been dismissed. So
I removed the code for view and notification to tell each other about one of
them going away. The client code simply deletes the notification when it wants
to dismiss the notification.

[xiyuan, 2014-08-29 02:44:18]

Mostly good.

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/ui/kiosk_external_update_notification.cc (right):

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/ui/kiosk_external_update_notification.cc:58:
InformOwnerForDismiss();
On 2014/08/28 22:41:07, jennyz wrote:
> In our case, both notification and view should go away whenever user dismisses
> the notification. We don't really re-use them after they have been dismissed.
So
> I removed the code for view and notification to tell each other about one of
> them going away. The client code simply deletes the notification when it wants
> to dismiss the notification.

InformOwnerForDismiss() code is necessary for handling edge cases. The app could
close its window while the update is in progress. In this case, the view could
be gone before the notification class and should be handled.

https://codereview.chromium.org/491403003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/kiosk_external_updater.cc (right):

https://codereview.chromium.org/491403003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:63: void
CopyExternalCrxAndDeleteTemp(const base::FilePath& external_file,
The arguments are tricky, e.g. |temp_file| and |temp_dir| looks alike. Please
add comments to document what this function does.

https://codereview.chromium.org/491403003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:200: base::FilePath
new_cache_file = crx_cache_dir_.AppendASCII(filename);
Nuke those two lines since no longer needed.

[jennyz, 2014-08-29 18:37:51]

Most changes are from Rebase, only one for this patch is in
kiosk_external_cache.cc and kiosk_external_update_notification.cc/h.

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/ui/kiosk_external_update_notification.cc (right):

https://codereview.chromium.org/491403003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/ui/kiosk_external_update_notification.cc:58:
InformOwnerForDismiss();
On 2014/08/29 02:44:17, xiyuan wrote:
> On 2014/08/28 22:41:07, jennyz wrote:
> > In our case, both notification and view should go away whenever user
dismisses
> > the notification. We don't really re-use them after they have been
dismissed.
> So
> > I removed the code for view and notification to tell each other about one of
> > them going away. The client code simply deletes the notification when it
wants
> > to dismiss the notification.
> 
> InformOwnerForDismiss() code is necessary for handling edge cases. The app
could
> close its window while the update is in progress. In this case, the view could
> be gone before the notification class and should be handled.
Add back InformOwnerForDismiss to handle this corner case.

https://codereview.chromium.org/491403003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/kiosk_external_updater.cc (right):

https://codereview.chromium.org/491403003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:63: void
CopyExternalCrxAndDeleteTemp(const base::FilePath& external_file,
On 2014/08/29 02:44:17, xiyuan wrote:
> The arguments are tricky, e.g. |temp_file| and |temp_dir| looks alike. Please
> add comments to document what this function does.

Done.

https://codereview.chromium.org/491403003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/kiosk_external_updater.cc:200: base::FilePath
new_cache_file = crx_cache_dir_.AppendASCII(filename);
On 2014/08/29 02:44:17, xiyuan wrote:
> Nuke those two lines since no longer needed.

Done.

[xiyuan, 2014-08-29 18:44:01]

lgtm

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

https://codereview.chromium.org/491403003/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/login/kiosk_browsertest.cc:1235:
IN_PROC_BROWSER_TEST_F(KioskUpdateTest, PRE_PRE_UsbStickUpdateAppNoNetwork) {
Can we use PreCacheAndLaunchApp in PRE_ test and get rid of this PRE_PRE?

[jennyz, 2014-08-29 20:53:07]

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

https://codereview.chromium.org/491403003/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/login/kiosk_browsertest.cc:1235:
IN_PROC_BROWSER_TEST_F(KioskUpdateTest, PRE_PRE_UsbStickUpdateAppNoNetwork) {
On 2014/08/29 18:44:00, xiyuan wrote:
> Can we use PreCacheAndLaunchApp in PRE_ test and get rid of this PRE_PRE?

Done.

[jennyz, 2014-08-29 20:53:30]

The CQ bit was checked by jennyz@chromium.org

[commit-bot: I haz the power, 2014-08-30 01:11:35]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-30 02:08:17]

Committed patchset #8 (id:180001) as b3de42774d1061768edb833d7dae5fac9b0722b1

[commit-bot: I haz the power, 2014-09-10 03:13:18]

Patchset 8 (id:??) landed as
https://crrev.com/ce634e9b901c485341182c546dd4d4c07bc59f5a
Cr-Commit-Position: refs/heads/master@{#292758}