Update all bookmarks which use an icon URL when a favicon's bitmap is updated

Update all bookmarks which use an icon URL when a favicon's bitmap is updated

This CL causes two different types of notifications to be dispatched when
favicon data is changed in the database:
Type 1: The page URL <-> icon URL mapping has changed.
  e.g. http://www.google.com changed from using
      http://www.google.com/favicon1.ico to using
      http://www.google.com/favicon2.ico
Type 2: The favicon bitmap data for an icon URL has changed.
  e.g. a webmaster replaced http://www.google.com/favicon1.ico with a new
  version

An update to the favicon database can cause either one type of notification
or both types of notifications to be dispatched.

Example Scenario:
1) A Google webmaster has updated http://www.google.com/favicon1.ico
2) A user has http://www.google.com bookmarked
3) A user visits http://www.google.com/searchresults
Both http://www.google.com and http://www.google.com/searchresults use
http://www.google.com/favicon1.ico

Old Behavior:
The favicon update which occurs in step #3 causes a notification to be sent
with an incomplete list of page URLs for which the favicon has changed. A
notification is sent for http://www.google.com/searchresults but not for
http://www.google.com even though both pages use the same icon URL.
As a consequence of no notification being sent for http://www.google.com:
- The favicon for the http://www.google.com bookmark is not updated
- If a user is logged into sync, sync reverts the favicon for
  http://www.google.com/favicon1.ico to the old version when Chrome is restarted

New Behavior:
A "Type 2 notification" is dispatched indicating that the favicon at
http://www.google.com/favicon1.ico has changed. This allows the BookmarkModel
to refetch the favicons for all the bookmarks which use
http://www.google.com/favicon1.ico, namely the http://www.google.com bookmark.
The favicon for http://www.google.com/favicon1.ico is not reverted to an old
version when Chrome is restarted

This bug can be fixed by passing to the BookmarkModel a list of all of the
page URLs which use http://www.google.com/favicon1.ico in step #3. This is
impractical because the subset of pages that the user visited which use a given
icon URL is usually much larger than the subset of bookmarked pages which use a
given icon URL.

BUG=485657
TEST=HistoryBackendTest.*
     TwoClientBookmarksSyncTest.SC_SetFaviconTwoBookmarksSameIconURL
     BookmarkModelTest.*

