Introduce structures to reduce the number of arguments in the FaviconService methods.

Introduce structures to reduce the number of arguments in the FaviconService methods.

Issue 10870022 introduced new arguments for the FaviconService methods, some of
them now having more than 6 arguments. These methods cannot be called using the
base::Bind mechanism because of template limitations.

This patch introduces auxiliar structures to reduce the number of arguments in
these methods and therefore allow using base::Bind on them.

BUG=146003
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155113

[Leandro Graciá Gil, 2012-09-05 06:20:39]

Context: comments at the end of https://chromiumcodereview.appspot.com/10919055/


Ryan, Peter, Albert: new structified approach for FaviconService methods.
jam: owner seal stamp.

[jam, 2012-09-05 16:34:43]

On 2012/09/05 06:20:39, Leandro Graciá Gil wrote:
> Context: comments at the end of
https://chromiumcodereview.appspot.com/10919055/
> 
> 
> Ryan, Peter, Albert: new structified approach for FaviconService methods.
> jam: owner seal stamp.

I'm overloaded with reviews for content, so please send this to another owner in
src\chrome\owners

[Leandro Graciá Gil, 2012-09-05 16:45:39]

On 2012/09/05 16:34:43, John Abd-El-Malek wrote:
> On 2012/09/05 06:20:39, Leandro Graciá Gil wrote:
> > Context: comments at the end of
> https://chromiumcodereview.appspot.com/10919055/
> > 
> > 
> > Ryan, Peter, Albert: new structified approach for FaviconService methods.
> > jam: owner seal stamp.
> 
> I'm overloaded with reviews for content, so please send this to another owner
in
> src\chrome\owners

Np. Sorry for any inconveniences.

[Leandro Graciá Gil, 2012-09-05 17:20:26]

darin: I previously asked jam@, but he's a bit overflown by patches now. I'll
need owner approval after Ryan, Peter or Albert approve.

Thanks.

[Peter Kasting, 2012-09-05 17:51:37]

The favicon_service.* code has lots of almost-duplicate logic that seems to
mainly differ by whether and how "desired scale factors" are passed in.  Then
the implementation has various TODO(pkotwicz) notes on actually passing a lot of
those values through.

It seems like it would be much better, if possible, to simplify the API here
down to one or two functions, and then if necessary provide multiple
constructors for your new struct that take no scale factors/one scale factor/a
vector of scale factors, and convert them all to the vector format.  Perhaps you
could consult with pkotwicz on whether this is feasible and how to accomplish
it?

Rubber-stamp LGTM otherwise.

[awong, 2012-09-05 17:52:43]

https://codereview.chromium.org/10918065/diff/6002/chrome/browser/favicon/fav...
File chrome/browser/favicon/favicon_service.cc (right):

https://codereview.chromium.org/10918065/diff/6002/chrome/browser/favicon/fav...
chrome/browser/favicon/favicon_service.cc:190: if (history_service_)
nit: use braces for the if/else since one of the bodies spans multiple lines.

https://codereview.chromium.org/10918065/diff/6002/chrome/browser/favicon/fav...
File chrome/browser/favicon/favicon_service.h (right):

https://codereview.chromium.org/10918065/diff/6002/chrome/browser/favicon/fav...
chrome/browser/favicon/favicon_service.h:46: const GURL& icon_url;
Storing a reference as a member variable seems dangerous since the argument
might be a local that goes out of scope. Shouldn't this be a copy?

https://codereview.chromium.org/10918065/diff/6002/chrome/browser/favicon/fav...
chrome/browser/favicon/favicon_service.h:67: const GURL& page_url;
same as above.

[Leandro Graciá Gil, 2012-09-05 17:57:50]

pkotwicz: could you tell us what do you think of this and about Peter's comment
above? I'd like to keep this as simple as possible as, until fixed, this is
breaking the downstream code of the Android port and preventing to upstream the
affected code.

[awong, 2012-09-05 18:11:45]

Just talked with Leandro offline a little.

Sounds like leaving this out will break Android's merge (I think that's
equivalent to Webkit change breaking gardening merge).

Can I suggest we change the const& member into a actual copy of the GURL,
and then address Peter's concerns in a follow up CL so we keep the Android
merge green?

-Albert



On Wed, Sep 5, 2012 at 10:57 AM, <leandrogracia@chromium.org> wrote:

