Incorrect LocalFrame::paintIntoDragImage picture bounds

Incorrect LocalFrame::paintIntoDragImage picture bounds

LocalFrame::paintIntoDragImage() translates the content to be origin-
aligned, but leaves the picture bounds unchanged => the picture content
lies outside the declared picture cull rect.

We need to also shift the picture bounds, to keep them aligned with its
content.

R=junov@chromium.org,mtklein@chromium.org
BUG=522143
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200763

[f(malita), 2015-08-18 12:56:14]

fmalita@chromium.org changed reviewers:
+ mtklein@chromium.org
- reed@google.com

[f(malita), 2015-08-18 13:02:39]

This was exposed by http://crrev.com/1264133003: it broke a handful of unit
tests when attempting to roll (and got reverted).

Since picture bounds are only used for culling, and the drag image code is not
participating in normal content/SP recording (which would presumably catch
bounds problems thanks to the BBH), I can't think of a good way to test this
ATM.  But as soon as we reland http://crrev.com/1264133003, the unit tests which
caught this in the first place should provide enough coverage.

[mtklein_C, 2015-08-18 14:07:06]

https://codereview.chromium.org/1303453003/diff/20001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1303453003/diff/20001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:601: SkPictureBuilder
pictureBuilder(FloatRect(FloatPoint(), paintingRect.size()));
Think it's worth adding a note similar to the CL description here?  Or is this
obvious in retrospect?

[Justin Novosad, 2015-08-18 14:36:33]

https://codereview.chromium.org/1303453003/diff/20001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1303453003/diff/20001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:607: transform.translate(-paintingRect.x(),
-paintingRect.y());
I suppose this is the transaction to counteract the origin shift? What use case
exactly was broken before this change? Could you add a BUG= to the description?

[Justin Novosad, 2015-08-18 15:11:00]

Um, I obviously meant "translation" not "transaction"

[f(malita), 2015-08-18 17:40:14]

https://codereview.chromium.org/1303453003/diff/20001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1303453003/diff/20001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:601: SkPictureBuilder
pictureBuilder(FloatRect(FloatPoint(), paintingRect.size()));
On 2015/08/18 14:07:06, mtklein_C wrote:
> Think it's worth adding a note similar to the CL description here?  Or is this
> obvious in retrospect?

Done.

https://codereview.chromium.org/1303453003/diff/20001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:607: transform.translate(-paintingRect.x(),
-paintingRect.y());
On 2015/08/18 14:36:33, Justin Novosad wrote:
> I suppose this is the transaction to counteract the origin shift? What use
case
> exactly was broken before this change? Could you add a BUG= to the
description?

We're ultimately drawing this into a (0,0) - (paintingRect.width(),
paintingRect.height()) bitmap, so this is just mapping the content rect to the
bitmap bounds.

The problem is we're not doing the same for the SKP cull rect - so while we're
drawing near origin, the SKP thinks its content is some place else.

Nothing is broken currently, because the SKP cull rect is only used for
quick-reject by the BBH (and these drag images are not recorded within the
normal SKP hierarchy).

But it blocks http://crrev.com/1264133003 which generalizes the quick reject at
the SkCanvas level.  With that in place, the dragImage SKP bounds need to be
correct to avoid spurious culling.

Added a BUG.

[Justin Novosad, 2015-08-18 18:13:33]

lgtm

[mtklein_C, 2015-08-18 18:33:05]

lgtm

[f(malita), 2015-08-18 20:59:17]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2015-08-18 20:59:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1303453003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1303453003/40001

[commit-bot: I haz the power, 2015-08-18 22:55:58]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200763