allow pictures to have a full bounds

allow pictures to have a full bounds

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/7e76bff26e7c74902841ca4f607eb0b24a833a4a

[reed1, 2014-11-18 20:20:19]

reed@google.com changed reviewers:
+ fmalita@google.com, mtklein@google.com, robertphillips@google.com

[reed1, 2014-11-18 20:20:36]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-11-18 20:20:49]

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

[commit-bot: I haz the power, 2014-11-18 20:20:50]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2014-11-19 02:20 UTC

[reed1, 2014-11-18 20:21:23]

The CQ bit was unchecked by reed@google.com

[reed1, 2014-11-18 20:34:49]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-11-18 20:35:35]

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

[commit-bot: I haz the power, 2014-11-18 20:35:36]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2014-11-19 02:35 UTC

[reed1, 2014-11-18 20:36:51]

The CQ bit was unchecked by reed@google.com

[robertphillips, 2014-11-18 20:51:53]

https://codereview.chromium.org/736583004/diff/20001/include/core/SkPicture.h
File include/core/SkPicture.h (right):

https://codereview.chromium.org/736583004/diff/20001/include/core/SkPicture.h...
include/core/SkPicture.h:132: */
return "const SkRect&" ?

https://codereview.chromium.org/736583004/diff/20001/src/core/SkRecorder.cpp
File src/core/SkRecorder.cpp (right):

https://codereview.chromium.org/736583004/diff/20001/src/core/SkRecorder.cpp#...
src/core/SkRecorder.cpp:12: 
Add one of these to SkRect?

[mtklein, 2014-11-18 21:06:49]

https://codereview.chromium.org/736583004/diff/20001/include/core/SkPicture.h
File include/core/SkPicture.h (right):

https://codereview.chromium.org/736583004/diff/20001/include/core/SkPicture.h...
include/core/SkPicture.h:132: */
On 2014/11/18 20:51:53, robertphillips wrote:
> return "const SkRect&" ?

I'd prefer to leave as many return types as SkRect as possible.

This leaves us more flexibility in how/if we store it.

https://codereview.chromium.org/736583004/diff/20001/samplecode/SampleArc.cpp
File samplecode/SampleArc.cpp (right):