> pkotwicz: could you tell us what do you think of this and about Peter's
> comment
> above? I'd like to keep this as simple as possible as, until fixed, this is
> breaking the downstream code of the Android port and preventing to
> upstream the
> affected code.
>
>
https://codereview.chromium.**org/10918065/<https://codereview.chromium.org/1...
>

[Peter Kasting, 2012-09-05 18:13:27]

I'm fine leaving my issue to be dealt with separately.

[Leandro Graciá Gil, 2012-09-05 18:15:39]

On 2012/09/05 18:13:27, Peter Kasting wrote:
> I'm fine leaving my issue to be dealt with separately.

Thanks, I'll prepare a new patch.

[Leandro Graciá Gil, 2012-09-05 18:28:36]

I'll be landing this manually and cherry-pick it downstream to prevent the
Android build breaking. Thanks for the review.

https://codereview.chromium.org/10918065/diff/6002/chrome/browser/favicon/fav...
File chrome/browser/favicon/favicon_service.cc (right):

https://codereview.chromium.org/10918065/diff/6002/chrome/browser/favicon/fav...
chrome/browser/favicon/favicon_service.cc:190: if (history_service_)
On 2012/09/05 17:52:44, awong wrote:
> nit: use braces for the if/else since one of the bodies spans multiple lines.

Done.

https://codereview.chromium.org/10918065/diff/6002/chrome/browser/favicon/fav...
File chrome/browser/favicon/favicon_service.h (right):

https://codereview.chromium.org/10918065/diff/6002/chrome/browser/favicon/fav...
chrome/browser/favicon/favicon_service.h:46: const GURL& icon_url;
On 2012/09/05 17:52:44, awong wrote:
> Storing a reference as a member variable seems dangerous since the argument
> might be a local that goes out of scope. Shouldn't this be a copy?

Done.

https://codereview.chromium.org/10918065/diff/6002/chrome/browser/favicon/fav...
chrome/browser/favicon/favicon_service.h:67: const GURL& page_url;
On 2012/09/05 17:52:44, awong wrote:
> same as above.

Done.

[Leandro Graciá Gil, 2012-09-05 18:32:29]

Looks like I'm still missing owner rubber stamp. Since there is a merge issue +
build break involved I'll be landing this with --bypass-hooks. Darin: please let
me know any changes you might require and I'll do them in a separate follow up.
Sorry for any inconveniences.

On 2012/09/05 18:28:36, Leandro Graciá Gil wrote:
> I'll be landing this manually and cherry-pick it downstream to prevent the
> Android build breaking. Thanks for the review.
> 
>
https://codereview.chromium.org/10918065/diff/6002/chrome/browser/favicon/fav...
> File chrome/browser/favicon/favicon_service.cc (right):
> 
>
https://codereview.chromium.org/10918065/diff/6002/chrome/browser/favicon/fav...
> chrome/browser/favicon/favicon_service.cc:190: if (history_service_)
> On 2012/09/05 17:52:44, awong wrote:
> > nit: use braces for the if/else since one of the bodies spans multiple
lines.
> 
> Done.
> 
>
https://codereview.chromium.org/10918065/diff/6002/chrome/browser/favicon/fav...
> File chrome/browser/favicon/favicon_service.h (right):
> 
>
https://codereview.chromium.org/10918065/diff/6002/chrome/browser/favicon/fav...
> chrome/browser/favicon/favicon_service.h:46: const GURL& icon_url;
> On 2012/09/05 17:52:44, awong wrote:
> > Storing a reference as a member variable seems dangerous since the argument
> > might be a local that goes out of scope. Shouldn't this be a copy?
> 
> Done.
> 
>
https://codereview.chromium.org/10918065/diff/6002/chrome/browser/favicon/fav...
> chrome/browser/favicon/favicon_service.h:67: const GURL& page_url;
> On 2012/09/05 17:52:44, awong wrote:
> > same as above.
> 
> Done.

[Leandro Graciá Gil, 2012-09-05 20:26:18]

stevenjb: since this involves the favicon code you might be interested. We
intend to land this as an initial fix to prevent the downstream Android build
from breaking down shortly. Please let me know if there are any specific changes
that should be done now. I'm happy to fix anything else in a follow-up patch.

