Moved WebDataService to ProfileKeyedService

Moved WebDataService to ProfileKeyedService

James:
chrome\browser\ui\intents
Peter:
chrome\browser\ui\search_engines
chrome\browser\search_engines
Nicolas:
chrome\browser\sync
Rachel:
chrome\browser\profiles and the whole cl

BUG=112234
TEST=unit-tests

[Ilya Sherman, 2012-03-27 01:01:17]

Drive-by with nits; otherwise, LG.  Thanks, George!

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/auto...
File chrome/browser/autocomplete/search_provider_unittest.cc (right):

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/auto...
chrome/browser/autocomplete/search_provider_unittest.cc:32: static
scoped_refptr<RefcountedProfileKeyedService> BuildNull(
nit: No need to declare this method static, since it's already in an anonymous
namespace.

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/pass...
File chrome/browser/password_manager/password_store_default.h (right):

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/pass...
chrome/browser/password_manager/password_store_default.h:15: #include
"chrome/browser/webdata/web_data_service.h"
nit: Why does this now need to be a #include rather than a forward-declaration?

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/sear...
File chrome/browser/search_engines/template_url_service.cc (right):

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/sear...
chrome/browser/search_engines/template_url_service.cc:494:
Profile::EXPLICIT_ACCESS);
nit: Please add curly braces.

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/webd...
File chrome/browser/webdata/web_data_service.h (right):

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/webd...
chrome/browser/webdata/web_data_service.h:306: virtual void
ShutdownOnUIThread();
nit: Please annotate this with OVERRIDE

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/webd...
File chrome/browser/webdata/web_data_service_factory.cc (right):

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/webd...
chrome/browser/webdata/web_data_service_factory.cc:25: NOTREACHED() << "This
profile is OffTheRecord";
nit: Please remove the logged comment, as it is clearly deducible from the code,
but if included as a string adds to the binary size.

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/webd...
chrome/browser/webdata/web_data_service_factory.cc:36: NOTREACHED() << "This
profile is OffTheRecord";
nit: Ditto.

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/webd...
chrome/browser/webdata/web_data_service_factory.cc:61: NOTREACHED() << "failed
to initialize WebService";
nit: Ditto.

[rpetterson, 2012-03-27 01:15:06]

George, thanks so much for taking care of this!

You'll also need to add the factory and dependency to the PKS Dependency
Manager: chrome/browser/profiles/profile_dependency_manager.cc

http://codereview.chromium.org/9834056/diff/10001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_metrics_unittest.cc (right):

http://codereview.chromium.org/9834056/diff/10001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_metrics_unittest.cc:933:
personal_data_.Shutdown();
Why is this needed now (as opposed to before)?

http://codereview.chromium.org/9834056/diff/10001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/9834056/diff/10001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:592: if (web_data_service_.get())
Should this even be here? The profile shouldn't even have a reference to the
web_data_service_ any more, right?

http://codereview.chromium.org/9834056/diff/10001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.h (right):

http://codereview.chromium.org/9834056/diff/10001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.h:247: scoped_refptr<WebDataService>
web_data_service_;
Does this need to be here? If so, when is it initialized?

http://codereview.chromium.org/9834056/diff/10001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.h:250: bool created_web_data_service_;
Does this need to be here?

[Peter Kasting, 2012-03-27 01:34:44]

When you name multiple reviewers, please list what you want each reviewer to
review.

[GeorgeY, 2012-03-27 17:26:21]

On 2012/03/27 01:34:44, Peter Kasting wrote:
> When you name multiple reviewers, please list what you want each reviewer to
> review.

Sorry,
James:
chrome\browser\ui\intents
Peter:
chrome\browser\ui\search_engines
chrome\browser\search_engines
Nicolas:
chrome\browser\sync
Rachel:
chrome\browser\profiles and the whole cl

[Peter Kasting, 2012-03-27 18:18:25]

