Create Memory Helper for memory warnings

Create Memory Helper for memory warnings

This CL creates a memory helper to handle the warnings about memory usage.

BUG=none

Committed: https://crrev.com/fae8266035e5e85404fb08093f94e207e6c93cda
Cr-Commit-Position: refs/heads/master@{#394745}

[gambard, 2016-05-18 13:38:41]

gambard@chromium.org changed reviewers:
+ marq@chromium.org

[marq (ping after 24h), 2016-05-18 14:05:22]

marq@chromium.org changed reviewers:
+ eugenebut@chromium.org

[marq (ping after 24h), 2016-05-18 14:05:23]

LGTM, +eugenebut@

[gambard, 2016-05-18 15:22:32]

The CQ bit was checked by gambard@chromium.org

[commit-bot: I haz the power, 2016-05-18 15:22:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1989873002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1989873002/20001

[commit-bot: I haz the power, 2016-05-18 15:33:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-18 15:33:32]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[gambard, 2016-05-18 15:38:44]

gambard@chromium.org changed reviewers:
+ sdefresne@chromium.org

[gambard, 2016-05-18 15:38:44]

+sdefresne for owner approval.

[Eugene But (OOO till 7-30), 2016-05-18 15:55:09]

lgtm

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
File ios/chrome/app/application_delegate/memory_recovery_helper.h (right):

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
ios/chrome/app/application_delegate/memory_recovery_helper.h:8: #import
<UIKit/UIKit.h>
NIT: s/UIKit/Foundation there is nothing UI related in this header.

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
ios/chrome/app/application_delegate/memory_recovery_helper.h:11: @interface
MemoryRecoveryHelper : NSObject
Should we be more specific with the name to reflect that this class is
responsible for memory warnings handling? F.e. MemoryWarningsHandler :)

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
File ios/chrome/app/application_delegate/memory_recovery_helper.mm (right):

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
ios/chrome/app/application_delegate/memory_recovery_helper.mm:28: -
(void)resetOutOfMemoryFlag;
s/resetOutOfMemoryFlag/resetOutOfMemoryFlagIfNecessary because this method does
not always resets the flag.

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
ios/chrome/app/application_delegate/memory_recovery_helper.mm:46: [self
performSelector:@selector(resetOutOfMemoryFlag)
Optional NIT: Consider using dispatch_after, which is more type safe pattern
than target action.

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
ios/chrome/app/application_delegate/memory_recovery_helper.mm:51:
breakpad_helper::SetMemoryWarningInProgress(YES);
s/YES/true

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
ios/chrome/app/application_delegate/memory_recovery_helper.mm:59:
breakpad_helper::SetMemoryWarningInProgress(NO);
s/NO/false

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
File ios/chrome/app/application_delegate/memory_recovery_helper_unittest.mm
(right):

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
ios/chrome/app/application_delegate/memory_recovery_helper_unittest.mm:5:
#import "base/mac/bind_objc_block.h"
s/import/include

https://engdoc.corp.google.com/eng/doc/objcguide.xml?showone=Order_of_Include...

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
ios/chrome/app/application_delegate/memory_recovery_helper_unittest.mm:6:
#include "base/mac/scoped_nsobject.h"
s/include/import

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
ios/chrome/app/application_delegate/memory_recovery_helper_unittest.mm:10:
#import "ios/chrome/app/application_delegate/memory_recovery_helper.h"
This import should go first, then libraries, then the rest of includes.
https://engdoc.corp.google.com/eng/doc/objcguide.xml?showone=Order_of_Include...

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
ios/chrome/app/application_delegate/memory_recovery_helper_unittest.mm:13:
namespace {
NIT: There is no need to put tests into anonymous namespace (there was a related
discussion on Chromium-dev and if you want I can find it). However I do
understand that most of our tests actually live in an anonymous namespace.

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
ios/chrome/app/application_delegate/memory_recovery_helper_unittest.mm:31: if
(!memory_helper_.get()) {
Drop |.get()|

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
ios/chrome/app/application_delegate/memory_recovery_helper_unittest.mm:56: //
foregroundMemoryWarningCount is setted to 0
Optional NIT: finish the comments with full stop

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
ios/chrome/app/application_delegate/memory_recovery_helper_unittest.mm:61: int
previousForegroundMemoryWarningCount =
NIT: Drop this variable and just use EXPECT_TRUE (or even better ASSERT_TRUE,
because it is a precondition):

EXPECT_TRUE(memoryHelper.foregroundMemoryWarningCount != 0);

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
ios/chrome/app/application_delegate/memory_recovery_helper_unittest.mm:75:
MemoryRecoveryHelper* memoryHelper = GetMemoryHelper();
Optional NIT: Drop this variable

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
ios/chrome/app/application_delegate/memory_recovery_helper_unittest.mm:86:
MemoryRecoveryHelper* memoryHelper = GetMemoryHelper();
Drop this variable, it is used only once and GetMemoryHelper() used many times.

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
ios/chrome/app/application_delegate/memory_recovery_helper_unittest.mm:93:
boolForKey:previous_session_info_constants::
Optional NIT: Could you please add using
previous_session_info_constants::kDidSeeMemoryWarningShortlyBeforeTerminating to
make this more readable.

[sdefresne, 2016-05-18 18:32:39]

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
File ios/chrome/app/application_delegate/memory_recovery_helper.h (right):

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
ios/chrome/app/application_delegate/memory_recovery_helper.h:10: // Helper for
handling memory warnings
Please use full sentences for your comments (i.e. need a full stop at end) here
and everywhere.

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
ios/chrome/app/application_delegate/memory_recovery_helper.h:15:
@property(nonatomic, readonly) int foregroundMemoryWarningCount;
Maybe use NSInteger instead of int here?

[gambard, 2016-05-19 09:13:26]

Patchset #3 (id:40001) has been deleted

[gambard, 2016-05-19 09:35:04]

PTAL sdefresne

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
File ios/chrome/app/application_delegate/memory_recovery_helper.h (right):

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
ios/chrome/app/application_delegate/memory_recovery_helper.h:8: #import
<UIKit/UIKit.h>
On 2016/05/18 15:55:07, Eugene But wrote:
> NIT: s/UIKit/Foundation there is nothing UI related in this header.

Done.

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
ios/chrome/app/application_delegate/memory_recovery_helper.h:10: // Helper for
handling memory warnings
On 2016/05/18 18:32:38, sdefresne wrote:
> Please use full sentences for your comments (i.e. need a full stop at end)
here
> and everywhere.

Done.

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
ios/chrome/app/application_delegate/memory_recovery_helper.h:11: @interface
MemoryRecoveryHelper : NSObject
On 2016/05/18 15:55:07, Eugene But wrote:
> Should we be more specific with the name to reflect that this class is
> responsible for memory warnings handling? F.e. MemoryWarningsHandler :)

Done.

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
ios/chrome/app/application_delegate/memory_recovery_helper.h:15:
@property(nonatomic, readonly) int foregroundMemoryWarningCount;
On 2016/05/18 18:32:38, sdefresne wrote:
> Maybe use NSInteger instead of int here?

Done.

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
File ios/chrome/app/application_delegate/memory_recovery_helper.mm (right):

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
ios/chrome/app/application_delegate/memory_recovery_helper.mm:28: -
(void)resetOutOfMemoryFlag;
On 2016/05/18 15:55:07, Eugene But wrote:
> s/resetOutOfMemoryFlag/resetOutOfMemoryFlagIfNecessary because this method
does
> not always resets the flag.

Done.

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
ios/chrome/app/application_delegate/memory_recovery_helper.mm:46: [self
performSelector:@selector(resetOutOfMemoryFlag)
On 2016/05/18 15:55:07, Eugene But wrote:
> Optional NIT: Consider using dispatch_after, which is more type safe pattern
> than target action.

Done.

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
ios/chrome/app/application_delegate/memory_recovery_helper.mm:51:
breakpad_helper::SetMemoryWarningInProgress(YES);
On 2016/05/18 15:55:07, Eugene But wrote:
> s/YES/true

Done.

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
ios/chrome/app/application_delegate/memory_recovery_helper.mm:59:
breakpad_helper::SetMemoryWarningInProgress(NO);
On 2016/05/18 15:55:07, Eugene But wrote:
> s/NO/false

Done.

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
File ios/chrome/app/application_delegate/memory_recovery_helper_unittest.mm
(right):

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
ios/chrome/app/application_delegate/memory_recovery_helper_unittest.mm:5:
#import "base/mac/bind_objc_block.h"
On 2016/05/18 15:55:08, Eugene But wrote:
> s/import/include
> 
>
https://engdoc.corp.google.com/eng/doc/objcguide.xml?showone=Order_of_Include...

Done.

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
ios/chrome/app/application_delegate/memory_recovery_helper_unittest.mm:6:
#include "base/mac/scoped_nsobject.h"
On 2016/05/18 15:55:09, Eugene But wrote:
> s/include/import

Done.

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
ios/chrome/app/application_delegate/memory_recovery_helper_unittest.mm:10:
#import "ios/chrome/app/application_delegate/memory_recovery_helper.h"
On 2016/05/18 15:55:09, Eugene But wrote:
> This import should go first, then libraries, then the rest of includes.
>
https://engdoc.corp.google.com/eng/doc/objcguide.xml?showone=Order_of_Include...

Done.

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
ios/chrome/app/application_delegate/memory_recovery_helper_unittest.mm:13:
namespace {
On 2016/05/18 15:55:08, Eugene But wrote:
> NIT: There is no need to put tests into anonymous namespace (there was a
related
> discussion on Chromium-dev and if you want I can find it). However I do
> understand that most of our tests actually live in an anonymous namespace. 

Done.

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
ios/chrome/app/application_delegate/memory_recovery_helper_unittest.mm:31: if
(!memory_helper_.get()) {
On 2016/05/18 15:55:09, Eugene But wrote:
> Drop |.get()|

Done.

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
ios/chrome/app/application_delegate/memory_recovery_helper_unittest.mm:56: //
foregroundMemoryWarningCount is setted to 0
On 2016/05/18 15:55:07, Eugene But wrote:
> Optional NIT: finish the comments with full stop

Done.

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
ios/chrome/app/application_delegate/memory_recovery_helper_unittest.mm:61: int
previousForegroundMemoryWarningCount =
On 2016/05/18 15:55:09, Eugene But wrote:
> NIT: Drop this variable and just use EXPECT_TRUE (or even better ASSERT_TRUE,
> because it is a precondition):
> 
> EXPECT_TRUE(memoryHelper.foregroundMemoryWarningCount != 0);

Done.

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
ios/chrome/app/application_delegate/memory_recovery_helper_unittest.mm:75:
MemoryRecoveryHelper* memoryHelper = GetMemoryHelper();
On 2016/05/18 15:55:09, Eugene But wrote:
> Optional NIT: Drop this variable

Done.

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
ios/chrome/app/application_delegate/memory_recovery_helper_unittest.mm:86:
MemoryRecoveryHelper* memoryHelper = GetMemoryHelper();
On 2016/05/18 15:55:09, Eugene But wrote:
> Drop this variable, it is used only once and GetMemoryHelper() used many
times.

Done.

https://codereview.chromium.org/1989873002/diff/20001/ios/chrome/app/applicat...
ios/chrome/app/application_delegate/memory_recovery_helper_unittest.mm:93:
boolForKey:previous_session_info_constants::
On 2016/05/18 15:55:07, Eugene But wrote:
> Optional NIT: Could you please add using
> previous_session_info_constants::kDidSeeMemoryWarningShortlyBeforeTerminating
to
> make this more readable.

Done.

[sdefresne, 2016-05-19 10:09:34]

Can you add an OWNERS file in that new directory? Should just have
marq@chromium.org (since you are not a committer yet).

https://codereview.chromium.org/1989873002/diff/100001/ios/chrome/app/applica...
File ios/chrome/app/application_delegate/memory_warning_helper.mm (right):

https://codereview.chromium.org/1989873002/diff/100001/ios/chrome/app/applica...
ios/chrome/app/application_delegate/memory_warning_helper.mm:51: // Add
information to breakpad.
nit: I would move this before the "dispatch_after" as creating a block does
allocate a tiny bit of memory and could cause the application to be killed at
that point.

[sdefresne, 2016-05-19 10:09:45]

lgtm once the OWNERS file is there.

[gambard, 2016-05-19 10:47:58]

The CQ bit was checked by gambard@chromium.org

[gambard, 2016-05-19 10:47:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org, marq@chromium.org, sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/1989873002/#ps110001
(title: "Add OWNERS")

[commit-bot: I haz the power, 2016-05-19 10:48:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1989873002/110001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1989873002/110001

[commit-bot: I haz the power, 2016-05-19 12:06:31]

Committed patchset #6 (id:110001)

[commit-bot: I haz the power, 2016-05-19 12:08:20]

Description was changed from

==========
Create Memory Helper for memory warnings

This CL creates a memory helper to handle the warnings about memory usage.

BUG=none
==========

to

==========
Create Memory Helper for memory warnings

This CL creates a memory helper to handle the warnings about memory usage.

BUG=none

Committed: https://crrev.com/fae8266035e5e85404fb08093f94e207e6c93cda
Cr-Commit-Position: refs/heads/master@{#394745}
==========

[commit-bot: I haz the power, 2016-05-19 12:08:21]

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