Pass clip to context

Pass clip to context

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/586d5d640b19860dfbbd903a5188da1bbbe87336

[joshualitt, 2015-02-19 15:50:14]

joshualitt@google.com changed reviewers:
+ bsalomon@google.com, jvanverth@google.com, senorblanco@google.com

[joshualitt, 2015-02-19 15:50:48]

joshualitt@google.com changed reviewers:
+ joshualitt@google.com

[joshualitt, 2015-02-19 15:50:48]

Sorry to spring this last minute, but I'd really like to land this before the
branch.  Speedy reviews are appreciated.

[bsalomon, 2015-02-19 16:08:10]

After talking to Joshua we're going to take a different approach for the short
term goal of fixing crashes seen in crash reports.

[joshualitt, 2015-02-23 22:18:41]

On 2015/02/19 16:08:10, bsalomon wrote:
> After talking to Joshua we're going to take a different approach for the short
> term goal of fixing crashes seen in crash reports.

Okay, this patch has been updated cleaned up.  Please take a look.

[Stephen White, 2015-02-24 16:47:37]

senorblanco@chromium.org changed reviewers:
+ senorblanco@chromium.org

[Stephen White, 2015-02-24 16:47:38]

https://codereview.chromium.org/936943002/diff/100001/include/core/SkImageFil...
File include/core/SkImageFilter.h (right):

https://codereview.chromium.org/936943002/diff/100001/include/core/SkImageFil...
include/core/SkImageFilter.h:136: virtual bool filterImageGPU(Proxy*, const
SkBitmap& src, const Context&, const GrClip&,
I'm not fond of this, because it moves the filterImageGPU() signature further
away from the onFilterImage() signature (to which it had been moving).

AFAIK, filters are always called with a wide-open clip, since
SkGpuDevice::filterTexture() uses a GrContext::AutoWideOpenIdentityDraw() before
calling filterImageGPU()). Could we simply make that assumption for now?

[joshualitt, 2015-02-24 16:51:14]

On 2015/02/24 16:47:38, Stephen White wrote:
>
https://codereview.chromium.org/936943002/diff/100001/include/core/SkImageFil...
> File include/core/SkImageFilter.h (right):
> 
>
https://codereview.chromium.org/936943002/diff/100001/include/core/SkImageFil...
> include/core/SkImageFilter.h:136: virtual bool filterImageGPU(Proxy*, const
> SkBitmap& src, const Context&, const GrClip&,
> I'm not fond of this, because it moves the filterImageGPU() signature further
> away from the onFilterImage() signature (to which it had been moving).
> 
> AFAIK, filters are always called with a wide-open clip, since
> SkGpuDevice::filterTexture() uses a GrContext::AutoWideOpenIdentityDraw()
before
> calling filterImageGPU()). Could we simply make that assumption for now?

I was thinking the same thing.  I'm happy to make that change because it
simplifies things.

[bsalomon, 2015-02-24 17:02:05]

https://codereview.chromium.org/936943002/diff/100001/include/gpu/GrClip.h
File include/gpu/GrClip.h (right):

https://codereview.chromium.org/936943002/diff/100001/include/gpu/GrClip.h#ne...
include/gpu/GrClip.h:18: * masks. 'A GrClip is either wide open, just an IRect,
just a Rect(TODO), or a full clipstack.
rm TODO?

https://codereview.chromium.org/936943002/diff/100001/src/gpu/GrClipMaskManag...
File src/gpu/GrClipMaskManager.cpp (right):

https://codereview.chromium.org/936943002/diff/100001/src/gpu/GrClipMaskManag...
src/gpu/GrClipMaskManager.cpp:236: default:
We have bots that have compile flags set that will give us a compile time
warning if there is a missing case (and no default:).

[joshualitt, 2015-02-24 20:50:03]

Patchset #8 (id:140001) has been deleted

[joshualitt, 2015-02-24 20:56:45]

Patchset #7 (id:120001) has been deleted

[joshualitt, 2015-02-24 21:32:38]

