Move favicon callbacks to favicon_base

Move favicon callbacks to favicon_base

Move FaviconImageCallback, FaviconResultsCallback and FaviconRawCallback
to favicon_base, those types are shared between favicon and history
components (methods are declared in FaviconService, but it delegates the
work to HistoryService) and only uses types from base:: and
favicon_base:: namespaces.

Also BookmarkClient::FaviconImageCallback is a copy of
FaviconService::FaviconImageCallback, replace it with favicon_base::
version.

BUG=372321
TBR=thakis for //chrome outside of //chrome/browser/history,
//chrome/browser/favicon

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

[tfarina, 2014-05-16 20:30:17]

https://codereview.chromium.org/291643002/diff/1/components/bookmarks/core/br...
File components/bookmarks/core/browser/bookmark_client.h (right):

https://codereview.chromium.org/291643002/diff/1/components/bookmarks/core/br...
components/bookmarks/core/browser/bookmark_client.h:24: struct
FaviconImageResult;
you can remove this forward declaration now, it is in favicon_callback.h now.

https://codereview.chromium.org/291643002/diff/1/components/favicon_base/favi...
File components/favicon_base/favicon_callback.h (right):

https://codereview.chromium.org/291643002/diff/1/components/favicon_base/favi...
components/favicon_base/favicon_callback.h:49: 
nit: remove trailing new line.

[Jiang Jiang, 2014-05-16 20:56:15]

https://codereview.chromium.org/291643002/diff/1/components/bookmarks/core/br...
File components/bookmarks/core/browser/bookmark_client.h (right):

https://codereview.chromium.org/291643002/diff/1/components/bookmarks/core/br...
components/bookmarks/core/browser/bookmark_client.h:24: struct
FaviconImageResult;
On 2014/05/16 20:30:17, tfarina wrote:
> you can remove this forward declaration now, it is in favicon_callback.h now.

Done.

https://codereview.chromium.org/291643002/diff/1/components/favicon_base/favi...
File components/favicon_base/favicon_callback.h (right):

https://codereview.chromium.org/291643002/diff/1/components/favicon_base/favi...
components/favicon_base/favicon_callback.h:49: 
On 2014/05/16 20:30:17, tfarina wrote:
> nit: remove trailing new line.

Done.

[blundell, 2014-05-19 07:50:30]

Sylvain,

Could you do an initial review?

https://codereview.chromium.org/291643002/diff/80001/components/favicon_base/...
File components/favicon_base/favicon_callback.h (right):

https://codereview.chromium.org/291643002/diff/80001/components/favicon_base/...
components/favicon_base/favicon_callback.h:17: // Callback for
FaviconService::GetFaviconImage(),
Hmm, these comments are not the right ones now that the functions being
referenced are in another component entirely. Sylvain, could you advise on what
the comments could/should be now? Maybe just generalize to something like
"Callback for functions that have asked for favicons by URL, icon type, desired
size" and etc?

[sdefresne, 2014-05-19 14:36:39]

Looks good once the comments have been fixed.

https://codereview.chromium.org/291643002/diff/80001/chrome/browser/favicon/f...
File chrome/browser/favicon/favicon_service.h (left):

https://codereview.chromium.org/291643002/diff/80001/chrome/browser/favicon/f...
chrome/browser/favicon/favicon_service.h:50: // |FaviconImageResult::image| is
constructed from the bitmaps for the
This comment describe the GetFaviconImage & GetFaviconImageForURL, not the
callback. It should be moved before those functions, not in favicon_callback.h.

https://codereview.chromium.org/291643002/diff/80001/chrome/browser/favicon/f...
chrome/browser/favicon/favicon_service.h:69: // The first argument is the set of
bitmaps for the passed in URL and
This comment describe the GetFavicon & GetFaviconForURL, not the
callback. It should be moved before those functions, not in favicon_callback.h.

https://codereview.chromium.org/291643002/diff/80001/components/favicon_base/...
File components/favicon_base/favicon_callback.h (right):

https://codereview.chromium.org/291643002/diff/80001/components/favicon_base/...
components/favicon_base/favicon_callback.h:17: // Callback for
FaviconService::GetFaviconImage(),
On 2014/05/19 07:50:30, blundell wrote:
> Hmm, these comments are not the right ones now that the functions being
> referenced are in another component entirely. Sylvain, could you advise on
what
> the comments could/should be now? Maybe just generalize to something like
> "Callback for functions that have asked for favicons by URL, icon type,
desired
> size" and etc?

I'd got with something like:

// Callback for functions returning a |gfx::Image| and the |GURL| it is loaded
// from. They are returned as a |favicon_base::FaviconImageResult| object.

https://codereview.chromium.org/291643002/diff/80001/components/favicon_base/...
components/favicon_base/favicon_callback.h:30: // Callback for GetRawFavicon(),
GetRawFaviconForURL() and
// Callback for functions returning raw data for a favicon.