Committed: https://crrev.com/ca240dde9b8b56120f93050ebfd200bf6714e56b
Cr-Commit-Position: refs/heads/master@{#338054}

[pkotwicz, 2015-05-12 02:41:25]

pkotwicz@chromium.org changed reviewers:
+ rogerm@chromium.org

[pkotwicz, 2015-05-12 02:41:26]

rogerm@ PTAL

[pkotwicz, 2015-05-12 02:43:40]

Patchset #1 (id:1) has been deleted

[pkotwicz, 2015-05-13 15:11:29]

rogerm@ Ping!

[Roger McFarlane (Chromium), 2015-05-13 15:15:44]

sorry! looking...

[Roger McFarlane (Chromium), 2015-05-13 16:07:05]

First pass: so far it looks fine to me. I'll do another quick pass after lunch.

https://codereview.chromium.org/1133463005/diff/20001/chrome/browser/sync/ses...
File chrome/browser/sync/sessions/sessions_sync_manager.cc (right):

https://codereview.chromium.org/1133463005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sessions_sync_manager.cc:400: const
std::vector<GURL>& icon_urls) {
nit: comment out icon_urls, as it's not used in the body.

https://codereview.chromium.org/1133463005/diff/20001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/sessions_helper.cc (right):

https://codereview.chromium.org/1133463005/diff/20001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/sessions_helper.cc:148: const
std::vector<GURL>& icon_urls) override {
nit: comment out the param names, as they're not used in the body.

https://codereview.chromium.org/1133463005/diff/20001/components/bookmarks/br...
File components/bookmarks/browser/bookmark_model.h (right):

https://codereview.chromium.org/1133463005/diff/20001/components/bookmarks/br...
components/bookmarks/browser/bookmark_model.h:307: void OnFaviconChanged(const
std::vector<GURL>& page_urls,
you've used OnFaviconsChanged elsewhere

[pkotwicz, 2015-05-13 17:26:49]

https://codereview.chromium.org/1133463005/diff/20001/chrome/browser/sync/ses...
File chrome/browser/sync/sessions/sessions_sync_manager.cc (right):

https://codereview.chromium.org/1133463005/diff/20001/chrome/browser/sync/ses...
chrome/browser/sync/sessions/sessions_sync_manager.cc:400: const
std::vector<GURL>& icon_urls) {
On 2015/05/13 16:07:05, Roger McFarlane (Chromium) wrote:
> nit: comment out icon_urls, as it's not used in the body.

Done.

https://codereview.chromium.org/1133463005/diff/20001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/sessions_helper.cc (right):

https://codereview.chromium.org/1133463005/diff/20001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/sessions_helper.cc:148: const
std::vector<GURL>& icon_urls) override {
On 2015/05/13 16:07:05, Roger McFarlane (Chromium) wrote:
> nit: comment out the param names, as they're not used in the body.

Done.

https://codereview.chromium.org/1133463005/diff/20001/components/bookmarks/br...
File components/bookmarks/browser/bookmark_model.h (right):

https://codereview.chromium.org/1133463005/diff/20001/components/bookmarks/br...
components/bookmarks/browser/bookmark_model.h:307: void OnFaviconChanged(const
std::vector<GURL>& page_urls,
On 2015/05/13 16:07:05, Roger McFarlane (Chromium) wrote:
> you've used OnFaviconsChanged elsewhere

Done.

[Roger McFarlane (Chromium), 2015-05-13 18:28:13]

LGTM with nits.

https://codereview.chromium.org/1133463005/diff/40001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/two_client_bookmarks_sync_test.cc
(right):

https://codereview.chromium.org/1133463005/diff/40001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_bookmarks_sync_test.cc:233: //
stored in the same row in the favicons database. Test that when the favicon
Assuming my understanding is correct, how about:

  When two bookmarks use the same icon URL, a single shared favicon
  row is stored in the favicons database. Test that...

https://codereview.chromium.org/1133463005/diff/40001/components/history/core...
File components/history/core/browser/expire_history_backend_unittest.cc (right):

https://codereview.chromium.org/1133463005/diff/40001/components/history/core...
components/history/core/browser/expire_history_backend_unittest.cc:165: const
std::vector<GURL>& icon_urls) override{};
nit: missing space between override and empty implementation.

https://codereview.chromium.org/1133463005/diff/40001/components/history/core...
File components/history/core/browser/history_service.h (right):

https://codereview.chromium.org/1133463005/diff/40001/components/history/core...
components/history/core/browser/history_service.h:449: OnFaviconChangedCallback;
uber-nit: OnFaviconsChangedCallback

[pkotwicz, 2015-05-14 14:18:53]

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

[pkotwicz, 2015-05-14 14:18:54]

Scott, can you please take a look?

https://codereview.chromium.org/1133463005/diff/40001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/two_client_bookmarks_sync_test.cc
(right):

https://codereview.chromium.org/1133463005/diff/40001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_bookmarks_sync_test.cc:233: //
stored in the same row in the favicons database. Test that when the favicon
I modified the first sentence to be less wordy

https://codereview.chromium.org/1133463005/diff/40001/components/history/core...
File components/history/core/browser/expire_history_backend_unittest.cc (right):

https://codereview.chromium.org/1133463005/diff/40001/components/history/core...
components/history/core/browser/expire_history_backend_unittest.cc:165: const
std::vector<GURL>& icon_urls) override{};
On 2015/05/13 18:28:12, Roger McFarlane (Chromium) wrote:
> nit: missing space between override and empty implementation.

Done.

https://codereview.chromium.org/1133463005/diff/40001/components/history/core...
File components/history/core/browser/history_service.h (right):

https://codereview.chromium.org/1133463005/diff/40001/components/history/core...
components/history/core/browser/history_service.h:449: OnFaviconChangedCallback;
Thanks for catching this!

[pkotwicz, 2015-05-22 00:49:25]

sky@ Ping!

[sky, 2015-05-22 03:09:06]

https://codereview.chromium.org/1133463005/diff/60001/components/bookmarks/br...
File components/bookmarks/browser/bookmark_model.cc (right):

https://codereview.chromium.org/1133463005/diff/60001/components/bookmarks/br...
components/bookmarks/browser/bookmark_model.cc:520: // TODO: Do something more
efficient if this gets called a lot
Any idea how often this will be called? Might a sync trigger this being called a
lot?

[pkotwicz, 2015-05-22 16:00:39]

https://codereview.chromium.org/1133463005/diff/60001/components/bookmarks/br...
File components/bookmarks/browser/bookmark_model.cc (right):

https://codereview.chromium.org/1133463005/diff/60001/components/bookmarks/br...
components/bookmarks/browser/bookmark_model.cc:520: // TODO: Do something more
efficient if this gets called a lot
Unfortunately, this is called more often than I expected. If a user has two
different devices which sync with each other then this can be called quite a
bit.
The favicon PNG data which is stored in the database is different when it is
encoded on iOS, Mac, Linux and Android (Android picks the largest bitmap of an
.ico and scales it down to 16x16)

GetNodesByIconURL() may be called for several icon URLs each time that a user
switches the device that they use

I am unsure whether this necessitates improving GetNodesByIconURL()

Unfortunately, fixing the Android behavior is hard (I hoped that the work that
rogerm@ was doing would fix it but he has moved on to another project)

[sky, 2015-05-22 17:45:27]

We need another solution then. Iterating over all the bookmarks lots
of times is going to hurt performance.

  -Scott

On Fri, May 22, 2015 at 9:00 AM,  <pkotwicz@chromium.org> wrote:
>
>
https://codereview.chromium.org/1133463005/diff/60001/components/bookmarks/br...
> File components/bookmarks/browser/bookmark_model.cc (right):
>
>
https://codereview.chromium.org/1133463005/diff/60001/components/bookmarks/br...
> components/bookmarks/browser/bookmark_model.cc:520: // TODO: Do
> something more efficient if this gets called a lot
> Unfortunately, this is called more often than I expected. If a user has
> two different devices which sync with each other then this can be called
> quite a bit.
> The favicon PNG data which is stored in the database is different when
> it is encoded on iOS, Mac, Linux and Android (Android picks the largest
> bitmap of an .ico and scales it down to 16x16)
>
> GetNodesByIconURL() may be called for several icon URLs each time that a
> user switches the device that they use
>
> I am unsure whether this necessitates improving GetNodesByIconURL()
>
> Unfortunately, fixing the Android behavior is hard (I hoped that the
> work that rogerm@ was doing would fix it but he has moved on to another
> project)
>
> https://codereview.chromium.org/1133463005/

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

[pkotwicz, 2015-06-07 20:39:51]

Patchset #4 (id:80001) has been deleted

[pkotwicz, 2015-06-07 20:45:12]

sky@ can you please take another look?

Sorry for the delay. I have added a multiset which has the BookmarkNodes ordered
based on the icon URL.
Please let me know what you think.
- Peter

[sky, 2015-06-08 15:01:43]

I'm not particularly keen on increasing the in memory size of the
bookmark model for a case that does not happen particularly often. Do
you have data on how often we get large changes?

  -Scott

On Sun, Jun 7, 2015 at 1:45 PM,  <pkotwicz@chromium.org> wrote:
> sky@ can you please take another look?
>
> Sorry for the delay. I have added a multiset which has the BookmarkNodes
> ordered
> based on the icon URL.
> Please let me know what you think.
> - Peter
>
>
> https://codereview.chromium.org/1133463005/

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

[pkotwicz, 2015-06-11 15:06:28]

Scott, can you please take another look?

What do you mean by: "Large changes"?
BookmarkModel::OnFaviconsChanged() should not ever be called with more than one
icon URL

If a user does not have a Mac / iOS device synced,
BookmarkModel::OnFaviconsChanged() will be called very very rarely with != 0
|icon_urls|

If a user has a Mac / iOS device and a non Mac / iOS device syncing with each
other, BookmarkModel::OnFaviconsChanged() will be called with 1 icon URL:
- at most once for each bookmark icon URL after the user switches the device
that they are using
- the first time that the user visits a page which shares the icon URL with a
bookmark

Example:
1 bookmark: http://www.google.com
Mac laptop and Windows desktop syncing with each other
http://www.google.com was last visited on the Windows desktop

1) An omnibox search on the Mac will cause a call to
BookmarkModel::OnFaviconsChanged() with www.google.com/favicon.ico as the only
icon URL
2) Visiting any more Google pages will not cause any more calls to
BookmarkModel::OnFaviconsChanged()
3) Switching to the Windows desktop and visiting any Google page which uses the
http://www.google.com/favicon.ico favicon will cause a call to
BookmarkModel::OnFaviconsChanged() with www.google.com/favicon.ico as the only
icon URL
4) Visiting any more Google pages on the Windows desktop will not cause any more
calls to BookmarkModel::OnFaviconsChanged()


