[ios] Cleanup SessionServiceIOS implementation.

[ios] Cleanup SessionServiceIOS implementation.

Before refactoring of SessionServiceIOS to make it easier to unit
test and to share with the new architecture, clean up legacy code.

-   Rename the files session_service{ => _ios}.{h,mm} to follow
    the convention of naming the files after the class defined.

-   Remove custom NSKeyedUnarchiver subclass SessionWindowUnarchiver
    as the additional property browserState is no longer and use a
    category of NSKeyedUnarchiver to define the method to register
    the aliases to load legacy saved session files.

-   Convert the code to ARC and merge "unit_tests_non_arc" target
    with "unit_tests" target as they both use ARC.

-   Reformat logged error messages, ensure that they include the
    path of the file or directory they apply to.

-   Use base::MakeCriticalClosure to prevent Chrome from suspending
    instead of using -beginBackgroundTaskWithExpirationHandler (the
    CriticalClosure code use the same functionality, but is shared
    code).

-   Remove SessionWindowIOS -clearSession as the method is no longer
    useful (obsolete).

-   Use lightweight generics and remove casts.

-   Remove obsolete #includes.

BUG=None

Review-Url: https://codereview.chromium.org/2802763002
Cr-Commit-Position: refs/heads/master@{#462527}
Committed: https://chromium.googlesource.com/chromium/src/+/da974c8881efb5c372206a5ac86ad807c0dbd931

[sdefresne, 2017-04-05 16:51:01]

The CQ bit was checked by sdefresne@chromium.org to run a CQ dry run

[sdefresne, 2017-04-05 16:51:28]

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

[sdefresne, 2017-04-05 16:51:28]

Please take a look.

[commit-bot: I haz the power, 2017-04-05 16:51:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 18:21:57]

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

[commit-bot: I haz the power, 2017-04-05 18:21:58]

Dry run: This issue passed the CQ dry run.

[marq (ping after 24h), 2017-04-06 11:58:16]

https://codereview.chromium.org/2802763002/diff/1/ios/chrome/browser/sessions...
File ios/chrome/browser/sessions/session_service_ios.mm (left):

https://codereview.chromium.org/2802763002/diff/1/ios/chrome/browser/sessions...
ios/chrome/browser/sessions/session_service_ios.mm:141:
UIBackgroundTaskIdentifier identifier = [[UIApplication sharedApplication]
Was this code just not doing anything useful? I'm not clear on the interaction
with TaskRunner::PostTask().

https://codereview.chromium.org/2802763002/diff/1/ios/chrome/browser/sessions...
File ios/chrome/browser/sessions/session_service_ios.mm (right):

https://codereview.chromium.org/2802763002/diff/1/ios/chrome/browser/sessions...
ios/chrome/browser/sessions/session_service_ios.mm:169: NSDictionary*
attributeDict =
Since you're here, make this 

NSDictionary<NSFileAttributeKey, id>* attributeDict = @{ NSFileProtectionKey :
NSFileProtectionComplete};

https://codereview.chromium.org/2802763002/diff/1/ios/chrome/browser/sessions...
ios/chrome/browser/sessions/session_service_ios.mm:223: return [unarchiver
decodeObjectForKey:@"root"];
Extract @"root" into a constant?

https://codereview.chromium.org/2802763002/diff/1/ios/chrome/browser/sessions...
ios/chrome/browser/sessions/session_service_ios.mm:237: FROM_HERE,
base::BindBlockArc(^{
Should this use criticalClosure()?

[sdefresne, 2017-04-06 14:38:52]

The CQ bit was checked by sdefresne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 14:39:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sdefresne, 2017-04-06 14:57:02]

Please take another look.

https://codereview.chromium.org/2802763002/diff/1/ios/chrome/browser/sessions...
File ios/chrome/browser/sessions/session_service_ios.mm (left):

https://codereview.chromium.org/2802763002/diff/1/ios/chrome/browser/sessions...
ios/chrome/browser/sessions/session_service_ios.mm:141:
UIBackgroundTaskIdentifier identifier = [[UIApplication sharedApplication]
On 2017/04/06 11:58:16, marq wrote:
> Was this code just not doing anything useful? I'm not clear on the interaction
> with TaskRunner::PostTask().

This is replaced by CriticalClosure (look in scoped_critical_action.mm, you'll
see that there is the same code used to implement critical closure).

https://codereview.chromium.org/2802763002/diff/1/ios/chrome/browser/sessions...
File ios/chrome/browser/sessions/session_service_ios.mm (right):

https://codereview.chromium.org/2802763002/diff/1/ios/chrome/browser/sessions...
ios/chrome/browser/sessions/session_service_ios.mm:169: NSDictionary*
attributeDict =
On 2017/04/06 11:58:16, marq wrote:
> Since you're here, make this 
> 
> NSDictionary<NSFileAttributeKey, id>* attributeDict = @{ NSFileProtectionKey :
> NSFileProtectionComplete};

Inlined in the method call.

https://codereview.chromium.org/2802763002/diff/1/ios/chrome/browser/sessions...
ios/chrome/browser/sessions/session_service_ios.mm:223: return [unarchiver
decodeObjectForKey:@"root"];
On 2017/04/06 11:58:16, marq wrote:
> Extract @"root" into a constant?

Done.

https://codereview.chromium.org/2802763002/diff/1/ios/chrome/browser/sessions...
ios/chrome/browser/sessions/session_service_ios.mm:237: FROM_HERE,
base::BindBlockArc(^{
On 2017/04/06 11:58:16, marq wrote:
> Should this use criticalClosure()?

Not sure. This is called from the startup code to delete the session file after
loading it when restoring the app after a crash. CriticalClosure prevent the
application from shutting down until the closure execute.

So, for this block to not be executed, the app would have to shutdown just after
startup. However, in that case, the user may not even have seen the "Chrome did
not terminate correctly, do you want to restore?" infobar.

This would also introduce a functional change in what is a refactoring/cleanup
CL, so, even if I make the change, I think it should be in a separate CL.

[commit-bot: I haz the power, 2017-04-06 15:49:05]

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

[commit-bot: I haz the power, 2017-04-06 15:49:06]

Dry run: This issue passed the CQ dry run.

[marq (ping after 24h), 2017-04-06 16:15:57]

lgtm

https://codereview.chromium.org/2802763002/diff/1/ios/chrome/browser/sessions...
File ios/chrome/browser/sessions/session_service_ios.mm (right):

https://codereview.chromium.org/2802763002/diff/1/ios/chrome/browser/sessions...
ios/chrome/browser/sessions/session_service_ios.mm:237: FROM_HERE,
base::BindBlockArc(^{
On 2017/04/06 14:57:01, sdefresne wrote:
> On 2017/04/06 11:58:16, marq wrote:
> > Should this use criticalClosure()?
> 
> Not sure. This is called from the startup code to delete the session file
after
> loading it when restoring the app after a crash. CriticalClosure prevent the
> application from shutting down until the closure execute.
> 
> So, for this block to not be executed, the app would have to shutdown just
after
> startup. However, in that case, the user may not even have seen the "Chrome
did
> not terminate correctly, do you want to restore?" infobar.
> 
> This would also introduce a functional change in what is a refactoring/cleanup
> CL, so, even if I make the change, I think it should be in a separate CL.

I think the answer to my question is "no", then. ;-)

[sdefresne, 2017-04-06 16:38:22]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2017-04-06 16:39:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 17:24:44]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491496702938290,
"parent_rev": "c00d9dec2a75f7ab0ca2220066c93f7749b797f2", "commit_rev":
"da974c8881efb5c372206a5ac86ad807c0dbd931"}

[commit-bot: I haz the power, 2017-04-06 17:25:39]

Description was changed from

==========
[ios] Cleanup SessionServiceIOS implementation.

Before refactoring of SessionServiceIOS to make it easier to unit
test and to share with the new architecture, clean up legacy code.

-   Rename the files session_service{ => _ios}.{h,mm} to follow
    the convention of naming the files after the class defined.

-   Remove custom NSKeyedUnarchiver subclass SessionWindowUnarchiver
    as the additional property browserState is no longer and use a
    category of NSKeyedUnarchiver to define the method to register
    the aliases to load legacy saved session files.

-   Convert the code to ARC and merge "unit_tests_non_arc" target
    with "unit_tests" target as they both use ARC.

-   Reformat logged error messages, ensure that they include the
    path of the file or directory they apply to.

-   Use base::MakeCriticalClosure to prevent Chrome from suspending
    instead of using -beginBackgroundTaskWithExpirationHandler (the
    CriticalClosure code use the same functionality, but is shared
    code).

-   Remove SessionWindowIOS -clearSession as the method is no longer
    useful (obsolete).

-   Use lightweight generics and remove casts.

-   Remove obsolete #includes.

BUG=None
==========

to

==========
[ios] Cleanup SessionServiceIOS implementation.

Before refactoring of SessionServiceIOS to make it easier to unit
test and to share with the new architecture, clean up legacy code.

-   Rename the files session_service{ => _ios}.{h,mm} to follow
    the convention of naming the files after the class defined.

-   Remove custom NSKeyedUnarchiver subclass SessionWindowUnarchiver
    as the additional property browserState is no longer and use a
    category of NSKeyedUnarchiver to define the method to register
    the aliases to load legacy saved session files.

-   Convert the code to ARC and merge "unit_tests_non_arc" target
    with "unit_tests" target as they both use ARC.

-   Reformat logged error messages, ensure that they include the
    path of the file or directory they apply to.

-   Use base::MakeCriticalClosure to prevent Chrome from suspending
    instead of using -beginBackgroundTaskWithExpirationHandler (the
    CriticalClosure code use the same functionality, but is shared
    code).

-   Remove SessionWindowIOS -clearSession as the method is no longer
    useful (obsolete).

-   Use lightweight generics and remove casts.

-   Remove obsolete #includes.

BUG=None

Review-Url: https://codereview.chromium.org/2802763002
Cr-Commit-Position: refs/heads/master@{#462527}
Committed:
https://chromium.googlesource.com/chromium/src/+/da974c8881efb5c372206a5ac86a...
==========

[commit-bot: I haz the power, 2017-04-06 17:25:40]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/da974c8881efb5c372206a5ac86a...