https://codereview.chromium.org/736583004/diff/20001/samplecode/SampleArc.cpp...
samplecode/SampleArc.cpp:41: static void test_pictbounds(SkCanvas* canvas) {
Intentionally leaving this in?

https://codereview.chromium.org/736583004/diff/20001/src/core/SkBBHFactory.cpp
File src/core/SkBBHFactory.cpp (right):

https://codereview.chromium.org/736583004/diff/20001/src/core/SkBBHFactory.cp...
src/core/SkBBHFactory.cpp:21: const int width =
SkScalarRoundToInt(bounds.width());
Is RoundToInt away-from-zero?

https://codereview.chromium.org/736583004/diff/20001/src/core/SkCanvasDrawabl...
File src/core/SkCanvasDrawable.cpp (right):

https://codereview.chromium.org/736583004/diff/20001/src/core/SkCanvasDrawabl...
src/core/SkCanvasDrawable.cpp:26: static void draw_bbox(SkCanvas* canvas, const
SkRect& r) {
Intentionally left in?

[reed1, 2014-11-18 22:05:30]

will update CL tomorrow

https://codereview.chromium.org/736583004/diff/20001/include/core/SkPicture.h
File include/core/SkPicture.h (right):

https://codereview.chromium.org/736583004/diff/20001/include/core/SkPicture.h...
include/core/SkPicture.h:132: */
On 2014/11/18 21:06:49, mtklein wrote:
> On 2014/11/18 20:51:53, robertphillips wrote:
> > return "const SkRect&" ?
> 
> I'd prefer to leave as many return types as SkRect as possible.
> 
> This leaves us more flexibility in how/if we store it.

Acknowledged.

https://codereview.chromium.org/736583004/diff/20001/samplecode/SampleArc.cpp
File samplecode/SampleArc.cpp (right):

https://codereview.chromium.org/736583004/diff/20001/samplecode/SampleArc.cpp...
samplecode/SampleArc.cpp:41: static void test_pictbounds(SkCanvas* canvas) {
On 2014/11/18 21:06:49, mtklein wrote:
> Intentionally leaving this in?

Nope, accidental commit

https://codereview.chromium.org/736583004/diff/20001/src/core/SkBBHFactory.cpp
File src/core/SkBBHFactory.cpp (right):

https://codereview.chromium.org/736583004/diff/20001/src/core/SkBBHFactory.cp...
src/core/SkBBHFactory.cpp:21: const int width =
SkScalarRoundToInt(bounds.width());
On 2014/11/18 21:06:49, mtklein wrote:
> Is RoundToInt away-from-zero?

Nope, but I can change it to that.
Really what we probably want is to roundOut the rect, and that that guy's
width/height.

https://codereview.chromium.org/736583004/diff/20001/src/core/SkCanvasDrawabl...
File src/core/SkCanvasDrawable.cpp (right):

https://codereview.chromium.org/736583004/diff/20001/src/core/SkCanvasDrawabl...
src/core/SkCanvasDrawable.cpp:26: static void draw_bbox(SkCanvas* canvas, const
SkRect& r) {
On 2014/11/18 21:06:49, mtklein wrote:
> Intentionally left in?

For now yea, was planning to make this a runtime (debug-only?) option on
drawable, so I can see diagnostics.

https://codereview.chromium.org/736583004/diff/20001/src/core/SkRecorder.cpp
File src/core/SkRecorder.cpp (right):

https://codereview.chromium.org/736583004/diff/20001/src/core/SkRecorder.cpp#...
src/core/SkRecorder.cpp:12: 
On 2014/11/18 20:51:53, robertphillips wrote:
> Add one of these to SkRect?

Love to, but I have to clean up SkRect.h first, which *has* roundOut() that
doesn't return anything but affects the "this" inplace :(

[mtklein, 2014-11-18 22:26:12]

https://codereview.chromium.org/736583004/diff/20001/src/core/SkBBHFactory.cpp
File src/core/SkBBHFactory.cpp (right):

https://codereview.chromium.org/736583004/diff/20001/src/core/SkBBHFactory.cp...
src/core/SkBBHFactory.cpp:21: const int width =
SkScalarRoundToInt(bounds.width());
On 2014/11/18 22:05:30, reed1 wrote:
> On 2014/11/18 21:06:49, mtklein wrote:
> > Is RoundToInt away-from-zero?
> 
> Nope, but I can change it to that.
> Really what we probably want is to roundOut the rect, and that that guy's
> width/height.

That sounds right.

https://codereview.chromium.org/736583004/diff/20001/src/core/SkCanvasDrawabl...
File src/core/SkCanvasDrawable.cpp (right):

https://codereview.chromium.org/736583004/diff/20001/src/core/SkCanvasDrawabl...
src/core/SkCanvasDrawable.cpp:26: static void draw_bbox(SkCanvas* canvas, const
SkRect& r) {
On 2014/11/18 22:05:30, reed1 wrote:
> On 2014/11/18 21:06:49, mtklein wrote:
> > Intentionally left in?
> 
> For now yea, was planning to make this a runtime (debug-only?) option on
> drawable, so I can see diagnostics. 

In that case, seems fine as is to me.

[reed1, 2014-11-19 14:49:16]

https://codereview.chromium.org/736583004/diff/20001/src/core/SkBBHFactory.cpp
File src/core/SkBBHFactory.cpp (right):

https://codereview.chromium.org/736583004/diff/20001/src/core/SkBBHFactory.cp...
src/core/SkBBHFactory.cpp:21: const int width =
SkScalarRoundToInt(bounds.width());
On 2014/11/18 22:26:11, mtklein wrote:
> On 2014/11/18 22:05:30, reed1 wrote:
> > On 2014/11/18 21:06:49, mtklein wrote:
> > > Is RoundToInt away-from-zero?
> > 
> > Nope, but I can change it to that.
> > Really what we probably want is to roundOut the rect, and that that guy's
> > width/height.
> 
> That sounds right.

Done.

https://codereview.chromium.org/736583004/diff/20001/src/core/SkRecorder.cpp
File src/core/SkRecorder.cpp (right):

https://codereview.chromium.org/736583004/diff/20001/src/core/SkRecorder.cpp#...
src/core/SkRecorder.cpp:12: 
On 2014/11/18 22:05:30, reed1 wrote:
> On 2014/11/18 20:51:53, robertphillips wrote:
> > Add one of these to SkRect?
> 
> Love to, but I have to clean up SkRect.h first, which *has* roundOut() that
> doesn't return anything but affects the "this" inplace :(

Done.

[reed1, 2014-11-19 14:49:36]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-11-19 14:50:45]

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

[commit-bot: I haz the power, 2014-11-19 14:50:45]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2014-11-19 20:50 UTC

[robertphillips, 2014-11-19 14:57:22]

lgtm + nit

https://codereview.chromium.org/736583004/diff/40001/include/core/SkPictureRe...
File include/core/SkPictureRecorder.h (right):

https://codereview.chromium.org/736583004/diff/40001/include/core/SkPictureRe...
include/core/SkPictureRecorder.h:45: 
comment change needed ?

[commit-bot: I haz the power, 2014-11-19 14:59:47]

Committed patchset #3 (id:40001) as
https://skia.googlesource.com/skia/+/7e76bff26e7c74902841ca4f607eb0b24a833a4a

[f(malita), 2014-11-19 15:02:26]

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

[f(malita), 2014-11-19 15:02:27]

Post-landing nit.

https://codereview.chromium.org/736583004/diff/40001/samplecode/SampleArc.cpp
File samplecode/SampleArc.cpp (right):

https://codereview.chromium.org/736583004/diff/40001/samplecode/SampleArc.cpp...
samplecode/SampleArc.cpp:200: if (true) { test_pictbounds(canvas); return; }
Disabling "Arcs" intentionally?

https://codereview.chromium.org/736583004/diff/40001/src/core/SkCanvasDrawabl...
File src/core/SkCanvasDrawable.cpp (right):

https://codereview.chromium.org/736583004/diff/40001/src/core/SkCanvasDrawabl...
src/core/SkCanvasDrawable.cpp:40: draw_bbox(canvas, this->getBounds());
This could be a debugger option.

(I'm planning to add similar functionality for text blobs - it seems a general
"draw op bounds" visualization option would make sense)

[mtklein, 2014-11-19 15:16:00]

https://codereview.chromium.org/736583004/diff/40001/src/core/SkCanvasDrawabl...
File src/core/SkCanvasDrawable.cpp (right):

https://codereview.chromium.org/736583004/diff/40001/src/core/SkCanvasDrawabl...
src/core/SkCanvasDrawable.cpp:40: draw_bbox(canvas, this->getBounds());
On 2014/11/19 15:02:27, Florin Malita wrote:
> This could be a debugger option.
> 
> (I'm planning to add similar functionality for text blobs - it seems a general
> "draw op bounds" visualization option would make sense)

This SGTM.  This would be pretty easy to do with a new subclass of SkBBH and
recording a picture with that.  In endRecording(), SkPicture will call
SkBBH::insert() with an array of bounds, one for each op.

[reed1, 2014-11-19 15:16:09]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/722043005/ by reed@google.com.

The reason for reverting is: needed to update legacy width() helpers.