Documents with many lines overallocated pagination info.

Documents with many lines overallocated pagination info.

There is an object LineFragmentationData that is used for pagination
that is created on demand. That object was created for normal documents
and cost roughly 32 bytes per line, adding up to 3-5 MB on a big text
document (5% of all memory usage).

Testcase: http://norvig.com/big.txt - 3-10 MB RAM savings.

I don't know what I'm doing but this patch prevents the allocation and
saves the memory.

BUG=353993
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170497

[Daniel Bratell, 2014-03-19 13:09:26]

dglazkov, can you please take a look at this one-liner? (Asking you because I
think you were the last owner to touch this file)

I'm not certain I'm determining whether there is pagination available in the
right way or not but I've run with this patch for a while and don't know of any
problems caused by it.

[dglazkov, 2014-03-19 15:45:27]

if I touched it, it might have been an accident :P

Julien, Levi, can you guys give this a look?

[leviw_travelin_and_unemployed, 2014-03-20 18:05:38]

https://codereview.chromium.org/200053007/diff/1/Source/core/rendering/RootIn...
File Source/core/rendering/RootInlineBox.cpp (right):

https://codereview.chromium.org/200053007/diff/1/Source/core/rendering/RootIn...
Source/core/rendering/RootInlineBox.cpp:252: bool paginated =
block().view()->layoutState() && block().view()->layoutState()->isPaginated();
If there's no LayoutState, this logic doesn't seem right.

[leviw_travelin_and_unemployed, 2014-03-20 18:41:26]

https://codereview.chromium.org/200053007/diff/1/Source/core/rendering/RootIn...
File Source/core/rendering/RootInlineBox.cpp (right):

https://codereview.chromium.org/200053007/diff/1/Source/core/rendering/RootIn...
Source/core/rendering/RootInlineBox.cpp:252: bool paginated =
block().view()->layoutState() && block().view()->layoutState()->isPaginated();
So thinking about this more, we should never be in this code without a
LayoutState. I've also verified, with an ASSERT, that we don't currently ever
hit this code path without one.

I think the proper fix is to just use if
(block().view()->layoutState()->isPaginated()) as your if statement.

[Daniel Bratell, 2014-03-21 10:33:15]

Good, then the code is even shorter!

This looking ok?

https://codereview.chromium.org/200053007/diff/1/Source/core/rendering/RootIn...
File Source/core/rendering/RootInlineBox.cpp (right):

https://codereview.chromium.org/200053007/diff/1/Source/core/rendering/RootIn...
Source/core/rendering/RootInlineBox.cpp:252: bool paginated =
block().view()->layoutState() && block().view()->layoutState()->isPaginated();
On 2014/03/20 18:41:26, Levi wrote:
> So thinking about this more, we should never be in this code without a
> LayoutState. I've also verified, with an ASSERT, that we don't currently ever
> hit this code path without one.
> 
> I think the proper fix is to just use if
> (block().view()->layoutState()->isPaginated()) as your if statement.

Done.

[Daniel Bratell, 2014-03-21 10:33:16]

Good, then the code is even shorter!

This looking ok?

[mstensho (USE GERRIT), 2014-03-28 09:49:21]

lgtm with a nit. (I'm not an owner)

https://codereview.chromium.org/200053007/diff/20001/Source/core/rendering/Ro...
File Source/core/rendering/RootInlineBox.cpp (right):

https://codereview.chromium.org/200053007/diff/20001/Source/core/rendering/Ro...
Source/core/rendering/RootInlineBox.cpp:252: bool paginated =
block().view()->layoutState()->isPaginated();
The 'paginated' variable seems rather unnecessary, if the test is that simple.

[Daniel Bratell, 2014-03-31 15:29:39]

I've done the requested simplifications.

https://codereview.chromium.org/200053007/diff/20001/Source/core/rendering/Ro...
File Source/core/rendering/RootInlineBox.cpp (right):

https://codereview.chromium.org/200053007/diff/20001/Source/core/rendering/Ro...
Source/core/rendering/RootInlineBox.cpp:252: bool paginated =
block().view()->layoutState()->isPaginated();
On 2014/03/28 09:49:21, Morten Stenshorne wrote:
> The 'paginated' variable seems rather unnecessary, if the test is that simple.

Done.

[leviw_travelin_and_unemployed, 2014-03-31 22:27:59]

You description feels like it needs an "I have no idea what I'm doing" dog :)

lgtm

[leviw_travelin_and_unemployed, 2014-03-31 22:28:05]

The CQ bit was checked by leviw@chromium.org

[commit-bot: I haz the power, 2014-03-31 22:28:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bratell@opera.com/200053007/80001

[commit-bot: I haz the power, 2014-04-01 00:17:21]

Change committed as 170497