Mac is special because it encodes PNGs using a different codec so favicons
encoded on a Mac are not byte-equivalent to those encoded on Windows.




The case that BookmarkModel::OnFaviconsChanged() can be called
The only case that I can think of where we will get calls to
BookmarkModel::OnFaviconsChanged() with many icon_urls is when syncing between a
Mac/iOS device and a non-Mac device
If a user has http://www.facebook.com bookmarked and the user is using the

[pkotwicz, 2015-06-11 15:10:21]

Scott, can you please take another look?

What do you mean by: "Large changes"?
BookmarkModel::OnFaviconsChanged() should not ever be called with more than one
icon URL

If a user does not have a Mac / iOS device synced,
BookmarkModel::OnFaviconsChanged() will be called very very rarely with != 0
|icon_urls| (I previously thought that syncing with Android would be problematic
but it is not)

If a user has a Mac / iOS device and a non Mac / iOS device syncing with each
other, BookmarkModel::OnFaviconsChanged() will be called with 1 icon URL:
- at most once for each bookmark icon URL after the user switches the device
that they are using
- the first time that the user visits a page which shares the icon URL with a
bookmark

Example:
1 bookmark: http://www.google.com
Mac laptop and Windows desktop syncing with each other
http://www.google.com was last visited on the Windows desktop

