Issue 12695015: Split Autofill webdata

Issue 12695015: Split Autofill webdata (Closed)

Created:
7 years, 9 months ago by Cait (Slow)

Modified:
7 years, 9 months ago

Reviewers:
tim (not reviewing), Jói, dhollowa

CC:
chromium-reviews, Raman Kakilate, akalin, Raghu Simha, benquan, dhollowa+watch_chromium.org, ahutter, browser-components-watch_chromium.org, dbeam+watch-autofill_chromium.org, Dane Wallinga, dyu1, Albert Bodenhamer, estade+watch_chromium.org, haitaol1, Ilya Sherman, tim (not reviewing)

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Split Autofill webdata Depends on https://codereview.chromium.org/12853004/ TBR=tim@chromium.org,sky@chromium.org (tim@ for sync, sky@ for seach_engines) COLLABORATOR=joi@chromium.org COLLABORATOR=kaiwang@chromium.org BUG=181277 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190317

Patch Set 1 #

Total comments: 15

Patch Set 2 : Set upstream to track 12780012 #

Patch Set 3 : Merge to the end of WDS pipeline #

Total comments: 7

Patch Set 4 : Convert all Autofill/Sync clients of WDS to using AutofillWDS #

Total comments: 26

Patch Set 5 : Pure merge to 12853004 (move table creation to factory) #

Total comments: 22

Patch Set 6 : Respond to comments and fix unittests #

Patch Set 7 : Add a dtor #

Patch Set 8 : Fix sync test #

Patch Set 9 : merge to ToT for commit #

Patch Set 10 : Fix Win build #

