Have URL Modifications sent Regardless of Typed.

Have URL Modifications sent Regardless of Typed.

URL visit adds and modifications are now noticed regardless of typed count. Filtering is no longer done before making notifications. Clients of the notifications filter on typed count if desired. Also, if a change notification indicates that a page title is now blank, ignore that change in the InMemoryURLIndex.

Renamed NOTIFICATION_HISTORY_TYPED_URLS_MODIFIED to NOTIFICATION_HISTORY_URLS_MODIFIED to better reflect its new purpose.

Reviewers:
  pkasting: Overall general changes.
  sky: Owner review for chrome/browser/history and minor changes in chrome/browser/ui/omnibox.
  akalin: TBR: Owner review for minor changes in chrome/browser/sync.

BUG=102957, 87803
TEST=All unit tests pass.
TBR=akalin@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=131865

[mrossetti, 2012-04-09 23:53:30]

Ready for review.

[Peter Kasting, 2012-04-10 02:18:56]

LGTM

http://codereview.chromium.org/9852002/diff/8001/chrome/browser/history/histo...
File chrome/browser/history/history_backend.cc (right):

http://codereview.chromium.org/9852002/diff/8001/chrome/browser/history/histo...
chrome/browser/history/history_backend.cc:882: URLRows changed_urls;
Nit: You can save copying the whole URLRows object with something like:

  scoped_ptr<URLsModifiedDetails> details(...);
  for (...) {
    ...;
    details->changed_urls.push_back(row);
  }

  if (!details->changed_urls.empty())
    BroadcastNotifications(..., details.release());

http://codereview.chromium.org/9852002/diff/8001/chrome/browser/history/url_i...
File chrome/browser/history/url_index_private_data.cc (right):

http://codereview.chromium.org/9852002/diff/8001/chrome/browser/history/url_i...
chrome/browser/history/url_index_private_data.cc:359: // Ignore title changes
where new title is empty. (See bug 102957.)
Nit: Instead of referring to a bug, can you put a full explanation here?  (Among
other reasons, it's not instantly clear to me from glancing at the bug what
causes us to get empty new titles here and why we'd actually want to ignore
those.)

http://codereview.chromium.org/9852002/diff/8001/chrome/browser/sync/glue/typ...
File chrome/browser/sync/glue/typed_url_change_processor.cc (right):

