Extensions: Pull duplicated functionality into ExtensionsTest fixture.

Extensions Tests: Pull duplicated functionality into ExtensionsTest fixture.

This CL pulls some duplicated functionality from a couple of ExtensionsTest
subclasses into the ExtensionsTest class. In particular:
  - It now provides an incognito context.
  - It also sets up Extension preferences.
This functionality will also be used in a subsequent CL. 

Also add a set_if_off_the_record method to the TestBrowserContext class to set
it up as an incognito context.

BUG=527548
Review-Url: https://codereview.chromium.org/2798503002
Cr-Commit-Position: refs/heads/master@{#462357}
Committed: https://chromium.googlesource.com/chromium/src/+/0daf48a414f373af5d47a38e22336215d10093d2

[karandeepb, 2017-04-04 03:20:07]

Description was changed from

==========
Extensions: Test cleanup.
==========

to

==========
Extensions: Pull duplicated functionality into ExtensionsTest fixture.

This CL pulls some duplicated functionality from some ExtensionsTest subclasses
into the ExtensionsTest class. In particular:
  - It now provides an incognito context.
  - It also sets up Extension preferences.
Also add a TestBrowserContextIncognito class to the TestBrowserContext header.

This functionality will also be used in a subsequent CL. 

BUG=527548
==========

[karandeepb, 2017-04-04 03:20:48]

Description was changed from

==========
Extensions: Pull duplicated functionality into ExtensionsTest fixture.

This CL pulls some duplicated functionality from some ExtensionsTest subclasses
into the ExtensionsTest class. In particular:
  - It now provides an incognito context.
  - It also sets up Extension preferences.
Also add a TestBrowserContextIncognito class to the TestBrowserContext header.

This functionality will also be used in a subsequent CL. 

BUG=527548
==========

to

==========
Extensions Tests: Pull duplicated functionality into ExtensionsTest fixture.

This CL pulls some duplicated functionality from some ExtensionsTest subclasses
into the ExtensionsTest class. In particular:
  - It now provides an incognito context.
  - It also sets up Extension preferences.
This functionality will also be used in a subsequent CL. 

Also add a TestBrowserContextIncognito class to the TestBrowserContext header.

BUG=527548
==========

[karandeepb, 2017-04-04 03:21:21]

Description was changed from

==========
Extensions Tests: Pull duplicated functionality into ExtensionsTest fixture.

This CL pulls some duplicated functionality from some ExtensionsTest subclasses
into the ExtensionsTest class. In particular:
  - It now provides an incognito context.
  - It also sets up Extension preferences.
This functionality will also be used in a subsequent CL. 

Also add a TestBrowserContextIncognito class to the TestBrowserContext header.

BUG=527548
==========

to

==========
Extensions Tests: Pull duplicated functionality into ExtensionsTest fixture.

This CL pulls some duplicated functionality from a couple of ExtensionsTest
subclasses into the ExtensionsTest class. In particular:
  - It now provides an incognito context.
  - It also sets up Extension preferences.
This functionality will also be used in a subsequent CL. 

Also add a TestBrowserContextIncognito class to the TestBrowserContext header.

BUG=527548
==========

[karandeepb, 2017-04-04 03:23:22]

karandeepb@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[karandeepb, 2017-04-04 03:23:22]

PTAL Devlin.

[Devlin, 2017-04-04 14:18:11]

Your code largely looks fine, but ExtensionsTest seems broken in some fun ways. 
It doesn't look like it'd be too hard to fix in this CL, but it probably depends
on which subclasses are relying on the wonky behavior.  Do you mind filing a new
bug for us to address these later, or taking a stab at it here?

https://codereview.chromium.org/2798503002/diff/1/extensions/browser/extensio...
File extensions/browser/extensions_test.cc (right):

https://codereview.chromium.org/2798503002/diff/1/extensions/browser/extensio...
extensions/browser/extensions_test.cc:23: // This class does work in the
constructor instead of SetUp() to give subclasses
This isn't related to this patch set at all, but this code is broken.

It claims it does this to allow subclasses do work in their ctors, but that's
kind of a broken pattern (see also below).  Instead, it subclasses should also
do work in their own SetUp methods.

https://codereview.chromium.org/2798503002/diff/1/extensions/browser/extensio...
extensions/browser/extensions_test.cc:35: incognito_context_(new
content::TestBrowserContextIncognito),
I wonder if we should create this as-needed instead?

https://codereview.chromium.org/2798503002/diff/1/extensions/browser/extensio...
extensions/browser/extensions_test.cc:79: // Allows individual tests to have
BrowserContextKeyedServiceFactory objects
This seems to be in direct violation of a comment in the header (though this is
probably more correct to do)

https://codereview.chromium.org/2798503002/diff/1/extensions/browser/extensio...
extensions/browser/extensions_test.cc:84: extensions_browser_client_.reset();
We initialize this in the ctor, but destroy it in tear down; that can cause
problems.  Also, it's not unset as the singleton until the dtor, so the
singleton instance can easily lead to UAF.

[karandeepb, 2017-04-04 19:00:37]

I tried just doing the work in SetUp() instead of the ctor, but multiple tests
failed. If it's ok, lets tackle these in another CL?

https://codereview.chromium.org/2798503002/diff/1/extensions/browser/extensio...
File extensions/browser/extensions_test.cc (right):

https://codereview.chromium.org/2798503002/diff/1/extensions/browser/extensio...
extensions/browser/extensions_test.cc:35: incognito_context_(new
content::TestBrowserContextIncognito),
On 2017/04/04 14:18:11, Devlin wrote:
> I wonder if we should create this as-needed instead?

I don't see any harm in creating one by default. There are no performance
concerns here and we should be trying to make it easier to write tests.

[Devlin, 2017-04-04 19:04:16]

On 2017/04/04 19:00:37, karandeepb wrote:
> I tried just doing the work in SetUp() instead of the ctor, but multiple tests
> failed. If it's ok, lets tackle these in another CL?

Separate CL + [TODO|Bug] sgtm. :)