On 2015/02/24 17:02:05, bsalomon wrote:
> https://codereview.chromium.org/936943002/diff/100001/include/gpu/GrClip.h
> File include/gpu/GrClip.h (right):
> 
>
https://codereview.chromium.org/936943002/diff/100001/include/gpu/GrClip.h#ne...
> include/gpu/GrClip.h:18: * masks. 'A GrClip is either wide open, just an
IRect,
> just a Rect(TODO), or a full clipstack.
> rm TODO?
> 
>
https://codereview.chromium.org/936943002/diff/100001/src/gpu/GrClipMaskManag...
> File src/gpu/GrClipMaskManager.cpp (right):
> 
>
https://codereview.chromium.org/936943002/diff/100001/src/gpu/GrClipMaskManag...
> src/gpu/GrClipMaskManager.cpp:236: default:
> We have bots that have compile flags set that will give us a compile time
> warning if there is a missing case (and no default:).

feedback incorporated

[Stephen White, 2015-02-24 21:44:38]

src/effects LGTM

[jvanverth1, 2015-02-24 21:52:08]

Text stuff looks okay + 1 comment:

https://codereview.chromium.org/936943002/diff/180001/src/gpu/GrTextContext.cpp
File src/gpu/GrTextContext.cpp (left):

https://codereview.chromium.org/936943002/diff/180001/src/gpu/GrTextContext.c...
src/gpu/GrTextContext.cpp:29:
fClip->getConservativeBounds(fRenderTarget->width(), fRenderTarget->height(),
&fClipRect);
Just wondering if we could remove fClipRect as a member and generate it when we
need it from fClip. That might mean passing it as a member to appendGlyph,
though.

[joshualitt, 2015-02-25 17:48:33]

The CQ bit was checked by joshualitt@google.com

[commit-bot: I haz the power, 2015-02-25 17:48:54]

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

[commit-bot: I haz the power, 2015-02-25 17:49:06]

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

[commit-bot: I haz the power, 2015-02-25 17:49:07]

Presubmit check for 936943002-180001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Since the CL is editing public API, you must have an LGTM from one of:
('reed@chromium.org', 'reed@google.com', 'bsalomon@chromium.org',
'bsalomon@google.com', 'djsollen@chromium.org', 'djsollen@google.com')

[joshualitt, 2015-02-25 17:52:44]

On 2015/02/25 17:49:07, I haz the power (commit-bot) wrote:
> Presubmit check for 936943002-180001 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Since the CL is editing public API, you must have an LGTM from one of:
> (mailto:, mailto:, mailto:,
> mailto:, mailto:, mailto:)

Hey Brian, could I get a quick LGTM?

[bsalomon, 2015-02-25 17:55:26]

lgtm

[joshualitt, 2015-02-25 19:10:54]

The CQ bit was checked by joshualitt@google.com

[commit-bot: I haz the power, 2015-02-25 19:12:00]

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

[commit-bot: I haz the power, 2015-02-25 19:21:27]

Committed patchset #8 (id:180001) as
https://skia.googlesource.com/skia/+/586d5d640b19860dfbbd903a5188da1bbbe87336

[joshualitt, 2015-02-25 21:03:47]

A revert of this CL (patchset #8 id:180001) has been created in
https://codereview.chromium.org/956083002/ by joshualitt@google.com.

The reason for reverting is: Strange blur problems on nexus 5.

[joshualitt, 2015-02-25 21:18:44]

The CQ bit was checked by joshualitt@google.com

[commit-bot: I haz the power, 2015-02-25 21:19:26]

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

[joshualitt, 2015-02-25 21:19:39]

The CQ bit was unchecked by joshualitt@google.com

[commit-bot: I haz the power, 2015-02-25 21:19:53]

Committed patchset #8 (id:180001) as
https://skia.googlesource.com/skia/+/570d2f81a65fc868d6300a7edf34c0d5d048c5d6

[joshualitt, 2015-02-25 21:20:08]

The CQ bit was checked by joshualitt@google.com

[joshualitt, 2015-02-25 21:20:12]

The CQ bit was unchecked by joshualitt@google.com