Make GrReducedClip's gen ID only apply to the element list

Make GrReducedClip's gen ID only apply to the element list

Renames fGenID to fElementsGenID and designates this value as
undefined when when the element list is empty.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2244223004

Committed: https://skia.googlesource.com/skia/+/8d3f92a92be713cedfee48e01987264bd4f3083f

[csmartdalton, 2016-08-16 02:54:15]

Description was changed from

==========
Fix GrReducedClip to set kEmptyGenID when scissor is empty

BUG=skia:
==========

to

==========
Fix GrReducedClip to set kEmptyGenID when scissor is empty

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2244223004
==========

[csmartdalton, 2016-08-16 02:54:48]

csmartdalton@google.com changed reviewers:
+ bsalomon@google.com

[csmartdalton, 2016-08-16 05:09:15]

Description was changed from

==========
Fix GrReducedClip to set kEmptyGenID when scissor is empty

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2244223004
==========

to

==========
Fix GrReducedClip::fGenID to be kEmpty/kWideOpen when appropriate

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2244223004
==========

[csmartdalton, 2016-08-16 05:18:49]

Actually I'm not so sure this is a good idea.

https://codereview.chromium.org/2244223004/diff/20001/src/gpu/GrReducedClip.cpp
File src/gpu/GrReducedClip.cpp (right):

https://codereview.chromium.org/2244223004/diff/20001/src/gpu/GrReducedClip.c...
src/gpu/GrReducedClip.cpp:321: *resultGenID = SkClipStack::kWideOpenGenID;
In fact, this can be wrong if you have a clip with an outer pixel aligned rect:
ibounds() takes care of the outer rect so we skip it. Is it really correct for
fGenID to be kWideOpen? Should we just always let it be the top element on the
stack?

[bsalomon, 2016-08-16 14:14:41]

lgtm

https://codereview.chromium.org/2244223004/diff/20001/src/gpu/GrReducedClip.cpp
File src/gpu/GrReducedClip.cpp (right):

https://codereview.chromium.org/2244223004/diff/20001/src/gpu/GrReducedClip.c...
src/gpu/GrReducedClip.cpp:321: *resultGenID = SkClipStack::kWideOpenGenID;
On 2016/08/16 05:18:49, csmartdalton wrote:
> In fact, this can be wrong if you have a clip with an outer pixel aligned
rect:
> ibounds() takes care of the outer rect so we skip it. Is it really correct for
> fGenID to be kWideOpen? Should we just always let it be the top element on the
> stack?

If I follow, I think it is correct. I think the function is documented to say
that the caller must not draw outside of the ibounds. That may not be the best
interface, though.

[csmartdalton, 2016-08-17 03:31:04]

Description was changed from

==========
Fix GrReducedClip::fGenID to be kEmpty/kWideOpen when appropriate

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2244223004
==========

to

==========
Make GrReducedClip's gen ID only apply to the element list

Renames fGenID to fElementsGenID and designates this value as
undefined when when the element list is empty.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2244223004
==========

[csmartdalton, 2016-08-17 03:31:32]

Complete redo. I think this approach is much better.

[bsalomon, 2016-08-17 13:34:53]

lgtm w/ note about comment

https://codereview.chromium.org/2244223004/diff/60001/src/gpu/GrReducedClip.h
File src/gpu/GrReducedClip.h (right):

https://codereview.chromium.org/2244223004/diff/60001/src/gpu/GrReducedClip.h...
src/gpu/GrReducedClip.h:46: * If elements() are nonempty, uniquely identifies
the list of elements. Otherwise undefined.
I don't think it matters other than for the comment, but don't we sometimes
remove elements from the head of the list? That would mean the last element's ID
doesn't uniquely identify the entire list. I believe in practice we use the id
plus the ibounds to uniquely identify.

[csmartdalton, 2016-08-17 14:49:14]

https://codereview.chromium.org/2244223004/diff/60001/src/gpu/GrReducedClip.h
File src/gpu/GrReducedClip.h (right):

https://codereview.chromium.org/2244223004/diff/60001/src/gpu/GrReducedClip.h...
src/gpu/GrReducedClip.h:46: * If elements() are nonempty, uniquely identifies
the list of elements. Otherwise undefined.
On 2016/08/17 13:34:53, bsalomon wrote:
> I don't think it matters other than for the comment, but don't we sometimes
> remove elements from the head of the list? That would mean the last element's
ID
> doesn't uniquely identify the entire list. I believe in practice we use the id
> plus the ibounds to uniquely identify.

If I follow, then this comment should clear it up?

[bsalomon, 2016-08-17 14:50:01]

On 2016/08/17 14:49:14, csmartdalton wrote:
> https://codereview.chromium.org/2244223004/diff/60001/src/gpu/GrReducedClip.h
> File src/gpu/GrReducedClip.h (right):
> 
>
https://codereview.chromium.org/2244223004/diff/60001/src/gpu/GrReducedClip.h...
> src/gpu/GrReducedClip.h:46: * If elements() are nonempty, uniquely identifies
> the list of elements. Otherwise undefined.
> On 2016/08/17 13:34:53, bsalomon wrote:
> > I don't think it matters other than for the comment, but don't we sometimes
> > remove elements from the head of the list? That would mean the last
element's
> ID
> > doesn't uniquely identify the entire list. I believe in practice we use the
id
> > plus the ibounds to uniquely identify.
> 
> If I follow, then this comment should clear it up?

lgtm

[csmartdalton, 2016-08-17 15:03:57]

The CQ bit was checked by csmartdalton@google.com

[commit-bot: I haz the power, 2016-08-17 15:04:08]

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-08-17 15:27:38]

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

[commit-bot: I haz the power, 2016-08-17 15:27:39]

Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-GN-Trybot on master.client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[csmartdalton, 2016-08-17 15:52:26]

The CQ bit was checked by csmartdalton@google.com

[csmartdalton, 2016-08-17 15:52:27]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/2244223004/#ps100001
(title: "bug")

[commit-bot: I haz the power, 2016-08-17 15:52:31]

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

[csmartdalton, 2016-08-17 15:53:51]

The CQ bit was unchecked by csmartdalton@google.com

[csmartdalton, 2016-08-17 15:58:57]

The CQ bit was checked by csmartdalton@google.com

[csmartdalton, 2016-08-17 15:58:57]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/2244223004/#ps120001
(title: "more improved testing")

[commit-bot: I haz the power, 2016-08-17 15:59:01]

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-08-17 16:39:40]

Description was changed from

==========
Make GrReducedClip's gen ID only apply to the element list

Renames fGenID to fElementsGenID and designates this value as
undefined when when the element list is empty.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2244223004
==========

to

==========
Make GrReducedClip's gen ID only apply to the element list

Renames fGenID to fElementsGenID and designates this value as
undefined when when the element list is empty.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2244223004

Committed:
https://skia.googlesource.com/skia/+/8d3f92a92be713cedfee48e01987264bd4f3083f
==========

[commit-bot: I haz the power, 2016-08-17 16:39:41]

Committed patchset #7 (id:120001) as
https://skia.googlesource.com/skia/+/8d3f92a92be713cedfee48e01987264bd4f3083f