http://codereview.chromium.org/9852002/diff/8001/chrome/browser/sync/glue/typ...
chrome/browser/sync/glue/typed_url_change_processor.cc:71:
DCHECK(chrome::NOTIFICATION_HISTORY_URLS_MODIFIED == type ||
Nit: Slightly simpler: Remove this DCHECK, change final conditional arm below to
"} else {" and then add DCHECK_EQ(...) inside it

http://codereview.chromium.org/9852002/diff/8001/chrome/browser/sync/glue/typ...
chrome/browser/sync/glue/typed_url_change_processor.cc:92: // Only consider
typed URLs.
Nit: This comment doesn't really add anything

http://codereview.chromium.org/9852002/diff/8001/chrome/common/chrome_notific...
File chrome/common/chrome_notification_types.h (right):

http://codereview.chromium.org/9852002/diff/8001/chrome/common/chrome_notific...
chrome/common/chrome_notification_types.h:210: // URL database and the
InMemoruURLIndex (both used by autocomplete) to track
Nit: typo

[sky, 2012-04-10 16:29:01]

http://codereview.chromium.org/9852002/diff/8001/chrome/browser/history/histo...
File chrome/browser/history/history_backend.cc (right):

http://codereview.chromium.org/9852002/diff/8001/chrome/browser/history/histo...
chrome/browser/history/history_backend.cc:889: row.set_id(row_id);
Doesn't GetRowForURL set the id?

http://codereview.chromium.org/9852002/diff/8001/chrome/browser/history/in_me...
File chrome/browser/history/in_memory_history_backend.cc (right):

http://codereview.chromium.org/9852002/diff/8001/chrome/browser/history/in_me...
chrome/browser/history/in_memory_history_backend.cc:115: i !=
details.changed_urls.end(); i++) {
++i

[mrossetti, 2012-04-11 19:38:55]

Issues addressed.

http://codereview.chromium.org/9852002/diff/8001/chrome/browser/history/histo...
File chrome/browser/history/history_backend.cc (right):

http://codereview.chromium.org/9852002/diff/8001/chrome/browser/history/histo...
chrome/browser/history/history_backend.cc:882: URLRows changed_urls;
Sweet!

On 2012/04/10 02:18:57, Peter Kasting wrote:
> Nit: You can save copying the whole URLRows object with something like:
> 
>   scoped_ptr<URLsModifiedDetails> details(...);
>   for (...) {
>     ...;
>     details->changed_urls.push_back(row);
>   }
> 
>   if (!details->changed_urls.empty())
>     BroadcastNotifications(..., details.release());

http://codereview.chromium.org/9852002/diff/8001/chrome/browser/history/histo...
chrome/browser/history/history_backend.cc:889: row.set_id(row_id);
Yes. Removed.

On 2012/04/10 16:29:01, sky wrote:
> Doesn't GetRowForURL set the id?

http://codereview.chromium.org/9852002/diff/8001/chrome/browser/history/in_me...
File chrome/browser/history/in_memory_history_backend.cc (right):

http://codereview.chromium.org/9852002/diff/8001/chrome/browser/history/in_me...
chrome/browser/history/in_memory_history_backend.cc:115: i !=
details.changed_urls.end(); i++) {
On 2012/04/10 16:29:01, sky wrote:
> ++i

Done.

http://codereview.chromium.org/9852002/diff/8001/chrome/browser/history/url_i...
File chrome/browser/history/url_index_private_data.cc (right):

http://codereview.chromium.org/9852002/diff/8001/chrome/browser/history/url_i...
chrome/browser/history/url_index_private_data.cc:359: // Ignore title changes
where new title is empty. (See bug 102957.)
Removed the test for empty(). I cannot now recreate the condition where I saw an
update go by where the title was empty subsequent to an earlier update with a
non-empty title.

On 2012/04/10 02:18:57, Peter Kasting wrote:
> Nit: Instead of referring to a bug, can you put a full explanation here? 
(Among
> other reasons, it's not instantly clear to me from glancing at the bug what
> causes us to get empty new titles here and why we'd actually want to ignore
> those.)

http://codereview.chromium.org/9852002/diff/8001/chrome/browser/sync/glue/typ...
File chrome/browser/sync/glue/typed_url_change_processor.cc (right):

http://codereview.chromium.org/9852002/diff/8001/chrome/browser/sync/glue/typ...
chrome/browser/sync/glue/typed_url_change_processor.cc:71:
DCHECK(chrome::NOTIFICATION_HISTORY_URLS_MODIFIED == type ||
On 2012/04/10 02:18:57, Peter Kasting wrote:
> Nit: Slightly simpler: Remove this DCHECK, change final conditional arm below
to
> "} else {" and then add DCHECK_EQ(...) inside it

Done.

http://codereview.chromium.org/9852002/diff/8001/chrome/browser/sync/glue/typ...
chrome/browser/sync/glue/typed_url_change_processor.cc:92: // Only consider
typed URLs.
On 2012/04/10 02:18:57, Peter Kasting wrote:
> Nit: This comment doesn't really add anything

Done.

http://codereview.chromium.org/9852002/diff/8001/chrome/common/chrome_notific...
File chrome/common/chrome_notification_types.h (right):

http://codereview.chromium.org/9852002/diff/8001/chrome/common/chrome_notific...
chrome/common/chrome_notification_types.h:210: // URL database and the
InMemoruURLIndex (both used by autocomplete) to track
On 2012/04/10 02:18:57, Peter Kasting wrote:
> Nit: typo

Done.

[sky, 2012-04-11 20:58:42]

LGTM