Make styled-label trimming less agressive, allowing whitespace

Make styled-label trimming less aggressive - now when wrapped on \n, only this \n is removed, not all whitespace sequence. This allows whitespace
"formatting" like line indentation with spaces. Also makes double newline behave like expected - insert an empty line between text before and after \n\n.
Syled label layout loop refactored to de-duplicate code.
Added tests coverage for styled label.
Fixed LegalMessageLine comment - no longer have to explain that it is based on styled_label with some caveats.

R=sky@chromium.org
BUG=

Committed: https://crrev.com/933ca2314a2ea20ceafe435bbc896500e67bd7a3
Cr-Commit-Position: refs/heads/master@{#438795}

[eugenebng, 2016-09-22 11:08:54]

ping sky

[sky, 2016-09-22 13:25:20]

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

[sky, 2016-09-22 13:25:21]

+estade as I believe he wanted the whitespace trimming logic. I tend to favor
not trimming *any* whitespace and leaving it up to the caller to trim whitespace
before creating the styledlabel. But that likely breaks some usage, so we may
need a property to give the new behavior.

[eugenebng, 2016-09-23 12:29:50]

On 2016/09/22 13:25:21, sky wrote:
> +estade as I believe he wanted the whitespace trimming logic. I tend to favor
> not trimming *any* whitespace and leaving it up to the caller to trim
whitespace
> before creating the styledlabel. But that likely breaks some usage, so we may
> need a property to give the new behavior.

Regarding not trimming any whitespace, there are two cases in my code in this
CL:
1) When line wrapped by \n, this \n goes to remaining_string. And should be
removed in order that single \n will not cause infinite line wrap cycle.
So that actally means not trimming, because \n being removed was "printed" (line
wrapped), do youy agree on this point?
2) When string doesn't fit into width and wrapped by one whitespace or sequence
of whitespaces, there can be different options: remove all sequence or remove
only one whitespace. I agree that trimming all seqence is more intrusive. 
So I will remove only one whitespace, because for example line "styled label",
when split into 2 lines because of not fitting into label width, should be
["styled" and "label"], but not ["styled" and " label"] (space in the beginning
of the second line). When wrapping line on two spcaces for example, [styled
space space label] will turn into [syled] and [space label] lines - with leading
space on the second line.

Does this conform to what you mean as not trimmimg *any* whitespace?

And I'm really interested in what Evan Stade thinks about it all, but I agree
that this change is likely to be an option/property.

[sky, 2016-09-23 17:51:10]

On Fri, Sep 23, 2016 at 5:29 AM,  <eugenebng@yandex-team.ru> wrote:
> On 2016/09/22 13:25:21, sky wrote:
>> +estade as I believe he wanted the whitespace trimming logic. I tend to
>> favor
>> not trimming *any* whitespace and leaving it up to the caller to trim
> whitespace
>> before creating the styledlabel. But that likely breaks some usage, so we
>> may
>> need a property to give the new behavior.
>
> Regarding not trimming any whitespace, there are two cases in my code in
> this
> CL:
> 1) When line wrapped by \n, this \n goes to remaining_string. And should be
> removed in order that single \n will not cause infinite line wrap cycle.
> So that actally means not trimming, because \n being removed was "printed"
> (line
> wrapped), do youy agree on this point?

Agreed.

> 2) When string doesn't fit into width and wrapped by one whitespace or
> sequence
> of whitespaces, there can be different options: remove all sequence or
> remove
> only one whitespace. I agree that trimming all seqence is more intrusive.
> So I will remove only one whitespace, because for example line "styled
> label",
> when split into 2 lines because of not fitting into label width, should be
> ["styled" and "label"], but not ["styled" and " label"] (space in the
> beginning
> of the second line).

Agreed.

> When wrapping line on two spcaces for example, [styled
> space space label] will turn into [syled] and [space label] lines - with
> leading
> space on the second line.

If we wrap I'm inclined to move white space on both sides. In other
words if you wrapped between 'styled' and 'label' it would look the
same regardless of how much white space was between 'styled' and
'label'.

  -Scott

> Does this conform to what you mean as not trimmimg *any* whitespace?
>
> And I'm really interested in what Evan Stade thinks about it all, but I
> agree
> that this change is likely to be an option/property.
>
> https://codereview.chromium.org/2348433002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[eugenebng, 2016-09-26 10:01:19]

