Fix RTL and complex script title in print preview header/footer.

chrome/renderer/print_web_view_helper.cc

Index: chrome/renderer/print_web_view_helper.cc
diff --git a/chrome/renderer/print_web_view_helper.cc b/chrome/renderer/print_web_view_helper.cc
index b31cd851cfe5213c2219515b3dde4681cd01e5f7..06810fa48a8ccebd079f7d854c5f2a037646014b 100644
--- a/chrome/renderer/print_web_view_helper.cc
+++ b/chrome/renderer/print_web_view_helper.cc
@@ -49,6 +49,10 @@
 #include "skia/ext/vector_canvas.h"
 #include "skia/ext/vector_platform_device_skia.h"
 #include "third_party/skia/include/core/SkTypeface.h"
+#if !defined(OS_MACOSX)  // USE_SKIA && !OS_MACOSX
+#include "ui/gfx/canvas.h"
+#include "ui/gfx/render_text.h"
+#endif  // USE_SKIA && !defined(OS_MACOSX)
 #elif defined(OS_MACOSX)
 #include <CoreGraphics/CGContext.h>
 
@@ -406,6 +410,48 @@ void PrintHeaderFooterText(
     printing::VerticalHeaderFooterPosition vertical_position,
     double offset_to_baseline) {
 #if defined(USE_SKIA)
+#if defined(OS_WIN)

[Alexei Svitkine (slow), 2012/05/02 15:04:05]

It might be a good idea to refactor this to a separate function and call it from
PrintHeaderFooterText(). Otherwise, this is making PrintHeaderFooterText() quite
messy.

[arthurhsu, 2012/05/03 00:28:44]

Done.

+  // TODO(arthurhsu): Following code works on Windows only so far.
+  // See crbug.com/108599 and its blockers for more information.
+  scoped_ptr<gfx::RenderText> render_text(gfx::RenderText::CreateRenderText());
+  DCHECK(render_text.get());

[Alexei Svitkine (slow), 2012/05/02 15:04:05]

What does this DCHECK() buy you? If it's NULL, then the next line will just
crash - which you'll notice just as well, if not better than a DCHECK().

[arthurhsu, 2012/05/03 00:28:44]

Done.

+  render_text->SetText(text);
+  int font_size = printing::kSettingHeaderFooterFontSize / webkit_scale_factor;
+  render_text->SetFontSize(font_size);
+  int font_height = render_text->GetStringSize().height() / webkit_scale_factor;

[Alexei Svitkine (slow), 2012/05/02 15:04:05]

It would be more accurate to name this |text_height|, since there may be
multiple fonts involved in a single line of text (multiple runs using different
fonts).

[Alexei Svitkine (slow), 2012/05/02 15:23:58]

Can you explain the webkit_scale_factor?

It seems to me you're getting the metrics from RenderText and modifying them by
this factor. But then, when you go to draw the render text, I don't see this
factor being used. So it seems incorrect to me - since you're drawing in a way
that doesn't take the factor into account, but calculating metrics in a way that
does. Maybe I'm missing something here - can you explain this?

[xji, 2012/05/02 17:44:12]

it re-SetFontSize() based on this factor on line 420.

[arthurhsu, 2012/05/03 00:28:44]

Done.

+  int y_offset = font_height - font_size;
+  SkScalar margin_left = page_layout.margin_left / webkit_scale_factor;
+  SkScalar margin_top = page_layout.margin_top / webkit_scale_factor;
+  SkScalar content_height = page_layout.content_height / webkit_scale_factor;
+
+  int save_count = canvas->save();
+  canvas->translate(-margin_left, -margin_top);
+  scoped_ptr<gfx::Canvas> gfx_canvas(new gfx::Canvas(canvas));

[Alexei Svitkine (slow), 2012/05/01 14:56:39]

I'm wondering whether you've tried using Canvas::DrawString() directly, which
(as of M20), uses RenderText. Might save you the trouble of managing your own
RenderText instance.

[arthurhsu, 2012/05/01 17:26:38]

Actually, I tried not to use it for debugging purposes (since I can easily watch
my own var in debugger, not going deep into statics/other object's members).  As
mentioned in e-mail, we have painting issues on Linux and I tend to have this
as-is for now.

On 2012/05/01 14:56:39, Alexei Svitkine wrote:

[Alexei Svitkine (slow), 2012/05/02 15:04:05]

I suggest moving the gfx_canvas declaration to right before where you actually
use it (after all the calculations - just above the draw call.

[arthurhsu, 2012/05/03 00:28:44]

Done.

+  int text_width = render_text->GetStringSize().width() / webkit_scale_factor;

[Alexei Svitkine (slow), 2012/05/02 15:04:05]

Suggest just calling GetStringSize() once and storing the Size returned as a
local variable. (You're calling it here and above.)

[arthurhsu, 2012/05/03 00:28:44]

Done.

+  SkPoint point = GetHeaderFooterPosition(webkit_scale_factor, page_layout,
+                                          horizontal_position,
+                                          vertical_position, offset_to_baseline,
+                                          SkScalarToDouble(text_width));
+  point.set(point.x() + margin_left, point.y() + margin_top);
+  // Workaround clipping issue of RenderText.
+  if (vertical_position == printing::TOP) {
+    if (point.y() - y_offset + font_height < margin_top + 1) {

[Alexei Svitkine (slow), 2012/05/02 15:23:58]

I think this logic can be written in a way that's less confusing.

It looks like what you're testing for is that the text bottom is outside the
margin. Can you express that logic in a more readable way? (i.e. keeping local
variables that are well named - e.g. text_bottom = point.y() - y_offset +
text_height - although perhaps you should apply y_offset to point.y() before
this, then you can have text_bottom = point.y() + text_height).

[arthurhsu, 2012/05/03 00:28:44]

I've added more comments on this.

On 2012/05/02 15:23:58, Alexei Svitkine wrote:

+      y_offset = point.y() + font_height - margin_top - 1;
+    }
+  } else {  // BOTTOM
+    if (point.y() - y_offset > margin_top + content_height - 1) {
+      y_offset = point.y() - margin_top - content_height + 1;
+    }
+  }
+
+  gfx::Rect rect(point.x(), point.y() - y_offset, text_width, font_height);
+  render_text->SetDisplayRect(rect);
+  render_text->Draw(gfx_canvas.get());
+  gfx_canvas.reset();

[Alexei Svitkine (slow), 2012/05/02 15:04:05]

Is it necessary to explicitly reset it here? You're using a scoped_ptr already,
so it will be freed when going out of scope.

Looking at the gfx::Canvas destructor, it's empty - so I don't think there's any
reason to explicitly reset it.

[arthurhsu, 2012/05/03 00:28:44]

I do not wish to assume that dtor of gfx::Canvas does nothing in the future
(say, it can call restoreToCount()).  Force releasing it guarantees that my
subsequent restoreToCount() does not have consequences.

On 2012/05/02 15:04:05, Alexei Svitkine wrote:

[Alexei Svitkine (slow), 2012/05/03 19:04:41]

If you want to be that paranoid, then I suggest using an inner scope. Also, you
should just stack allocate gfx::Canvas. e.g.:

{
  gfx::Canvas gfx_canvas(canvas);
  render_text->Draw(&gfx_canvas);
}

[arthurhsu, 2012/05/07 17:28:06]

Done.

+  canvas->restoreToCount(save_count);
+#else
+  // TODO(arthurhsu): following code has issues with i18n BiDi, see
+  //                  crbug.com/108599.
   size_t text_byte_length = text.length() * sizeof(char16);
   double text_width_in_points = SkScalarToDouble(paint.measureText(
       text.c_str(), text_byte_length));
@@ -417,6 +463,7 @@ void PrintHeaderFooterText(
       paint.getTextSize() / webkit_scale_factor));
   canvas->drawText(text.c_str(), text_byte_length, point.x(), point.y(),
                    paint);
+#endif  // USE_SKIA && OS_WIN
 #elif defined(OS_MACOSX)
   ScopedCFTypeRef<CFStringRef> cf_text(base::SysUTF16ToCFStringRef(text));
   ScopedCFTypeRef<CFAttributedStringRef> cf_attr_text(