Refactor BreakingContext::handleText

Refactor and comment BreakingContext::handleText()

This is a large loop condition which spends a lot of its time working out
whether the current whitespace character is breakable or not. Refactor it
to make its operation clearer and add some comments that make it easier
to take in at a glance what the loop is doing.

This first part of the refactoring gets rid of a >100 line if-clause and converts
it to an early |continue|. There is just no way of making this part of the
refactoring easy to follow. :) (It's even worse if I duplicate the block of code
I've here put into a prepareNewCharacter() function.)

This is the first of a number of planned passes to make this function more friendly.

BUG=567080
Committed: https://crrev.com/65ecdc54baece8049b76ee11a1ed69f9436d5be9
Cr-Commit-Position: refs/heads/master@{#365629}

[rhogan, 2015-12-07 17:32:00]

Description was changed from

==========
Refactor BreakingContext::handleText

BUG=567080
==========

to

==========
Refactor and comment BreakingContext::handleText()

This is a large loop condition which spends a lot of its time working out
whether the current whitespace character is breakable or not. Refactor it
to make its operation clearer and add some comments that make it easier
to take in at a glance what the loop is doing.

BUG=567080
==========

[rhogan, 2015-12-07 17:32:00]

robhogan@gmail.com changed reviewers:
+ eae@chromium.org

[leviw_travelin_and_unemployed, 2015-12-07 22:56:35]

leviw@chromium.org changed reviewers:
+ kojii@chromium.org, leviw@chromium.org

[leviw_travelin_and_unemployed, 2015-12-07 22:56:47]

eae is out, kojii would be a good reviewer :)

[kojii, 2015-12-08 08:02:10]

https://codereview.chromium.org/1504083002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h
(right):

https://codereview.chromium.org/1504083002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h:739:
// We just entered a mode where we are ignoring spaces. Create a midpoint to
terminate the run  before the second space.
nit: two spaces between "run" and "before".

https://codereview.chromium.org/1504083002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h:796:
inline WordMeasurement& BreakingContext::calculateWordWidth(WordMeasurements&
wordMeasurements, LineLayoutText& layoutText, float lastSpace, float&
lastWidthMeasurement, float wordSpacingForWordMeasurement, const Font& font,
float wordTrailingSpaceWidth, UChar c)
Shouldn't lastSpace be unsigned rather than float?

[rhogan, 2015-12-08 10:01:56]

On 2015/12/08 at 08:02:10, kojii wrote:
>
https://codereview.chromium.org/1504083002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h
(right):
> 
>
https://codereview.chromium.org/1504083002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h:739:
// We just entered a mode where we are ignoring spaces. Create a midpoint to
terminate the run  before the second space.
> nit: two spaces between "run" and "before".
> 
>
https://codereview.chromium.org/1504083002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h:796:
inline WordMeasurement& BreakingContext::calculateWordWidth(WordMeasurements&
wordMeasurements, LineLayoutText& layoutText, float lastSpace, float&
lastWidthMeasurement, float wordSpacingForWordMeasurement, const Font& font,
float wordTrailingSpaceWidth, UChar c)
> Shouldn't lastSpace be unsigned rather than float?

Thanks! Updated the CL.

[kojii, 2015-12-08 10:18:59]

non-owner LGTM.

Levi?

[leviw_travelin_and_unemployed, 2015-12-08 19:46:39]

I have little doubt this is a good patch. Thanks for doing this! Honestly
though, I'm having trouble parsing through all the changes. Would you be willing
to break it into a series of smaller changes? It'd make it a lot easier for me
to review if you landed this refactor incrementally.

[rhogan, 2015-12-08 19:51:40]

On 2015/12/08 at 19:46:39, leviw wrote:
> I have little doubt this is a good patch. Thanks for doing this! Honestly
though, I'm having trouble parsing through all the changes. Would you be willing
to break it into a series of smaller changes? It'd make it a lot easier for me
to review if you landed this refactor incrementally.

Would you be willing to apply it locally and take a look at it there? Doing it
incrementally would be quite hard and also tedious.

This is going to be the first of several refactorings in this file I hope.

[leviw_travelin_and_unemployed, 2015-12-08 19:59:25]

On 2015/12/08 at 19:51:40, robhogan wrote:
> On 2015/12/08 at 19:46:39, leviw wrote:
> > I have little doubt this is a good patch. Thanks for doing this! Honestly
though, I'm having trouble parsing through all the changes. Would you be willing
to break it into a series of smaller changes? It'd make it a lot easier for me
to review if you landed this refactor incrementally.
> 
> Would you be willing to apply it locally and take a look at it there? Doing it
incrementally would be quite hard and also tedious.
> 
> This is going to be the first of several refactorings in this file I hope.

I'll sit down with it before EOD and try and get it reviewed. The mis-spelling
and smaller helper functions seem easy to land separately from what I can see.
All put together, it's just a lot more review work following the changes.

[rhogan, 2015-12-08 21:08:51]

On 2015/12/08 at 19:59:25, leviw wrote:
> On 2015/12/08 at 19:51:40, robhogan wrote:
> > On 2015/12/08 at 19:46:39, leviw wrote:
> > > I have little doubt this is a good patch. Thanks for doing this! Honestly
though, I'm having trouble parsing through all the changes. Would you be willing
to break it into a series of smaller changes? It'd make it a lot easier for me
to review if you landed this refactor incrementally.
> > 
> > Would you be willing to apply it locally and take a look at it there? Doing
it incrementally would be quite hard and also tedious.
> > 
> > This is going to be the first of several refactorings in this file I hope.
> 
> I'll sit down with it before EOD and try and get it reviewed. The mis-spelling
and smaller helper functions seem easy to land separately from what I can see.
All put together, it's just a lot more review work following the changes.

It looks worse than it is I think. I've just moved a lot of stuff out to
functions and turned the inner loop into a series of commented steps. Hopefully
it will make more sense when you view the applied patch locally - once you see
it the diff will probably make more sense.

I appreciate the extra time this takes on your part.

[kojii, 2015-12-10 08:47:47]

I ended up loading the patch locally, but code are moved around so much that it
was quite time consuming to see if we were doing the same thing as before,
especially when removed code and added code does not fit in one screen.

I agree with Levi that splitting the patch would help better review; comments
and typo in a patch, then move one or two functions in a patch should make
things easier.

[rhogan, 2015-12-12 16:40:58]

Description was changed from

==========
Refactor and comment BreakingContext::handleText()

This is a large loop condition which spends a lot of its time working out
whether the current whitespace character is breakable or not. Refactor it
to make its operation clearer and add some comments that make it easier
to take in at a glance what the loop is doing.

BUG=567080
==========

to

==========
Refactor and comment BreakingContext::handleText()

This is a large loop condition which spends a lot of its time working out
whether the current whitespace character is breakable or not. Refactor it
to make its operation clearer and add some comments that make it easier
to take in at a glance what the loop is doing.

This first part of the refactoring gets rid of a 100 line if-clause and converts
it to an early |continue|.

BUG=567080
==========

[rhogan, 2015-12-12 16:45:55]

Description was changed from

==========
Refactor and comment BreakingContext::handleText()

This is a large loop condition which spends a lot of its time working out
whether the current whitespace character is breakable or not. Refactor it
to make its operation clearer and add some comments that make it easier
to take in at a glance what the loop is doing.

This first part of the refactoring gets rid of a 100 line if-clause and converts
it to an early |continue|.

BUG=567080
==========

to

==========
Refactor and comment BreakingContext::handleText()

This is a large loop condition which spends a lot of its time working out
whether the current whitespace character is breakable or not. Refactor it
to make its operation clearer and add some comments that make it easier
to take in at a glance what the loop is doing.

This first part of the refactoring gets rid of a 100 line if-clause and converts
it to an early |continue|. There is just no way of making this part of the
refactoring easy to follow.

BUG=567080
==========

[rhogan, 2015-12-12 16:54:41]

Description was changed from

==========
Refactor and comment BreakingContext::handleText()

This is a large loop condition which spends a lot of its time working out
whether the current whitespace character is breakable or not. Refactor it
to make its operation clearer and add some comments that make it easier
to take in at a glance what the loop is doing.

This first part of the refactoring gets rid of a 100 line if-clause and converts
it to an early |continue|. There is just no way of making this part of the
refactoring easy to follow.

BUG=567080
==========

to

==========
Refactor and comment BreakingContext::handleText()

This is a large loop condition which spends a lot of its time working out
whether the current whitespace character is breakable or not. Refactor it
to make its operation clearer and add some comments that make it easier
to take in at a glance what the loop is doing.

This first part of the refactoring gets rid of a >100 line if-clause and
converts
it to an early |continue|. There is just no way of making this part of the
refactoring easy to follow. :) (It's even worse if I duplicate the block of code
I've here put into a prepareNewCharacter() function.)

This is the first of a number of planned passes to make this function more
friendly.

BUG=567080
==========

[rhogan, 2015-12-12 16:56:00]

On 2015/12/10 at 08:47:47, kojii wrote:
> I ended up loading the patch locally, but code are moved around so much that
it was quite time consuming to see if we were doing the same thing as before,
especially when removed code and added code does not fit in one screen.
> 
> I agree with Levi that splitting the patch would help better review; comments
and typo in a patch, then move one or two functions in a patch should make
things easier.

Sure - have reduced it to the key part of this change, which is getting rid of
the 100-line if-clause. Further passes will simplify the rest of the logic.

[leviw_travelin_and_unemployed, 2015-12-15 21:40:07]

This is still pretty tricky to follow, but lgtm with a couple nits.

https://codereview.chromium.org/1504083002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h
(right):

https://codereview.chromium.org/1504083002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h:637:
// If we're in the middle of a word or at the start of a new one and can't break
there then continue to the next character.
I'd add a comma
"... break there, then..."

https://codereview.chromium.org/1504083002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h:674:
// So we're in the first whitespace after a word or in whitespace that we don't
collapse, which means we may have a breaking opportunity here.
"We're in the first whitespace character after a word..."

[rhogan, 2015-12-16 21:22:42]

The CQ bit was checked by robhogan@gmail.com

[rhogan, 2015-12-16 21:22:42]

The patchset sent to the CQ was uploaded after l-g-t-m from kojii@chromium.org,
leviw@chromium.org
Link to the patchset: https://codereview.chromium.org/1504083002/#ps60001
(title: "Updated")

[commit-bot: I haz the power, 2015-12-16 21:23:22]

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

[commit-bot: I haz the power, 2015-12-16 21:48:28]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-16 21:50:12]