On 2016/09/23 17:51:10, sky wrote:
> On Fri, Sep 23, 2016 at 5:29 AM,  <mailto:eugenebng@yandex-team.ru> wrote:
> > On 2016/09/22 13:25:21, sky wrote:
> >> +estade as I believe he wanted the whitespace trimming logic. I tend to
> >> favor
> >> not trimming *any* whitespace and leaving it up to the caller to trim
> > whitespace
> >> before creating the styledlabel. But that likely breaks some usage, so we
> >> may
> >> need a property to give the new behavior.
> >
> > Regarding not trimming any whitespace, there are two cases in my code in
> > this
> > CL:
> > 1) When line wrapped by \n, this \n goes to remaining_string. And should be
> > removed in order that single \n will not cause infinite line wrap cycle.
> > So that actally means not trimming, because \n being removed was "printed"
> > (line
> > wrapped), do youy agree on this point?
> 
> Agreed.
> 
> > 2) When string doesn't fit into width and wrapped by one whitespace or
> > sequence
> > of whitespaces, there can be different options: remove all sequence or
> > remove
> > only one whitespace. I agree that trimming all seqence is more intrusive.
> > So I will remove only one whitespace, because for example line "styled
> > label",
> > when split into 2 lines because of not fitting into label width, should be
> > ["styled" and "label"], but not ["styled" and " label"] (space in the
> > beginning
> > of the second line).
> 
> Agreed.
> 
> > When wrapping line on two spcaces for example, [styled
> > space space label] will turn into [syled] and [space label] lines - with
> > leading
> > space on the second line.
> 
> If we wrap I'm inclined to move white space on both sides. In other
> words if you wrapped between 'styled' and 'label' it would look the
> same regardless of how much white space was between 'styled' and
> 'label'.

Then it looks like removing all whitespace sequence does it (else block in
code).

> 
>   -Scott
> 
> > Does this conform to what you mean as not trimmimg *any* whitespace?
> >
> > And I'm really interested in what Evan Stade thinks about it all, but I
> > agree
> > that this change is likely to be an option/property.
> >
> > https://codereview.chromium.org/2348433002/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
> 

So it looks like this change should be under option/property, rather then change
default styled_label behavior, but I'm not quite sure, PTAL, Evan Stade?

[sky, 2016-09-26 16:37:56]

I agree your change seems good and no additional property. Please add test
coverage.

[Evan Stade, 2016-09-26 16:50:45]

is there a bug this is fixing or a new UI surface this is enabling? You could
easily just use multiple StyledLabels for multiple paragraphs. (Hopefully there
are few to no places where we have so much text it has to be broken into
multiple paragraphs.)

[eugenebng, 2016-09-27 10:00:31]

On 2016/09/26 16:50:45, Evan Stade wrote:
> is there a bug this is fixing or a new UI surface this is enabling? You could
> easily just use multiple StyledLabels for multiple paragraphs. (Hopefully
there
> are few to no places where we have so much text it has to be broken into
> multiple paragraphs.)
This CL allows formatting styled label text with spaces. For example, with this
CL applied a line "This label presents a list to you:\n  (bullet) item one\n 
(bullet) item two" will be printed unmodified. However,existing styled label
code will trim spaces before bullets, so "item one" and "item two" will lose
indentation. And nope, there is no existing bug.

Considering that are few to no places where there are multiple paragraphs and
the fact that this CL changes whitespace handling on \n line wraps - maybe this
change is reasonable as a change to default styled label behavior because it
makes output text less different from original input string? In this case, I'll
have to review all styled label uses to see that nothing is broken of course.

[sky, 2016-09-27 13:24:37]

I'm ok with this fix, but you need to add test coverage.

On Tue, Sep 27, 2016 at 3:00 AM,  <eugenebng@yandex-team.ru> wrote:
> On 2016/09/26 16:50:45, Evan Stade wrote:
>> is there a bug this is fixing or a new UI surface this is enabling? You
>> could
>> easily just use multiple StyledLabels for multiple paragraphs. (Hopefully
> there
>> are few to no places where we have so much text it has to be broken into
>> multiple paragraphs.)
> This CL allows formatting styled label text with spaces. For example, with
> this
> CL applied a line "This label presents a list to you:\n (bullet) item one\n
> (bullet) item two" will be printed unmodified. However,existing styled label
> code will trim spaces before bullets, so "item one" and "item two" will lose
> indentation. And nope, there is no existing bug.
>
> Considering that are few to no places where there are multiple paragraphs
> and
> the fact that this CL changes whitespace handling on \n line wraps - maybe
> this
> change is reasonable as a change to default styled label behavior because it
> makes output text less different from original input string? In this case,
> I'll
> have to review all styled label uses to see that nothing is broken of
> course.
>
> https://codereview.chromium.org/2348433002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[eugenebng, 2016-09-28 11:22:41]

On 2016/09/27 13:24:37, sky wrote:
> I'm ok with this fix, but you need to add test coverage.
> 

Evan Stade, is that ok for you too?