1) An omnibox search on the Mac will cause a call to
BookmarkModel::OnFaviconsChanged() with www.google.com/favicon.ico as the only
icon URL
2) Visiting any more Google pages will not cause any more calls to
BookmarkModel::OnFaviconsChanged()
3) Switching to the Windows desktop and visiting any Google page which uses the
http://www.google.com/favicon.ico favicon will cause a call to
BookmarkModel::OnFaviconsChanged() with www.google.com/favicon.ico as the only
icon URL
4) Visiting any more Google pages on the Windows desktop will not cause any more
calls to BookmarkModel::OnFaviconsChanged()

Mac is special because it encodes PNGs using a different codec so favicons
encoded on a Mac are not byte-equivalent to those encoded on Windows.

[sky, 2015-06-15 14:40:54]

https://codereview.chromium.org/1133463005/diff/100001/components/bookmarks/b...
File components/bookmarks/browser/bookmark_model.h (right):

https://codereview.chromium.org/1133463005/diff/100001/components/bookmarks/b...
components/bookmarks/browser/bookmark_model.h:457: NodesOrderedByIconURLSet
nodes_ordered_by_favicon_url_set_;
If you're saying the favicons rarely change, then I don't see the point in
maintaining the set. Am I missing something?

[pkotwicz, 2015-06-15 14:50:21]

Scott, can you please take another look?

https://codereview.chromium.org/1133463005/diff/100001/components/bookmarks/b...
File components/bookmarks/browser/bookmark_model.h (right):

https://codereview.chromium.org/1133463005/diff/100001/components/bookmarks/b...
components/bookmarks/browser/bookmark_model.h:457: NodesOrderedByIconURLSet
nodes_ordered_by_favicon_url_set_;
I expect there to be 5 - 10 changes a day when syncing a Mac/iOS device with a
non-Mac device.

I don't know if this is considered "rare".

[sky, 2015-06-15 20:33:53]

Seems to me that is enough to not bother with the extra cache. How
about adding some histograms so we can be sure of that.

  -Scott

On Mon, Jun 15, 2015 at 7:50 AM,  <pkotwicz@chromium.org> wrote:
> Scott, can you please take another look?
>
>
>
https://codereview.chromium.org/1133463005/diff/100001/components/bookmarks/b...
> File components/bookmarks/browser/bookmark_model.h (right):
>
>
https://codereview.chromium.org/1133463005/diff/100001/components/bookmarks/b...
> components/bookmarks/browser/bookmark_model.h:457:
> NodesOrderedByIconURLSet nodes_ordered_by_favicon_url_set_;
> I expect there to be 5 - 10 changes a day when syncing a Mac/iOS device
> with a non-Mac device.
>
> I don't know if this is considered "rare".
>
> https://codereview.chromium.org/1133463005/

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

