Eliminate AutofillWebDataService::FromBrowserContext().

Eliminate AutofillWebDataService::FromBrowserContext().

Core code in the Autofill component now gets the AutofillWebDataService from
the embedder. This CL eliminates the declaration of the static
AutofillWebDataService::FromBrowserContext() from the Autofill component's core
code, instead having the concept of getting an AutofillWebDataService from a
BrowserContext/Profile be entirely an embedder-level concept.

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

[blundell, 2013-11-14 16:56:45]

This CL is dependent on https://codereview.chromium.org/71683003/, which
eliminates the usage of AutofillWebDataService::FromBrowserContext() within core
code of the Autofill component.

joi@: the CL as a whole
isherman@: Autofill/whole CL
pkasting@: //chrome/browser/webdata
benm@: //android_webview

[blundell, 2013-11-14 17:46:33]

zea@: //chrome/browser/sync

[blundell, 2013-11-14 17:46:52]

+zea for real.

[Ilya Sherman, 2013-11-14 23:54:33]

LGTM

https://codereview.chromium.org/72613006/diff/1/android_webview/native/aw_aut...
File android_webview/native/aw_autofill_manager_delegate.cc (right):

https://codereview.chromium.org/72613006/diff/1/android_webview/native/aw_aut...
android_webview/native/aw_autofill_manager_delegate.cc:80: DCHECK(service);
nit: This is redundant with just making a method call on the service on the line
below, IMO.

https://codereview.chromium.org/72613006/diff/1/chrome/browser/sync/glue/auto...
File chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc (right):

https://codereview.chromium.org/72613006/diff/1/chrome/browser/sync/glue/auto...
chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc:258:
WebDataServiceFactory::GetAutofillWebDataForProfile(&profile_).get());
nit: This line should be indented two more spaces.

[Nicolas Zea, 2013-11-15 01:19:38]

sync LGTM

[Peter Kasting, 2013-11-15 01:30:59]

https://codereview.chromium.org/72613006/diff/1/chrome/browser/webdata/web_da...
File chrome/browser/webdata/web_data_service_factory.cc (right):

