Add APIs to protect critical tasks on iOS.

Add APIs to protect critical tasks on iOS.

On iOS, once an application goes into the background, it receives no CPU cycles until it is moved to foreground. At any time the OS may kill the application, but the application itself is never told or given any chance to clean up. Thus the time before going into the background is critical to ensure certain tasks have chance to complete. 

Luckily, an application may ask for a little more time when going into the background by calling certain methods that let the OS know it wants to perform some short-lived work in the background. This cl provides APIs to protect critical tasks in this case.

- A class is provided to mark the beginning and end of a critical task for iOS.

- A wrapper around a task is introduced that annotates that a class is "critical". Right now we have the wrapping code for iOS and it is just a no-op for other platforms.


BUG=NONE

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149422

[Chen Yu, 2012-07-30 14:17:19]

Starting with stuartmorgan for initial look. Thanks!

[stuartmorgan, 2012-07-31 07:58:15]

https://chromiumcodereview.appspot.com/10835032/diff/5005/base/base.gypi
File base/base.gypi (right):

https://chromiumcodereview.appspot.com/10835032/diff/5005/base/base.gypi#newc...
base/base.gypi:91: 'critical_closure_stub.cc',
There needs to be a line later that excludes this file on iOS. Did this actually
compile?

https://chromiumcodereview.appspot.com/10835032/diff/5005/base/critical_closu...
File base/critical_closure.h (right):

https://chromiumcodereview.appspot.com/10835032/diff/5005/base/critical_closu...
base/critical_closure.h:11: // background if possible, otherwise the original
task is returned.
s/background/background (on platforms where applications don't execute while
backgrounded)/

[Chen Yu, 2012-07-31 10:38:50]

Thanks for the review!

I also move MakeCriticalClosure() to namespace base. PTAL.

https://chromiumcodereview.appspot.com/10835032/diff/5005/base/base.gypi
File base/base.gypi (right):

https://chromiumcodereview.appspot.com/10835032/diff/5005/base/base.gypi#newc...
base/base.gypi:91: 'critical_closure_stub.cc',
Yes, somehow it does compile (I build target 'base' and 'base_unittests'; I did
a test by making a function call to MakeCriticalClosure() and it is the iOS
version that is called; I also tried by adding another implementation file and
still pass the compilation. Not clear how compiler handles it)

I excluded the _stub.cc file for iOS. Thanks!

On 2012/07/31 07:58:15, stuartmorgan wrote:
> There needs to be a line later that excludes this file on iOS. Did this
actually
> compile?

[stuartmorgan, 2012-07-31 10:55:13]

LGTM; over to Mark.

[Mark Mentovai, 2012-07-31 23:16:33]

https://chromiumcodereview.appspot.com/10835032/diff/1002/base/critical_closu...
File base/critical_closure.h (right):

https://chromiumcodereview.appspot.com/10835032/diff/1002/base/critical_closu...
base/critical_closure.h:12: // Returns a closure that is protected when the
application goes to the
You don’t say what “protected” means, but it’s totally unclear so you should.

https://chromiumcodereview.appspot.com/10835032/diff/1002/base/critical_closu...
File base/critical_closure_stub.cc (right):

