Track the region where text is painted.

Track the region where text is painted.
BUG=181245
R=bungeman@chromium.org, eseidel@chromium.org

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

[alokp, 2013-04-18 20:51:27]

Plumb text-rect (a loose rect-bound for text which is already available to
Blink) all the way down to PlatformContextSkia. Currently it is only being used
to track the text region.

Next step is to:
1. visualize the text region for debugging
2. pass it down to skia for quick-reject/fast-clipping

[eseidel, 2013-04-18 21:04:38]

Seems OK.  It's unfortunate that this has to pipe this arg everywhere.  I wonder
if we shouldn't bite the bullet and create  TextRun wrapper or some other object
to encapsulate these common args.

I like the direction of exposing more of this to Skia.  This adds a new class of
bugs (where these rects don't match the drawn text).

If we had 5 patches of this type, the text drawing code would be unwieldy.  So
we're definitely near the point where we need to start thinking in better
draw-request/arg-wrapper abstractions. :)

https://codereview.chromium.org/14160005/diff/4001/Source/WebKit/chromium/src...
File Source/WebKit/chromium/src/WebFontImpl.cpp (right):

https://codereview.chromium.org/14160005/diff/4001/Source/WebKit/chromium/src...
Source/WebKit/chromium/src/WebFontImpl.cpp:105: m_font.drawText(&gc, run,
leftBaseline, WebCore::FloatRect(clip), from, to);
nit: Seems like a local would be useful.  We do this FloatRect convert twice.

https://codereview.chromium.org/14160005/diff/4001/Source/core/platform/graph...
File Source/core/platform/graphics/Font.h (right):

https://codereview.chromium.org/14160005/diff/4001/Source/core/platform/graph...
Source/core/platform/graphics/Font.h:180: void
drawEmphasisMarks(GraphicsContext*, const TextRun&, const GlyphBuffer&, const
AtomicString&, const FloatPoint&, const FloatRect& textRect) const;
The rest of these arguments would benifit from clearer names.  Not that you need
to do that in this patch, just noting.

https://codereview.chromium.org/14160005/diff/4001/Source/core/platform/graph...
File Source/core/platform/graphics/skia/PlatformContextSkia.cpp (right):

https://codereview.chromium.org/14160005/diff/4001/Source/core/platform/graph...
Source/core/platform/graphics/skia/PlatformContextSkia.cpp:622:
m_textRegion.join(textRect);
I hope this isn't a WebCore::Region (which is abysmally slow)

https://codereview.chromium.org/14160005/diff/4001/Source/core/platform/graph...
File Source/core/platform/graphics/skia/PlatformContextSkia.h (right):

https://codereview.chromium.org/14160005/diff/4001/Source/core/platform/graph...
Source/core/platform/graphics/skia/PlatformContextSkia.h:496:
trackTextRegion(textRect);
Do we ever stop tracking?  Should this be didDrawTextInRect? or similar?

https://codereview.chromium.org/14160005/diff/4001/Source/core/platform/graph...
Source/core/platform/graphics/skia/PlatformContextSkia.h:526:
trackTextRegion(textRect);
It's not clear to me that "textRect" is the right name.  If I pass the wrong
rect, what will happen?  Should this be textClipRect?  Will the rect actually
clip?

This introduces a whole new class of bugs (rect mismatches) to our text system,
but I suspect that the perf wins are definitely worth the added complexity.

[alokp, 2013-04-18 22:22:37]

I like the idea of TextRun wrapper. Can you elaborate on
draw-request/arg-wrapper abstractions?

Ideas to further improve the state of text-drawing code:
- Delete obsolete non-skia path for drawing text.
- Consolidate duplicated code across various platforms for skia path.

https://codereview.chromium.org/14160005/diff/4001/Source/WebKit/chromium/src...
File Source/WebKit/chromium/src/WebFontImpl.cpp (right):

https://codereview.chromium.org/14160005/diff/4001/Source/WebKit/chromium/src...
Source/WebKit/chromium/src/WebFontImpl.cpp:105: m_font.drawText(&gc, run,
leftBaseline, WebCore::FloatRect(clip), from, to);
On 2013/04/18 21:04:38, Eric Seidel (Google) wrote:
> nit: Seems like a local would be useful.  We do this FloatRect convert twice.

Done.

https://codereview.chromium.org/14160005/diff/4001/Source/core/platform/graph...
File Source/core/platform/graphics/Font.h (right):

https://codereview.chromium.org/14160005/diff/4001/Source/core/platform/graph...
Source/core/platform/graphics/Font.h:180: void
drawEmphasisMarks(GraphicsContext*, const TextRun&, const GlyphBuffer&, const
AtomicString&, const FloatPoint&, const FloatRect& textRect) const;
Will do in a separate patch.

https://codereview.chromium.org/14160005/diff/4001/Source/core/platform/graph...
File Source/core/platform/graphics/skia/PlatformContextSkia.cpp (right):