[pkotwicz, 2015-06-19 14:50:59]

Patchset #5 (id:120001) has been deleted

[pkotwicz, 2015-06-19 14:53:48]

Scott, can you please take another look?

I have removed the extra caching from BookmarkModel and have added a histogram
as you have suggested.

[pkotwicz, 2015-06-19 14:55:28]

Patchset #5 (id:140001) has been deleted

[sky, 2015-06-19 16:38:02]

https://codereview.chromium.org/1133463005/diff/160001/components/bookmarks/b...
File components/bookmarks/browser/bookmark_model.cc (right):

https://codereview.chromium.org/1133463005/diff/160001/components/bookmarks/b...
components/bookmarks/browser/bookmark_model.cc:500: // Log Histogram to
determine how often this method is called in practice.
WDYT of moving this to OnFaviconsChanged? What we care about there is both how
often it's called, and how many nodes are updated.

https://codereview.chromium.org/1133463005/diff/160001/components/bookmarks/b...
components/bookmarks/browser/bookmark_model.cc:506: for (const BookmarkNode*
node : nodes_ordered_by_url_set_) {
I prefer inlining this in OnFaviconsChanged and changing the ordering so you
loop through all the nodes seeing if the icon url of the node is in icon_urls.

https://codereview.chromium.org/1133463005/diff/160001/components/bookmarks/b...
File components/bookmarks/browser/bookmark_model.h (right):

https://codereview.chromium.org/1133463005/diff/160001/components/bookmarks/b...
components/bookmarks/browser/bookmark_model.h:171: std::vector<const
BookmarkNode*>* nodes);
I don't think we need order here. How about a set?

https://codereview.chromium.org/1133463005/diff/160001/components/history/cor...
File components/history/core/browser/history_service.h (right):

https://codereview.chromium.org/1133463005/diff/160001/components/history/cor...
components/history/core/browser/history_service.h:461: // Callback for when
favicon data changes. The first std::vector contains a
Is there a reason why these are vectors and not sets?

[pkotwicz, 2015-06-21 21:38:15]

Patchset #6 (id:180001) has been deleted

[pkotwicz, 2015-06-21 22:00:57]

Patchset #6 (id:200001) has been deleted

[pkotwicz, 2015-06-21 22:13:09]

Patchset #6 (id:220001) has been deleted

[pkotwicz, 2015-06-21 22:16:09]

Scott, can you please take another look?

https://codereview.chromium.org/1133463005/diff/160001/components/history/cor...
File components/history/core/browser/history_service.h (right):

https://codereview.chromium.org/1133463005/diff/160001/components/history/cor...
components/history/core/browser/history_service.h:461: // Callback for when
favicon data changes. The first std::vector contains a
I used std::vectors because we do not call std::set::find() on either of the
parameters and I think that std::vectors are slightly faster and slightly more
memory efficient than std::sets.

Regardless, I changed the code to use std::sets as you requested.

[sky, 2015-06-22 15:19:48]

https://codereview.chromium.org/1133463005/diff/240001/components/bookmarks/b...
File components/bookmarks/browser/bookmark_model.cc (right):

https://codereview.chromium.org/1133463005/diff/240001/components/bookmarks/b...
components/bookmarks/browser/bookmark_model.cc:448: for (const BookmarkNode*
node : nodes)
to_update.insert(nodes.begin(), nodes.end())

https://codereview.chromium.org/1133463005/diff/240001/components/bookmarks/b...
components/bookmarks/browser/bookmark_model.cc:457:
UMA_HISTOGRAM_BOOLEAN("Bookmarks.OnFaviconsChangedMultipleIconURLs", true);
Can you also include the size of icon_urls?

https://codereview.chromium.org/1133463005/diff/240001/components/bookmarks/b...
File components/bookmarks/browser/bookmark_model.h (right):

https://codereview.chromium.org/1133463005/diff/240001/components/bookmarks/b...
components/bookmarks/browser/bookmark_model.h:299: // Notify BookmarkModel that
the favicons for |urls| have changed and have to
nit: update comment.