> On Tue, Sep 27, 2016 at 3:00 AM,  <mailto:eugenebng@yandex-team.ru> wrote:
> > On 2016/09/26 16:50:45, Evan Stade wrote:
> >> is there a bug this is fixing or a new UI surface this is enabling? You
> >> could
> >> easily just use multiple StyledLabels for multiple paragraphs. (Hopefully
> > there
> >> are few to no places where we have so much text it has to be broken into
> >> multiple paragraphs.)
> > This CL allows formatting styled label text with spaces. For example, with
> > this
> > CL applied a line "This label presents a list to you:\n (bullet) item one\n
> > (bullet) item two" will be printed unmodified. However,existing styled label
> > code will trim spaces before bullets, so "item one" and "item two" will lose
> > indentation. And nope, there is no existing bug.
> >
> > Considering that are few to no places where there are multiple paragraphs
> > and
> > the fact that this CL changes whitespace handling on \n line wraps - maybe
> > this
> > change is reasonable as a change to default styled label behavior because it
> > makes output text less different from original input string? In this case,
> > I'll
> > have to review all styled label uses to see that nothing is broken of
> > course.
> >
> > https://codereview.chromium.org/2348433002/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
>

[Evan Stade, 2016-09-28 16:22:45]

On 2016/09/28 11:22:41, eugenebng wrote:
> On 2016/09/27 13:24:37, sky wrote:
> > I'm ok with this fix, but you need to add test coverage.
> > 
> 
> Evan Stade, is that ok for you too?
> 
> 
> > On Tue, Sep 27, 2016 at 3:00 AM,  <mailto:eugenebng@yandex-team.ru> wrote:
> > > On 2016/09/26 16:50:45, Evan Stade wrote:
> > >> is there a bug this is fixing or a new UI surface this is enabling? You
> > >> could
> > >> easily just use multiple StyledLabels for multiple paragraphs. (Hopefully
> > > there
> > >> are few to no places where we have so much text it has to be broken into
> > >> multiple paragraphs.)
> > > This CL allows formatting styled label text with spaces. For example, with
> > > this
> > > CL applied a line "This label presents a list to you:\n (bullet) item
one\n
> > > (bullet) item two" will be printed unmodified. However,existing styled
label
> > > code will trim spaces before bullets, so "item one" and "item two" will
lose
> > > indentation. And nope, there is no existing bug.
> > >
> > > Considering that are few to no places where there are multiple paragraphs
> > > and
> > > the fact that this CL changes whitespace handling on \n line wraps - maybe
> > > this
> > > change is reasonable as a change to default styled label behavior because
it
> > > makes output text less different from original input string? In this case,
> > > I'll
> > > have to review all styled label uses to see that nothing is broken of
> > > course.
> > >
> > > https://codereview.chromium.org/2348433002/
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> >

this change is OK with me if it's fixing a bug. You said it's not fixing a bug.
I like to err on the side of leaving things that aren't broken alone --- as you
point out, every change has the potential to cause breakages, and every
additional bit of complexity makes it harder to modify this code down the line.

[eugenebng, 2016-09-29 07:42:24]

On 2016/09/28 16:22:45, Evan Stade wrote:
> On 2016/09/28 11:22:41, eugenebng wrote:
> > On 2016/09/27 13:24:37, sky wrote:
> > > I'm ok with this fix, but you need to add test coverage.
> > > 
> > 
> > Evan Stade, is that ok for you too?
> > 
> > 
> > > On Tue, Sep 27, 2016 at 3:00 AM,  <mailto:eugenebng@yandex-team.ru> wrote:
> > > > On 2016/09/26 16:50:45, Evan Stade wrote:
> > > >> is there a bug this is fixing or a new UI surface this is enabling? You
> > > >> could
> > > >> easily just use multiple StyledLabels for multiple paragraphs.
(Hopefully
> > > > there
> > > >> are few to no places where we have so much text it has to be broken
into
> > > >> multiple paragraphs.)
> > > > This CL allows formatting styled label text with spaces. For example,
with
> > > > this
> > > > CL applied a line "This label presents a list to you:\n (bullet) item
> one\n
> > > > (bullet) item two" will be printed unmodified. However,existing styled
> label
> > > > code will trim spaces before bullets, so "item one" and "item two" will
> lose
> > > > indentation. And nope, there is no existing bug.
> > > >
> > > > Considering that are few to no places where there are multiple
paragraphs
> > > > and
> > > > the fact that this CL changes whitespace handling on \n line wraps -
maybe
> > > > this
> > > > change is reasonable as a change to default styled label behavior
because
> it
> > > > makes output text less different from original input string? In this
case,
> > > > I'll
> > > > have to review all styled label uses to see that nothing is broken of
> > > > course.
> > > >
> > > > https://codereview.chromium.org/2348433002/
> > > 
> > > -- 
> > > You received this message because you are subscribed to the Google Groups
> > > "Chromium-reviews" group.
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > email
> > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> > >
> 
> this change is OK with me if it's fixing a bug. You said it's not fixing a
bug.
> I like to err on the side of leaving things that aren't broken alone --- as
you
> point out, every change has the potential to cause breakages, and every
> additional bit of complexity makes it harder to modify this code down the
line.

