Remove unused custom vertical text alignment in CanvasSkia.

Remove unused custom vertical text alignment in CanvasSkia.

This feature is unused in production code.
Nix gfx::Canvas::TEXT_VALIGN_[TOP|MIDDLE|BOTTOM] flags.
Simplify CanvasSkia vertical aligment; inline VAlignText.
Simplify pango_util; inline AdjustTextRectBasedOnLayout.
Remove TextExample vertical alignment combobox, etc.

BUG=none
TEST=No behavior changes.
R=sky@chromium.org,asvitkine@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=166126

[msw, 2012-11-03 04:42:43]

Hey Scott and Alexei, please take a look; thanks!

[sky, 2012-11-05 15:25:36]

LGTM

[Alexei Svitkine (slow), 2012-11-05 15:44:42]

I'm a bit mixed on this.

On one hand, it's great to delete dead code (and please change your CL title to
say "Delete" not "Deprecate", since "Deprecate" usually means keeping the code
for a while but marking it as deprecated).

But I also think that having vertical alignment is a pretty basic functionality
that API users will want, so I'm weary of removing it now, only causing someone
down the line to re-add it (possibly coded in a different way), when adding a
feature that needs it.

With only middle alignment, it's difficult to position the text precisely, since
the metrics of the string (e.g. height) wouldn't be known until fallback fonts
are determined. Using top or bottom alignment allows you to "pin" your text to
something above or below it - and you can just give it a rect that's
sufficiently tall and get a good result. With middle alignment only, it becomes
more difficult.

I guess I'll defer to sky on this one.

http://codereview.chromium.org/11362070/diff/7001/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (left):

http://codereview.chromium.org/11362070/diff/7001/ui/gfx/canvas_skia.cc#oldco...
ui/gfx/canvas_skia.cc:267: //                  http://crbug.com/107357
Last time I attempted to remove this block, it caused a regression with the SSL
settings bubble on Windows with the paragraph that described the current
connection (I believe that bubble has now been changed to have that paragraph in
a 2nd tab inside the bubble).

Can you verify that your ifdef added below doesn't cause a regression? (From
reading the code, it looks correct - so just pointing out a way to test this, if
you haven't seen already.)

http://codereview.chromium.org/11362070/diff/7001/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (right):

http://codereview.chromium.org/11362070/diff/7001/ui/gfx/canvas_skia.cc#newco...
ui/gfx/canvas_skia.cc:447: const int line_height =
render_text->GetStringSize().height();
Nit: This block is the same as above. Can you make a helper function?

[sky, 2012-11-05 15:59:49]

I see your point, but if we're only using vertical I don't see a
compelling reason to keep the others.

  -Scott

