Remove NOTIFICATION_HISTORY_URLS_DELETED

Remove NOTIFICATION_HISTORY_URLS_DELETED

Add the method OnURLsDeleted() to History{Backend,Service}Observer and
change History{Backend,Service} to call this method instead of sending
the notification.

Port client code to implement the History{Backend,Service}Observer
interface instead of using chrome::NOTIFICATION_HISTORY_URLS_DELETED.

Remove obsolete code for broadcasting notification from history code and
HistoryDetails type.

BUG=373326
Committed: https://crrev.com/5017fc699bb6c353538103770944216450eaa43c
Cr-Commit-Position: refs/heads/master@{#313406}

[nshaik, 2014-12-03 20:19:54]

naiem.shaik@gmail.com changed reviewers:
+ sdefresne@chromium.org

[nshaik, 2014-12-03 20:20:20]

@sdefresne, PTAL.

[sdefresne, 2014-12-04 17:21:13]

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/android/p...
File chrome/browser/android/provider/chrome_browser_provider.cc (right):

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/android/p...
chrome/browser/android/provider/chrome_browser_provider.cc:1635: if (type ==
chrome::NOTIFICATION_HISTORY_KEYWORD_SEARCH_TERM_UPDATED) {
Since this is now the last notification that use content::NotificationObserver,
change this to

DCHECK_EQ(type, chrome::NOTIFICATION_HISTORY_KEYWORD_SEARCH_TERM_UPDATED);

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/shortcuts_backend.cc (right):

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_backend.cc:190: if (type ==
extensions::NOTIFICATION_EXTENSION_UNLOADED_DEPRECATED) {
Same here, this is the only notification listened for, so use DCHECK:

DCHECK_EQ(type, extensions::NOTIFICATION_EXTENSION_UNLOADED_DEPRECATED);

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_backend.cc:336: for
(GuidMap::const_iterator it(guid_map_.begin()); it != guid_map_.end();
nit: c++11 for-loop and auto are now allowed, so this can be rewritten into

for (const auto& guid_pair : guid_map_) {
  if (std::find_if(row.begin(), rows.end(),
                   history::URLRow::URLRowHasURL(
                       guid_pair.second.second.match_core.destination_url)) !=
      rows.end()) {
    shortcut_ids.push_back(guid_pair.first);
  }
}

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/history/history_api.cc (right):

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/history/history_api.cc:21: #include
"chrome/browser/chrome_notification_types.h"
This include can also be removed ;-)

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/history/history_api.cc:155:
HistoryUrlsRemoved(history_service->profile(), &deleted_details);
Remove HistoryUrlsRemoved() method and inline the body there.

Use profile_ instead of history_service->profile() and remove the friend
declaration in HistoryService. If you want to be sure this is the correct
profile, you can add a DCHECK:

DCHECK_EQ(HistoryServiceFactory::GetForProfileIfExists(profile_,
Profile::EXPLICIT_ACCESS), profile_);

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/a...
File chrome/browser/history/android/android_provider_backend_unittest.cc (left):

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/a...
chrome/browser/history/android/android_provider_backend_unittest.cc:90:
DCHECK_EQ(type, chrome::NOTIFICATION_HISTORY_URLS_DELETED);
nit: NOTREACHED();

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
File chrome/browser/history/history_backend.cc (right):

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_backend.cc:2538: const_cast<URLRows*>(&rows));
Do *not* use const_cast, instead copy rows into a local variable and use it for
the whole function. Note that typed_url_syncable_service_->OnUrlsDeleted(...)
may change the URLRows, so you need to use that updated list when calling the
delegate / observers.

URLRows copied_rows(rows);
if (typed_url_syncable_service_.get()) {
  typed_url_syncable_service_->OnUrlsDeleted(all_history, expired,
&copied_rows);
}

FOR_EACH_OBSERVER(HistoryBackedObserver, observers_,
                  OnURLsDeleted(this, all_history, expired, copied_rows));

if (delegate_)
  delegate_->NotifyURLsDeleted(all_history, expired, copied_rows);

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
File chrome/browser/history/history_backend.h (right):

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_backend.h:151: const URLsDeletedDetails&
deleted_details) = 0;
I'd prefer if we also stopped using URLsDeletedDetails by declaring the method
as:

virtual void NotifyURLsDeleted(bool all_history,
                               bool expired,
                               const URLRows& rows) = 0;

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
File chrome/browser/history/history_backend_unittest.cc (right):

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_backend_unittest.cc:145: typedef
std::vector<std::pair<int, HistoryDetails*> > NotificationList;
This is no longer used, remove.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_backend_unittest.cc:156:
STLDeleteValues(&broadcasted_notifications_);
This is no longer used, remove.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_backend_unittest.cc:188: int
num_broadcasted_notifications() const {
This is no longer used, remove.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_backend_unittest.cc:192: const NotificationList&
broadcasted_notifications() const {
This is no longer used, remove.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_backend_unittest.cc:192: const NotificationList&
broadcasted_notifications() const {
This is no longer used, remove.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_backend_unittest.cc:197:
url_visited_notifications_.clear();
Please clear urls_deleted_notifications_ here too.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_backend_unittest.cc:199:
STLDeleteValues(&broadcasted_notifications_);
This is no longer used, remove.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_backend_unittest.cc:232:
content::Details<HistoryDetails> det(details.get());
This method should not longer be called, please delete.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_backend_unittest.cc:275: NotificationList
broadcasted_notifications_;
This is no longer used, remove.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
File chrome/browser/history/history_notifications.h (right):

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_notifications.h:20: struct URLsModifiedDetails :
public HistoryDetails {
Please delete this struct, it is no longer used.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_notifications.h:30: struct URLsDeletedDetails :
public HistoryDetails {
I'd like to get this removed; please see my comments about NotifyURLsDeleted and
OnURLsDeleted on how to do it.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
File chrome/browser/history/history_service.h (right):

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_service.h:539: friend class
extensions::HistoryEventRouter;
Please remove, we do not want a dependency (even if just a friend).

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_service.h:617: void NotifyURLsDeleted(const
history::URLsDeletedDetails& deleted_details);
I'd like to also remove URLsDeletedDetails by declaring the method like this:

void NotifyURLsDeleted(bool all_history,
                       bool expired,
                       const URLRows& rows) = 0;

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_service.h:623: // Notify all
HistoryServiceObservers regigstered that HistoryService is being
nit: registered

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/i...
File chrome/browser/history/in_memory_history_backend.cc (right):

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/i...
chrome/browser/history/in_memory_history_backend.cc:104: for
(URLRows::const_iterator row = details.rows.begin();
nit: you can use the new c++11 for loops here too.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/i...
File chrome/browser/history/in_memory_url_index.cc (right):

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/i...
chrome/browser/history/in_memory_url_index.cc:11: #include
"chrome/browser/chrome_notification_types.h"
This #include can be removed too.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/i...
File chrome/browser/history/in_memory_url_index_unittest.cc (right):

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/i...
chrome/browser/history/in_memory_url_index_unittest.cc:18: #include
"chrome/browser/chrome_notification_types.h"
This #include can be removed.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/t...
File chrome/browser/history/top_sites_impl.cc (right):

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/t...
chrome/browser/history/top_sites_impl.cc:744: if (type ==
content::NOTIFICATION_NAV_ENTRY_COMMITTED) {
This is the only notification listened for, so DCHECK

DCHECK_EQ(type, content::NOTIFICATION_NAV_ENTRY_COMMITTED);

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/predictor...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/predictor...
chrome/browser/predictors/resource_prefetch_predictor.cc:18: #include
"chrome/browser/chrome_notification_types.h"
This #include is no longer useful, remove.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/favicon_cache.cc (right):

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/favicon_cache.cc:9: #include
"chrome/browser/chrome_notification_types.h"
Again, unused #include ;-)

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/typed_url_change_processor.cc (left):

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/typed_url_change_processor.cc:55: if
(!content::NotificationService::current())
\o/

https://codereview.chromium.org/773103004/diff/20001/components/history/core/...
File components/history/core/browser/history_backend_observer.h (right):

https://codereview.chromium.org/773103004/diff/20001/components/history/core/...
components/history/core/browser/history_backend_observer.h:45: virtual void
OnURLsDeleted(
So, looking at the calling site of the
chrome::NOTIFICATION_HISTORY_URLS_DELETED, if all_history is set to true, both
expired and deleted_rows are ignored, so we can either have two methods:

virtual void OnAllURLsDeleted(HistoryBackend* history_backend) = 0;
virtual void OnURLsDeleted(HistoryBackend* history_backend,
                           bool expired,
                           const URLRows& deleted_rows) = 0;

If you think this is too much work, I'd like to have at least the method not use
URLsDeletedDetails and instead be defined as below with a comment stating that
if all_history is true, the two other parameters are undefined:

virtual void OnURLsDeleted(HistoryBackend* history_backend,
                           bool all_history,
                           bool expired,
                           const URLRows& deleted_rows) = 0;

https://codereview.chromium.org/773103004/diff/20001/components/history/core/...
File components/history/core/browser/history_service_observer.h (right):

https://codereview.chromium.org/773103004/diff/20001/components/history/core/...
components/history/core/browser/history_service_observer.h:50: virtual void
OnURLsDeleted(
Same comment as for HistoryBackendObserver.

[nshaik, 2014-12-04 18:37:08]

https://codereview.chromium.org/773103004/diff/20001/components/history/core/...
File components/history/core/browser/history_backend_observer.h (right):

https://codereview.chromium.org/773103004/diff/20001/components/history/core/...
components/history/core/browser/history_backend_observer.h:45: virtual void
OnURLsDeleted(
On 2014/12/04 17:21:13, sdefresne wrote:
> So, looking at the calling site of the
> chrome::NOTIFICATION_HISTORY_URLS_DELETED, if all_history is set to true, both
> expired and deleted_rows are ignored, so we can either have two methods:
> 
> virtual void OnAllURLsDeleted(HistoryBackend* history_backend) = 0;
> virtual void OnURLsDeleted(HistoryBackend* history_backend,
>                            bool expired,
>                            const URLRows& deleted_rows) = 0;
> 
> If you think this is too much work, I'd like to have at least the method not
use
> URLsDeletedDetails and instead be defined as below with a comment stating that
> if all_history is true, the two other parameters are undefined:
> 
> virtual void OnURLsDeleted(HistoryBackend* history_backend,
>                            bool all_history,
>                            bool expired,
>                            const URLRows& deleted_rows) = 0;

Should I remove the |favicon_urls| and also remove it from
HistoryBackendNotifier?

[sdefresne, 2014-12-04 18:46:57]

On 2014/12/04 at 18:37:08, naiem.shaik wrote:
>
https://codereview.chromium.org/773103004/diff/20001/components/history/core/...
> File components/history/core/browser/history_backend_observer.h (right):
> 
>
https://codereview.chromium.org/773103004/diff/20001/components/history/core/...
> components/history/core/browser/history_backend_observer.h:45: virtual void
OnURLsDeleted(
> On 2014/12/04 17:21:13, sdefresne wrote:
> > So, looking at the calling site of the
> > chrome::NOTIFICATION_HISTORY_URLS_DELETED, if all_history is set to true,
both
> > expired and deleted_rows are ignored, so we can either have two methods:
> > 
> > virtual void OnAllURLsDeleted(HistoryBackend* history_backend) = 0;
> > virtual void OnURLsDeleted(HistoryBackend* history_backend,
> >                            bool expired,
> >                            const URLRows& deleted_rows) = 0;
> > 
> > If you think this is too much work, I'd like to have at least the method not
use
> > URLsDeletedDetails and instead be defined as below with a comment stating
that
> > if all_history is true, the two other parameters are undefined:
> > 
> > virtual void OnURLsDeleted(HistoryBackend* history_backend,
> >                            bool all_history,
> >                            bool expired,
> >                            const URLRows& deleted_rows) = 0;
> 
> Should I remove the |favicon_urls| and also remove it from
HistoryBackendNotifier?

Ooops, good catch. No, you should add it to the method, I forgot about it.
Apparently it is used by chrome/browser/sync/glue/favicon_cache.cc :-)

[nshaik, 2014-12-07 09:34:50]

PTAL.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/android/p...
File chrome/browser/android/provider/chrome_browser_provider.cc (right):

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/android/p...
chrome/browser/android/provider/chrome_browser_provider.cc:1635: if (type ==
chrome::NOTIFICATION_HISTORY_KEYWORD_SEARCH_TERM_UPDATED) {
On 2014/12/04 17:21:12, sdefresne wrote:
> Since this is now the last notification that use
content::NotificationObserver,
> change this to
> 
> DCHECK_EQ(type, chrome::NOTIFICATION_HISTORY_KEYWORD_SEARCH_TERM_UPDATED);

Done.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/shortcuts_backend.cc (right):

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_backend.cc:190: if (type ==
extensions::NOTIFICATION_EXTENSION_UNLOADED_DEPRECATED) {
On 2014/12/04 17:21:12, sdefresne wrote:
> Same here, this is the only notification listened for, so use DCHECK:
> 
> DCHECK_EQ(type, extensions::NOTIFICATION_EXTENSION_UNLOADED_DEPRECATED);

Done.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_backend.cc:336: for
(GuidMap::const_iterator it(guid_map_.begin()); it != guid_map_.end();
On 2014/12/04 17:21:12, sdefresne wrote:
> nit: c++11 for-loop and auto are now allowed, so this can be rewritten into
> 
> for (const auto& guid_pair : guid_map_) {
>   if (std::find_if(row.begin(), rows.end(),
>                    history::URLRow::URLRowHasURL(
>                        guid_pair.second.second.match_core.destination_url)) !=
>       rows.end()) {
>     shortcut_ids.push_back(guid_pair.first);
>   }
> }

Done.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/history/history_api.cc (right):

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/history/history_api.cc:21: #include
"chrome/browser/chrome_notification_types.h"
On 2014/12/04 17:21:12, sdefresne wrote:
> This include can also be removed ;-)

:) Done

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/history/history_api.cc:155:
HistoryUrlsRemoved(history_service->profile(), &deleted_details);
On 2014/12/04 17:21:12, sdefresne wrote:
> Remove HistoryUrlsRemoved() method and inline the body there.
> 
> Use profile_ instead of history_service->profile() and remove the friend
> declaration in HistoryService. If you want to be sure this is the correct
> profile, you can add a DCHECK:
> 
> DCHECK_EQ(HistoryServiceFactory::GetForProfileIfExists(profile_,
> Profile::EXPLICIT_ACCESS), profile_);

Done.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/a...
File chrome/browser/history/android/android_provider_backend_unittest.cc (left):

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/a...
chrome/browser/history/android/android_provider_backend_unittest.cc:90:
DCHECK_EQ(type, chrome::NOTIFICATION_HISTORY_URLS_DELETED);
On 2014/12/04 17:21:12, sdefresne wrote:
> nit: NOTREACHED();

Done.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
File chrome/browser/history/history_backend.cc (right):

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_backend.cc:2538: const_cast<URLRows*>(&rows));
On 2014/12/04 17:21:12, sdefresne wrote:
> Do *not* use const_cast, instead copy rows into a local variable and use it
for
> the whole function. Note that typed_url_syncable_service_->OnUrlsDeleted(...)
> may change the URLRows, so you need to use that updated list when calling the
> delegate / observers.
> 
> URLRows copied_rows(rows);
> if (typed_url_syncable_service_.get()) {
>   typed_url_syncable_service_->OnUrlsDeleted(all_history, expired,
> &copied_rows);
> }
> 
> FOR_EACH_OBSERVER(HistoryBackedObserver, observers_,
>                   OnURLsDeleted(this, all_history, expired, copied_rows));
> 
> if (delegate_)
>   delegate_->NotifyURLsDeleted(all_history, expired, copied_rows);

Done.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
File chrome/browser/history/history_backend.h (right):

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_backend.h:151: const URLsDeletedDetails&
deleted_details) = 0;
On 2014/12/04 17:21:12, sdefresne wrote:
> I'd prefer if we also stopped using URLsDeletedDetails by declaring the method
> as:
> 
> virtual void NotifyURLsDeleted(bool all_history,
>                                bool expired,
>                                const URLRows& rows) = 0;

Done.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
File chrome/browser/history/history_backend_unittest.cc (right):

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_backend_unittest.cc:145: typedef
std::vector<std::pair<int, HistoryDetails*> > NotificationList;
On 2014/12/04 17:21:13, sdefresne wrote:
> This is no longer used, remove.

Done.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_backend_unittest.cc:156:
STLDeleteValues(&broadcasted_notifications_);
On 2014/12/04 17:21:12, sdefresne wrote:
> This is no longer used, remove.

Done.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_backend_unittest.cc:188: int
num_broadcasted_notifications() const {
On 2014/12/04 17:21:12, sdefresne wrote:
> This is no longer used, remove.

Done.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_backend_unittest.cc:192: const NotificationList&
broadcasted_notifications() const {
On 2014/12/04 17:21:12, sdefresne wrote:
> This is no longer used, remove.

Done.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_backend_unittest.cc:192: const NotificationList&
broadcasted_notifications() const {
On 2014/12/04 17:21:12, sdefresne wrote:
> This is no longer used, remove.

Done.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_backend_unittest.cc:197:
url_visited_notifications_.clear();
On 2014/12/04 17:21:12, sdefresne wrote:
> Please clear urls_deleted_notifications_ here too.

sorry missed it. done.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_backend_unittest.cc:199:
STLDeleteValues(&broadcasted_notifications_);
On 2014/12/04 17:21:12, sdefresne wrote:
> This is no longer used, remove.

Done.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_backend_unittest.cc:232:
content::Details<HistoryDetails> det(details.get());
On 2014/12/04 17:21:12, sdefresne wrote:
> This method should not longer be called, please delete.

I think this is still needed for
chrome::NOTIFICATION_HISTORY_KEYWORD_SEARCH_TERM_UPDATED.
Should i work for removing that or leave this for now?

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_backend_unittest.cc:275: NotificationList
broadcasted_notifications_;
On 2014/12/04 17:21:13, sdefresne wrote:
> This is no longer used, remove.

Done.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
File chrome/browser/history/history_notifications.h (right):

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_notifications.h:20: struct URLsModifiedDetails :
public HistoryDetails {
On 2014/12/04 17:21:13, sdefresne wrote:
> Please delete this struct, it is no longer used.

Done.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_notifications.h:30: struct URLsDeletedDetails :
public HistoryDetails {
On 2014/12/04 17:21:13, sdefresne wrote:
> I'd like to get this removed; please see my comments about NotifyURLsDeleted
and
> OnURLsDeleted on how to do it.

had the same thoughts. done.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
File chrome/browser/history/history_service.h (right):

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_service.h:539: friend class
extensions::HistoryEventRouter;
On 2014/12/04 17:21:13, sdefresne wrote:
> Please remove, we do not want a dependency (even if just a friend).

Done.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_service.h:617: void NotifyURLsDeleted(const
history::URLsDeletedDetails& deleted_details);
On 2014/12/04 17:21:13, sdefresne wrote:
> I'd like to also remove URLsDeletedDetails by declaring the method like this:
> 
> void NotifyURLsDeleted(bool all_history,
>                        bool expired,
>                        const URLRows& rows) = 0;

Done.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_service.h:623: // Notify all
HistoryServiceObservers regigstered that HistoryService is being
On 2014/12/04 17:21:13, sdefresne wrote:
> nit: registered

Done.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/i...
File chrome/browser/history/in_memory_history_backend.cc (right):

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/i...
chrome/browser/history/in_memory_history_backend.cc:104: for
(URLRows::const_iterator row = details.rows.begin();
On 2014/12/04 17:21:13, sdefresne wrote:
> nit: you can use the new c++11 for loops here too.

Done.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/i...
File chrome/browser/history/in_memory_url_index.cc (right):

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/i...
chrome/browser/history/in_memory_url_index.cc:11: #include
"chrome/browser/chrome_notification_types.h"
On 2014/12/04 17:21:13, sdefresne wrote:
> This #include can be removed too.

Done.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/i...
File chrome/browser/history/in_memory_url_index_unittest.cc (right):

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/i...
chrome/browser/history/in_memory_url_index_unittest.cc:18: #include
"chrome/browser/chrome_notification_types.h"
On 2014/12/04 17:21:13, sdefresne wrote:
> This #include can be removed.

Done.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/t...
File chrome/browser/history/top_sites_impl.cc (right):

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/history/t...
chrome/browser/history/top_sites_impl.cc:744: if (type ==
content::NOTIFICATION_NAV_ENTRY_COMMITTED) {
On 2014/12/04 17:21:13, sdefresne wrote:
> This is the only notification listened for, so DCHECK
> 
> DCHECK_EQ(type, content::NOTIFICATION_NAV_ENTRY_COMMITTED);

Done.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/predictor...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/predictor...
chrome/browser/predictors/resource_prefetch_predictor.cc:18: #include
"chrome/browser/chrome_notification_types.h"
On 2014/12/04 17:21:13, sdefresne wrote:
> This #include is no longer useful, remove.

Done.

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/favicon_cache.cc (right):

https://codereview.chromium.org/773103004/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/favicon_cache.cc:9: #include
"chrome/browser/chrome_notification_types.h"
On 2014/12/04 17:21:13, sdefresne wrote:
> Again, unused #include ;-)

Done.

https://codereview.chromium.org/773103004/diff/20001/components/history/core/...
File components/history/core/browser/history_service_observer.h (right):

https://codereview.chromium.org/773103004/diff/20001/components/history/core/...
components/history/core/browser/history_service_observer.h:50: virtual void
OnURLsDeleted(
On 2014/12/04 17:21:13, sdefresne wrote:
> Same comment as for HistoryBackendObserver.

Done.

[sdefresne, 2014-12-08 17:41:06]

https://codereview.chromium.org/773103004/diff/120001/chrome/browser/android/...
File chrome/browser/android/provider/chrome_browser_provider.h (right):

https://codereview.chromium.org/773103004/diff/120001/chrome/browser/android/...
chrome/browser/android/provider/chrome_browser_provider.h:194: void
OnURLsDeleted(HistoryBackend* history_backend bool all_history,
This won't compile due to the missing comma. Should be fixed in:

void OnURLsDeleted(HistoryBackend* history_backend,
                   bool all_history,
                   bool expired,
                   const URLRows& deleted_rows,
                   const std::set<GURL>& favicon_urls) override;

https://codereview.chromium.org/773103004/diff/120001/chrome/browser/autocomp...
File chrome/browser/autocomplete/shortcuts_backend.cc (right):

https://codereview.chromium.org/773103004/diff/120001/chrome/browser/autocomp...
chrome/browser/autocomplete/shortcuts_backend.cc:95: HistoryService* hs =
Since you're adding a dependency on HistoryService to ShortcutsBackend that is
also a KeyedService, you should explicit that dependency into the factory:

ShortcutsBackendFactory::ShortcutsBackendFactory()
    : RefcountedBrowserContextKeyedServiceFactory(
        "ShortcutsBackend",
        BrowserContextDependencyManager::GetInstance()) {
  DependsOn(HistoryServiceFactory::GetInstance());
}

https://codereview.chromium.org/773103004/diff/120001/chrome/browser/history/...
File chrome/browser/history/expire_history_backend_unittest.cc (right):

https://codereview.chromium.org/773103004/diff/120001/chrome/browser/history/...
chrome/browser/history/expire_history_backend_unittest.cc:20: #include
"chrome/browser/history/history_details.h"
You should be able to remove this #include if you remove the notifications_
instance variable (I think it is unused).

https://codereview.chromium.org/773103004/diff/120001/chrome/browser/history/...
chrome/browser/history/expire_history_backend_unittest.cc:117: typedef
std::vector< std::pair<int, HistoryDetails*> >
I think this typedef and the corresponding instance variable are no longer
useful.

https://codereview.chromium.org/773103004/diff/120001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

https://codereview.chromium.org/773103004/diff/120001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:769: const std::string
url_spec = it.url().spec();
nit: const std::string& url_spec = it.url().spec();

https://codereview.chromium.org/773103004/diff/120001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:775: const std::string
host = it.url().host();
nit: const std::string& host = it.url().host();

https://codereview.chromium.org/773103004/diff/120001/components/history/core...
File components/history/core/browser/history_backend_observer.h (right):

https://codereview.chromium.org/773103004/diff/120001/components/history/core...
components/history/core/browser/history_backend_observer.h:39: const
history::URLRows& changed_urls) = 0;
nit: history::URLRows -> URLRows

[nshaik, 2014-12-08 21:29:53]

PTAL.

https://codereview.chromium.org/773103004/diff/120001/chrome/browser/android/...
File chrome/browser/android/provider/chrome_browser_provider.h (right):

https://codereview.chromium.org/773103004/diff/120001/chrome/browser/android/...
chrome/browser/android/provider/chrome_browser_provider.h:194: void
OnURLsDeleted(HistoryBackend* history_backend bool all_history,
On 2014/12/08 17:41:06, sdefresne wrote:
> This won't compile due to the missing comma. Should be fixed in:
> 
> void OnURLsDeleted(HistoryBackend* history_backend,
>                    bool all_history,
>                    bool expired,
>                    const URLRows& deleted_rows,
>                    const std::set<GURL>& favicon_urls) override;
> 

aaagh, I was hoping I didnt end up doing something like this..

https://codereview.chromium.org/773103004/diff/120001/chrome/browser/autocomp...
File chrome/browser/autocomplete/shortcuts_backend.cc (right):

https://codereview.chromium.org/773103004/diff/120001/chrome/browser/autocomp...
chrome/browser/autocomplete/shortcuts_backend.cc:95: HistoryService* hs =
On 2014/12/08 17:41:06, sdefresne wrote:
> Since you're adding a dependency on HistoryService to ShortcutsBackend that is
> also a KeyedService, you should explicit that dependency into the factory:
> 
> ShortcutsBackendFactory::ShortcutsBackendFactory()
>     : RefcountedBrowserContextKeyedServiceFactory(
>         "ShortcutsBackend",
>         BrowserContextDependencyManager::GetInstance()) {
>   DependsOn(HistoryServiceFactory::GetInstance());
> }

Done.

https://codereview.chromium.org/773103004/diff/120001/chrome/browser/history/...
File chrome/browser/history/expire_history_backend_unittest.cc (right):

https://codereview.chromium.org/773103004/diff/120001/chrome/browser/history/...
chrome/browser/history/expire_history_backend_unittest.cc:20: #include
"chrome/browser/history/history_details.h"
On 2014/12/08 17:41:06, sdefresne wrote:
> You should be able to remove this #include if you remove the notifications_
> instance variable (I think it is unused).

Done.

https://codereview.chromium.org/773103004/diff/120001/chrome/browser/history/...
chrome/browser/history/expire_history_backend_unittest.cc:117: typedef
std::vector< std::pair<int, HistoryDetails*> >
On 2014/12/08 17:41:06, sdefresne wrote:
> I think this typedef and the corresponding instance variable are no longer
> useful.

Done.

https://codereview.chromium.org/773103004/diff/120001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

https://codereview.chromium.org/773103004/diff/120001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:769: const std::string
url_spec = it.url().spec();
On 2014/12/08 17:41:06, sdefresne wrote:
> nit: const std::string& url_spec = it.url().spec();

Done.

https://codereview.chromium.org/773103004/diff/120001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:775: const std::string
host = it.url().host();
On 2014/12/08 17:41:06, sdefresne wrote:
> nit: const std::string& host = it.url().host();

Done.

https://codereview.chromium.org/773103004/diff/120001/components/history/core...
File components/history/core/browser/history_backend_observer.h (right):

https://codereview.chromium.org/773103004/diff/120001/components/history/core...
components/history/core/browser/history_backend_observer.h:39: const
history::URLRows& changed_urls) = 0;
On 2014/12/08 17:41:06, sdefresne wrote:
> nit: history::URLRows -> URLRows

Done.

[sdefresne, 2014-12-10 16:41:47]

LGTM

https://codereview.chromium.org/773103004/diff/160001/chrome/browser/autocomp...
File chrome/browser/autocomplete/shortcuts_backend.cc (right):

https://codereview.chromium.org/773103004/diff/160001/chrome/browser/autocomp...
chrome/browser/autocomplete/shortcuts_backend.cc:339: deleted_rows.end())
style: In general, curly braces are not required for single-line statements, but
they are allowed if you like them; conditional or loop statements with complex
conditions or statements may be more readable with curly braces. Some projects
require that an if must always always have an accompanying brace.

I would recommend putting braces here since the conditional is complex.

[nshaik, 2014-12-11 03:11:08]

naiem.shaik@gmail.com changed reviewers:
+ arv@chromium.org, jeremy@chromium.org, kalman@chromium.org,
miguelg@chromium.org, pavely@chromium.org, sky@chromium.org

[nshaik, 2014-12-11 03:11:09]

PTAL, OWNERs:

miguelg@chromium.org:
chrome/browser/android/provider/

kalman@chromium.org:
chrome/browser/extensions/api/



sky@chromium.org:
chrome/browser/autocomplete/
chrome/browser/predictors/
chrome/browser/ssl (This is just inclusion of a header file, I can include ssl
specific OWNERs if needed. please let me know)

pavely@chromium.org
chrome/browser/sync/


jeremy@chromium.org
chrome/browser/ui/cocoa/

arv@chromium.org
chrome/browser/ui/webui/

https://codereview.chromium.org/773103004/diff/160001/chrome/browser/autocomp...
File chrome/browser/autocomplete/shortcuts_backend.cc (right):

https://codereview.chromium.org/773103004/diff/160001/chrome/browser/autocomp...
chrome/browser/autocomplete/shortcuts_backend.cc:339: deleted_rows.end())
On 2014/12/10 16:41:47, sdefresne wrote:
> style: In general, curly braces are not required for single-line statements,
but
> they are allowed if you like them; conditional or loop statements with complex
> conditions or statements may be more readable with curly braces. Some projects
> require that an if must always always have an accompanying brace.
> 
> I would recommend putting braces here since the conditional is complex.

Done.

[Miguel Garcia, 2014-12-11 10:09:11]

miguelg@chromium.org changed reviewers:
+ haaawk@chromium.org

[Miguel Garcia, 2014-12-11 10:09:12]

+haaawk since this will affect stuff downstream. Please wait until he comments
before landing this.

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/android/...
File chrome/browser/android/provider/chrome_browser_provider.h (left):

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/android/...
chrome/browser/android/provider/chrome_browser_provider.h:189: virtual void
OnURLVisited(HistoryService* history_service,
This method needs to remain virtual since it is overriden in the downstream (non
public) repository.

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/android/...
File chrome/browser/android/provider/chrome_browser_provider.h (right):

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/android/...
chrome/browser/android/provider/chrome_browser_provider.h:194: void
OnURLsDeleted(HistoryService* history_service,
and it probably makes sense to make this virtual too for the same reason (it
will be overriden downstream as well).

[haaawk, 2014-12-11 10:34:26]

haaawk@google.com changed reviewers:
+ ianwen@chromium.org

[haaawk, 2014-12-11 10:36:14]

Thanks Miguel. You're right. This will break data_observer.h downstream.
+Ian. Could you adapt data_observer.h to this change please?

On 2014/12/11 10:09:12, Miguel Garcia wrote:
> +haaawk since this will affect stuff downstream. Please wait until he comments
> before landing this.
> 
>
https://codereview.chromium.org/773103004/diff/180001/chrome/browser/android/...
> File chrome/browser/android/provider/chrome_browser_provider.h (left):
> 
>
https://codereview.chromium.org/773103004/diff/180001/chrome/browser/android/...
> chrome/browser/android/provider/chrome_browser_provider.h:189: virtual void
> OnURLVisited(HistoryService* history_service,
> This method needs to remain virtual since it is overriden in the downstream
(non
> public) repository.
> 
>
https://codereview.chromium.org/773103004/diff/180001/chrome/browser/android/...
> File chrome/browser/android/provider/chrome_browser_provider.h (right):
> 
>
https://codereview.chromium.org/773103004/diff/180001/chrome/browser/android/...
> chrome/browser/android/provider/chrome_browser_provider.h:194: void
> OnURLsDeleted(HistoryService* history_service,
> and it probably makes sense to make this virtual too for the same reason (it
> will be overriden downstream as well).

[sdefresne, 2014-12-11 10:48:07]

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/android/...
File chrome/browser/android/provider/chrome_browser_provider.h (left):

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/android/...
chrome/browser/android/provider/chrome_browser_provider.h:189: virtual void
OnURLVisited(HistoryService* history_service,
On 2014/12/11 10:09:12, Miguel Garcia wrote:
> This method needs to remain virtual since it is overriden in the downstream
(non
> public) repository.

miguelg: It is still virtual. But C++ style guide says that methods should only
have one of "virtual", "override", "final". Since the method is "override", then
we remove "virtual" (which is redundant since you cannot override a non-virtual
method).

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/android/...
File chrome/browser/android/provider/chrome_browser_provider.h (right):

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/android/...
chrome/browser/android/provider/chrome_browser_provider.h:194: void
OnURLsDeleted(HistoryService* history_service,
On 2014/12/11 10:09:12, Miguel Garcia wrote:
> and it probably makes sense to make this virtual too for the same reason (it
> will be overriden downstream as well).

miguelg: See previous comment about "virtual", "override" and "final".

[Miguel Garcia, 2014-12-11 11:38:10]

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/android/...
File chrome/browser/android/provider/chrome_browser_provider.h (right):

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/android/...
chrome/browser/android/provider/chrome_browser_provider.h:194: void
OnURLsDeleted(HistoryService* history_service,
On 2014/12/11 10:48:06, sdefresne wrote:
> On 2014/12/11 10:09:12, Miguel Garcia wrote:
> > and it probably makes sense to make this virtual too for the same reason (it
> > will be overriden downstream as well).
> 
> miguelg: See previous comment about "virtual", "override" and "final".

Yeah I missed the override bit, never mind.

[sky, 2014-12-11 15:59:54]

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_quick_provider_unittest.cc (right):

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_quick_provider_unittest.cc:106: // Wait for
OnURLsDeletedNotification.
Waits for OnOURSDeletedNotification and when run quits the supplied run loop.

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_quick_provider_unittest.cc:121: };
DISALLOW_...

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_quick_provider_unittest.cc:141:
scoped_refptr<content::MessageLoopRunner> runner =
Why not use a RunLoop on the stack here? Seems simpler.

[sky, 2014-12-11 16:00:22]

sky@chromium.org changed reviewers:
+ pkasting@chromium.org

[sky, 2014-12-11 16:00:23]

+pkasting in case he wants to see changes to autocomplete.

[sdefresne, 2014-12-11 16:04:09]

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_quick_provider_unittest.cc (right):

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_quick_provider_unittest.cc:141:
scoped_refptr<content::MessageLoopRunner> runner =
On 2014/12/11 15:59:54, sky wrote:
> Why not use a RunLoop on the stack here? Seems simpler.

content::MessageLoopRunner is a base::RefCounted and has a private destructor.
The only way to get it destroyed is to go through a Release call that does end
up calling "delete this".

[sky, 2014-12-11 17:02:04]

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_quick_provider_unittest.cc (right):

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_quick_provider_unittest.cc:141:
scoped_refptr<content::MessageLoopRunner> runner =
On 2014/12/11 16:04:08, sdefresne wrote:
> On 2014/12/11 15:59:54, sky wrote:
> > Why not use a RunLoop on the stack here? Seems simpler.
> 
> content::MessageLoopRunner is a base::RefCounted and has a private destructor.
> The only way to get it destroyed is to go through a Release call that does end
> up calling "delete this".

I'm suggesting you change this line to:

base::RunLoop runner;

[not at google - send to devlin, 2014-12-11 17:27:36]

extensions lgtm

[pavely, 2014-12-11 19:45:36]

chrome/browser/sync/ lgtm

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/sync/glu...
File chrome/browser/sync/glue/favicon_cache_unittest.cc (right):

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/sync/glu...
chrome/browser/sync/glue/favicon_cache_unittest.cc:1497: std::set<GURL>
favicon_urls;
Could you rename this variable to favicon_urls_to_delete.

[Peter Kasting, 2014-12-11 19:59:35]

c/b/autocomplete LGTM

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_quick_provider_unittest.cc (right):

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_quick_provider_unittest.cc:119: // weak
Nit: Blank line above this; also, comments should have capitalization, periods,
etc.  (You could take the opportunity to say who actually owns the runner.)

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/autocomp...
File chrome/browser/autocomplete/shortcuts_backend.cc (right):

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/autocomp...
chrome/browser/autocomplete/shortcuts_backend.cc:185: DCHECK_EQ(type,
extensions::NOTIFICATION_EXTENSION_UNLOADED_DEPRECATED);
Nit: (expected, actual)

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/autocomp...
chrome/browser/autocomplete/shortcuts_backend.cc:193: false);
Nit: I think both these are legal, but somehow this looks less weird to me:

  DeleteShortcutsWithURL(
      content::Details<extensions::UnloadedExtensionInfo>(
          details)->extension->url(),
      false);

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/autocomp...
chrome/browser/autocomplete/shortcuts_backend.cc:328: if (all_history) {
Nit: I'd probably put a blank line above this

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/autocomp...
chrome/browser/autocomplete/shortcuts_backend.cc:339: deleted_rows.end()) {
Nit: {} unnecessary

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/autocomp...
File chrome/browser/autocomplete/shortcuts_backend.h (right):

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/autocomp...
chrome/browser/autocomplete/shortcuts_backend.h:134: //
history::HistoryServiceObserver:
Nit: Please move this to just below the private NotificationObserver override
(also in the .cc file).

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/autocomp...
chrome/browser/autocomplete/shortcuts_backend.h:161:
ScopedObserver<HistoryService, HistoryServiceObserver>
Nit: I would probably move this above the |no_db_access_| variable so that it
sits right by the similarly-purposed notification registrar.

[nshaik, 2014-12-12 05:40:56]

PTAL.

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/android/...
File chrome/browser/android/provider/chrome_browser_provider.h (right):

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/android/...
chrome/browser/android/provider/chrome_browser_provider.h:194: void
OnURLsDeleted(HistoryService* history_service,
On 2014/12/11 11:38:10, Miguel Garcia wrote:
> On 2014/12/11 10:48:06, sdefresne wrote:
> > On 2014/12/11 10:09:12, Miguel Garcia wrote:
> > > and it probably makes sense to make this virtual too for the same reason
(it
> > > will be overriden downstream as well).
> > 
> > miguelg: See previous comment about "virtual", "override" and "final".
> 
> Yeah I missed the override bit, never mind.

Acknowledged.

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_quick_provider_unittest.cc (right):

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_quick_provider_unittest.cc:106: // Wait for
OnURLsDeletedNotification.
On 2014/12/11 15:59:54, sky wrote:
> Waits for OnOURSDeletedNotification and when run quits the supplied run loop.

Done.

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_quick_provider_unittest.cc:119: // weak
On 2014/12/11 19:59:34, Peter Kasting wrote:
> Nit: Blank line above this; also, comments should have capitalization,
periods,
> etc.  (You could take the opportunity to say who actually owns the runner.)

Done.

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_quick_provider_unittest.cc:121: };
On 2014/12/11 15:59:54, sky wrote:
> DISALLOW_...

Done.

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_quick_provider_unittest.cc:141:
scoped_refptr<content::MessageLoopRunner> runner =
On 2014/12/11 17:02:04, sky wrote:
> On 2014/12/11 16:04:08, sdefresne wrote:
> > On 2014/12/11 15:59:54, sky wrote:
> > > Why not use a RunLoop on the stack here? Seems simpler.
> > 
> > content::MessageLoopRunner is a base::RefCounted and has a private
destructor.
> > The only way to get it destroyed is to go through a Release call that does
end
> > up calling "delete this".
> 
> I'm suggesting you change this line to:
> 
> base::RunLoop runner;

Done.

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/autocomp...
File chrome/browser/autocomplete/shortcuts_backend.cc (right):

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/autocomp...
chrome/browser/autocomplete/shortcuts_backend.cc:185: DCHECK_EQ(type,
extensions::NOTIFICATION_EXTENSION_UNLOADED_DEPRECATED);
On 2014/12/11 19:59:34, Peter Kasting wrote:
> Nit: (expected, actual)

Done.

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/autocomp...
chrome/browser/autocomplete/shortcuts_backend.cc:193: false);
On 2014/12/11 19:59:35, Peter Kasting wrote:
> Nit: I think both these are legal, but somehow this looks less weird to me:
> 
>   DeleteShortcutsWithURL(
>       content::Details<extensions::UnloadedExtensionInfo>(
>           details)->extension->url(),
>       false);

Done.

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/autocomp...
chrome/browser/autocomplete/shortcuts_backend.cc:328: if (all_history) {
On 2014/12/11 19:59:34, Peter Kasting wrote:
> Nit: I'd probably put a blank line above this

Done.

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/autocomp...
chrome/browser/autocomplete/shortcuts_backend.cc:339: deleted_rows.end()) {
On 2014/12/11 19:59:34, Peter Kasting wrote:
> Nit: {} unnecessary

True. But since the condition is complex, added the braces for better
readability.
Please let me know if you think its not needed.

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/autocomp...
File chrome/browser/autocomplete/shortcuts_backend.h (right):

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/autocomp...
chrome/browser/autocomplete/shortcuts_backend.h:134: //
history::HistoryServiceObserver:
On 2014/12/11 19:59:35, Peter Kasting wrote:
> Nit: Please move this to just below the private NotificationObserver override
> (also in the .cc file).

Done.

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/autocomp...
chrome/browser/autocomplete/shortcuts_backend.h:161:
ScopedObserver<HistoryService, HistoryServiceObserver>
On 2014/12/11 19:59:35, Peter Kasting wrote:
> Nit: I would probably move this above the |no_db_access_| variable so that it
> sits right by the similarly-purposed notification registrar.

Done.

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/sync/glu...
File chrome/browser/sync/glue/favicon_cache_unittest.cc (right):

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/sync/glu...
chrome/browser/sync/glue/favicon_cache_unittest.cc:1497: std::set<GURL>
favicon_urls;
On 2014/12/11 19:45:36, pavely wrote:
> Could you rename this variable to favicon_urls_to_delete.

Done.

[nshaik, 2014-12-12 05:50:44]

https://codereview.chromium.org/773103004/diff/200001/chrome/browser/autocomp...
File chrome/browser/autocomplete/shortcuts_backend.cc (right):

https://codereview.chromium.org/773103004/diff/200001/chrome/browser/autocomp...
chrome/browser/autocomplete/shortcuts_backend.cc:197: DCHECK(0);
@pkasting, please see if this CHECK is necessary.

[jeremy, 2014-12-12 07:57:47]

c/b/ui/cocoa LGTM

[Peter Kasting, 2014-12-12 19:06:52]

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/autocomp...
File chrome/browser/autocomplete/shortcuts_backend.cc (right):

https://codereview.chromium.org/773103004/diff/180001/chrome/browser/autocomp...
chrome/browser/autocomplete/shortcuts_backend.cc:339: deleted_rows.end()) {
On 2014/12/12 05:40:56, nshaik wrote:
> On 2014/12/11 19:59:34, Peter Kasting wrote:
> > Nit: {} unnecessary
> 
> True. But since the condition is complex, added the braces for better
> readability.
> Please let me know if you think its not needed.

I don't think it's necessary, but either way is allowed, so it's up to you.

https://codereview.chromium.org/773103004/diff/200001/chrome/browser/autocomp...
File chrome/browser/autocomplete/shortcuts_backend.cc (right):

https://codereview.chromium.org/773103004/diff/200001/chrome/browser/autocomp...
chrome/browser/autocomplete/shortcuts_backend.cc:197: DCHECK(0);
On 2014/12/12 05:50:44, nshaik wrote:
> @pkasting, please see if this CHECK is necessary.

Don't do this.  Either just omit it, or else make the entire Observe() override
(and the notification registrar member, and the NotificationObserver base class)
all #ifdefed under ENABLE_EXTENSIONS.

[nshaik, 2014-12-12 21:20:13]

Thanks for your comments, removed the DCHECK for now.
Adding ifdef every where was making the code murkier for not much value addition

[nshaik, 2014-12-12 21:21:35]

@haaawk1, @Miguel, @Ian,

can you PTAL.

[Ian Wen, 2014-12-12 21:26:15]

lgtm. A CL on downstream
https://chrome-internal-review.googlesource.com/#/c/188097/ is ready. Once this
CL is submitted, I will land the corresponding one on downstream.

[nshaik, 2014-12-12 22:51:28]

The CQ bit was checked by naiem.shaik@gmail.com

[commit-bot: I haz the power, 2014-12-12 22:52:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/773103004/220001

[nshaik, 2014-12-12 23:50:27]

The CQ bit was unchecked by naiem.shaik@gmail.com

[nshaik, 2014-12-12 23:59:54]

PTAL.
Need your approval for

@sky:
chrome/browser/
chrome/browser/predictors/
chrome/browser/ssl/

@haaawk1:
chrome/browser/android/

[sky, 2014-12-13 00:29:19]

https://codereview.chromium.org/773103004/diff/240001/chrome/browser/predicto...
File chrome/browser/predictors/autocomplete_action_predictor.cc (right):

https://codereview.chromium.org/773103004/diff/240001/chrome/browser/predicto...
chrome/browser/predictors/autocomplete_action_predictor.cc:591:
DCHECK(initialized_);
It looks as though the observer is added in the constructor. How do you know
that initialized_ is a true when you get here?

https://codereview.chromium.org/773103004/diff/240001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

https://codereview.chromium.org/773103004/diff/240001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:1306:
DCHECK_EQ(INITIALIZED, initialization_state_);
Similar comment here.

[nshaik, 2014-12-13 01:49:21]

@sky,
Thanks for your comments.

PTAL if the changes are fine.

https://codereview.chromium.org/773103004/diff/240001/chrome/browser/predicto...
File chrome/browser/predictors/autocomplete_action_predictor.cc (right):

https://codereview.chromium.org/773103004/diff/240001/chrome/browser/predicto...
chrome/browser/predictors/autocomplete_action_predictor.cc:591:
DCHECK(initialized_);
On 2014/12/13 00:29:18, sky wrote:
> It looks as though the observer is added in the constructor. How do you know
> that initialized_ is a true when you get here?

I wanted to replicate the exact behavior, but I agree in this case the
notification might come before the AutocompleteActionProvider is initialized.
I will return if the state is not intialized instead of DCHECKing.

https://codereview.chromium.org/773103004/diff/240001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

https://codereview.chromium.org/773103004/diff/240001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:1306:
DCHECK_EQ(INITIALIZED, initialization_state_);
On 2014/12/13 00:29:18, sky wrote:
> Similar comment here.

True, I can modify the code to check for initialization_state_ and return if it
is not.

[sky, 2014-12-15 15:55:35]

Seems less error prone if you set up the listeners after initialized_
is set. Is there a reason not to do that?

  -Scott

On Fri, Dec 12, 2014 at 5:49 PM,  <naiem.shaik@gmail.com> wrote:
> @sky,
> Thanks for your comments.
>
> PTAL if the changes are fine.
>
>
>
https://codereview.chromium.org/773103004/diff/240001/chrome/browser/predicto...
> File chrome/browser/predictors/autocomplete_action_predictor.cc (right):
>
>
https://codereview.chromium.org/773103004/diff/240001/chrome/browser/predicto...
> chrome/browser/predictors/autocomplete_action_predictor.cc:591:
> DCHECK(initialized_);
> On 2014/12/13 00:29:18, sky wrote:
>>
>> It looks as though the observer is added in the constructor. How do
>
> you know
>>
>> that initialized_ is a true when you get here?
>
>
> I wanted to replicate the exact behavior, but I agree in this case the
> notification might come before the AutocompleteActionProvider is
> initialized.
> I will return if the state is not intialized instead of DCHECKing.
>
>
https://codereview.chromium.org/773103004/diff/240001/chrome/browser/predicto...
> File chrome/browser/predictors/resource_prefetch_predictor.cc (right):
>
>
https://codereview.chromium.org/773103004/diff/240001/chrome/browser/predicto...
> chrome/browser/predictors/resource_prefetch_predictor.cc:1306:
> DCHECK_EQ(INITIALIZED, initialization_state_);
> On 2014/12/13 00:29:18, sky wrote:
>>
>> Similar comment here.
>
>
> True, I can modify the code to check for initialization_state_ and
> return if it is not.
>
> https://codereview.chromium.org/773103004/

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

[nshaik, 2014-12-15 22:03:24]

@Scott,
I need to listen for the HistoryServiceLoaded notification which happens as part
of initialization.
So I would not be able to add to listeners after initialization.
Do you see any problem with the latest patchset.
I was thinking, it is a slight modification in logic but should be equivalent as
the earlier code.

Thanks.
On 2014/12/15 15:55:35, sky wrote:
> Seems less error prone if you set up the listeners after initialized_
> is set. Is there a reason not to do that?
> 
>   -Scott
>

[sky, 2014-12-15 23:05:43]

https://codereview.chromium.org/773103004/diff/240001/chrome/browser/predicto...
File chrome/browser/predictors/autocomplete_action_predictor.cc (right):

https://codereview.chromium.org/773103004/diff/240001/chrome/browser/predicto...
chrome/browser/predictors/autocomplete_action_predictor.cc:591:
DCHECK(initialized_);
On 2014/12/13 01:49:21, nshaik wrote:
> On 2014/12/13 00:29:18, sky wrote:
> > It looks as though the observer is added in the constructor. How do you know
> > that initialized_ is a true when you get here?
> 
> I wanted to replicate the exact behavior, but I agree in this case the
> notification might come before the AutocompleteActionProvider is initialized.
> I will return if the state is not intialized instead of DCHECKing.

I'm not entirely sure that's true. Can you get a more local owner to review
these files to make sure initialization order is correct.

[nshaik, 2014-12-16 00:21:06]

naiem.shaik@gmail.com changed reviewers:
+ shishir@chromium.org
- arv@chromium.org, haaawk@chromium.org

[nshaik, 2014-12-16 00:21:07]

@pkasting, for chrome/browser/autocomplete

@thakis, for chrome/browser/predictors, please point me to the correct person if
you are not the owner for this.

Can you please check the initialization order to see if
there is any issue in initialization with regards to Scott's comments.

Thanks.

[nshaik, 2014-12-16 00:21:37]

naiem.shaik@gmail.com changed reviewers:
+ thakis@chromium.org
- shishir@chromium.org

[sdefresne, 2014-12-18 10:27:19]

sdefresne@chromium.org changed reviewers:
+ tburkard@chromium.org, thestig@chromium.org

[sdefresne, 2014-12-18 10:28:51]

tburkard, thestig: you recently reviewed changes to //chrome/browser/predictors,
so can you review those paths in this CL (there is no OWNERS for the directory)?
If not, could you please say so quickly and (if possible) point to a better
OWNERS for this directory.

[Lei Zhang, 2014-12-18 10:53:58]

On 2014/12/18 10:28:51, sdefresne wrote:
> tburkard, thestig: you recently reviewed changes to
//chrome/browser/predictors,
> so can you review those paths in this CL (there is no OWNERS for the
directory)?
> If not, could you please say so quickly and (if possible) point to a better
> OWNERS for this directory.

lgtm

[Peter Kasting, 2014-12-18 18:45:53]

On 2014/12/16 00:21:07, nshaik wrote:
> @pkasting, for chrome/browser/autocomplete

I already LGTMed that directory.  Are you asking for a re-review?  Has something
important changed since the last time I reviewed?

[nshaik, 2014-12-18 18:59:42]

On 2014/12/18 18:45:53, Peter Kasting wrote:
> On 2014/12/16 00:21:07, nshaik wrote:
> > @pkasting, for chrome/browser/autocomplete
> 
> I already LGTMed that directory.  Are you asking for a re-review?  Has
something
> important changed since the last time I reviewed?

Scott found an invalid DCHECK that I have fixed in the latest patchset.
He wanted to make sure if the initialization order is correct.

[Peter Kasting, 2014-12-18 19:07:12]

On 2014/12/18 18:59:42, nshaik wrote:
> On 2014/12/18 18:45:53, Peter Kasting wrote:
> > On 2014/12/16 00:21:07, nshaik wrote:
> > > @pkasting, for chrome/browser/autocomplete
> > 
> > I already LGTMed that directory.  Are you asking for a re-review?  Has
> something
> > important changed since the last time I reviewed?
> 
> Scott found an invalid DCHECK that I have fixed in the latest patchset.
> He wanted to make sure if the initialization order is correct.

OK, but that change doesn't seem to be in the c/b/autocomplete directory. 
AFAICT nothing meaningful has changed there related to this issue.  (I
re-reviewed things to check, and they look fine.)

[sky, 2014-12-18 20:54:31]

On 2014/12/18 19:07:12, Peter Kasting wrote:
> On 2014/12/18 18:59:42, nshaik wrote:
> > On 2014/12/18 18:45:53, Peter Kasting wrote:
> > > On 2014/12/16 00:21:07, nshaik wrote:
> > > > @pkasting, for chrome/browser/autocomplete
> > > 
> > > I already LGTMed that directory.  Are you asking for a re-review?  Has
> > something
> > > important changed since the last time I reviewed?
> > 
> > Scott found an invalid DCHECK that I have fixed in the latest patchset.
> > He wanted to make sure if the initialization order is correct.
> 
> OK, but that change doesn't seem to be in the c/b/autocomplete directory. 
> AFAICT nothing meaningful has changed there related to this issue.  (I
> re-reviewed things to check, and they look fine.)

Peter, see my comments at 38. The ordering is slightly different now and I
wanted to make sure that isn't going to cause problems.

[Peter Kasting, 2014-12-18 21:40:26]

On 2014/12/18 20:54:31, sky wrote:
> On 2014/12/18 19:07:12, Peter Kasting wrote:
> > On 2014/12/18 18:59:42, nshaik wrote:
> > > On 2014/12/18 18:45:53, Peter Kasting wrote:
> > > > On 2014/12/16 00:21:07, nshaik wrote:
> > > > > @pkasting, for chrome/browser/autocomplete
> > > > 
> > > > I already LGTMed that directory.  Are you asking for a re-review?  Has
> > > something
> > > > important changed since the last time I reviewed?
> > > 
> > > Scott found an invalid DCHECK that I have fixed in the latest patchset.
> > > He wanted to make sure if the initialization order is correct.
> > 
> > OK, but that change doesn't seem to be in the c/b/autocomplete directory. 
> > AFAICT nothing meaningful has changed there related to this issue.  (I
> > re-reviewed things to check, and they look fine.)
> 
> Peter, see my comments at 38. The ordering is slightly different now and I
> wanted to make sure that isn't going to cause problems.

Figured out offline that this is all in reference to c/b/predictors that thakis
was asked to review.  I don't think offhand he's the owner of this code, but I'm
not really either.

[Nico, 2014-12-18 22:53:26]

On 2014/12/18 21:40:26, Peter Kasting wrote:
> On 2014/12/18 20:54:31, sky wrote:
> > On 2014/12/18 19:07:12, Peter Kasting wrote:
> > > On 2014/12/18 18:59:42, nshaik wrote:
> > > > On 2014/12/18 18:45:53, Peter Kasting wrote:
> > > > > On 2014/12/16 00:21:07, nshaik wrote:
> > > > > > @pkasting, for chrome/browser/autocomplete
> > > > > 
> > > > > I already LGTMed that directory.  Are you asking for a re-review?  Has
> > > > something
> > > > > important changed since the last time I reviewed?
> > > > 
> > > > Scott found an invalid DCHECK that I have fixed in the latest patchset.
> > > > He wanted to make sure if the initialization order is correct.
> > > 
> > > OK, but that change doesn't seem to be in the c/b/autocomplete directory. 
> > > AFAICT nothing meaningful has changed there related to this issue.  (I
> > > re-reviewed things to check, and they look fine.)
> > 
> > Peter, see my comments at 38. The ordering is slightly different now and I
> > wanted to make sure that isn't going to cause problems.
> 
> Figured out offline that this is all in reference to c/b/predictors that
thakis
> was asked to review.

(s/thakis/thestig/ I think? From looking at #47, #48.)

[nshaik, 2014-12-18 23:47:36]

On 2014/12/18 22:53:26, Nico wrote:
> On 2014/12/18 21:40:26, Peter Kasting wrote:
> > On 2014/12/18 20:54:31, sky wrote:
> > > On 2014/12/18 19:07:12, Peter Kasting wrote:
> > > > On 2014/12/18 18:59:42, nshaik wrote:
> > > > > On 2014/12/18 18:45:53, Peter Kasting wrote:
> > > > > > On 2014/12/16 00:21:07, nshaik wrote:
> > > > > > > @pkasting, for chrome/browser/autocomplete
> > > > > > 
> > > > > > I already LGTMed that directory.  Are you asking for a re-review? 
Has
> > > > > something
> > > > > > important changed since the last time I reviewed?
> > > > > 
> > > > > Scott found an invalid DCHECK that I have fixed in the latest
patchset.
> > > > > He wanted to make sure if the initialization order is correct.
> > > > 
> > > > OK, but that change doesn't seem to be in the c/b/autocomplete
directory. 
> > > > AFAICT nothing meaningful has changed there related to this issue.  (I
> > > > re-reviewed things to check, and they look fine.)
> > > 
> > > Peter, see my comments at 38. The ordering is slightly different now and I
> > > wanted to make sure that isn't going to cause problems.
> > 
> > Figured out offline that this is all in reference to c/b/predictors that
> thakis
> > was asked to review.
> 
> (s/thakis/thestig/ I think? From looking at #47, #48.)


 @pkasting am really sorry for the confusion, 
I wanted to get the owners for chrome/browser/predictors/autocomplete*

@sky, there are no OWNERs for chrome/browser/predictors.
Lei Zhang took a look at it and LGTM'ed it.
Please let me know if any one else needs to review it.

[nshaik, 2014-12-19 22:51:31]

Scott, can you PTAL.

[nshaik, 2014-12-29 14:34:03]

On 2014/12/19 22:51:31, nshaik wrote:
> Scott, can you PTAL.

Scott, please let me know if I should add
anyone else to the review.

[sky, 2015-01-05 17:16:12]

On 2014/12/29 14:34:03, nshaik wrote:
> On 2014/12/19 22:51:31, nshaik wrote:
> > Scott, can you PTAL.
> 
> Scott, please let me know if I should add
> anyone else to the review.

Try looking at the history of files in that directory to see who has changed it
the most.

[nshaik, 2015-01-05 18:50:47]

naiem.shaik@gmail.com changed reviewers:
+ hashimoto@chromium.org, zhenw@chromium.org

[nshaik, 2015-01-05 18:53:13]

On 2015/01/05 17:16:12, sky wrote:
> On 2014/12/29 14:34:03, nshaik wrote:
> > On 2014/12/19 22:51:31, nshaik wrote:
> > > Scott, can you PTAL.
> > 
> > Scott, please let me know if I should add
> > anyone else to the review.
> 
> Try looking at the history of files in that directory to see who has changed
it
> the most.

 PTAL.

@hashimoto
chrome/browser/predictors/autocomplete_action_predictor.cc 

@zhenw
chrome /browser/predictors/resource_prefetch_predictor.cc

[hashimoto, 2015-01-06 04:30:55]

On 2015/01/05 18:53:13, nshaik wrote:
> On 2015/01/05 17:16:12, sky wrote:
> > On 2014/12/29 14:34:03, nshaik wrote:
> > > On 2014/12/19 22:51:31, nshaik wrote:
> > > > Scott, can you PTAL.
> > > 
> > > Scott, please let me know if I should add
> > > anyone else to the review.
> > 
> > Try looking at the history of files in that directory to see who has changed
> it
> > the most.
> 
>  PTAL.
> 
> @hashimoto
> chrome/browser/predictors/autocomplete_action_predictor.cc 
Sorry, all of my changes were trivial refactoring and I know nothing about this
file.
(so sad chrome/browser/predictors doesn't have OWNERS explicitly specified...)

According to git blame, it seems dominich@ is responsible to most parts of this
file.
(https://chromium.googlesource.com/chromium/src/+blame/master/chrome/browser/p...)

Also, anyone from chrome/browser/autocomplete might be an appropriate reviewer
as this file was originally placed under chrome/browser/autocomplete.
It was moved to chrome/browser/predictor by http://crrev.com/134865.
> 
> @zhenw
> chrome /browser/predictors/resource_prefetch_predictor.cc

[sdefresne, 2015-01-06 08:52:58]

sdefresne@chromium.org changed reviewers:
+ dominich@chromium.org

[sdefresne, 2015-01-06 08:53:00]

+ dominich: potential OWNERS of //chrome/browser/predictors following hashimoto@
suggestion

[sdefresne, 2015-01-06 08:53:18]

sdefresne@chromium.org changed reviewers:
- hashimoto@chromium.org

[nshaik, 2015-01-20 02:38:35]

On 2015/01/06 08:53:00, sdefresne wrote:
> + dominich: potential OWNERS of //chrome/browser/predictors following
hashimoto@
> suggestion

@dominich
PTAL.

[sdefresne, 2015-01-23 21:27:03]

thestig:

I see that you've already given l-g-t-m on this review, but sky raised some
concern on comment #38 after your l-g-t-m. We've seen been looking for an
OWNERS of //chrome/browser/predictors but were unable to find anyone feeling
responsible for that piece of code. It looks like you've been reviewing some
review of that code. Could you check whether the issue raised by sky may or
not be problematic?

For information, here is the discussion between sky and naiem.shaik:

> > > It looks as though the observer is added in the constructor. How do you
> > > know that initialized_ is a true when you get here?
> > 
> > I wanted to replicate the exact behavior, but I agree in this case the
> > notification might come before the AutocompleteActionProvider is
> > initialized. I will return if the state is not intialized instead of
> > DCHECKing.
> 
> I'm not entirely sure that's true. Can you get a more local owner to review
> these files to make sure initialization order is correct.

Sorry for the bother, but I'd be really glad if we could solve this question
as this CL is blocking the componentization of history and thus the effort of
upstreaming of the iOS code.

[Lei Zhang, 2015-01-23 21:43:01]

On 2015/01/23 21:27:03, sdefresne wrote:
> thestig:
> 
> I see that you've already given l-g-t-m on this review, but sky raised some
> concern on comment #38 after your l-g-t-m. We've seen been looking for an
> OWNERS of //chrome/browser/predictors but were unable to find anyone feeling
> responsible for that piece of code. It looks like you've been reviewing some
> review of that code. Could you check whether the issue raised by sky may or
> not be problematic?
> 
> For information, here is the discussion between sky and naiem.shaik:
> 
> > > > It looks as though the observer is added in the constructor. How do you
> > > > know that initialized_ is a true when you get here?
> > > 
> > > I wanted to replicate the exact behavior, but I agree in this case the
> > > notification might come before the AutocompleteActionProvider is
> > > initialized. I will return if the state is not intialized instead of
> > > DCHECKing.
> > 
> > I'm not entirely sure that's true. Can you get a more local owner to review
> > these files to make sure initialization order is correct.
> 
> Sorry for the bother, but I'd be really glad if we could solve this question
> as this CL is blocking the componentization of history and thus the effort of
> upstreaming of the iOS code.

I believe I reviewed patch set 14, which is still the latest patch set. I looked
at it again and c/b/predictor still looks ok to me.

Taking autocomplete_action_predictor.cc as an example. With
NOTIFICATION_HISTORY_URLS_DELETED, it only start listening for the notification
when |initialized_| gets set to true. With the new observer, it start observing
in the ctor. Thus there's a period of time between then and when |initialized_|
gets set to true, where AutocompleteActionPredictor::OnURLsDeleted() can get
called. AutocompleteActionPredictor::OnURLsDeleted() checks |initialized_| and
ignores the call when uninitialized. That looks liek effectively the same
behavior to me unless I'm missing something.

[sdefresne, 2015-01-23 22:51:00]

On 2015/01/23 at 21:43:01, thestig wrote:
> On 2015/01/23 21:27:03, sdefresne wrote:
> > thestig:
> > 
> > I see that you've already given l-g-t-m on this review, but sky raised some
> > concern on comment #38 after your l-g-t-m. We've seen been looking for an
> > OWNERS of //chrome/browser/predictors but were unable to find anyone feeling
> > responsible for that piece of code. It looks like you've been reviewing some
> > review of that code. Could you check whether the issue raised by sky may or
> > not be problematic?
> > 
> > For information, here is the discussion between sky and naiem.shaik:
> > 
> > > > > It looks as though the observer is added in the constructor. How do
you
> > > > > know that initialized_ is a true when you get here?
> > > > 
> > > > I wanted to replicate the exact behavior, but I agree in this case the
> > > > notification might come before the AutocompleteActionProvider is
> > > > initialized. I will return if the state is not intialized instead of
> > > > DCHECKing.
> > > 
> > > I'm not entirely sure that's true. Can you get a more local owner to
review
> > > these files to make sure initialization order is correct.
> > 
> > Sorry for the bother, but I'd be really glad if we could solve this question
> > as this CL is blocking the componentization of history and thus the effort
of
> > upstreaming of the iOS code.
> 
> I believe I reviewed patch set 14, which is still the latest patch set. I
looked at it again and c/b/predictor still looks ok to me.
> 
> Taking autocomplete_action_predictor.cc as an example. With
NOTIFICATION_HISTORY_URLS_DELETED, it only start listening for the notification
when |initialized_| gets set to true. With the new observer, it start observing
in the ctor. Thus there's a period of time between then and when |initialized_|
gets set to true, where AutocompleteActionPredictor::OnURLsDeleted() can get
called. AutocompleteActionPredictor::OnURLsDeleted() checks |initialized_| and
ignores the call when uninitialized. That looks liek effectively the same
behavior to me unless I'm missing something.

Thank you for having another look. It also looked the same to me, but after sky
comments, we wanted to have some OWNERS (which proved difficult) validate
naiem.shaik code. If I understand correctly, this CL has no received the missing
l-g-t-m for //chrome/browser/predictors.

naiem.shaik: I think the CL is now ready for landing. Can you please rebase it
at your convenience and send it to the try bots to check that all the tests are
still passing and there are no conflicts due to the other changes introducted to
//chrome/browser/history?

[Lei Zhang, 2015-01-23 22:58:49]

On 2015/01/23 22:51:00, sdefresne wrote:
> Thank you for having another look. It also looked the same to me, but after
sky
> comments, we wanted to have some OWNERS (which proved difficult) validate
> naiem.shaik code. If I understand correctly, this CL has no received the
missing
> l-g-t-m for //chrome/browser/predictors.

Umm, there's no chrome/browser/predictors/OWNERS, which is why I ended up
reviewing the code. Believe me, I've tried to get people to own this code,
without success. :\

[nshaik, 2015-01-27 23:45:32]

The CQ bit was checked by naiem.shaik@gmail.com

[commit-bot: I haz the power, 2015-01-27 23:46:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/773103004/280001

[commit-bot: I haz the power, 2015-01-27 23:52:49]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2015-01-27 23:53:43]

Patchset 15 (id:??) landed as
https://crrev.com/5017fc699bb6c353538103770944216450eaa43c
Cr-Commit-Position: refs/heads/master@{#313406}