Split IndexedDBFactory into virtual base + impl.

Split IndexedDBFactory into virtual base + impl.

This change makes IndexedDBFactory a virtual base class, and what was
IndexedDBFactory is now IndexedDBFactoryImpl. This change will allow tests
to create proper mock factories and not require them to instantiate the
"real" factory (which often doesn't work) in the test. It will also take us
one step closer to eliminating special cases in IDB's other classes where
having a factory is optional (because tests don't create them).

BUG=393974
R=ericu@chromium.org, jochen@chromium.org, jsbell@chromium.org, michaeln@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284161

[ericu, 2014-06-08 19:04:14]

I'm missing the point here.  All of your test objects now inherit from the impl,
not the interface, so where are you expecting to make use of this split?

[cmumford, 2014-06-10 17:47:23]

On 2014/06/08 19:04:14, ericu wrote:
> I'm missing the point here.  All of your test objects now inherit from the
impl,
> not the interface, so where are you expecting to make use of this split?

Here's a downstream CL which depends on this change:

https://codereview.chromium.org/308103004/

[ericu, 2014-06-11 00:04:23]

OK, LGTM.

It would be nice to have a standard mock/fake object for use in tests, so we
don't have multiple copies of mock code, as in that other CL.

[cmumford, 2014-06-11 18:18:48]

On 2014/06/11 00:04:23, ericu wrote:
> OK, LGTM.
> 
> It would be nice to have a standard mock/fake object for use in tests, so we
> don't have multiple copies of mock code, as in that other CL.

I was thinking the same thing. I'll put one into this CL and we'll see if that
simplifies things - which I suspect it will.

[cmumford, 2014-06-16 17:45:37]

On 2014/06/11 18:18:48, cmumford wrote:
> On 2014/06/11 00:04:23, ericu wrote:
> > OK, LGTM.
> > 
> > It would be nice to have a standard mock/fake object for use in tests, so we
> > don't have multiple copies of mock code, as in that other CL.
> 
> I was thinking the same thing. I'll put one into this CL and we'll see if that
> simplifies things - which I suspect it will.

standard mock object does work and simplifies code:

https://codereview.chromium.org/308103004/#ps60001

[cmumford, 2014-07-14 19:40:23]

On 2014/06/16 17:45:37, cmumford wrote:
> On 2014/06/11 18:18:48, cmumford wrote:
> > On 2014/06/11 00:04:23, ericu wrote:
> > > OK, LGTM.
> > > 
> > > It would be nice to have a standard mock/fake object for use in tests, so
we
> > > don't have multiple copies of mock code, as in that other CL.
> > 
> > I was thinking the same thing. I'll put one into this CL and we'll see if
that
> > simplifies things - which I suspect it will.
> 
> standard mock object does work and simplifies code:
> 
> https://codereview.chromium.org/308103004/#ps60001

PTAL

[ericu, 2014-07-14 20:23:41]

Still LGTM; I suspect you want to use Michael's other account, though.

[michaeln, 2014-07-15 02:00:46]

lgtm2

[jochen (gone - plz use gerrit), 2014-07-15 11:44:46]

can you reference a bug in the CL description?

https://codereview.chromium.org/313883003/diff/20001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_factory.h (right):

https://codereview.chromium.org/313883003/diff/20001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_factory.h:33: : NON_EXPORTED_BASE(public
base::RefCountedThreadSafe<IndexedDBFactory>) {
an interface shouldn't inherit from something else

https://codereview.chromium.org/313883003/diff/20001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_factory.h:83: IndexedDBFactory() {}
no need for a ctor on an interface

https://codereview.chromium.org/313883003/diff/20001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_factory.h:84: virtual ~IndexedDBFactory()
{}
the dtor can be public as well

https://codereview.chromium.org/313883003/diff/20001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_factory_impl.h (right):

https://codereview.chromium.org/313883003/diff/20001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_factory_impl.h:18: virtual void
ReleaseDatabase(const IndexedDBDatabase::Identifier& identifier,
can you add an comment a la // IndexedDBFactory overrides

https://codereview.chromium.org/313883003/diff/20001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_factory_impl.h:109:
DISALLOW_COPY_AND_ASSIGN(IndexedDBFactoryImpl);
should go at the very end

[cmumford, 2014-07-15 16:11:48]

Thanks for review jochen. I hunted the logs for a better description than
"virtual base class", but couldn't find another example. Let me know if you have
a recommendation.

https://codereview.chromium.org/313883003/diff/20001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_factory.h (right):

https://codereview.chromium.org/313883003/diff/20001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_factory.h:33: : NON_EXPORTED_BASE(public
base::RefCountedThreadSafe<IndexedDBFactory>) {
On 2014/07/15 11:44:46, jochen wrote:
> an interface shouldn't inherit from something else

Then maybe I shouldn't use the word "interface" in my commit comment? The
implementation does still need to have a reference counted base class from which
other implementation classes inherit. I'm modeling this after the existing
IndexedDBContext/IndexedDBContextImpl. I'll change the description to say
something like "base implementation".

https://codereview.chromium.org/313883003/diff/20001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_factory.h:83: IndexedDBFactory() {}
On 2014/07/15 11:44:46, jochen wrote:
> no need for a ctor on an interface

W/o it I get this error:

error: constructor for 'content::IndexedDBFactoryImpl' must explicitly
initialize the base class 'content::IndexedDBFactory' which does not have a
default constructor

https://codereview.chromium.org/313883003/diff/20001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_factory.h:84: virtual ~IndexedDBFactory()
{}
On 2014/07/15 11:44:46, jochen wrote:
> the dtor can be public as well

I think that describing as not an interface satisfies this.

https://codereview.chromium.org/313883003/diff/20001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_factory_impl.h (right):

https://codereview.chromium.org/313883003/diff/20001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_factory_impl.h:18: virtual void
ReleaseDatabase(const IndexedDBDatabase::Identifier& identifier,
On 2014/07/15 11:44:46, jochen wrote:
> can you add an comment a la // IndexedDBFactory overrides

Done.

https://codereview.chromium.org/313883003/diff/20001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_factory_impl.h:109:
DISALLOW_COPY_AND_ASSIGN(IndexedDBFactoryImpl);
On 2014/07/15 11:44:46, jochen wrote:
> should go at the very end

Done.

[jochen (gone - plz use gerrit), 2014-07-16 08:43:26]

sorry for the back and forth.

After rereading this CL and the follow-up CL, I'm not sure I understand why
extracting an interface is required at all. You can just make the methods
virtual and override them all in your mock (also, I'd just provide empty bodies
instead of using gmock), no?

I think the ctor in the IDBFactory class is required because of the disallow
copy/assign (which you shouldn't need on an interface), and the unimplemented
methods ReleaseBackingStore/CloseBackingStore

[cmumford, 2014-07-16 16:44:08]

On 2014/07/16 08:43:26, jochen wrote:
> sorry for the back and forth.
> 
> After rereading this CL and the follow-up CL, I'm not sure I understand why
> extracting an interface is required at all. You can just make the methods
> virtual and override them all in your mock (also, I'd just provide empty
bodies
> instead of using gmock), no?
> 
> I think the ctor in the IDBFactory class is required because of the disallow
> copy/assign (which you shouldn't need on an interface), and the unimplemented
> methods ReleaseBackingStore/CloseBackingStore

No problem jochen. Yes, I could do as you suggest, "make the methods virtual and
override them all in your mock", but that would make the tests fragile. For
example, if a future new method was added to IDBFactory and called by another
class the compiler would be of no help in identifying the various mock factories
in use in our tests, and this new method in IDBFactory would actually be called
in our test - leading to unpredictable results. Having an interface turns this
from a runtime error to a compile time one.

On the subject of using gmock vs. empty implementations. My intention with this
CL is to lay the groundwork for better tests going forward. Ones which assert
things like "function XXX was called exactly once with this value". This is what
gmock was invented for and I'm hoping to introduce more robust tests going
forward. I didn't want to refactor our tests in this CL as it's already big
enough.

Oh, one more point. There does seem to be precedent for interfaces which aren't
exactly pure (i.e. they are RefCounted interfaces). Here are a handful of them:

chrome/browser/browsing_data/browsing_data_quota_helper.h
chrome/browser/history/top_sites.h
components/usb_service/usb_device.h
components/usb_service/usb_device_handle.h
content/browser/speech/speech_recognizer.h
content/public/browser/devtools_agent_host.h
content/public/browser/site_instance.h
content/renderer/p2p/socket_client.h
media/audio/audio_output_dispatcher.h

Again, thx for your review.

[jochen (gone - plz use gerrit), 2014-07-17 09:22:01]

ok

lgtm with nits addressed

https://codereview.chromium.org/313883003/diff/40001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_factory.h (right):

https://codereview.chromium.org/313883003/diff/40001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_factory.h:105: void
ReleaseBackingStore(const GURL& origin_url, bool immediate);
delete this and the next line

https://codereview.chromium.org/313883003/diff/40001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_factory.h:110:
ForceCloseOpenDatabasesOnCommitFailure);
should not be required

[cmumford, 2014-07-17 18:19:46]

jsbell@ PTAL

https://codereview.chromium.org/313883003/diff/40001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_factory.h (right):

https://codereview.chromium.org/313883003/diff/40001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_factory.h:105: void
ReleaseBackingStore(const GURL& origin_url, bool immediate);
On 2014/07/17 09:22:00, jochen wrote:
> delete this and the next line

Done - thx for catching that.

https://codereview.chromium.org/313883003/diff/40001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_factory.h:110:
ForceCloseOpenDatabasesOnCommitFailure);
On 2014/07/17 09:22:00, jochen wrote:
> should not be required

If you mean the FRIEND_TEST... then it's there because
ForceCloseOpenDatabasesOnCommitFailure has a IndexedDBFactory pointer and calls
the private IsBackingStoreOpen method. Do you see an alternative?

[jsbell, 2014-07-17 18:45:46]

lgtm

https://codereview.chromium.org/313883003/diff/1/content/browser/indexed_db/i...
File content/browser/indexed_db/indexed_db_factory.h (right):

https://codereview.chromium.org/313883003/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_factory.h:104: virtual bool
IsBackingStoreOpen(const GURL& origin_url) const = 0;
Since this is an impl detail, not part of the interface except for testing, how
about moving this to the Impl and have
IndexedDBTest::ForceCloseOpenDatabasesOnCommitFailure call
static_cast<IndexedDBFactoryImpl*>(factory)->IsBackingStoreOpen(...) ?

https://codereview.chromium.org/313883003/diff/60001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_factory_unittest.cc (right):

https://codereview.chromium.org/313883003/diff/60001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_factory_unittest.cc:13: #include
"content/browser/indexed_db/indexed_db_factory_impl.h"
Huh... I thought we put "self" includes by unittests of the class they're
testing at the top, but it looks like we're inconsistent throughout content. :(

[cmumford, 2014-07-17 20:11:52]

https://codereview.chromium.org/313883003/diff/1/content/browser/indexed_db/i...
File content/browser/indexed_db/indexed_db_factory.h (right):

https://codereview.chromium.org/313883003/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_factory.h:104: virtual bool
IsBackingStoreOpen(const GURL& origin_url) const = 0;
On 2014/07/17 18:45:46, jsbell wrote:
> Since this is an impl detail, not part of the interface except for testing,
how
> about moving this to the Impl and have
> IndexedDBTest::ForceCloseOpenDatabasesOnCommitFailure call
> static_cast<IndexedDBFactoryImpl*>(factory)->IsBackingStoreOpen(...) ?

Yeah, I'm not a fan of downcasting like this, but I agree that a test helper
function shouldn't be in the interface, so your suggestion is clearly better.

Done.

https://codereview.chromium.org/313883003/diff/60001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_factory_unittest.cc (right):

https://codereview.chromium.org/313883003/diff/60001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_factory_unittest.cc:13: #include
"content/browser/indexed_db/indexed_db_factory_impl.h"
On 2014/07/17 18:45:46, jsbell wrote:
> Huh... I thought we put "self" includes by unittests of the class they're
> testing at the top, but it looks like we're inconsistent throughout content.
:(

I did that because it produced a presubmit warning. But I think you're right so
I moved it back, and sent an email to marja.

[cmumford, 2014-07-17 20:14:48]

The CQ bit was checked by cmumford@chromium.org

[commit-bot: I haz the power, 2014-07-17 20:18:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/cmumford@chromium.org/313883003/80001

[commit-bot: I haz the power, 2014-07-18 00:27:05]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-07-18 00:50:53]

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

[commit-bot: I haz the power, 2014-07-18 00:50:54]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[jsbell, 2014-07-18 18:21:48]

Committed patchset #5 manually as r284161 (presubmit successful).