https://codereview.chromium.org/291643002/diff/80001/components/favicon_base/...
components/favicon_base/favicon_callback.h:35: // Callback for GetFavicon() and
GetFaviconForURL().
// Callback for functions returning raw data for a favicon in multiple
resolution.

[blundell, 2014-05-19 15:12:58]

https://codereview.chromium.org/291643002/diff/80001/components/favicon_base/...
File components/favicon_base/favicon_callback.h (right):

https://codereview.chromium.org/291643002/diff/80001/components/favicon_base/...
components/favicon_base/favicon_callback.h:17: // Callback for
FaviconService::GetFaviconImage(),
s/for functions returning/that can be used to return

here and in the below suggested comments

On 2014/05/19 14:36:39, sdefresne wrote:
> On 2014/05/19 07:50:30, blundell wrote:
> > Hmm, these comments are not the right ones now that the functions being
> > referenced are in another component entirely. Sylvain, could you advise on
> what
> > the comments could/should be now? Maybe just generalize to something like
> > "Callback for functions that have asked for favicons by URL, icon type,
> desired
> > size" and etc?
> 
> I'd got with something like:
> 
> // Callback for functions returning a |gfx::Image| and the |GURL| it is loaded
> // from. They are returned as a |favicon_base::FaviconImageResult| object.

https://codereview.chromium.org/291643002/diff/80001/components/favicon_base/...
components/favicon_base/favicon_callback.h:30: // Callback for GetRawFavicon(),
GetRawFaviconForURL() and
Sylvain,

what is raw data? is there any way to be more specific?

On 2014/05/19 14:36:39, sdefresne wrote:
> // Callback for functions returning raw data for a favicon.

https://codereview.chromium.org/291643002/diff/80001/components/favicon_base/...
components/favicon_base/favicon_callback.h:35: // Callback for GetFavicon() and
GetFaviconForURL().
same question as above for Sylvain

On 2014/05/19 14:36:39, sdefresne wrote:
> // Callback for functions returning raw data for a favicon in multiple
> resolution.

[Jiang Jiang, 2014-05-19 15:56:40]

https://codereview.chromium.org/291643002/diff/80001/chrome/browser/favicon/f...
File chrome/browser/favicon/favicon_service.h (left):

https://codereview.chromium.org/291643002/diff/80001/chrome/browser/favicon/f...
chrome/browser/favicon/favicon_service.h:50: // |FaviconImageResult::image| is
constructed from the bitmaps for the
On 2014/05/19 14:36:39, sdefresne wrote:
> This comment describe the GetFaviconImage & GetFaviconImageForURL, not the
> callback. It should be moved before those functions, not in
favicon_callback.h.

Done.

https://codereview.chromium.org/291643002/diff/80001/chrome/browser/favicon/f...
chrome/browser/favicon/favicon_service.h:69: // The first argument is the set of
bitmaps for the passed in URL and
On 2014/05/19 14:36:39, sdefresne wrote:
> This comment describe the GetFavicon & GetFaviconForURL, not the
> callback. It should be moved before those functions, not in
favicon_callback.h.

Done.

https://codereview.chromium.org/291643002/diff/80001/components/favicon_base/...
File components/favicon_base/favicon_callback.h (right):

https://codereview.chromium.org/291643002/diff/80001/components/favicon_base/...
components/favicon_base/favicon_callback.h:17: // Callback for
FaviconService::GetFaviconImage(),
On 2014/05/19 15:12:59, blundell wrote:
> s/for functions returning/that can be used to return
> 
> here and in the below suggested comments
> 
> On 2014/05/19 14:36:39, sdefresne wrote:
> > On 2014/05/19 07:50:30, blundell wrote:
> > > Hmm, these comments are not the right ones now that the functions being
> > > referenced are in another component entirely. Sylvain, could you advise on
> > what
> > > the comments could/should be now? Maybe just generalize to something like
> > > "Callback for functions that have asked for favicons by URL, icon type,
> > desired
> > > size" and etc?
> > 
> > I'd got with something like:
> > 
> > // Callback for functions returning a |gfx::Image| and the |GURL| it is
loaded
> > // from. They are returned as a |favicon_base::FaviconImageResult| object.
> 

Done.

[sdefresne, 2014-05-19 16:30:04]

https://codereview.chromium.org/291643002/diff/80001/components/favicon_base/...
File components/favicon_base/favicon_callback.h (right):

https://codereview.chromium.org/291643002/diff/80001/components/favicon_base/...
components/favicon_base/favicon_callback.h:30: // Callback for GetRawFavicon(),
GetRawFaviconForURL() and
On 2014/05/19 15:12:59, blundell wrote:
> Sylvain,
> 
> what is raw data? is there any way to be more specific?
> 
> On 2014/05/19 14:36:39, sdefresne wrote:
> > // Callback for functions returning raw data for a favicon.
> 

