Cleanup unnecessary code in canvas_skia

Cleanup unnecessary code in canvas_skia

* Removed AdjustStringDirection()
* Removed FORCE_XXXX_DIRECTIONALITY and replace with TEXT_ALIGN_TO_HEAD. This is necessary not to use Canvas::DefaultCanvasTextAlignment for labels.
* Also removed the safe guard in SizeStringFloat because RenderTextWin has been removed.

BUG=None
TEST=No functional change. Removed LabelTest, DirectionalityFromText that no longer makes sense.
R=msw@chromium.org

Committed: https://crrev.com/7410ba7817276da208e2794bae934bebbc8e9ab8
Cr-Commit-Position: refs/heads/master@{#319166}

[oshima, 2015-02-25 02:06:39]

Patchset #1 (id:1) has been deleted

[oshima, 2015-02-25 02:06:44]

Patchset #1 (id:20001) has been deleted

[oshima, 2015-02-25 02:06:48]

Patchset #1 (id:40001) has been deleted

[oshima, 2015-02-25 02:06:53]

Patchset #1 (id:60001) has been deleted

[msw, 2015-02-25 17:05:20]

https://codereview.chromium.org/953093003/diff/80001/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

https://codereview.chromium.org/953093003/diff/80001/ui/gfx/canvas.h#newcode43
ui/gfx/canvas.h:43: // Specifies the alignment for text rendered with the
DrawStringRect method.
nit: move this comment inside the enum to be grouped with TEXT_ALIGN_*.

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

https://codereview.chromium.org/953093003/diff/80001/ui/gfx/canvas_skia.cc#ol...
ui/gfx/canvas_skia.cc:204: if (adjusted_text.length() >= kMaxRenderTextLength) {
Did you test some very long strings here? (up to 10k chars)

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

https://codereview.chromium.org/953093003/diff/80001/ui/gfx/canvas_skia.cc#ne...
ui/gfx/canvas_skia.cc:142: ElideRectangleText(text, font_list,
nit: update line wrapping.

https://codereview.chromium.org/953093003/diff/80001/ui/gfx/canvas_skia.cc#ne...
ui/gfx/canvas_skia.cc:329: // align with content directionality instead of
fading both ends.
nit: Align

https://codereview.chromium.org/953093003/diff/80001/ui/gfx/canvas_skia.cc#ne...
ui/gfx/canvas_skia.cc:330: flags &= ~TEXT_ALIGN_CENTER;
nit: refactor the code to unconditionally prevent center fading:
  // Align with content directionality instead of fading both ends.
  flags &= ~TEXT_ALIGN_CENTER;
  if (!(flags & (TEXT_ALIGN_LEFT | TEXT_ALIGN_RIGHT)))
    flags |= TEXT_ALIGN_TO_HEAD;

[msw, 2015-02-25 23:27:20]

msw@chromium.org changed reviewers:
+ mukai@chromium.org

[msw, 2015-02-25 23:27:21]

+mukai, the Label rewrite uses this, I think...

[msw, 2015-02-25 23:49:45]

On 2015/02/25 23:27:21, msw wrote:
> +mukai, the Label rewrite uses this, I think...

Nevermind, I mixed up gfx::DIRECTIONALITY_FORCE_RTL and
gfx::Canvas::FORCE_RTL_DIRECTIONALITY!

[oshima, 2015-03-04 08:01:50]

https://codereview.chromium.org/953093003/diff/80001/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

https://codereview.chromium.org/953093003/diff/80001/ui/gfx/canvas.h#newcode43
ui/gfx/canvas.h:43: // Specifies the alignment for text rendered with the
DrawStringRect method.
On 2015/02/25 17:05:20, msw wrote:
> nit: move this comment inside the enum to be grouped with TEXT_ALIGN_*.

Done.

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

https://codereview.chromium.org/953093003/diff/80001/ui/gfx/canvas_skia.cc#ol...
ui/gfx/canvas_skia.cc:204: if (adjusted_text.length() >= kMaxRenderTextLength) {
On 2015/02/25 17:05:20, msw wrote:
> Did you test some very long strings here? (up to 10k chars)

RenderTextWin no longer exists, so E_INVALIDARG can't happen, can it? 

or do you want me to add test to canvas_unittest? (There is
RenderTexTest.StringSizeLongStrings for RT itself)

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

https://codereview.chromium.org/953093003/diff/80001/ui/gfx/canvas_skia.cc#ne...
ui/gfx/canvas_skia.cc:142: ElideRectangleText(text, font_list,
On 2015/02/25 17:05:20, msw wrote:
> nit: update line wrapping.

Done.

https://codereview.chromium.org/953093003/diff/80001/ui/gfx/canvas_skia.cc#ne...
ui/gfx/canvas_skia.cc:329: // align with content directionality instead of
fading both ends.
On 2015/02/25 17:05:20, msw wrote:
> nit: Align

Done.

https://codereview.chromium.org/953093003/diff/80001/ui/gfx/canvas_skia.cc#ne...
ui/gfx/canvas_skia.cc:330: flags &= ~TEXT_ALIGN_CENTER;
On 2015/02/25 17:05:20, msw wrote:
> nit: refactor the code to unconditionally prevent center fading:
>   // Align with content directionality instead of fading both ends.
>   flags &= ~TEXT_ALIGN_CENTER;
>   if (!(flags & (TEXT_ALIGN_LEFT | TEXT_ALIGN_RIGHT)))
>     flags |= TEXT_ALIGN_TO_HEAD;

Done.

[msw, 2015-03-04 17:09:36]

lgtm with minor comments.

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

https://codereview.chromium.org/953093003/diff/80001/ui/gfx/canvas_skia.cc#ol...
ui/gfx/canvas_skia.cc:203: const size_t kMaxRenderTextLength = 5000;
You should update the comment in text_elider.cc referencing this value.
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/text_elider...

https://codereview.chromium.org/953093003/diff/80001/ui/gfx/canvas_skia.cc#ol...
ui/gfx/canvas_skia.cc:204: if (adjusted_text.length() >= kMaxRenderTextLength) {
On 2015/03/04 08:01:50, oshima wrote:
> On 2015/02/25 17:05:20, msw wrote:
> > Did you test some very long strings here? (up to 10k chars)
> 
> RenderTextWin no longer exists, so E_INVALIDARG can't happen, can it? 
> 
> or do you want me to add test to canvas_unittest? (There is
> RenderTexTest.StringSizeLongStrings for RT itself)

This was unconditionally clamping at 5k chars, even for RTHB, but now this will
pass a string of unbounded length to RenderText. RTHB should truncate at
kMaxTextLength = 10000, and StringSizeLongStrings seems to test this
functionality sufficiently, so I think this will be okay. I just thought it
might be worth testing in practice to ensure we don't hit any crash/hang/jank,
and I wonder if the value of determining the precise size of a string longer
than 5k-10k chars here outwieghs any performance degradation that might occur.

[oshima, 2015-03-04 20:17:42]

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

https://codereview.chromium.org/953093003/diff/80001/ui/gfx/canvas_skia.cc#ol...
ui/gfx/canvas_skia.cc:204: if (adjusted_text.length() >= kMaxRenderTextLength) {
On 2015/03/04 17:09:36, msw wrote:
> On 2015/03/04 08:01:50, oshima wrote:
> > On 2015/02/25 17:05:20, msw wrote:
> > > Did you test some very long strings here? (up to 10k chars)
> > 
> > RenderTextWin no longer exists, so E_INVALIDARG can't happen, can it? 
> > 
> > or do you want me to add test to canvas_unittest? (There is
> > RenderTexTest.StringSizeLongStrings for RT itself)
> 
> This was unconditionally clamping at 5k chars, even for RTHB, but now this
will
> pass a string of unbounded length to RenderText. RTHB should truncate at
> kMaxTextLength = 10000, and StringSizeLongStrings seems to test this
> functionality sufficiently, so I think this will be okay. I just thought it
> might be worth testing in practice to ensure we don't hit any crash/hang/jank,
> and I wonder if the value of determining the precise size of a string longer
> than 5k-10k chars here outwieghs any performance degradation that might occur.

In the omibox, which uses TextField that uses RT directly, I tested the long
text
(that I can see elide "..." in omnibox) and I didn't see big performance issue.

[oshima, 2015-03-04 20:17:46]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2015-03-04 20:26:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/953093003/120001

[commit-bot: I haz the power, 2015-03-04 20:47:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-04 20:47:10]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[oshima, 2015-03-04 21:42:22]

oshima@chromium.org changed reviewers:
+ sky@chromium.org

[oshima, 2015-03-04 21:42:22]

sky@ -> ui/views

[sky, 2015-03-04 22:45:25]

LGTM

[Jun Mukai, 2015-03-04 23:14:17]

lgtm

[oshima, 2015-03-04 23:15:20]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2015-03-04 23:16:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/953093003/120001

[commit-bot: I haz the power, 2015-03-05 00:09:03]

Committed patchset #3 (id:120001)

[commit-bot: I haz the power, 2015-03-05 00:09:37]

Patchset 3 (id:??) landed as
https://crrev.com/7410ba7817276da208e2794bae934bebbc8e9ab8
Cr-Commit-Position: refs/heads/master@{#319166}