Add SkCanvas::drawPosTextBounded.

Add SkCanvas::drawPosTextBounded.

This allows subclasses which need bounding box information to get it
from the caller if it has already been computed.

BUG=chromium:269080
Committed: http://code.google.com/p/skia/source/detail?r=10612

[jbroman, 2013-08-07 13:49:29]

In Chromium, when impl-side painting is on we end up recording via an
SkBBoxRecord. Whenever we paint text, SkBBoxRecord::drawPosText computes the
bounding box of the text to be drawn.

However, the caller in Blink already has a bounding SkRect, so it would be great
to be able to pass that in. This doesn't seem to break anything, but leads to a
substantial performance improvement (see bug).

I'm not 100% sure how you would like this exposed in Skia, but this seems like a
reasonable approach to me (and is the approach I used to benchmark in the bug).

[tomhudson, 2013-08-07 15:48:39]

lgtm

https://codereview.chromium.org/22585002/diff/4001/src/core/SkBBoxRecord.cpp
File src/core/SkBBoxRecord.cpp (right):

https://codereview.chromium.org/22585002/diff/4001/src/core/SkBBoxRecord.cpp#...
src/core/SkBBoxRecord.cpp:181: const SkPaint& paint) {
Is it worth putting in an SkASSERT here that the bbox passed in matches the bbox
that would be computed by drawPosText?

[jbroman, 2013-08-07 15:51:35]

On 2013/08/07 15:48:39, tomhudson wrote:
> lgtm
> 
> https://codereview.chromium.org/22585002/diff/4001/src/core/SkBBoxRecord.cpp
> File src/core/SkBBoxRecord.cpp (right):
> 
>
https://codereview.chromium.org/22585002/diff/4001/src/core/SkBBoxRecord.cpp#...
> src/core/SkBBoxRecord.cpp:181: const SkPaint& paint) {
> Is it worth putting in an SkASSERT here that the bbox passed in matches the
bbox
> that would be computed by drawPosText?

I'm not sure it necessarily will be the same (in both cases, a heuristic is used
-- and I don't think the ones used in Blink and SkBBoxRecord are precisely
identical). I'd say it's up to the caller to give a valid bounding box.

WDYT?

[tomhudson, 2013-08-07 15:55:42]

On 2013/08/07 15:51:35, jbroman wrote:
> On 2013/08/07 15:48:39, tomhudson wrote:
> > Is it worth putting in an SkASSERT here that the bbox passed in matches the
bbox
> > that would be computed by drawPosText?
> 
> I'm not sure it necessarily will be the same (in both cases, a heuristic is
used
> -- and I don't think the ones used in Blink and SkBBoxRecord are precisely
> identical). I'd say it's up to the caller to give a valid bounding box.
> 
> WDYT?

SGTM

[commit-bot: I haz the power, 2013-08-07 15:57:17]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/jbroman@chromium.org/22585002/4001

[commit-bot: I haz the power, 2013-08-07 16:22:52]

Change committed as 10612

[reed1, 2013-08-07 17:10:39]

I'm sorry, I should have spoken up sooner.

I'd like to consider other ways to add bboxes to the public API. I don't need to
revert this per-se, but please don't start using this new API until I/we can
talk more about it.

I don't know how to formalize this, but in general I'd like to be a required
reviewer on any public API changes, esp. big ones that effect an inheritance
hierarchy like canvas.

[jbroman, 2013-08-07 17:12:41]

On 2013/08/07 17:10:39, reed1 wrote:
> I'm sorry, I should have spoken up sooner.
> 
> I'd like to consider other ways to add bboxes to the public API. I don't need
to
> revert this per-se, but please don't start using this new API until I/we can
> talk more about it.
> 
> I don't know how to formalize this, but in general I'd like to be a required
> reviewer on any public API changes, esp. big ones that effect an inheritance
> hierarchy like canvas.

Sure. I'll wait for your further comments on this; I'm not wedded to this
approach.

[tomhudson, 2013-08-20 13:00:16]

Ping, Mike?

We've missed M30, but a 20% recording speedup on text-heavy sites would be nice
to have in the pipeline for M31. Should we file a Skia bug to track this
particular need? Start a design doc? Tracking bug for all the performance issues
we've got that impact the Skia public API?

[reed1, 2013-08-20 15:07:32]

Filing a skia issue would be fine. We are actively working on a different patch
with the same goal in mind. Perhaps we will circle back and also land this one
(or a variant of it).