Deprecate unused custom vertical text alignment in CanvasSkia. (issue 11362070)

[m...@chromium.org]

Reviewers: sky, Alexei Svitkine,

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

Description:
Deprecate 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=s...@chromium.org,asvi...@chromium.org

Please review this at https://codereview.chromium.org/11362070/

SVN Base: svn://svn.chromium.org/chrome/trunk/src

Affected files:
M ash/shell/app_list.cc
M ui/gfx/canvas.h
M ui/gfx/canvas_skia.cc
M ui/gfx/pango_util.h
M ui/gfx/pango_util.cc
M ui/views/controls/menu/menu_item_view.cc
M ui/views/controls/menu/menu_item_view_views.cc
M ui/views/controls/menu/menu_item_view_win.cc
M ui/views/examples/text_example.h
M ui/views/examples/text_example.cc

[s...@chromium.org]

LGTM

https://codereview.chromium.org/11362070/

[asvi...@chromium.org]

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#oldcode267
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#newcode447
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/

[Scott Violet]

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

-Scott