https://codereview.chromium.org/72613006/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_service_factory.cc:206:
WebDataServiceFactory::GetAutofillWebDataForProfile(Profile* profile) {
Nit: Indent 4

https://codereview.chromium.org/72613006/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_service_factory.cc:209: // cheat and always say
EXPLICIT_ACCESS.
"Just a DCHECK" sounds kind of worrisome; DCHECKs usually indicate some state
that's supposed to hold.  If this distinction _truly_ doesn't matter, the
service shouldn't be DCHECKing it.  Otherwise, this function should take an
access parameter from the caller and pass it on to GetForProfile().

I realize you're merely moving this code, but since you're touching it, it
should be fixed.

If this would be super-hairy, it's OK to do it in a separate CL -- but I'd still
want to see it happen.

https://codereview.chromium.org/72613006/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_service_factory.cc:215: return
scoped_refptr<AutofillWebDataService>(NULL);
Nit: Simpler:

  // |wrapper| can be NULL in Incognito.
  return wrapper ?
      wrapper->GetAutofillWebData() :
      scoped_refptr<AutofillWebDataService>(NULL);

https://codereview.chromium.org/72613006/diff/1/chrome/browser/webdata/web_da...
File chrome/browser/webdata/web_data_service_factory.h (right):

https://codereview.chromium.org/72613006/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_service_factory.h:67:
GetAutofillWebDataForProfile(Profile* profile);
This seems like a random place for this.  Why wouldn't this be a static method
on either AutofillWebDataService?

[Peter Kasting, 2013-11-15 01:35:58]

https://codereview.chromium.org/72613006/diff/1/chrome/browser/webdata/web_da...
File chrome/browser/webdata/web_data_service_factory.h (right):

https://codereview.chromium.org/72613006/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_service_factory.h:67:
GetAutofillWebDataForProfile(Profile* profile);
On 2013/11/15 01:30:59, Peter Kasting wrote:
> This seems like a random place for this.  Why wouldn't this be a static method
> on either AutofillWebDataService?

Belay that, I should learn to read your comments first.

Two followup questions:
(1) Should this be on WebDataServiceWrapper instead, since that's really the
class that owns this?
(2) Can we at least add a comment as to why this isn't defined on
AutofillWebDataService itself?

I am still uneasy about this path overall.  It seems like this should live on
something more directly autofill-related, but I don't know what that might be.

[blundell, 2013-11-15 06:46:33]

On 2013/11/15 01:35:58, Peter Kasting wrote:
>
https://codereview.chromium.org/72613006/diff/1/chrome/browser/webdata/web_da...
> File chrome/browser/webdata/web_data_service_factory.h (right):
> 
>
https://codereview.chromium.org/72613006/diff/1/chrome/browser/webdata/web_da...
> chrome/browser/webdata/web_data_service_factory.h:67:
> GetAutofillWebDataForProfile(Profile* profile);
> On 2013/11/15 01:30:59, Peter Kasting wrote:
> > This seems like a random place for this.  Why wouldn't this be a static
method
> > on either AutofillWebDataService?
> 
> Belay that, I should learn to read your comments first.
> 
> Two followup questions:
> (1) Should this be on WebDataServiceWrapper instead, since that's really the
> class that owns this?
> (2) Can we at least add a comment as to why this isn't defined on
> AutofillWebDataService itself?
> 
> I am still uneasy about this path overall.  It seems like this should live on
> something more directly autofill-related, but I don't know what that might be.

PK/Ilya/Jói: 

It could be a static method on WebDataServiceWrapper instead, and I should
definitely add a comment. Alternatively, is it worth creating an
AutofillWebDataServiceFactory that would depend on the WebDataServiceFactory?
Then these calls could just be AutofillWebDataServiceFactory::GetForProfile() at
the expense of some more factory code.

[Jói, 2013-11-15 13:25:33]

LGTM

W.r.t. Peter's comment: On the embedder side, i.e. in //chrome/browser, all of
the WebData subtypes need to be instantiated and owned by the same wrapper, so
that they can share a single database. This is why there are already getters for
AutofillWebDataService and others on the WebDataServiceWrapper in
web_data_service.h. So I don't think it's really a random place to have a
GetAutofillWebDataForProfile call, although it is perhaps a non-obvious one.

Cheers,
Jói

https://codereview.chromium.org/72613006/diff/1/chrome/browser/webdata/web_da...
File chrome/browser/webdata/web_data_service_factory.cc (right):

https://codereview.chromium.org/72613006/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_service_factory.cc:209: // cheat and always say
EXPLICIT_ACCESS.
On 2013/11/15 01:30:59, Peter Kasting wrote:
> "Just a DCHECK" sounds kind of worrisome; DCHECKs usually indicate some state
> that's supposed to hold.  If this distinction _truly_ doesn't matter, the
> service shouldn't be DCHECKing it.  Otherwise, this function should take an
> access parameter from the caller and pass it on to GetForProfile().
> 
> I realize you're merely moving this code, but since you're touching it, it
> should be fixed.
> 
> If this would be super-hairy, it's OK to do it in a separate CL -- but I'd
still
> want to see it happen.

It would be OK to bring back the parameter and DCHECK now, since this code won't
be moving out of //chrome.

OTOH, all the DCHECK does is verify that the caller passed in a certain
parameter rather than a different parameter, and there is no code to handle what
would happen for a different value of the parameter.

[blundell, 2013-11-15 14:01:47]

On 2013/11/15 13:25:33, Jói wrote:
> LGTM
> 
> W.r.t. Peter's comment: On the embedder side, i.e. in //chrome/browser, all of
> the WebData subtypes need to be instantiated and owned by the same wrapper, so
> that they can share a single database. This is why there are already getters
for
> AutofillWebDataService and others on the WebDataServiceWrapper in
> web_data_service.h. So I don't think it's really a random place to have a
> GetAutofillWebDataForProfile call, although it is perhaps a non-obvious one.
> 
> Cheers,
> Jói
> 
>
https://codereview.chromium.org/72613006/diff/1/chrome/browser/webdata/web_da...
> File chrome/browser/webdata/web_data_service_factory.cc (right):
> 
>
https://codereview.chromium.org/72613006/diff/1/chrome/browser/webdata/web_da...
> chrome/browser/webdata/web_data_service_factory.cc:209: // cheat and always
say
> EXPLICIT_ACCESS.
> On 2013/11/15 01:30:59, Peter Kasting wrote:
> > "Just a DCHECK" sounds kind of worrisome; DCHECKs usually indicate some
state
> > that's supposed to hold.  If this distinction _truly_ doesn't matter, the
> > service shouldn't be DCHECKing it.  Otherwise, this function should take an
> > access parameter from the caller and pass it on to GetForProfile().
> > 
> > I realize you're merely moving this code, but since you're touching it, it
> > should be fixed.
> > 
> > If this would be super-hairy, it's OK to do it in a separate CL -- but I'd
> still
> > want to see it happen.
> 
> It would be OK to bring back the parameter and DCHECK now, since this code
won't
> be moving out of //chrome.
> 
> OTOH, all the DCHECK does is verify that the caller passed in a certain
> parameter rather than a different parameter, and there is no code to handle
what
> would happen for a different value of the parameter.

Jói, given your explanation, do you think it would be better to have a static
WebDataServiceWrapper::GetAutofillWebDataForProfile(Profile* profile) method
instead of having it be on WebDataServiceFactory?

[Jói, 2013-11-15 14:08:07]

I don't think that would be any cleaner.

Maybe cleanest would be to just call
WebDataServiceFactory::GetForProfile(...)->GetAutofillWebData()
everywhere.

On Fri, Nov 15, 2013 at 2:01 PM,  <blundell@chromium.org> wrote:
> On 2013/11/15 13:25:33, Jói wrote:
>>
>> LGTM
>
>
>> W.r.t. Peter's comment: On the embedder side, i.e. in //chrome/browser,
>> all of
>> the WebData subtypes need to be instantiated and owned by the same
>> wrapper, so
>> that they can share a single database. This is why there are already
>> getters
>
> for
>>
>> AutofillWebDataService and others on the WebDataServiceWrapper in
>> web_data_service.h. So I don't think it's really a random place to have a
>> GetAutofillWebDataForProfile call, although it is perhaps a non-obvious
>> one.
>
>
>> Cheers,
>> Jói
>
>
>
>
https://codereview.chromium.org/72613006/diff/1/chrome/browser/webdata/web_da...
>>
>> File chrome/browser/webdata/web_data_service_factory.cc (right):
>
>
>
>
https://codereview.chromium.org/72613006/diff/1/chrome/browser/webdata/web_da...
>>
>> chrome/browser/webdata/web_data_service_factory.cc:209: // cheat and
>> always
>
> say
>>
>> EXPLICIT_ACCESS.
>> On 2013/11/15 01:30:59, Peter Kasting wrote:
>> > "Just a DCHECK" sounds kind of worrisome; DCHECKs usually indicate some
>
> state
>>
>> > that's supposed to hold.  If this distinction _truly_ doesn't matter,
>> > the
>> > service shouldn't be DCHECKing it.  Otherwise, this function should take
>> > an
>> > access parameter from the caller and pass it on to GetForProfile().
>> >
>> > I realize you're merely moving this code, but since you're touching it,
>> > it
>> > should be fixed.
>> >
>> > If this would be super-hairy, it's OK to do it in a separate CL -- but
>> > I'd
>> still
>> > want to see it happen.
>
>
>> It would be OK to bring back the parameter and DCHECK now, since this code
>
> won't
>>
>> be moving out of //chrome.
>
>
>> OTOH, all the DCHECK does is verify that the caller passed in a certain
>> parameter rather than a different parameter, and there is no code to
>> handle
>
> what
>>
>> would happen for a different value of the parameter.
>
>
> Jói, given your explanation, do you think it would be better to have a
> static
> WebDataServiceWrapper::GetAutofillWebDataForProfile(Profile* profile) method
> instead of having it be on WebDataServiceFactory?
>
> https://codereview.chromium.org/72613006/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[blundell, 2013-11-15 14:25:35]

Whatever we decide here will set the pattern for the other
"*WebDataService::FromBrowserContext()" methods (which I assume componentization
will likely reach its tentacles into in due time).

I'm fine with any approach that Jói and PK agree on.

On 2013/11/15 14:08:07, Jói wrote:
> I don't think that would be any cleaner.
> 
> Maybe cleanest would be to just call
> WebDataServiceFactory::GetForProfile(...)->GetAutofillWebData()
> everywhere.
> 
> On Fri, Nov 15, 2013 at 2:01 PM,  <mailto:blundell@chromium.org> wrote:
> > On 2013/11/15 13:25:33, Jói wrote:
> >>
> >> LGTM
> >
> >
> >> W.r.t. Peter's comment: On the embedder side, i.e. in //chrome/browser,
> >> all of
> >> the WebData subtypes need to be instantiated and owned by the same
> >> wrapper, so
> >> that they can share a single database. This is why there are already
> >> getters
> >
> > for
> >>
> >> AutofillWebDataService and others on the WebDataServiceWrapper in
> >> web_data_service.h. So I don't think it's really a random place to have a
> >> GetAutofillWebDataForProfile call, although it is perhaps a non-obvious
> >> one.
> >
> >
> >> Cheers,
> >> Jói
> >
> >
> >
> >
>
https://codereview.chromium.org/72613006/diff/1/chrome/browser/webdata/web_da...
> >>
> >> File chrome/browser/webdata/web_data_service_factory.cc (right):
> >
> >
> >
> >
>
https://codereview.chromium.org/72613006/diff/1/chrome/browser/webdata/web_da...
> >>
> >> chrome/browser/webdata/web_data_service_factory.cc:209: // cheat and
> >> always
> >
> > say
> >>
> >> EXPLICIT_ACCESS.
> >> On 2013/11/15 01:30:59, Peter Kasting wrote:
> >> > "Just a DCHECK" sounds kind of worrisome; DCHECKs usually indicate some
> >
> > state
> >>
> >> > that's supposed to hold.  If this distinction _truly_ doesn't matter,
> >> > the
> >> > service shouldn't be DCHECKing it.  Otherwise, this function should take
> >> > an
> >> > access parameter from the caller and pass it on to GetForProfile().
> >> >
> >> > I realize you're merely moving this code, but since you're touching it,
> >> > it
> >> > should be fixed.
> >> >
> >> > If this would be super-hairy, it's OK to do it in a separate CL -- but
> >> > I'd
> >> still
> >> > want to see it happen.
> >
> >
> >> It would be OK to bring back the parameter and DCHECK now, since this code
> >
> > won't
> >>
> >> be moving out of //chrome.
> >
> >
> >> OTOH, all the DCHECK does is verify that the caller passed in a certain
> >> parameter rather than a different parameter, and there is no code to
> >> handle
> >
> > what
> >>
> >> would happen for a different value of the parameter.
> >
> >
> > Jói, given your explanation, do you think it would be better to have a
> > static
> > WebDataServiceWrapper::GetAutofillWebDataForProfile(Profile* profile) method
> > instead of having it be on WebDataServiceFactory?
> >
> > https://codereview.chromium.org/72613006/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Jói, 2013-11-15 16:53:23]

>> Maybe cleanest would be to just call
>> WebDataServiceFactory::GetForProfile(...)->GetAutofillWebData()
>> everywhere.

Peter, I agree on my approach ;)  Do you?