Created: 7 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+731 lines, -273 lines)			Patch
M	chrome/browser/api/webdata/autofill_web_data_service.h	View	1 2 3 4 5	1 chunk	+18 lines, -2 lines	0 comments	Download
M	chrome/browser/api/webdata/web_data_service_base.h	View	1 2 3 4 5	3 chunks	+8 lines, -7 lines	0 comments	Download
M	chrome/browser/memory_purger.cc	View	1 2 3	1 chunk	+3 lines, -3 lines	0 comments	Download
M	chrome/browser/password_manager/password_store_win_unittest.cc	View	1 2 3 4 5 6 7 8 9	4 chunks	+9 lines, -3 lines	0 comments	Download
M	chrome/browser/search_engines/template_url_service_test_util.cc	View	1 2 3	1 chunk	+1 line, -2 lines	0 comments	Download
M	chrome/browser/search_engines/template_url_service_unittest.cc	View	1 2 3	1 chunk	+1 line, -2 lines	0 comments	Download
M	chrome/browser/sync/glue/autofill_data_type_controller.h	View	1 2 3	2 chunks	+2 lines, -2 lines	0 comments	Download
M	chrome/browser/sync/glue/autofill_data_type_controller.cc	View	1 2 3	2 chunks	+3 lines, -3 lines	0 comments	Download
M	chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc	View	1 2 3 4	7 chunks	+12 lines, -9 lines	0 comments	Download
M	chrome/browser/sync/glue/autofill_profile_data_type_controller.h	View	1 2 3 4 5 6 7	2 chunks	+2 lines, -2 lines	0 comments	Download
M	chrome/browser/sync/glue/autofill_profile_data_type_controller.cc	View	1 2 3 4 5 6 7	3 chunks	+3 lines, -3 lines	0 comments	Download
M	chrome/browser/sync/glue/password_data_type_controller.cc	View	1 2 3 4 5 6 7	1 chunk	+0 lines, -1 line	0 comments	Download
M	chrome/browser/sync/profile_sync_components_factory_impl.h	View	1 2 3	2 chunks	+3 lines, -2 lines	0 comments	Download
M	chrome/browser/sync/profile_sync_components_factory_impl.cc	View	1 2 3 4 5 6 7 8	3 chunks	+2 lines, -2 lines	0 comments	Download
M	chrome/browser/sync/profile_sync_service_autofill_unittest.cc	View	1 2 3 4 5 6 7 8	13 chunks	+52 lines, -25 lines	0 comments	Download
M	chrome/browser/sync/test/integration/autofill_helper.h	View	1 2 3 4 5 6 7	2 chunks	+3 lines, -2 lines	0 comments	Download
M	chrome/browser/sync/test/integration/autofill_helper.cc	View	1 2 3 4 5 6 7	7 chunks	+8 lines, -8 lines	0 comments	Download
M	chrome/browser/webdata/DEPS	View	1 2 3 4 5 6 7 8	1 chunk	+28 lines, -13 lines	0 comments	Download
M	chrome/browser/webdata/autocomplete_syncable_service.h	View	1 2 3 4 5	4 chunks	+7 lines, -9 lines	0 comments	Download
M	chrome/browser/webdata/autocomplete_syncable_service.cc	View	1 2 3 4 5 6 7 8	6 chunks	+13 lines, -12 lines	0 comments	Download
M	chrome/browser/webdata/autofill_profile_syncable_service.h	View	1 2 3 4 5	4 chunks	+8 lines, -10 lines	0 comments	Download
M	chrome/browser/webdata/autofill_profile_syncable_service.cc	View	1 2 3 4 5 6 7 8	6 chunks	+12 lines, -11 lines	0 comments	Download
M	chrome/browser/webdata/autofill_web_data_service_impl.h	View	1 2 3 4 5	2 chunks	+41 lines, -13 lines	0 comments	Download
M	chrome/browser/webdata/autofill_web_data_service_impl.cc	View	1 2 3 4 5	1 chunk	+355 lines, -32 lines	0 comments	Download
M	chrome/browser/webdata/web_data_request_manager.cc	View	1 2 3 4 5	2 chunks	+1 line, -2 lines	0 comments	Download
M	chrome/browser/webdata/web_data_service.h	View	1 2 3 4 5	4 chunks	+2 lines, -5 lines	0 comments	Download
M	chrome/browser/webdata/web_data_service.cc	View	1 2 3 4 5	8 chunks	+9 lines, -9 lines	0 comments	Download
M	chrome/browser/webdata/web_data_service_base.cc	View	1 2 3 4 5 6 7 8	3 chunks	+4 lines, -7 lines	0 comments	Download
M	chrome/browser/webdata/web_data_service_factory.h	View	1 2 3 4 5 6 7 8	3 chunks	+13 lines, -5 lines	0 comments	Download
M	chrome/browser/webdata/web_data_service_factory.cc	View	1 2 3 4 5 6 7 8	6 chunks	+55 lines, -44 lines	0 comments	Download
M	chrome/browser/webdata/web_data_service_test_util.h	View	1 2 3 4 5 6 7 8	1 chunk	+8 lines, -3 lines	0 comments	Download
M	chrome/browser/webdata/web_data_service_test_util.cc	View	1 2 3 4 5	1 chunk	+13 lines, -3 lines	0 comments	Download
M	chrome/browser/webdata/web_data_service_unittest.cc	View	1 2 3 4 5 6 7 8	3 chunks	+11 lines, -3 lines	0 comments	Download
M	chrome/browser/webdata/web_database_service.h	View	1 2 3 4 5	3 chunks	+10 lines, -3 lines	0 comments	Download
M	chrome/browser/webdata/web_database_service.cc	View	1 2 3 4 5	2 chunks	+2 lines, -3 lines	0 comments	Download
M	components/autofill/browser/autocomplete_history_manager_unittest.cc	View	1 2 3 4 5	3 chunks	+5 lines, -4 lines	0 comments	Download
M	components/autofill/browser/autofill_metrics_unittest.cc	View	1 2 3 4 5 6 7	1 chunk	+1 line, -1 line	0 comments	Download
M	components/autofill/browser/personal_data_manager_unittest.cc	View	1 2 3 4 5 6 7 8	2 chunks	+3 lines, -3 lines	0 comments	Download

Messages

Total messages: 16 (0 generated)

Expand Messages | Collapse Messages

Jói

Note my comments are based on the first patchset, which seems to have been directly ...

7 years, 9 months ago (2013-03-19 20:53:58 UTC) #1

Jói

Not a full review, just some comments. Looks like this isn't quite ready, at least ...

