Add support to download web store promo logos over https.

Add support to download web store promo logos over https.

This lets you define apps promo logos as https URLs in the promo feed. The AppsPromoLogoDownloader only downloads the image over https and from *.google.com domains. AppsPromo caches the image until the URL changes in the promo feed.

BUG=84506
TEST=*Promo*

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

[jstritar, 2011-08-31 20:41:28]

mirandac: Could you take a look at the PromoResourceService changes?
mihaip: Could you review the rest? I thought you could be good for this part
since you just worked with URLFetcher.

[Mihai Parparita -not on Chrome, 2011-08-31 23:26:23]

http://codereview.chromium.org/7820003/diff/1001/chrome/browser/extensions/ap...
File chrome/browser/extensions/apps_promo.h (right):

http://codereview.chromium.org/7820003/diff/1001/chrome/browser/extensions/ap...
chrome/browser/extensions/apps_promo.h:53: std::string expire;
A comment pointing to PromoResourceService::UnpackWebStoreSignal for
descriptions of these fields (especially expire and user_group) might be
helpful.

http://codereview.chromium.org/7820003/diff/1001/chrome/browser/extensions/ap...
chrome/browser/extensions/apps_promo.h:79: // Gets the original URL of the logo.
This should only be set when the logo
"Source URL" might be a better name than "original URL".

http://codereview.chromium.org/7820003/diff/1001/chrome/browser/extensions/ap...
chrome/browser/extensions/apps_promo.h:80: // was served over https.
Nit: capitalize HTTPS (applies to other parts of the CL too).

http://codereview.chromium.org/7820003/diff/1001/chrome/browser/extensions/ap...
chrome/browser/extensions/apps_promo.h:81: static GURL GetPromoLogoOriginal();
Get/SetOriginalPromoLogoUrl reads a bit better.

http://codereview.chromium.org/7820003/diff/1001/chrome/browser/extensions/ap...
chrome/browser/extensions/apps_promo.h:148: // cache the image as long as
possible.
Not seeing anything that would make us "cache the image as long as possible". Or
do you mean just the URL change check?

http://codereview.chromium.org/7820003/diff/1001/chrome/browser/web_resource/...
File chrome/browser/web_resource/promo_resource_service.cc (right):

http://codereview.chromium.org/7820003/diff/1001/chrome/browser/web_resource/...
chrome/browser/web_resource/promo_resource_service.cc:375: // The downloader
will set the promo prefs when complete, even if it
To be clearer, this should say "set the promo prefs and send the
NOTIFICATION_WEB_STORE_PROMO_LOADED notification".

http://codereview.chromium.org/7820003/diff/1001/content/test/test_url_fetche...
File content/test/test_url_fetcher_factory.cc (right):

http://codereview.chromium.org/7820003/diff/1001/content/test/test_url_fetche...
content/test/test_url_fetcher_factory.cc:141: // These methods are overriden so
we can use the version of
Add OVERRIDE to the methods that you're overriding.

[Miranda Callahan, 2011-09-01 07:43:38]

PromoResourceService changes LGTM with nit. I like the struct for apps promo
data; we should do that with the other promos as well.

http://codereview.chromium.org/7820003/diff/1001/chrome/browser/extensions/ap...
File chrome/browser/extensions/apps_promo.cc (right):

http://codereview.chromium.org/7820003/diff/1001/chrome/browser/extensions/ap...
chrome/browser/extensions/apps_promo.cc:30: // The default promo data (this is
only used with --force-apps-promo-visible).
nit: could you just make it "this is only used for testing with...", just to
make sure that future code readers understand exactly why these lines aren't
localized?

[jstritar, 2011-09-01 16:10:13]

http://codereview.chromium.org/7820003/diff/1001/chrome/browser/extensions/ap...
File chrome/browser/extensions/apps_promo.cc (right):

http://codereview.chromium.org/7820003/diff/1001/chrome/browser/extensions/ap...
chrome/browser/extensions/apps_promo.cc:30: // The default promo data (this is
only used with --force-apps-promo-visible).
On 2011/09/01 07:43:38, Miranda Callahan wrote:
> nit: could you just make it "this is only used for testing with...", just to
> make sure that future code readers understand exactly why these lines aren't
> localized?

Done.

http://codereview.chromium.org/7820003/diff/1001/chrome/browser/extensions/ap...
File chrome/browser/extensions/apps_promo.h (right):

