[ios] Refactor SessionServiceIOS to remove dependency on BrowserState.

[ios] Refactor SessionServiceIOS to remove dependency on BrowserState.

Change SessionServiceIOS API to remove dependency on BrowserState
by explicitly passing location where the SessionWindowIOS will be
saved to/loaded from.

Add public initializer for SessionServiceIOS taking an explicit
SequencialTaskRunner to decouple unit testing from BrowserState,
WebState and WebThread (and to avoid using global singleton).

BUG=None

Review-Url: https://codereview.chromium.org/2810743002
Cr-Commit-Position: refs/heads/master@{#464005}
Committed: https://chromium.googlesource.com/chromium/src/+/80a544628c10dd3fa27936f69d94b1fb1a1327ab

[sdefresne, 2017-04-10 14:33:40]

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

[commit-bot: I haz the power, 2017-04-10 14:33:59]

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-10 14:37:18]

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

[sdefresne, 2017-04-10 14:37:19]

Please take a look.

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

https://codereview.chromium.org/2810743002/diff/1/ios/chrome/browser/sessions...
ios/chrome/browser/sessions/session_service_ios_unittest.mm:84:
TEST_F(SessionServiceTest, Singleton) {
I removed this test intentionally as the singleton accessor does initialise the
SessionServiceIOS task runner from WebThread, but the test was using
web::TestWebThreadBundle, thus the singleton was referencing a TaskRunner that
was completely disconnected from everything.

In general it is incorrect for unit tests to use singletons.

[commit-bot: I haz the power, 2017-04-10 14:43:16]

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

[commit-bot: I haz the power, 2017-04-10 14:43:17]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[sdefresne, 2017-04-10 14:56:18]

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

[commit-bot: I haz the power, 2017-04-10 14:56:30]

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-10 16:00:41]

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

[commit-bot: I haz the power, 2017-04-10 16:00:42]

Dry run: This issue passed the CQ dry run.

[sdefresne, 2017-04-11 08:25:18]

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

[commit-bot: I haz the power, 2017-04-11 08:25: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-11 09:30:52]

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

[commit-bot: I haz the power, 2017-04-11 09:30:53]

Dry run: This issue passed the CQ dry run.

[marq (ping after 24h), 2017-04-11 10:46:29]

https://codereview.chromium.org/2810743002/diff/40001/ios/chrome/browser/cras...
File ios/chrome/browser/crash_report/crash_restore_helper_unittest.mm (right):

