[Contextual Search] Fetch and display thumbnails returned in resolution response

[Contextual Search] Fetch and display thumbnails returned in resolution response

The search resolution response will potentially include thumbnail urls.
This patch adds support for fetching the thumbnail, using BitmapFetcher,
and displaying it whre the 'G' logo is currently displayed.

Also cleans up unused an unused field trial param for disabling sprite
animations.

Note that the work to parse the JSON and extract a thumbnail url has
not been completed yet. This CL adds the plumbing and UI changes
necessary to display once the parsing is implemented.

BUG=644932
Committed: https://crrev.com/c56e3d6b9b0d2559a0f33821b20ea75c8a824963
Cr-Commit-Position: refs/heads/master@{#417613}

[Theresa, 2016-09-07 23:11:37]

The CQ bit was checked by twellington@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 23:12:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Theresa, 2016-09-07 23:18:40]

The CQ bit was unchecked by twellington@chromium.org

[Theresa, 2016-09-07 23:55:07]

The CQ bit was checked by twellington@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 23:55:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Theresa, 2016-09-07 23:57:09]

twellington@chromium.org changed reviewers:
+ donnd@chromium.org, mdjones@chromium.org

[Theresa, 2016-09-07 23:57:10]

mdjones@ - ptal at cc layer things in contextual_search_layer.cc (let me know if
I should add someone else)

donnd@ - ptal at everything

[commit-bot: I haz the power, 2016-09-07 23:58:54]

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

[commit-bot: I haz the power, 2016-09-07 23:58:55]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Theresa, 2016-09-08 01:13:01]

The CQ bit was checked by twellington@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 01:13:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 02:04:18]

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

[commit-bot: I haz the power, 2016-09-08 02:04:19]

Dry run: This issue passed the CQ dry run.

[mdjones, 2016-09-08 17:14:21]

https://codereview.chromium.org/2322793002/diff/40001/chrome/browser/android/...
File
chrome/browser/android/compositor/scene_layer/contextual_search_scene_layer.cc
(right):

https://codereview.chromium.org/2322793002/diff/40001/chrome/browser/android/...
chrome/browser/android/compositor/scene_layer/contextual_search_scene_layer.cc:208:
chrome::BitmapFetcher* fetcher = new chrome::BitmapFetcher(*gurl, this);
Does BitmapFetcher delete itself when it is done?

[Donn Denman, 2016-09-08 18:13:40]

My only real question is whether we really want to crop as well as scale.  I'd
think that just scaling might work better (assuming with letterbox).

Otherwise mostly nits and nit-suggestions.

https://codereview.chromium.org/2322793002/diff/40001/chrome/browser/android/...
File chrome/browser/android/compositor/layer/contextual_search_layer.cc (right):

