Fix off by one in creating a RasterShape

Fix off by one in creating a RasterShape

marginY1 is supposed to be an inclusive endpoint, however, maxY() is an
exclusive endpoint. To fix this, subtract one from maxY() before
assiging to marginY1 so that we don't walk off the end of the intervals
vector.

BUG=379108
R=betravis@adobe.com,pdr@chromium.org,inferno@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175433

[pdr., 2014-06-03 23:08:55]

https://codereview.chromium.org/318443004/diff/1/Source/core/rendering/shapes...
File Source/core/rendering/shapes/RasterShape.cpp (right):

https://codereview.chromium.org/318443004/diff/1/Source/core/rendering/shapes...
Source/core/rendering/shapes/RasterShape.cpp:89: int marginY1 = std::min(maxY()
- 1, y + shapeMargin);
This is tricky code and I'm certainly not an expert here, but I'd recommend a
small tweak. In general, for (... < ... ++) is by-far the most common
incrementing for loop in Blink, and for(... >= ... --) is the most common
decrementing for loop.

I think this may be easier to understand if we change it to:
int marginY1 = std::min(maxY(), y + shapeMargin + 1);

And update the for loop below to:
for (int marginY = y + 1; marginY < marginY1; ++marginY) {

Possibly updating the variable name to be easier to understand as well. LGTM for
the fix with or without these suggestions.

[Bem Jones-Bey (adobe), 2014-06-03 23:13:36]

On 2014/06/03 23:08:55, pdr wrote:
>
https://codereview.chromium.org/318443004/diff/1/Source/core/rendering/shapes...
> File Source/core/rendering/shapes/RasterShape.cpp (right):
> 
>
https://codereview.chromium.org/318443004/diff/1/Source/core/rendering/shapes...
> Source/core/rendering/shapes/RasterShape.cpp:89: int marginY1 =
std::min(maxY()
> - 1, y + shapeMargin);
> This is tricky code and I'm certainly not an expert here, but I'd recommend a
> small tweak. In general, for (... < ... ++) is by-far the most common
> incrementing for loop in Blink, and for(... >= ... --) is the most common
> decrementing for loop.
> 
> I think this may be easier to understand if we change it to:
> int marginY1 = std::min(maxY(), y + shapeMargin + 1);
> 
> And update the for loop below to:
> for (int marginY = y + 1; marginY < marginY1; ++marginY) {

I'll make these updates.

> 
> Possibly updating the variable name to be easier to understand as well. LGTM
for
> the fix with or without these suggestions.

Thanks for the lgtm. I'm going to spend some more time with this code and would
like to update the names of many of the variables to make it easier to
understand. So I'll do the name updates in a future patch, probably with some
refactoring.

[Bem Jones-Bey (adobe), 2014-06-03 23:29:01]

The CQ bit was checked by bjonesbe@adobe.com

[commit-bot: I haz the power, 2014-06-03 23:29:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bjonesbe@adobe.com/318443004/20001

[commit-bot: I haz the power, 2014-06-04 02:02:36]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/10483)

[commit-bot: I haz the power, 2014-06-04 03:09:05]

Change committed as 175433