https://codereview.chromium.org/2798503002/diff/1/extensions/browser/extensio...
File extensions/browser/extensions_test.cc (right):

https://codereview.chromium.org/2798503002/diff/1/extensions/browser/extensio...
extensions/browser/extensions_test.cc:35: incognito_context_(new
content::TestBrowserContextIncognito),
On 2017/04/04 19:00:37, karandeepb wrote:
> On 2017/04/04 14:18:11, Devlin wrote:
> > I wonder if we should create this as-needed instead?
> 
> I don't see any harm in creating one by default. There are no performance
> concerns here and we should be trying to make it easier to write tests.

My concern is whether the instantiation has side effects elsewhere.  e.g., if
this were a profile, it would register the profile as an OTR profile, etc.  If
it were a RenderProcessHost, it would be returned from
RenderProcessHost::AllHostsIterator.  I'm not sure whether or not there's
anything like that for BrowserContexts.  But, given lack of evidence one or way
the other, I'm fine keeping it until it becomes a problem (though it'd probably
be good to move it into SetUp(), as well).

[karandeepb, 2017-04-04 19:16:42]

PTAL Devlin.

https://codereview.chromium.org/2798503002/diff/1/extensions/browser/extensio...
File extensions/browser/extensions_test.cc (right):

https://codereview.chromium.org/2798503002/diff/1/extensions/browser/extensio...
extensions/browser/extensions_test.cc:23: // This class does work in the
constructor instead of SetUp() to give subclasses
On 2017/04/04 14:18:11, Devlin wrote:
> This isn't related to this patch set at all, but this code is broken.
> 
> It claims it does this to allow subclasses do work in their ctors, but that's
> kind of a broken pattern (see also below).  Instead, it subclasses should also
> do work in their own SetUp methods.

Filed a bug and added a TODO.