Description was changed from

==========
Refactor and comment BreakingContext::handleText()

This is a large loop condition which spends a lot of its time working out
whether the current whitespace character is breakable or not. Refactor it
to make its operation clearer and add some comments that make it easier
to take in at a glance what the loop is doing.

This first part of the refactoring gets rid of a >100 line if-clause and
converts
it to an early |continue|. There is just no way of making this part of the
refactoring easy to follow. :) (It's even worse if I duplicate the block of code
I've here put into a prepareNewCharacter() function.)

This is the first of a number of planned passes to make this function more
friendly.

BUG=567080
==========

to

==========
Refactor and comment BreakingContext::handleText()

This is a large loop condition which spends a lot of its time working out
whether the current whitespace character is breakable or not. Refactor it
to make its operation clearer and add some comments that make it easier
to take in at a glance what the loop is doing.

This first part of the refactoring gets rid of a >100 line if-clause and
converts
it to an early |continue|. There is just no way of making this part of the
refactoring easy to follow. :) (It's even worse if I duplicate the block of code
I've here put into a prepareNewCharacter() function.)

This is the first of a number of planned passes to make this function more
friendly.

BUG=567080

Committed: https://crrev.com/65ecdc54baece8049b76ee11a1ed69f9436d5be9
Cr-Commit-Position: refs/heads/master@{#365629}
==========

[commit-bot: I haz the power, 2015-12-16 21:50:13]

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