[Thumbnails] Specify copy size in Pixels, not DIPs

CopyFromCompositingSurface expects a size in pixels, not DIPs.

This caused an issue where HiDPI devices would request 212x142pixels no matter what.

This changes the code to get the proper scaled dimension for the device (in case of HiDPI, this would be 424x284 if user is on a 2x device, or more accordingly).

BUG=470875
TEST=ContentBased*, SimpleThumb* unit_tests
TBR=jbauman

Committed: https://crrev.com/7898d8d28e5d02efcf161cc68ad8ce9b3d970ee2
Cr-Commit-Position: refs/heads/master@{#322705}

[Mathieu, 2015-03-26 15:16:25]

mathp@chromium.org changed reviewers:
+ thestig@chromium.org

[Mathieu, 2015-03-26 15:16:33]

Patchset #1 (id:1) has been deleted

[Mathieu, 2015-03-26 15:19:11]

Patchset #1 (id:20001) has been deleted

[Mathieu, 2015-03-26 16:05:28]

Patchset #2 (id:60001) has been deleted

[Mathieu, 2015-03-26 16:09:07]

Hi Lei, 

Can you have a look?

[Lei Zhang, 2015-03-26 18:30:33]

thestig@chromium.org changed reviewers:
+ motek@chromium.org

[Lei Zhang, 2015-03-26 18:30:34]

motek@ worked on this code before, so he might be a better reviewer.

[Lei Zhang, 2015-03-26 18:34:55]

thestig@chromium.org changed reviewers:
+ mazda@chromium.org

[Lei Zhang, 2015-03-26 18:34:56]

+mazda as well.

https://codereview.chromium.org/1028393003/diff/80001/chrome/browser/thumbnai...
File chrome/browser/thumbnails/thumbnailing_algorithm.h (right):

https://codereview.chromium.org/1028393003/diff/80001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnailing_algorithm.h:27: // canvas of
|source_size|. Auxiliary |scale_factor| helps compute the target
The comments still refer to target and |target_size| even though you have
changed the variable name.

[motek., 2015-03-26 21:06:03]

Generally seems OK, modulo a comment nit.

https://codereview.chromium.org/1028393003/diff/80001/chrome/browser/thumbnai...
File chrome/browser/thumbnails/simple_thumbnail_crop.cc (left):

https://codereview.chromium.org/1028393003/diff/80001/chrome/browser/thumbnai...
chrome/browser/thumbnails/simple_thumbnail_crop.cc:95: // the copied pixels
should be as small as possible.
You should leave a trace of connection to RenderWidgetHost somewhere here.
Otherwise it is easy to get misguided by that downsample routine and just look
for the wrong thing.

[Mathieu, 2015-03-26 21:19:48]

Thanks all. Have another look.

According to discussion on the bug I also added a 1x to 2x conversion since it
was something we were doing before (taking a 2x screenshot on 1x devices to
improve quality).

https://codereview.chromium.org/1028393003/diff/80001/chrome/browser/thumbnai...
File chrome/browser/thumbnails/simple_thumbnail_crop.cc (left):

https://codereview.chromium.org/1028393003/diff/80001/chrome/browser/thumbnai...
chrome/browser/thumbnails/simple_thumbnail_crop.cc:95: // the copied pixels
should be as small as possible.
On 2015/03/26 21:06:02, motek. wrote:
> You should leave a trace of connection to RenderWidgetHost somewhere here.
> Otherwise it is easy to get misguided by that downsample routine and just look
> for the wrong thing. 

Done.

https://codereview.chromium.org/1028393003/diff/80001/chrome/browser/thumbnai...
File chrome/browser/thumbnails/thumbnailing_algorithm.h (right):

https://codereview.chromium.org/1028393003/diff/80001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnailing_algorithm.h:27: // canvas of
|source_size|. Auxiliary |scale_factor| helps compute the target
On 2015/03/26 18:34:56, Lei Zhang wrote:
> The comments still refer to target and |target_size| even though you have
> changed the variable name.

Done.

[motek., 2015-03-27 16:47:43]

lgtm

[Lei Zhang, 2015-03-27 19:09:17]

rs lgtm

[Mathieu, 2015-03-27 20:32:31]

mathp@chromium.org changed reviewers:
+ jbauman@chromium.org

[Mathieu, 2015-03-27 20:33:10]

Adding a comment to CopyFromCompositingSurface to avoid confusion in the future.
+TBR jbauman for that comment.

[Mathieu, 2015-03-27 20:33:17]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2015-03-27 20:33:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, motek@chromium.org
Link to the patchset: https://codereview.chromium.org/1028393003/#ps140001
(title: "added comment")

[commit-bot: I haz the power, 2015-03-27 20:33:48]

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

[commit-bot: I haz the power, 2015-03-27 20:37:49]

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

[commit-bot: I haz the power, 2015-03-27 20:37:50]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Mathieu, 2015-03-27 20:42:33]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2015-03-27 20:42:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, motek@chromium.org
Link to the patchset: https://codereview.chromium.org/1028393003/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2015-03-27 20:42:53]

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

[commit-bot: I haz the power, 2015-03-27 21:57:33]

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

[commit-bot: I haz the power, 2015-03-27 21:57:34]

Failed to apply patch for content/browser/compositor/delegated_frame_host.h:
While running git apply --index -3 -p1;
  error: patch failed: content/browser/compositor/delegated_frame_host.h:94
  Falling back to three-way merge...
  Applied patch to 'content/browser/compositor/delegated_frame_host.h' with
conflicts.
  U content/browser/compositor/delegated_frame_host.h

Patch:       content/browser/compositor/delegated_frame_host.h
Index: content/browser/compositor/delegated_frame_host.h
diff --git a/content/browser/compositor/delegated_frame_host.h
b/content/browser/compositor/delegated_frame_host.h
index
e6519c602500d4b1919aa28d4321c56105711bb2..385b48817ea05af992accdec16abbde4a95cde3a
100644
--- a/content/browser/compositor/delegated_frame_host.h
+++ b/content/browser/compositor/delegated_frame_host.h
@@ -94,6 +94,8 @@ class CONTENT_EXPORT DelegatedFrameHost
   void SetCompositor(ui::Compositor* compositor);
   void ResetCompositor();
   void SetVSyncParameters(base::TimeTicks timebase, base::TimeDelta interval);
+  // Note: |src_subset| is specified in DIP dimensions while |output_size|
+  // expects pixels.
   void CopyFromCompositingSurface(const gfx::Rect& src_subrect,
                                   const gfx::Size& output_size,
                                   ReadbackRequestCallback& callback,

[commit-bot: I haz the power, 2015-03-27 21:58:17]

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

[Mathieu, 2015-03-28 12:59:23]

Patchset #7 (id:180001) has been deleted

[Mathieu, 2015-03-28 12:59:29]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2015-03-28 12:59:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, motek@chromium.org
Link to the patchset: https://codereview.chromium.org/1028393003/#ps200001
(title: "correct rebase")

[commit-bot: I haz the power, 2015-03-28 12:59:34]

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

[commit-bot: I haz the power, 2015-03-28 13:53:12]

Committed patchset #7 (id:200001)

[commit-bot: I haz the power, 2015-03-28 13:53:50]

Patchset 7 (id:??) landed as
https://crrev.com/7898d8d28e5d02efcf161cc68ad8ce9b3d970ee2
Cr-Commit-Position: refs/heads/master@{#322705}