[AiS] Add prefetching for Answers images.

[AiS] Add prefetching for Answers images.

BUG=380916
R=pkasting@chromium.org
TBR=sky@chromium.org

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

[groby-ooo-7-16, 2014-06-05 03:42:49]

This starts fetching images as soon as the search provider as received an
AnswersSuggestion. The client's later RequestImage calls will attach to the
in-flight request.

[groby-ooo-7-16, 2014-06-05 03:43:03]

And I meant to say, please take a look :)

[Peter Kasting, 2014-06-05 23:42:47]

Can this be unittested?

https://codereview.chromium.org/314013003/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/base_search_provider.cc (right):

https://codereview.chromium.org/314013003/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/base_search_provider.cc:949: // TODO(groby): Do we
want prefetch in incognito?
We don't currently send suggest requests in incognito, so I wouldn't think you
could reach here then.

https://codereview.chromium.org/314013003/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/base_search_provider.cc:967: // overly harsh as
response.
This comment doesn't seem necessary -- clearly we have to validate any
server-provided data and never crash, so it doesn't seem important to try and
distinguish this sort of malformation from the cases we handle above.

https://codereview.chromium.org/314013003/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/base_search_provider.cc:970: LOG(ERROR) <<
"Prefetching image for " << imageUrl;
I don't think you should check this line in.

https://codereview.chromium.org/314013003/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/base_search_provider.cc:971:
image_service->Prefetch(GURL(imageUrl));
What if GURL(imageUrl) is invalid?  This can happen for more than just an empty
|imageUrl|.  Either Prefetch() needs to validate its input -- in which case you
don't need the empty() check above, since that will handle that case -- or you
need to validity-check the GURL here (and probably DCHECK that it's valid in
Prefetch()), which will again obviate the need for the empty() check above.

[groby-ooo-7-16, 2014-06-14 00:22:08]

PTAL

No unittest yet - we don't have any setup for base_search_provider. Will play
around with it over weekend.

https://codereview.chromium.org/314013003/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/base_search_provider.cc (right):

https://codereview.chromium.org/314013003/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/base_search_provider.cc:949: // TODO(groby): Do we
want prefetch in incognito?
On 2014/06/05 23:42:47, Peter Kasting wrote:
> We don't currently send suggest requests in incognito, so I wouldn't think you
> could reach here then.

Done.

https://codereview.chromium.org/314013003/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/base_search_provider.cc:967: // overly harsh as
response.
On 2014/06/05 23:42:47, Peter Kasting wrote:
> This comment doesn't seem necessary -- clearly we have to validate any
> server-provided data and never crash, so it doesn't seem important to try and
> distinguish this sort of malformation from the cases we handle above.

Removed.

https://codereview.chromium.org/314013003/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/base_search_provider.cc:970: LOG(ERROR) <<
"Prefetching image for " << imageUrl;
On 2014/06/05 23:42:47, Peter Kasting wrote:
> I don't think you should check this line in.

Done.

https://codereview.chromium.org/314013003/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/base_search_provider.cc:971:
image_service->Prefetch(GURL(imageUrl));
On 2014/06/05 23:42:47, Peter Kasting wrote:
> What if GURL(imageUrl) is invalid?  This can happen for more than just an
empty
> |imageUrl|.  Either Prefetch() needs to validate its input -- in which case
you
> don't need the empty() check above, since that will handle that case -- or you
> need to validity-check the GURL here (and probably DCHECK that it's valid in
> Prefetch()), which will again obviate the need for the empty() check above.

Done.

[Peter Kasting, 2014-06-14 01:00:51]

LGTM

https://codereview.chromium.org/314013003/diff/20001/chrome/browser/bitmap_fe...
File chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc (right):

https://codereview.chromium.org/314013003/diff/20001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc:120:
(void)EnsureFetcherForUrl(url);
Nit: While here, please fix this; we disallow C-style casts.  If removing this
causes some sort of warning that we're ignoring the result of something marked
with WARN_UNUSED_RESULT, use the ignore_result() method from base/macros.h.

[groby-ooo-7-16, 2014-06-17 21:06:35]

Waiting for previous CLs to land. Please ignore the changes to
.gypi/bitmap_fetcher.h, something w/ upstream/rebase is broken. Will fill next
rebase round

https://codereview.chromium.org/314013003/diff/20001/chrome/browser/bitmap_fe...
File chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc (right):

https://codereview.chromium.org/314013003/diff/20001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc:120:
(void)EnsureFetcherForUrl(url);
On 2014/06/14 01:00:51, Peter Kasting wrote:
> Nit: While here, please fix this; we disallow C-style casts.  If removing this
> causes some sort of warning that we're ignoring the result of something marked
> with WARN_UNUSED_RESULT, use the ignore_result() method from base/macros.h.

Fixed in base-cl. Also, inverted logic to kill early-out return.

[groby-ooo-7-16, 2014-06-19 14:30:55]

The CQ bit was checked by groby@chromium.org

[commit-bot: I haz the power, 2014-06-19 14:32:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/groby@chromium.org/314013003/60001

[commit-bot: I haz the power, 2014-06-19 15:32:31]

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

[commit-bot: I haz the power, 2014-06-19 15:32:32]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/18957)

[groby-ooo-7-16, 2014-06-20 01:46:21]

sky: TBR'd you for two-line change.

[groby-ooo-7-16, 2014-06-20 01:47:22]

Again, with proper reviewers:
sky: TBR'd for two-line change

[groby-ooo-7-16, 2014-06-20 01:47:31]

The CQ bit was checked by groby@chromium.org

[commit-bot: I haz the power, 2014-06-20 01:48:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/groby@chromium.org/314013003/80001

[commit-bot: I haz the power, 2014-06-20 09:26:17]

Change committed as 278653

[sky, 2014-06-20 15:13:56]

LGTM

https://codereview.chromium.org/314013003/diff/80001/chrome/browser/bitmap_fe...
File chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc (right):

https://codereview.chromium.org/314013003/diff/80001/chrome/browser/bitmap_fe...
chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc:117: if (url.is_valid())
If you're going to check the url here, shouldn't you also check it in
RequestImage?

[groby-ooo-7-16, 2014-06-20 21:34:38]

On 2014/06/20 15:13:56, sky wrote:
> LGTM
> 
>
https://codereview.chromium.org/314013003/diff/80001/chrome/browser/bitmap_fe...
> File chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc (right):
> 
>
https://codereview.chromium.org/314013003/diff/80001/chrome/browser/bitmap_fe...
> chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc:117: if
(url.is_valid())
> If you're going to check the url here, shouldn't you also check it in
> RequestImage?

Follow-up CL in the works.