Cheers,
Jói


On Fri, Nov 15, 2013 at 2:25 PM,  <blundell@chromium.org> wrote:
> Whatever we decide here will set the pattern for the other
> "*WebDataService::FromBrowserContext()" methods (which I assume
> componentization
> will likely reach its tentacles into in due time).
>
> I'm fine with any approach that Jói and PK agree on.
>
>
> On 2013/11/15 14:08:07, Jói wrote:
>>
>> I don't think that would be any cleaner.
>
>
>> Maybe cleanest would be to just call
>> WebDataServiceFactory::GetForProfile(...)->GetAutofillWebData()
>> everywhere.
>
>
>> On Fri, Nov 15, 2013 at 2:01 PM,  <mailto:blundell@chromium.org> wrote:
>> > On 2013/11/15 13:25:33, Jói wrote:
>> >>
>> >> LGTM
>> >
>> >
>> >> W.r.t. Peter's comment: On the embedder side, i.e. in //chrome/browser,
>> >> all of
>> >> the WebData subtypes need to be instantiated and owned by the same
>> >> wrapper, so
>> >> that they can share a single database. This is why there are already
>> >> getters
>> >
>> > for
>> >>
>> >> AutofillWebDataService and others on the WebDataServiceWrapper in
>> >> web_data_service.h. So I don't think it's really a random place to have
>> >> a
>> >> GetAutofillWebDataForProfile call, although it is perhaps a non-obvious
>> >> one.
>> >
>> >
>> >> Cheers,
>> >> Jói
>> >
>> >
>> >
>> >
>
>
>
https://codereview.chromium.org/72613006/diff/1/chrome/browser/webdata/web_da...
>>
>> >>
>> >> File chrome/browser/webdata/web_data_service_factory.cc (right):
>> >
>> >
>> >
>> >
>
>
>
https://codereview.chromium.org/72613006/diff/1/chrome/browser/webdata/web_da...
>>
>> >>
>> >> chrome/browser/webdata/web_data_service_factory.cc:209: // cheat and
>> >> always
>> >
>> > say
>> >>
>> >> EXPLICIT_ACCESS.
>> >> On 2013/11/15 01:30:59, Peter Kasting wrote:
>> >> > "Just a DCHECK" sounds kind of worrisome; DCHECKs usually indicate
>> >> > some
>> >
>> > state
>> >>
>> >> > that's supposed to hold.  If this distinction _truly_ doesn't matter,
>> >> > the
>> >> > service shouldn't be DCHECKing it.  Otherwise, this function should
>> >> > take
>> >> > an
>> >> > access parameter from the caller and pass it on to GetForProfile().
>> >> >
>> >> > I realize you're merely moving this code, but since you're touching
>> >> > it,
>> >> > it
>> >> > should be fixed.
>> >> >
>> >> > If this would be super-hairy, it's OK to do it in a separate CL --
>> >> > but
>> >> > I'd
>> >> still
>> >> > want to see it happen.
>> >
>> >
>> >> It would be OK to bring back the parameter and DCHECK now, since this
>> >> code
>> >
>> > won't
>> >>
>> >> be moving out of //chrome.
>> >
>> >
>> >> OTOH, all the DCHECK does is verify that the caller passed in a certain
>> >> parameter rather than a different parameter, and there is no code to
>> >> handle
>> >
>> > what
>> >>
>> >> would happen for a different value of the parameter.
>> >
>> >
>> > Jói, given your explanation, do you think it would be better to have a
>> > static
>> > WebDataServiceWrapper::GetAutofillWebDataForProfile(Profile* profile)
>> > method
>> > instead of having it be on WebDataServiceFactory?
>> >
>> > https://codereview.chromium.org/72613006/
>
>
>> To unsubscribe from this group and stop receiving emails from it, send an
>
> email
>>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>
>
> https://codereview.chromium.org/72613006/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Kasting, 2013-11-15 22:41:46]