[James Hawkins, 2012-09-05 22:02:58]

https://codereview.chromium.org/10918065/diff/1017/chrome/browser/favicon/fav...
File chrome/browser/favicon/favicon_service.h (right):

https://codereview.chromium.org/10918065/diff/1017/chrome/browser/favicon/fav...
chrome/browser/favicon/favicon_service.h:34: // Auxiliary argument structure for
requesting favicons.
Why is this necessary?

[Leandro Graciá Gil, 2012-09-05 22:04:27]

https://codereview.chromium.org/10918065/diff/1017/chrome/browser/favicon/fav...
File chrome/browser/favicon/favicon_service.h (right):

https://codereview.chromium.org/10918065/diff/1017/chrome/browser/favicon/fav...
chrome/browser/favicon/favicon_service.h:34: // Auxiliary argument structure for
requesting favicons.
On 2012/09/05 22:02:58, James Hawkins wrote:
> Why is this necessary?

I could remove this case and leave only the URL ones if you prefer. Just wanted
to make the change consistent.

[James Hawkins, 2012-09-05 22:11:49]

https://codereview.chromium.org/10918065/diff/1017/chrome/browser/favicon/fav...
File chrome/browser/favicon/favicon_service.h (right):

https://codereview.chromium.org/10918065/diff/1017/chrome/browser/favicon/fav...
chrome/browser/favicon/favicon_service.h:34: // Auxiliary argument structure for
requesting favicons.
On 2012/09/05 22:04:28, Leandro Graciá Gil wrote:
> On 2012/09/05 22:02:58, James Hawkins wrote:
> > Why is this necessary?
> 
> I could remove this case and leave only the URL ones if you prefer. Just
wanted
> to make the change consistent.

Yes, please remove.  The FaviconForURLParams is a temporary solution until we
solve the underlying issue.

[Leandro Graciá Gil, 2012-09-05 22:38:10]

On 2012/09/05 22:11:49, James Hawkins wrote:
>
https://codereview.chromium.org/10918065/diff/1017/chrome/browser/favicon/fav...
> File chrome/browser/favicon/favicon_service.h (right):
> 
>
https://codereview.chromium.org/10918065/diff/1017/chrome/browser/favicon/fav...
> chrome/browser/favicon/favicon_service.h:34: // Auxiliary argument structure
for
> requesting favicons.
> On 2012/09/05 22:04:28, Leandro Graciá Gil wrote:
> > On 2012/09/05 22:02:58, James Hawkins wrote:
> > > Why is this necessary?
> > 
> > I could remove this case and leave only the URL ones if you prefer. Just
> wanted
> > to make the change consistent.
> 
> Yes, please remove.  The FaviconForURLParams is a temporary solution until we
> solve the underlying issue.

Done.

[James Hawkins, 2012-09-05 22:39:49]

LGTM with nit.  Make sure to TBR stevenjb if he's out of town.

https://codereview.chromium.org/10918065/diff/1018/chrome/browser/ui/webui/nt...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

https://codereview.chromium.org/10918065/diff/1018/chrome/browser/ui/webui/nt...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:709:
FaviconService::FaviconForURLParams(profile, launch_url, history::FAVICON,
nit: The start of param rows must align on the same column.

[Leandro Graciá Gil, 2012-09-05 23:02:28]

On 2012/09/05 22:39:49, James Hawkins wrote:
> LGTM with nit.  Make sure to TBR stevenjb if he's out of town.
> 
>
https://codereview.chromium.org/10918065/diff/1018/chrome/browser/ui/webui/nt...
> File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):
> 
>
https://codereview.chromium.org/10918065/diff/1018/chrome/browser/ui/webui/nt...
> chrome/browser/ui/webui/ntp/app_launcher_handler.cc:709:
> FaviconService::FaviconForURLParams(profile, launch_url, history::FAVICON,
> nit: The start of param rows must align on the same column.

As commented by chat I'll be fixing the nit in any follow-ups. I've got problems
accessing my machine and I can't upload a new patch until probably tomorrow.
Since we want this ASAP I'll be landing it as it is.

Thanks for all your help and patience!

[commit-bot: I haz the power, 2012-09-05 23:02:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leandrogracia@chromium.org/10918065/1018

[commit-bot: I haz the power, 2012-09-06 03:42:39]

Change committed as 155113