https://codereview.chromium.org/2810743002/diff/40001/ios/chrome/browser/cras...
ios/chrome/browser/crash_report/crash_restore_helper_unittest.mm:63: NSString*
otrStatePath = base::SysUTF8ToNSString(
Per crbug.com/3333, we prefer to just use 'incognito' instead of 'OTR'. 

(and, if not, we'd prefer offTheRecordStatePath to avoid the initialism).

https://codereview.chromium.org/2810743002/diff/40001/ios/chrome/browser/sess...
File ios/chrome/browser/sessions/session_service_ios.h (right):

https://codereview.chromium.org/2810743002/diff/40001/ios/chrome/browser/sess...
ios/chrome/browser/sessions/session_service_ios.h:31: // blocking the UI thread.
As a result, |sessionWindow| should contain copies
Should, or could? Or shouldn't?

https://codereview.chromium.org/2810743002/diff/40001/ios/chrome/browser/sess...
ios/chrome/browser/sessions/session_service_ios.h:56: -
(void)performSaveSessionData:(NSData*)sessionData
Can we remove the need for this by running tests on an instance whose taskRunner
runs synchronously on the main thread?

https://codereview.chromium.org/2810743002/diff/40001/ios/chrome/browser/sess...
File ios/chrome/browser/sessions/session_service_ios.mm (right):

https://codereview.chromium.org/2810743002/diff/40001/ios/chrome/browser/sess...
ios/chrome/browser/sessions/session_service_ios.mm:188: } @catch (NSException*
e) {
e -> exception

https://codereview.chromium.org/2810743002/diff/40001/ios/chrome/browser/sess...
File ios/chrome/browser/sessions/session_service_ios_unittest.mm (right):

https://codereview.chromium.org/2810743002/diff/40001/ios/chrome/browser/sess...
ios/chrome/browser/sessions/session_service_ios_unittest.mm:36: void SetUp()
override {
Can this move into the constructor?

[sdefresne, 2017-04-11 11:49:41]

Please take another look.

https://codereview.chromium.org/2810743002/diff/40001/ios/chrome/browser/cras...
File ios/chrome/browser/crash_report/crash_restore_helper_unittest.mm (right):

https://codereview.chromium.org/2810743002/diff/40001/ios/chrome/browser/cras...
ios/chrome/browser/crash_report/crash_restore_helper_unittest.mm:63: NSString*
otrStatePath = base::SysUTF8ToNSString(
On 2017/04/11 10:46:29, marq wrote:
> Per crbug.com/3333, we prefer to just use 'incognito' instead of 'OTR'. 
> 
> (and, if not, we'd prefer offTheRecordStatePath to avoid the initialism).

Done.

https://codereview.chromium.org/2810743002/diff/40001/ios/chrome/browser/sess...
File ios/chrome/browser/sessions/session_service_ios.h (right):

https://codereview.chromium.org/2810743002/diff/40001/ios/chrome/browser/sess...
ios/chrome/browser/sessions/session_service_ios.h:31: // blocking the UI thread.
As a result, |sessionWindow| should contain copies
On 2017/04/11 10:46:29, marq wrote:
> Should, or could? Or shouldn't?

Removed as the object is now only accessed on the main thread, so the comment is
irrelevant.

https://codereview.chromium.org/2810743002/diff/40001/ios/chrome/browser/sess...
ios/chrome/browser/sessions/session_service_ios.h:56: -
(void)performSaveSessionData:(NSData*)sessionData
On 2017/04/11 10:46:29, marq wrote:
> Can we remove the need for this by running tests on an instance whose
taskRunner
> runs synchronously on the main thread?

Updated the tests to not use this method. I still need to call it from subclass
TestSessionService, so I renamed the extension category to SubClassing.

https://codereview.chromium.org/2810743002/diff/40001/ios/chrome/browser/sess...
File ios/chrome/browser/sessions/session_service_ios.mm (right):

https://codereview.chromium.org/2810743002/diff/40001/ios/chrome/browser/sess...
ios/chrome/browser/sessions/session_service_ios.mm:188: } @catch (NSException*
e) {
On 2017/04/11 10:46:29, marq wrote:
> e -> exception

Done.

https://codereview.chromium.org/2810743002/diff/40001/ios/chrome/browser/sess...
File ios/chrome/browser/sessions/session_service_ios_unittest.mm (right):

https://codereview.chromium.org/2810743002/diff/40001/ios/chrome/browser/sess...
ios/chrome/browser/sessions/session_service_ios_unittest.mm:36: void SetUp()
override {
On 2017/04/11 10:46:29, marq wrote:
> Can this move into the constructor?

ScopedTempDir::CreateUniqueTempDir() does return an error that need to be
checked and the test should fail in that case (i.e. ASSERT_TRUE). However,
ASSERT_TRUE returns from the current function after setting a global flag, which
does not work in a constructor and fails with the following error:

../../ios/chrome/browser/sessions/session_service_ios_unittest.mm:35:5: error:
constructor 'SessionServiceTest' must not return void expression

[sdefresne, 2017-04-11 11:49:58]

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

[commit-bot: I haz the power, 2017-04-11 11:50:05]

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-11 12:53:42]

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

[commit-bot: I haz the power, 2017-04-11 12:53:42]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[sdefresne, 2017-04-12 11:16:22]

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

[commit-bot: I haz the power, 2017-04-12 11:16:39]

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

[marq (ping after 24h), 2017-04-12 11:24:41]

lgtm

[sdefresne, 2017-04-12 11:31:47]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2017-04-12 11:31:48]

The patchset sent to the CQ was uploaded after l-g-t-m from marq@chromium.org
Link to the patchset: https://codereview.chromium.org/2810743002/#ps100001
(title: "Fix a typo in SessionServiceIOS initializer's comment.")

[commit-bot: I haz the power, 2017-04-12 11:32:22]

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-12 12:44:31]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491996707561930,
"parent_rev": "8a4add0c160a539fba0a3ddd47b7d2b05d0d5e4d", "commit_rev":
"80a544628c10dd3fa27936f69d94b1fb1a1327ab"}

[commit-bot: I haz the power, 2017-04-12 12:45:28]

Description was changed from

==========
[ios] Refactor SessionServiceIOS to remove dependency on BrowserState.

Change SessionServiceIOS API to remove dependency on BrowserState
by explicitly passing location where the SessionWindowIOS will be
saved to/loaded from.

Add public initializer for SessionServiceIOS taking an explicit
SequencialTaskRunner to decouple unit testing from BrowserState,
WebState and WebThread (and to avoid using global singleton).

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

to

==========
[ios] Refactor SessionServiceIOS to remove dependency on BrowserState.

Change SessionServiceIOS API to remove dependency on BrowserState
by explicitly passing location where the SessionWindowIOS will be
saved to/loaded from.

Add public initializer for SessionServiceIOS taking an explicit
SequencialTaskRunner to decouple unit testing from BrowserState,
WebState and WebThread (and to avoid using global singleton).

BUG=None

Review-Url: https://codereview.chromium.org/2810743002
Cr-Commit-Position: refs/heads/master@{#464005}
Committed:
https://chromium.googlesource.com/chromium/src/+/80a544628c10dd3fa27936f69d94...
==========

[commit-bot: I haz the power, 2017-04-12 12:45:29]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/80a544628c10dd3fa27936f69d94...