Ensure incognito TestingProfiles are incognito for their whole lifetime.

Ensure incognito TestingProfiles are incognito for their whole lifetime.

Otherwise keyed services see different profiles in unit_tests at
different times since the incognito->GetOriginalProfile() changes. This
is causing release code to pick up unnecessary workarounds to keep
unit_tests green in try runs.

The fix is achieved by adding `TestingProfile::Builder::BuildIncognito`,
which differs from `Build` by requiring an |original_profile| argument,
which will own the incognito profile (so no need to return scoped_ptr).
This simplifies a lot of call sites, since they no longer need to do a
scoped_ptr dance of their own.

BUG=277296
Committed: https://crrev.com/1ab9b709cb4d691f667c843040b65b46a9c82ebd
Cr-Commit-Position: refs/heads/master@{#294931}

[tapted, 2014-09-11 01:20:14]

Patchset #3 (id:40001) has been deleted

[tapted, 2014-09-11 08:20:08]

Patchset #6 (id:120001) has been deleted

[tapted, 2014-09-12 03:37:36]

Patchset #2 (id:20001) has been deleted

[tapted, 2014-09-12 03:37:45]

Patchset #3 (id:80001) has been deleted

[tapted, 2014-09-12 07:54:06]

tapted@chromium.org changed reviewers:
+ sky@chromium.org

[tapted, 2014-09-12 07:54:09]

Hi sky - please review - mainly for chrome/test/base . The other changes are
mostly mechanical (some notes where not), so please let me know if I should
collect more owners :)

There is some context for this in
https://codereview.chromium.org/441553002/#msg16 (summary: unnecessary state was
being put in Release code just so that some unit tests could pass - this will
fix that).

https://codereview.chromium.org/556173002/diff/180001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager_unittest.cc (left):

https://codereview.chromium.org/556173002/diff/180001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_unittest.cc:769: 
this removed code is essentially what's done by the
`profile1->GetOffTheRecordProfile()` call below

https://codereview.chromium.org/556173002/diff/180001/chrome/browser/ui/sync/...
File chrome/browser/ui/sync/one_click_signin_helper_unittest.cc (right):

https://codereview.chromium.org/556173002/diff/180001/chrome/browser/ui/sync/...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:401: }
I didn't want to use `ForceIncognito` here, but it was getting complicated
without it. However, I've realised ForceIncognito isn't always to blame for the
problems in http://crbug.com/277296 . Instead it's the GetOriginalProfile()
mismatch while creating the incognito TestingProfile "properly", which this CL
is fixing.

https://codereview.chromium.org/556173002/diff/180001/chrome/test/base/testin...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/556173002/diff/180001/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:416: incognito_profile_.reset();
This was needed to resolve a lifetime issue in the
UserCloudPolicyManagerChromeOSTest tests, under
BrowserContextKeyedServiceFactory::Disassociate(). It matches what
ProfileImpl::~ProfileImpl() does.

[sky, 2014-09-12 15:33:30]

https://codereview.chromium.org/556173002/diff/180001/chrome/test/base/testin...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/556173002/diff/180001/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:635: void
TestingProfile::SetOffTheRecordProfile(scoped_ptr<Profile> profile) {
Can this be changed to take a TestingProfile? It would be nice if it set up the
necessarily backpointers.

https://codereview.chromium.org/556173002/diff/180001/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:729: void
TestingProfile::SetTestingOffTheRecordProfile(Profile* incognito_profile) {
Is there a reason we need this? Why not have the constructor call
SetOffTheRecordProfile directly?

https://codereview.chromium.org/556173002/diff/180001/chrome/test/base/testin...
File chrome/test/base/testing_profile.h (right):

https://codereview.chromium.org/556173002/diff/180001/chrome/test/base/testin...
chrome/test/base/testing_profile.h:114: // need customize the Builder, or access
TestingProfile member functions,
to customize?

https://codereview.chromium.org/556173002/diff/180001/chrome/test/base/testin...
chrome/test/base/testing_profile.h:115: // you can probably just call
original_profile->GetOffTheRecordProfile().
'probably just call' -> use

https://codereview.chromium.org/556173002/diff/180001/chrome/test/base/testin...
chrome/test/base/testing_profile.h:158: TestingProfile* incognito_parent,
incognito_parent is mildly confusing, in so far as it makes it sound like the
parent is incognito, which isn't the case. Maybe just parent?

[tapted, 2014-09-15 01:12:03]

Patchset #7 (id:200001) has been deleted

[tapted, 2014-09-15 01:17:56]

Patchset #7 (id:220001) has been deleted

[tapted, 2014-09-15 02:51:20]

Patchset #7 (id:240001) has been deleted

[tapted, 2014-09-15 05:05:17]

https://codereview.chromium.org/556173002/diff/180001/chrome/test/base/testin...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/556173002/diff/180001/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:635: void
TestingProfile::SetOffTheRecordProfile(scoped_ptr<Profile> profile) {
On 2014/09/12 15:33:29, sky wrote:
> Can this be changed to take a TestingProfile? It would be nice if it set up
the
> necessarily backpointers.

Not easily - that was actually my first attempt, but the
OffTheRecordProfileImplTest unit tests pass an instance of
OffTheRecordProfileImpl rather than TestingProfile. However,
OffTheRecordProfileImpl sets its original_profile in the constructor too, so
I've added `DCHECK_EQ(this, profile->GetOriginalProfile());` to ensure the
backpointer is already set.

https://codereview.chromium.org/556173002/diff/180001/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:729: void
TestingProfile::SetTestingOffTheRecordProfile(Profile* incognito_profile) {
On 2014/09/12 15:33:29, sky wrote:
> Is there a reason we need this? Why not have the constructor call
> SetOffTheRecordProfile directly?

Nope! Removed. I think I had this taking a TestingProfile* at one point, but
that became unnecessary.

https://codereview.chromium.org/556173002/diff/180001/chrome/test/base/testin...
File chrome/test/base/testing_profile.h (right):

https://codereview.chromium.org/556173002/diff/180001/chrome/test/base/testin...
chrome/test/base/testing_profile.h:114: // need customize the Builder, or access
TestingProfile member functions,
On 2014/09/12 15:33:29, sky wrote:
> to customize?

Done.

https://codereview.chromium.org/556173002/diff/180001/chrome/test/base/testin...
chrome/test/base/testing_profile.h:115: // you can probably just call
original_profile->GetOffTheRecordProfile().
On 2014/09/12 15:33:29, sky wrote:
> 'probably just call' -> use

Done.

https://codereview.chromium.org/556173002/diff/180001/chrome/test/base/testin...
chrome/test/base/testing_profile.h:158: TestingProfile* incognito_parent,
On 2014/09/12 15:33:29, sky wrote:
> incognito_parent is mildly confusing, in so far as it makes it sound like the
> parent is incognito, which isn't the case. Maybe just parent?

Done.

[sky, 2014-09-15 15:44:06]

LGTM

[tapted, 2014-09-15 22:21:33]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2014-09-15 22:39:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/556173002/260001

[commit-bot: I haz the power, 2014-09-15 23:50:38]

Committed patchset #7 (id:260001) as 780bf4b4051c00381a034ca509696d5e68f69c19

[commit-bot: I haz the power, 2014-09-15 23:53:33]

Patchset 7 (id:??) landed as
https://crrev.com/1ab9b709cb4d691f667c843040b65b46a9c82ebd
Cr-Commit-Position: refs/heads/master@{#294931}