http://codereview.chromium.org/7820003/diff/1001/chrome/browser/extensions/ap...
chrome/browser/extensions/apps_promo.h:53: std::string expire;
On 2011/08/31 23:26:23, Mihai Parparita wrote:
> A comment pointing to PromoResourceService::UnpackWebStoreSignal for
> descriptions of these fields (especially expire and user_group) might be
> helpful.

Done.

http://codereview.chromium.org/7820003/diff/1001/chrome/browser/extensions/ap...
chrome/browser/extensions/apps_promo.h:79: // Gets the original URL of the logo.
This should only be set when the logo
On 2011/08/31 23:26:23, Mihai Parparita wrote:
> "Source URL" might be a better name than "original URL".

Done.

http://codereview.chromium.org/7820003/diff/1001/chrome/browser/extensions/ap...
chrome/browser/extensions/apps_promo.h:80: // was served over https.
On 2011/08/31 23:26:23, Mihai Parparita wrote:
> Nit: capitalize HTTPS (applies to other parts of the CL too).

Done.

http://codereview.chromium.org/7820003/diff/1001/chrome/browser/extensions/ap...
chrome/browser/extensions/apps_promo.h:81: static GURL GetPromoLogoOriginal();
On 2011/08/31 23:26:23, Mihai Parparita wrote:
> Get/SetOriginalPromoLogoUrl reads a bit better.

Done. Updated to Get/SetSourcePromoLogoURL.

http://codereview.chromium.org/7820003/diff/1001/chrome/browser/extensions/ap...
chrome/browser/extensions/apps_promo.h:148: // cache the image as long as
possible.
On 2011/08/31 23:26:23, Mihai Parparita wrote:
> Not seeing anything that would make us "cache the image as long as possible".
Or
> do you mean just the URL change check?

Hehe, yeah I guess that isn't quite accurate. I updated the comment to read
"cache the image until its source URL changes."

http://codereview.chromium.org/7820003/diff/1001/chrome/browser/web_resource/...
File chrome/browser/web_resource/promo_resource_service.cc (right):

http://codereview.chromium.org/7820003/diff/1001/chrome/browser/web_resource/...
chrome/browser/web_resource/promo_resource_service.cc:375: // The downloader
will set the promo prefs when complete, even if it
On 2011/08/31 23:26:23, Mihai Parparita wrote:
> To be clearer, this should say "set the promo prefs and send the
> NOTIFICATION_WEB_STORE_PROMO_LOADED notification".

Done.

http://codereview.chromium.org/7820003/diff/1001/content/test/test_url_fetche...
File content/test/test_url_fetcher_factory.cc (right):

http://codereview.chromium.org/7820003/diff/1001/content/test/test_url_fetche...
content/test/test_url_fetcher_factory.cc:141: // These methods are overriden so
we can use the version of
On 2011/08/31 23:26:23, Mihai Parparita wrote:
> Add OVERRIDE to the methods that you're overriding.

Done.

[Mihai Parparita -not on Chrome, 2011-09-02 03:02:21]

http://codereview.chromium.org/7820003/diff/10011/chrome/browser/extensions/a...
File chrome/browser/extensions/apps_promo.cc (right):

http://codereview.chromium.org/7820003/diff/10011/chrome/browser/extensions/a...
chrome/browser/extensions/apps_promo.cc:340:
AppsPromo::SetSourcePromoLogoURL(GURL());
Given that you're resetting the source URL if the logo doesn't need to be
fetched, doesn't this mean that you'll at most reuse the same logo twice (since
source URL will ping-pong between the empty URL and the source URL, so the
equality check at line 397 won't hold true)?

[jstritar, 2011-09-02 20:17:05]

http://codereview.chromium.org/7820003/diff/10011/chrome/browser/extensions/a...
File chrome/browser/extensions/apps_promo.cc (right):

http://codereview.chromium.org/7820003/diff/10011/chrome/browser/extensions/a...
chrome/browser/extensions/apps_promo.cc:340:
AppsPromo::SetSourcePromoLogoURL(GURL());
On 2011/09/02 03:02:21, Mihai Parparita wrote:
> Given that you're resetting the source URL if the logo doesn't need to be
> fetched, doesn't this mean that you'll at most reuse the same logo twice
(since
> source URL will ping-pong between the empty URL and the source URL, so the
> equality check at line 397 won't hold true)?

Yes, thanks for finding that! I fixed the problem and made sure
UnpackWebStoreSignalHttpsLogo tests this. I also renamed AppsPromoLogoDownloader
to AppsPromoLogoFetcher to be more consistent.

[Mihai Parparita -not on Chrome, 2011-09-02 20:46:51]

LGTM

[commit-bot: I haz the power, 2011-09-08 03:06:05]

Change committed as 100089