Avoid sending notifications when the bitmap data in history has not changed

Avoid sending notifications when the bitmap data in the history backend is replaced with bitmap data which is byte for byte equal.

Bug=168223
Test=HistoryBackendTest.*
     Manual: see comment #7 in bug

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=176691

[pkotwicz, 2013-01-09 17:03:15]

Scott, can you please take a look?

This fixes the performance problems with duplicate bookmarks.
If there are n duplicate bookmarks,
HistoryBackend::MergeFavicon() will get called n times,
Send n notifications. As a result of each notification,
BookmarkChangeProcessor::BookmarkNodeFaviconChanged() is called n times.
This CL changes MergeFavicon() to only send notifications when the bitmap data
actually changed.

We do an additional query to do the byte comparison but it is faster than
decoding the bitmap data which we would almost certainly do as a result of
sending a NOTIFICATION_FAVICON_CHANGED when it was unnecessary.

This CL has the nice side effect of not calling BookmarkModel::LoadFavicon() as
a result of HistoryBackend::MergeFavicon() when the bitmap data is equal. As a
result, in the common case, we do not load the favicons for all of the bookmarks
at startup.

I unfortunately did not merge this CL to
https://codereview.chromium.org/11746010/ as I hope to backport this CL to M25
at least for iOS.

[pkotwicz, 2013-01-09 17:03:37]

Scott, can you please take a look?

[sky, 2013-01-09 17:55:25]

https://codereview.chromium.org/11830007/diff/6002/chrome/browser/history/his...
File chrome/browser/history/history_backend.cc (right):

https://codereview.chromium.org/11830007/diff/6002/chrome/browser/history/his...
chrome/browser/history/history_backend.cc:2377: return
original_bitmap_data.get() &&
Should this be on RefCountedMemory? Specfically and Equals method?

[pkotwicz, 2013-01-09 19:00:46]

Scott, can you please take another look?
Added base::RefCountedMemory::Equals() as requested.

[sky, 2013-01-09 20:38:24]

LGTM for non-base changes. You'll need a base OWNER for the base change.

https://codereview.chromium.org/11830007/diff/8006/base/memory/ref_counted_me...
File base/memory/ref_counted_memory.cc (right):

https://codereview.chromium.org/11830007/diff/8006/base/memory/ref_counted_me...
base/memory/ref_counted_memory.cc:12: return size() == other->size() &&
How about a NULL check on |other|, and if possible const RefCountedMemory*

[pkotwicz, 2013-01-09 21:12:27]

jar@, can you please take a look at the changes in base/

[jar (doing other things), 2013-01-10 07:09:06]

https://codereview.chromium.org/11830007/diff/9010/base/memory/ref_counted_me...
File base/memory/ref_counted_memory.cc (right):

https://codereview.chromium.org/11830007/diff/9010/base/memory/ref_counted_me...
base/memory/ref_counted_memory.cc:11: bool RefCountedMemory::Equals(const
RefCountedMemory* other) const {
Why doesn't this take a const ref as an argument?

[pkotwicz, 2013-01-10 15:50:56]

Changed argument to scoped_refptr const ref.

[jar (doing other things), 2013-01-12 15:53:34]

base in patch set 8 LGTM

[commit-bot: I haz the power, 2013-01-14 15:49:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/11830007/17001

[commit-bot: I haz the power, 2013-01-14 18:59:56]

Change committed as 176691

[sky, 2013-03-19 21:27:08]

LGTM