https://codereview.chromium.org/2798503002/diff/1/extensions/browser/extensio...
extensions/browser/extensions_test.cc:35: incognito_context_(new
content::TestBrowserContextIncognito),
On 2017/04/04 19:04:15, Devlin wrote:
> On 2017/04/04 19:00:37, karandeepb wrote:
> > On 2017/04/04 14:18:11, Devlin wrote:
> > > I wonder if we should create this as-needed instead?
> > 
> > I don't see any harm in creating one by default. There are no performance
> > concerns here and we should be trying to make it easier to write tests.
> 
> My concern is whether the instantiation has side effects elsewhere.  e.g., if
> this were a profile, it would register the profile as an OTR profile, etc.  If
> it were a RenderProcessHost, it would be returned from
> RenderProcessHost::AllHostsIterator.  I'm not sure whether or not there's
> anything like that for BrowserContexts.  But, given lack of evidence one or
way
> the other, I'm fine keeping it until it becomes a problem (though it'd
probably
> be good to move it into SetUp(), as well).

One side effect(desired) is that
ExtensionsBrowserClient::GetOffTheRecordContext(browser_context()) would return
this incogito_context_ and
ExtensionsBrowserClient::HasOffTheRecordContext(browser_context()) would return
true. I am not sure whether there are other unintended side effects.

https://codereview.chromium.org/2798503002/diff/1/extensions/browser/extensio...
extensions/browser/extensions_test.cc:79: // Allows individual tests to have
BrowserContextKeyedServiceFactory objects
On 2017/04/04 14:18:11, Devlin wrote:
> This seems to be in direct violation of a comment in the header (though this
is
> probably more correct to do)

Removed the comment.

https://codereview.chromium.org/2798503002/diff/1/extensions/browser/extensio...
extensions/browser/extensions_test.cc:84: extensions_browser_client_.reset();
On 2017/04/04 14:18:11, Devlin wrote:
> We initialize this in the ctor, but destroy it in tear down; that can cause
> problems.  Also, it's not unset as the singleton until the dtor, so the
> singleton instance can easily lead to UAF.

Added a TODO.

[karandeepb, 2017-04-04 19:25:23]

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

[karandeepb, 2017-04-04 19:25:24]

PTAL sky@ for changes in content/public/test/.

[sky, 2017-04-04 20:43:37]

https://codereview.chromium.org/2798503002/diff/20001/content/public/test/tes...
File content/public/test/test_browser_context.h (right):

https://codereview.chromium.org/2798503002/diff/20001/content/public/test/tes...
content/public/test/test_browser_context.h:82: bool IsOffTheRecord() const
override;
How about a setter on TestBrowserContext as if in incognito?

[karandeepb, 2017-04-04 23:03:16]

PTAL sky@.

https://codereview.chromium.org/2798503002/diff/20001/content/public/test/tes...
File content/public/test/test_browser_context.h (right):

https://codereview.chromium.org/2798503002/diff/20001/content/public/test/tes...
content/public/test/test_browser_context.h:82: bool IsOffTheRecord() const
override;
On 2017/04/04 20:43:37, sky wrote:
> How about a setter on TestBrowserContext as if in incognito?

Done.

[karandeepb, 2017-04-04 23:10:43]

Description was changed from

==========
Extensions Tests: Pull duplicated functionality into ExtensionsTest fixture.

This CL pulls some duplicated functionality from a couple of ExtensionsTest
subclasses into the ExtensionsTest class. In particular:
  - It now provides an incognito context.
  - It also sets up Extension preferences.
This functionality will also be used in a subsequent CL. 

Also add a TestBrowserContextIncognito class to the TestBrowserContext header.

BUG=527548
==========

to

==========
Extensions Tests: Pull duplicated functionality into ExtensionsTest fixture.

This CL pulls some duplicated functionality from a couple of ExtensionsTest
subclasses into the ExtensionsTest class. In particular:
  - It now provides an incognito context.
  - It also sets up Extension preferences.
This functionality will also be used in a subsequent CL. 

Also add a SetOffTheRecord method to the TestBrowserContext class.

BUG=527548
==========

[sky, 2017-04-05 02:56:55]

LGTM

https://codereview.chromium.org/2798503002/diff/60001/content/public/test/tes...
File content/public/test/test_browser_context.h (right):

https://codereview.chromium.org/2798503002/diff/60001/content/public/test/tes...
content/public/test/test_browser_context.h:39: void SetOffTheRecord(bool
is_off_the_record);
set_is_off_the_record() and inline.