ok, let's leave it alone.

[eugenebng, 2016-11-30 16:44:10]

Description was changed from

==========
Make styled-label trimming less agressive, allowing whitespace
"formatting" like line indentation with spaces etc

What was in the code:
Trimming all whitespaces at the beginning of the line (except first)
in styled label is a bit odd, especially considering first line
exeption with known reason - leading whitespaces.

I suggest:
If line is wrapped by one ore more whitespaces in the middle, it looks
fine to trim them all.
But if line is wrapped by \n, it seems better to trim only that \n,
allowing non-first line to have leading spaces, just like first line.
And displaying string with less modifications seems good too.

Do you like the idea?

R=sky@chromium.org
BUG=
==========

to

==========
Make styled-label trimming less agressive, allowing whitespace
"formatting" like line indentation with spaces etc

What's fixed in the client code: LegalMessageLine no longer has to have a
comment in it' interface about being implememnted on StyledLabel and having odd
behavior because of that

What was in the code:
Trimming all whitespaces at the beginning of the line (except first)
in styled label is a bit odd, especially considering first line
exeption with known reason - leading whitespaces.

I suggest:
If line is wrapped by one ore more whitespaces in the middle, it looks
fine to trim them all.
But if line is wrapped by \n, it seems better to trim only that \n,
allowing non-first line to have leading spaces, just like first line.
And displaying string with less modifications seems good too.

Do you like the idea?

R=sky@chromium.org
BUG=
==========

[eugenebng, 2016-12-01 09:29:52]

PTAL at patch set 2. It's now not only change in styled_label, but also fixes
other place in code.

[sky, 2016-12-01 16:07:16]

Please add test coverage.

[eugenebng, 2016-12-05 12:25:55]

On 2016/12/01 16:07:16, sky wrote:
> Please add test coverage.
Added tests, checking that empty lines are allowed and indentation with spaces /
leading spaces in non-first line is allowed too.

[sky, 2016-12-05 16:13:19]

Evan had concerns about this in the past. Evan, this change seems reasonable to
me, WDYT?

[Evan Stade, 2016-12-05 17:12:14]

I see some style nits, but generally no objection

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

https://codereview.chromium.org/2348433002/diff/40001/ui/views/controls/style...
ui/views/controls/styled_label_unittest.cc:101:
EXPECT_EQ(ASCIIToUTF16(indented_line),
can you test what would happen if we naturally wrap after "First line" (i.e. we
try to start the second line with a newline)

[eugenebng, 2016-12-06 11:30:50]

On 2016/12/05 17:12:14, Evan Stade wrote:
> I see some style nits, but generally no objection
> 
>
https://codereview.chromium.org/2348433002/diff/40001/ui/views/controls/style...
> File ui/views/controls/styled_label_unittest.cc (right):
> 
>
https://codereview.chromium.org/2348433002/diff/40001/ui/views/controls/style...
> ui/views/controls/styled_label_unittest.cc:101:
> EXPECT_EQ(ASCIIToUTF16(indented_line),
> can you test what would happen if we naturally wrap after "First line" (i.e.
we
> try to start the second line with a newline)

Added new test to cover this case - PTAL, Evan.

There are no styled_label code changes, it already did work. Even when wrapped
because of \n, not width limit - this \n goes to |remaining_string|, and it's
removed in the beginning of next line processing.

[Evan Stade, 2016-12-06 17:04:47]

just nits, I'll leave it to sky for OWNERS perspective/review.

https://codereview.chromium.org/2348433002/diff/80001/ui/views/controls/style...
File ui/views/controls/styled_label.cc (right):

https://codereview.chromium.org/2348433002/diff/80001/ui/views/controls/style...
ui/views/controls/styled_label.cc:264: // wrapped to the next line on \n, remove
it. Other whitespaces,
nit: capitalization
nit: s/whitespaces/whitespace

https://codereview.chromium.org/2348433002/diff/80001/ui/views/controls/style...
ui/views/controls/styled_label.cc:268: // wrapped on whitespace character or
characters in the middle of the
nit: capitalization

https://codereview.chromium.org/2348433002/diff/80001/ui/views/controls/style...
ui/views/controls/styled_label.cc:269: // line - none of them are needed at the
beginning of the next line
nit: final punctuation

https://codereview.chromium.org/2348433002/diff/80001/ui/views/controls/style...
ui/views/controls/styled_label.cc:271: &remaining_string);
nit: indentation

https://codereview.chromium.org/2348433002/diff/80001/ui/views/controls/style...
ui/views/controls/styled_label.cc:303: // otput it as empty line. Otherwise
there is no room for anything; abort.
nit: spelling
nit: "...an empty line"

https://codereview.chromium.org/2348433002/diff/80001/ui/views/controls/style...
ui/views/controls/styled_label.cc:311: line++;
nit: ++line (I don't know why we mix line++ and ++line in this fn, but it should
be ++line)

also, I think you can invert this check ---

if (line == 0) {
  Trim
  continue
} else if (remaining_string.empty() || remaining_string.front() != L'\n') {
  break;
}

because this branch mimics L317-319

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

https://codereview.chromium.org/2348433002/diff/80001/ui/views/controls/style...
ui/views/controls/styled_label_unittest.cc:119:
static_cast<Label*>(styled()->child_at(1))->text());
nit: check contents of first line as well?

