Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10908065:
Introduce a couple of abstract bases for WebDataService. (Closed)
|
Created:
8 years, 3 months ago by Jói Modified:
8 years, 3 months ago CC:
chromium-reviews, dhollowa+watch_chromium.org, Ilya Sherman, browser-components-watch_chromium.org, dyu1 Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionIntroduce a couple of abstract bases for WebDataService.
WebDataService is really a collection of different services that have a common initialization and some commonality e.g. how to cancel requests, and it's probably best that the interfaces reflect this logical split so that the implementation might some daybe tidied up. Starting this off with a WebDataServiceBase for the common bits, and an AutofillWebData for the Autofill-specific functionalities of WebDataService.
Using these interfaces to break Autofill's concrete dependency on WebDataService.
TBR=ben@chromium.org
BUG=140037
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156567
Patch Set 1 #
Total comments: 7
Patch Set 2 : Merge to LKGR, remove unneeded {} #Patch Set 3 : . #
Total comments: 12
Patch Set 4 : . #
Total comments: 40
Patch Set 5 : Respond to review comments. #Patch Set 6 : Fix gypi file #
Total comments: 4
Patch Set 7 : Respond to comments #Messages
Total messages: 38 (0 generated)
Erik, what do you think? Implicit in this approach is that ProfileKeyedService and RefcountedProfileKeyedService would move to the api directory, whereas the factory stuff under chrome/browser/profiles would not. The current factory for PersonalDataManager would move to chrome/browser/configuration. I wanted to do all of these changes as follow-ups, in particular because I'm not sure I like the current setup... it's just that it's the only relatively simple way I can think of to deal with RefcountedProfileKeyedService guys... the use of scoped_refptr and especially the use of non-standard traits in RefcountedProfileKeyedService makes it hard not to have this base class at the top of the class hiearchy for a service (whereas a regular ProfileKeyedService would not necessarily need to have a ProfileKeyedService class at the top of the hierarchy). Cheers, Jói
I forgot to mention: This will of course go to a reviewer more familiar with WebDataService once we agree this is the way forward. There are no OWNERS for the chrome/browser/webdata directory but looking at the code I think isherman@ would be familiar enough to review. Cheers, Jói
+isherman to review WebDataService changes and the extracted API. Ilya, are you a good reviewer for this code? Erik will review the whole change, so you can focus on the interfaces I extracted for WebDataService. My plan going forward (i.e. once we get to other components) is to extract one new interface per client, i.e. we now have WebDataServiceBase that all clients use, plus AutofillWebData that Autofill uses, and might in the future add e.g. WebAppsWebData, KeywordsWebData and so forth. Cheers, Jói
I'm out on vacation until next week, so you might want to ask David Holloway to do the review instead. Otherwise, I can take a look in a week. Sent from my phone; apologies for any typos or brevity. On Sep 3, 2012 6:03 PM, <joi@chromium.org> wrote: > +isherman to review WebDataService changes and the extracted API. > > Ilya, are you a good reviewer for this code? Erik will review the whole > change, > so you can focus on the interfaces I extracted for WebDataService. > > My plan going forward (i.e. once we get to other components) is to extract > one > new interface per client, i.e. we now have WebDataServiceBase that all > clients > use, plus AutofillWebData that Autofill uses, and might in the future add > e.g. > WebAppsWebData, KeywordsWebData and so forth. > > Cheers, > Jói > > > http://codereview.chromium.**org/10908065/<http://codereview.chromium.org/109... >
http://codereview.chromium.org/10908065/diff/1/chrome/browser/webdata/web_dat... File chrome/browser/webdata/web_data_service.h (right): http://codereview.chromium.org/10908065/diff/1/chrome/browser/webdata/web_dat... chrome/browser/webdata/web_data_service.h:105: public AutofillWebData { Instead of splitting out just the *interface* for AutofillWebData, what about using aggregation and splitting off the interface *and* the implementation. I.e. something akin to the tab-contents "helper" classes? I.e.: AutofillWebData* AutofillWebData::ForService(WebDataServiceBase* service) { return static_cast<WebDataService*>(service)->autofill_web_data_; }
On 2012/09/04 16:26:54, dhollowa wrote: > Instead of splitting out just the *interface* for AutofillWebData, what about > using aggregation and splitting off the interface *and* the implementation. I agree that might be a nice path going forward. However: a) I'm working on the Browser Components, and the primary motivation there is to break concrete dependencies from feature code (like Autofill) to the rest of the browser so that feature components can be reused in different contexts. Where possible, it's best to do this as safe refactoring changes that don't modify the implementation too much. b) We would still want an abstract interface (like this one) in the API directory, the idea being that ports such as Android or iOS could replace the WebData implementation, and test harnesses could replace it for more targeted tests. c) I'd prefer to leave the splitting of the implementation itself to someone more knowledgeable of this code - but certainly the idea to split the interfaces was motivated by thinking that it might be good to split the implementation up at some point. Cheers, Jói
http://codereview.chromium.org/10908065/diff/1/chrome/browser/autofill/autoco... File chrome/browser/autofill/autocomplete_history_manager.cc (right): http://codereview.chromium.org/10908065/diff/1/chrome/browser/autofill/autoco... chrome/browser/autofill/autocomplete_history_manager.cc:118: web_data_service_ = WebDataServiceBase::ForProfile(profile_); Why go through WebDataServiceBase? Why not have AutofillWebDataService::ForProfile and have the detail that this is implemented by WebDataService be hidden?
> Why go through WebDataServiceBase? Why not have > AutofillWebDataService::ForProfile and have the detail that this is > implemented by WebDataService be hidden? A couple of reasons: a) WebDataService is a RefcountedProfileKeyedService. This means you need to keep it via a scoped_ptr and it's hard to break that out to multiple different interfaces. RefcountedProfileKeyedService even has a custom traits class so it's either something like this, or some kind of wrapper class per individual interface that keeps the reference for you; b) You need some common bits from WebDataService; for now this is just CancelRequest but I'm guessing there might be others used by other features. Cheers, Jói On Tue, Sep 4, 2012 at 10:43 AM, <erikwright@chromium.org> wrote: > > http://codereview.chromium.org/10908065/diff/1/chrome/browser/autofill/autoco... > File chrome/browser/autofill/autocomplete_history_manager.cc (right): > > http://codereview.chromium.org/10908065/diff/1/chrome/browser/autofill/autoco... > chrome/browser/autofill/autocomplete_history_manager.cc:118: > web_data_service_ = WebDataServiceBase::ForProfile(profile_); > Why go through WebDataServiceBase? Why not have > AutofillWebDataService::ForProfile and have the detail that this is > implemented by WebDataService be hidden? > > http://codereview.chromium.org/10908065/
To my understanding, scoped_refptr<T> is constructable from scoped_refptr<U> if U is assignable to T. So it might be possible to just make AutofillWebDataService and WebDataServiceBase inherit virtually from RefCountedThreadSafeBase (directly or indirectly via RefCountedProfileKeyedService). On Tue, Sep 4, 2012 at 1:49 PM, Jói Sigurðsson <joi@chromium.org> wrote: > > Why go through WebDataServiceBase? Why not have > > AutofillWebDataService::ForProfile and have the detail that this is > > implemented by WebDataService be hidden? > > A couple of reasons: > a) WebDataService is a RefcountedProfileKeyedService. This means you > need to keep it via a scoped_ptr and it's hard to break that out to > multiple different interfaces. RefcountedProfileKeyedService even has > a custom traits class so it's either something like this, or some kind > of wrapper class per individual interface that keeps the reference for > you; > b) You need some common bits from WebDataService; for now this is just > CancelRequest but I'm guessing there might be others used by other > features. > > Cheers, > Jói > > > On Tue, Sep 4, 2012 at 10:43 AM, <erikwright@chromium.org> wrote: > > > > > http://codereview.chromium.org/10908065/diff/1/chrome/browser/autofill/autoco... > > File chrome/browser/autofill/autocomplete_history_manager.cc (right): > > > > > http://codereview.chromium.org/10908065/diff/1/chrome/browser/autofill/autoco... > > chrome/browser/autofill/autocomplete_history_manager.cc:118: > > web_data_service_ = WebDataServiceBase::ForProfile(profile_); > > Why go through WebDataServiceBase? Why not have > > AutofillWebDataService::ForProfile and have the detail that this is > > implemented by WebDataService be hidden? > > > > http://codereview.chromium.org/10908065/ >
By "U is assignable to T" I meant U* is assignable to T*. On Tue, Sep 4, 2012 at 2:21 PM, Erik Wright <erikwright@chromium.org> wrote: > To my understanding, scoped_refptr<T> is constructable from > scoped_refptr<U> if U is assignable to T. So it might be possible to just > make AutofillWebDataService and WebDataServiceBase inherit virtually from > RefCountedThreadSafeBase (directly or indirectly via > RefCountedProfileKeyedService). > > On Tue, Sep 4, 2012 at 1:49 PM, Jói Sigurðsson <joi@chromium.org> wrote: > >> > Why go through WebDataServiceBase? Why not have >> > AutofillWebDataService::ForProfile and have the detail that this is >> > implemented by WebDataService be hidden? >> >> A couple of reasons: >> a) WebDataService is a RefcountedProfileKeyedService. This means you >> need to keep it via a scoped_ptr and it's hard to break that out to >> multiple different interfaces. RefcountedProfileKeyedService even has >> a custom traits class so it's either something like this, or some kind >> of wrapper class per individual interface that keeps the reference for >> you; >> b) You need some common bits from WebDataService; for now this is just >> CancelRequest but I'm guessing there might be others used by other >> features. >> >> Cheers, >> Jói >> >> >> On Tue, Sep 4, 2012 at 10:43 AM, <erikwright@chromium.org> wrote: >> > >> > >> http://codereview.chromium.org/10908065/diff/1/chrome/browser/autofill/autoco... >> > File chrome/browser/autofill/autocomplete_history_manager.cc (right): >> > >> > >> http://codereview.chromium.org/10908065/diff/1/chrome/browser/autofill/autoco... >> > chrome/browser/autofill/autocomplete_history_manager.cc:118: >> > web_data_service_ = WebDataServiceBase::ForProfile(profile_); >> > Why go through WebDataServiceBase? Why not have >> > AutofillWebDataService::ForProfile and have the detail that this is >> > implemented by WebDataService be hidden? >> > >> > http://codereview.chromium.org/10908065/ >> > >
Autofill interface refactoring LGTM. http://codereview.chromium.org/10908065/diff/1/chrome/browser/autofill/person... File chrome/browser/autofill/personal_data_manager.cc (right): http://codereview.chromium.org/10908065/diff/1/chrome/browser/autofill/person... chrome/browser/autofill/personal_data_manager.cc:174: if (service) { nit: no need for {} with single-line if-body. http://codereview.chromium.org/10908065/diff/1/chrome/browser/autofill/person... chrome/browser/autofill/personal_data_manager.cc:367: scoped_refptr<WebDataServiceBase> wds = Maybe some helpers like |autofill_data()| and |web_data_service()| would be nice here too (like you have in autocomplete_history_manager.h) ?
http://codereview.chromium.org/10908065/diff/1/chrome/browser/autofill/autoco... File chrome/browser/autofill/autocomplete_history_manager.cc (right): http://codereview.chromium.org/10908065/diff/1/chrome/browser/autofill/autoco... chrome/browser/autofill/autocomplete_history_manager.cc:118: web_data_service_ = WebDataServiceBase::ForProfile(profile_); On 2012/09/04 17:43:50, erikwright wrote: > Why go through WebDataServiceBase? Why not have > AutofillWebDataService::ForProfile and have the detail that this is implemented > by WebDataService be hidden? As discussed, there are common bits in WebDataServiceBase that any consumer of the service would use (currently just the typedef of Handle, and the CancelRequest method). Since we can't multiply inherit concrete base classes, having AutofillWebData also be a refcounted object seems out of the question. That leaves a few choices: a) The current approach, where knowing how to get an AutofillWebData is apparent by looking at the AutofillWebData interface and seeing the FromService method (i.e. you see that you need a WebDataServiceBase). This is error prone because you might forget to keep the reference to WebDataserviceBase for as long as you keep the AutofillWebData pointer. b) The current approach, except on its head; instead of AutofillWebData::ForService(), you have an accessor named GetAutofillWebData() on WebDataServiceBase that returns an AutofillWebData*. This has the same limitation w.r.t. the reference, although you are perhaps slightly less likely to forget to keep the WebDataServiceBase reference. c) Have WebDataService implement WebDataServiceBase and AutofillWebData as it does today, but using either approach (a) or (b), instead of getting an AutofillWebData*, you are returned a scoped_ptr<AutofillWebData> which instead of actually pointing to an underlying WebDataService object, points to an AutofillWebDataRef object that inherits from AutofillWebData, contains an inner scoped_refptr<WebDataService> to ensure lifetime, and implements all methods of AutofillWebData by delegating to the inner WebDataService. Approach (c) is probably the safest, and it leaves flexibility in whether the underlying implementation is one big class (as today) or split up (as may be desired in the future). Its drawback is that it is a bit more implementation because it uses delegation rather than inheritance, but that is probably fine. All in favor? The last outstanding question, which I'm relatively agnostic on, is whether to have an accessor on WebDataServiceBase to retrieve AutofillWebData, or whether to have a ForService factory on AutofillWebData. I lean slightly to the latter, since it decouples things a bit more, but am willing to be persuaded of the former. http://codereview.chromium.org/10908065/diff/1/chrome/browser/autofill/person... File chrome/browser/autofill/personal_data_manager.cc (right): http://codereview.chromium.org/10908065/diff/1/chrome/browser/autofill/person... chrome/browser/autofill/personal_data_manager.cc:174: if (service) { On 2012/09/04 21:53:52, dhollowa wrote: > nit: no need for {} with single-line if-body. Done. http://codereview.chromium.org/10908065/diff/1/chrome/browser/autofill/person... chrome/browser/autofill/personal_data_manager.cc:367: scoped_refptr<WebDataServiceBase> wds = On 2012/09/04 21:53:52, dhollowa wrote: > Maybe some helpers like |autofill_data()| and |web_data_service()| would be nice > here too (like you have in autocomplete_history_manager.h) ? Will look at this once we've finished discussing how to retrieve AutofillWebData and ensure lifetime of the WebDataService through it (see large comment somewhere in this email).
On 2012/09/06 01:17:27, Jói wrote: > http://codereview.chromium.org/10908065/diff/1/chrome/browser/autofill/autoco... > File chrome/browser/autofill/autocomplete_history_manager.cc (right): > > http://codereview.chromium.org/10908065/diff/1/chrome/browser/autofill/autoco... > chrome/browser/autofill/autocomplete_history_manager.cc:118: web_data_service_ = > WebDataServiceBase::ForProfile(profile_); > On 2012/09/04 17:43:50, erikwright wrote: > > Why go through WebDataServiceBase? Why not have > > AutofillWebDataService::ForProfile and have the detail that this is > implemented > > by WebDataService be hidden? > > As discussed, there are common bits in WebDataServiceBase that any consumer of > the service would use (currently just the typedef of Handle, and the > CancelRequest method). > > Since we can't multiply inherit concrete base classes, having AutofillWebData > also be a refcounted object seems out of the question. That leaves a few > choices: > > a) The current approach, where knowing how to get an AutofillWebData is apparent > by looking at the AutofillWebData interface and seeing the FromService method > (i.e. you see that you need a WebDataServiceBase). This is error prone because > you might forget to keep the reference to WebDataserviceBase for as long as you > keep the AutofillWebData pointer. > > b) The current approach, except on its head; instead of > AutofillWebData::ForService(), you have an accessor named GetAutofillWebData() > on WebDataServiceBase that returns an AutofillWebData*. This has the same > limitation w.r.t. the reference, although you are perhaps slightly less likely > to forget to keep the WebDataServiceBase reference. > > c) Have WebDataService implement WebDataServiceBase and AutofillWebData as it > does today, but using either approach (a) or (b), instead of getting an > AutofillWebData*, you are returned a scoped_ptr<AutofillWebData> which instead > of actually pointing to an underlying WebDataService object, points to an > AutofillWebDataRef object that inherits from AutofillWebData, contains an inner > scoped_refptr<WebDataService> to ensure lifetime, and implements all methods of > AutofillWebData by delegating to the inner WebDataService. > > Approach (c) is probably the safest, and it leaves flexibility in whether the > underlying implementation is one big class (as today) or split up (as may be > desired in the future). Its drawback is that it is a bit more implementation > because it uses delegation rather than inheritance, but that is probably fine. > > All in favor? > > The last outstanding question, which I'm relatively agnostic on, is whether to > have an accessor on WebDataServiceBase to retrieve AutofillWebData, or whether > to have a ForService factory on AutofillWebData. I lean slightly to the latter, > since it decouples things a bit more, but am willing to be persuaded of the > former. > > http://codereview.chromium.org/10908065/diff/1/chrome/browser/autofill/person... > File chrome/browser/autofill/personal_data_manager.cc (right): > > http://codereview.chromium.org/10908065/diff/1/chrome/browser/autofill/person... > chrome/browser/autofill/personal_data_manager.cc:174: if (service) { > On 2012/09/04 21:53:52, dhollowa wrote: > > nit: no need for {} with single-line if-body. > > Done. > > http://codereview.chromium.org/10908065/diff/1/chrome/browser/autofill/person... > chrome/browser/autofill/personal_data_manager.cc:367: > scoped_refptr<WebDataServiceBase> wds = > On 2012/09/04 21:53:52, dhollowa wrote: > > Maybe some helpers like |autofill_data()| and |web_data_service()| would be > nice > > here too (like you have in autocomplete_history_manager.h) ? > > Will look at this once we've finished discussing how to retrieve AutofillWebData > and ensure lifetime of the WebDataService through it (see large comment > somewhere in this email). Another approach might be: d) Make AutofillWebData be a direct subclass of RefcountedProfileKeyedService, with a AutofillWebData::ForProfile(Profile* profile). Demote WebDataServiceBase to be an abstract base class that collects the common methods for the specific services (ie. CancelRequest(...)), it would not be a RefcountedProfileKeyedService directly, just an abstract interface. The current clients of WebDataService would instead hold scoped_refptrs to AutofillData objects. Under the hood add-ref'ing an AutofillData would add-ref the common WebDataService (hidden) implementation. A couple benefits to this: (1) Clients don't need to know anything about the WebDataService "collection" of services, they just know about the specific service they're interested in. The interface for managing lifetime of this service becomes simpler because clients don't need to know that the lifetime of WebDataService is tied to the lifetime of AutofillWebData.
Thanks for the suggestion. That would work great for now, but I believe it would break as soon as we have more than one interface on top of WebDataService, e.g. in addition to an AutofillWebData we might also have an IntentsWebData since we couldn't have both of these be a RefcountedProfileKeyedService. OTOH, your idea made me think of something similar that would work: - Make WebDataServiceBase into just a pure virtual interface, not refcounted. - Have AutofillWebData a pure virtual interface as we do now. - WebDataService would inherit RefcountedProfileKeyedService, WebDataServiceBase, and AutofillWebData. - Add a subclass of AutofillWebData called e.g. AutofillWebDataService. This would inherit from both WebDataServiceBase and AutofillWebData, and this is where we would have ::ForProfile(), which would return a scoped_ptr<AutofillWebDataService> class. The implementation of this class would keep a scoped_refptr<WebDataService> and would delegate to that concrete class for all of its methods. Perhaps that is cleanest? It removes the confusion between different types and leaves flexibility to later split the implementation or not. Cheers, Jói On Thu, Sep 6, 2012 at 9:31 AM, <dhollowa@chromium.org> wrote: > On 2012/09/06 01:17:27, Jói wrote: > > http://codereview.chromium.org/10908065/diff/1/chrome/browser/autofill/autoco... >> >> File chrome/browser/autofill/autocomplete_history_manager.cc (right): > > > > http://codereview.chromium.org/10908065/diff/1/chrome/browser/autofill/autoco... >> >> chrome/browser/autofill/autocomplete_history_manager.cc:118: >> web_data_service_ > > = >> >> WebDataServiceBase::ForProfile(profile_); >> On 2012/09/04 17:43:50, erikwright wrote: >> > Why go through WebDataServiceBase? Why not have >> > AutofillWebDataService::ForProfile and have the detail that this is >> implemented >> > by WebDataService be hidden? > > >> As discussed, there are common bits in WebDataServiceBase that any >> consumer of >> the service would use (currently just the typedef of Handle, and the >> CancelRequest method). > > >> Since we can't multiply inherit concrete base classes, having >> AutofillWebData >> also be a refcounted object seems out of the question. That leaves a few >> choices: > > >> a) The current approach, where knowing how to get an AutofillWebData is > > apparent >> >> by looking at the AutofillWebData interface and seeing the FromService >> method >> (i.e. you see that you need a WebDataServiceBase). This is error prone > > because >> >> you might forget to keep the reference to WebDataserviceBase for as long >> as > > you >> >> keep the AutofillWebData pointer. > > >> b) The current approach, except on its head; instead of >> AutofillWebData::ForService(), you have an accessor named >> GetAutofillWebData() >> on WebDataServiceBase that returns an AutofillWebData*. This has the same >> limitation w.r.t. the reference, although you are perhaps slightly less >> likely >> to forget to keep the WebDataServiceBase reference. > > >> c) Have WebDataService implement WebDataServiceBase and AutofillWebData as >> it >> does today, but using either approach (a) or (b), instead of getting an >> AutofillWebData*, you are returned a scoped_ptr<AutofillWebData> which >> instead >> of actually pointing to an underlying WebDataService object, points to an >> AutofillWebDataRef object that inherits from AutofillWebData, contains an > > inner >> >> scoped_refptr<WebDataService> to ensure lifetime, and implements all >> methods > > of >> >> AutofillWebData by delegating to the inner WebDataService. > > >> Approach (c) is probably the safest, and it leaves flexibility in whether >> the >> underlying implementation is one big class (as today) or split up (as may >> be >> desired in the future). Its drawback is that it is a bit more >> implementation >> because it uses delegation rather than inheritance, but that is probably >> fine. > > >> All in favor? > > >> The last outstanding question, which I'm relatively agnostic on, is >> whether to >> have an accessor on WebDataServiceBase to retrieve AutofillWebData, or >> whether >> to have a ForService factory on AutofillWebData. I lean slightly to the > > latter, >> >> since it decouples things a bit more, but am willing to be persuaded of >> the >> former. > > > > http://codereview.chromium.org/10908065/diff/1/chrome/browser/autofill/person... >> >> File chrome/browser/autofill/personal_data_manager.cc (right): > > > > http://codereview.chromium.org/10908065/diff/1/chrome/browser/autofill/person... >> >> chrome/browser/autofill/personal_data_manager.cc:174: if (service) { >> On 2012/09/04 21:53:52, dhollowa wrote: >> > nit: no need for {} with single-line if-body. > > >> Done. > > > > http://codereview.chromium.org/10908065/diff/1/chrome/browser/autofill/person... >> >> chrome/browser/autofill/personal_data_manager.cc:367: >> scoped_refptr<WebDataServiceBase> wds = >> On 2012/09/04 21:53:52, dhollowa wrote: >> > Maybe some helpers like |autofill_data()| and |web_data_service()| would >> > be >> nice >> > here too (like you have in autocomplete_history_manager.h) ? > > >> Will look at this once we've finished discussing how to retrieve > > AutofillWebData >> >> and ensure lifetime of the WebDataService through it (see large comment >> somewhere in this email). > > > Another approach might be: > d) Make AutofillWebData be a direct subclass of > RefcountedProfileKeyedService, > with a AutofillWebData::ForProfile(Profile* profile). Demote > WebDataServiceBase > to be an abstract base class that collects the common methods for the > specific > services (ie. CancelRequest(...)), it would not be a > RefcountedProfileKeyedService directly, just an abstract interface. The > current clients of WebDataService would instead hold scoped_refptrs to > AutofillData objects. Under the hood add-ref'ing an AutofillData would > add-ref > the common WebDataService (hidden) implementation. > > A couple benefits to this: (1) Clients don't need to know anything about the > WebDataService "collection" of services, they just know about the specific > service they're interested in. The interface for managing lifetime of this > service becomes simpler because clients don't need to know that the lifetime > of WebDataService is tied to the lifetime of AutofillWebData. > > > > http://codereview.chromium.org/10908065/ > > -- > You received this message because you are subscribed to the Google Groups > "browser-components-watch" group. > To post to this group, send email to browser-components-watch@chromium.org. > To unsubscribe from this group, send email to > browser-components-watch+unsubscribe@chromium.org. > For more options, visit > https://groups.google.com/a/chromium.org/groups/opt_out. > >
On 2012/09/06 16:31:26, dhollowa wrote: > On 2012/09/06 01:17:27, Jói wrote: > > > http://codereview.chromium.org/10908065/diff/1/chrome/browser/autofill/autoco... > > File chrome/browser/autofill/autocomplete_history_manager.cc (right): > > > > > http://codereview.chromium.org/10908065/diff/1/chrome/browser/autofill/autoco... > > chrome/browser/autofill/autocomplete_history_manager.cc:118: web_data_service_ > = > > WebDataServiceBase::ForProfile(profile_); > > On 2012/09/04 17:43:50, erikwright wrote: > > > Why go through WebDataServiceBase? Why not have > > > AutofillWebDataService::ForProfile and have the detail that this is > > implemented > > > by WebDataService be hidden? > > > > As discussed, there are common bits in WebDataServiceBase that any consumer of > > the service would use (currently just the typedef of Handle, and the > > CancelRequest method). > > > > Since we can't multiply inherit concrete base classes, having AutofillWebData > > also be a refcounted object seems out of the question. That leaves a few > > choices: > > > > a) The current approach, where knowing how to get an AutofillWebData is > apparent > > by looking at the AutofillWebData interface and seeing the FromService method > > (i.e. you see that you need a WebDataServiceBase). This is error prone > because > > you might forget to keep the reference to WebDataserviceBase for as long as > you > > keep the AutofillWebData pointer. > > > > b) The current approach, except on its head; instead of > > AutofillWebData::ForService(), you have an accessor named GetAutofillWebData() > > on WebDataServiceBase that returns an AutofillWebData*. This has the same > > limitation w.r.t. the reference, although you are perhaps slightly less likely > > to forget to keep the WebDataServiceBase reference. > > > > c) Have WebDataService implement WebDataServiceBase and AutofillWebData as it > > does today, but using either approach (a) or (b), instead of getting an > > AutofillWebData*, you are returned a scoped_ptr<AutofillWebData> which instead > > of actually pointing to an underlying WebDataService object, points to an > > AutofillWebDataRef object that inherits from AutofillWebData, contains an > inner > > scoped_refptr<WebDataService> to ensure lifetime, and implements all methods > of > > AutofillWebData by delegating to the inner WebDataService. > > > > Approach (c) is probably the safest, and it leaves flexibility in whether the > > underlying implementation is one big class (as today) or split up (as may be > > desired in the future). Its drawback is that it is a bit more implementation > > because it uses delegation rather than inheritance, but that is probably fine. > > > > All in favor? > > > > The last outstanding question, which I'm relatively agnostic on, is whether to > > have an accessor on WebDataServiceBase to retrieve AutofillWebData, or whether > > to have a ForService factory on AutofillWebData. I lean slightly to the > latter, > > since it decouples things a bit more, but am willing to be persuaded of the > > former. > > > > > http://codereview.chromium.org/10908065/diff/1/chrome/browser/autofill/person... > > File chrome/browser/autofill/personal_data_manager.cc (right): > > > > > http://codereview.chromium.org/10908065/diff/1/chrome/browser/autofill/person... > > chrome/browser/autofill/personal_data_manager.cc:174: if (service) { > > On 2012/09/04 21:53:52, dhollowa wrote: > > > nit: no need for {} with single-line if-body. > > > > Done. > > > > > http://codereview.chromium.org/10908065/diff/1/chrome/browser/autofill/person... > > chrome/browser/autofill/personal_data_manager.cc:367: > > scoped_refptr<WebDataServiceBase> wds = > > On 2012/09/04 21:53:52, dhollowa wrote: > > > Maybe some helpers like |autofill_data()| and |web_data_service()| would be > > nice > > > here too (like you have in autocomplete_history_manager.h) ? > > > > Will look at this once we've finished discussing how to retrieve > AutofillWebData > > and ensure lifetime of the WebDataService through it (see large comment > > somewhere in this email). > > Another approach might be: > d) Make AutofillWebData be a direct subclass of RefcountedProfileKeyedService, > with a AutofillWebData::ForProfile(Profile* profile). Demote WebDataServiceBase > to be an abstract base class that collects the common methods for the specific > services (ie. CancelRequest(...)), it would not be a > RefcountedProfileKeyedService directly, just an abstract interface. The > current clients of WebDataService would instead hold scoped_refptrs to > AutofillData objects. Under the hood add-ref'ing an AutofillData would add-ref > the common WebDataService (hidden) implementation. > > A couple benefits to this: (1) Clients don't need to know anything about the > WebDataService "collection" of services, they just know about the specific > service they're interested in. The interface for managing lifetime of this > service becomes simpler because clients don't need to know that the lifetime > of WebDataService is tied to the lifetime of AutofillWebData. I think (c) or (d) make sense. Regarding (d), I have not looked through the code, but it's not obvious to me that anyone needs or would use the common interface that would be implemented by AutofillWebData and its peers. Is there any generic code that would need to be able to interoperate blindly with all of the peer classes? Joi, I prefer the ForService factory on AutofillWebData for your other question.
On 2012/09/06 17:55:08, Jói wrote: > Thanks for the suggestion. That would work great for now, but I > believe it would break as soon as we have more than one interface on > top of WebDataService, e.g. in addition to an AutofillWebData we might > also have an IntentsWebData since we couldn't have both of these be a > RefcountedProfileKeyedService. > > OTOH, your idea made me think of something similar that would work: > > - Make WebDataServiceBase into just a pure virtual interface, not refcounted. > - Have AutofillWebData a pure virtual interface as we do now. > - WebDataService would inherit RefcountedProfileKeyedService, > WebDataServiceBase, and AutofillWebData. > - Add a subclass of AutofillWebData called e.g. > AutofillWebDataService. This would inherit from both > WebDataServiceBase and AutofillWebData, and this is where we would > have ::ForProfile(), which would return a > scoped_ptr<AutofillWebDataService> class. The implementation of this > class would keep a scoped_refptr<WebDataService> and would delegate to > that concrete class for all of its methods. > > Perhaps that is cleanest? It removes the confusion between different > types and leaves flexibility to later split the implementation or not. > > Cheers, > Jói > > > > On Thu, Sep 6, 2012 at 9:31 AM, <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=dhollowa@chromium.org> wrote: > > On 2012/09/06 01:17:27, Jói wrote: > > > > > http://codereview.chromium.org/10908065/diff/1/chrome/browser/autofill/autoco... > >> > >> File chrome/browser/autofill/autocomplete_history_manager.cc (right): > > > > > > > > > http://codereview.chromium.org/10908065/diff/1/chrome/browser/autofill/autoco... > >> > >> chrome/browser/autofill/autocomplete_history_manager.cc:118: > >> web_data_service_ > > > > = > >> > >> WebDataServiceBase::ForProfile(profile_); > >> On 2012/09/04 17:43:50, erikwright wrote: > >> > Why go through WebDataServiceBase? Why not have > >> > AutofillWebDataService::ForProfile and have the detail that this is > >> implemented > >> > by WebDataService be hidden? > > > > > >> As discussed, there are common bits in WebDataServiceBase that any > >> consumer of > >> the service would use (currently just the typedef of Handle, and the > >> CancelRequest method). > > > > > >> Since we can't multiply inherit concrete base classes, having > >> AutofillWebData > >> also be a refcounted object seems out of the question. That leaves a few > >> choices: > > > > > >> a) The current approach, where knowing how to get an AutofillWebData is > > > > apparent > >> > >> by looking at the AutofillWebData interface and seeing the FromService > >> method > >> (i.e. you see that you need a WebDataServiceBase). This is error prone > > > > because > >> > >> you might forget to keep the reference to WebDataserviceBase for as long > >> as > > > > you > >> > >> keep the AutofillWebData pointer. > > > > > >> b) The current approach, except on its head; instead of > >> AutofillWebData::ForService(), you have an accessor named > >> GetAutofillWebData() > >> on WebDataServiceBase that returns an AutofillWebData*. This has the same > >> limitation w.r.t. the reference, although you are perhaps slightly less > >> likely > >> to forget to keep the WebDataServiceBase reference. > > > > > >> c) Have WebDataService implement WebDataServiceBase and AutofillWebData as > >> it > >> does today, but using either approach (a) or (b), instead of getting an > >> AutofillWebData*, you are returned a scoped_ptr<AutofillWebData> which > >> instead > >> of actually pointing to an underlying WebDataService object, points to an > >> AutofillWebDataRef object that inherits from AutofillWebData, contains an > > > > inner > >> > >> scoped_refptr<WebDataService> to ensure lifetime, and implements all > >> methods > > > > of > >> > >> AutofillWebData by delegating to the inner WebDataService. > > > > > >> Approach (c) is probably the safest, and it leaves flexibility in whether > >> the > >> underlying implementation is one big class (as today) or split up (as may > >> be > >> desired in the future). Its drawback is that it is a bit more > >> implementation > >> because it uses delegation rather than inheritance, but that is probably > >> fine. > > > > > >> All in favor? > > > > > >> The last outstanding question, which I'm relatively agnostic on, is > >> whether to > >> have an accessor on WebDataServiceBase to retrieve AutofillWebData, or > >> whether > >> to have a ForService factory on AutofillWebData. I lean slightly to the > > > > latter, > >> > >> since it decouples things a bit more, but am willing to be persuaded of > >> the > >> former. > > > > > > > > > http://codereview.chromium.org/10908065/diff/1/chrome/browser/autofill/person... > >> > >> File chrome/browser/autofill/personal_data_manager.cc (right): > > > > > > > > > http://codereview.chromium.org/10908065/diff/1/chrome/browser/autofill/person... > >> > >> chrome/browser/autofill/personal_data_manager.cc:174: if (service) { > >> On 2012/09/04 21:53:52, dhollowa wrote: > >> > nit: no need for {} with single-line if-body. > > > > > >> Done. > > > > > > > > > http://codereview.chromium.org/10908065/diff/1/chrome/browser/autofill/person... > >> > >> chrome/browser/autofill/personal_data_manager.cc:367: > >> scoped_refptr<WebDataServiceBase> wds = > >> On 2012/09/04 21:53:52, dhollowa wrote: > >> > Maybe some helpers like |autofill_data()| and |web_data_service()| would > >> > be > >> nice > >> > here too (like you have in autocomplete_history_manager.h) ? > > > > > >> Will look at this once we've finished discussing how to retrieve > > > > AutofillWebData > >> > >> and ensure lifetime of the WebDataService through it (see large comment > >> somewhere in this email). > > > > > > Another approach might be: > > d) Make AutofillWebData be a direct subclass of > > RefcountedProfileKeyedService, > > with a AutofillWebData::ForProfile(Profile* profile). Demote > > WebDataServiceBase > > to be an abstract base class that collects the common methods for the > > specific > > services (ie. CancelRequest(...)), it would not be a > > RefcountedProfileKeyedService directly, just an abstract interface. The > > current clients of WebDataService would instead hold scoped_refptrs to > > AutofillData objects. Under the hood add-ref'ing an AutofillData would > > add-ref > > the common WebDataService (hidden) implementation. > > > > A couple benefits to this: (1) Clients don't need to know anything about the > > WebDataService "collection" of services, they just know about the specific > > service they're interested in. The interface for managing lifetime of this > > service becomes simpler because clients don't need to know that the lifetime > > of WebDataService is tied to the lifetime of AutofillWebData. > > > > > > > > http://codereview.chromium.org/10908065/ > > > > -- > > You received this message because you are subscribed to the Google Groups > > "browser-components-watch" group. > > To post to this group, send email to https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=browser-components-watch@c.... > > To unsubscribe from this group, send email to > > https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=browser-components-watch+u.... > > For more options, visit > > https://groups.google.com/a/chromium.org/groups/opt_out. > > > > I'm of the impression that you guys are essentially describing the same thing. Or, at least, when I read David's suggestion in my mind I closely imagine Joi's solution.
> [...] > I'm of the impression that you guys are essentially describing the same thing. > Or, at least, when I read David's suggestion in my mind I closely imagine Joi's > solution. Yes, I was a little loose with the details... But Joi's solution sounds great and keeps the benefits I was hoping for. My take on your "peer" question is that these are only peers because of the common WebDataService implementation, and not because any client treats them as a logical grouping.
Sounds like we are all in agreement, I think I will go with proposal (d) as detailed in my last email. Cheers, Jói On Thu, Sep 6, 2012 at 11:18 AM, <dhollowa@chromium.org> wrote: >> [...] >> >> I'm of the impression that you guys are essentially describing the same >> thing. >> Or, at least, when I read David's suggestion in my mind I closely imagine > > Joi's >> >> solution. > > > Yes, I was a little loose with the details... But Joi's solution sounds > great > and keeps the benefits I was hoping for. > > My take on your "peer" question is that these are only peers because of the > common WebDataService implementation, and not because any client treats them > as > a logical grouping. > > > > http://codereview.chromium.org/10908065/
I uploaded a new patch with the approach discussed. It doesn't compile yet but I need to catch my flight, and wanted to let you guys comment on it early. Cheers, Jói
Very exciting! Great direction! http://codereview.chromium.org/10908065/diff/12002/chrome/browser/api/webdata... File chrome/browser/api/webdata/autofill_web_data.h (right): http://codereview.chromium.org/10908065/diff/12002/chrome/browser/api/webdata... chrome/browser/api/webdata/autofill_web_data.h:94: : public AutofillWebData, nit: ": public AutofillWebData" should be on line 93. http://codereview.chromium.org/10908065/diff/12002/chrome/browser/autofill/au... File chrome/browser/autofill/autocomplete_history_manager.h (right): http://codereview.chromium.org/10908065/diff/12002/chrome/browser/autofill/au... chrome/browser/autofill/autocomplete_history_manager.h:69: AutofillWebDataService* wds); nit: I liked your change to using |service| as the variable name for this. http://codereview.chromium.org/10908065/diff/12002/chrome/browser/autofill/au... chrome/browser/autofill/autocomplete_history_manager.h:81: scoped_ptr<AutofillWebDataService> autofill_data_; nit: |service_| or |data_service_|? http://codereview.chromium.org/10908065/diff/12002/chrome/browser/autofill/pe... File chrome/browser/autofill/personal_data_manager.cc (right): http://codereview.chromium.org/10908065/diff/12002/chrome/browser/autofill/pe... chrome/browser/autofill/personal_data_manager.cc:200: scoped_ptr<AutofillWebDataService> autofill_data = |service| or |data_service|? And elsewhere. http://codereview.chromium.org/10908065/diff/12002/chrome/browser/autofill/pe... chrome/browser/autofill/personal_data_manager.cc:232: DCHECK(autofill_data_.get() && Compile error with |autofill_data_|? http://codereview.chromium.org/10908065/diff/12002/chrome/browser/webdata/aut... File chrome/browser/webdata/autofill_web_data_service_impl.h (right): http://codereview.chromium.org/10908065/diff/12002/chrome/browser/webdata/aut... chrome/browser/webdata/autofill_web_data_service_impl.h:13: class AutofillWebDataServiceImpl : public AutofillWebDataService { I find this name a bit confusing. Mainly because the WebDataService is really the "impl" and this class is more of a "bridge". Maybe name this class |AutofillWebDataServiceBridge|? http://codereview.chromium.org/10908065/diff/12002/chrome/browser/webdata/aut... chrome/browser/webdata/autofill_web_data_service_impl.h:15: AutofillWebDataServiceImpl(scoped_refptr<WebDataService> service); nit: explicit http://codereview.chromium.org/10908065/diff/12002/chrome/browser/webdata/aut... chrome/browser/webdata/autofill_web_data_service_impl.h:17: virtual WebDataServiceBase* NotificationSource() OVERRIDE; // AutofillWebDataService implementation. http://codereview.chromium.org/10908065/diff/12002/chrome/browser/webdata/aut... chrome/browser/webdata/autofill_web_data_service_impl.h:46: const scoped_refptr<WebDataService> service_; How about |impl_|? http://codereview.chromium.org/10908065/diff/12002/chrome/browser/webdata/web... File chrome/browser/webdata/web_data_service.h (right): http://codereview.chromium.org/10908065/diff/12002/chrome/browser/webdata/web... chrome/browser/webdata/web_data_service.h:104: : public WebDataServiceBase, nit: Common pattern is unwrapped style. I.e. line 104 should go up to 103. "class WebDataService : public WebDataServiceBase,\n"
http://codereview.chromium.org/10908065/diff/12002/chrome/browser/api/webdata... File chrome/browser/api/webdata/web_data_results.h (right): http://codereview.chromium.org/10908065/diff/12002/chrome/browser/api/webdata... chrome/browser/api/webdata/web_data_results.h:11: // Result types for WebDataService. Not for this CL, but it strikes me this should really go away and we should be using "base/values.h" here instead.
Erik: LGTY?
I don't understand why AutofillWebDataService needs to extend WebDataServiceBase. Reading through this CL, the only use I actually saw was the code that creates a notification source. Perhaps it would be more appropriate to expose a ::GetNotificationSource() method on AutofillWebData() so it can hide the exact T of the Source<T> subclass. Perhaps there are some reasons for this class relationship that I'm missing, however. http://codereview.chromium.org/10908065/diff/12002/chrome/browser/webdata/web... File chrome/browser/webdata/web_data_service_factory.cc (right): http://codereview.chromium.org/10908065/diff/12002/chrome/browser/webdata/web... chrome/browser/webdata/web_data_service_factory.cc:16: // TODO(joi): Does the implicit/explicit distinction really matter? Presumably you will answer this question before committing this code?
Erik, apologies: My still-jetlagged brain somehow forgot that this change was only half-ready; the patch currently on Rietveld doesn't even compile, so my "LGTY?" ping was premature - you were of course waiting for me to upload the finished product. I'm working on it and will upload a fully-functional version soon. In the meantime, let me address your questions: > I don't understand why AutofillWebDataService needs to extend > WebDataServiceBase. For CancelRequest, and because there is an is-a relationship. There will be other XyzWebDataService service interfaces, each of which is-a WebDataServiceBase. We could avoid this, but it would mean duplicating CancelRequest into each XyzWebData interface. OTOH, thinking about the as-is-ness of these things, and how the next XyzWebDataService would look, I realized I should pop the NotificationSource method up to WebDataServiceBase. I'm also changing its name to GetNotificationSource to be consistent with our standard naming for methods. > chrome/browser/webdata/web_data_service_factory.cc:16: // TODO(joi): > Does the implicit/explicit distinction really matter? > Presumably you will answer this question before committing this code? Right, thanks for catching that. For this service it doesn't make any difference so I'm leaving it out. I've added a note to WebDataServiceFactory::GetForProfile to say that if we start using the access_type parameter for anything but the DCHECK, we would have to revisit the interface of AutofillWebDataServiceImpl::ForContext. Cheers, Jói On Tue, Sep 11, 2012 at 8:49 PM, <erikwright@chromium.org> wrote: > I don't understand why AutofillWebDataService needs to extend > WebDataServiceBase. > > Reading through this CL, the only use I actually saw was the code that > creates a > notification source. Perhaps it would be more appropriate to expose a > ::GetNotificationSource() method on AutofillWebData() so it can hide the > exact T > of the Source<T> subclass. > > Perhaps there are some reasons for this class relationship that I'm missing, > however. > > > http://codereview.chromium.org/10908065/diff/12002/chrome/browser/webdata/web... > File chrome/browser/webdata/web_data_service_factory.cc (right): > > http://codereview.chromium.org/10908065/diff/12002/chrome/browser/webdata/web... > chrome/browser/webdata/web_data_service_factory.cc:16: // TODO(joi): > Does the implicit/explicit distinction really matter? > Presumably you will answer this question before committing this code? > > http://codereview.chromium.org/10908065/
On Wed, Sep 12, 2012 at 7:21 AM, Jói Sigurðsson <joi@chromium.org> wrote: > Erik, apologies: My still-jetlagged brain somehow forgot that this > change was only half-ready; the patch currently on Rietveld doesn't > even compile, so my "LGTY?" ping was premature - you were of course > waiting for me to upload the finished product. I'm working on it and > will upload a fully-functional version soon. > > In the meantime, let me address your questions: > > > I don't understand why AutofillWebDataService needs to extend > > WebDataServiceBase. > > For CancelRequest, and because there is an is-a relationship. There > will be other XyzWebDataService service interfaces, each of which is-a > WebDataServiceBase. > But is there a single client that needs to be able to interoperate with any WebDataServiceBase without knowing which one it is? If it isn't, it seems the "is-a" relationship is artificial and irrelevant. > We could avoid this, but it would mean duplicating CancelRequest into > each XyzWebData interface. > There are lots of common idioms throughout the codebase. Unless there are utility classes that need to be able to call CancelRequest on any WebDataServiceBase without knowing which one it is, I don't understand why having one method in common (especially when the implementation is actually not shared - all subclasses will reimplement any delegation) warrants a common interface. > OTOH, thinking about the as-is-ness of these things, and how the next > XyzWebDataService would look, I realized I should pop the > NotificationSource method up to WebDataServiceBase. I'm also changing > its name to GetNotificationSource to be consistent with our standard > naming for methods. > > > chrome/browser/webdata/web_data_service_factory.cc:16: // TODO(joi): > > Does the implicit/explicit distinction really matter? > > Presumably you will answer this question before committing this code? > > Right, thanks for catching that. For this service it doesn't make any > difference so I'm leaving it out. I've added a note to > WebDataServiceFactory::GetForProfile to say that if we start using the > access_type parameter for anything but the DCHECK, we would have to > revisit the interface of AutofillWebDataServiceImpl::ForContext. > > Cheers, > Jói > > > On Tue, Sep 11, 2012 at 8:49 PM, <erikwright@chromium.org> wrote: > > I don't understand why AutofillWebDataService needs to extend > > WebDataServiceBase. > > > > Reading through this CL, the only use I actually saw was the code that > > creates a > > notification source. Perhaps it would be more appropriate to expose a > > ::GetNotificationSource() method on AutofillWebData() so it can hide the > > exact T > > of the Source<T> subclass. > > > > Perhaps there are some reasons for this class relationship that I'm > missing, > > however. > > > > > > > http://codereview.chromium.org/10908065/diff/12002/chrome/browser/webdata/web... > > File chrome/browser/webdata/web_data_service_factory.cc (right): > > > > > http://codereview.chromium.org/10908065/diff/12002/chrome/browser/webdata/web... > > chrome/browser/webdata/web_data_service_factory.cc:16: // TODO(joi): > > Does the implicit/explicit distinction really matter? > > Presumably you will answer this question before committing this code? > > > > http://codereview.chromium.org/10908065/ >
> But is there a single client that needs to be able to interoperate with any > WebDataServiceBase without knowing which one it is? If it isn't, it seems > the "is-a" relationship is artificial and irrelevant. I don't agree with that. If we know we will have multiple interfaces, each of which has a couple of methods that are semantically identical between them, with implementation that may be shared (or may in fact down the line reflect the inheritance hierarchy in the interfaces) then we shouldn't wait to extract the common base class until we have tools that need it - we should establish the common base right away so that it will be easier to realize that you can write tools (or whatever) that work across all at the same time. Also, note that the implementation for delegating the methods in WebDataServiceBase to the real WebDataService, as well as keeping the scoped_refptr to WebDataService, could in fact be moved to an implementation base class as soon as we have more than one XyzDataServiceImpl class. If you think it would rhyme better with the interfaces to also extract that implementation base class now, I could do that. Cheers, Jói On Wed, Sep 12, 2012 at 1:55 PM, Erik Wright <erikwright@chromium.org> wrote: > > > On Wed, Sep 12, 2012 at 7:21 AM, Jói Sigurðsson <joi@chromium.org> wrote: >> >> Erik, apologies: My still-jetlagged brain somehow forgot that this >> change was only half-ready; the patch currently on Rietveld doesn't >> even compile, so my "LGTY?" ping was premature - you were of course >> waiting for me to upload the finished product. I'm working on it and >> will upload a fully-functional version soon. >> >> In the meantime, let me address your questions: >> >> > I don't understand why AutofillWebDataService needs to extend >> > WebDataServiceBase. >> >> For CancelRequest, and because there is an is-a relationship. There >> will be other XyzWebDataService service interfaces, each of which is-a >> WebDataServiceBase. > > > But is there a single client that needs to be able to interoperate with any > WebDataServiceBase without knowing which one it is? If it isn't, it seems > the "is-a" relationship is artificial and irrelevant. > >> >> We could avoid this, but it would mean duplicating CancelRequest into >> each XyzWebData interface. > > > There are lots of common idioms throughout the codebase. Unless there are > utility classes that need to be able to call CancelRequest on any > WebDataServiceBase without knowing which one it is, I don't understand why > having one method in common (especially when the implementation is actually > not shared - all subclasses will reimplement any delegation) warrants a > common interface. > >> >> OTOH, thinking about the as-is-ness of these things, and how the next >> XyzWebDataService would look, I realized I should pop the >> NotificationSource method up to WebDataServiceBase. I'm also changing >> its name to GetNotificationSource to be consistent with our standard >> naming for methods. >> >> > chrome/browser/webdata/web_data_service_factory.cc:16: // TODO(joi): >> > Does the implicit/explicit distinction really matter? >> > Presumably you will answer this question before committing this code? >> >> Right, thanks for catching that. For this service it doesn't make any >> difference so I'm leaving it out. I've added a note to >> WebDataServiceFactory::GetForProfile to say that if we start using the >> access_type parameter for anything but the DCHECK, we would have to >> revisit the interface of AutofillWebDataServiceImpl::ForContext. >> >> Cheers, >> Jói >> >> >> On Tue, Sep 11, 2012 at 8:49 PM, <erikwright@chromium.org> wrote: >> > I don't understand why AutofillWebDataService needs to extend >> > WebDataServiceBase. >> > >> > Reading through this CL, the only use I actually saw was the code that >> > creates a >> > notification source. Perhaps it would be more appropriate to expose a >> > ::GetNotificationSource() method on AutofillWebData() so it can hide the >> > exact T >> > of the Source<T> subclass. >> > >> > Perhaps there are some reasons for this class relationship that I'm >> > missing, >> > however. >> > >> > >> > >> > http://codereview.chromium.org/10908065/diff/12002/chrome/browser/webdata/web... >> > File chrome/browser/webdata/web_data_service_factory.cc (right): >> > >> > >> > http://codereview.chromium.org/10908065/diff/12002/chrome/browser/webdata/web... >> > chrome/browser/webdata/web_data_service_factory.cc:16: // TODO(joi): >> > Does the implicit/explicit distinction really matter? >> > Presumably you will answer this question before committing this code? >> > >> > http://codereview.chromium.org/10908065/ > >
On Wed, Sep 12, 2012 at 10:20 AM, Jói Sigurðsson <joi@chromium.org> wrote: > > But is there a single client that needs to be able to interoperate with > any > > WebDataServiceBase without knowing which one it is? If it isn't, it seems > > the "is-a" relationship is artificial and irrelevant. > > I don't agree with that. If we know we will have multiple interfaces, > each of which has a couple of methods that are semantically identical > between them, with implementation that may be shared (or may in fact > down the line reflect the inheritance hierarchy in the interfaces) > then we shouldn't wait to extract the common base class until we have > tools that need it - we should establish the common base right away so > that it will be easier to realize that you can write tools (or > whatever) that work across all at the same time. > I'm not convinced but I also recognize that there is little difference in either approach for now. I don't feel any need to stand in the way of the subclass existing. > Also, note that the implementation for delegating the methods in > WebDataServiceBase to the real WebDataService, as well as keeping the > scoped_refptr to WebDataService, could in fact be moved to an > implementation base class as soon as we have more than one > XyzDataServiceImpl class. If you think it would rhyme better with the > interfaces to also extract that implementation base class now, I could > do that. I don't think that's necessary. It can occur in a separate CL where it has actual benefits. The overall structure of this CL, assuming it compile ;) LGTM. > > Cheers, > Jói > > > > On Wed, Sep 12, 2012 at 1:55 PM, Erik Wright <erikwright@chromium.org> > wrote: > > > > > > On Wed, Sep 12, 2012 at 7:21 AM, Jói Sigurðsson <joi@chromium.org> > wrote: > >> > >> Erik, apologies: My still-jetlagged brain somehow forgot that this > >> change was only half-ready; the patch currently on Rietveld doesn't > >> even compile, so my "LGTY?" ping was premature - you were of course > >> waiting for me to upload the finished product. I'm working on it and > >> will upload a fully-functional version soon. > >> > >> In the meantime, let me address your questions: > >> > >> > I don't understand why AutofillWebDataService needs to extend > >> > WebDataServiceBase. > >> > >> For CancelRequest, and because there is an is-a relationship. There > >> will be other XyzWebDataService service interfaces, each of which is-a > >> WebDataServiceBase. > > > > > > But is there a single client that needs to be able to interoperate with > any > > WebDataServiceBase without knowing which one it is? If it isn't, it seems > > the "is-a" relationship is artificial and irrelevant. > > > >> > >> We could avoid this, but it would mean duplicating CancelRequest into > >> each XyzWebData interface. > > > > > > There are lots of common idioms throughout the codebase. Unless there are > > utility classes that need to be able to call CancelRequest on any > > WebDataServiceBase without knowing which one it is, I don't understand > why > > having one method in common (especially when the implementation is > actually > > not shared - all subclasses will reimplement any delegation) warrants a > > common interface. > > > >> > >> OTOH, thinking about the as-is-ness of these things, and how the next > >> XyzWebDataService would look, I realized I should pop the > >> NotificationSource method up to WebDataServiceBase. I'm also changing > >> its name to GetNotificationSource to be consistent with our standard > >> naming for methods. > >> > >> > chrome/browser/webdata/web_data_service_factory.cc:16: // TODO(joi): > >> > Does the implicit/explicit distinction really matter? > >> > Presumably you will answer this question before committing this code? > >> > >> Right, thanks for catching that. For this service it doesn't make any > >> difference so I'm leaving it out. I've added a note to > >> WebDataServiceFactory::GetForProfile to say that if we start using the > >> access_type parameter for anything but the DCHECK, we would have to > >> revisit the interface of AutofillWebDataServiceImpl::ForContext. > >> > >> Cheers, > >> Jói > >> > >> > >> On Tue, Sep 11, 2012 at 8:49 PM, <erikwright@chromium.org> wrote: > >> > I don't understand why AutofillWebDataService needs to extend > >> > WebDataServiceBase. > >> > > >> > Reading through this CL, the only use I actually saw was the code that > >> > creates a > >> > notification source. Perhaps it would be more appropriate to expose a > >> > ::GetNotificationSource() method on AutofillWebData() so it can hide > the > >> > exact T > >> > of the Source<T> subclass. > >> > > >> > Perhaps there are some reasons for this class relationship that I'm > >> > missing, > >> > however. > >> > > >> > > >> > > >> > > http://codereview.chromium.org/10908065/diff/12002/chrome/browser/webdata/web... > >> > File chrome/browser/webdata/web_data_service_factory.cc (right): > >> > > >> > > >> > > http://codereview.chromium.org/10908065/diff/12002/chrome/browser/webdata/web... > >> > chrome/browser/webdata/web_data_service_factory.cc:16: // TODO(joi): > >> > Does the implicit/explicit distinction really matter? > >> > Presumably you will answer this question before committing this code? > >> > > >> > http://codereview.chromium.org/10908065/ > > > > > > -- > You received this message because you are subscribed to the Google Groups > "browser-components-watch" group. > To post to this group, send email to browser-components-watch@chromium.org > . > To unsubscribe from this group, send email to > browser-components-watch+unsubscribe@chromium.org. > For more options, visit > https://groups.google.com/a/chromium.org/groups/opt_out. > > >
On Wed, Sep 12, 2012 at 11:15 AM, Erik Wright <erikwright@chromium.org>wrote: > > > On Wed, Sep 12, 2012 at 10:20 AM, Jói Sigurðsson <joi@chromium.org> wrote: > >> > But is there a single client that needs to be able to interoperate with >> any >> > WebDataServiceBase without knowing which one it is? If it isn't, it >> seems >> > the "is-a" relationship is artificial and irrelevant. >> >> I don't agree with that. If we know we will have multiple interfaces, >> each of which has a couple of methods that are semantically identical >> between them, with implementation that may be shared (or may in fact >> down the line reflect the inheritance hierarchy in the interfaces) >> then we shouldn't wait to extract the common base class until we have >> tools that need it - we should establish the common base right away so >> that it will be easier to realize that you can write tools (or >> whatever) that work across all at the same time. >> > > I'm not convinced but I also recognize that there is little difference in > either approach for now. I don't feel any need to stand in the way of the > subclass existing. > subclass -> base class > > >> Also, note that the implementation for delegating the methods in >> WebDataServiceBase to the real WebDataService, as well as keeping the >> scoped_refptr to WebDataService, could in fact be moved to an >> implementation base class as soon as we have more than one >> XyzDataServiceImpl class. If you think it would rhyme better with the >> interfaces to also extract that implementation base class now, I could >> do that. > > > I don't think that's necessary. It can occur in a separate CL where it has > actual benefits. > > The overall structure of this CL, assuming it compile ;) LGTM. > > >> >> Cheers, >> Jói >> >> >> >> On Wed, Sep 12, 2012 at 1:55 PM, Erik Wright <erikwright@chromium.org> >> wrote: >> > >> > >> > On Wed, Sep 12, 2012 at 7:21 AM, Jói Sigurðsson <joi@chromium.org> >> wrote: >> >> >> >> Erik, apologies: My still-jetlagged brain somehow forgot that this >> >> change was only half-ready; the patch currently on Rietveld doesn't >> >> even compile, so my "LGTY?" ping was premature - you were of course >> >> waiting for me to upload the finished product. I'm working on it and >> >> will upload a fully-functional version soon. >> >> >> >> In the meantime, let me address your questions: >> >> >> >> > I don't understand why AutofillWebDataService needs to extend >> >> > WebDataServiceBase. >> >> >> >> For CancelRequest, and because there is an is-a relationship. There >> >> will be other XyzWebDataService service interfaces, each of which is-a >> >> WebDataServiceBase. >> > >> > >> > But is there a single client that needs to be able to interoperate with >> any >> > WebDataServiceBase without knowing which one it is? If it isn't, it >> seems >> > the "is-a" relationship is artificial and irrelevant. >> > >> >> >> >> We could avoid this, but it would mean duplicating CancelRequest into >> >> each XyzWebData interface. >> > >> > >> > There are lots of common idioms throughout the codebase. Unless there >> are >> > utility classes that need to be able to call CancelRequest on any >> > WebDataServiceBase without knowing which one it is, I don't understand >> why >> > having one method in common (especially when the implementation is >> actually >> > not shared - all subclasses will reimplement any delegation) warrants a >> > common interface. >> > >> >> >> >> OTOH, thinking about the as-is-ness of these things, and how the next >> >> XyzWebDataService would look, I realized I should pop the >> >> NotificationSource method up to WebDataServiceBase. I'm also changing >> >> its name to GetNotificationSource to be consistent with our standard >> >> naming for methods. >> >> >> >> > chrome/browser/webdata/web_data_service_factory.cc:16: // TODO(joi): >> >> > Does the implicit/explicit distinction really matter? >> >> > Presumably you will answer this question before committing this code? >> >> >> >> Right, thanks for catching that. For this service it doesn't make any >> >> difference so I'm leaving it out. I've added a note to >> >> WebDataServiceFactory::GetForProfile to say that if we start using the >> >> access_type parameter for anything but the DCHECK, we would have to >> >> revisit the interface of AutofillWebDataServiceImpl::ForContext. >> >> >> >> Cheers, >> >> Jói >> >> >> >> >> >> On Tue, Sep 11, 2012 at 8:49 PM, <erikwright@chromium.org> wrote: >> >> > I don't understand why AutofillWebDataService needs to extend >> >> > WebDataServiceBase. >> >> > >> >> > Reading through this CL, the only use I actually saw was the code >> that >> >> > creates a >> >> > notification source. Perhaps it would be more appropriate to expose a >> >> > ::GetNotificationSource() method on AutofillWebData() so it can hide >> the >> >> > exact T >> >> > of the Source<T> subclass. >> >> > >> >> > Perhaps there are some reasons for this class relationship that I'm >> >> > missing, >> >> > however. >> >> > >> >> > >> >> > >> >> > >> http://codereview.chromium.org/10908065/diff/12002/chrome/browser/webdata/web... >> >> > File chrome/browser/webdata/web_data_service_factory.cc (right): >> >> > >> >> > >> >> > >> http://codereview.chromium.org/10908065/diff/12002/chrome/browser/webdata/web... >> >> > chrome/browser/webdata/web_data_service_factory.cc:16: // TODO(joi): >> >> > Does the implicit/explicit distinction really matter? >> >> > Presumably you will answer this question before committing this code? >> >> > >> >> > http://codereview.chromium.org/10908065/ >> > >> > >> >> -- >> You received this message because you are subscribed to the Google Groups >> "browser-components-watch" group. >> To post to this group, send email to >> browser-components-watch@chromium.org. >> To unsubscribe from this group, send email to >> browser-components-watch+unsubscribe@chromium.org. >> For more options, visit >> https://groups.google.com/a/chromium.org/groups/opt_out. >> >> >> >
Erik, please take a (hopefully final) look, seems to be complete and functional now. Cheers, Jói
http://codereview.chromium.org/10908065/diff/15004/chrome/browser/api/webdata... File chrome/browser/api/webdata/autofill_web_data.h (right): http://codereview.chromium.org/10908065/diff/15004/chrome/browser/api/webdata... chrome/browser/api/webdata/autofill_web_data.h:13: #include "chrome/browser/api/webdata/web_data_service_consumer.h" forward-decl? http://codereview.chromium.org/10908065/diff/15004/chrome/browser/api/webdata... chrome/browser/api/webdata/autofill_web_data.h:93: class AutofillWebDataService It may seem pedantic, but I wish this were in its own file. I, for one, keep looking for it in the list of files in this CL and being confused. One class per CL is a good hard-and-fast rule in my book. http://codereview.chromium.org/10908065/diff/15004/chrome/browser/api/webdata... File chrome/browser/api/webdata/web_data_service_base.h (right): http://codereview.chromium.org/10908065/diff/15004/chrome/browser/api/webdata... chrome/browser/api/webdata/web_data_service_base.h:26: virtual WebDataServiceBase* GetNotificationSource() = 0; It seems you could return a NotificationSource (const-ref to member instance or by value) and then hide the type of the source. http://codereview.chromium.org/10908065/diff/15004/chrome/browser/autofill/au... File chrome/browser/autofill/autocomplete_history_manager.cc (right): http://codereview.chromium.org/10908065/diff/15004/chrome/browser/autofill/au... chrome/browser/autofill/autocomplete_history_manager.cc:242: autofill_data_(awd.Pass()), I'm more familiar with release() in this scenario. I suppose Pass is OK too (it seems that if you call Pass but don't actually move to a new scoped_ptr, it would have no effect? If so, I guess it's safer than release()). http://codereview.chromium.org/10908065/diff/15004/chrome/browser/autofill/au... File chrome/browser/autofill/autocomplete_history_manager_unittest.cc (right): http://codereview.chromium.org/10908065/diff/15004/chrome/browser/autofill/au... chrome/browser/autofill/autocomplete_history_manager_unittest.cc:52: scoped_ptr<AutofillWebDataService>( make_scoped_ptr would save the explicit template parameter... http://codereview.chromium.org/10908065/diff/15004/chrome/browser/autofill/pe... File chrome/browser/autofill/personal_data_manager.cc (right): http://codereview.chromium.org/10908065/diff/15004/chrome/browser/autofill/pe... chrome/browser/autofill/personal_data_manager.cc:172: scoped_ptr<AutofillWebDataService> service = I think constructor semantics are preferred here. http://codereview.chromium.org/10908065/diff/15004/chrome/browser/autofill/pe... chrome/browser/autofill/personal_data_manager.cc:200: scoped_ptr<AutofillWebDataService> autofill_data = ditto. http://codereview.chromium.org/10908065/diff/15004/chrome/browser/autofill/pe... chrome/browser/autofill/personal_data_manager.cc:229: WebDataServiceBase* notification_source = Arguably lines 229 to 236 should be in some kind of IF DCHECK_ENABLED construct. http://codereview.chromium.org/10908065/diff/15004/chrome/browser/autofill/pe... chrome/browser/autofill/personal_data_manager.cc:232: scoped_ptr<AutofillWebDataService> autofill_data = constructor semantics. http://codereview.chromium.org/10908065/diff/15004/chrome/browser/autofill/pe... chrome/browser/autofill/personal_data_manager.cc:369: scoped_ptr<AutofillWebDataService> autofill_data = ditto. http://codereview.chromium.org/10908065/diff/15004/chrome/browser/autofill/pe... chrome/browser/autofill/personal_data_manager.cc:397: scoped_ptr<AutofillWebDataService> autofill_data = ditto. http://codereview.chromium.org/10908065/diff/15004/chrome/browser/autofill/pe... chrome/browser/autofill/personal_data_manager.cc:416: scoped_ptr<AutofillWebDataService> autofill_data = ditto. http://codereview.chromium.org/10908065/diff/15004/chrome/browser/autofill/pe... chrome/browser/autofill/personal_data_manager.cc:448: scoped_ptr<AutofillWebDataService> autofill_data = ditto. http://codereview.chromium.org/10908065/diff/15004/chrome/browser/autofill/pe... chrome/browser/autofill/personal_data_manager.cc:476: scoped_ptr<AutofillWebDataService> autofill_data = etc. and below. http://codereview.chromium.org/10908065/diff/15004/chrome/browser/autofill/pe... File chrome/browser/autofill/personal_data_manager_unittest.cc (right): http://codereview.chromium.org/10908065/diff/15004/chrome/browser/autofill/pe... chrome/browser/autofill/personal_data_manager_unittest.cc:18: #include "chrome/browser/webdata/web_data_service.h" needed? http://codereview.chromium.org/10908065/diff/15004/chrome/browser/sync/test/i... File chrome/browser/sync/test/integration/autofill_helper.cc (right): http://codereview.chromium.org/10908065/diff/15004/chrome/browser/sync/test/i... chrome/browser/sync/test/integration/autofill_helper.cc:20: #include "chrome/browser/webdata/web_data_service.h" needed? http://codereview.chromium.org/10908065/diff/15004/chrome/browser/webdata/aut... File chrome/browser/webdata/autofill_web_data_service_impl.cc (right): http://codereview.chromium.org/10908065/diff/15004/chrome/browser/webdata/aut... chrome/browser/webdata/autofill_web_data_service_impl.cc:10: DCHECK(service.get()); include logging.h? http://codereview.chromium.org/10908065/diff/15004/chrome/browser/webdata/aut... File chrome/browser/webdata/autofill_web_data_service_impl.h (right): http://codereview.chromium.org/10908065/diff/15004/chrome/browser/webdata/aut... chrome/browser/webdata/autofill_web_data_service_impl.h:9: #include "chrome/browser/webdata/web_data_service.h" #include "chrome/browser/api/webdata/autofill_web_data_service.h"? http://codereview.chromium.org/10908065/diff/15004/chrome/browser/webdata/aut... chrome/browser/webdata/autofill_web_data_service_impl.h:46: const scoped_refptr<WebDataService> service_; DISALLOW_COPY_AND_ASSIGN?
Thanks for the thorough review Erik. I've uploaded a patch to address your comments. Cheers, Jói http://codereview.chromium.org/10908065/diff/15004/chrome/browser/api/webdata... File chrome/browser/api/webdata/autofill_web_data.h (right): http://codereview.chromium.org/10908065/diff/15004/chrome/browser/api/webdata... chrome/browser/api/webdata/autofill_web_data.h:13: #include "chrome/browser/api/webdata/web_data_service_consumer.h" On 2012/09/13 01:45:11, erikwright wrote: > forward-decl? Done. http://codereview.chromium.org/10908065/diff/15004/chrome/browser/api/webdata... chrome/browser/api/webdata/autofill_web_data.h:93: class AutofillWebDataService On 2012/09/13 01:45:11, erikwright wrote: > It may seem pedantic, but I wish this were in its own file. I, for one, keep > looking for it in the list of files in this CL and being confused. > > One class per CL is a good hard-and-fast rule in my book. Done. http://codereview.chromium.org/10908065/diff/15004/chrome/browser/api/webdata... File chrome/browser/api/webdata/web_data_service_base.h (right): http://codereview.chromium.org/10908065/diff/15004/chrome/browser/api/webdata... chrome/browser/api/webdata/web_data_service_base.h:26: virtual WebDataServiceBase* GetNotificationSource() = 0; On 2012/09/13 01:45:11, erikwright wrote: > It seems you could return a NotificationSource (const-ref to member instance or > by value) and then hide the type of the source. I think it's nicer to have it typed... if it's a NotificationSource and you need to use the pointer, you end up having to first downcast it to Source<WebDataServiceBase> just to get the pointer. http://codereview.chromium.org/10908065/diff/15004/chrome/browser/autofill/au... File chrome/browser/autofill/autocomplete_history_manager.cc (right): http://codereview.chromium.org/10908065/diff/15004/chrome/browser/autofill/au... chrome/browser/autofill/autocomplete_history_manager.cc:242: autofill_data_(awd.Pass()), On 2012/09/13 01:45:11, erikwright wrote: > I'm more familiar with release() in this scenario. I suppose Pass is OK too (it > seems that if you call Pass but don't actually move to a new scoped_ptr, it > would have no effect? If so, I guess it's safer than release()). Pass() is the right thing to use in this scenario to get move semantics, whereas release() just releases the raw pointer (and I don't think it would even compile in this case); see base/memory/scoped_ptr.h The new move semantics of scoped_ptr are very very cool. By declaring the parameter to the constructor as a scoped_ptr, there is no need for documentation about ownership semantics - doing it this way definitively means that AutocompleteHistoryManager takes ownership of the pointer that you pass in. http://codereview.chromium.org/10908065/diff/15004/chrome/browser/autofill/au... File chrome/browser/autofill/autocomplete_history_manager_unittest.cc (right): http://codereview.chromium.org/10908065/diff/15004/chrome/browser/autofill/au... chrome/browser/autofill/autocomplete_history_manager_unittest.cc:52: scoped_ptr<AutofillWebDataService>( On 2012/09/13 01:45:11, erikwright wrote: > make_scoped_ptr would save the explicit template parameter... The type needs to be exactly scoped_ptr<AutofillWebDataService> (not scoped_ptr<AutofillWebDataServiceImpl>) or the copy-semantics implementation complains. http://codereview.chromium.org/10908065/diff/15004/chrome/browser/autofill/pe... File chrome/browser/autofill/personal_data_manager.cc (right): http://codereview.chromium.org/10908065/diff/15004/chrome/browser/autofill/pe... chrome/browser/autofill/personal_data_manager.cc:172: scoped_ptr<AutofillWebDataService> service = On 2012/09/13 01:45:11, erikwright wrote: > I think constructor semantics are preferred here. Done. http://codereview.chromium.org/10908065/diff/15004/chrome/browser/autofill/pe... chrome/browser/autofill/personal_data_manager.cc:200: scoped_ptr<AutofillWebDataService> autofill_data = On 2012/09/13 01:45:11, erikwright wrote: > ditto. Done. http://codereview.chromium.org/10908065/diff/15004/chrome/browser/autofill/pe... chrome/browser/autofill/personal_data_manager.cc:229: WebDataServiceBase* notification_source = On 2012/09/13 01:45:11, erikwright wrote: > Arguably lines 229 to 236 should be in some kind of IF DCHECK_ENABLED construct. Done. http://codereview.chromium.org/10908065/diff/15004/chrome/browser/autofill/pe... chrome/browser/autofill/personal_data_manager.cc:232: scoped_ptr<AutofillWebDataService> autofill_data = On 2012/09/13 01:45:11, erikwright wrote: > constructor semantics. Done. http://codereview.chromium.org/10908065/diff/15004/chrome/browser/autofill/pe... chrome/browser/autofill/personal_data_manager.cc:369: scoped_ptr<AutofillWebDataService> autofill_data = On 2012/09/13 01:45:11, erikwright wrote: > ditto. Done. http://codereview.chromium.org/10908065/diff/15004/chrome/browser/autofill/pe... chrome/browser/autofill/personal_data_manager.cc:397: scoped_ptr<AutofillWebDataService> autofill_data = On 2012/09/13 01:45:11, erikwright wrote: > ditto. Done. http://codereview.chromium.org/10908065/diff/15004/chrome/browser/autofill/pe... chrome/browser/autofill/personal_data_manager.cc:416: scoped_ptr<AutofillWebDataService> autofill_data = On 2012/09/13 01:45:11, erikwright wrote: > ditto. Done. http://codereview.chromium.org/10908065/diff/15004/chrome/browser/autofill/pe... chrome/browser/autofill/personal_data_manager.cc:448: scoped_ptr<AutofillWebDataService> autofill_data = On 2012/09/13 01:45:11, erikwright wrote: > ditto. Done. http://codereview.chromium.org/10908065/diff/15004/chrome/browser/autofill/pe... chrome/browser/autofill/personal_data_manager.cc:476: scoped_ptr<AutofillWebDataService> autofill_data = On 2012/09/13 01:45:11, erikwright wrote: > etc. and below. Done. http://codereview.chromium.org/10908065/diff/15004/chrome/browser/autofill/pe... File chrome/browser/autofill/personal_data_manager_unittest.cc (right): http://codereview.chromium.org/10908065/diff/15004/chrome/browser/autofill/pe... chrome/browser/autofill/personal_data_manager_unittest.cc:18: #include "chrome/browser/webdata/web_data_service.h" On 2012/09/13 01:45:11, erikwright wrote: > needed? Yes, for line 446. http://codereview.chromium.org/10908065/diff/15004/chrome/browser/sync/test/i... File chrome/browser/sync/test/integration/autofill_helper.cc (right): http://codereview.chromium.org/10908065/diff/15004/chrome/browser/sync/test/i... chrome/browser/sync/test/integration/autofill_helper.cc:20: #include "chrome/browser/webdata/web_data_service.h" On 2012/09/13 01:45:11, erikwright wrote: > needed? Yes, for line 67. http://codereview.chromium.org/10908065/diff/15004/chrome/browser/webdata/aut... File chrome/browser/webdata/autofill_web_data_service_impl.cc (right): http://codereview.chromium.org/10908065/diff/15004/chrome/browser/webdata/aut... chrome/browser/webdata/autofill_web_data_service_impl.cc:10: DCHECK(service.get()); On 2012/09/13 01:45:11, erikwright wrote: > include logging.h? Done. http://codereview.chromium.org/10908065/diff/15004/chrome/browser/webdata/aut... File chrome/browser/webdata/autofill_web_data_service_impl.h (right): http://codereview.chromium.org/10908065/diff/15004/chrome/browser/webdata/aut... chrome/browser/webdata/autofill_web_data_service_impl.h:9: #include "chrome/browser/webdata/web_data_service.h" On 2012/09/13 01:45:11, erikwright wrote: > #include "chrome/browser/api/webdata/autofill_web_data_service.h"? Done. http://codereview.chromium.org/10908065/diff/15004/chrome/browser/webdata/aut... chrome/browser/webdata/autofill_web_data_service_impl.h:46: const scoped_refptr<WebDataService> service_; On 2012/09/13 01:45:11, erikwright wrote: > DISALLOW_COPY_AND_ASSIGN? Done.
LGTM. http://codereview.chromium.org/10908065/diff/15004/chrome/browser/api/webdata... File chrome/browser/api/webdata/web_data_service_base.h (right): http://codereview.chromium.org/10908065/diff/15004/chrome/browser/api/webdata... chrome/browser/api/webdata/web_data_service_base.h:26: virtual WebDataServiceBase* GetNotificationSource() = 0; On 2012/09/13 12:34:50, Jói wrote: > On 2012/09/13 01:45:11, erikwright wrote: > > It seems you could return a NotificationSource (const-ref to member instance > or > > by value) and then hide the type of the source. > > I think it's nicer to have it typed... if it's a NotificationSource and you need > to use the pointer, you end up having to first downcast it to > Source<WebDataServiceBase> just to get the pointer. I don't think there's a case in this CL where the pointer is needed. It's only used for an equality check, and the untyped NotificationSource _does_ have an equality operator. But I'm happy to leave it this way for now. It just seems to me that encapsulation would be more complete if the specific type that is the source of a notification were not leaked. http://codereview.chromium.org/10908065/diff/4006/chrome/browser/api/webdata/... File chrome/browser/api/webdata/autofill_web_data.h (right): http://codereview.chromium.org/10908065/diff/4006/chrome/browser/api/webdata/... chrome/browser/api/webdata/autofill_web_data.h:16: class BrowserContext; nit: move to autofill_web_data_service.h http://codereview.chromium.org/10908065/diff/4006/chrome/browser/api/webdata/... File chrome/browser/api/webdata/web_data_service_base.h (right): http://codereview.chromium.org/10908065/diff/4006/chrome/browser/api/webdata/... chrome/browser/api/webdata/web_data_service_base.h:9: #include "content/public/browser/notification_source.h" nit: I guess you added this while experimenting with my comment below. It can go.
LGTM.
Thanks, I decided to switch to NotificationSource as you had suggested. I'm passing it by value rather than as a reference to a member, since this is a more flexible interface and also the object just contains a single pointer anyway so there is no added overhead. Will pull the CQ trigger now. Cheers, Jói http://codereview.chromium.org/10908065/diff/15004/chrome/browser/api/webdata... File chrome/browser/api/webdata/web_data_service_base.h (right): http://codereview.chromium.org/10908065/diff/15004/chrome/browser/api/webdata... chrome/browser/api/webdata/web_data_service_base.h:26: virtual WebDataServiceBase* GetNotificationSource() = 0; On 2012/09/13 13:28:10, erikwright wrote: > On 2012/09/13 12:34:50, Jói wrote: > > On 2012/09/13 01:45:11, erikwright wrote: > > > It seems you could return a NotificationSource (const-ref to member instance > > or > > > by value) and then hide the type of the source. > > > > I think it's nicer to have it typed... if it's a NotificationSource and you > need > > to use the pointer, you end up having to first downcast it to > > Source<WebDataServiceBase> just to get the pointer. > > I don't think there's a case in this CL where the pointer is needed. It's only > used for an equality check, and the untyped NotificationSource _does_ have an > equality operator. > > But I'm happy to leave it this way for now. It just seems to me that > encapsulation would be more complete if the specific type that is the source of > a notification were not leaked. I took another look and yeah, switching to NotificationSource actually does turn out nice and clean. Did that. http://codereview.chromium.org/10908065/diff/4006/chrome/browser/api/webdata/... File chrome/browser/api/webdata/autofill_web_data.h (right): http://codereview.chromium.org/10908065/diff/4006/chrome/browser/api/webdata/... chrome/browser/api/webdata/autofill_web_data.h:16: class BrowserContext; On 2012/09/13 13:28:10, erikwright wrote: > nit: move to autofill_web_data_service.h Done. http://codereview.chromium.org/10908065/diff/4006/chrome/browser/api/webdata/... File chrome/browser/api/webdata/web_data_service_base.h (right): http://codereview.chromium.org/10908065/diff/4006/chrome/browser/api/webdata/... chrome/browser/api/webdata/web_data_service_base.h:9: #include "content/public/browser/notification_source.h" On 2012/09/13 13:28:10, erikwright wrote: > nit: I guess you added this while experimenting with my comment below. It can > go. Thanks for the catch, but now it needs to stay :)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/joi@chromium.org/10908065/6021
Presubmit check for 10908065-6021 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit Messages **
You added one or more #includes of files that are temporarily
allowed but being removed. Can you avoid introducing the
#include? See relevant DEPS file(s) for details and contacts.
chrome/browser/api/webdata/web_data_service_base.h
Illegal include:
"chrome/browser/profiles/refcounted_profile_keyed_service.h"
Because of "!chrome/browser/profiles/refcounted_profile_keyed_service.h"
from chrome/browser/api's include_rules. \
chrome/browser/autofill/autocomplete_history_manager_unittest.cc
Illegal include: "chrome/browser/webdata/autofill_web_data_service_impl.h"
Because of "!chrome/browser/webdata/autofill_web_data_service_impl.h" from
chrome/browser/autofill's specific_include_rules. \
chrome/browser/autofill/personal_data_manager_unittest.cc
Illegal include: "chrome/browser/webdata/web_data_service.h"
Because of "!chrome/browser/webdata/web_data_service.h" from
chrome/browser/autofill's specific_include_rules.
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome
chrome/browser/sync
Presubmit checks took 2.8s to calculate.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/joi@chromium.org/10908065/6021
Change committed as 156567 |