https://codereview.chromium.org/2322793002/diff/40001/chrome/browser/android/...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:41:
scoped_refptr<cc::Layer> ContextualSearchLayer::GetIconLayer() {
nit: I think this section could benefit from some refactoring.  There's some
code duplication removing the sprite layer, there are returns in multiple places
(which seems error-prone), and it's hard to reason about what's happening.  

Is there a way to restructure this to say:
  layer present = sprite or thumbnail;
  layer desired = sprite or thumbnail;
  if (present) remove;
  add desired;
  return desired;

Just a suggestion, adding a TODO is fine too!

https://codereview.chromium.org/2322793002/diff/40001/chrome/browser/android/...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:480: //
Determine the scaled thumbnail width and height. If both the height and
Don't we need scaling if *either* height or width is beyond thumbnail_size?

https://codereview.chromium.org/2322793002/diff/40001/chrome/browser/android/...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:487:
scaled_thumbnail_width =
nit: just use *= here, and below?  Now that I think about it, since this is
integer math you may get a different outcome due to different ordering of the
calculations.  So I think good choices include computing a float for
thumbnail_size_ / min_dimension, OR just use scaled_thumbnail_width instead of
thumbnail->width() here.

https://codereview.chromium.org/2322793002/diff/40001/chrome/browser/android/...
File
chrome/browser/android/compositor/scene_layer/contextual_search_scene_layer.cc
(right):

https://codereview.chromium.org/2322793002/diff/40001/chrome/browser/android/...
chrome/browser/android/compositor/scene_layer/contextual_search_scene_layer.cc:224:
contextual_search_layer_->SetThumbnail(bitmap);
Should this line be conditional on success?

https://codereview.chromium.org/2322793002/diff/40001/chrome/browser/android/...
File
chrome/browser/android/compositor/scene_layer/contextual_search_scene_layer.h
(right):

https://codereview.chromium.org/2322793002/diff/40001/chrome/browser/android/...
chrome/browser/android/compositor/scene_layer/contextual_search_scene_layer.h:120:
void FetchThumbnail(jobject j_profile);
nit: move up to top of private?  IIRC we normally declare methods before data.

https://codereview.chromium.org/2322793002/diff/40001/chrome/browser/android/...
File chrome/browser/android/contextualsearch/contextual_search_delegate.cc
(right):

https://codereview.chromium.org/2322793002/diff/40001/chrome/browser/android/...
chrome/browser/android/contextualsearch/contextual_search_delegate.cc:489: //
TODO(twellington): extract thumbnail_url. Also extract caption and pipe
Feel free to change to donnd.

[Donn Denman, 2016-09-08 18:21:44]

I didn't really look at the details of the scale-download logic, hopefully it
can be shared/reused somehow.

https://codereview.chromium.org/2322793002/diff/40001/chrome/browser/android/...
File chrome/browser/android/compositor/layer/contextual_search_layer.cc (right):

https://codereview.chromium.org/2322793002/diff/40001/chrome/browser/android/...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:479: void
ContextualSearchLayer::SetThumbnail(const SkBitmap* thumbnail) {
Can this scale-and-crop logic be shared with similar code in the download code?

[Theresa, 2016-09-08 20:18:23]

https://codereview.chromium.org/2322793002/diff/40001/chrome/browser/android/...
File chrome/browser/android/compositor/layer/contextual_search_layer.cc (right):

https://codereview.chromium.org/2322793002/diff/40001/chrome/browser/android/...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:41:
scoped_refptr<cc::Layer> ContextualSearchLayer::GetIconLayer() {
On 2016/09/08 18:13:39, Donn Denman wrote:
> nit: I think this section could benefit from some refactoring.  There's some
> code duplication removing the sprite layer, there are returns in multiple
places
> (which seems error-prone), and it's hard to reason about what's happening.  
> 
> Is there a way to restructure this to say:
>   layer present = sprite or thumbnail;
>   layer desired = sprite or thumbnail;
>   if (present) remove;
>   add desired;
>   return desired;
> 
> Just a suggestion, adding a TODO is fine too!

I got rid off the duplicate code removing the sprite layer and multiple returns.
Does it look better?

https://codereview.chromium.org/2322793002/diff/40001/chrome/browser/android/...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:479: void
ContextualSearchLayer::SetThumbnail(const SkBitmap* thumbnail) {
On 2016/09/08 18:21:44, Donn Denman wrote:
> Can this scale-and-crop logic be shared with similar code in the download
code?

Unfortunately not. Downloads displays its thumbnails using Java views, not CC
layers. The few lines that determine the min dimension and scaled width/height
are the same, but I don't think it make sense to pull out ~4 lines of code into
a helper method.

https://codereview.chromium.org/2322793002/diff/40001/chrome/browser/android/...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:480: //
Determine the scaled thumbnail width and height. If both the height and
On 2016/09/08 18:13:39, Donn Denman wrote:
> Don't we need scaling if *either* height or width is beyond thumbnail_size?

No, only if they're both greater than thumbnail_size. This scaling is to make
the minimum side (height or width) equal to thumbnail_size. If one of them is
already less than or equal to thumbnail size, we don't scale, only crop.

Example: We get a long horizontal image, the height is < thumbnail size. We
won't scale, but we will crop the edges off. This will result in some white
space on the top and bottom of the image. We *should* get back images that are
large enough that this doesn't happen, but I want the code to handle it
correctly anyway.

https://codereview.chromium.org/2322793002/diff/40001/chrome/browser/android/...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:487:
scaled_thumbnail_width =
On 2016/09/08 18:13:39, Donn Denman wrote:
> nit: just use *= here, and below?  Now that I think about it, since this is
> integer math you may get a different outcome due to different ordering of the
> calculations.  So I think good choices include computing a float for
> thumbnail_size_ / min_dimension, OR just use scaled_thumbnail_width instead of
> thumbnail->width() here. 

Done.

https://codereview.chromium.org/2322793002/diff/40001/chrome/browser/android/...
File
chrome/browser/android/compositor/scene_layer/contextual_search_scene_layer.cc
(right):

https://codereview.chromium.org/2322793002/diff/40001/chrome/browser/android/...
chrome/browser/android/compositor/scene_layer/contextual_search_scene_layer.cc:208:
chrome::BitmapFetcher* fetcher = new chrome::BitmapFetcher(*gurl, this);
On 2016/09/08 17:14:21, mdjones wrote:
> Does BitmapFetcher delete itself when it is done?

I don't think so. Add an explicit call to the destructor below.

https://codereview.chromium.org/2322793002/diff/40001/chrome/browser/android/...
chrome/browser/android/compositor/scene_layer/contextual_search_scene_layer.cc:224:
contextual_search_layer_->SetThumbnail(bitmap);
On 2016/09/08 18:13:39, Donn Denman wrote:
> Should this line be conditional on success?

Yes, done.

https://codereview.chromium.org/2322793002/diff/40001/chrome/browser/android/...
File
chrome/browser/android/compositor/scene_layer/contextual_search_scene_layer.h
(right):

https://codereview.chromium.org/2322793002/diff/40001/chrome/browser/android/...
chrome/browser/android/compositor/scene_layer/contextual_search_scene_layer.h:120:
void FetchThumbnail(jobject j_profile);
On 2016/09/08 18:13:39, Donn Denman wrote:
> nit: move up to top of private?  IIRC we normally declare methods before data.

Done.

https://codereview.chromium.org/2322793002/diff/40001/chrome/browser/android/...
File chrome/browser/android/contextualsearch/contextual_search_delegate.cc
(right):

https://codereview.chromium.org/2322793002/diff/40001/chrome/browser/android/...
chrome/browser/android/contextualsearch/contextual_search_delegate.cc:489: //
TODO(twellington): extract thumbnail_url. Also extract caption and pipe
On 2016/09/08 18:13:39, Donn Denman wrote:
> Feel free to change to donnd.

Done :)

[Theresa, 2016-09-08 20:18:28]

The CQ bit was checked by twellington@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 20:19:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 20:21:00]

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

[commit-bot: I haz the power, 2016-09-08 20:21:01]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Donn Denman, 2016-09-08 20:41:52]

LGTM!

https://codereview.chromium.org/2322793002/diff/40001/chrome/browser/android/...
File chrome/browser/android/compositor/layer/contextual_search_layer.cc (right):

https://codereview.chromium.org/2322793002/diff/40001/chrome/browser/android/...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:41:
scoped_refptr<cc::Layer> ContextualSearchLayer::GetIconLayer() {
On 2016/09/08 20:18:22, Theresa Wellington wrote:
> On 2016/09/08 18:13:39, Donn Denman wrote:
> > nit: I think this section could benefit from some refactoring.  There's some
> > code duplication removing the sprite layer, there are returns in multiple
> places
> > (which seems error-prone), and it's hard to reason about what's happening.  
> > 
> > Is there a way to restructure this to say:
> >   layer present = sprite or thumbnail;
> >   layer desired = sprite or thumbnail;
> >   if (present) remove;
> >   add desired;
> >   return desired;
> > 
> > Just a suggestion, adding a TODO is fine too!
> 
> I got rid off the duplicate code removing the sprite layer and multiple
returns.
> Does it look better?

Yes, that's most of what I was looking for, and if we do more then readability
may suffer.  Thanks!

https://codereview.chromium.org/2322793002/diff/40001/chrome/browser/android/...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:479: void
ContextualSearchLayer::SetThumbnail(const SkBitmap* thumbnail) {
On 2016/09/08 20:18:22, Theresa Wellington wrote:
> On 2016/09/08 18:21:44, Donn Denman wrote:
> > Can this scale-and-crop logic be shared with similar code in the download
> code?
> 
> Unfortunately not. Downloads displays its thumbnails using Java views, not CC
> layers. The few lines that determine the min dimension and scaled width/height
> are the same, but I don't think it make sense to pull out ~4 lines of code
into
> a helper method.

Agreed!

https://codereview.chromium.org/2322793002/diff/40001/chrome/browser/android/...
chrome/browser/android/compositor/layer/contextual_search_layer.cc:480: //
Determine the scaled thumbnail width and height. If both the height and
On 2016/09/08 20:18:23, Theresa Wellington wrote:
> On 2016/09/08 18:13:39, Donn Denman wrote:
> > Don't we need scaling if *either* height or width is beyond thumbnail_size?
> 
> No, only if they're both greater than thumbnail_size. This scaling is to make
> the minimum side (height or width) equal to thumbnail_size. If one of them is
> already less than or equal to thumbnail size, we don't scale, only crop.
> 
> Example: We get a long horizontal image, the height is < thumbnail size. We
> won't scale, but we will crop the edges off. This will result in some white
> space on the top and bottom of the image. We *should* get back images that are
> large enough that this doesn't happen, but I want the code to handle it
> correctly anyway.

Acknowledged.  Sorry you wasted some time cuz I didn't remove this comment after
we agreed on scale&crop.

[mdjones, 2016-09-08 21:31:29]

https://codereview.chromium.org/2322793002/diff/80001/chrome/browser/android/...
File
chrome/browser/android/compositor/scene_layer/contextual_search_scene_layer.cc
(right):

https://codereview.chromium.org/2322793002/diff/80001/chrome/browser/android/...
chrome/browser/android/compositor/scene_layer/contextual_search_scene_layer.cc:226:
fetcher_->~BitmapFetcher();
I think this would be "delete fetcher_;". However, instead of having a raw
pointer, it might be worth having a std::unique_ptr<T>. You would call
fetcher_.reset(nullptr) here and in the event that this doesn't get called (for
whatever reason), it will be cleaned up with the object's destruction or when it
is given a new pointer.

[Theresa, 2016-09-08 22:28:39]

The CQ bit was checked by twellington@chromium.org to run a CQ dry run

[Theresa, 2016-09-08 22:28:52]

https://codereview.chromium.org/2322793002/diff/80001/chrome/browser/android/...
File
chrome/browser/android/compositor/scene_layer/contextual_search_scene_layer.cc
(right):

https://codereview.chromium.org/2322793002/diff/80001/chrome/browser/android/...
chrome/browser/android/compositor/scene_layer/contextual_search_scene_layer.cc:226:
fetcher_->~BitmapFetcher();
On 2016/09/08 21:31:28, mdjones wrote:
> I think this would be "delete fetcher_;". However, instead of having a raw
> pointer, it might be worth having a std::unique_ptr<T>. You would call
> fetcher_.reset(nullptr) here and in the event that this doesn't get called
(for
> whatever reason), it will be cleaned up with the object's destruction or when
it
> is given a new pointer.

Done.

I didn't know about std::unique_ptr's -- thanks!

[commit-bot: I haz the power, 2016-09-08 22:29:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chromium-reviews, 2016-09-08 22:43:54]

BTW std::unique_ptr used to be scoped_ptr<> but now we switched to the
std.  Very handy.

On Thu, Sep 8, 2016 at 3:28 PM, <twellington@chromium.org> wrote:

>
> https://codereview.chromium.org/2322793002/diff/80001/
> chrome/browser/android/compositor/scene_layer/
> contextual_search_scene_layer.cc
> File
> chrome/browser/android/compositor/scene_layer/
> contextual_search_scene_layer.cc
> (right):
>
> https://codereview.chromium.org/2322793002/diff/80001/
> chrome/browser/android/compositor/scene_layer/
> contextual_search_scene_layer.cc#newcode226
> chrome/browser/android/compositor/scene_layer/
> contextual_search_scene_layer.cc:226:
> fetcher_->~BitmapFetcher();
> On 2016/09/08 21:31:28, mdjones wrote:
> > I think this would be "delete fetcher_;". However, instead of having a
> raw
> > pointer, it might be worth having a std::unique_ptr<T>. You would call
> > fetcher_.reset(nullptr) here and in the event that this doesn't get
> called (for
> > whatever reason), it will be cleaned up with the object's destruction
> or when it
> > is given a new pointer.
>
> Done.
>
> I didn't know about std::unique_ptr's -- thanks!
>
> https://codereview.chromium.org/2322793002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mdjones, 2016-09-08 23:10:58]

lgtm

[Theresa, 2016-09-08 23:15:53]

The CQ bit was unchecked by twellington@chromium.org

[Theresa, 2016-09-08 23:15:53]

The CQ bit was checked by twellington@chromium.org

[Theresa, 2016-09-08 23:15:54]

The patchset sent to the CQ was uploaded after l-g-t-m from donnd@chromium.org
Link to the patchset: https://codereview.chromium.org/2322793002/#ps100001
(title: "std::unique_ptr")

[commit-bot: I haz the power, 2016-09-08 23:16:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 23:22:55]

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

[commit-bot: I haz the power, 2016-09-08 23:22:56]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Theresa, 2016-09-08 23:24:05]

twellington@chromium.org changed reviewers:
+ tedchoc@chromium.org

[Theresa, 2016-09-08 23:24:05]

+tedchoc for dimens.xml OWNERS

[Ted C, 2016-09-09 17:07:31]

dimens.xml lgtm

[Theresa, 2016-09-09 17:18:23]

The CQ bit was checked by twellington@chromium.org

[commit-bot: I haz the power, 2016-09-09 17:18:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 17:24:08]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-09 17:26:12]