[eugenebng, 2016-12-07 16:49:24]

On 2016/12/06 17:04:47, Evan Stade wrote:
> just nits, I'll leave it to sky for OWNERS perspective/review.
> 
>
https://codereview.chromium.org/2348433002/diff/80001/ui/views/controls/style...
> File ui/views/controls/styled_label.cc (right):
> 
>
https://codereview.chromium.org/2348433002/diff/80001/ui/views/controls/style...
> ui/views/controls/styled_label.cc:264: // wrapped to the next line on \n,
remove
> it. Other whitespaces,
> nit: capitalization
> nit: s/whitespaces/whitespace
> 
>
https://codereview.chromium.org/2348433002/diff/80001/ui/views/controls/style...
> ui/views/controls/styled_label.cc:268: // wrapped on whitespace character or
> characters in the middle of the
> nit: capitalization
> 
>
https://codereview.chromium.org/2348433002/diff/80001/ui/views/controls/style...
> ui/views/controls/styled_label.cc:269: // line - none of them are needed at
the
> beginning of the next line
> nit: final punctuation
> 
>
https://codereview.chromium.org/2348433002/diff/80001/ui/views/controls/style...
> ui/views/controls/styled_label.cc:271: &remaining_string);
> nit: indentation
> 
>
https://codereview.chromium.org/2348433002/diff/80001/ui/views/controls/style...
> ui/views/controls/styled_label.cc:303: // otput it as empty line. Otherwise
> there is no room for anything; abort.
> nit: spelling
> nit: "...an empty line"
> 
>
https://codereview.chromium.org/2348433002/diff/80001/ui/views/controls/style...
> ui/views/controls/styled_label.cc:311: line++;
> nit: ++line (I don't know why we mix line++ and ++line in this fn, but it
should
> be ++line)

Fixed these nits, including changing to ++line rather then line++ in styled
label.

> also, I think you can invert this check ---
> 
> if (line == 0) {
>   Trim
>   continue
> } else if (remaining_string.empty() || remaining_string.front() != L'\n') {
>   break;
> }
> 
> because this branch mimics L317-319

Certainly. Done.

>
https://codereview.chromium.org/2348433002/diff/80001/ui/views/controls/style...
> File ui/views/controls/styled_label_unittest.cc (right):
> 
>
https://codereview.chromium.org/2348433002/diff/80001/ui/views/controls/style...
> ui/views/controls/styled_label_unittest.cc:119:
> static_cast<Label*>(styled()->child_at(1))->text());
> nit: check contents of first line as well?

Now checking both lines.

[eugenebng, 2016-12-09 10:39:37]

WDYT Evan?

On 2016/12/07 16:49:24, eugenebng wrote:
> On 2016/12/06 17:04:47, Evan Stade wrote:
> > just nits, I'll leave it to sky for OWNERS perspective/review.
> > 
> >
>
https://codereview.chromium.org/2348433002/diff/80001/ui/views/controls/style...
> > File ui/views/controls/styled_label.cc (right):
> > 
> >
>
https://codereview.chromium.org/2348433002/diff/80001/ui/views/controls/style...
> > ui/views/controls/styled_label.cc:264: // wrapped to the next line on \n,
> remove
> > it. Other whitespaces,
> > nit: capitalization
> > nit: s/whitespaces/whitespace
> > 
> >
>
https://codereview.chromium.org/2348433002/diff/80001/ui/views/controls/style...
> > ui/views/controls/styled_label.cc:268: // wrapped on whitespace character or
> > characters in the middle of the
> > nit: capitalization
> > 
> >
>
https://codereview.chromium.org/2348433002/diff/80001/ui/views/controls/style...
> > ui/views/controls/styled_label.cc:269: // line - none of them are needed at
> the
> > beginning of the next line
> > nit: final punctuation
> > 
> >
>
https://codereview.chromium.org/2348433002/diff/80001/ui/views/controls/style...
> > ui/views/controls/styled_label.cc:271: &remaining_string);
> > nit: indentation
> > 
> >
>
https://codereview.chromium.org/2348433002/diff/80001/ui/views/controls/style...
> > ui/views/controls/styled_label.cc:303: // otput it as empty line. Otherwise
> > there is no room for anything; abort.
> > nit: spelling
> > nit: "...an empty line"
> > 
> >
>
https://codereview.chromium.org/2348433002/diff/80001/ui/views/controls/style...
> > ui/views/controls/styled_label.cc:311: line++;
> > nit: ++line (I don't know why we mix line++ and ++line in this fn, but it
> should
> > be ++line)
> 
> Fixed these nits, including changing to ++line rather then line++ in styled
> label.
> 
> > also, I think you can invert this check ---
> > 
> > if (line == 0) {
> >   Trim
> >   continue
> > } else if (remaining_string.empty() || remaining_string.front() != L'\n') {
> >   break;
> > }
> > 
> > because this branch mimics L317-319
> 
> Certainly. Done.
> 
> >
>
https://codereview.chromium.org/2348433002/diff/80001/ui/views/controls/style...
> > File ui/views/controls/styled_label_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/2348433002/diff/80001/ui/views/controls/style...
> > ui/views/controls/styled_label_unittest.cc:119:
> > static_cast<Label*>(styled()->child_at(1))->text());
> > nit: check contents of first line as well?
> 
> Now checking both lines.

