[Extensions] Update developerPrivate API to use base-64 data urls for icons

[Extensions] Update developerPrivate API to use base-64 data urls for icons

Change the developerPrivate API to uniformly use base-64 data urls for icons,
rather than relying on the custom url handler.

BUG=461039
Committed: https://crrev.com/59f6756e6aa719a7d727ecb98456d185d1b57752
Cr-Commit-Position: refs/heads/master@{#327095}

[Devlin, 2015-04-22 20:12:35]

Patchset #1 (id:1) has been deleted

[Devlin, 2015-04-22 20:13:40]

rdevlin.cronin@chromium.org changed reviewers:
+ kalman@chromium.org

[Devlin, 2015-04-22 20:13:40]

Kind of a pain... but more-or-less necessary, too.

[not at google - send to devlin, 2015-04-22 21:38:37]

https://codereview.chromium.org/1092953004/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/1092953004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:459:
Respond(info.get() ? OneArgument(info->ToValue().release()) :
there is a version of OneArgument which takes a scoped_ptr

https://codereview.chromium.org/1092953004/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/developer_private/extension_info_generator.cc
(right):

https://codereview.chromium.org/1092953004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/extension_info_generator.cc:186:
callback_ = callback;
You already assigned on line 166?

https://codereview.chromium.org/1092953004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/extension_info_generator.cc:222:
callback_ = callback;
(ditto)

https://codereview.chromium.org/1092953004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/extension_info_generator.cc:443:
gfx::Size(128, 128),
Where does 128/128 come from?

https://codereview.chromium.org/1092953004/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/developer_private/extension_info_generator.h
(right):

https://codereview.chromium.org/1092953004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/extension_info_generator.h:91:
ExtensionInfosCallback callback_;
|callback_| and |list_| mean that there can be only one outgoing call to
CreateExtension[s]Info. The easiest way for callers to think about that is that
ExtensionInfoGenerator is therefore single-use; construct one each time a call
needs to be made.

However, that then kills your default icon caching. However, callers don't
actually cache them anyway so it's no worse.

Here are some options:
- Hide the ability to construct an ExtensionInfoGenerator, and instead make its
constructor private with CreateExtension[s]Info static and take an extra
BrowserContext variable. Typesafe way of making it single-use.
- Pass around |list_| and |callback_| through your callback chains.
- Cache the default icons globally rather than per ExtensionInfoGenerator (not
mutually exclusive to other options).

https://codereview.chromium.org/1092953004/diff/20001/chrome/browser/extensio...
File
chrome/browser/extensions/api/developer_private/extension_info_generator_unittest.cc
(right):

https://codereview.chromium.org/1092953004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/extension_info_generator_unittest.cc:77:
linked_ptr<developer::ExtensionInfo> result = last_info_;
Rather than making last_info_ a member variable you can keep everything local by
passing a linked_ptr* to OnInfosGenerated. Assign list[0] to it. It would look
like:

linked_ptr<developer::ExtensionInfo> result;
generator->CreateExtensionInfo(..., base::Bind(..., &result));
return result;

https://codereview.chromium.org/1092953004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/extension_info_generator_unittest.cc:103:
linked_ptr<developer::ExtensionInfo> CreateExtensionInfoFromPath(
Why did you need to change this from scoped_ptr to linked_ptr?

[Devlin, 2015-04-23 19:17:21]

https://codereview.chromium.org/1092953004/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/1092953004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:459:
Respond(info.get() ? OneArgument(info->ToValue().release()) :
On 2015/04/22 21:38:37, kalman wrote:
> there is a version of OneArgument which takes a scoped_ptr

Oh yeah, forgot I added that.  Done.

https://codereview.chromium.org/1092953004/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/developer_private/extension_info_generator.cc
(right):

https://codereview.chromium.org/1092953004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/extension_info_generator.cc:186:
callback_ = callback;
On 2015/04/22 21:38:37, kalman wrote:
> You already assigned on line 166?

Whoops!  I don't want to.

https://codereview.chromium.org/1092953004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/extension_info_generator.cc:222:
callback_ = callback;
On 2015/04/22 21:38:37, kalman wrote:
> (ditto)

Done.

https://codereview.chromium.org/1092953004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/extension_info_generator.cc:443:
gfx::Size(128, 128),
On 2015/04/22 21:38:37, kalman wrote:
> Where does 128/128 come from?

Added a comment.

https://codereview.chromium.org/1092953004/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/developer_private/extension_info_generator.h
(right):

https://codereview.chromium.org/1092953004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/extension_info_generator.h:91:
ExtensionInfosCallback callback_;
On 2015/04/22 21:38:37, kalman wrote:
> |callback_| and |list_| mean that there can be only one outgoing call to
> CreateExtension[s]Info. The easiest way for callers to think about that is
that
> ExtensionInfoGenerator is therefore single-use; construct one each time a call
> needs to be made.
> 
> However, that then kills your default icon caching. However, callers don't
> actually cache them anyway so it's no worse.
> 
> Here are some options:
> - Hide the ability to construct an ExtensionInfoGenerator, and instead make
its
> constructor private with CreateExtension[s]Info static and take an extra
> BrowserContext variable. Typesafe way of making it single-use.
> - Pass around |list_| and |callback_| through your callback chains.
> - Cache the default icons globally rather than per ExtensionInfoGenerator (not
> mutually exclusive to other options).

Yeah, always meant this to be a one-time-use thing (passing around list_ and
callback_ gets pretty crazy since these are all asynchronous in multiple
places).  The disadvantage to doing a static method is that early-aborting if
our owner goes away isn't an option, and it makes for a slightly less pretty
interface, IMO.  Would a comment and a few DCHECKS not be sufficient (I've added
these, so you can see what you think)?  (We do have a bunch of single use things
already).  If you feel strongly, I can change it.

https://codereview.chromium.org/1092953004/diff/20001/chrome/browser/extensio...
File
chrome/browser/extensions/api/developer_private/extension_info_generator_unittest.cc
(right):

https://codereview.chromium.org/1092953004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/extension_info_generator_unittest.cc:77:
linked_ptr<developer::ExtensionInfo> result = last_info_;
On 2015/04/22 21:38:37, kalman wrote:
> Rather than making last_info_ a member variable you can keep everything local
by
> passing a linked_ptr* to OnInfosGenerated. Assign list[0] to it. It would look
> like:
> 
> linked_ptr<developer::ExtensionInfo> result;
> generator->CreateExtensionInfo(..., base::Bind(..., &result));
> return result;

I like it. Done.

https://codereview.chromium.org/1092953004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/extension_info_generator_unittest.cc:103:
linked_ptr<developer::ExtensionInfo> CreateExtensionInfoFromPath(
On 2015/04/22 21:38:37, kalman wrote:
> Why did you need to change this from scoped_ptr to linked_ptr?

Originally thought because we couldn't be sure that it would be the only
linked_ptr reference (and if it isn't, everything explodes) - but looking
closer, I think we can make it work.  Done.

[not at google - send to devlin, 2015-04-23 22:25:21]

lgtm

https://codereview.chromium.org/1092953004/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/developer_private/extension_info_generator.cc
(right):

https://codereview.chromium.org/1092953004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/extension_info_generator.cc:443:
gfx::Size(128, 128),
On 2015/04/23 19:17:21, Devlin wrote:
> On 2015/04/22 21:38:37, kalman wrote:
> > Where does 128/128 come from?
> 
> Added a comment.

I presume you meant to use max_size here.

https://codereview.chromium.org/1092953004/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/developer_private/extension_info_generator.h
(right):

https://codereview.chromium.org/1092953004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/extension_info_generator.h:91:
ExtensionInfosCallback callback_;
On 2015/04/23 19:17:21, Devlin wrote:
> On 2015/04/22 21:38:37, kalman wrote:
> > |callback_| and |list_| mean that there can be only one outgoing call to
> > CreateExtension[s]Info. The easiest way for callers to think about that is
> that
> > ExtensionInfoGenerator is therefore single-use; construct one each time a
call
> > needs to be made.
> > 
> > However, that then kills your default icon caching. However, callers don't
> > actually cache them anyway so it's no worse.
> > 
> > Here are some options:
> > - Hide the ability to construct an ExtensionInfoGenerator, and instead make
> its
> > constructor private with CreateExtension[s]Info static and take an extra
> > BrowserContext variable. Typesafe way of making it single-use.
> > - Pass around |list_| and |callback_| through your callback chains.
> > - Cache the default icons globally rather than per ExtensionInfoGenerator
(not
> > mutually exclusive to other options).
> 
> Yeah, always meant this to be a one-time-use thing (passing around list_ and
> callback_ gets pretty crazy since these are all asynchronous in multiple
> places).

Doesn't look like it'd be that bad to me, you could bundle the 2 arguments into
a struct if it really bothers you.

> The disadvantage to doing a static method is that early-aborting if
> our owner goes away isn't an option,

Good point.

> and it makes for a slightly less pretty
> interface, IMO.

I agree; which is why passing around the arguments is my preferred way of doing
it.

> Would a comment and a few DCHECKS not be sufficient (I've added
> these, so you can see what you think)?  (We do have a bunch of single use
things
> already).  If you feel strongly, I can change it.

Much prefer type safety over comments + DCHECK, but to put things into
perspective, we *are* just talking about the developerPrivate API here. If I've
made you feel guilty enough to change it then great; if not, let's save some
time and get this submitted.

[Devlin, 2015-04-27 17:06:30]

https://codereview.chromium.org/1092953004/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/developer_private/extension_info_generator.cc
(right):

https://codereview.chromium.org/1092953004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/extension_info_generator.cc:443:
gfx::Size(128, 128),
On 2015/04/23 22:25:21, kalman wrote:
> On 2015/04/23 19:17:21, Devlin wrote:
> > On 2015/04/22 21:38:37, kalman wrote:
> > > Where does 128/128 come from?
> > 
> > Added a comment.
> 
> I presume you meant to use max_size here.

Done.

https://codereview.chromium.org/1092953004/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/developer_private/extension_info_generator.h
(right):

https://codereview.chromium.org/1092953004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/extension_info_generator.h:91:
ExtensionInfosCallback callback_;
On 2015/04/23 22:25:21, kalman wrote:
> I agree; which is why passing around the arguments is my preferred way of
doing
> it.

But it's not easy to pass them around, even in a struct - we need to *store* the
list and the callback somewhere, and can't just bind them in, because there are
multiple calls to create the extension info (but only one list/callback).  We
could, I suppose, store collections of lists/callbacks on this object, and curry
in pointers to them, and then delete them when they are done - but that seems
like a recipe for pain and segfaults.

Given that this is just the dev private API, I'm inclined to do the easy way,
and if we ever find that we can't stand it, we'll change it then. :)

[Devlin, 2015-04-27 17:06:33]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2015-04-27 17:06:33]

The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1092953004/#ps80001
(title: "Latest master")

[commit-bot: I haz the power, 2015-04-27 17:07:00]

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

[commit-bot: I haz the power, 2015-04-27 19:08:40]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-04-27 19:09:41]

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