Discovery API supports specifying a tile in a URL.

Discovery API supports specifying a tile in a URL.

BUG=none
TEST=none


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

[beaudoin, 2012-07-25 19:40:16]

Hi Evan,

This patch makes it possible to specify arbitrary imagew as tiles in the NTP
"suggested page", which is under a flag. This work is needed for the upcoming
experimental discovery API.

The current approach allows an arbitrary image URL, specified by an extension
via the discovery API, to be used as the background-image property of the tile
div. We plan on implementing local image caching at some point.

Cheers,

   Philippe

[Evan Stade, 2012-07-26 04:26:01]

lgtm with nits. However I don't know why I am reviewing this. Seems like
something for the extensions team.

http://codereview.chromium.org/10647003/diff/2002/chrome/browser/extensions/a...
File chrome/browser/extensions/api/discovery/discovery_api.cc (right):

http://codereview.chromium.org/10647003/diff/2002/chrome/browser/extensions/a...
chrome/browser/extensions/api/discovery/discovery_api.cc:38: const std::string*
url_image(&empty);
think it's easier to read if you don't use () constructor for PODs.

http://codereview.chromium.org/10647003/diff/2002/chrome/browser/extensions/a...
chrome/browser/extensions/api/discovery/discovery_api.cc:39: if
(params->details.url_image != NULL) {
no curlies

http://codereview.chromium.org/10647003/diff/2002/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/suggestions_page.js (right):

http://codereview.chromium.org/10647003/diff/2002/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/suggestions_page.js:113: if (data.urlImage) {
use ternary operator

http://codereview.chromium.org/10647003/diff/2002/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/suggestions_source_discovery.cc (right):

http://codereview.chromium.org/10647003/diff/2002/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/suggestions_source_discovery.cc:80: if
(url_image.length() > 0)
I don't think there's any need for this check.

[beaudoin, 2012-07-26 17:08:37]

Thanks for the review Evan. I wanted you mostly to look at the NTP change (the
.js) I'm owner on the rest and it was a simple enough change so I opted to
bother only one person. :)

http://codereview.chromium.org/10647003/diff/2002/chrome/browser/extensions/a...
File chrome/browser/extensions/api/discovery/discovery_api.cc (right):

http://codereview.chromium.org/10647003/diff/2002/chrome/browser/extensions/a...
chrome/browser/extensions/api/discovery/discovery_api.cc:38: const std::string*
url_image(&empty);
On 2012/07/26 04:26:02, Evan Stade wrote:
> think it's easier to read if you don't use () constructor for PODs.

Done.

http://codereview.chromium.org/10647003/diff/2002/chrome/browser/extensions/a...
chrome/browser/extensions/api/discovery/discovery_api.cc:39: if
(params->details.url_image != NULL) {
On 2012/07/26 04:26:02, Evan Stade wrote:
> no curlies

Done.

http://codereview.chromium.org/10647003/diff/2002/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/suggestions_page.js (right):

http://codereview.chromium.org/10647003/diff/2002/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/suggestions_page.js:113: if (data.urlImage) {
On 2012/07/26 04:26:02, Evan Stade wrote:
> use ternary operator

Done.

http://codereview.chromium.org/10647003/diff/2002/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/suggestions_source_discovery.cc (right):

http://codereview.chromium.org/10647003/diff/2002/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/suggestions_source_discovery.cc:80: if
(url_image.length() > 0)
On 2012/07/26 04:26:02, Evan Stade wrote:
> I don't think there's any need for this check.

It is needed because url_image is an optional parameter in the API. If it's
missing from the page_value dictionary we want to use the Chrome-provided
thumbnail. It makes it possible for an extension to suggest an item from
Chrome's history without having to provide an image for it.

[commit-bot: I haz the power, 2012-07-26 17:09:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaudoin@chromium.org/10647003/4004

[commit-bot: I haz the power, 2012-07-26 17:09:14]

Presubmit check for 10647003-4004 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/common/extensions/api
    chrome/common/extensions

Presubmit checks took 1.3s to calculate.

[beaudoin, 2012-07-26 17:41:59]

Oh well... I missed I touched stuff in chrome/common/extensions.

Aaron? :)

[Evan Stade, 2012-07-26 21:36:06]

On 2012/07/26 17:08:37, beaudoin wrote:
> Thanks for the review Evan. I wanted you mostly to look at the NTP change (the
> .js) I'm owner on the rest and it was a simple enough change so I opted to
> bother only one person. :)

Just because you are an owner doesn't mean the code doesn't need review. If I'm
only looking at the NTP change, who is reviewing the rest of it?

[beaudoin1, 2012-07-26 21:39:01]

I expressed myself badly. I meant, I was hoping you'd do the entire review
because I needed your approval on the .js.



On Thu, Jul 26, 2012 at 5:36 PM, <estade@chromium.org> wrote:

> On 2012/07/26 17:08:37, beaudoin wrote:
>
>> Thanks for the review Evan. I wanted you mostly to look at the NTP change
>> (the
>> .js) I'm owner on the rest and it was a simple enough change so I opted to
>> bother only one person. :)
>>
>
> Just because you are an owner doesn't mean the code doesn't need review.
> If I'm
> only looking at the NTP change, who is reviewing the rest of it?
>
>
http://codereview.chromium.**org/10647003/<http://codereview.chromium.org/106...
>

[Aaron Boodman, 2012-07-30 11:49:39]

lgtm

[commit-bot: I haz the power, 2012-07-31 17:23:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaudoin@chromium.org/10647003/4004

[commit-bot: I haz the power, 2012-07-31 19:40:12]

Change committed as 149243