On 2013/11/15 16:53:23, Jói wrote:
> >> Maybe cleanest would be to just call
> >> WebDataServiceFactory::GetForProfile(...)->GetAutofillWebData()
> >> everywhere.
> 
> Peter, I agree on my approach ;)  Do you?

I have no strong opinion.  You guys understand the relationships of these
components better than I do.  If the AutofillWebData object is owned by the web
data service, and there are no differences between them in terms of how they're
instantiated for incognito, then this seems like a good route.

This would also avoid the need for the access type param and DCHECK, which would
be nice.

[blundell, 2013-11-18 09:55:22]

Oops, I just realized the flaw in this approach: the returned
WebDataServiceWrapper can be NULL, so the entire implementation of the static
method would have to be inlined everywhere.

Based on this discussion + that fact, it seems like the way to go is to keep
this static method in WebDataServiceFactory (per joi@'s opinion), and to add the
access type parameter to it. Agreed?

On 2013/11/15 22:41:46, Peter Kasting wrote:
> On 2013/11/15 16:53:23, Jói wrote:
> > >> Maybe cleanest would be to just call
> > >> WebDataServiceFactory::GetForProfile(...)->GetAutofillWebData()
> > >> everywhere.
> > 
> > Peter, I agree on my approach ;)  Do you?
> 
> I have no strong opinion.  You guys understand the relationships of these
> components better than I do.  If the AutofillWebData object is owned by the
web
> data service, and there are no differences between them in terms of how
they're
> instantiated for incognito, then this seems like a good route.
> 
> This would also avoid the need for the access type param and DCHECK, which
would
> be nice.

