Reduce window rectangles cap to 8

Reduce window rectangles cap to 8

Lowers the cap to 8 and adds a warning message if this value is ever
exceeded. The largest (only) implementation currently supports 8, so
there isn't yet reason to go higher.

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

Committed: https://skia.googlesource.com/skia/+/52d721580ee22525c285e2d13cf3975a7a1b2843
Committed: https://skia.googlesource.com/skia/+/7535f419e235e224559ee2b4b62ee6bfddc01556

[csmartdalton, 2016-08-17 18:26:41]

Description was changed from

==========
Reduce window rectangles cap to 8

Lowers the cap to 8 and adds a warning message if this value is ever
exceeded. The largest (only) implementation currently supports 8, so
there isn't yet reason to go higher.

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

to

==========
Reduce window rectangles cap to 8

Lowers the cap to 8 and adds a warning message if this value is ever
exceeded. The largest (only) implementation currently supports 8, so
there isn't yet reason to go higher.

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

[csmartdalton, 2016-08-17 18:27:24]

csmartdalton@google.com changed reviewers:
+ bsalomon@google.com, mjk@nvidia.com

[csmartdalton, 2016-08-17 18:28:21]

https://codereview.chromium.org/2254013002/diff/1/src/gpu/GrWindowRectangles.h
File src/gpu/GrWindowRectangles.h (right):

https://codereview.chromium.org/2254013002/diff/1/src/gpu/GrWindowRectangles....
src/gpu/GrWindowRectangles.h:16: constexpr static int kMaxWindows = 8;
The debug message is an obvious keeper, but let's have Brian weigh in on whether
we want 8 or 16 for the cap.

[bsalomon, 2016-08-17 19:13:24]

lgtm

[csmartdalton, 2016-08-17 19:23:11]

The CQ bit was checked by csmartdalton@google.com

[commit-bot: I haz the power, 2016-08-17 19:23:22]

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 20:52:19]

Description was changed from

==========
Reduce window rectangles cap to 8

Lowers the cap to 8 and adds a warning message if this value is ever
exceeded. The largest (only) implementation currently supports 8, so
there isn't yet reason to go higher.

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

to

==========
Reduce window rectangles cap to 8

Lowers the cap to 8 and adds a warning message if this value is ever
exceeded. The largest (only) implementation currently supports 8, so
there isn't yet reason to go higher.

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

Committed:
https://skia.googlesource.com/skia/+/52d721580ee22525c285e2d13cf3975a7a1b2843
==========

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

Committed patchset #1 (id:1) as
https://skia.googlesource.com/skia/+/52d721580ee22525c285e2d13cf3975a7a1b2843

[robertphillips, 2016-08-17 23:29:25]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/2253293003/ by robertphillips@google.com.

The reason for reverting is: Breaking Ubuntu .

[robertphillips, 2016-08-17 23:33:40]

On 2016/08/17 23:29:25, robertphillips wrote:
> A revert of this CL (patchset #1 id:1) has been created in
> https://codereview.chromium.org/2253293003/ by
mailto:robertphillips@google.com.
> 
> The reason for reverting is: Breaking Ubuntu .

I believe this CL was causing:

In file included from ../../../src/gpu/gl/GrGLGpu.h:12:0,
                 from ../../../src/gpu/gl/GrGLGpu.cpp:8:
../../../src/gpu/gl/GrGLIRect.h: In member function 'void
GrGLGpu::flushWindowRectangles(const GrWindowRectangles&, const
GrGLRenderTarget*)':
../../../src/gpu/gl/GrGLIRect.h:67:42: error: array subscript is above array
bounds [-Werror=array-bounds]
         fLeft = glRect.fLeft + leftOffset;
                                          ^
../../../src/gpu/gl/GrGLIRect.h:67:42: error: array subscript is above array
bounds [-Werror=array-bounds]
../../../src/gpu/gl/GrGLIRect.h:67:42: error: array subscript is above array
bounds [-Werror=array-bounds]
../../../src/gpu/gl/GrGLIRect.h:67:42: error: array subscript is above array
bounds [-Werror=array-bounds]
../../../src/gpu/gl/GrGLIRect.h:67:42: error: array subscript is above array
bounds [-Werror=array-bounds]
../../../src/gpu/gl/GrGLIRect.h:67:42: error: array subscript is above array
bounds [-Werror=array-bounds]
../../../src/gpu/gl/GrGLIRect.h:67:42: error: array subscript is above array
bounds [-Werror=array-bounds]
../../../src/gpu/gl/GrGLIRect.h:67:42: error: array subscript is above array
bounds [-Werror=array-bounds]
../../../src/gpu/gl/GrGLIRect.h:67:42: error: array subscript is above array
bounds [-Werror=array-bounds]
../../../src/gpu/gl/GrGLIRect.h:67:42: error: array subscript is above array
bounds [-Werror=array-bounds]
../../../src/gpu/gl/GrGLIRect.h:67:42: error: array subscript is above array
bounds [-Werror=array-bounds]
../../../src/gpu/gl/GrGLIRect.h:67:42: error: array subscript is above array
bounds [-Werror=array-bounds]

[csmartdalton, 2016-08-19 20:56:56]

Description was changed from

==========
Reduce window rectangles cap to 8

Lowers the cap to 8 and adds a warning message if this value is ever
exceeded. The largest (only) implementation currently supports 8, so
there isn't yet reason to go higher.

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

Committed:
https://skia.googlesource.com/skia/+/52d721580ee22525c285e2d13cf3975a7a1b2843
==========

to

==========
Reduce window rectangles cap to 8

Lowers the cap to 8 and adds a warning message if this value is ever
exceeded. The largest (only) implementation currently supports 8, so
there isn't yet reason to go higher.

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

Committed:
https://skia.googlesource.com/skia/+/52d721580ee22525c285e2d13cf3975a7a1b2843
==========

[csmartdalton, 2016-08-20 04:57:37]

Looks like a gcc bug:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59124

[csmartdalton, 2016-08-20 15:02:26]

Here's a WAR to the gcc warning. It comes with a runtime cost but is maybe a
good practice anyway..

[bsalomon, 2016-08-20 16:37:31]

https://codereview.chromium.org/2254013002/diff/140001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/2254013002/diff/140001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:2012: int numWindows = SkTMin(windows.count(),
int(GrWindowRectangles::kMaxWindows));
We can't just assert here? Is that the nature of the bug?