LGTM.  Is there any way to reduce the usage of BuildNull() functions by either
defining the "service is normally null in unittests" function or by just
allowing the service to be created on some of these tests?

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/sear...
File chrome/browser/search_engines/template_url_service_sync_unittest.cc
(right):

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/sear...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:304: static
scoped_refptr<RefcountedProfileKeyedService> BuildNull(
Nit: It seems like this is something that should or could exist somewhere in the
core ProfileKeyedService headers, as a utility for people who want to disable
their services for certain tests, so you wouldn't need to re-define it in
several files.

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/sear...
File chrome/browser/search_engines/template_url_service_test_util.cc (right):

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/sear...
chrome/browser/search_engines/template_url_service_test_util.cc:199:
model()->service_ =
Nit: Another way would be

  model()->service_ = WebDataServiceFactory::GetForProfile(
      profile_.get(), Profile::EXPLICIT_ACCESS);

[rpetterson, 2012-03-27 18:26:28]

On 2012/03/27 18:18:25, Peter Kasting wrote:
> LGTM.  Is there any way to reduce the usage of BuildNull() functions by either
> defining the "service is normally null in unittests" function or by just
> allowing the service to be created on some of these tests?
> 
>
https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/sear...
> File chrome/browser/search_engines/template_url_service_sync_unittest.cc
> (right):
> 
>
https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/sear...
> chrome/browser/search_engines/template_url_service_sync_unittest.cc:304:
static
> scoped_refptr<RefcountedProfileKeyedService> BuildNull(
> Nit: It seems like this is something that should or could exist somewhere in
the
> core ProfileKeyedService headers, as a utility for people who want to disable
> their services for certain tests, so you wouldn't need to re-define it in
> several files.
> 

This does exist: ProfileKeyedBaseFactory::ServiceIsNULLWhileTesting(). 

>
https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/sear...
> File chrome/browser/search_engines/template_url_service_test_util.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/sear...
> chrome/browser/search_engines/template_url_service_test_util.cc:199:
> model()->service_ =
> Nit: Another way would be
> 
>   model()->service_ = WebDataServiceFactory::GetForProfile(
>       profile_.get(), Profile::EXPLICIT_ACCESS);

[Peter Kasting, 2012-03-27 18:28:35]

On 2012/03/27 18:26:28, rpetterson wrote:
>
https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/sear...
> > chrome/browser/search_engines/template_url_service_sync_unittest.cc:304:
> static
> > scoped_refptr<RefcountedProfileKeyedService> BuildNull(
> > Nit: It seems like this is something that should or could exist somewhere in
> the
> > core ProfileKeyedService headers, as a utility for people who want to
disable
> > their services for certain tests, so you wouldn't need to re-define it in
> > several files.
> > 
> 
> This does exist: ProfileKeyedBaseFactory::ServiceIsNULLWhileTesting().

Not the same thing (and in fact I referred separately to this in my overall
comments).

One causes all unittests to get a NULL service.  The other is an override
function to plug in for a specific test in order to just get a NULL service in
that case.

[rpetterson, 2012-03-27 18:50:10]

On 2012/03/27 18:28:35, Peter Kasting wrote:
> On 2012/03/27 18:26:28, rpetterson wrote:
> >
>
https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/sear...
> > > chrome/browser/search_engines/template_url_service_sync_unittest.cc:304:
> > static
> > > scoped_refptr<RefcountedProfileKeyedService> BuildNull(
> > > Nit: It seems like this is something that should or could exist somewhere
in
> > the
> > > core ProfileKeyedService headers, as a utility for people who want to
> disable
> > > their services for certain tests, so you wouldn't need to re-define it in
> > > several files.
> > > 
> > 
> > This does exist: ProfileKeyedBaseFactory::ServiceIsNULLWhileTesting().
> 
> Not the same thing (and in fact I referred separately to this in my overall
> comments).
> 
> One causes all unittests to get a NULL service.  The other is an override
> function to plug in for a specific test in order to just get a NULL service in
> that case.

Right, but he should be able to set this to return true so that in general it
creates a NULL service and then use the injection method of SetTestingFactory
for just those he wants an actual service for.

Alternatively, he can call SetTestingFactory(profile, NULL) to set the service
to null as well rather than passing in BuildNull.

[Nicolas Zea, 2012-03-27 21:35:47]

nice, sync LGTM

http://codereview.chromium.org/9834056/diff/10001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_autofill_unittest.cc (right):

http://codereview.chromium.org/9834056/diff/10001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_autofill_unittest.cc:356:
~ProfileSyncServiceAutofillTest() {
virtual

[GeorgeY, 2012-03-30 19:20:12]

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/auto...
File chrome/browser/autocomplete/search_provider_unittest.cc (right):

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/auto...
chrome/browser/autocomplete/search_provider_unittest.cc:32: static
scoped_refptr<RefcountedProfileKeyedService> BuildNull(
On 2012/03/27 01:01:17, Ilya Sherman wrote:
> nit: No need to declare this method static, since it's already in an anonymous
> namespace.

Removed it.

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/auto...
File chrome/browser/autofill/autofill_metrics_unittest.cc (right):

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/auto...
chrome/browser/autofill/autofill_metrics_unittest.cc:933:
personal_data_.Shutdown();
On 2012/03/27 01:15:06, rpetterson wrote:
> Why is this needed now (as opposed to before)?

It is either this or make an empty service like for some other tests. Init() add
notification if service is not NULL, Shutdown() removes it. If you call Init()
twice in a row with non-null data service you get an assert for adding the same
notification twice. I thought this is a better way for fixing this problem.

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/pass...
File chrome/browser/password_manager/password_store_default.h (right):

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/pass...
chrome/browser/password_manager/password_store_default.h:15: #include
"chrome/browser/webdata/web_data_service.h"
On 2012/03/27 01:01:17, Ilya Sherman wrote:
> nit: Why does this now need to be a #include rather than a
forward-declaration?

Line 52. Otherwise it needs to be included every time prior to this include.

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/prof...
File chrome/browser/profiles/profile_impl.cc (right):

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/prof...
chrome/browser/profiles/profile_impl.cc:592: if (web_data_service_.get())
On 2012/03/27 01:15:06, rpetterson wrote:
> Should this even be here? The profile shouldn't even have a reference to the
> web_data_service_ any more, right?
yes removed and adjusted the comment

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/prof...
File chrome/browser/profiles/profile_impl.h (right):

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/prof...
chrome/browser/profiles/profile_impl.h:247: scoped_refptr<WebDataService>
web_data_service_;
On 2012/03/27 01:15:06, rpetterson wrote:
> Does this need to be here? If so, when is it initialized?
good catch - removed

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/prof...
chrome/browser/profiles/profile_impl.h:250: bool created_web_data_service_;
On 2012/03/27 01:15:06, rpetterson wrote:
> Does this need to be here?

and this

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/sear...
File chrome/browser/search_engines/template_url_service.cc (right):

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/sear...
chrome/browser/search_engines/template_url_service.cc:494:
Profile::EXPLICIT_ACCESS);
On 2012/03/27 01:01:17, Ilya Sherman wrote:
> nit: Please add curly braces.

Done.

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/sear...
File chrome/browser/search_engines/template_url_service_sync_unittest.cc
(right):

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/sear...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:304: static
scoped_refptr<RefcountedProfileKeyedService> BuildNull(
On 2012/03/27 18:18:25, Peter Kasting wrote:
> Nit: It seems like this is something that should or could exist somewhere in
the
> core ProfileKeyedService headers, as a utility for people who want to disable
> their services for certain tests, so you wouldn't need to re-define it in
> several files.

Done - removed

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/sear...
File chrome/browser/search_engines/template_url_service_test_util.cc (right):

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/sear...
chrome/browser/search_engines/template_url_service_test_util.cc:199:
model()->service_ =
On 2012/03/27 18:18:25, Peter Kasting wrote:
> Nit: Another way would be
> 
>   model()->service_ = WebDataServiceFactory::GetForProfile(
>       profile_.get(), Profile::EXPLICIT_ACCESS);

Done.

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/sync...
File chrome/browser/sync/profile_sync_service_autofill_unittest.cc (right):

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/sync...
chrome/browser/sync/profile_sync_service_autofill_unittest.cc:356:
~ProfileSyncServiceAutofillTest() {
On 2012/03/27 21:35:48, nzea wrote:
> virtual

Done.

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/webd...
File chrome/browser/webdata/web_data_service.h (right):

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/webd...
chrome/browser/webdata/web_data_service.h:306: virtual void
ShutdownOnUIThread();
On 2012/03/27 01:01:17, Ilya Sherman wrote:
> nit: Please annotate this with OVERRIDE

Done.

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/webd...
File chrome/browser/webdata/web_data_service_factory.cc (right):

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/webd...
chrome/browser/webdata/web_data_service_factory.cc:25: NOTREACHED() << "This
profile is OffTheRecord";
On 2012/03/27 01:01:17, Ilya Sherman wrote:
> nit: Please remove the logged comment, as it is clearly deducible from the
code,
> but if included as a string adds to the binary size.

Done.

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/webd...
chrome/browser/webdata/web_data_service_factory.cc:36: NOTREACHED() << "This
profile is OffTheRecord";
On 2012/03/27 01:01:17, Ilya Sherman wrote:
> nit: Ditto.

Done.

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/webd...
chrome/browser/webdata/web_data_service_factory.cc:61: NOTREACHED() << "failed
to initialize WebService";
On 2012/03/27 01:01:17, Ilya Sherman wrote:
> nit: Ditto.

Done.

[Peter Kasting, 2012-03-30 20:31:13]

Still LGTM

https://chromiumcodereview.appspot.com/9834056/diff/20001/chrome/browser/ui/s...
File chrome/browser/ui/search_engines/keyword_editor_controller_unittest.cc
(right):

https://chromiumcodereview.appspot.com/9834056/diff/20001/chrome/browser/ui/s...
chrome/browser/ui/search_engines/keyword_editor_controller_unittest.cc:34:
KeywordEditorControllerTest() {}
Nit: It would be nice while here to move these inline definitions out-of-line,
and mark the virtual functions with OVERRIDE

https://chromiumcodereview.appspot.com/9834056/diff/20001/chrome/browser/webd...
File chrome/browser/webdata/web_data_service_factory.cc (right):

https://chromiumcodereview.appspot.com/9834056/diff/20001/chrome/browser/webd...
chrome/browser/webdata/web_data_service_factory.cc:25: NOTREACHED();
Don't use NOTREACHED() like this -- it violates our style guide rule of "don't
handle DCHECK failure".

Instead, just DCHECK((access_type != Profile::IMPLICIT_ACCESS) ||
!profile->IsOffTheRecord()) and continue executing.  (3 places)

[Ilya Sherman, 2012-03-30 20:50:05]

(Still LGTM)

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/pass...
File chrome/browser/password_manager/password_store_default.h (right):

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/pass...
chrome/browser/password_manager/password_store_default.h:15: #include
"chrome/browser/webdata/web_data_service.h"
On 2012/03/30 19:20:12, GeorgeY wrote:
> On 2012/03/27 01:01:17, Ilya Sherman wrote:
> > nit: Why does this now need to be a #include rather than a
> forward-declaration?
> 
> Line 52. Otherwise it needs to be included every time prior to this include.

Huh, a scoped_refptr<Foo> needs more than a forward-declaration for Foo?  That
seems surprising to me, but o.k.

[Peter Kasting, 2012-03-30 20:53:53]

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/pass...
File chrome/browser/password_manager/password_store_default.h (right):

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/pass...
chrome/browser/password_manager/password_store_default.h:15: #include
"chrome/browser/webdata/web_data_service.h"
On 2012/03/30 20:50:05, Ilya Sherman wrote:
> On 2012/03/30 19:20:12, GeorgeY wrote:
> > On 2012/03/27 01:01:17, Ilya Sherman wrote:
> > > nit: Why does this now need to be a #include rather than a
> > forward-declaration?
> > 
> > Line 52. Otherwise it needs to be included every time prior to this include.
> 
> Huh, a scoped_refptr<Foo> needs more than a forward-declaration for Foo?  That
> seems surprising to me, but o.k.

No, we shouldn't need a #include to do scoped_ptr<Foo> as long as that object is
not actually used by e.g. an inline function definition.

Rather, password_store_default.cc as well as any subclass implementations should
#include the header.

Other files that just #include this file will not need to also #include
web_data_service.h; if they do it's for some other reason.

[tfarina, 2012-03-30 20:54:02]

On Fri, Mar 30, 2012 at 5:50 PM,  <isherman@chromium.org> wrote:
> (Still LGTM)
>
>
>
>
https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/pass...
> File chrome/browser/password_manager/password_store_default.h (right):
>
>
https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/pass...
> chrome/browser/password_manager/password_store_default.h:15: #include
> "chrome/browser/webdata/web_data_service.h"
> On 2012/03/30 19:20:12, GeorgeY wrote:
>>
>> On 2012/03/27 01:01:17, Ilya Sherman wrote:
>> > nit: Why does this now need to be a #include rather than a
>> forward-declaration?
>
>
>> Line 52. Otherwise it needs to be included every time prior to this
>
> include.
>
> Huh, a scoped_refptr<Foo> needs more than a forward-declaration for Foo?
>  That seems surprising to me, but o.k.
>
You can use forward declarations with scoped_refptr if you include the
header file for Foo in the source files needed to see the declaration.
John (jam) pursued this over me, and he was correct.

> https://chromiumcodereview.appspot.com/9834056/



-- 
Thiago

[GeorgeY, 2012-04-02 21:57:43]

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/pass...
File chrome/browser/password_manager/password_store_default.h (right):

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/pass...
chrome/browser/password_manager/password_store_default.h:15: #include
"chrome/browser/webdata/web_data_service.h"
On 2012/03/30 20:53:53, Peter Kasting wrote:
> On 2012/03/30 20:50:05, Ilya Sherman wrote:
> > On 2012/03/30 19:20:12, GeorgeY wrote:
> > > On 2012/03/27 01:01:17, Ilya Sherman wrote:
> > > > nit: Why does this now need to be a #include rather than a
> > > forward-declaration?
> > > 
> > > Line 52. Otherwise it needs to be included every time prior to this
include.
> > 
> > Huh, a scoped_refptr<Foo> needs more than a forward-declaration for Foo? 
That
> > seems surprising to me, but o.k.
> 
> No, we shouldn't need a #include to do scoped_ptr<Foo> as long as that object
is
> not actually used by e.g. an inline function definition.
> 
> Rather, password_store_default.cc as well as any subclass implementations
should
> #include the header.
> 
> Other files that just #include this file will not need to also #include
> web_data_service.h; if they do it's for some other reason.
There is a some non obvious usage in one of the header files, that
alphabetically precede "chrome/browser/webdata/web_data_service.h".
Unfortunately compiler does not tell at what moment it tried to expand template,
just that it failed to find AddRef/Release methods.

https://chromiumcodereview.appspot.com/9834056/diff/20001/chrome/browser/ui/s...
File chrome/browser/ui/search_engines/keyword_editor_controller_unittest.cc
(right):

https://chromiumcodereview.appspot.com/9834056/diff/20001/chrome/browser/ui/s...
chrome/browser/ui/search_engines/keyword_editor_controller_unittest.cc:34:
KeywordEditorControllerTest() {}
On 2012/03/30 20:31:13, Peter Kasting wrote:
> Nit: It would be nice while here to move these inline definitions out-of-line,
> and mark the virtual functions with OVERRIDE
Sure

https://chromiumcodereview.appspot.com/9834056/diff/20001/chrome/browser/webd...
File chrome/browser/webdata/web_data_service_factory.cc (right):

https://chromiumcodereview.appspot.com/9834056/diff/20001/chrome/browser/webd...
chrome/browser/webdata/web_data_service_factory.cc:25: NOTREACHED();
On 2012/03/30 20:31:13, Peter Kasting wrote:
> Don't use NOTREACHED() like this -- it violates our style guide rule of "don't
> handle DCHECK failure".
> 
> Instead, just DCHECK((access_type != Profile::IMPLICIT_ACCESS) ||
> !profile->IsOffTheRecord()) and continue executing.  (3 places)
I think it is one of the few places where it makes sense: if we return NULL here
it is guaranteed we won't write anything to disk, we might even crash. On the
other hand, if in some case we actually *try* to access it implicitly in
Incognito and succeed, we would violate privacy, which, I think, is worse

[James Hawkins, 2012-04-02 21:58:48]

intents LGTM

[rpetterson, 2012-04-02 21:59:59]

profiles (and whatever else I could find) LGTM

[Peter Kasting, 2012-04-02 22:09:05]

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/pass...
File chrome/browser/password_manager/password_store_default.h (right):

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/pass...
chrome/browser/password_manager/password_store_default.h:15: #include
"chrome/browser/webdata/web_data_service.h"
On 2012/04/02 21:57:43, GeorgeY wrote:
> There is a some non obvious usage in one of the header files, that
> alphabetically precede "chrome/browser/webdata/web_data_service.h".
> Unfortunately compiler does not tell at what moment it tried to expand
template,
> just that it failed to find AddRef/Release methods.

That doesn't justify making this change.  It just means we need to fix whatever
the other problem is.

Did you try running the patch, sans this change, through all the different
platforms to see if one of the compilers gives you more helpful error output?

https://chromiumcodereview.appspot.com/9834056/diff/20001/chrome/browser/webd...
File chrome/browser/webdata/web_data_service_factory.cc (right):

https://chromiumcodereview.appspot.com/9834056/diff/20001/chrome/browser/webd...
chrome/browser/webdata/web_data_service_factory.cc:25: NOTREACHED();
On 2012/04/02 21:57:43, GeorgeY wrote:
> On 2012/03/30 20:31:13, Peter Kasting wrote:
> > Don't use NOTREACHED() like this -- it violates our style guide rule of
"don't
> > handle DCHECK failure".
> > 
> > Instead, just DCHECK((access_type != Profile::IMPLICIT_ACCESS) ||
> > !profile->IsOffTheRecord()) and continue executing.  (3 places)
> I think it is one of the few places where it makes sense: if we return NULL
here
> it is guaranteed we won't write anything to disk, we might even crash. On the
> other hand, if in some case we actually *try* to access it implicitly in
> Incognito and succeed, we would violate privacy, which, I think, is worse

Sorry, I don't agree that this qualifies for an exemption.  Please change this.

[GeorgeY, 2012-04-02 22:49:02]

Found and fixed dependency

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/pass...
File chrome/browser/password_manager/password_store_default.h (right):

https://chromiumcodereview.appspot.com/9834056/diff/10001/chrome/browser/pass...
chrome/browser/password_manager/password_store_default.h:15: #include
"chrome/browser/webdata/web_data_service.h"
On 2012/04/02 22:09:05, Peter Kasting wrote:
> On 2012/04/02 21:57:43, GeorgeY wrote:
> > There is a some non obvious usage in one of the header files, that
> > alphabetically precede "chrome/browser/webdata/web_data_service.h".
> > Unfortunately compiler does not tell at what moment it tried to expand
> template,
> > just that it failed to find AddRef/Release methods.
> 
> That doesn't justify making this change.  It just means we need to fix
whatever
> the other problem is.
> 
> Did you try running the patch, sans this change, through all the different
> platforms to see if one of the compilers gives you more helpful error output?

found and fixed

https://chromiumcodereview.appspot.com/9834056/diff/20001/chrome/browser/webd...
File chrome/browser/webdata/web_data_service_factory.cc (right):

https://chromiumcodereview.appspot.com/9834056/diff/20001/chrome/browser/webd...
chrome/browser/webdata/web_data_service_factory.cc:25: NOTREACHED();
On 2012/04/02 22:09:06, Peter Kasting wrote:
> On 2012/04/02 21:57:43, GeorgeY wrote:
> > On 2012/03/30 20:31:13, Peter Kasting wrote:
> > > Don't use NOTREACHED() like this -- it violates our style guide rule of
> "don't
> > > handle DCHECK failure".
> > > 
> > > Instead, just DCHECK((access_type != Profile::IMPLICIT_ACCESS) ||
> > > !profile->IsOffTheRecord()) and continue executing.  (3 places)
> > I think it is one of the few places where it makes sense: if we return NULL
> here
> > it is guaranteed we won't write anything to disk, we might even crash. On
the
> > other hand, if in some case we actually *try* to access it implicitly in
> > Incognito and succeed, we would violate privacy, which, I think, is worse
> 
> Sorry, I don't agree that this qualifies for an exemption.  Please change
this.
OK. Removed NOTREACHED in first two places - the third one continues the
execution after NOTREACHED

[GeorgeY, 2012-04-02 23:06:20]

Changed return NULL;  to NOTREACHED()

[Peter Kasting, 2012-04-02 23:21:13]

LGTM

https://chromiumcodereview.appspot.com/9834056/diff/25008/chrome/browser/webd...
File chrome/browser/webdata/web_data_service_factory.cc (right):

https://chromiumcodereview.appspot.com/9834056/diff/25008/chrome/browser/webd...
chrome/browser/webdata/web_data_service_factory.cc:24: if (access_type ==
Profile::IMPLICIT_ACCESS && profile->IsOffTheRecord()) {
Nit: Use DCHECK() as requested previously:


  DCHECK((access_type == Profile::EXPLICIT_ACCESS) ||
         !profile->IsOffTheRecord());

(2 places)

https://chromiumcodereview.appspot.com/9834056/diff/25008/chrome/browser/webd...
chrome/browser/webdata/web_data_service_factory.cc:58: if
(!wds->Init(profile->GetPath())) {
Nit: {} unnecessary

[GeorgeY, 2012-04-03 20:13:28]

done

https://chromiumcodereview.appspot.com/9834056/diff/25008/chrome/browser/webd...
File chrome/browser/webdata/web_data_service_factory.cc (right):

https://chromiumcodereview.appspot.com/9834056/diff/25008/chrome/browser/webd...
chrome/browser/webdata/web_data_service_factory.cc:24: if (access_type ==
Profile::IMPLICIT_ACCESS && profile->IsOffTheRecord()) {
On 2012/04/02 23:21:13, Peter Kasting wrote:
> Nit: Use DCHECK() as requested previously:
> 
> 
>   DCHECK((access_type == Profile::EXPLICIT_ACCESS) ||
>          !profile->IsOffTheRecord());
> 
> (2 places)

Done.

https://chromiumcodereview.appspot.com/9834056/diff/25008/chrome/browser/webd...
chrome/browser/webdata/web_data_service_factory.cc:58: if
(!wds->Init(profile->GetPath())) {
On 2012/04/02 23:21:13, Peter Kasting wrote:
> Nit: {} unnecessary

Done.

[commit-bot: I haz the power, 2012-04-03 20:20:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/georgey@chromium.org/9834056/25014

[commit-bot: I haz the power, 2012-04-03 20:20:38]

Can't apply patch for file
chrome/browser/search_engines/template_url_service_sync_unittest.cc.
While running patch -p1 --forward --force;
patching file
chrome/browser/search_engines/template_url_service_sync_unittest.cc
Hunk #2 FAILED at 161.
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/search_engines/template_url_service_sync_unittest.cc.rej

[commit-bot: I haz the power, 2012-04-03 21:37:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/georgey@chromium.org/9834056/26062

[commit-bot: I haz the power, 2012-04-03 21:52:21]

Can't apply patch for file
chrome/browser/sync/profile_sync_components_factory_impl.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/sync/profile_sync_components_factory_impl.cc
Hunk #1 succeeded at 47 (offset 2 lines).
Hunk #2 FAILED at 84.
Hunk #3 succeeded at 228 (offset 4 lines).
1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/sync/profile_sync_components_factory_impl.cc.rej

[commit-bot: I haz the power, 2012-04-04 00:24:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/georgey@chromium.org/9834056/20009

[commit-bot: I haz the power, 2012-04-04 00:53:19]

Try job failure for 9834056-20009 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-04-04 20:37:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/georgey@chromium.org/9834056/28001

[commit-bot: I haz the power, 2012-04-04 23:16:26]

Try job failure for 9834056-28001 (retry) on mac_rel for step "unit_tests".
It's a second try, previously, steps "browser_tests, unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-04-05 00:13:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/georgey@chromium.org/9834056/28001

[commit-bot: I haz the power, 2012-04-05 01:08:26]

Can't apply patch for file
chrome/browser/sync/profile_sync_components_factory_impl.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/sync/profile_sync_components_factory_impl.cc
Hunk #2 FAILED at 88.
1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/sync/profile_sync_components_factory_impl.cc.rej

[commit-bot: I haz the power, 2012-04-05 18:02:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/georgey@chromium.org/9834056/43001

[commit-bot: I haz the power, 2012-04-05 18:03:09]

Can't process patch for file
chrome/browser/sync/profile_sync_service_autofill_unittest.cc.
File's status is None, patchset upload is incomplete.