[Jói, 2013-11-18 11:29:47]

Ah, ok. Yes, keep it on WebDataServiceFactory I think.

On Mon, Nov 18, 2013 at 9:55 AM,  <blundell@chromium.org> wrote:
> Oops, I just realized the flaw in this approach: the returned
> WebDataServiceWrapper can be NULL, so the entire implementation of the
> static
> method would have to be inlined everywhere.
>
> Based on this discussion + that fact, it seems like the way to go is to keep
> this static method in WebDataServiceFactory (per joi@'s opinion), and to add
> the
> access type parameter to it. Agreed?
>
>
> On 2013/11/15 22:41:46, Peter Kasting wrote:
>>
>> On 2013/11/15 16:53:23, Jói wrote:
>> > >> Maybe cleanest would be to just call
>> > >> WebDataServiceFactory::GetForProfile(...)->GetAutofillWebData()
>> > >> everywhere.
>> >
>> > Peter, I agree on my approach ;)  Do you?
>
>
>> I have no strong opinion.  You guys understand the relationships of these
>> components better than I do.  If the AutofillWebData object is owned by
>> the
>
> web
>>
>> data service, and there are no differences between them in terms of how
>
> they're
>>
>> instantiated for incognito, then this seems like a good route.
>
>
>> This would also avoid the need for the access type param and DCHECK, which
>
> would
>>
>> be nice.
>
>
>
>
> https://codereview.chromium.org/72613006/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[blundell, 2013-11-19 14:05:37]

