Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
573553004:
Eliminate NOTIFICATION_HISTORY_LOADED notification (Closed)
|
Created:
6 years, 3 months ago by nshaik Modified:
5 years, 10 months ago Reviewers:
Peter Kasting, Mark P, mattm, Andrew T Wilson (Slow), Nico, sdefresne, Paweł Hajdan Jr., brettw, Nicolas Zea CC:
chromium-reviews, tim+watch_chromium.org, zea+watch_chromium.org, shishir+watch_chromium.org, browser-components-watch_chromium.org, haitaol+watch_chromium.org, dominich+watch_chromium.org, maniscalco+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionEliminate NOTIFICATION_HISTORY_LOADED notification
- add HistoryServiceLoaded method to HistoryServiceObserver
- invoke observer methods instead of sending notification
BUG=373322
TEST=unit_tests
Committed: https://crrev.com/793ee5bff54aa3cb1c1873dfeeeb1f36c42e810f
Cr-Commit-Position: refs/heads/master@{#306027}
Patch Set 1 #
Total comments: 49
Patch Set 2 : Address review comments #
Total comments: 32
Patch Set 3 : Fixed styling issues #
Total comments: 10
Patch Set 4 : Clean up of code in ResourcePrefetchPredictor #
Total comments: 8
Patch Set 5 : clean up bookmark_data_type_controller #
Total comments: 10
Patch Set 6 : Fix Clean up state in bookmark_data_type_controller #
Total comments: 2
Patch Set 7 : Add HistoryServiceBeingDeleted to HistoryServiceObserver #
Total comments: 2
Patch Set 8 : Listen HistoryServiceBeingDeleted in LastDownloadFinder for cleanup #
Total comments: 28
Patch Set 9 : Move HistoryServiceObserver functions to private #
Total comments: 10
Patch Set 10 : Address review comments in historymenubridge #Patch Set 11 : Remove bookmark_model as member variable #
Total comments: 2
Patch Set 12 : Fixed a nit in last_download_finder #Patch Set 13 : Rebase changes #Patch Set 14 : Add ScopedObserver to InMemoryHistoryBackend,PrerenderLocalPredictor,ChromeTemplateURLServiceClient #
Total comments: 1
Messages
Total messages: 77 (22 generated)
naiem.shaik@gmail.com changed reviewers: + sdefresne@chromium.org
On 2014/09/18 01:10:32, naiem.shaik wrote: > mailto:naiem.shaik@gmail.com changed reviewers: > + mailto:sdefresne@chromium.org @sdefresne PTAL, i will add reviewers and owners For all the files if you think the The code looks fine. Thanks.
https://codereview.chromium.org/573553004/diff/1/chrome/browser/history/histo... File chrome/browser/history/history_service.h (right): https://codereview.chromium.org/573553004/diff/1/chrome/browser/history/histo... chrome/browser/history/history_service.h:522: Profile* profile() { return profile_; } We'll have to remove this method later. Can you make it private and add LastDownloadFinder as a friend class, so that no new client is added for it, with a comment? // TODO(sdefresne): remove this method https://codereview.chromium.org/573553004/diff/1/chrome/browser/history/in_me... File chrome/browser/history/in_memory_url_index.h (right): https://codereview.chromium.org/573553004/diff/1/chrome/browser/history/in_me... chrome/browser/history/in_memory_url_index.h:149: // Is called when |service| is loaded. When overriding methods from an interface, you don't copy descriptions for the methods, but just put the name of the interface providing those methods (same for the other places). // history::HistoryServiceObserver: virtual void HistoryServiceLoaded(HistoryService* service) OVERRIDE; https://codereview.chromium.org/573553004/diff/1/chrome/browser/predictors/au... File chrome/browser/predictors/autocomplete_action_predictor.cc (right): https://codereview.chromium.org/573553004/diff/1/chrome/browser/predictors/au... chrome/browser/predictors/autocomplete_action_predictor.cc:100: HistoryService* history_service = AutocompleteActionPredictor is a KeyedService. Shouldn't the unregistration happens in the Shutdown method instead? https://codereview.chromium.org/573553004/diff/1/chrome/browser/sync/glue/boo... File chrome/browser/sync/glue/bookmark_data_type_controller.cc (right): https://codereview.chromium.org/573553004/diff/1/chrome/browser/sync/glue/boo... chrome/browser/sync/glue/bookmark_data_type_controller.cc:57: history->AddHistoryServiceObserver(this); You never call "history->RemoveHistoryServiceObserver(this);". I think you should do it a the location you removed the registrar_.RemoveAll(). https://codereview.chromium.org/573553004/diff/1/chrome/browser/sync/glue/boo... chrome/browser/sync/glue/bookmark_data_type_controller.cc:115: if (!DependentsLoaded()) You removed the following assertion, maybe you should put it back. DCHECK_EQ(state_, MODEL_STARTING); https://codereview.chromium.org/573553004/diff/1/chrome/browser/sync/glue/boo... File chrome/browser/sync/glue/bookmark_data_type_controller.h (right): https://codereview.chromium.org/573553004/diff/1/chrome/browser/sync/glue/boo... chrome/browser/sync/glue/bookmark_data_type_controller.h:14: #include "content/public/browser/notification_observer.h" Remove those two includes, they should no longer be required since you remove last usage of content::Notification* classes. https://codereview.chromium.org/573553004/diff/1/chrome/test/base/ui_test_uti... File chrome/test/base/ui_test_utils.h (right): https://codereview.chromium.org/573553004/diff/1/chrome/test/base/ui_test_uti... chrome/test/base/ui_test_utils.h:309: // Wait for HistoryService to load. You can move this class in ui_test_utils.cc since it is not used from any other file. https://codereview.chromium.org/573553004/diff/1/components/history/core/brow... File components/history/core/browser/history_service_observer.h (right): https://codereview.chromium.org/573553004/diff/1/components/history/core/brow... components/history/core/browser/history_service_observer.h:13: class HistoryServiceObserver { This class need to have a virtual destructor. https://codereview.chromium.org/573553004/diff/1/components/history/core/brow... components/history/core/browser/history_service_observer.h:16: virtual void HistoryServiceLoaded(HistoryService* service) = 0; Use DISALLOW_COPY_AND_ASSIGN to disallow copying and assignation.
Sylvain,
Thanks for your comments.
I have one question, in chrome/browser/history/in_memory_url_index.cc:
The code I have added is not safe if service is NULL and I cannot really
register for a callback, if service is NULL.
Is that a real use case that I need to address?
Also if I have to address it I would have to end up resorting to the older
Notification system and in that case,
I would need ChromeHistoryClient. Can you please suggest what is the right way
to handle this.
if (service && service->backend_loaded()) {
ScheduleRebuildFromHistory();
} else {
- registrar_.Add(this, chrome::NOTIFICATION_HISTORY_LOADED,
- content::Source<Profile>(profile_));
+ service->AddHistoryServiceObserver(this);
}
On 2014/10/09 18:41:46, naiem.shaik wrote:
> Sylvain,
> Thanks for your comments.
> I have one question, in chrome/browser/history/in_memory_url_index.cc:
> The code I have added is not safe if service is NULL and I cannot really
> register for a callback, if service is NULL.
> Is that a real use case that I need to address?
> Also if I have to address it I would have to end up resorting to the older
> Notification system and in that case,
> I would need ChromeHistoryClient. Can you please suggest what is the right way
> to handle this.
>
> if (service && service->backend_loaded()) {
> ScheduleRebuildFromHistory();
> } else {
> - registrar_.Add(this, chrome::NOTIFICATION_HISTORY_LOADED,
> - content::Source<Profile>(profile_));
> + service->AddHistoryServiceObserver(this);
> }
The InMemoryURLIndex is only created by 1. unit tests, 2. HistoryService itself
(according to results when searching for "new InMemoryURLIndex" and "new
history::InMemoryURLIndex"). The method InMemoryURLIndex::OnCacheLoadDone() is
then indirectly called by InMemoryURLIndex::Init() that is called just after the
creation of the instance (see
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/his...).
So when we reach that point, if the HistoryService does not exists it means we
are running an unit test (and it should just be fixed to call the Observer
method) or the HistoryService has already been destroyed (and we probably have
already been destroyed or we will).
So I think you should rewrite the code to:
if (service) {
if (service->backend_loaded()) {
ScheduleRebuildFromHistory();
} else {
service-AddHistoryServiceObserver(this);
}
}
On 2014/10/10 14:59:44, sdefresne wrote:
> On 2014/10/09 18:41:46, naiem.shaik wrote:
> > Sylvain,
> > Thanks for your comments.
> > I have one question, in chrome/browser/history/in_memory_url_index.cc:
> > The code I have added is not safe if service is NULL and I cannot really
> > register for a callback, if service is NULL.
> > Is that a real use case that I need to address?
> > Also if I have to address it I would have to end up resorting to the older
> > Notification system and in that case,
> > I would need ChromeHistoryClient. Can you please suggest what is the right
way
> > to handle this.
> >
> > if (service && service->backend_loaded()) {
> > ScheduleRebuildFromHistory();
> > } else {
> > - registrar_.Add(this, chrome::NOTIFICATION_HISTORY_LOADED,
> > - content::Source<Profile>(profile_));
> > + service->AddHistoryServiceObserver(this);
> > }
>
> The InMemoryURLIndex is only created by 1. unit tests, 2. HistoryService
itself
> (according to results when searching for "new InMemoryURLIndex" and "new
> history::InMemoryURLIndex"). The method InMemoryURLIndex::OnCacheLoadDone() is
> then indirectly called by InMemoryURLIndex::Init() that is called just after
the
> creation of the instance (see
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/his...).
>
> So when we reach that point, if the HistoryService does not exists it means we
> are running an unit test (and it should just be fixed to call the Observer
> method) or the HistoryService has already been destroyed (and we probably have
> already been destroyed or we will).
>
> So I think you should rewrite the code to:
>
> if (service) {
> if (service->backend_loaded()) {
> ScheduleRebuildFromHistory();
> } else {
> service-AddHistoryServiceObserver(this);
> }
> }
Oh, and while I'm thinking about this, we probably want to pass the
HistoryService* instance to InMemoryURLIndex so that it can stop going through
the Factory. In fact, this is what I did in
https://codereview.chromium.org/631253002.
On 2014/10/10 15:01:20, sdefresne wrote:
> On 2014/10/10 14:59:44, sdefresne wrote:
> > On 2014/10/09 18:41:46, naiem.shaik wrote:
> > > Sylvain,
> > > Thanks for your comments.
> > > I have one question, in chrome/browser/history/in_memory_url_index.cc:
> > > The code I have added is not safe if service is NULL and I cannot really
> > > register for a callback, if service is NULL.
> > > Is that a real use case that I need to address?
> > > Also if I have to address it I would have to end up resorting to the older
> > > Notification system and in that case,
> > > I would need ChromeHistoryClient. Can you please suggest what is the right
> way
> > > to handle this.
> > >
> > > if (service && service->backend_loaded()) {
> > > ScheduleRebuildFromHistory();
> > > } else {
> > > - registrar_.Add(this, chrome::NOTIFICATION_HISTORY_LOADED,
> > > - content::Source<Profile>(profile_));
> > > + service->AddHistoryServiceObserver(this);
> > > }
> >
> > The InMemoryURLIndex is only created by 1. unit tests, 2. HistoryService
> itself
> > (according to results when searching for "new InMemoryURLIndex" and "new
> > history::InMemoryURLIndex"). The method InMemoryURLIndex::OnCacheLoadDone()
is
> > then indirectly called by InMemoryURLIndex::Init() that is called just after
> the
> > creation of the instance (see
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/his...).
> >
> > So when we reach that point, if the HistoryService does not exists it means
we
> > are running an unit test (and it should just be fixed to call the Observer
> > method) or the HistoryService has already been destroyed (and we probably
have
> > already been destroyed or we will).
> >
> > So I think you should rewrite the code to:
> >
> > if (service) {
> > if (service->backend_loaded()) {
> > ScheduleRebuildFromHistory();
> > } else {
> > service-AddHistoryServiceObserver(this);
> > }
> > }
>
> Oh, and while I'm thinking about this, we probably want to pass the
> HistoryService* instance to InMemoryURLIndex so that it can stop going through
> the Factory. In fact, this is what I did in
> https://codereview.chromium.org/631253002.
Incase of InMemoryUrlIndex the ScheduleRebuildFromHistory is accessing the
history_service
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/his...
.
Like I said I do not see any tests failing but it will be a problem if
history_service is going to be NULL.
I have same issue with HistoryMenuBridge as well:
if (!history_service_) {
registrar_.Add(
this, chrome::NOTIFICATION_HISTORY_LOADED,
content::Source<Profile>(profile_));
}
although the change I uploaded does not fail any of the unittests, I think it
might be an issue later.
Should I leave the NOTIFICATION_HISTORY_LOADED and let ChromeHistoryClient send
the notification?
In which case, I would have to find a way to ensure ChromeHistoryClient is
always created for getting this notification.
Please suggest.
I did review the code, and in particular how you switched the classes to use HistoryServiceObserver::HistoryLoaded instead of NOTIFICATION_HISTORY_LOADED and added comments on how I think you can fix the only case I see as problematic. For the rest, the way you changed the code from notification to observer is correct, so I think the logic of the CL is sound. Please address comments and upload a new version of the CL. https://codereview.chromium.org/573553004/diff/1/chrome/browser/history/histo... File chrome/browser/history/history_service.cc (right): https://codereview.chromium.org/573553004/diff/1/chrome/browser/history/histo... chrome/browser/history/history_service.cc:1230: void HistoryService::AddHistoryServiceObserver( Note: those two methods have been added already. I think you probably want to rebase your code on top of origin/master and remove them. https://codereview.chromium.org/573553004/diff/1/chrome/browser/history/in_me... File chrome/browser/history/in_memory_url_index.cc (right): https://codereview.chromium.org/573553004/diff/1/chrome/browser/history/in_me... chrome/browser/history/in_memory_url_index.cc:143: service->RemoveHistoryServiceObserver(this); You can use ScopedObserver<> to simplify the code. https://codereview.chromium.org/573553004/diff/1/chrome/browser/history/in_me... chrome/browser/history/in_memory_url_index.cc:301: if (service && service->backend_loaded()) { |service| can only be NULL if running tests, so rewrite as if (service) { if (!service->backend_loaded()) { service->AddObserver(this); } else { ScheduleRebuildFromHistory(); } } https://codereview.chromium.org/573553004/diff/1/chrome/browser/history/in_me... File chrome/browser/history/in_memory_url_index.h (right): https://codereview.chromium.org/573553004/diff/1/chrome/browser/history/in_me... chrome/browser/history/in_memory_url_index.h:150: virtual void HistoryServiceLoaded(HistoryService* service) OVERRIDE; OVERRIDE -> override https://codereview.chromium.org/573553004/diff/1/chrome/browser/predictors/au... File chrome/browser/predictors/autocomplete_action_predictor.cc (right): https://codereview.chromium.org/573553004/diff/1/chrome/browser/predictors/au... chrome/browser/predictors/autocomplete_action_predictor.cc:100: HistoryService* history_service = On 2014/09/23 08:45:09, sdefresne wrote: > AutocompleteActionPredictor is a KeyedService. Shouldn't the unregistration > happens in the Shutdown method instead? Please move the unregistration into AutocompleteActionPredictor::Shutdown(). https://codereview.chromium.org/573553004/diff/1/chrome/browser/predictors/au... chrome/browser/predictors/autocomplete_action_predictor.cc:102: if (history_service) Use ScopedObserver<>. https://codereview.chromium.org/573553004/diff/1/chrome/browser/predictors/au... File chrome/browser/predictors/autocomplete_action_predictor.h (right): https://codereview.chromium.org/573553004/diff/1/chrome/browser/predictors/au... chrome/browser/predictors/autocomplete_action_predictor.h:117: // Is called when |service| is loaded. // historyHistoryServiceObserver: https://codereview.chromium.org/573553004/diff/1/chrome/browser/predictors/au... chrome/browser/predictors/autocomplete_action_predictor.h:118: virtual void HistoryServiceLoaded(HistoryService* service) OVERRIDE; OVERRIDE -> override https://codereview.chromium.org/573553004/diff/1/chrome/browser/safe_browsing... File chrome/browser/safe_browsing/incident_reporting/last_download_finder.h (right): https://codereview.chromium.org/573553004/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/incident_reporting/last_download_finder.h:57: // Is called when |service| is loaded. // history::HistoryServiceObserver: https://codereview.chromium.org/573553004/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/incident_reporting/last_download_finder.h:58: virtual void HistoryServiceLoaded(HistoryService* service) OVERRIDE; OVERRIDE -> override https://codereview.chromium.org/573553004/diff/1/chrome/browser/sync/glue/boo... File chrome/browser/sync/glue/bookmark_data_type_controller.cc (right): https://codereview.chromium.org/573553004/diff/1/chrome/browser/sync/glue/boo... chrome/browser/sync/glue/bookmark_data_type_controller.cc:57: history->AddHistoryServiceObserver(this); On 2014/09/23 08:45:09, sdefresne wrote: > You never call "history->RemoveHistoryServiceObserver(this);". I think you > should do it a the location you removed the registrar_.RemoveAll(). Use ScopedObserver<>. https://codereview.chromium.org/573553004/diff/1/chrome/browser/sync/glue/boo... File chrome/browser/sync/glue/bookmark_data_type_controller.h (right): https://codereview.chromium.org/573553004/diff/1/chrome/browser/sync/glue/boo... chrome/browser/sync/glue/bookmark_data_type_controller.h:31: // Is called when |service| is loaded. // history::HistoryServiceObserver: https://codereview.chromium.org/573553004/diff/1/chrome/browser/sync/glue/boo... chrome/browser/sync/glue/bookmark_data_type_controller.h:32: virtual void HistoryServiceLoaded(HistoryService* service) OVERRIDE; OVERRIDE -> override https://codereview.chromium.org/573553004/diff/1/chrome/browser/ui/cocoa/hist... File chrome/browser/ui/cocoa/history_menu_bridge.h (right): https://codereview.chromium.org/573553004/diff/1/chrome/browser/ui/cocoa/hist... chrome/browser/ui/cocoa/history_menu_bridge.h:153: // Is called when |service| is loaded. // history::HistoryServiceObserver: https://codereview.chromium.org/573553004/diff/1/chrome/browser/ui/cocoa/hist... chrome/browser/ui/cocoa/history_menu_bridge.h:154: virtual void HistoryServiceLoaded(HistoryService* service) OVERRIDE; OVERRIDE -> override https://codereview.chromium.org/573553004/diff/1/chrome/browser/ui/cocoa/hist... File chrome/browser/ui/cocoa/history_menu_bridge.mm (right): https://codereview.chromium.org/573553004/diff/1/chrome/browser/ui/cocoa/hist... chrome/browser/ui/cocoa/history_menu_bridge.mm:82: hs->AddHistoryServiceObserver(this); Use ScopedObserver<> https://codereview.chromium.org/573553004/diff/1/chrome/browser/ui/cocoa/hist... chrome/browser/ui/cocoa/history_menu_bridge.mm:456: HistoryServiceFactory::GetForProfile(profile_, Profile::EXPLICIT_ACCESS); No need to use HistoryServiceFactory to get the HistoryService, it is passed as parameter to the function, just use it. DCHECK(history_service); DCHECK(history_service->BackendLoaded()); history_service->RemoveObserver(this); history_service_ = history_service; Init(); https://codereview.chromium.org/573553004/diff/1/chrome/browser/ui/omnibox/om... File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right): https://codereview.chromium.org/573553004/diff/1/chrome/browser/ui/omnibox/om... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:146: history_service->RemoveHistoryServiceObserver(this); Use ScopedObserver<> https://codereview.chromium.org/573553004/diff/1/chrome/browser/ui/omnibox/om... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:151: Profile* profile = browser()->profile(); You can remove this if you use ScopedObserver<> https://codereview.chromium.org/573553004/diff/1/chrome/test/base/ui_test_uti... File chrome/test/base/ui_test_utils.cc (right): https://codereview.chromium.org/573553004/diff/1/chrome/test/base/ui_test_uti... chrome/test/base/ui_test_utils.cc:593: history_service->AddHistoryServiceObserver(&observer); Use ScopedObserver https://codereview.chromium.org/573553004/diff/1/chrome/test/base/ui_test_uti... File chrome/test/base/ui_test_utils.h (right): https://codereview.chromium.org/573553004/diff/1/chrome/test/base/ui_test_uti... chrome/test/base/ui_test_utils.h:313: // Is called when |service| is loaded. // history::HistoryServiceObserver: https://codereview.chromium.org/573553004/diff/1/chrome/test/base/ui_test_uti... chrome/test/base/ui_test_utils.h:314: virtual void HistoryServiceLoaded(HistoryService* service) OVERRIDE; OVERRIDE -> override https://codereview.chromium.org/573553004/diff/1/chrome/test/base/ui_test_uti... chrome/test/base/ui_test_utils.h:315: virtual ~WaitHistoryLoadedObserver(); Move the destructor with the constructor. https://codereview.chromium.org/573553004/diff/1/components/history/core/brow... File components/history/core/browser/history_service_observer.h (right): https://codereview.chromium.org/573553004/diff/1/components/history/core/brow... components/history/core/browser/history_service_observer.h:13: class HistoryServiceObserver { On 2014/09/23 08:45:10, sdefresne wrote: > This class need to have a virtual destructor. This class has already been added, please rebase.
PTAL https://codereview.chromium.org/573553004/diff/1/chrome/browser/history/histo... File chrome/browser/history/history_service.cc (right): https://codereview.chromium.org/573553004/diff/1/chrome/browser/history/histo... chrome/browser/history/history_service.cc:1230: void HistoryService::AddHistoryServiceObserver( On 2014/10/20 13:15:41, sdefresne wrote: > Note: those two methods have been added already. I think you probably want to > rebase your code on top of origin/master and remove them. Done. https://codereview.chromium.org/573553004/diff/1/chrome/browser/history/histo... File chrome/browser/history/history_service.h (right): https://codereview.chromium.org/573553004/diff/1/chrome/browser/history/histo... chrome/browser/history/history_service.h:522: Profile* profile() { return profile_; } On 2014/09/23 08:45:09, sdefresne wrote: > We'll have to remove this method later. Can you make it private and add > LastDownloadFinder as a friend class, so that no new client is added for it, > with a comment? > // TODO(sdefresne): remove this method Done. https://codereview.chromium.org/573553004/diff/1/chrome/browser/history/in_me... File chrome/browser/history/in_memory_url_index.cc (right): https://codereview.chromium.org/573553004/diff/1/chrome/browser/history/in_me... chrome/browser/history/in_memory_url_index.cc:301: if (service && service->backend_loaded()) { On 2014/10/20 13:15:41, sdefresne wrote: > |service| can only be NULL if running tests, so rewrite as > > if (service) { > if (!service->backend_loaded()) { > service->AddObserver(this); > } else { > ScheduleRebuildFromHistory(); > } > } Done. https://codereview.chromium.org/573553004/diff/1/chrome/browser/history/in_me... File chrome/browser/history/in_memory_url_index.h (right): https://codereview.chromium.org/573553004/diff/1/chrome/browser/history/in_me... chrome/browser/history/in_memory_url_index.h:149: // Is called when |service| is loaded. On 2014/09/23 08:45:09, sdefresne wrote: > When overriding methods from an interface, you don't copy descriptions for the > methods, but just put the name of the interface providing those methods (same > for the other places). > > // history::HistoryServiceObserver: > virtual void HistoryServiceLoaded(HistoryService* service) OVERRIDE; Done. https://codereview.chromium.org/573553004/diff/1/chrome/browser/predictors/au... File chrome/browser/predictors/autocomplete_action_predictor.cc (right): https://codereview.chromium.org/573553004/diff/1/chrome/browser/predictors/au... chrome/browser/predictors/autocomplete_action_predictor.cc:100: HistoryService* history_service = On 2014/09/23 08:45:09, sdefresne wrote: > AutocompleteActionPredictor is a KeyedService. Shouldn't the unregistration > happens in the Shutdown method instead? Done. https://codereview.chromium.org/573553004/diff/1/chrome/browser/sync/glue/boo... File chrome/browser/sync/glue/bookmark_data_type_controller.cc (right): https://codereview.chromium.org/573553004/diff/1/chrome/browser/sync/glue/boo... chrome/browser/sync/glue/bookmark_data_type_controller.cc:57: history->AddHistoryServiceObserver(this); On 2014/09/23 08:45:09, sdefresne wrote: > You never call "history->RemoveHistoryServiceObserver(this);". I think you > should do it a the location you removed the registrar_.RemoveAll(). Done. https://codereview.chromium.org/573553004/diff/1/chrome/browser/sync/glue/boo... chrome/browser/sync/glue/bookmark_data_type_controller.cc:57: history->AddHistoryServiceObserver(this); On 2014/10/20 13:15:42, sdefresne wrote: > On 2014/09/23 08:45:09, sdefresne wrote: > > You never call "history->RemoveHistoryServiceObserver(this);". I think you > > should do it a the location you removed the registrar_.RemoveAll(). > > Use ScopedObserver<>. I used ScopedObserver and I m running into an issue where the HistoryService is already destroyed and then we are trying to remove the object from ScopedObserver. For now I will keep this as is if it is not an issue. https://codereview.chromium.org/573553004/diff/1/chrome/browser/sync/glue/boo... chrome/browser/sync/glue/bookmark_data_type_controller.cc:115: if (!DependentsLoaded()) On 2014/09/23 08:45:09, sdefresne wrote: > You removed the following assertion, maybe you should put it back. > DCHECK_EQ(state_, MODEL_STARTING); Done. https://codereview.chromium.org/573553004/diff/1/chrome/browser/sync/glue/boo... File chrome/browser/sync/glue/bookmark_data_type_controller.h (right): https://codereview.chromium.org/573553004/diff/1/chrome/browser/sync/glue/boo... chrome/browser/sync/glue/bookmark_data_type_controller.h:14: #include "content/public/browser/notification_observer.h" On 2014/09/23 08:45:09, sdefresne wrote: > Remove those two includes, they should no longer be required since you remove > last usage of content::Notification* classes. Done. https://codereview.chromium.org/573553004/diff/1/chrome/browser/ui/cocoa/hist... File chrome/browser/ui/cocoa/history_menu_bridge.h (right): https://codereview.chromium.org/573553004/diff/1/chrome/browser/ui/cocoa/hist... chrome/browser/ui/cocoa/history_menu_bridge.h:153: // Is called when |service| is loaded. On 2014/10/20 13:15:42, sdefresne wrote: > // history::HistoryServiceObserver: Done. https://codereview.chromium.org/573553004/diff/1/chrome/browser/ui/cocoa/hist... chrome/browser/ui/cocoa/history_menu_bridge.h:154: virtual void HistoryServiceLoaded(HistoryService* service) OVERRIDE; On 2014/10/20 13:15:42, sdefresne wrote: > OVERRIDE -> override Done. https://codereview.chromium.org/573553004/diff/1/chrome/browser/ui/cocoa/hist... File chrome/browser/ui/cocoa/history_menu_bridge.mm (right): https://codereview.chromium.org/573553004/diff/1/chrome/browser/ui/cocoa/hist... chrome/browser/ui/cocoa/history_menu_bridge.mm:456: HistoryServiceFactory::GetForProfile(profile_, Profile::EXPLICIT_ACCESS); On 2014/10/20 13:15:42, sdefresne wrote: > No need to use HistoryServiceFactory to get the HistoryService, it is passed as > parameter to the function, just use it. > > DCHECK(history_service); > DCHECK(history_service->BackendLoaded()); > history_service->RemoveObserver(this); > history_service_ = history_service; > Init(); Okay. The only reason why I did this was to ensure I received notification for the right profile. I was trying to write the code in line with the comment present before "A history service is now ready. Check to see if it's the one for the main profile." Implemented it the way you suggested. https://codereview.chromium.org/573553004/diff/1/chrome/browser/ui/omnibox/om... File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right): https://codereview.chromium.org/573553004/diff/1/chrome/browser/ui/omnibox/om... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:146: history_service->RemoveHistoryServiceObserver(this); On 2014/10/20 13:15:42, sdefresne wrote: > Use ScopedObserver<> Done. https://codereview.chromium.org/573553004/diff/1/chrome/browser/ui/omnibox/om... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:151: Profile* profile = browser()->profile(); On 2014/10/20 13:15:42, sdefresne wrote: > You can remove this if you use ScopedObserver<> Done. https://codereview.chromium.org/573553004/diff/1/chrome/test/base/ui_test_uti... File chrome/test/base/ui_test_utils.cc (right): https://codereview.chromium.org/573553004/diff/1/chrome/test/base/ui_test_uti... chrome/test/base/ui_test_utils.cc:593: history_service->AddHistoryServiceObserver(&observer); On 2014/10/20 13:15:42, sdefresne wrote: > Use ScopedObserver Done. https://codereview.chromium.org/573553004/diff/1/chrome/test/base/ui_test_uti... File chrome/test/base/ui_test_utils.h (right): https://codereview.chromium.org/573553004/diff/1/chrome/test/base/ui_test_uti... chrome/test/base/ui_test_utils.h:309: // Wait for HistoryService to load. On 2014/09/23 08:45:10, sdefresne wrote: > You can move this class in ui_test_utils.cc since it is not used from any other > file. Done.
https://codereview.chromium.org/573553004/diff/60001/chrome/browser/history/i... File chrome/browser/history/in_memory_url_index.h (right): https://codereview.chromium.org/573553004/diff/60001/chrome/browser/history/i... chrome/browser/history/in_memory_url_index.h:249: virtual void OnHistoryServiceLoaded(HistoryService* history_service) override; style: Chromium style changed so that virtual method must only have one keyword amongst "virtual", "override" or "final", so remove the "virtual" here and just keep "override". https://codereview.chromium.org/573553004/diff/60001/chrome/browser/predictor... File chrome/browser/predictors/autocomplete_action_predictor.cc (left): https://codereview.chromium.org/573553004/diff/60001/chrome/browser/predictor... chrome/browser/predictors/autocomplete_action_predictor.cc:478: // If the history service is ready, delete any old or invalid entries. Please restore comment. https://codereview.chromium.org/573553004/diff/60001/chrome/browser/predictor... File chrome/browser/predictors/autocomplete_action_predictor.cc (right): https://codereview.chromium.org/573553004/diff/60001/chrome/browser/predictor... chrome/browser/predictors/autocomplete_action_predictor.cc:472: TryDeleteOldEntries(history_service); Could you keep the old structure and comment and just change how the listening for the event is done? I.e. write it like that: if (!TryDeleteOldEntries(history_service)) { // ... if (history_service) history_service_observer_.Add(history_service); } https://codereview.chromium.org/573553004/diff/60001/chrome/browser/predictor... chrome/browser/predictors/autocomplete_action_predictor.cc:485: history_service_observer_.Add(service); See previous comment. https://codereview.chromium.org/573553004/diff/60001/chrome/browser/predictor... File chrome/browser/predictors/autocomplete_action_predictor.h (right): https://codereview.chromium.org/573553004/diff/60001/chrome/browser/predictor... chrome/browser/predictors/autocomplete_action_predictor.h:119: virtual void Shutdown() override; style: remove "virtual" https://codereview.chromium.org/573553004/diff/60001/chrome/browser/predictor... chrome/browser/predictors/autocomplete_action_predictor.h:122: virtual void OnHistoryServiceLoaded(HistoryService* history_service) override; style: remove "virtual" https://codereview.chromium.org/573553004/diff/60001/chrome/browser/predictor... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/573553004/diff/60001/chrome/browser/predictor... chrome/browser/predictors/resource_prefetch_predictor.cc:329: if (!CheckForHistoryService()) Question: is it really required to insert all those calls to CheckForHistoryService()? I'm afraid it may be introducing behaviour changes. Isn't it enough to just call the method from ResourcePrefetchPredictor::CreateCaches()? https://codereview.chromium.org/573553004/diff/60001/chrome/browser/predictor... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/573553004/diff/60001/chrome/browser/predictor... chrome/browser/predictors/resource_prefetch_predictor.h:133: virtual void OnHistoryServiceLoaded(HistoryService* history_service) override; style: remove "virtual" https://codereview.chromium.org/573553004/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc (right): https://codereview.chromium.org/573553004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc:250: AbandonSearchInProfile(content::Source<Profile>(source).ptr()); You need to stop observing the HistoryService associated to the profile here, since the HistoryService lifetime is bound to the Profile lifetime, hence it will become invalid after this event has been processed. If LastDownloadFinder outlives Profile, then the destructor of ScopedObserver will have pointers to dangling HistoryService and will cause undefined behaviour. https://codereview.chromium.org/573553004/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/last_download_finder.h (right): https://codereview.chromium.org/573553004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/last_download_finder.h:39: class LastDownloadFinder : public content::NotificationObserver, style: indent next line to line up the public https://codereview.chromium.org/573553004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/last_download_finder.h:59: virtual void OnHistoryServiceLoaded(HistoryService* service) override; style: remove "virtual" https://codereview.chromium.org/573553004/diff/60001/chrome/browser/sync/glue... File chrome/browser/sync/glue/bookmark_data_type_controller.h (right): https://codereview.chromium.org/573553004/diff/60001/chrome/browser/sync/glue... chrome/browser/sync/glue/bookmark_data_type_controller.h:31: virtual void OnHistoryServiceLoaded(HistoryService* service) override; style: remove virtual https://codereview.chromium.org/573553004/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/history_menu_bridge.h (right): https://codereview.chromium.org/573553004/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/history_menu_bridge.h:151: virtual void OnHistoryServiceLoaded(HistoryService* service) override; style: remove virtual https://codereview.chromium.org/573553004/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/history_menu_bridge.mm (right): https://codereview.chromium.org/573553004/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/history_menu_bridge.mm:465: void HistoryMenuBridge::OnHistoryServiceLoaded( nit-style: try to keep the same order for method in header and implementation https://codereview.chromium.org/573553004/diff/60001/chrome/test/base/ui_test... File chrome/test/base/ui_test_utils.cc (right): https://codereview.chromium.org/573553004/diff/60001/chrome/test/base/ui_test... chrome/test/base/ui_test_utils.cc:541: virtual ~WaitHistoryLoadedObserver() {} style: remove virtual, add override https://codereview.chromium.org/573553004/diff/60001/chrome/test/base/ui_test... chrome/test/base/ui_test_utils.cc:543: virtual void OnHistoryServiceLoaded(HistoryService* service) override { style: remove virtual
naiem.shaik@gmail.com changed reviewers: + sky@chromium.org
PTAL https://codereview.chromium.org/573553004/diff/60001/chrome/browser/history/i... File chrome/browser/history/in_memory_url_index.h (right): https://codereview.chromium.org/573553004/diff/60001/chrome/browser/history/i... chrome/browser/history/in_memory_url_index.h:249: virtual void OnHistoryServiceLoaded(HistoryService* history_service) override; On 2014/10/30 17:37:03, sdefresne wrote: > style: Chromium style changed so that virtual method must only have one keyword > amongst "virtual", "override" or "final", so remove the "virtual" here and just > keep "override". Done. https://codereview.chromium.org/573553004/diff/60001/chrome/browser/predictor... File chrome/browser/predictors/autocomplete_action_predictor.cc (left): https://codereview.chromium.org/573553004/diff/60001/chrome/browser/predictor... chrome/browser/predictors/autocomplete_action_predictor.cc:478: // If the history service is ready, delete any old or invalid entries. On 2014/10/30 17:37:04, sdefresne wrote: > Please restore comment. Done. https://codereview.chromium.org/573553004/diff/60001/chrome/browser/predictor... File chrome/browser/predictors/autocomplete_action_predictor.cc (right): https://codereview.chromium.org/573553004/diff/60001/chrome/browser/predictor... chrome/browser/predictors/autocomplete_action_predictor.cc:472: TryDeleteOldEntries(history_service); On 2014/10/30 17:37:03, sdefresne wrote: > Could you keep the old structure and comment and just change how the listening > for the event is done? I.e. write it like that: > > if (!TryDeleteOldEntries(history_service)) { > // ... > if (history_service) > history_service_observer_.Add(history_service); > } Done. https://codereview.chromium.org/573553004/diff/60001/chrome/browser/predictor... chrome/browser/predictors/autocomplete_action_predictor.cc:485: history_service_observer_.Add(service); On 2014/10/30 17:37:03, sdefresne wrote: > See previous comment. Done. https://codereview.chromium.org/573553004/diff/60001/chrome/browser/predictor... File chrome/browser/predictors/autocomplete_action_predictor.h (right): https://codereview.chromium.org/573553004/diff/60001/chrome/browser/predictor... chrome/browser/predictors/autocomplete_action_predictor.h:119: virtual void Shutdown() override; On 2014/10/30 17:37:04, sdefresne wrote: > style: remove "virtual" Done. https://codereview.chromium.org/573553004/diff/60001/chrome/browser/predictor... chrome/browser/predictors/autocomplete_action_predictor.h:122: virtual void OnHistoryServiceLoaded(HistoryService* history_service) override; On 2014/10/30 17:37:04, sdefresne wrote: > style: remove "virtual" Done. https://codereview.chromium.org/573553004/diff/60001/chrome/browser/predictor... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/573553004/diff/60001/chrome/browser/predictor... chrome/browser/predictors/resource_prefetch_predictor.cc:329: if (!CheckForHistoryService()) On 2014/10/30 17:37:04, sdefresne wrote: > Question: is it really required to insert all those calls to > CheckForHistoryService()? I'm afraid it may be introducing behaviour changes. > Isn't it enough to just call the method from > ResourcePrefetchPredictor::CreateCaches()? I was trying to be extra careful which I think is needless. I just realized that StartInitialization is called every time RecordMainFrameLoadComplete is done. Changed the code. https://codereview.chromium.org/573553004/diff/60001/chrome/browser/predictor... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/573553004/diff/60001/chrome/browser/predictor... chrome/browser/predictors/resource_prefetch_predictor.h:133: virtual void OnHistoryServiceLoaded(HistoryService* history_service) override; On 2014/10/30 17:37:04, sdefresne wrote: > style: remove "virtual" Done. https://codereview.chromium.org/573553004/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc (right): https://codereview.chromium.org/573553004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc:250: AbandonSearchInProfile(content::Source<Profile>(source).ptr()); On 2014/10/30 17:37:04, sdefresne wrote: > You need to stop observing the HistoryService associated to the profile here, > since the HistoryService lifetime is bound to the Profile lifetime, hence it > will become invalid after this event has been processed. If LastDownloadFinder > outlives Profile, then the destructor of ScopedObserver will have pointers to > dangling HistoryService and will cause undefined behaviour. Fixed it. https://codereview.chromium.org/573553004/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/last_download_finder.h (right): https://codereview.chromium.org/573553004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/last_download_finder.h:39: class LastDownloadFinder : public content::NotificationObserver, On 2014/10/30 17:37:04, sdefresne wrote: > style: indent next line to line up the public Done. https://codereview.chromium.org/573553004/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/last_download_finder.h:59: virtual void OnHistoryServiceLoaded(HistoryService* service) override; On 2014/10/30 17:37:04, sdefresne wrote: > style: remove "virtual" Done. https://codereview.chromium.org/573553004/diff/60001/chrome/browser/sync/glue... File chrome/browser/sync/glue/bookmark_data_type_controller.h (right): https://codereview.chromium.org/573553004/diff/60001/chrome/browser/sync/glue... chrome/browser/sync/glue/bookmark_data_type_controller.h:31: virtual void OnHistoryServiceLoaded(HistoryService* service) override; On 2014/10/30 17:37:04, sdefresne wrote: > style: remove virtual Done. https://codereview.chromium.org/573553004/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/history_menu_bridge.h (right): https://codereview.chromium.org/573553004/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/history_menu_bridge.h:151: virtual void OnHistoryServiceLoaded(HistoryService* service) override; On 2014/10/30 17:37:04, sdefresne wrote: > style: remove virtual Done. https://codereview.chromium.org/573553004/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/history_menu_bridge.mm (right): https://codereview.chromium.org/573553004/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/history_menu_bridge.mm:465: void HistoryMenuBridge::OnHistoryServiceLoaded( On 2014/10/30 17:37:04, sdefresne wrote: > nit-style: try to keep the same order for method in header and implementation Done. https://codereview.chromium.org/573553004/diff/60001/chrome/test/base/ui_test... File chrome/test/base/ui_test_utils.cc (right): https://codereview.chromium.org/573553004/diff/60001/chrome/test/base/ui_test... chrome/test/base/ui_test_utils.cc:541: virtual ~WaitHistoryLoadedObserver() {} On 2014/10/30 17:37:04, sdefresne wrote: > style: remove virtual, add override Done. https://codereview.chromium.org/573553004/diff/60001/chrome/test/base/ui_test... chrome/test/base/ui_test_utils.cc:543: virtual void OnHistoryServiceLoaded(HistoryService* service) override { On 2014/10/30 17:37:04, sdefresne wrote: > style: remove virtual Done.
The CQ bit was checked by naiem.shaik@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/573553004/80001
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was unchecked by naiem.shaik@gmail.com
@sdefresne,sky, PTAL: @sky: chrome/browser/chrome_notification_types.h chrome/browser/predictors/autocomplete_action_predictor.cc chrome/browser/predictors/autocomplete_action_predictor.h chrome/browser/predictors/resource_prefetch_predictor.cc chrome/browser/predictors/resource_prefetch_predictor.h chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc chrome/browser/safe_browsing/incident_reporting/last_download_finder.h chrome/browser/sync/glue/bookmark_data_type_controller.cc chrome/browser/sync/glue/bookmark_data_type_controller.h chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc chrome/browser/ui/cocoa/history_menu_bridge.h chrome/browser/ui/cocoa/history_menu_bridge.mm chrome/browser/ui/omnibox/omnibox_view_browsertest.cc chrome/test/base/ui_test_utils.cc @sdefresne: chrome/browser/history/history_service.h chrome/browser/history/in_memory_url_index.cc chrome/browser/history/in_memory_url_index.h components/history/core/browser/history_service_observer.h
sky@chromium.org changed reviewers: + brettw@chromium.org - sky@chromium.org
sky->brettw
On 2014/10/31 18:06:01, sky wrote: > sky->brettw PTAL.
Just one comment and some nits. This is starting to look good. https://codereview.chromium.org/573553004/diff/80001/chrome/browser/predictor... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/573553004/diff/80001/chrome/browser/predictor... chrome/browser/predictors/resource_prefetch_predictor.cc:1212: HistoryServiceFactory::GetForProfile(profile_, Profile::EXPLICIT_ACCESS); nit: it would be simpler to test whether history_service is not NULL here, something like if (!history_service) return false; if (history_service->BackendLoaded()) { OnHistoryAndCacheLoaded(); return true; } DCHECK(!history_service_observer_.IsObserving(history_service)); history_service_observer_.Add(history_service); return false; https://codereview.chromium.org/573553004/diff/80001/chrome/browser/predictor... chrome/browser/predictors/resource_prefetch_predictor.cc:1217: } else if (history_service && not: no need for else here since the if branch always return https://codereview.chromium.org/573553004/diff/80001/chrome/browser/predictor... chrome/browser/predictors/resource_prefetch_predictor.cc:1218: !history_service_observer_.IsObserving(history_service)) { nit: ResourcePrefetchPredictor::CreateCaches asserts that it is only called once, so ResourcePrefetchPredictor::CheckForHistoryService can only be called once, and this test will always be true, so use DCHECK instead. https://codereview.chromium.org/573553004/diff/80001/chrome/browser/predictor... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/573553004/diff/80001/chrome/browser/predictor... chrome/browser/predictors/resource_prefetch_predictor.h:298: // else Observe for the HistoryService to be loaded. nit: s/Observe/wait for OnHistoryLoaded event/ maybe https://codereview.chromium.org/573553004/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/history_menu_bridge.mm (right): https://codereview.chromium.org/573553004/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/history_menu_bridge.mm:265: history_service_observer_.Remove(history_service); Do not call Remove here, as HistoryMenuBridge is interested in the other events (OnURLVisited, and later OnURLsModified and OnURLsDeleted from HistoryServiceObserver).
@sdefresne, Thanks for your comments. PTAL. https://codereview.chromium.org/573553004/diff/80001/chrome/browser/predictor... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/573553004/diff/80001/chrome/browser/predictor... chrome/browser/predictors/resource_prefetch_predictor.cc:1212: HistoryServiceFactory::GetForProfile(profile_, Profile::EXPLICIT_ACCESS); On 2014/11/03 19:14:13, sdefresne wrote: > nit: it would be simpler to test whether history_service is not NULL here, > something like > > if (!history_service) > return false; > if (history_service->BackendLoaded()) { > OnHistoryAndCacheLoaded(); > return true; > } > DCHECK(!history_service_observer_.IsObserving(history_service)); > history_service_observer_.Add(history_service); > return false; Done. https://codereview.chromium.org/573553004/diff/80001/chrome/browser/predictor... chrome/browser/predictors/resource_prefetch_predictor.cc:1217: } else if (history_service && On 2014/11/03 19:14:13, sdefresne wrote: > not: no need for else here since the if branch always return Done. https://codereview.chromium.org/573553004/diff/80001/chrome/browser/predictor... chrome/browser/predictors/resource_prefetch_predictor.cc:1218: !history_service_observer_.IsObserving(history_service)) { On 2014/11/03 19:14:13, sdefresne wrote: > nit: ResourcePrefetchPredictor::CreateCaches asserts that it is only called > once, so ResourcePrefetchPredictor::CheckForHistoryService can only be called > once, and this test will always be true, so use DCHECK instead. Done. https://codereview.chromium.org/573553004/diff/80001/chrome/browser/predictor... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/573553004/diff/80001/chrome/browser/predictor... chrome/browser/predictors/resource_prefetch_predictor.h:298: // else Observe for the HistoryService to be loaded. On 2014/11/03 19:14:13, sdefresne wrote: > nit: s/Observe/wait for OnHistoryLoaded event/ maybe Done. https://codereview.chromium.org/573553004/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/history_menu_bridge.mm (right): https://codereview.chromium.org/573553004/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/history_menu_bridge.mm:265: history_service_observer_.Remove(history_service); On 2014/11/03 19:14:13, sdefresne wrote: > Do not call Remove here, as HistoryMenuBridge is interested in the other events > (OnURLVisited, and later OnURLsModified and OnURLsDeleted from > HistoryServiceObserver). Done.
Sorry, I missed an issue in my previous review. https://codereview.chromium.org/573553004/diff/100001/chrome/browser/history/... File chrome/browser/history/history_service.h (right): https://codereview.chromium.org/573553004/diff/100001/chrome/browser/history/... chrome/browser/history/history_service.h:854: // TODO(sdefresne): remove this method. nit: add reference to http://crbug.com/430070 (I just created it, sorry). https://codereview.chromium.org/573553004/diff/100001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/573553004/diff/100001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:1209: bool ResourcePrefetchPredictor::CheckForHistoryService() { nit: you never use the return value, so you can change the method to return void https://codereview.chromium.org/573553004/diff/100001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc (right): https://codereview.chromium.org/573553004/diff/100001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc:250: HistoryService* history_service = HistoryServiceFactory::GetForProfile( nit: use HistoryServiceFactory::GetForProfileIfExists() to avoid creating an HistoryService during the Profile shutdown. https://codereview.chromium.org/573553004/diff/100001/chrome/browser/sync/glu... File chrome/browser/sync/glue/bookmark_data_type_controller.cc (right): https://codereview.chromium.org/573553004/diff/100001/chrome/browser/sync/glu... chrome/browser/sync/glue/bookmark_data_type_controller.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. I think you are introducing a logic error in the code. The original code did register on both BookmarkModel and HistoryService if any of them was not loaded, and then on notification, unregistered from both of them. In the modified code, you do not correctly unregister in all code path. I recommend that you use two ScopedObserver to simplify the logic of the code. Then both BookmarkDataTypeController::BookmarkModelLoaded and BookmarkDataTypeController::OnHistoryServiceLoaded should have the following structure: scoped_observer_for_this_service.RemoveAll(); if (!DependentLoaded()) return; scoped_observer_for_other_service.RemoveAll(); OnModelLoaded(); Then CleanupState() can just clean both ScopedObservers.
PTAL. https://codereview.chromium.org/573553004/diff/100001/chrome/browser/history/... File chrome/browser/history/history_service.h (right): https://codereview.chromium.org/573553004/diff/100001/chrome/browser/history/... chrome/browser/history/history_service.h:854: // TODO(sdefresne): remove this method. On 2014/11/04 14:12:29, sdefresne wrote: > nit: add reference to http://crbug.com/430070 (I just created it, sorry). Done. https://codereview.chromium.org/573553004/diff/100001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/573553004/diff/100001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:1209: bool ResourcePrefetchPredictor::CheckForHistoryService() { On 2014/11/04 14:12:29, sdefresne wrote: > nit: you never use the return value, so you can change the method to return void Done. https://codereview.chromium.org/573553004/diff/100001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc (right): https://codereview.chromium.org/573553004/diff/100001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc:250: HistoryService* history_service = HistoryServiceFactory::GetForProfile( On 2014/11/04 14:12:29, sdefresne wrote: > nit: use HistoryServiceFactory::GetForProfileIfExists() to avoid creating an > HistoryService during the Profile shutdown. Done. https://codereview.chromium.org/573553004/diff/100001/chrome/browser/sync/glu... File chrome/browser/sync/glue/bookmark_data_type_controller.cc (right): https://codereview.chromium.org/573553004/diff/100001/chrome/browser/sync/glu... chrome/browser/sync/glue/bookmark_data_type_controller.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2014/11/04 14:12:29, sdefresne wrote: > I think you are introducing a logic error in the code. > > The original code did register on both BookmarkModel and HistoryService if any > of them was not loaded, and then on notification, unregistered from both of > them. In the modified code, you do not correctly unregister in all code path. I > recommend that you use two ScopedObserver to simplify the logic of the code. > > Then both BookmarkDataTypeController::BookmarkModelLoaded and > BookmarkDataTypeController::OnHistoryServiceLoaded should have the following > structure: > > scoped_observer_for_this_service.RemoveAll(); > if (!DependentLoaded()) > return; > scoped_observer_for_other_service.RemoveAll(); > OnModelLoaded(); > > Then CleanupState() can just clean both ScopedObservers. Done.
https://codereview.chromium.org/573553004/diff/120001/chrome/browser/history/... File chrome/browser/history/in_memory_url_index.cc (right): https://codereview.chromium.org/573553004/diff/120001/chrome/browser/history/... chrome/browser/history/in_memory_url_index.cc:305: history_service_observer_.Add(service); InMemoryURLIndex constructor already register 'this' as an observer, so this line should either be removed or protected with "!history_service_observer_.IsObserving(service))" if this is required to make the tests pass. https://codereview.chromium.org/573553004/diff/120001/chrome/browser/history/... chrome/browser/history/in_memory_url_index.cc:384: history_service_observer_.Remove(history_service); Remove this line, otherwise InMemoryURLIndex::OnURLVisited won't be called. https://codereview.chromium.org/573553004/diff/120001/chrome/browser/sync/glu... File chrome/browser/sync/glue/bookmark_data_type_controller.cc (right): https://codereview.chromium.org/573553004/diff/120001/chrome/browser/sync/glu... chrome/browser/sync/glue/bookmark_data_type_controller.cc:36: installed_bookmark_observer_(false), You can remove installed_bookmark_observer_ everywhere since it is no longer useful (ScopedObserver already has the necessary information). The field is only set, never read which is another indication that it's time is over. https://codereview.chromium.org/573553004/diff/120001/chrome/browser/ui/omnib... File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right): https://codereview.chromium.org/573553004/diff/120001/chrome/browser/ui/omnib... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:145: // history::HisoryServiceObserver: nit: history::HistoryServiceObserver https://codereview.chromium.org/573553004/diff/120001/chrome/browser/ui/omnib... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:146: virtual void OnHistoryServiceLoaded( style: remove "virtual" and add "override" instead
PTAL https://codereview.chromium.org/573553004/diff/120001/chrome/browser/history/... File chrome/browser/history/in_memory_url_index.cc (right): https://codereview.chromium.org/573553004/diff/120001/chrome/browser/history/... chrome/browser/history/in_memory_url_index.cc:305: history_service_observer_.Add(service); On 2014/11/05 14:51:52, sdefresne wrote: > InMemoryURLIndex constructor already register 'this' as an observer, so this > line should either be removed or protected with > "!history_service_observer_.IsObserving(service))" if this is required to make > the tests pass. Done. https://codereview.chromium.org/573553004/diff/120001/chrome/browser/history/... chrome/browser/history/in_memory_url_index.cc:384: history_service_observer_.Remove(history_service); On 2014/11/05 14:51:52, sdefresne wrote: > Remove this line, otherwise InMemoryURLIndex::OnURLVisited won't be called. My bad. fixed it. https://codereview.chromium.org/573553004/diff/120001/chrome/browser/sync/glu... File chrome/browser/sync/glue/bookmark_data_type_controller.cc (right): https://codereview.chromium.org/573553004/diff/120001/chrome/browser/sync/glu... chrome/browser/sync/glue/bookmark_data_type_controller.cc:36: installed_bookmark_observer_(false), On 2014/11/05 14:51:52, sdefresne wrote: > You can remove installed_bookmark_observer_ everywhere since it is no longer > useful (ScopedObserver already has the necessary information). The field is only > set, never read which is another indication that it's time is over. Took care of this, but I had to listen to NOTIFICATION_PROFILE_DESTROYED for taking care of clean up. Please let me know if there is a better way to do this. https://codereview.chromium.org/573553004/diff/120001/chrome/browser/ui/omnib... File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right): https://codereview.chromium.org/573553004/diff/120001/chrome/browser/ui/omnib... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:145: // history::HisoryServiceObserver: On 2014/11/05 14:51:52, sdefresne wrote: > nit: history::HistoryServiceObserver Done. https://codereview.chromium.org/573553004/diff/120001/chrome/browser/ui/omnib... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:146: virtual void OnHistoryServiceLoaded( On 2014/11/05 14:51:52, sdefresne wrote: > style: remove "virtual" and add "override" instead Not sure how i missed it. Fixed it..
https://codereview.chromium.org/573553004/diff/140001/chrome/browser/sync/glu... File chrome/browser/sync/glue/bookmark_data_type_controller.cc (right): https://codereview.chromium.org/573553004/diff/140001/chrome/browser/sync/glu... chrome/browser/sync/glue/bookmark_data_type_controller.cc:39: notification_registrar_.Add(this, chrome::NOTIFICATION_PROFILE_DESTROYED, Another option would be to add a HistoryServiceBeingDeleted() method to HistoryServiceObserver (like there is a BookmarkModelBeingDeleted() in BookmarkModelObserver) and to call the CleanUpState() method from there. This is probably better since other objects may have to listen for that notification. You can then that notification from HistoryService::~HistoryService.
PTAL. https://codereview.chromium.org/573553004/diff/140001/chrome/browser/sync/glu... File chrome/browser/sync/glue/bookmark_data_type_controller.cc (right): https://codereview.chromium.org/573553004/diff/140001/chrome/browser/sync/glu... chrome/browser/sync/glue/bookmark_data_type_controller.cc:39: notification_registrar_.Add(this, chrome::NOTIFICATION_PROFILE_DESTROYED, On 2014/11/07 16:39:42, sdefresne wrote: > Another option would be to add a HistoryServiceBeingDeleted() method to > HistoryServiceObserver (like there is a BookmarkModelBeingDeleted() in > BookmarkModelObserver) and to call the CleanUpState() method from there. This is > probably better since other objects may have to listen for that notification. > > You can then that notification from HistoryService::~HistoryService. Done. For now I am not listening to this notification anywhere else. If it is needed to be done for rest of the classes for ensuring there are no crashes, please let me know.
LGTM brettw: can you also have a look, it's a complex refactoring, and I'd prefer you to also review it, thank you. https://codereview.chromium.org/573553004/diff/160001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc (right): https://codereview.chromium.org/573553004/diff/160001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc:253: if (history_service) nit: you could now instead implement HistoryServiceObserver::HistoyrServiceBeingDeleted() (optional, listening to chrome::NOTIFICATION_PROFILE_DESTROYED also work).
@brettw, PTAL. https://codereview.chromium.org/573553004/diff/160001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc (right): https://codereview.chromium.org/573553004/diff/160001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc:253: if (history_service) On 2014/11/08 15:16:42, sdefresne wrote: > nit: you could now instead implement > HistoryServiceObserver::HistoyrServiceBeingDeleted() (optional, listening to > chrome::NOTIFICATION_PROFILE_DESTROYED also work). Done.
@brettw, PTAL.
PTAL. phajdan.jr: chrome/test/ thakis: chrome/browser/predictors/ mattm: chrome/browser/safe_browsing/ atwilson: chrome/browser/sync/ pkasting: chrome/browser/ui/
https://codereview.chromium.org/573553004/diff/180001/chrome/browser/history/... File chrome/browser/history/in_memory_url_index.cc (right): https://codereview.chromium.org/573553004/diff/180001/chrome/browser/history/... chrome/browser/history/in_memory_url_index.cc:305: !history_service_observer_.IsObserving(service)) { Are you sure you want to change this condition like this? This will do a rebuild in the case that you are already observing, which isn't what the old code did. https://codereview.chromium.org/573553004/diff/180001/chrome/browser/predicto... File chrome/browser/predictors/autocomplete_action_predictor.cc (right): https://codereview.chromium.org/573553004/diff/180001/chrome/browser/predicto... chrome/browser/predictors/autocomplete_action_predictor.cc:604: void AutocompleteActionPredictor::Shutdown() { The order doesn't match the header file. https://codereview.chromium.org/573553004/diff/180001/chrome/browser/predicto... File chrome/browser/predictors/autocomplete_action_predictor.h (right): https://codereview.chromium.org/573553004/diff/180001/chrome/browser/predicto... chrome/browser/predictors/autocomplete_action_predictor.h:119: void Shutdown() override; I would usually make these private just so it's obviously not part of the classes public API (even though since we don't have private inheritance other classes can still theoretically call it). https://codereview.chromium.org/573553004/diff/180001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/573553004/diff/180001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:1225: void ResourcePrefetchPredictor::OnHistoryServiceLoaded( This should match the order in the header. https://codereview.chromium.org/573553004/diff/180001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/573553004/diff/180001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:133: void OnHistoryServiceLoaded(HistoryService* history_service) override; I would make this private as I mentioned above. https://codereview.chromium.org/573553004/diff/180001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:297: // If HistoryService exists and its Backend is loaded, This comment reads like code. I can read the code. Something a little higher level and easier to read would be better. Also, this function name doesn't quite seem right. Maybe something more like "ConnectToHistoryService" would be more appropriate? https://codereview.chromium.org/573553004/diff/180001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc (right): https://codereview.chromium.org/573553004/diff/180001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc:248: case chrome::NOTIFICATION_PROFILE_DESTROYED: { I think this change is unnecessary. https://codereview.chromium.org/573553004/diff/180001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc:257: void LastDownloadFinder::OnHistoryServiceLoaded( Order should match. https://codereview.chromium.org/573553004/diff/180001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/last_download_finder.h (right): https://codereview.chromium.org/573553004/diff/180001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/last_download_finder.h:59: void OnHistoryServiceLoaded(HistoryService* service) override; Private or protected. https://codereview.chromium.org/573553004/diff/180001/chrome/browser/sync/glu... File chrome/browser/sync/glue/bookmark_data_type_controller.cc (right): https://codereview.chromium.org/573553004/diff/180001/chrome/browser/sync/glu... chrome/browser/sync/glue/bookmark_data_type_controller.cc:105: void BookmarkDataTypeController::OnHistoryServiceLoaded( order https://codereview.chromium.org/573553004/diff/180001/chrome/browser/sync/glu... File chrome/browser/sync/glue/bookmark_data_type_controller.h (right): https://codereview.chromium.org/573553004/diff/180001/chrome/browser/sync/glu... chrome/browser/sync/glue/bookmark_data_type_controller.h:31: void OnHistoryServiceLoaded(HistoryService* service) override; private. https://codereview.chromium.org/573553004/diff/180001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/history_menu_bridge.mm (right): https://codereview.chromium.org/573553004/diff/180001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/history_menu_bridge.mm:78: if (hs != NULL && hs->BackendLoaded()) { Can you delete the "!= NULL" here so it looks the same as your new condition (which I think is better than explicitly comparing to NULL). https://codereview.chromium.org/573553004/diff/180001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/history_menu_bridge.mm:473: I think this is a mistake. https://codereview.chromium.org/573553004/diff/180001/chrome/test/base/ui_tes... File chrome/test/base/ui_test_utils.cc (right): https://codereview.chromium.org/573553004/diff/180001/chrome/test/base/ui_tes... chrome/test/base/ui_test_utils.cc:541: // history::HistoryServiceObserver: Blank line before here.
brettw: Thanks for your comments. PTAL. https://codereview.chromium.org/573553004/diff/180001/chrome/browser/history/... File chrome/browser/history/in_memory_url_index.cc (right): https://codereview.chromium.org/573553004/diff/180001/chrome/browser/history/... chrome/browser/history/in_memory_url_index.cc:305: !history_service_observer_.IsObserving(service)) { On 2014/11/14 21:13:06, brettw wrote: > Are you sure you want to change this condition like this? This will do a rebuild > in the case that you are already observing, which isn't what the old code did. Fixed it. https://codereview.chromium.org/573553004/diff/180001/chrome/browser/predicto... File chrome/browser/predictors/autocomplete_action_predictor.cc (right): https://codereview.chromium.org/573553004/diff/180001/chrome/browser/predicto... chrome/browser/predictors/autocomplete_action_predictor.cc:604: void AutocompleteActionPredictor::Shutdown() { On 2014/11/14 21:13:06, brettw wrote: > The order doesn't match the header file. Done. https://codereview.chromium.org/573553004/diff/180001/chrome/browser/predicto... File chrome/browser/predictors/autocomplete_action_predictor.h (right): https://codereview.chromium.org/573553004/diff/180001/chrome/browser/predicto... chrome/browser/predictors/autocomplete_action_predictor.h:119: void Shutdown() override; On 2014/11/14 21:13:06, brettw wrote: > I would usually make these private just so it's obviously not part of the > classes public API (even though since we don't have private inheritance other > classes can still theoretically call it). Done. https://codereview.chromium.org/573553004/diff/180001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/573553004/diff/180001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:1225: void ResourcePrefetchPredictor::OnHistoryServiceLoaded( On 2014/11/14 21:13:06, brettw wrote: > This should match the order in the header. Done. https://codereview.chromium.org/573553004/diff/180001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/573553004/diff/180001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:133: void OnHistoryServiceLoaded(HistoryService* history_service) override; On 2014/11/14 21:13:06, brettw wrote: > I would make this private as I mentioned above. Done. https://codereview.chromium.org/573553004/diff/180001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:297: // If HistoryService exists and its Backend is loaded, On 2014/11/14 21:13:06, brettw wrote: > This comment reads like code. I can read the code. Something a little higher > level and easier to read would be better. > > Also, this function name doesn't quite seem right. Maybe something more like > "ConnectToHistoryService" would be more appropriate? Done. https://codereview.chromium.org/573553004/diff/180001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc (right): https://codereview.chromium.org/573553004/diff/180001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc:248: case chrome::NOTIFICATION_PROFILE_DESTROYED: { On 2014/11/14 21:13:06, brettw wrote: > I think this change is unnecessary. Done. https://codereview.chromium.org/573553004/diff/180001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc:257: void LastDownloadFinder::OnHistoryServiceLoaded( On 2014/11/14 21:13:07, brettw wrote: > Order should match. Done. https://codereview.chromium.org/573553004/diff/180001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/last_download_finder.h (right): https://codereview.chromium.org/573553004/diff/180001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/last_download_finder.h:59: void OnHistoryServiceLoaded(HistoryService* service) override; On 2014/11/14 21:13:07, brettw wrote: > Private or protected. Done. https://codereview.chromium.org/573553004/diff/180001/chrome/browser/sync/glu... File chrome/browser/sync/glue/bookmark_data_type_controller.cc (right): https://codereview.chromium.org/573553004/diff/180001/chrome/browser/sync/glu... chrome/browser/sync/glue/bookmark_data_type_controller.cc:105: void BookmarkDataTypeController::OnHistoryServiceLoaded( On 2014/11/14 21:13:07, brettw wrote: > order Done. https://codereview.chromium.org/573553004/diff/180001/chrome/browser/sync/glu... File chrome/browser/sync/glue/bookmark_data_type_controller.h (right): https://codereview.chromium.org/573553004/diff/180001/chrome/browser/sync/glu... chrome/browser/sync/glue/bookmark_data_type_controller.h:31: void OnHistoryServiceLoaded(HistoryService* service) override; On 2014/11/14 21:13:07, brettw wrote: > private. Done. https://codereview.chromium.org/573553004/diff/180001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/history_menu_bridge.mm (right): https://codereview.chromium.org/573553004/diff/180001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/history_menu_bridge.mm:78: if (hs != NULL && hs->BackendLoaded()) { On 2014/11/14 21:13:07, brettw wrote: > Can you delete the "!= NULL" here so it looks the same as your new condition > (which I think is better than explicitly comparing to NULL). Done. https://codereview.chromium.org/573553004/diff/180001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/history_menu_bridge.mm:473: On 2014/11/14 21:13:07, brettw wrote: > I think this is a mistake. Done. https://codereview.chromium.org/573553004/diff/180001/chrome/test/base/ui_tes... File chrome/test/base/ui_test_utils.cc (right): https://codereview.chromium.org/573553004/diff/180001/chrome/test/base/ui_tes... chrome/test/base/ui_test_utils.cc:541: // history::HistoryServiceObserver: On 2014/11/14 21:13:07, brettw wrote: > Blank line before here. Done.
lgtm
The CQ bit was checked by naiem.shaik@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/573553004/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by naiem.shaik@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/573553004/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by naiem.shaik@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/573553004/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
atwilson@chromium.org changed reviewers: + zea@chromium.org
LG, but adding zea@ in case he wants to review since I'm only passingly familiar with the bookmark model code.
chrome/test LGTM
c/b/ui LGTM https://codereview.chromium.org/573553004/diff/200001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/history_menu_bridge.mm (right): https://codereview.chromium.org/573553004/diff/200001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/history_menu_bridge.mm:78: if (hs && hs->BackendLoaded()) { Nit: Longer, but seems a little clearer to me: if (hs) { if (hs->BackendLoaded()) { history_service_ = hs; Init(); } else { history_service_observer_.Add(hs); } } https://codereview.chromium.org/573553004/diff/200001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/history_menu_bridge.mm:135: // changed. Nit: Please update the comment ("other" is now confusing). https://codereview.chromium.org/573553004/diff/200001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/history_menu_bridge.mm:266: DCHECK(history_service->BackendLoaded()); Nit: I dunno how valuable these DCHECKs are (doesn't the function name sort of imply them?) https://codereview.chromium.org/573553004/diff/200001/chrome/browser/ui/omnib... File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right): https://codereview.chromium.org/573553004/diff/200001/chrome/browser/ui/omnib... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:372: }; Nit: Technically, this class should probably have DISALLOW_COPY_AND_ASSIGN (and really should have had it already)
@pkasting, Thanks for your comments. PTAL. https://codereview.chromium.org/573553004/diff/200001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/history_menu_bridge.mm (right): https://codereview.chromium.org/573553004/diff/200001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/history_menu_bridge.mm:78: if (hs && hs->BackendLoaded()) { On 2014/11/17 18:45:31, Peter Kasting wrote: > Nit: Longer, but seems a little clearer to me: > > if (hs) { > if (hs->BackendLoaded()) { > history_service_ = hs; > Init(); > } else { > history_service_observer_.Add(hs); > } > } Done. https://codereview.chromium.org/573553004/diff/200001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/history_menu_bridge.mm:135: // changed. On 2014/11/17 18:45:32, Peter Kasting wrote: > Nit: Please update the comment ("other" is now confusing). Done. https://codereview.chromium.org/573553004/diff/200001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/history_menu_bridge.mm:266: DCHECK(history_service->BackendLoaded()); On 2014/11/17 18:45:31, Peter Kasting wrote: > Nit: I dunno how valuable these DCHECKs are (doesn't the function name sort of > imply them?) Done. https://codereview.chromium.org/573553004/diff/200001/chrome/browser/ui/omnib... File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right): https://codereview.chromium.org/573553004/diff/200001/chrome/browser/ui/omnib... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:372: }; On 2014/11/17 18:45:32, Peter Kasting wrote: > Nit: Technically, this class should probably have DISALLOW_COPY_AND_ASSIGN (and > really should have had it already) Done.
The CQ bit was checked by naiem.shaik@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/573553004/220001
Still LGTM
sync LGTM with a request https://codereview.chromium.org/573553004/diff/200001/chrome/browser/sync/glu... File chrome/browser/sync/glue/bookmark_data_type_controller.cc (right): https://codereview.chromium.org/573553004/diff/200001/chrome/browser/sync/glu... chrome/browser/sync/glue/bookmark_data_type_controller.cc:46: bookmark_model_ = BookmarkModelFactory::GetForProfile(profile_); nit: lets get rid of this member and just do GetForProfile everywhere? (same as you're doing for the history_service)
The CQ bit was unchecked by naiem.shaik@gmail.com
@zea, Thanks for your comments. PTAL. https://codereview.chromium.org/573553004/diff/200001/chrome/browser/sync/glu... File chrome/browser/sync/glue/bookmark_data_type_controller.cc (right): https://codereview.chromium.org/573553004/diff/200001/chrome/browser/sync/glu... chrome/browser/sync/glue/bookmark_data_type_controller.cc:46: bookmark_model_ = BookmarkModelFactory::GetForProfile(profile_); On 2014/11/17 23:59:50, Nicolas Zea wrote: > nit: lets get rid of this member and just do GetForProfile everywhere? (same as > you're doing for the history_service) Done.
safe_browsing lgtm with nit https://codereview.chromium.org/573553004/diff/240001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc (right): https://codereview.chromium.org/573553004/diff/240001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc:200: // waiting for OnHistoryServiceLoaded.. nit: extra .
@mattm, Thanks. fixed the nit. https://codereview.chromium.org/573553004/diff/240001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc (right): https://codereview.chromium.org/573553004/diff/240001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/last_download_finder.cc:200: // waiting for OnHistoryServiceLoaded.. On 2014/11/18 02:19:54, mattm wrote: > nit: extra . Done.
The CQ bit was checked by naiem.shaik@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/573553004/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by naiem.shaik@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/573553004/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by naiem.shaik@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/573553004/300001
\o/ https://codereview.chromium.org/573553004/diff/300001/chrome/browser/chrome_n... File chrome/browser/chrome_notification_types.h (left): https://codereview.chromium.org/573553004/diff/300001/chrome/browser/chrome_n... chrome/browser/chrome_notification_types.h:236: NOTIFICATION_HISTORY_LOADED, Whoo hoo!
Message was sent while issue was closed.
Committed patchset #14 (id:300001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/793ee5bff54aa3cb1c1873dfeeeb1f36c42e810f Cr-Commit-Position: refs/heads/master@{#306027}
Message was sent while issue was closed.
mpearson@chromium.org changed reviewers: + mpearson@chromium.org
Message was sent while issue was closed.
lgtm with one trivial comment Thanks for tracking this down and fixing it. Our users will appreciate it. --mark |