[Devlin, 2017-04-05 18:48:19]

lgtm

https://codereview.chromium.org/2798503002/diff/60001/extensions/browser/exte...
File extensions/browser/extensions_test.cc (right):

https://codereview.chromium.org/2798503002/diff/60001/extensions/browser/exte...
extensions/browser/extensions_test.cc:24:
std::unique_ptr<content::TestBrowserContext> incognito_context(
prefer base::MakeUnique<>

[karandeepb, 2017-04-05 20:25:08]

Description was changed from

==========
Extensions Tests: Pull duplicated functionality into ExtensionsTest fixture.

This CL pulls some duplicated functionality from a couple of ExtensionsTest
subclasses into the ExtensionsTest class. In particular:
  - It now provides an incognito context.
  - It also sets up Extension preferences.
This functionality will also be used in a subsequent CL. 

Also add a SetOffTheRecord method to the TestBrowserContext class.

BUG=527548
==========

to

==========
Extensions Tests: Pull duplicated functionality into ExtensionsTest fixture.

This CL pulls some duplicated functionality from a couple of ExtensionsTest
subclasses into the ExtensionsTest class. In particular:
  - It now provides an incognito context.
  - It also sets up Extension preferences.
This functionality will also be used in a subsequent CL. 

Also add a set_if_off_the_record method to the TestBrowserContext class to set
it up as an incognito context.

BUG=527548
==========

[karandeepb, 2017-04-05 20:25:25]

Patchset #5 (id:80001) has been deleted

[karandeepb, 2017-04-05 20:25:48]

https://codereview.chromium.org/2798503002/diff/60001/content/public/test/tes...
File content/public/test/test_browser_context.h (right):

https://codereview.chromium.org/2798503002/diff/60001/content/public/test/tes...
content/public/test/test_browser_context.h:39: void SetOffTheRecord(bool
is_off_the_record);
On 2017/04/05 02:56:55, sky wrote:
> set_is_off_the_record() and inline.

Done.

https://codereview.chromium.org/2798503002/diff/60001/extensions/browser/exte...
File extensions/browser/extensions_test.cc (right):

https://codereview.chromium.org/2798503002/diff/60001/extensions/browser/exte...
extensions/browser/extensions_test.cc:24:
std::unique_ptr<content::TestBrowserContext> incognito_context(
On 2017/04/05 18:48:19, Devlin wrote:
> prefer base::MakeUnique<>

Done.

[karandeepb, 2017-04-06 02:56:25]

The CQ bit was checked by karandeepb@chromium.org

[karandeepb, 2017-04-06 02:56:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2798503002/#ps100001
(title: "Nits")

[commit-bot: I haz the power, 2017-04-06 02:56:46]

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 04:38:38]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491447385517270,
"parent_rev": "9b622930a92d073c8a558ee00ff9fc14c90478c0", "commit_rev":
"0daf48a414f373af5d47a38e22336215d10093d2"}

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

Description was changed from

==========
Extensions Tests: Pull duplicated functionality into ExtensionsTest fixture.

This CL pulls some duplicated functionality from a couple of ExtensionsTest
subclasses into the ExtensionsTest class. In particular:
  - It now provides an incognito context.
  - It also sets up Extension preferences.
This functionality will also be used in a subsequent CL. 

Also add a set_if_off_the_record method to the TestBrowserContext class to set
it up as an incognito context.

BUG=527548
==========

to

==========
Extensions Tests: Pull duplicated functionality into ExtensionsTest fixture.

This CL pulls some duplicated functionality from a couple of ExtensionsTest
subclasses into the ExtensionsTest class. In particular:
  - It now provides an incognito context.
  - It also sets up Extension preferences.
This functionality will also be used in a subsequent CL. 

Also add a set_if_off_the_record method to the TestBrowserContext class to set
it up as an incognito context.

BUG=527548

Review-Url: https://codereview.chromium.org/2798503002
Cr-Commit-Position: refs/heads/master@{#462357}
Committed:
https://chromium.googlesource.com/chromium/src/+/0daf48a414f373af5d47a38e2233...
==========

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

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/0daf48a414f373af5d47a38e2233...