https://codereview.chromium.org/14160005/diff/4001/Source/core/platform/graph...
Source/core/platform/graphics/skia/PlatformContextSkia.cpp:622:
m_textRegion.join(textRect);
Not WebCore::Region. It is SkRect (for now). I may need a tighter low-cost
region in future, so keeping the name generic.

https://codereview.chromium.org/14160005/diff/4001/Source/core/platform/graph...
File Source/core/platform/graphics/skia/PlatformContextSkia.h (right):

https://codereview.chromium.org/14160005/diff/4001/Source/core/platform/graph...
Source/core/platform/graphics/skia/PlatformContextSkia.h:496:
trackTextRegion(textRect);
I like didDrawTextInRect. Changed.

https://codereview.chromium.org/14160005/diff/4001/Source/core/platform/graph...
Source/core/platform/graphics/skia/PlatformContextSkia.h:526:
trackTextRegion(textRect);
On 2013/04/18 21:04:38, Eric Seidel (Google) wrote:
> It's not clear to me that "textRect" is the right name.  If I pass the wrong
> rect, what will happen?  Should this be textClipRect?  Will the rect actually
> clip?

It does not clip right now. The plan is to use for quick-reject and tracking
text-region for LCD text.

> This introduces a whole new class of bugs (rect mismatches) to our text
system,

Yes. That is a concern. But I guess those bugs will be quite obvious - the text
will be rejected. Moreover in most cases we are reusing the text-rect computed
during layout, so mismatched rects may not be a big problem.

[alokp, 2013-04-26 16:37:08]

eseidel: Would you like to see the TextRun wrapper done in this patch?

bungeman: Could you take a look as well? Thanks.

[bungeman-chromium, 2013-04-26 16:58:29]

On 2013/04/26 16:37:08, Alok Priyadarshi wrote:
> eseidel: Would you like to see the TextRun wrapper done in this patch?
> 
> bungeman: Could you take a look as well? Thanks.

With all this plumbing going through, it would be nice if this were a struct of
some sort that we could modify so we don't need to do this plumbing again.
Something like a TextRunExtraData.

The case that I'm thinking of right off the top of my head is for text shaping
with a glyph<==>char mapping. PDF has been wanting this for some time (so that
it can write out a PDF from which one can reliably copy-paste), and I think it
didn't happen because of this sort of pluming effort.

[alokp, 2013-04-30 23:44:58]

I added TextRunPaintInfo, a wrapper for TextRun, which is plumbed up to the Font
class.

Note that this only compiles on linux for now as I have not updated the windows
and mac specific code to incorporate the new wrapper class.

Eric/Ben: Please see if you had something similar in mind.

[alokp, 2013-05-06 18:50:05]

Eric/Ben: Ping!

[bungeman-chromium, 2013-05-06 19:30:43]

https://codereview.chromium.org/14160005/diff/19001/Source/core/platform/grap...
File Source/core/platform/graphics/Font.h (right):

https://codereview.chromium.org/14160005/diff/19001/Source/core/platform/grap...
Source/core/platform/graphics/Font.h:179: void drawGlyphs(GraphicsContext*,
const SimpleFontData*, const GlyphBuffer&, int from, int to, const FloatPoint&,
const FloatRect& textRect) const;
Any reason why this is just a FloatRect?

[alokp, 2013-05-06 20:39:54]

https://codereview.chromium.org/14160005/diff/19001/Source/core/platform/grap...
File Source/core/platform/graphics/Font.h (right):

https://codereview.chromium.org/14160005/diff/19001/Source/core/platform/grap...
Source/core/platform/graphics/Font.h:179: void drawGlyphs(GraphicsContext*,
const SimpleFontData*, const GlyphBuffer&, int from, int to, const FloatPoint&,
const FloatRect& textRect) const;
On 2013/05/06 19:30:43, bungeman2 wrote:
> Any reason why this is just a FloatRect?

I do not understand. If you mean why drawGlyphs does not use TextRunPaintInfo,
it is because the parameters for this function refers to the GlyphBuffer and not
TextRun.

[bungeman-chromium, 2013-05-06 21:02:31]

I don't want to hold this up just for something were not working on right at
this moment, and this does look much better than what we had before. If this
meets all the original needs, lgtm.

https://codereview.chromium.org/14160005/diff/19001/Source/core/platform/grap...
File Source/core/platform/graphics/Font.h (right):

https://codereview.chromium.org/14160005/diff/19001/Source/core/platform/grap...
Source/core/platform/graphics/Font.h:179: void drawGlyphs(GraphicsContext*,
const SimpleFontData*, const GlyphBuffer&, int from, int to, const FloatPoint&,
const FloatRect& textRect) const;
On 2013/05/06 20:39:54, Alok Priyadarshi wrote:
> On 2013/05/06 19:30:43, bungeman2 wrote:
> > Any reason why this is just a FloatRect?
> 
> I do not understand. If you mean why drawGlyphs does not use TextRunPaintInfo,
> it is because the parameters for this function refers to the GlyphBuffer and
not
> TextRun.