All comments addressed, thanks!

https://codereview.chromium.org/72613006/diff/1/android_webview/native/aw_aut...
File android_webview/native/aw_autofill_manager_delegate.cc (right):

https://codereview.chromium.org/72613006/diff/1/android_webview/native/aw_aut...
android_webview/native/aw_autofill_manager_delegate.cc:80: DCHECK(service);
On 2013/11/14 23:54:33, Ilya Sherman wrote:
> nit: This is redundant with just making a method call on the service on the
line
> below, IMO.

Done.

https://codereview.chromium.org/72613006/diff/1/chrome/browser/sync/glue/auto...
File chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc (right):

https://codereview.chromium.org/72613006/diff/1/chrome/browser/sync/glue/auto...
chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc:258:
WebDataServiceFactory::GetAutofillWebDataForProfile(&profile_).get());
On 2013/11/14 23:54:33, Ilya Sherman wrote:
> nit: This line should be indented two more spaces.

Done.

https://codereview.chromium.org/72613006/diff/1/chrome/browser/webdata/web_da...
File chrome/browser/webdata/web_data_service_factory.cc (right):

https://codereview.chromium.org/72613006/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_service_factory.cc:206:
WebDataServiceFactory::GetAutofillWebDataForProfile(Profile* profile) {
No indentation after return type:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
(in the "Some points to note" section).

On 2013/11/15 01:30:59, Peter Kasting wrote:
> Nit: Indent 4

https://codereview.chromium.org/72613006/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_service_factory.cc:209: // cheat and always say
EXPLICIT_ACCESS.
Added in the parameter.

On 2013/11/15 13:25:34, Jói wrote:
> On 2013/11/15 01:30:59, Peter Kasting wrote:
> > "Just a DCHECK" sounds kind of worrisome; DCHECKs usually indicate some
state
> > that's supposed to hold.  If this distinction _truly_ doesn't matter, the
> > service shouldn't be DCHECKing it.  Otherwise, this function should take an
> > access parameter from the caller and pass it on to GetForProfile().
> > 
> > I realize you're merely moving this code, but since you're touching it, it
> > should be fixed.
> > 
> > If this would be super-hairy, it's OK to do it in a separate CL -- but I'd
> still
> > want to see it happen.
> 
> It would be OK to bring back the parameter and DCHECK now, since this code
won't
> be moving out of //chrome.
> 
> OTOH, all the DCHECK does is verify that the caller passed in a certain
> parameter rather than a different parameter, and there is no code to handle
what
> would happen for a different value of the parameter.

https://codereview.chromium.org/72613006/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_service_factory.cc:215: return
scoped_refptr<AutofillWebDataService>(NULL);
On 2013/11/15 01:30:59, Peter Kasting wrote:
> Nit: Simpler:
> 
>   // |wrapper| can be NULL in Incognito.
>   return wrapper ?
>       wrapper->GetAutofillWebData() :
>       scoped_refptr<AutofillWebDataService>(NULL);

Done.

https://codereview.chromium.org/72613006/diff/1/chrome/browser/webdata/web_da...
File chrome/browser/webdata/web_data_service_factory.h (right):

https://codereview.chromium.org/72613006/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_service_factory.h:67:
GetAutofillWebDataForProfile(Profile* profile);
(1) Via discussion on this review thread, resolved that this location is
actually the most appropriate place for this getter.
(2) Comment added.

On 2013/11/15 01:35:59, Peter Kasting wrote:
> On 2013/11/15 01:30:59, Peter Kasting wrote:
> > This seems like a random place for this.  Why wouldn't this be a static
method
> > on either AutofillWebDataService?
> 
> Belay that, I should learn to read your comments first.
> 
> Two followup questions:
> (1) Should this be on WebDataServiceWrapper instead, since that's really the
> class that owns this?
> (2) Can we at least add a comment as to why this isn't defined on
> AutofillWebDataService itself?
> 
> I am still uneasy about this path overall.  It seems like this should live on
> something more directly autofill-related, but I don't know what that might be.

[blundell, 2013-11-19 14:06:34]

+joth@ for android_webview/

[Peter Kasting, 2013-11-19 19:14:14]

LGTM

https://codereview.chromium.org/72613006/diff/540001/chrome/browser/webdata/w...
File chrome/browser/webdata/web_data_service_factory.cc (right):

https://codereview.chromium.org/72613006/diff/540001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.cc:184: Profile* profile,
Profile::ServiceAccessType access_type) {
Nit: While here: Same comment about one arg per line (3 places)

https://codereview.chromium.org/72613006/diff/540001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.cc:206:
WebDataServiceFactory::GetAutofillWebDataForProfile(
Nit: Indent 4

https://codereview.chromium.org/72613006/diff/540001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.cc:212:
wrapper->GetAutofillWebData() :
Nit: Indent 4, not 2

https://codereview.chromium.org/72613006/diff/540001/chrome/browser/webdata/w...
File chrome/browser/webdata/web_data_service_factory.h (right):

https://codereview.chromium.org/72613006/diff/540001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.h:56: // Returns the
|WebDataServiceWrapper| associated with the |profile|.
Nit: While here: remove pipes around type name (these are only used for variable
names).  (2 places)

https://codereview.chromium.org/72613006/diff/540001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.h:58: // (see its definition).
Nit: While here: Remove this last sentence enumerating the values of the enum. 
If someone doesn't understand access types they can figure out how to look up
the declaration.  (2 places)

https://codereview.chromium.org/72613006/diff/540001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.h:60: Profile* profile,
Profile::ServiceAccessType access_type);
Nit: While here: This is legal, but I tend to prefer one arg per line if the
whole function declaration doesn't fit on one line.  (2 places)

https://codereview.chromium.org/72613006/diff/540001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.h:67: // BrowserContext/Profile
and thus cannot define this getter itself).
Nit: Just:

  // Returns the AutofillWebDataService associated with the |profile|.