[csmartdalton, 2016-08-20 16:56:03]

https://codereview.chromium.org/2254013002/diff/140001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/2254013002/diff/140001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:2012: int numWindows = SkTMin(windows.count(),
int(GrWindowRectangles::kMaxWindows));
On 2016/08/20 16:37:31, bsalomon wrote:
> We can't just assert here? Is that the nature of the bug?

This is a release build so an SkASSERT would disapppear anyway right?

[csmartdalton, 2016-08-21 19:43:12]

Patchset #10 (id:180001) has been deleted

[bsalomon, 2016-08-22 13:21:57]

https://codereview.chromium.org/2254013002/diff/200001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/2254013002/diff/200001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:1999: // Blah blah blah.
I like this better.

[csmartdalton, 2016-08-22 14:10:39]

https://codereview.chromium.org/2254013002/diff/200001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/2254013002/diff/200001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:1999: // Blah blah blah.
On 2016/08/22 13:21:57, bsalomon wrote:
> I like this better.

This one doesn't seem to work.. The pragma needs to appear before GrGLIRect.h
gets included, but if we do that first it breaks a commit hook that likes the
includes to be first.

We might be able to sneak a couple includes before the pragma though, and then
do it before including GrGLIRect.h

[bsalomon, 2016-08-22 20:43:02]

On 2016/08/22 14:10:39, csmartdalton wrote:
> https://codereview.chromium.org/2254013002/diff/200001/src/gpu/gl/GrGLGpu.cpp
> File src/gpu/gl/GrGLGpu.cpp (right):
> 
>
https://codereview.chromium.org/2254013002/diff/200001/src/gpu/gl/GrGLGpu.cpp...
> src/gpu/gl/GrGLGpu.cpp:1999: // Blah blah blah.
> On 2016/08/22 13:21:57, bsalomon wrote:
> > I like this better.
> 
> This one doesn't seem to work.. The pragma needs to appear before GrGLIRect.h
> gets included, but if we do that first it breaks a commit hook that likes the
> includes to be first.
> 
> We might be able to sneak a couple includes before the pragma though, and then
> do it before including GrGLIRect.h

I'm ok with the min if you add a comment citing the gcc bug so that our future
selves might remember to remove it.

[csmartdalton, 2016-08-22 22:14:32]

On 2016/08/22 20:43:02, bsalomon wrote:
> On 2016/08/22 14:10:39, csmartdalton wrote:
> >
https://codereview.chromium.org/2254013002/diff/200001/src/gpu/gl/GrGLGpu.cpp
> > File src/gpu/gl/GrGLGpu.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2254013002/diff/200001/src/gpu/gl/GrGLGpu.cpp...
> > src/gpu/gl/GrGLGpu.cpp:1999: // Blah blah blah.
> > On 2016/08/22 13:21:57, bsalomon wrote:
> > > I like this better.
> > 
> > This one doesn't seem to work.. The pragma needs to appear before
GrGLIRect.h
> > gets included, but if we do that first it breaks a commit hook that likes
the
> > includes to be first.
> > 
> > We might be able to sneak a couple includes before the pragma though, and
then
> > do it before including GrGLIRect.h
> 
> I'm ok with the min if you add a comment citing the gcc bug so that our future
> selves might remember to remove it.

I don't love it either but it compiles and the SkTMax realistically isn't going
to affect perf

[bsalomon, 2016-08-23 13:16:12]

lgtm

[csmartdalton, 2016-08-23 13:21:04]

The CQ bit was checked by csmartdalton@google.com

[commit-bot: I haz the power, 2016-08-23 13:21:12]

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-23 13:51:02]

Description was changed from

==========
Reduce window rectangles cap to 8

Lowers the cap to 8 and adds a warning message if this value is ever
exceeded. The largest (only) implementation currently supports 8, so
there isn't yet reason to go higher.

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

Committed:
https://skia.googlesource.com/skia/+/52d721580ee22525c285e2d13cf3975a7a1b2843
==========

to

==========
Reduce window rectangles cap to 8

Lowers the cap to 8 and adds a warning message if this value is ever
exceeded. The largest (only) implementation currently supports 8, so
there isn't yet reason to go higher.

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

Committed:
https://skia.googlesource.com/skia/+/52d721580ee22525c285e2d13cf3975a7a1b2843
Committed:
https://skia.googlesource.com/skia/+/7535f419e235e224559ee2b4b62ee6bfddc01556
==========

[commit-bot: I haz the power, 2016-08-23 13:51:04]

Committed patchset #12 (id:240001) as
https://skia.googlesource.com/skia/+/7535f419e235e224559ee2b4b62ee6bfddc01556