https://codereview.chromium.org/1133463005/diff/240001/components/bookmarks/b...
components/bookmarks/browser/bookmark_model.h:301: void OnFaviconsChanged(const
std::set<GURL>& page_urls,
Please add test coverage of this function too.

[pkotwicz, 2015-06-26 15:13:26]

Patchset #7 (id:260001) has been deleted

[pkotwicz, 2015-06-26 15:14:28]

Scott, can you please take another look?

I have added tests for BookmarkModel::OnFaviconsChanged()

https://codereview.chromium.org/1133463005/diff/240001/components/bookmarks/b...
File components/bookmarks/browser/bookmark_model.cc (right):

https://codereview.chromium.org/1133463005/diff/240001/components/bookmarks/b...
components/bookmarks/browser/bookmark_model.cc:448: for (const BookmarkNode*
node : nodes)
On 2015/06/22 15:19:48, sky wrote:
> to_update.insert(nodes.begin(), nodes.end())

Done.

https://codereview.chromium.org/1133463005/diff/240001/components/bookmarks/b...
components/bookmarks/browser/bookmark_model.cc:457:
UMA_HISTOGRAM_BOOLEAN("Bookmarks.OnFaviconsChangedMultipleIconURLs", true);
|icon_urls| has at most one element. Do you want me to make the histogram take
in the size of |icon_urls| in case this changes?
My hope is to deprecate the histogram a couple of months after this CL lands

[sky, 2015-06-29 14:51:03]

https://codereview.chromium.org/1133463005/diff/240001/components/bookmarks/b...
File components/bookmarks/browser/bookmark_model.cc (right):

https://codereview.chromium.org/1133463005/diff/240001/components/bookmarks/b...
components/bookmarks/browser/bookmark_model.cc:457:
UMA_HISTOGRAM_BOOLEAN("Bookmarks.OnFaviconsChangedMultipleIconURLs", true);
On 2015/06/26 15:14:28, pkotwicz wrote:
> |icon_urls| has at most one element. Do you want me to make the histogram take
> in the size of |icon_urls| in case this changes?

In that case why do we need a set? Why not a single url and use an empty url to
indicate none?

> My hope is to deprecate the histogram a couple of months after this CL lands

[pkotwicz, 2015-07-01 17:33:40]

Patchset #8 (id:300001) has been deleted

[pkotwicz, 2015-07-01 17:42:10]

Patchset #8 (id:310022) has been deleted

[pkotwicz, 2015-07-01 17:43:02]

Scott, can you please take a look? I have replaced the icon URL std::set
parameter which a URL parameter as you have suggested

[sky, 2015-07-01 19:40:57]

LGTM

https://codereview.chromium.org/1133463005/diff/330001/components/history/cor...
File components/history/core/browser/history_service.h (right):

https://codereview.chromium.org/1133463005/diff/330001/components/history/cor...
components/history/core/browser/history_service.h:461: // Callback for when
favicon data changes. Contains a std::set of page URls
nit: URls -> URLs
also, you have URLs twice in a row.

[pkotwicz, 2015-07-05 22:23:42]

Patchset #9 (id:350001) has been deleted

[pkotwicz, 2015-07-05 22:23:50]

Patchset #9 (id:370001) has been deleted

[pkotwicz, 2015-07-05 22:27:29]

Patchset #9 (id:390001) has been deleted

[pkotwicz, 2015-07-05 22:53:06]

Patchset #9 (id:410001) has been deleted

[pkotwicz, 2015-07-05 22:57:05]

Patchset #9 (id:430001) has been deleted

[pkotwicz, 2015-07-05 22:59:42]

Patchset #9 (id:450001) has been deleted

[pkotwicz, 2015-07-05 23:39:56]

pkotwicz@chromium.org changed reviewers:
+ isherman@chromium.org

[pkotwicz, 2015-07-05 23:39:57]

isherman@ can you please take a look at the change in
tools/metrics/histograms/histograms.xml

[Ilya Sherman, 2015-07-06 17:57:40]

https://codereview.chromium.org/1133463005/diff/470001/components/bookmarks/b...
File components/bookmarks/browser/bookmark_model.cc (right):

https://codereview.chromium.org/1133463005/diff/470001/components/bookmarks/b...
components/bookmarks/browser/bookmark_model.cc:452:
UMA_HISTOGRAM_BOOLEAN("Bookmarks.OnFaviconsChangedIconURL", true);
I'd recommend building in a baseline for this histogram, so that you have some
context for the numbers you see on the dashboard.

https://codereview.chromium.org/1133463005/diff/470001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1133463005/diff/470001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:2620: +    non-empty |icon_url|.
How do you plan to interpret the numbers in this histogram?

[pkotwicz, 2015-07-06 18:47:30]

https://codereview.chromium.org/1133463005/diff/470001/components/bookmarks/b...
File components/bookmarks/browser/bookmark_model.cc (right):

https://codereview.chromium.org/1133463005/diff/470001/components/bookmarks/b...
components/bookmarks/browser/bookmark_model.cc:452:
UMA_HISTOGRAM_BOOLEAN("Bookmarks.OnFaviconsChangedIconURL", true);
How would I build a baseline?

https://codereview.chromium.org/1133463005/diff/470001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1133463005/diff/470001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:2620: +    non-empty |icon_url|.
I am planning on figuring out the average # of times that this histogram gets
logged per user per day. I may need to get logs access for that. If the average
is > 10 per day, then I will consider adding caching for BookmarkNodes based on
icon URL

[pkotwicz, 2015-07-06 18:47:57]

isherman@ can you please take another look?

[Ilya Sherman, 2015-07-07 02:40:36]

https://codereview.chromium.org/1133463005/diff/470001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1133463005/diff/470001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:2620: +    non-empty |icon_url|.
On 2015/07/06 18:47:30, pkotwicz wrote:
> I am planning on figuring out the average # of times that this histogram gets
> logged per user per day. I may need to get logs access for that. If the
average
> is > 10 per day, then I will consider adding caching for BookmarkNodes based
on
> icon URL

Hmm, I'd discourage you from just looking at an average.  For example, suppose
that 50% of users log this histogram once per day, and 50% log it 17 times per
day.  That looks like 9 times per day on average; but really, there are two
types of users: ones who hit it rarely, and ones who hit it frequently. 
Alternately, suppose that 99% of users log this histogram once per day, and a
very atypical 1% of users log it 1000 times per day -- perhaps because they are
actually page cycler bots.  Again, you'd get misleading data from this sort of
usage pattern.

Rather than looking at an average, I'd recommend trying to find a way that gives
you a per-user score or "cost", so that you can look at a distribution of costs
for analysis.  If you're willing to write a Dremel script, then this histogram
structure will probably give you the information that you need, so LGTM.  But I
do strongly recommend taking this advice into account when conducting the
statistical analysis of the data.

[pkotwicz, 2015-07-07 14:31:29]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2015-07-07 14:31:30]