Hmmm... I see. The use case I had in mind meant passing this sort of information
potentially all the way down to Skia. This would allow the knowledge of glyph =>
character mappings to be pushed all the way down to the drawText call so that
PDF/XPS could save the mapping. Just passing a FloatRect here somewhat defeats
the purpose of the suggestion. Perhaps such a use case needs to be plumbed in
some other way (potentially somehow adding such information to the GlyphBuffer
directly).

On the other hand perhaps this isn't an impossible issue as this is just the
simple text path, the complex text path does get the full info.

[alokp, 2013-05-06 22:12:34]

eseidel: does it look any better?

https://codereview.chromium.org/14160005/diff/19001/Source/core/platform/grap...
File Source/core/platform/graphics/Font.h (right):

https://codereview.chromium.org/14160005/diff/19001/Source/core/platform/grap...
Source/core/platform/graphics/Font.h:179: void drawGlyphs(GraphicsContext*,
const SimpleFontData*, const GlyphBuffer&, int from, int to, const FloatPoint&,
const FloatRect& textRect) const;
On 2013/05/06 21:02:31, bungeman2 wrote:
> On 2013/05/06 20:39:54, Alok Priyadarshi wrote:
> > On 2013/05/06 19:30:43, bungeman2 wrote:
> > > Any reason why this is just a FloatRect?
> > 
> > I do not understand. If you mean why drawGlyphs does not use
TextRunPaintInfo,
> > it is because the parameters for this function refers to the GlyphBuffer and
> not
> > TextRun.
> 
> Hmmm... I see. The use case I had in mind meant passing this sort of
information
> potentially all the way down to Skia. This would allow the knowledge of glyph
=>
> character mappings to be pushed all the way down to the drawText call so that
> PDF/XPS could save the mapping. Just passing a FloatRect here somewhat defeats
> the purpose of the suggestion. Perhaps such a use case needs to be plumbed in
> some other way (potentially somehow adding such information to the GlyphBuffer
> directly).

Yes. Extra information could either be added directly to GlyphBuffer or to a
GlyphBuffer wrapper. The textRect parameter could then be moved to GlyphBuffer
wrapper.
 
> On the other hand perhaps this isn't an impossible issue as this is just the
> simple text path, the complex text path does get the full info.

[eseidel, 2013-05-08 19:32:00]

lgtm

Seems totally reasonable to me.

[commit-bot: I haz the power, 2013-05-08 19:32:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alokp@chromium.org/14160005/19001

[commit-bot: I haz the power, 2013-05-08 19:32:15]

Failed to apply patch for Source/WebKit/chromium/src/WebFontImpl.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/WebKit/chromium/src/WebFontImpl.cpp
  Hunk #1 FAILED at 94.
  Hunk #2 succeeded at 98 (offset -3 lines).
  1 out of 2 hunks FAILED -- saving rejects to file
Source/WebKit/chromium/src/WebFontImpl.cpp.rej

Patch:       Source/WebKit/chromium/src/WebFontImpl.cpp
Index: Source/WebKit/chromium/src/WebFontImpl.cpp
diff --git a/Source/WebKit/chromium/src/WebFontImpl.cpp
b/Source/WebKit/chromium/src/WebFontImpl.cpp
index
6797602de90760fea8f4ffc9ca7c54039ccf2fe3..f14ac2691f02878644dcfb9cdd97dcd9c3c66b73
100644
--- a/Source/WebKit/chromium/src/WebFontImpl.cpp
+++ b/Source/WebKit/chromium/src/WebFontImpl.cpp
@@ -94,6 +94,11 @@ void WebFontImpl::drawText(WebCanvas* canvas, const
WebTextRun& run, const WebFl
                            int from, int to) const
 {
     FontCachePurgePreventer fontCachePurgePreventer;
+    WebCore::FloatRect textClipRect(clip);
+    TextRunPaintInfo runInfo(run);
+    runInfo.from = from;
+    runInfo.to = to;
+    runInfo.bounds = textClipRect;
 
     GraphicsContextBuilder builder(canvas);
     GraphicsContext& gc = builder.context();
@@ -101,8 +106,8 @@ void WebFontImpl::drawText(WebCanvas* canvas, const
WebTextRun& run, const WebFl
     gc.save();
     gc.setShouldSmoothFonts(canvasIsOpaque);
     gc.setFillColor(color, ColorSpaceDeviceRGB);
-    gc.clip(WebCore::FloatRect(clip));
-    m_font.drawText(&gc, run, leftBaseline, from, to);
+    gc.clip(textClipRect);
+    m_font.drawText(&gc, runInfo, leftBaseline);
     gc.restore();
 
 #if defined(WIN32)

[commit-bot: I haz the power, 2013-05-10 02:26:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alokp@chromium.org/14160005/34001

[commit-bot: I haz the power, 2013-05-10 08:39:21]

Retried try job too often on mac_layout for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[alokp, 2013-05-10 16:22:29]

Committed patchset #6 manually as r150096 (presubmit successful).