[Evan Stade, 2016-12-09 18:41:58]

https://codereview.chromium.org/2348433002/diff/120001/ui/views/controls/styl...
File ui/views/controls/styled_label.cc (right):

https://codereview.chromium.org/2348433002/diff/120001/ui/views/controls/styl...
ui/views/controls/styled_label.cc:304: // Otherwise there is no room for
anything; abort.
When I rewrite this portion of the function (L299-319) to this[1], all tests
still pass. Is this a valid rewrite or is the test coverage incomplete?

[1]
    if (substrings.empty())
      break;

    if (substrings[0].empty()) {
      x = 0;
      ++line;
      continue;
    }

[eugenebng, 2016-12-12 12:08:41]

On 2016/12/09 18:41:58, Evan Stade wrote:
>
https://codereview.chromium.org/2348433002/diff/120001/ui/views/controls/styl...
> File ui/views/controls/styled_label.cc (right):
> 
>
https://codereview.chromium.org/2348433002/diff/120001/ui/views/controls/styl...
> ui/views/controls/styled_label.cc:304: // Otherwise there is no room for
> anything; abort.
> When I rewrite this portion of the function (L299-319) to this[1], all tests
> still pass. Is this a valid rewrite or is the test coverage incomplete?
> 
> [1]
>     if (substrings.empty())
>       break;
> 
>     if (substrings[0].empty()) {
>       x = 0;
>       ++line;
>       continue;
>     }

That was incomplete test coverage - covered new case in patch set 8.

[Evan Stade, 2016-12-12 16:15:33]

On 2016/12/12 12:08:41, eugenebng wrote:
> On 2016/12/09 18:41:58, Evan Stade wrote:
> >
>
https://codereview.chromium.org/2348433002/diff/120001/ui/views/controls/styl...
> > File ui/views/controls/styled_label.cc (right):
> > 
> >
>
https://codereview.chromium.org/2348433002/diff/120001/ui/views/controls/styl...
> > ui/views/controls/styled_label.cc:304: // Otherwise there is no room for
> > anything; abort.
> > When I rewrite this portion of the function (L299-319) to this[1], all tests
> > still pass. Is this a valid rewrite or is the test coverage incomplete?
> > 
> > [1]
> >     if (substrings.empty())
> >       break;
> > 
> >     if (substrings[0].empty()) {
> >       x = 0;
> >       ++line;
> >       continue;
> >     }
> 
> That was incomplete test coverage - covered new case in patch set 8.

