Make StyledLabel wrap long words

Make StyledLabel wrap long words

This fixes StyleLabel layout in languages that don't have word breaks
(e.g. Japanese).

BUG=479859
Committed: https://crrev.com/49236cbd96cda78df70d3f00c9ababcacf9bbcc8
Cr-Commit-Position: refs/heads/master@{#330807}

[tbarzic, 2015-05-13 18:24:22]

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

[msw, 2015-05-13 23:05:40]

Can you get an author to review? I don't know this code very well.

https://codereview.chromium.org/1140083002/diff/1/ui/views/controls/styled_la...
File ui/views/controls/styled_label.cc (right):

https://codereview.chromium.org/1140083002/diff/1/ui/views/controls/styled_la...
ui/views/controls/styled_label.cc:284: gfx::TRUNCATE_LONG_WORDS,
Why doesn't this use WRAP_LONG_WORDS?

https://codereview.chromium.org/1140083002/diff/1/ui/views/controls/styled_la...
ui/views/controls/styled_label.cc:289: if (chunk.empty()) {
nit: declare/assign |chunk| below this block and change this to check if
(substrings.empty() || substrings[0].empty())

https://codereview.chromium.org/1140083002/diff/1/ui/views/controls/styled_la...
ui/views/controls/styled_label.cc:363: if (substrings.size() > 1 &&
whole_chunk_used) {
I don't think you need |whole_chunk_used|, can you just check chunk.size() <
substrings[0].size(). Also, can you add a comment here? The existing code is
hard to follow, and any clarification will help.

https://codereview.chromium.org/1140083002/diff/1/ui/views/controls/styled_la...
File ui/views/controls/styled_label_unittest.cc (right):

https://codereview.chromium.org/1140083002/diff/1/ui/views/controls/styled_la...
ui/views/controls/styled_label_unittest.cc:128: const std::string
text("Thisistextasasingleword");
nit: camel case words :)

https://codereview.chromium.org/1140083002/diff/1/ui/views/controls/styled_la...
ui/views/controls/styled_label_unittest.cc:135:
styled()->SetBorder(Border::CreateEmptyBorder(3, 3, 3, 3));
Is this 3px border necessary? Would no border simplify the test?

https://codereview.chromium.org/1140083002/diff/1/ui/views/controls/styled_la...
ui/views/controls/styled_label_unittest.cc:141: EXPECT_EQ(3,
styled()->child_at(0)->x());
nit: check if the origin is equal to gfx::Point()

[tbarzic, 2015-05-14 00:02:28]

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

[tbarzic, 2015-05-14 00:02:29]

also, adding sky

https://codereview.chromium.org/1140083002/diff/1/ui/views/controls/styled_la...
File ui/views/controls/styled_label.cc (right):

https://codereview.chromium.org/1140083002/diff/1/ui/views/controls/styled_la...
ui/views/controls/styled_label.cc:284: gfx::TRUNCATE_LONG_WORDS,
On 2015/05/13 23:05:39, msw wrote:
> Why doesn't this use WRAP_LONG_WORDS?

WRAP_LONG_WORDS didn't work in first patch because it always requires that at
least one character remains on the first line (should be fine now though, since
we go to the next line when whole first line gets used).

https://codereview.chromium.org/1140083002/diff/1/ui/views/controls/styled_la...
ui/views/controls/styled_label.cc:289: if (chunk.empty()) {
On 2015/05/13 23:05:39, msw wrote:
> nit: declare/assign |chunk| below this block and change this to check if
> (substrings.empty() || substrings[0].empty())

Done.

https://codereview.chromium.org/1140083002/diff/1/ui/views/controls/styled_la...
ui/views/controls/styled_label.cc:363: if (substrings.size() > 1 &&
whole_chunk_used) {
On 2015/05/13 23:05:39, msw wrote:
> I don't think you need |whole_chunk_used|, can you just check chunk.size() <
> substrings[0].size(). Also, can you add a comment here? The existing code is
> hard to follow, and any clarification will help.

Done.

https://codereview.chromium.org/1140083002/diff/1/ui/views/controls/styled_la...
File ui/views/controls/styled_label_unittest.cc (right):

https://codereview.chromium.org/1140083002/diff/1/ui/views/controls/styled_la...
ui/views/controls/styled_label_unittest.cc:128: const std::string
text("Thisistextasasingleword");
On 2015/05/13 23:05:39, msw wrote:
> nit: camel case words :)

Done.

https://codereview.chromium.org/1140083002/diff/1/ui/views/controls/styled_la...
ui/views/controls/styled_label_unittest.cc:135:
styled()->SetBorder(Border::CreateEmptyBorder(3, 3, 3, 3));
On 2015/05/13 23:05:40, msw wrote:
> Is this 3px border necessary? Would no border simplify the test?

wouldn't really simplify it, but it's not essential either.

https://codereview.chromium.org/1140083002/diff/1/ui/views/controls/styled_la...
ui/views/controls/styled_label_unittest.cc:141: EXPECT_EQ(3,
styled()->child_at(0)->x());
On 2015/05/13 23:05:40, msw wrote:
> nit: check if the origin is equal to gfx::Point()

Done.

[msw, 2015-05-14 00:31:42]

lgtm with a test nit

https://codereview.chromium.org/1140083002/diff/40001/ui/views/controls/style...
File ui/views/controls/styled_label_unittest.cc (right):

https://codereview.chromium.org/1140083002/diff/40001/ui/views/controls/style...
ui/views/controls/styled_label_unittest.cc:144: EXPECT_EQ(styled()->height() /
2, styled()->child_at(1)->height());
nit: maybe check the height for both children or don't bother checking heights
(origins are sufficient to check for two lines)? Actually, testing the precise
layout of the labels seems tangential to this test of wrapping a long word, I
wonder if some checks are unnecessary here.

[tbarzic, 2015-05-14 01:29:25]

https://codereview.chromium.org/1140083002/diff/40001/ui/views/controls/style...
File ui/views/controls/styled_label_unittest.cc (right):

https://codereview.chromium.org/1140083002/diff/40001/ui/views/controls/style...
ui/views/controls/styled_label_unittest.cc:144: EXPECT_EQ(styled()->height() /
2, styled()->child_at(1)->height());
On 2015/05/14 00:31:42, msw wrote:
> nit: maybe check the height for both children or don't bother checking heights
> (origins are sufficient to check for two lines)? Actually, testing the precise
> layout of the labels seems tangential to this test of wrapping a long word, I
> wonder if some checks are unnecessary here.

removed height check.
I left origin check, just to make sure each label is really in its own line
(rather than two labels in the same line)

[msw, 2015-05-14 01:34:08]

lgtm

[tbarzic, 2015-05-19 18:01:48]

On 2015/05/14 01:34:08, msw wrote:
> lgtm

sky: ping

[sky, 2015-05-20 19:43:03]

LGTM - sorry for the delay.

[tbarzic, 2015-05-20 20:09:31]

The CQ bit was checked by tbarzic@chromium.org

[commit-bot: I haz the power, 2015-05-20 20:10:01]

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

[commit-bot: I haz the power, 2015-05-20 21:26:36]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-05-20 21:28:19]

Patchset 4 (id:??) landed as
https://crrev.com/49236cbd96cda78df70d3f00c9ababcacf9bbcc8
Cr-Commit-Position: refs/heads/master@{#330807}