FaviconBitmapResult does not store the favicon as a gfx::Image but as a
base::RefCountedMemory. I think this is to allow the code to select a favicon
before loading it as a gfx::Image (which may introduce additional memory
allocation). Maybe adding a reference to FaviconBitmapResult and stating that
the data is not yet converted as a gfx::Image?

[blundell, 2014-05-20 11:51:20]

Sylvain,

Could you take one more pass over the comments in favicon_callback.h and
favicon_service.h?

[sdefresne, 2014-05-20 13:56:22]

lgtm

[blundell, 2014-05-20 14:12:18]

LGTM with comment addressed

You can TBR=thakis for //chrome OWNERS

https://codereview.chromium.org/291643002/diff/110001/components/favicon_base...
File components/favicon_base/favicon_callback.h (right):

https://codereview.chromium.org/291643002/diff/110001/components/favicon_base...
components/favicon_base/favicon_callback.h:5: #ifndef
COMPONENTS_FAVICON_BASE_FAVICON_CALLBACK_H_
This file should get added to favicon_base.gypi.

[Jiang Jiang, 2014-05-20 16:19:10]

On 2014/05/20 14:12:18, blundell wrote:
> LGTM with comment addressed
> 
> You can TBR=thakis for //chrome OWNERS
> 
>
https://codereview.chromium.org/291643002/diff/110001/components/favicon_base...
> File components/favicon_base/favicon_callback.h (right):
> 
>
https://codereview.chromium.org/291643002/diff/110001/components/favicon_base...
> components/favicon_base/favicon_callback.h:5: #ifndef
> COMPONENTS_FAVICON_BASE_FAVICON_CALLBACK_H_
> This file should get added to favicon_base.gypi.

Done.

[Jiang Jiang, 2014-05-20 16:19:19]

The CQ bit was checked by jiangj@opera.com

[commit-bot: I haz the power, 2014-05-20 16:19:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jiangj@opera.com/291643002/150001

[Jiang Jiang, 2014-05-20 17:04:07]

The CQ bit was unchecked by jiangj@opera.com

[Jiang Jiang, 2014-05-20 17:05:22]

The CQ bit was checked by jiangj@opera.com

[commit-bot: I haz the power, 2014-05-20 17:06:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jiangj@opera.com/291643002/170001

[commit-bot: I haz the power, 2014-05-20 19:49:26]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)

[commit-bot: I haz the power, 2014-05-20 20:04:31]

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

[commit-bot: I haz the power, 2014-05-20 20:04:32]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)

[Jiang Jiang, 2014-05-20 22:45:50]

The CQ bit was checked by jiangj@opera.com

[commit-bot: I haz the power, 2014-05-20 22:46:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jiangj@opera.com/291643002/170001

[commit-bot: I haz the power, 2014-05-20 23:20:08]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)

[commit-bot: I haz the power, 2014-05-20 23:37:49]

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

[commit-bot: I haz the power, 2014-05-20 23:37:50]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)

[sdefresne, 2014-05-21 08:10:11]

On 2014/05/20 23:37:50, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   android_clang_dbg on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)

I think you CL is conflicting with https://codereview.chromium.org/284893003.
Rebase should be easy, as the pointed CL just moved files.

[Jiang Jiang, 2014-05-21 08:15:56]

On 2014/05/21 08:10:11, sdefresne wrote:
> On 2014/05/20 23:37:50, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   android_clang_dbg on tryserver.chromium
> >
>
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
> 
> I think you CL is conflicting with https://codereview.chromium.org/284893003.
> Rebase should be easy, as the pointed CL just moved files.

Right, I just noticed that as well. Will rebase.

[blundell, 2014-05-21 08:22:02]

TBR=thakis for //chrome outside of //chrome/browser/history,
//chrome/browser/favicon

(Jiang: note that when adding a TBR, you should add the OWNER to the reviewers
list and send out an email alerting them to what you're asking them to review).

[Jiang Jiang, 2014-05-21 09:20:56]

On 2014/05/21 08:22:02, blundell wrote:
> TBR=thakis for //chrome outside of //chrome/browser/history,
> //chrome/browser/favicon
> 
> (Jiang: note that when adding a TBR, you should add the OWNER to the reviewers
> list and send out an email alerting them to what you're asking them to
review).

Thanks! I will keep that in mind.

[Jiang Jiang, 2014-05-21 09:21:28]

The CQ bit was checked by jiangj@opera.com

[commit-bot: I haz the power, 2014-05-21 09:23:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jiangj@opera.com/291643002/190001

[Jiang Jiang, 2014-05-21 18:30:47]

The CQ bit was unchecked by jiangj@opera.com

[Jiang Jiang, 2014-05-21 18:30:55]

The CQ bit was checked by jiangj@opera.com

[commit-bot: I haz the power, 2014-05-21 19:57:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jiangj@opera.com/291643002/190001

[commit-bot: I haz the power, 2014-05-22 03:11:20]

Change committed as 272065