7 years, 9 months ago (2013-03-20 12:41:29 UTC) #2

Cait (Slow)

https://codereview.chromium.org/12695015/diff/1/chrome/browser/api/webdata/autofill_web_data_service.h File chrome/browser/api/webdata/autofill_web_data_service.h (right): https://codereview.chromium.org/12695015/diff/1/chrome/browser/api/webdata/autofill_web_data_service.h#newcode17 chrome/browser/api/webdata/autofill_web_data_service.h:17: class AutofillWebDataService I'll wait for a later CL to ...

7 years, 9 months ago (2013-03-20 22:21:09 UTC) #3

Jói

I'm just starting my review now, but wanted to send you this update to the ...

7 years, 9 months ago (2013-03-20 22:51:13 UTC) #4

I'm just starting my review now, but wanted to send you this update to the
chrome/browser/webdata/DEPS file.  Could you apply it in your change, and fix
the 3 issues related to includes of web_data_service.h?  I think you just need
to remove the includes and use WebDataServiceBase::Handle instead of
WebDataService::Handle.

File follows:

include_rules = [
  # Webdata is being made into a component (it will end up at
  # //components/webdata and not depend on //chrome), so we have these basic
  # rules followed by temporary exceptions.  Please don't add to the list of
  # exceptions!
  "-chrome/browser",
  "-chrome/common",
  "+chrome/browser/api/webdata",
  "+chrome/browser/webdata",

  # Ensure the files that are going to be componentized don't depend
  # on the ones that aren't.
  "-chrome/browser/webdata/autocomplete_syncable_service.h",
  "-chrome/browser/webdata/autofill_profile_syncable_service.h",
  "-chrome/browser/webdata/web_data_service_factory.h",
  "-chrome/browser/webdata/web_data_service.h",

  # TODO(caitkp): Bring this list to zero.
  "!chrome/browser/password_manager/encryptor.h",
  "!chrome/browser/password_manager/ie7_password.h",
  "!chrome/browser/profiles/profile.h",
  "!chrome/common/chrome_constants.h",
  "!chrome/common/chrome_paths.h",

  # TODO(caitkp): Remove this, at slightly lower priority - this is
  # just a set of header-only enums, so we can live with it
  # temporarily even after we componentize.
  "!chrome/common/chrome_notification_types.h",
]

specific_include_rules = {
  # These files are staying in //chrome, so no need to restrict.
 
r'((autocomplete|autofill_profile)_syncable_service|web_data_service(_win)?(_factory)?)(_unittest)?\.(cc|h)':
[
    '+chrome/browser',
    '+chrome/common',
  ],

  # TODO(caitkp): Componentize all the different tables. For now, we
  # write these rules so they don't add bad dependencies.
  r'^(?!autofill)[a-z_]+_table(_unittest)?\.(cc|h)': [
    "!chrome/browser/history/history_database.h",
    "!chrome/browser/search_engines/search_terms_data.h",
    "!chrome/browser/search_engines/template_url.h",
    "!chrome/browser/search_engines/template_url_id.h",
    "!chrome/browser/search_engines/template_url_service.h",
  ],

  # TODO(caitkp): Extract unit tests from //chrome, at lower priority
  # than production code.
  r'(.*_unittest|.*_test_util)\.(cc|h)': [
    "+chrome/browser/webdata/web_data_service.h",
    "+chrome/browser/webdata/web_data_service_factory.h",
  ],
}

Jói

This is looking good to me, I think you can ask David to review once ...

7 years, 9 months ago (2013-03-20 23:09:10 UTC) #5

Cait (Slow)

David -- This is the actual autofillWebData split. Most of the changes are converting Autofill-related ...

7 years, 9 months ago (2013-03-21 14:17:55 UTC) #6

Cait (Slow)

Hey Jói, Which were the includes in web_data_service.h that were in violation of DEPS? when ...

7 years, 9 months ago (2013-03-21 16:10:52 UTC) #7

Hey Jói,
Which were the includes in web_data_service.h that were in violation of DEPS?
when I run the presubmit, the only one that comes up is
chrome/common/chrome_notification_types.h.