On Mon, Nov 5, 2012 at 7:44 AM,  <asvitkine@chromium.org> wrote:
> I'm a bit mixed on this.
>
> On one hand, it's great to delete dead code (and please change your CL title
> to
> say "Delete" not "Deprecate", since "Deprecate" usually means keeping the
> code
> for a while but marking it as deprecated).
>
> But I also think that having vertical alignment is a pretty basic
> functionality
> that API users will want, so I'm weary of removing it now, only causing
> someone
> down the line to re-add it (possibly coded in a different way), when adding
> a
> feature that needs it.
>
> With only middle alignment, it's difficult to position the text precisely,
> since
> the metrics of the string (e.g. height) wouldn't be known until fallback
> fonts
> are determined. Using top or bottom alignment allows you to "pin" your text
> to
> something above or below it - and you can just give it a rect that's
> sufficiently tall and get a good result. With middle alignment only, it
> becomes
> more difficult.
>
> I guess I'll defer to sky on this one.
>
>
>
>
>
>
> http://codereview.chromium.org/11362070/diff/7001/ui/gfx/canvas_skia.cc
> File ui/gfx/canvas_skia.cc (left):
>
>
http://codereview.chromium.org/11362070/diff/7001/ui/gfx/canvas_skia.cc#oldco...
> ui/gfx/canvas_skia.cc:267: //                  http://crbug.com/107357
> Last time I attempted to remove this block, it caused a regression with
> the SSL settings bubble on Windows with the paragraph that described the
> current connection (I believe that bubble has now been changed to have
> that paragraph in a 2nd tab inside the bubble).
>
> Can you verify that your ifdef added below doesn't cause a regression?
> (From reading the code, it looks correct - so just pointing out a way to
> test this, if you haven't seen already.)
>
> http://codereview.chromium.org/11362070/diff/7001/ui/gfx/canvas_skia.cc
> File ui/gfx/canvas_skia.cc (right):
>
>
http://codereview.chromium.org/11362070/diff/7001/ui/gfx/canvas_skia.cc#newco...
> ui/gfx/canvas_skia.cc:447: const int line_height =
> render_text->GetStringSize().height();
> Nit: This block is the same as above. Can you make a helper function?
>
> http://codereview.chromium.org/11362070/

[msw, 2012-11-05 22:40:50]

I hope RenderText will ultimately handle vertical alignment (with
top/center/bottom settings as necessary) along with multi-line text rendering,
so I think this is a good step in that direction.

https://codereview.chromium.org/11362070/diff/7001/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (left):

https://codereview.chromium.org/11362070/diff/7001/ui/gfx/canvas_skia.cc#oldc...
ui/gfx/canvas_skia.cc:267: //                  http://crbug.com/107357
On 2012/11/05 15:44:42, Alexei Svitkine wrote:
> Last time I attempted to remove this block, it caused a regression with the
SSL
> settings bubble on Windows with the paragraph that described the current
> connection (I believe that bubble has now been changed to have that paragraph
in
> a 2nd tab inside the bubble).
> 
> Can you verify that your ifdef added below doesn't cause a regression? (From
> reading the code, it looks correct - so just pointing out a way to test this,
if
> you haven't seen already.)

The website settings bubble's connection tab doesn't change on Win/CrOS.

https://codereview.chromium.org/11362070/diff/7001/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (right):

https://codereview.chromium.org/11362070/diff/7001/ui/gfx/canvas_skia.cc#newc...
ui/gfx/canvas_skia.cc:447: const int line_height =
render_text->GetStringSize().height();
On 2012/11/05 15:44:42, Alexei Svitkine wrote:
> Nit: This block is the same as above. Can you make a helper function?

I'd rather follow up with bigger vertical placement changes.

[commit-bot: I haz the power, 2012-11-05 22:41:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/11362070/7001

[commit-bot: I haz the power, 2012-11-05 23:58:28]

Failed to apply patch for ui/views/controls/menu/menu_item_view.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file ui/views/controls/menu/menu_item_view.cc
  Hunk #1 FAILED at 741.
  1 out of 1 hunk FAILED -- saving rejects to file
ui/views/controls/menu/menu_item_view.cc.rej

Patch:       ui/views/controls/menu/menu_item_view.cc
Index: ui/views/controls/menu/menu_item_view.cc
diff --git a/ui/views/controls/menu/menu_item_view.cc
b/ui/views/controls/menu/menu_item_view.cc
index
98df77b8792fe1d6ef2834a933964818defb3818..30b955faf3df71193f9b8676bbf4cb439845b90a
100644
--- a/ui/views/controls/menu/menu_item_view.cc
+++ b/ui/views/controls/menu/menu_item_view.cc
@@ -741,8 +741,7 @@ void MenuItemView::PaintAccelerator(gfx::Canvas* canvas) {
   gfx::Rect accel_bounds(width() - accel_right_margin - max_accel_width,
                          GetTopMargin(), max_accel_width, available_height);
   accel_bounds.set_x(GetMirroredXForRect(accel_bounds));
-  int flags = GetRootMenuItem()->GetDrawStringFlags() |
-      gfx::Canvas::TEXT_VALIGN_MIDDLE;
+  int flags = GetRootMenuItem()->GetDrawStringFlags();
   flags &= ~(gfx::Canvas::TEXT_ALIGN_RIGHT | gfx::Canvas::TEXT_ALIGN_LEFT);
   if (base::i18n::IsRTL())
     flags |= gfx::Canvas::TEXT_ALIGN_LEFT;

[commit-bot: I haz the power, 2012-11-06 00:09:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/11362070/11002

[commit-bot: I haz the power, 2012-11-06 03:34:14]

Change committed as 166126