Move creation of WebDatabaseTable subclasses to WebDatabaseServiceFactory.

Move creation of WebDatabaseTable subclasses to WebDatabaseServiceFactory.

Ownership still needs to stay with WebDatabaseService, but since WDS
is intended to move out of //chrome, it must not know about all the
individual types.

TBR=isherman@chromium.org
COLLABORATOR=caitkp@chromium.org
COLLABORATOR=kaiwang@chromium.org
BUG=181277
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=189721

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

[Jói, 2013-03-13 23:32:35]

Hi Cait,

Does this make sense to you? I realize it will conflict with your WebDataManager
change, but the merge won't be too difficult I think.

Cheers,
Jói

[Cait (Slow), 2013-03-13 23:45:06]

I agree. I think in order to merge with my change all we will have to do is move
the init code from WebDatabaseServiceFactory into WebDataManager.

On 2013/03/13 23:32:35, Jói wrote:
> Hi Cait,
> 
> Does this make sense to you? I realize it will conflict with your
WebDataManager
> change, but the merge won't be too difficult I think.
> 
> Cheers,
> Jói

[Cait (Slow), 2013-03-19 05:30:50]

lgtm

[Cait (Slow), 2013-03-21 00:01:56]

https://codereview.chromium.org/12853004/diff/16001/chrome/browser/webdata/we...
File chrome/browser/webdata/web_data_service_base.cc (right):

https://codereview.chromium.org/12853004/diff/16001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_service_base.cc:51: wdbs_.reset(new
WebDatabaseService(path));
I think we can just add a temporary |path_| member to WDSBase to store the db
path and either pass it in in the ctor or set it after. both can go away after
my change lands as the DB will be created and init'ed in the factory.

[Jói, 2013-03-21 00:03:14]

Yep, I'll do it that way, thanks.

On Thu, Mar 21, 2013 at 12:01 AM,  <caitkp@chromium.org> wrote:
>
>
https://codereview.chromium.org/12853004/diff/16001/chrome/browser/webdata/we...
> File chrome/browser/webdata/web_data_service_base.cc (right):
>
>
https://codereview.chromium.org/12853004/diff/16001/chrome/browser/webdata/we...
> chrome/browser/webdata/web_data_service_base.cc:51: wdbs_.reset(new
> WebDatabaseService(path));
> I think we can just add a temporary |path_| member to WDSBase to store
> the db path and either pass it in in the ctor or set it after. both can
> go away after my change lands as the DB will be created and init'ed in
> the factory.
>
> https://codereview.chromium.org/12853004/

[Jói, 2013-03-21 00:17:12]

caitkp: Main reviewer, PTAL.

dhollowa: OWNERS stamp please.

Cheers,
Jói

[Cait (Slow), 2013-03-21 00:37:49]

lgtm (with a couple nits)

Cheers,
Cait

https://codereview.chromium.org/12853004/diff/21001/chrome/browser/api/webdat...
File chrome/browser/api/webdata/web_data_service_base.h (right):