https://chromiumcodereview.appspot.com/10835032/diff/1002/base/critical_closu...
base/critical_closure_stub.cc:9: base::Closure MakeCriticalClosure(const
base::Closure& closure) {
You should get rid of this stub file and write this code inline in the header
for non-iOS. There should be to be zero code size impact on other platforms.

https://chromiumcodereview.appspot.com/10835032/diff/1002/base/ios/scoped_cri...
File base/ios/scoped_critical_action.h (right):

https://chromiumcodereview.appspot.com/10835032/diff/1002/base/ios/scoped_cri...
base/ios/scoped_critical_action.h:32: unsigned int background_task_id_;
What does this do? Comment it.

https://chromiumcodereview.appspot.com/10835032/diff/1002/base/ios/scoped_cri...
base/ios/scoped_critical_action.h:33: Lock id_lock_;
What does this protect? Comment it. If it protects background_task_id_, it
should be named background_task_id_lock_ to be self-documenting (and in that
case, you need not provide a comment unless there’s something non-obvious about
the way the lock is used.)

https://chromiumcodereview.appspot.com/10835032/diff/1002/base/ios/scoped_cri...
File base/ios/scoped_critical_action.mm (right):

https://chromiumcodereview.appspot.com/10835032/diff/1002/base/ios/scoped_cri...
base/ios/scoped_critical_action.mm:10: #import <UIKit/UIKit.h>
C system includes before project includes, with the exception of this file’s
associated header (scoped_critical_action.h), which should be first.

You got the order right in critical_closure_ios.mm.

https://chromiumcodereview.appspot.com/10835032/diff/1002/base/ios/scoped_cri...
base/ios/scoped_critical_action.mm:19: background_task_id_ = [[UIApplication
sharedApplication]
This function returns a UIBackgroundTaskIdentifier, so why did you make
background_task_id_ an unsigned int? It should be a UIBackgroundTaskIdentifier.
If you need to #import <UIKit/UIKit.h> in the .h to get the type declaration,
that’s fine.

https://chromiumcodereview.appspot.com/10835032/diff/1002/base/ios/scoped_cri...
base/ios/scoped_critical_action.mm:27: VLOG(3) << "Beginning background task
with id " << background_task_id_;
The docs say something about UIBackgroundTaskInvalid being returned here. Do you
want to handle that?

[stuartmorgan, 2012-08-01 04:31:55]

https://chromiumcodereview.appspot.com/10835032/diff/1002/base/critical_closu...
File base/critical_closure_stub.cc (right):

https://chromiumcodereview.appspot.com/10835032/diff/1002/base/critical_closu...
base/critical_closure_stub.cc:9: base::Closure MakeCriticalClosure(const
base::Closure& closure) {
On 2012/07/31 23:16:33, Mark Mentovai wrote:
> You should get rid of this stub file and write this code inline in the header
> for non-iOS. There should be to be zero code size impact on other platforms.

Okay; we did it this way originally because I thought our style guide made
non-header declarations preferred even for cases like this. I guess this falls
under the "simple getter" case though (in a somewhat non-obvious way).

https://chromiumcodereview.appspot.com/10835032/diff/1002/base/ios/scoped_cri...
File base/ios/scoped_critical_action.mm (right):

https://chromiumcodereview.appspot.com/10835032/diff/1002/base/ios/scoped_cri...
base/ios/scoped_critical_action.mm:19: background_task_id_ = [[UIApplication
sharedApplication]
On 2012/07/31 23:16:33, Mark Mentovai wrote:
> If you need to #import <UIKit/UIKit.h> in the .h to get the type declaration,
> that’s fine.

Except it isn't, which is why we did this. It makes it impossible to use
ScopedCriticalAction in .cc files (which is almost always where we end up
needing to use it)

[Chen Yu, 2012-08-01 11:26:13]

Thanks for the review! PTAL.

https://chromiumcodereview.appspot.com/10835032/diff/1002/base/critical_closu...
File base/critical_closure.h (right):

https://chromiumcodereview.appspot.com/10835032/diff/1002/base/critical_closu...
base/critical_closure.h:12: // Returns a closure that is protected when the
application goes to the
On 2012/07/31 23:16:33, Mark Mentovai wrote:
> You don’t say what “protected” means, but it’s totally unclear so you should.

Done.

https://chromiumcodereview.appspot.com/10835032/diff/1002/base/critical_closu...
File base/critical_closure_stub.cc (right):

https://chromiumcodereview.appspot.com/10835032/diff/1002/base/critical_closu...
base/critical_closure_stub.cc:9: base::Closure MakeCriticalClosure(const
base::Closure& closure) {
On 2012/08/01 04:31:55, stuartmorgan wrote:
> On 2012/07/31 23:16:33, Mark Mentovai wrote:
> > You should get rid of this stub file and write this code inline in the
header
> > for non-iOS. There should be to be zero code size impact on other platforms.
> 
> Okay; we did it this way originally because I thought our style guide made
> non-header declarations preferred even for cases like this. I guess this falls
> under the "simple getter" case though (in a somewhat non-obvious way).

Done.

https://chromiumcodereview.appspot.com/10835032/diff/1002/base/ios/scoped_cri...
File base/ios/scoped_critical_action.h (right):

https://chromiumcodereview.appspot.com/10835032/diff/1002/base/ios/scoped_cri...
base/ios/scoped_critical_action.h:32: unsigned int background_task_id_;
On 2012/07/31 23:16:33, Mark Mentovai wrote:
> What does this do? Comment it.

Done.

https://chromiumcodereview.appspot.com/10835032/diff/1002/base/ios/scoped_cri...
base/ios/scoped_critical_action.h:33: Lock id_lock_;
On 2012/07/31 23:16:33, Mark Mentovai wrote:
> What does this protect? Comment it. If it protects background_task_id_, it
> should be named background_task_id_lock_ to be self-documenting (and in that
> case, you need not provide a comment unless there’s something non-obvious
about
> the way the lock is used.)

Done.

https://chromiumcodereview.appspot.com/10835032/diff/1002/base/ios/scoped_cri...
File base/ios/scoped_critical_action.mm (right):

https://chromiumcodereview.appspot.com/10835032/diff/1002/base/ios/scoped_cri...
base/ios/scoped_critical_action.mm:10: #import <UIKit/UIKit.h>
On 2012/07/31 23:16:33, Mark Mentovai wrote:
> C system includes before project includes, with the exception of this file’s
> associated header (scoped_critical_action.h), which should be first.
> 
> You got the order right in critical_closure_ios.mm.

Done.

https://chromiumcodereview.appspot.com/10835032/diff/1002/base/ios/scoped_cri...
base/ios/scoped_critical_action.mm:27: VLOG(3) << "Beginning background task
with id " << background_task_id_;
There is no much we can do in this case; a warning in logged here, also made
some changes in |EndBackgroundTask()|.
On 2012/07/31 23:16:33, Mark Mentovai wrote:
> The docs say something about UIBackgroundTaskInvalid being returned here. Do
you
> want to handle that?

[stuartmorgan, 2012-08-01 12:03:17]

https://chromiumcodereview.appspot.com/10835032/diff/7005/base/ios/scoped_cri...
File base/ios/scoped_critical_action.mm (right):

https://chromiumcodereview.appspot.com/10835032/diff/7005/base/ios/scoped_cri...
base/ios/scoped_critical_action.mm:7: #import <UIKit/UIKit.h>
Blank line after this.

https://chromiumcodereview.appspot.com/10835032/diff/7005/base/ios/scoped_cri...
base/ios/scoped_critical_action.mm:27: DLOG(WARNING) << "Running in the
background is not possible.";
Be specific; "beginBackgroundTaskWithExpirationHandler: returned an invalid ID"

[Chen Yu, 2012-08-01 12:15:27]

https://chromiumcodereview.appspot.com/10835032/diff/7005/base/ios/scoped_cri...
File base/ios/scoped_critical_action.mm (right):

https://chromiumcodereview.appspot.com/10835032/diff/7005/base/ios/scoped_cri...
base/ios/scoped_critical_action.mm:7: #import <UIKit/UIKit.h>
On 2012/08/01 12:03:17, stuartmorgan wrote:
> Blank line after this.

Done.

https://chromiumcodereview.appspot.com/10835032/diff/7005/base/ios/scoped_cri...
base/ios/scoped_critical_action.mm:27: DLOG(WARNING) << "Running in the
background is not possible.";
On 2012/08/01 12:03:17, stuartmorgan wrote:
> Be specific; "beginBackgroundTaskWithExpirationHandler: returned an invalid
ID"

Done.

[Mark Mentovai, 2012-08-01 13:50:41]

LGTM

https://chromiumcodereview.appspot.com/10835032/diff/1002/base/ios/scoped_cri...
File base/ios/scoped_critical_action.mm (right):

https://chromiumcodereview.appspot.com/10835032/diff/1002/base/ios/scoped_cri...
base/ios/scoped_critical_action.mm:19: background_task_id_ = [[UIApplication
sharedApplication]
stuartmorgan wrote:
> On 2012/07/31 23:16:33, Mark Mentovai wrote:
> > If you need to #import <UIKit/UIKit.h> in the .h to get the type
declaration,
> > that’s fine.
> 
> Except it isn't, which is why we did this. It makes it impossible to use
> ScopedCriticalAction in .cc files (which is almost always where we end up
> needing to use it)

Then the use of the unexpected type requires a comment (and if it were used in
more than one spot, a typedef would be helpful too).

https://chromiumcodereview.appspot.com/10835032/diff/16002/base/ios/scoped_cr...
File base/ios/scoped_critical_action.h (right):

https://chromiumcodereview.appspot.com/10835032/diff/16002/base/ios/scoped_cr...
base/ios/scoped_critical_action.h:34: // |UIBackgroundTaskIdentifier| returned
by
Blank line before this, so EndBackgroundTask() appears as its own thing and
doesn’t get lost.

https://chromiumcodereview.appspot.com/10835032/diff/16002/base/ios/scoped_cr...
base/ios/scoped_critical_action.h:37: // of a |UIBackgroundTaskIdentifier| so
this class can be used in .cc files.
This comment suffices.

[commit-bot: I haz the power, 2012-08-01 14:00:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/chenyu@chromium.org/10835032/13005

[commit-bot: I haz the power, 2012-08-01 15:42:17]

Change committed as 149422