Once AutofillWebDataService is a layered component it won't even be able to
compile if someone adds a browser context dependency, so there's no need for the
rest of the comment.

https://codereview.chromium.org/72613006/diff/540001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.h:70: static
scoped_refptr<autofill::AutofillWebDataService>
I thought our current recommendation on scoped_refptr was to return it by raw
pointer until we'd eliminated the implicit refptr->raw pointer conversion.

[blundell, 2013-12-02 16:10:59]

Thanks for the reviews.

joth@/benm@: ping for android_webview.
mkwst@: //chrome/browser/browsing_data

https://codereview.chromium.org/72613006/diff/540001/chrome/browser/webdata/w...
File chrome/browser/webdata/web_data_service_factory.cc (right):

https://codereview.chromium.org/72613006/diff/540001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.cc:184: Profile* profile,
Profile::ServiceAccessType access_type) {
On 2013/11/19 19:14:15, Peter Kasting wrote:
> Nit: While here: Same comment about one arg per line (3 places)

Done.

https://codereview.chromium.org/72613006/diff/540001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.cc:206:
WebDataServiceFactory::GetAutofillWebDataForProfile(
No indentation after return type:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
("Some points to note" section).

On 2013/11/19 19:14:15, Peter Kasting wrote:
> Nit: Indent 4

https://codereview.chromium.org/72613006/diff/540001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.cc:212:
wrapper->GetAutofillWebData() :
On 2013/11/19 19:14:15, Peter Kasting wrote:
> Nit: Indent 4, not 2

Done.

https://codereview.chromium.org/72613006/diff/540001/chrome/browser/webdata/w...
File chrome/browser/webdata/web_data_service_factory.h (right):

https://codereview.chromium.org/72613006/diff/540001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.h:56: // Returns the
|WebDataServiceWrapper| associated with the |profile|.
On 2013/11/19 19:14:15, Peter Kasting wrote:
> Nit: While here: remove pipes around type name (these are only used for
variable
> names).  (2 places)

Done.

https://codereview.chromium.org/72613006/diff/540001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.h:58: // (see its definition).
On 2013/11/19 19:14:15, Peter Kasting wrote:
> Nit: While here: Remove this last sentence enumerating the values of the enum.

> If someone doesn't understand access types they can figure out how to look up
> the declaration.  (2 places)

Done.

https://codereview.chromium.org/72613006/diff/540001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.h:60: Profile* profile,
Profile::ServiceAccessType access_type);
On 2013/11/19 19:14:15, Peter Kasting wrote:
> Nit: While here: This is legal, but I tend to prefer one arg per line if the
> whole function declaration doesn't fit on one line.  (2 places)

Done.

https://codereview.chromium.org/72613006/diff/540001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.h:67: // BrowserContext/Profile
and thus cannot define this getter itself).
On 2013/11/19 19:14:15, Peter Kasting wrote:
> Nit: Just:
> 
>   // Returns the AutofillWebDataService associated with the |profile|.
> 
> Once AutofillWebDataService is a layered component it won't even be able to
> compile if someone adds a browser context dependency, so there's no need for
the
> rest of the comment.

Done.

https://codereview.chromium.org/72613006/diff/540001/chrome/browser/webdata/w...
chrome/browser/webdata/web_data_service_factory.h:70: static
scoped_refptr<autofill::AutofillWebDataService>
Yes, I filed crbug.com/321059. I'd rather keep those changes separate from this
refactoring to help ensure that this CL doesn't introduce any functional changes
(note that the API I'm moving away from is returning a scoped_refptr).

On 2013/11/19 19:14:15, Peter Kasting wrote:
> I thought our current recommendation on scoped_refptr was to return it by raw
> pointer until we'd eliminated the implicit refptr->raw pointer conversion.

[Mike West, 2013-12-02 16:26:24]

browsing_data LGTM.

[blundell, 2013-12-03 09:12:46]

torne@: Could you take a look at the //android_webview changes here? Thanks!

[Torne, 2013-12-03 10:57:06]

android_webview LGTM

[commit-bot: I haz the power, 2013-12-03 11:20:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/72613006/790001

[commit-bot: I haz the power, 2013-12-03 14:08:34]

Change committed as 238389