Thanks,
Cait


On 2013/03/20 22:51:13, Jói wrote:
> I'm just starting my review now, but wanted to send you this update to the
> chrome/browser/webdata/DEPS file.  Could you apply it in your change, and fix
> the 3 issues related to includes of web_data_service.h?  I think you just need
> to remove the includes and use WebDataServiceBase::Handle instead of
> WebDataService::Handle.
> 
> File follows:
> 
> include_rules = [
>   # Webdata is being made into a component (it will end up at
>   # //components/webdata and not depend on //chrome), so we have these basic
>   # rules followed by temporary exceptions.  Please don't add to the list of
>   # exceptions!
>   "-chrome/browser",
>   "-chrome/common",
>   "+chrome/browser/api/webdata",
>   "+chrome/browser/webdata",
> 
>   # Ensure the files that are going to be componentized don't depend
>   # on the ones that aren't.
>   "-chrome/browser/webdata/autocomplete_syncable_service.h",
>   "-chrome/browser/webdata/autofill_profile_syncable_service.h",
>   "-chrome/browser/webdata/web_data_service_factory.h",
>   "-chrome/browser/webdata/web_data_service.h",
> 
>   # TODO(caitkp): Bring this list to zero.
>   "!chrome/browser/password_manager/encryptor.h",
>   "!chrome/browser/password_manager/ie7_password.h",
>   "!chrome/browser/profiles/profile.h",
>   "!chrome/common/chrome_constants.h",
>   "!chrome/common/chrome_paths.h",
> 
>   # TODO(caitkp): Remove this, at slightly lower priority - this is
>   # just a set of header-only enums, so we can live with it
>   # temporarily even after we componentize.
>   "!chrome/common/chrome_notification_types.h",
> ]
> 
> specific_include_rules = {
>   # These files are staying in //chrome, so no need to restrict.
>  
>
r'((autocomplete|autofill_profile)_syncable_service|web_data_service(_win)?(_factory)?)(_unittest)?\.(cc|h)':
> [
>     '+chrome/browser',
>     '+chrome/common',
>   ],
> 
>   # TODO(caitkp): Componentize all the different tables. For now, we
>   # write these rules so they don't add bad dependencies.
>   r'^(?!autofill)[a-z_]+_table(_unittest)?\.(cc|h)': [
>     "!chrome/browser/history/history_database.h",
>     "!chrome/browser/search_engines/search_terms_data.h",
>     "!chrome/browser/search_engines/template_url.h",
>     "!chrome/browser/search_engines/template_url_id.h",
>     "!chrome/browser/search_engines/template_url_service.h",
>   ],
> 
>   # TODO(caitkp): Extract unit tests from //chrome, at lower priority
>   # than production code.
>   r'(.*_unittest|.*_test_util)\.(cc|h)': [
>     "+chrome/browser/webdata/web_data_service.h",
>     "+chrome/browser/webdata/web_data_service_factory.h",
>   ],
> }

dhollowa

On 2013/03/21 14:17:55, caitkp wrote: > David -- This is the actual autofillWebData split. Most ...

7 years, 9 months ago (2013-03-21 16:21:04 UTC) #8

dhollowa

Nits mostly... https://codereview.chromium.org/12695015/diff/29001/chrome/browser/api/webdata/autofill_web_data_service.h File chrome/browser/api/webdata/autofill_web_data_service.h (right): https://codereview.chromium.org/12695015/diff/29001/chrome/browser/api/webdata/autofill_web_data_service.h#newcode27 chrome/browser/api/webdata/autofill_web_data_service.h:27: const ProfileErrorCallback& callback); nit: Indent https://codereview.chromium.org/12695015/diff/29001/chrome/browser/api/webdata/web_data_service_base.h File ...

7 years, 9 months ago (2013-03-21 16:44:06 UTC) #9

Jói

> Hey Jói, > Which were the includes in web_data_service.h that were in violation of ...

7 years, 9 months ago (2013-03-21 16:59:32 UTC) #10

> Hey Jói,
> Which were the includes in web_data_service.h that were in violation of

There were three includes of web_data_service.h in files that should
be independent of that.  If they're gone, great.