The patchset sent to the CQ was uploaded after l-g-t-m from rogerm@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1133463005/#ps470001
(title: " ")

[commit-bot: I haz the power, 2015-07-07 14:31:51]

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

[commit-bot: I haz the power, 2015-07-07 14:43:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-07 14:43:21]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[pkotwicz, 2015-07-07 15:31:17]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2015-07-07 15:31:17]

The patchset sent to the CQ was uploaded after l-g-t-m from rogerm@chromium.org,
isherman@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1133463005/#ps490001
(title: " ")

[commit-bot: I haz the power, 2015-07-07 15:31:53]

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

[commit-bot: I haz the power, 2015-07-07 15:42:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-07 15:42:15]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[pkotwicz, 2015-07-07 17:08:34]

Patchset #11 (id:510001) has been deleted

[pkotwicz, 2015-07-07 18:38:37]

Patchset #10 (id:490001) has been deleted

[pkotwicz, 2015-07-07 18:39:32]

pkotwicz@chromium.org changed reviewers:
+ rohitrao@chromium.org

[pkotwicz, 2015-07-07 18:39:33]

rohitrao@ can you please take a look at the ios changes?

[rohitrao (ping after 24h), 2015-07-09 11:59:18]

ios/ LGTM

[pkotwicz, 2015-07-09 14:39:22]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2015-07-09 14:39:23]

The patchset sent to the CQ was uploaded after l-g-t-m from rogerm@chromium.org,
isherman@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1133463005/#ps530001
(title: " ")

[commit-bot: I haz the power, 2015-07-09 14:40:04]

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

[commit-bot: I haz the power, 2015-07-09 16:15:45]

Committed patchset #10 (id:530001)

[commit-bot: I haz the power, 2015-07-09 16:16:45]

Patchset 10 (id:??) landed as
https://crrev.com/ca240dde9b8b56120f93050ebfd200bf6714e56b
Cr-Commit-Position: refs/heads/master@{#338054}