Stop using (deprecated) getTotalClip

Stop using (deprecated) getTotalClip

patch from issue 342553006

BUG=
android failures unrelated to this CL
NOTRY=True

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

[f(malita), 2014-06-19 15:15:47]

https://codereview.chromium.org/344793003/diff/1/skia/ext/skia_utils_mac.mm
File skia/ext/skia_utils_mac.mm (right):

https://codereview.chromium.org/344793003/diff/1/skia/ext/skia_utils_mac.mm#n...
skia/ext/skia_utils_mac.mm:376: const SkIPoint& pt = device->getOrigin();
Nit: maybe inline this since we're only using pt once.

https://codereview.chromium.org/344793003/diff/1/skia/ext/skia_utils_mac.mm#n...
skia/ext/skia_utils_mac.mm:377: // get clipbounds into the coordinate systme of
the top layer/device
Typo: system

https://codereview.chromium.org/344793003/diff/1/skia/ext/skia_utils_mac.mm#n...
skia/ext/skia_utils_mac.mm:390: if (!bitmap_.allocN32Pixels(clipBounds.width(),
clipBounds.height()))
We used to allocate a bitmap matching the device backing size, and now we limit
it to the visible part/clip rect.

But for this to work, we also need to account for the clip rect offset in a) the
CGContext CTM and b) the compositing phase - no? I *think* the current
assumption is that the CGContext origin == device origin.

[reed1, 2014-06-19 16:40:54]

https://codereview.chromium.org/344793003/diff/1/skia/ext/skia_utils_mac.mm
File skia/ext/skia_utils_mac.mm (right):

https://codereview.chromium.org/344793003/diff/1/skia/ext/skia_utils_mac.mm#n...
skia/ext/skia_utils_mac.mm:376: const SkIPoint& pt = device->getOrigin();
On 2014/06/19 15:15:46, Florin Malita wrote:
> Nit: maybe inline this since we're only using pt once.

We use it down on line 412

https://codereview.chromium.org/344793003/diff/1/skia/ext/skia_utils_mac.mm#n...
skia/ext/skia_utils_mac.mm:377: // get clipbounds into the coordinate systme of
the top layer/device
On 2014/06/19 15:15:47, Florin Malita wrote:
> Typo: system

Done.

https://codereview.chromium.org/344793003/diff/1/skia/ext/skia_utils_mac.mm#n...
skia/ext/skia_utils_mac.mm:390: if (!bitmap_.allocN32Pixels(clipBounds.width(),
clipBounds.height()))
On 2014/06/19 15:15:47, Florin Malita wrote:
> We used to allocate a bitmap matching the device backing size, and now we
limit
> it to the visible part/clip rect.
> 
> But for this to work, we also need to account for the clip rect offset in a)
the
> CGContext <span id="abbrex-spnAbbr_112174" class="abbrex-Abbr abbrex-Abbr_CTM
abbrex-AbbrUnknown abbrex-AbbrUnknownVisible">CTM</span> and b) the compositing
phase - no? I *think* the current
> assumption is that the CGContext origin == device origin.

I think you're right, I will need to adjust the offset during compose. I don't
have to make this change, but it seems smart to limit the size of the allocation
if possible.

[reed1, 2014-06-19 17:55:28]

ptal

[f(malita), 2014-06-19 18:39:54]

I'm a bit worried about test coverage: there's
SkiaUtilsMacTest::RunBitLockerTest() but it seems to only exercise the
useDeviceBits_ path (clipRect only).

https://codereview.chromium.org/344793003/diff/20001/skia/ext/skia_utils_mac.mm
File skia/ext/skia_utils_mac.mm (right):

https://codereview.chromium.org/344793003/diff/20001/skia/ext/skia_utils_mac....
skia/ext/skia_utils_mac.mm:385: bitmap_.lockPixels();
        bitmapOffset_.setZero();

https://codereview.chromium.org/344793003/diff/20001/skia/ext/skia_utils_mac....
skia/ext/skia_utils_mac.mm:413:
skMatrix.postTranslate(-SkIntToScalar(clip_bounds.x()),
Do we still need to include the clip offset here when useDeviceBits_ == true?

[reed1, 2014-06-19 18:57:19]

https://codereview.chromium.org/344793003/diff/20001/skia/ext/skia_utils_mac.mm
File skia/ext/skia_utils_mac.mm (right):

https://codereview.chromium.org/344793003/diff/20001/skia/ext/skia_utils_mac....
skia/ext/skia_utils_mac.mm:385: bitmap_.lockPixels();
On 2014/06/19 18:39:54, Florin Malita wrote:
>         bitmapOffset_.setZero();

It won't get read in this case, but I will add the setZero() call.

https://codereview.chromium.org/344793003/diff/20001/skia/ext/skia_utils_mac....
skia/ext/skia_utils_mac.mm:413:
skMatrix.postTranslate(-SkIntToScalar(clip_bounds.x()),
On 2014/06/19 18:39:54, Florin Malita wrote:
> Do we still need to include the clip offset here when useDeviceBits_ == true?

I think so. From CG's perspective, both scenarios are the same -- we just create
a CGContext that points to some of our memory. In both cases, we need to tell CG
the offset between local coordinates and the bits we are pointing at, and
clip_bounds.origin is that offset.

[reed1, 2014-06-19 20:39:29]

ptal

[f(malita), 2014-06-19 20:58:42]

lgtm

https://codereview.chromium.org/344793003/diff/60001/skia/ext/skia_utils_mac.mm
File skia/ext/skia_utils_mac.mm (right):

https://codereview.chromium.org/344793003/diff/60001/skia/ext/skia_utils_mac....
skia/ext/skia_utils_mac.mm:378: bitmapOffset_.set(clip_bounds.x(),
clip_bounds.y());
I think this needs to happen after the releaseIfNeeded() call below - otherwise
we could overwrite the bitmapOffset for the current/pending cgContext.

[reed1, 2014-06-19 20:59:47]

On 2014/06/19 20:58:42, Florin Malita wrote:
> lgtm
> 
>
https://codereview.chromium.org/344793003/diff/60001/skia/ext/skia_utils_mac.mm
> File skia/ext/skia_utils_mac.mm (right):
> 
>
https://codereview.chromium.org/344793003/diff/60001/skia/ext/skia_utils_mac....
> skia/ext/skia_utils_mac.mm:378: bitmapOffset_.set(clip_bounds.x(),
> clip_bounds.y());
> I think this needs to happen after the releaseIfNeeded() call below -
otherwise
> we could overwrite the bitmapOffset for the current/pending cgContext.

oh wow, great catch.

[reed1, 2014-06-19 21:23:08]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-06-19 21:24:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/reed@google.com/344793003/80001

[reed2, 2014-06-20 02:24:17]

The CQ bit was unchecked by reed@chromium.org

[reed2, 2014-06-20 02:24:26]

The CQ bit was checked by reed@chromium.org

[commit-bot: I haz the power, 2014-06-20 02:25:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/reed@google.com/344793003/80001

[commit-bot: I haz the power, 2014-06-20 05:23:11]

Change committed as 278613