Cheers,
Jói


On Thu, Mar 21, 2013 at 4:10 PM,  <caitkp@chromium.org> wrote:
> Hey Jói,
> Which were the includes in web_data_service.h that were in violation of
> DEPS?
> when I run the presubmit, the only one that comes up is
> chrome/common/chrome_notification_types.h.
>
> Thanks,
> Cait
>
>
>
> On 2013/03/20 22:51:13, Jói wrote:
>>
>> I'm just starting my review now, but wanted to send you this update to the
>> chrome/browser/webdata/DEPS file.  Could you apply it in your change, and
>> fix
>> the 3 issues related to includes of web_data_service.h?  I think you just
>> need
>> to remove the includes and use WebDataServiceBase::Handle instead of
>> WebDataService::Handle.
>
>
>> File follows:
>
>
>> include_rules = [
>>    # Webdata is being made into a component (it will end up at
>>    # //components/webdata and not depend on //chrome), so we have these
>> basic
>>    # rules followed by temporary exceptions.  Please don't add to the list
>> of
>>    # exceptions!
>>    "-chrome/browser",
>>    "-chrome/common",
>>    "+chrome/browser/api/webdata",
>>    "+chrome/browser/webdata",
>
>
>>    # Ensure the files that are going to be componentized don't depend
>>    # on the ones that aren't.
>>    "-chrome/browser/webdata/autocomplete_syncable_service.h",
>>    "-chrome/browser/webdata/autofill_profile_syncable_service.h",
>>    "-chrome/browser/webdata/web_data_service_factory.h",
>>    "-chrome/browser/webdata/web_data_service.h",
>
>
>>    # TODO(caitkp): Bring this list to zero.
>>    "!chrome/browser/password_manager/encryptor.h",
>>    "!chrome/browser/password_manager/ie7_password.h",
>>    "!chrome/browser/profiles/profile.h",
>>    "!chrome/common/chrome_constants.h",
>>    "!chrome/common/chrome_paths.h",
>
>
>>    # TODO(caitkp): Remove this, at slightly lower priority - this is
>>    # just a set of header-only enums, so we can live with it
>>    # temporarily even after we componentize.
>>    "!chrome/common/chrome_notification_types.h",
>> ]
>
>
>> specific_include_rules = {
>>    # These files are staying in //chrome, so no need to restrict.
>
>
>
>
r'((autocomplete|autofill_profile)_syncable_service|web_data_service(_win)?(_factory)?)(_unittest)?\.(cc|h)':
>>
>> [
>>      '+chrome/browser',
>>      '+chrome/common',
>>    ],
>
>
>>    # TODO(caitkp): Componentize all the different tables. For now, we
>>    # write these rules so they don't add bad dependencies.
>>    r'^(?!autofill)[a-z_]+_table(_unittest)?\.(cc|h)': [
>>      "!chrome/browser/history/history_database.h",
>>      "!chrome/browser/search_engines/search_terms_data.h",
>>      "!chrome/browser/search_engines/template_url.h",
>>      "!chrome/browser/search_engines/template_url_id.h",
>>      "!chrome/browser/search_engines/template_url_service.h",
>>    ],
>
>
>>    # TODO(caitkp): Extract unit tests from //chrome, at lower priority
>>    # than production code.
>>    r'(.*_unittest|.*_test_util)\.(cc|h)': [
>>      "+chrome/browser/webdata/web_data_service.h",
>>      "+chrome/browser/webdata/web_data_service_factory.h",
>>    ],
>> }
>
>
>
> https://codereview.chromium.org/12695015/

Cait (Slow)

Unit tests are running on: https://codereview.chromium.org/12897009/ (as this CL won't patch cleanly onto the trunk ...

7 years, 9 months ago (2013-03-21 23:23:27 UTC) #11

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/12695015/70011

7 years, 9 months ago (2013-03-24 22:02:28 UTC) #14

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/12695015/42070

7 years, 9 months ago (2013-03-24 23:21:12 UTC) #15

Message was sent while issue was closed.

Change committed as 190317

Expand Messages | Collapse Messages