https://codereview.chromium.org/12853004/diff/21001/chrome/browser/api/webdat...
chrome/browser/api/webdata/web_data_service_base.h:42: explicit
WebDataServiceBase(const base::FilePath& path,
explicit not needed anymore.

https://codereview.chromium.org/12853004/diff/21001/chrome/browser/webdata/we...
File chrome/browser/webdata/web_data_service.h (right):

https://codereview.chromium.org/12853004/diff/21001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_service.h:110: explicit WebDataService(const
base::FilePath& path,
explicit not needed anymore.

https://codereview.chromium.org/12853004/diff/21001/chrome/browser/webdata/we...
File chrome/browser/webdata/web_database_service.cc (right):

https://codereview.chromium.org/12853004/diff/21001/chrome/browser/webdata/we...
chrome/browser/webdata/web_database_service.cc:82: // WebDatabaseServiceImpl,
and lookup by type is provided by the
WebDatabaseServiceImpl --> WebDatabaseService

[dhollowa, 2013-03-21 01:37:03]

lgtm

https://codereview.chromium.org/12853004/diff/21001/chrome/browser/webdata/we...
File chrome/browser/webdata/web_data_service_factory.cc (right):

https://codereview.chromium.org/12853004/diff/21001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_service_factory.cc:55: // All tables objects
that participate in managing the database must
nit: "All table objects..."

[kaiwang, 2013-03-21 06:30:56]

Help Joi to address comments:

https://codereview.chromium.org/12853004/diff/21001/chrome/browser/api/webdat...
File chrome/browser/api/webdata/web_data_service_base.h (right):

https://codereview.chromium.org/12853004/diff/21001/chrome/browser/api/webdat...
chrome/browser/api/webdata/web_data_service_base.h:42: explicit
WebDataServiceBase(const base::FilePath& path,
On 2013/03/21 00:37:49, caitkp wrote:
> explicit not needed anymore.

Done.

https://codereview.chromium.org/12853004/diff/21001/chrome/browser/webdata/we...
File chrome/browser/webdata/web_data_service.h (right):

https://codereview.chromium.org/12853004/diff/21001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_service.h:110: explicit WebDataService(const
base::FilePath& path,
On 2013/03/21 00:37:49, caitkp wrote:
> explicit not needed anymore.

Done.

https://codereview.chromium.org/12853004/diff/21001/chrome/browser/webdata/we...
File chrome/browser/webdata/web_data_service_factory.cc (right):

https://codereview.chromium.org/12853004/diff/21001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_service_factory.cc:55: // All tables objects
that participate in managing the database must
On 2013/03/21 01:37:03, dhollowa wrote:
> nit: "All table objects..."

Done.

https://codereview.chromium.org/12853004/diff/21001/chrome/browser/webdata/we...
File chrome/browser/webdata/web_database_service.cc (right):

https://codereview.chromium.org/12853004/diff/21001/chrome/browser/webdata/we...
chrome/browser/webdata/web_database_service.cc:82: // WebDatabaseServiceImpl,
and lookup by type is provided by the
On 2013/03/21 00:37:49, caitkp wrote:
> WebDatabaseServiceImpl --> WebDatabaseService

Done.

[commit-bot: I haz the power, 2013-03-21 06:35:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/12853004/29001

[commit-bot: I haz the power, 2013-03-21 07:46:05]

Retried try job too often on linux_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-03-21 09:51:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/12853004/58002

[commit-bot: I haz the power, 2013-03-21 09:51:49]

Presubmit check for 12853004-58002 failed and returned exit status 1.

INFO:root:Found 10 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/browser/password_manager/password_store_win_unittest.cc

Presubmit checks took 1.3s to calculate.

[Jói, 2013-03-21 10:28:40]

TBR=isherman@chromium.org for trivial usage update to
password_store_win_unittest.cc.

Cheers,
Jói

[commit-bot: I haz the power, 2013-03-21 10:30:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/12853004/60002

[commit-bot: I haz the power, 2013-03-21 22:22:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/12853004/65002

[Jói, 2013-03-22 00:04:39]

Committed patchset #10 manually as r189721 (presubmit successful).

[commit-bot: I haz the power, 2013-03-22 03:43:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/12853004/78001

[commit-bot: I haz the power, 2013-03-22 13:31:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/12853004/78001

[Cait (Slow), 2013-03-22 14:59:33]

https://codereview.chromium.org/12853004/diff/78001/chrome/browser/password_m...
File chrome/browser/password_manager/password_store_win_unittest.cc (right):

https://codereview.chromium.org/12853004/diff/78001/chrome/browser/password_m...
chrome/browser/password_manager/password_store_win_unittest.cc:119:
WebDataServiceBase::ProfileErrorCallback());
I just realized we need to manually add a loginTable to this for these tests to
work (as is done for the autofillwebdataservice tests) I'll upload a patch.

[commit-bot: I haz the power, 2013-03-22 15:29:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/12853004/69003

[Jói, 2013-03-22 16:50:22]

Thanks Cait. I had half given up on landing this change today because
of the trybot situation, and had been busy with Perf. I'm glad if you
can get this landed so your change to split stuff can go in.

Cheers,
Jói


On Fri, Mar 22, 2013 at 3:29 PM,  <commit-bot@chromium.org> wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/joi@chromium.org/12853004/69003
>
>
> https://chromiumcodereview.appspot.com/12853004/

[commit-bot: I haz the power, 2013-03-22 18:55:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/12853004/69003

[commit-bot: I haz the power, 2013-03-23 14:00:51]

Failed to trigger a try job on linux_aura
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-03-23 14:01:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/12853004/94001

[commit-bot: I haz the power, 2013-03-23 15:39:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/12853004/94001

[commit-bot: I haz the power, 2013-03-23 17:28:56]

Failed to trigger a try job on win7_aura
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-03-23 17:29:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/12853004/106001

[commit-bot: I haz the power, 2013-03-23 19:58:55]

Retried try job too often on win_rel for step(s) net_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-03-24 13:22:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/12853004/107001

[commit-bot: I haz the power, 2013-03-24 13:29:02]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-03-24 13:40:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/12853004/116002

[commit-bot: I haz the power, 2013-03-24 14:52:17]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[Jói, 2013-03-24 17:13:57]

Committed patchset #16 manually as r190277 (presubmit successful).