Delete temporary application file as soon as the application is launched.

Delete temporary application file as soon as the application is launched.

This prevent the shell to fill the temporary directory with applications
that are never deleted because the applications never finish.

R=aa@chromium.org, eseidel@chromium.org
BUG=449003
Committed: https://chromium.googlesource.com/external/mojo/+/671001e79618e4e1445524af66356144444984fd

[eseidel, 2015-01-22 17:56:39]

eseidel@chromium.org changed reviewers:
+ viettrungluu@chromium.org

[eseidel, 2015-01-22 17:56:39]

In WebKit one of our styles was to require enums as arguments instead of bools. 
I'm not sure if Google follows the same, but it makes things like:

StartApp(app, false);
more readable as:
StartApp(app, DeleteAfterLaunch);

I'm not the right reviewer for this, but I support fixing this to actually work!
:)  I swear I tested this and saw the /tmp files gone...

[Aaron Boodman, 2015-01-22 19:23:40]

It's not required by Google style, but I totally agree and usually recommend to
rename such 'magic constants'. Only exception is cases like setFooEnabled(bool).

[Aaron Boodman, 2015-01-22 19:51:31]

Structural comment: I think a callback, similar to the existing one for app
completed, would be more generally useful. Perhaps we should do that instead.

Also, can you please undo eseidel's earlier patch since it is no longer needed
with your patch?

https://codereview.chromium.org/865253002/diff/20001/shell/dynamic_applicatio...
File shell/dynamic_application_loader.cc (right):

https://codereview.chromium.org/865253002/diff/20001/shell/dynamic_applicatio...
shell/dynamic_application_loader.cc:84: virtual bool CleanPath() = 0;
This doesn't appear to ever be answered based on runtime state. It only depends
on what the subclass is. In that case, it seems like the subclass should just
pass a constant to the superclass' constructor.

https://codereview.chromium.org/865253002/diff/20001/shell/dynamic_service_ru...
File shell/dynamic_service_runner.h (right):

https://codereview.chromium.org/865253002/diff/20001/shell/dynamic_service_ru...
shell/dynamic_service_runner.h:33: bool clean_app_path,
Recommend using an enum as suggested by eseidel for readability. Something like:
DeleteAppPath|DontDeleteAppPath.

[qsr, 2015-01-23 09:49:33]

On 2015/01/22 19:51:31, Aaron Boodman wrote:
> Structural comment: I think a callback, similar to the existing one for app
> completed, would be more generally useful. Perhaps we should do that instead.

 If you do not mind, I'd like not to do that now. If needed, we can go back and
change, but if not, this makes it quite more complex, as some of this as to be
made cross-process.

> Also, can you please undo eseidel's earlier patch since it is no longer needed
> with your patch?

 Will do.

>
https://codereview.chromium.org/865253002/diff/20001/shell/dynamic_applicatio...
> File shell/dynamic_application_loader.cc (right):
> 
>
https://codereview.chromium.org/865253002/diff/20001/shell/dynamic_applicatio...
> shell/dynamic_application_loader.cc:84: virtual bool CleanPath() = 0;
> This doesn't appear to ever be answered based on runtime state. It only
depends
> on what the subclass is. In that case, it seems like the subclass should just
> pass a constant to the superclass' constructor.
> 
>
https://codereview.chromium.org/865253002/diff/20001/shell/dynamic_service_ru...
> File shell/dynamic_service_runner.h (right):
> 
>
https://codereview.chromium.org/865253002/diff/20001/shell/dynamic_service_ru...
> shell/dynamic_service_runner.h:33: bool clean_app_path,
> Recommend using an enum as suggested by eseidel for readability. Something
like:
> DeleteAppPath|DontDeleteAppPath.

[qsr, 2015-01-26 09:12:14]

https://codereview.chromium.org/865253002/diff/20001/shell/dynamic_applicatio...
File shell/dynamic_application_loader.cc (right):

https://codereview.chromium.org/865253002/diff/20001/shell/dynamic_applicatio...
shell/dynamic_application_loader.cc:84: virtual bool CleanPath() = 0;
On 2015/01/22 19:51:30, Aaron Boodman wrote:
> This doesn't appear to ever be answered based on runtime state. It only
depends
> on what the subclass is. In that case, it seems like the subclass should just
> pass a constant to the superclass' constructor.

Done.

https://codereview.chromium.org/865253002/diff/20001/shell/dynamic_service_ru...
File shell/dynamic_service_runner.h (right):

https://codereview.chromium.org/865253002/diff/20001/shell/dynamic_service_ru...
shell/dynamic_service_runner.h:33: bool clean_app_path,
On 2015/01/22 19:51:31, Aaron Boodman wrote:
> Recommend using an enum as suggested by eseidel for readability. Something
like:
> DeleteAppPath|DontDeleteAppPath.

Done.

[qsr, 2015-01-27 09:06:49]

Aaron, gentle ping?

[Aaron Boodman, 2015-01-27 16:05:02]

LGTM w/ one comment.

https://codereview.chromium.org/865253002/diff/40001/shell/app_child_process....
File shell/app_child_process.mojom (right):

https://codereview.chromium.org/865253002/diff/40001/shell/app_child_process....
shell/app_child_process.mojom:11: uint8 cleanup_behavior,
Please change this to be either a bool or a mojom enum.

[qsr, 2015-01-28 12:44:31]

Committed patchset #3 (id:40001) manually as
671001e79618e4e1445524af66356144444984fd (presubmit successful).