Description was changed from

==========
[Contextual Search] Fetch and display thumbnails returned in resolution response

The search resolution response will potentially include thumbnail urls.
This patch adds support for fetching the thumbnail, using BitmapFetcher,
and displaying it whre the 'G' logo is currently displayed.

Also cleans up unused an unused field trial param for disabling sprite
animations.

Note that the work to parse the JSON and extract a thumbnail url has
not been completed yet. This CL adds the plumbing and UI changes
necessary to display once the parsing is implemented.

BUG=644932
==========

to

==========
[Contextual Search] Fetch and display thumbnails returned in resolution response

The search resolution response will potentially include thumbnail urls.
This patch adds support for fetching the thumbnail, using BitmapFetcher,
and displaying it whre the 'G' logo is currently displayed.

Also cleans up unused an unused field trial param for disabling sprite
animations.

Note that the work to parse the JSON and extract a thumbnail url has
not been completed yet. This CL adds the plumbing and UI changes
necessary to display once the parsing is implemented.

BUG=644932

Committed: https://crrev.com/c56e3d6b9b0d2559a0f33821b20ea75c8a824963
Cr-Commit-Position: refs/heads/master@{#417613}
==========

[commit-bot: I haz the power, 2016-09-09 17:26:13]

Patchset 6 (id:??) landed as
https://crrev.com/c56e3d6b9b0d2559a0f33821b20ea75c8a824963
Cr-Commit-Position: refs/heads/master@{#417613}