Move encoding SkBitmaps to PNG out of FaviconService::SetFavicons() and onto the history thread

Move encoding SkBitmaps to PNG out of  FaviconService::SetFavicons() and onto
the history thread.

BUG=402104
TEST=None

Committed: https://crrev.com/687b5c0f1d008714d57e22dc3e419a27b8d6cf07
Cr-Commit-Position: refs/heads/master@{#293240}

[pkotwicz, 2014-09-01 21:37:07]

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

[pkotwicz, 2014-09-01 21:37:08]

Scott, PTAL

[sky, 2014-09-02 17:57:43]

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

https://codereview.chromium.org/531493002/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_backend.cc:1974: bool* favicon_bitmaps_changed) {
This code and call site would be simpler if you remove setting
favicon_bitamps_changed to false and assume non-null.

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

https://codereview.chromium.org/531493002/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_service.h:706: void SetFavicon(const GURL&
page_url,
I think the plural is more typical of what we use, eg SetFaviconsOutOfDate.
CloneFavicons...

[pkotwicz, 2014-09-03 04:15:09]

Scott, can you please take another look?

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

https://codereview.chromium.org/531493002/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_backend.cc:1974: bool* favicon_bitmaps_changed) {
You're right, done!

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

https://codereview.chromium.org/531493002/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_service.h:706: void SetFavicon(const GURL&
page_url,
SetFavicon() makes more sense to me. (It matches with the names of the history
database tables)
There is one favicon per icon URL.
There are multiple favicons (each with its own icon URL) per page URL. This is
the case for SetFaviconsOutOfDateForPage(), CloneFavicons() etc.
There are multiple favicon bitmaps per icon URL

[sky, 2014-09-03 15:34:08]

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

https://codereview.chromium.org/531493002/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_service.h:706: void SetFavicon(const GURL&
page_url,
On 2014/09/03 04:15:08, pkotwicz wrote:
> SetFavicon() makes more sense to me. (It matches with the names of the history
> database tables)
> There is one favicon per icon URL.
> There are multiple favicons (each with its own icon URL) per page URL. This is
> the case for SetFaviconsOutOfDateForPage(), CloneFavicons() etc.
> There are multiple favicon bitmaps per icon URL

I get what you're saying, but I still think plural is more consistent with with
the rest of this class.

[pkotwicz, 2014-09-03 20:18:56]

Scott, can you please take another look?

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

https://codereview.chromium.org/531493002/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_service.h:706: void SetFavicon(const GURL&
page_url,
On 2014/09/03 15:34:08, sky wrote:
> On 2014/09/03 04:15:08, pkotwicz wrote:
> > SetFavicon() makes more sense to me. (It matches with the names of the
history
> > database tables)
> > There is one favicon per icon URL.
> > There are multiple favicons (each with its own icon URL) per page URL. This
is
> > the case for SetFaviconsOutOfDateForPage(), CloneFavicons() etc.
> > There are multiple favicon bitmaps per icon URL
> 
> I get what you're saying, but I still think plural is more consistent with
with
> the rest of this class.

Done.

[sky, 2014-09-03 21:23:49]

LGTM

[pkotwicz, 2014-09-03 21:24:57]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-09-03 21:26:59]

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

[commit-bot: I haz the power, 2014-09-04 00:44:11]

Committed patchset #4 (id:60001) as 3d2424aea9883cedcf78479de6ac358a754938f2

[commit-bot: I haz the power, 2014-09-10 03:29:09]

Patchset 4 (id:??) landed as
https://crrev.com/687b5c0f1d008714d57e22dc3e419a27b8d6cf07
Cr-Commit-Position: refs/heads/master@{#293240}