thanks for adding that test. My main concern is that this seems a bit
duplicative (and it's hard to even see that fact). We could use a lambda[1] but
I think it's even simpler just to introduce a new boolean that separates the
concept of line number from loop iteration number.


  bool first_loop_iteration = true;

  // Iterate over the text, creating a bunch of labels and links and laying them
  // out in the appropriate positions.
  while (!remaining_string.empty()) {
    if (x == 0 && !first_loop_iteration) {
      if (remaining_string.front() == L'\n') {
        // Wrapped to the next line on \n, remove it. Other whitespace,
        // eg, spaces to indent next line, are preserved.
        remaining_string.erase(0, 1);
      } else {
        // Wrapped on whitespace character or characters in the middle of
        // the line - none of them are needed at the beginning of the next
        // line.
        base::TrimWhitespace(remaining_string, base::TRIM_LEADING,
                             &remaining_string);
      }
    }
    first_loop_iteration = false;

// ...

    if (substrings.empty())
      break;
    if (substrings[0].empty()) {
      x = 0;
      // docs
      if (line > 0)
        ++line;
      continue;
    }



[1] auto remove_whitespace =
        [&remaining_string]() {
          if (remaining_string.front() == L'\n') {
            // Wrapped to the next line on \n, remove it. Other whitespace,
            // eg, spaces to indent next line, are preserved.
            remaining_string.erase(0, 1);
          } else {
            // Wrapped on whitespace character or characters in the middle of
            // the line - none of them are needed at the beginning of the next
            // line.
            base::TrimWhitespace(remaining_string, base::TRIM_LEADING,
                                 &remaining_string);
          }
        };

    if (x == 0 && line > 0)
      remove_whitespace();

// ...

[eugenebng, 2016-12-13 05:18:08]

I agree that code style can be improved with introducing bool
first_loop_iteration. But I didn't understand one point regarding this fragment
of code you suggested:
>    if (substrings.empty())
>      break;
>    if (substrings[0].empty()) {
>      x = 0;
>      // docs
>      if (line > 0)
>        ++line;
>      continue;
>    }
-- There is no second TrimWhitespace call here. Maybe I haven't explained enough
details in my previous comment, but the check in test added in patch set 8
covers execution of the following path in existing code (from line 299): 
>    if (substrings.empty() || substrings[0].empty()) {
>      // long comment
>      if (x == 0) {
>        if (line == 0) {
>          base::TrimWhitespace(remaining_string, base::TRIM_LEADING,
>                               &remaining_string);
This trimming results in text "              a" is printed as one line "a"
(rather then empty line and then next line "a" - if this code path is removed)
when it doesn't fit into styled label width.
Don't you think this call to TrimWhitespace should be preserved in new changed
code?

[Evan Stade, 2016-12-13 16:07:19]

On 2016/12/13 05:18:08, eugenebng wrote:
> I agree that code style can be improved with introducing bool
> first_loop_iteration. But I didn't understand one point regarding this
fragment
> of code you suggested:
> >    if (substrings.empty())
> >      break;
> >    if (substrings[0].empty()) {
> >      x = 0;
> >      // docs
> >      if (line > 0)
> >        ++line;
> >      continue;
> >    }
> -- There is no second TrimWhitespace call here. Maybe I haven't explained
enough
> details in my previous comment, but the check in test added in patch set 8
> covers execution of the following path in existing code (from line 299): 

the second trimwhitespace comes at the beginning of the next loop iteration.
With my code snippet, all tests pass.

> >    if (substrings.empty() || substrings[0].empty()) {
> >      // long comment
> >      if (x == 0) {
> >        if (line == 0) {
> >          base::TrimWhitespace(remaining_string, base::TRIM_LEADING,
> >                               &remaining_string);
> This trimming results in text "              a" is printed as one line "a"
> (rather then empty line and then next line "a" - if this code path is removed)
> when it doesn't fit into styled label width.
> Don't you think this call to TrimWhitespace should be preserved in new changed
> code?

[eugenebng, 2016-12-14 11:32:56]

On 2016/12/13 16:07:19, Evan Stade wrote:
> On 2016/12/13 05:18:08, eugenebng wrote:
> > I agree that code style can be improved with introducing bool
> > first_loop_iteration. But I didn't understand one point regarding this
> fragment
> > of code you suggested:
> > >    if (substrings.empty())
> > >      break;
> > >    if (substrings[0].empty()) {
> > >      x = 0;
> > >      // docs
> > >      if (line > 0)
> > >        ++line;
> > >      continue;
> > >    }
> > -- There is no second TrimWhitespace call here. Maybe I haven't explained
> enough
> > details in my previous comment, but the check in test added in patch set 8
> > covers execution of the following path in existing code (from line 299): 
> 
> the second trimwhitespace comes at the beginning of the next loop iteration.
> With my code snippet, all tests pass.
> 
> > >    if (substrings.empty() || substrings[0].empty()) {
> > >      // long comment
> > >      if (x == 0) {
> > >        if (line == 0) {
> > >          base::TrimWhitespace(remaining_string, base::TRIM_LEADING,
> > >                               &remaining_string);
> > This trimming results in text "              a" is printed as one line "a"
> > (rather then empty line and then next line "a" - if this code path is
removed)
> > when it doesn't fit into styled label width.
> > Don't you think this call to TrimWhitespace should be preserved in new
changed
> > code?

Thanks, Evan. 
It looks like really correct change. Looked thu it once more, found no issues.
New code (with re-styled comment too) is in patch set 9.

[Evan Stade, 2016-12-15 00:02:36]

lgtm

https://codereview.chromium.org/2348433002/diff/160001/ui/views/controls/styl...
File ui/views/controls/styled_label.cc (right):

https://codereview.chromium.org/2348433002/diff/160001/ui/views/controls/styl...
ui/views/controls/styled_label.cc:309: // As for the first line, don't advance
line number so that it will be
nit: instead of "it will be handled again" perhaps "we'll try again after
trimming whitespace"

[Evan Stade, 2016-12-15 00:04:40]

And thanks for sticking with this change. One last request: could you update the
CL description to make it more of a commit message and less of a message to
reviewers? i.e. reformat it to not include things like "I suggest" and "do you
like the idea?"

[sky, 2016-12-15 00:46:51]

LGTM

[eugenebng, 2016-12-15 08:45:54]

Description was changed from

==========
Make styled-label trimming less agressive, allowing whitespace
"formatting" like line indentation with spaces etc

What's fixed in the client code: LegalMessageLine no longer has to have a
comment in it' interface about being implememnted on StyledLabel and having odd
behavior because of that

What was in the code:
Trimming all whitespaces at the beginning of the line (except first)
in styled label is a bit odd, especially considering first line
exeption with known reason - leading whitespaces.

I suggest:
If line is wrapped by one ore more whitespaces in the middle, it looks
fine to trim them all.
But if line is wrapped by \n, it seems better to trim only that \n,
allowing non-first line to have leading spaces, just like first line.
And displaying string with less modifications seems good too.

Do you like the idea?

R=sky@chromium.org
BUG=
==========

to

==========
Make styled-label trimming less aggressive - now when wrapped on \n, only this
\n is removed, not all whitespace sequence. This allows whitespace
"formatting" like line indentation with spaces. Also makes double newline behave
like expected - insert an empty line between text before and after \n\n.
Syled label layout loop refactored to de-duplicate code.
Added tests coverage for styled label.
Fixed LegalMessageLine comment - no longer have to explain that it is based on
styled_label with some caveats.

R=sky@chromium.org
BUG=
==========

[eugenebng, 2016-12-15 08:46:36]

On 2016/12/15 00:04:40, Evan Stade wrote:
> And thanks for sticking with this change. One last request: could you update
the
> CL description to make it more of a commit message and less of a message to
> reviewers? i.e. reformat it to not include things like "I suggest" and "do you
> like the idea?"
Updated description.

[eugenebng, 2016-12-15 08:47:07]

The CQ bit was checked by eugenebng@yandex-team.ru

[commit-bot: I haz the power, 2016-12-15 08:47:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 09:51:26]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1481791627202350,
"parent_rev": "b5403af9b49ceef8dc042479611782d30a4a308c", "commit_rev":
"bc914bf435f3f488b7862f9f9772365d0430ec0e"}

[commit-bot: I haz the power, 2016-12-15 09:51:56]

Description was changed from

==========
Make styled-label trimming less aggressive - now when wrapped on \n, only this
\n is removed, not all whitespace sequence. This allows whitespace
"formatting" like line indentation with spaces. Also makes double newline behave
like expected - insert an empty line between text before and after \n\n.
Syled label layout loop refactored to de-duplicate code.
Added tests coverage for styled label.
Fixed LegalMessageLine comment - no longer have to explain that it is based on
styled_label with some caveats.

R=sky@chromium.org
BUG=
==========

to

==========
Make styled-label trimming less aggressive - now when wrapped on \n, only this
\n is removed, not all whitespace sequence. This allows whitespace
"formatting" like line indentation with spaces. Also makes double newline behave
like expected - insert an empty line between text before and after \n\n.
Syled label layout loop refactored to de-duplicate code.
Added tests coverage for styled label.
Fixed LegalMessageLine comment - no longer have to explain that it is based on
styled_label with some caveats.

R=sky@chromium.org
BUG=

Review-Url: https://codereview.chromium.org/2348433002
==========

[commit-bot: I haz the power, 2016-12-15 09:51:57]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-12-15 09:54:00]

Description was changed from

==========
Make styled-label trimming less aggressive - now when wrapped on \n, only this
\n is removed, not all whitespace sequence. This allows whitespace
"formatting" like line indentation with spaces. Also makes double newline behave
like expected - insert an empty line between text before and after \n\n.
Syled label layout loop refactored to de-duplicate code.
Added tests coverage for styled label.
Fixed LegalMessageLine comment - no longer have to explain that it is based on
styled_label with some caveats.

R=sky@chromium.org
BUG=

Review-Url: https://codereview.chromium.org/2348433002
==========

to

==========
Make styled-label trimming less aggressive - now when wrapped on \n, only this
\n is removed, not all whitespace sequence. This allows whitespace
"formatting" like line indentation with spaces. Also makes double newline behave
like expected - insert an empty line between text before and after \n\n.
Syled label layout loop refactored to de-duplicate code.
Added tests coverage for styled label.
Fixed LegalMessageLine comment - no longer have to explain that it is based on
styled_label with some caveats.

R=sky@chromium.org
BUG=

Committed: https://crrev.com/933ca2314a2ea20ceafe435bbc896500e67bd7a3
Cr-Commit-Position: refs/heads/master@{#438795}
==========

[commit-bot: I haz the power, 2016-12-15 09:54:01]

Patchset 9 (id:??) landed as
https://crrev.com/933ca2314a2ea20ceafe435bbc896500e67bd7a3
Cr-Commit-Position: refs/heads/master@{#438795}