Add support for printing multicol containers, and enable it.

Add support for printing multicol containers, and enable it.

Introduce an abstract class FragmentationContext, which is either implemented
by LayoutMultiColumnFlowThread for multicol, or by the new
ViewFragmentationContext class, which is attached to a LayoutView when
printing. This way it will act as an enclosing fragmentation context for a
multicol container in the document. This is similar to how an outer multicol
container acts as an enclosing fragmentation context for an inner multicol
container.

The multicol flow thread implementation will now obtain and use a
FragmentationContext when attempting to locate its enclosing fragmentation
context, rather than only looking for another flowthread up there (and assume
that it's not nested if none was found). A big part of this CL is to teach the
multicol implementation about this, which means that there are quite a few
mechanical changes from enclosingFlowThread() (and LayoutMultiColumnFlowThread
method calls) to enclosingFragmentationContext() (and FragmentationContext
method calls).

Replaced an old printing test that tested that multicol didn't work, with one
that tests that multicol does work. :)

Also added another test that splits a multicol container over two pages.

BUG=99358
R=leviw@chromium.org

Committed: https://crrev.com/562a7409200d3bffd2e1270e227e36d87724a07c
Cr-Commit-Position: refs/heads/master@{#363838}

[mstensho (USE GERRIT), 2015-12-03 22:03:59]

We should probably land https://codereview.chromium.org/1492993002/ first, but
then again, it's no big deal, as long as it lands soon-ish.

[mstensho (USE GERRIT), 2015-12-08 08:45:52]

@leviw - can you review this, please?

[leviw_travelin_and_unemployed, 2015-12-08 19:00:55]

Apologies for the delay. Mostly I think this looks fine, I just find the
LayoutView code a bit confusing/unfortunate. Perhaps you could help me along to
your reasoning.

FWIW, I'm okay with pushing off the print page height testRunner support to the
future.

https://codereview.chromium.org/1492143002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/printing/multicol-2-pages.html (right):

https://codereview.chromium.org/1492143002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/printing/multicol-2-pages.html:10: assumes that 5
lines will fit in each column on the first page, and the remaining ones on the
D'oh :( We could add a testRunner feature to enable that :-/

https://codereview.chromium.org/1492143002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutView.cpp (right):

https://codereview.chromium.org/1492143002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutView.cpp:258:
m_fragmentationContext.clear();
It seems a little weird to new a ViewFragmentationContext every time the view
gets height. We could avoid some thrashing if FragmentationContext had a concept
of being active.

I see we can set the pageLogicalHeight above:
if (!document().paginated())
        setPageLogicalHeight(0);

but then we may bail before getting to this code if !needsLayout(). Does this
code properly account for that case?

[mstensho (USE GERRIT), 2015-12-08 20:09:12]

Made some changes in LayoutView. See if you like it better.

https://codereview.chromium.org/1492143002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/printing/multicol-2-pages.html (right):

https://codereview.chromium.org/1492143002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/printing/multicol-2-pages.html:10: assumes that 5
lines will fit in each column on the first page, and the remaining ones on the
On 2015/12/08 19:00:55, leviw wrote:
> D'oh :( We could add a testRunner feature to enable that :-/

Uh... yeah, I'd like that. I actually tried to just make testRunner handle @page
{ size:<...>; }. That's pretty straight-forward, but it turned out that the code
is generally confused when it comes to page size. I managed to make this work,
but what I specified in @page wasn't exactly the page size I got in practice.
Off by a few percents or so.

I stepped through the code (in Chrome). If you have @page { size:200px; }

Document::pageSizeAndMarginsInPixels() does exactly what it says. I don't
remember now who called it, but I think it was on behalf of non-Blink code. The
first thing we do afterwards is convert it to points. There are some complicated
dance steps to follow here, but we end up with 150pt. This makes sense, since
CSS says that DPI is always 96, so 200px == 150pt. Then we call some Blink API
(don't remember which one), which specifies the page parameters with struct
WebPrintParams, where all values are in pt, and we also get this right. Then,
back on the Blink side, probably in WebLocalFrameImpl::printBegin(), we are told
to make 150pt wide and tall pages. So far so good. We pass this to
PrintContext::begin(), which calls deeper into Blink. At this point, everyone
thinks we have the values specified in pixels, so we'd end up with 150px, not
150pt. But wait! There's more! PrintContext::begin() scales the size by 1.25
(printingMinimumShrinkFactor), because.... PRINTING! So if @page asks for a
200px by 200px page, we get a 187.5px by 187.5px one. Pretty close, indeed,
given the circumstances.

Long story short: Better clean up this before attempting to make reliable tests.

https://codereview.chromium.org/1492143002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutView.cpp (right):

https://codereview.chromium.org/1492143002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutView.cpp:258:
m_fragmentationContext.clear();
On 2015/12/08 19:00:55, leviw wrote:
> It seems a little weird to new a ViewFragmentationContext every time the view
> gets height.

Gets *page* height, not just any height. Right?

That's definitely not my intention. I only want to new it every time we enter
printing (i.e. get a page logical height). And I want to delete it when we leave
printing.

> We could avoid some thrashing if FragmentationContext had a concept
> of being active.

Sure, that would be easy enough to do (just add a flag, and have the descendant
multicol container ask "Do I have an enclosing ViewFragmentationContext? Am I
sure? Hey, you up there, are you active?"). But do we really need it?

> I see we can set the pageLogicalHeight above:
> if (!document().paginated())
>         setPageLogicalHeight(0);
> 
> but then we may bail before getting to this code if !needsLayout(). Does this
> code properly account for that case?

I would imagine that switching from printing mode to normal mode will always
mark the view for layout, but yeah, better safe than sorry.

I'll move the code a bit further up, and piggy-back it on
shouldUsePrintingLayout() (only the root frame will normally paginate, so this
makes sense to check). I'll wait for more comments from you before doing
anything else.

[leviw_travelin_and_unemployed, 2015-12-08 21:30:27]

On 2015/12/08 at 20:09:12, mstensho wrote:
> Made some changes in LayoutView. See if you like it better.
> 
>
https://codereview.chromium.org/1492143002/diff/40001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/printing/multicol-2-pages.html (right):
> 
>
https://codereview.chromium.org/1492143002/diff/40001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/printing/multicol-2-pages.html:10: assumes that
5 lines will fit in each column on the first page, and the remaining ones on the
> On 2015/12/08 19:00:55, leviw wrote:
> > D'oh :( We could add a testRunner feature to enable that :-/
> 
> Uh... yeah, I'd like that. I actually tried to just make testRunner handle
@page { size:<...>; }. That's pretty straight-forward, but it turned out that
the code is generally confused when it comes to page size. I managed to make
this work, but what I specified in @page wasn't exactly the page size I got in
practice. Off by a few percents or so.
> 
> I stepped through the code (in Chrome). If you have @page { size:200px; }
> 
> Document::pageSizeAndMarginsInPixels() does exactly what it says. I don't
remember now who called it, but I think it was on behalf of non-Blink code. The
first thing we do afterwards is convert it to points. There are some complicated
dance steps to follow here, but we end up with 150pt. This makes sense, since
CSS says that DPI is always 96, so 200px == 150pt. Then we call some Blink API
(don't remember which one), which specifies the page parameters with struct
WebPrintParams, where all values are in pt, and we also get this right. Then,
back on the Blink side, probably in WebLocalFrameImpl::printBegin(), we are told
to make 150pt wide and tall pages. So far so good. We pass this to
PrintContext::begin(), which calls deeper into Blink. At this point, everyone
thinks we have the values specified in pixels, so we'd end up with 150px, not
150pt. But wait! There's more! PrintContext::begin() scales the size by 1.25
(printingMinimumShrinkFactor), because.... PRINTING! So if @page asks for a
200px by 200px page, we get a 187.5px by 187.5px one. Pretty close, indeed,
given the circumstances.

I think I just threw up in my mouth a little bit. Where the hell did the 1.25
come from? I started a git log -S 10 minutes ago to find the origin, but all
it's found so far is old ChangeLog tweaks. *sigh*

> 
> Long story short: Better clean up this before attempting to make reliable
tests.
> 
>
https://codereview.chromium.org/1492143002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutView.cpp (right):
> 
>
https://codereview.chromium.org/1492143002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutView.cpp:258:
m_fragmentationContext.clear();
> On 2015/12/08 19:00:55, leviw wrote:
> > It seems a little weird to new a ViewFragmentationContext every time the
view
> > gets height.
> 
> Gets *page* height, not just any height. Right?
> 
> That's definitely not my intention. I only want to new it every time we enter
printing (i.e. get a page logical height). And I want to delete it when we leave
printing.
> 
> > We could avoid some thrashing if FragmentationContext had a concept
> > of being active.
> 
> Sure, that would be easy enough to do (just add a flag, and have the
descendant multicol container ask "Do I have an enclosing
ViewFragmentationContext? Am I sure? Hey, you up there, are you active?"). But
do we really need it?

I'm sold on your original approach. I was getting ahead of myself a bit here.

> 
> > I see we can set the pageLogicalHeight above:
> > if (!document().paginated())
> >         setPageLogicalHeight(0);
> > 
> > but then we may bail before getting to this code if !needsLayout(). Does
this
> > code properly account for that case?
> 
> I would imagine that switching from printing mode to normal mode will always
mark the view for layout, but yeah, better safe than sorry.
> 
> I'll move the code a bit further up, and piggy-back it on
shouldUsePrintingLayout() (only the root frame will normally paginate, so this
makes sense to check). I'll wait for more comments from you before doing
anything else.

This will satisfy me.


LGTM.

[mstensho (USE GERRIT), 2015-12-08 22:15:48]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2015-12-08 22:17:02]

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

[leviw_travelin_and_unemployed, 2015-12-08 22:25:02]

On 2015/12/08 at 21:30:27, leviw wrote:
> On 2015/12/08 at 20:09:12, mstensho wrote:
> > Made some changes in LayoutView. See if you like it better.
> > 
> >
https://codereview.chromium.org/1492143002/diff/40001/third_party/WebKit/Layo...
> > File third_party/WebKit/LayoutTests/printing/multicol-2-pages.html (right):
> > 
> >
https://codereview.chromium.org/1492143002/diff/40001/third_party/WebKit/Layo...
> > third_party/WebKit/LayoutTests/printing/multicol-2-pages.html:10: assumes
that 5 lines will fit in each column on the first page, and the remaining ones
on the
> > On 2015/12/08 19:00:55, leviw wrote:
> > > D'oh :( We could add a testRunner feature to enable that :-/
> > 
> > Uh... yeah, I'd like that. I actually tried to just make testRunner handle
@page { size:<...>; }. That's pretty straight-forward, but it turned out that
the code is generally confused when it comes to page size. I managed to make
this work, but what I specified in @page wasn't exactly the page size I got in
practice. Off by a few percents or so.
> > 
> > I stepped through the code (in Chrome). If you have @page { size:200px; }
> > 
> > Document::pageSizeAndMarginsInPixels() does exactly what it says. I don't
remember now who called it, but I think it was on behalf of non-Blink code. The
first thing we do afterwards is convert it to points. There are some complicated
dance steps to follow here, but we end up with 150pt. This makes sense, since
CSS says that DPI is always 96, so 200px == 150pt. Then we call some Blink API
(don't remember which one), which specifies the page parameters with struct
WebPrintParams, where all values are in pt, and we also get this right. Then,
back on the Blink side, probably in WebLocalFrameImpl::printBegin(), we are told
to make 150pt wide and tall pages. So far so good. We pass this to
PrintContext::begin(), which calls deeper into Blink. At this point, everyone
thinks we have the values specified in pixels, so we'd end up with 150px, not
150pt. But wait! There's more! PrintContext::begin() scales the size by 1.25
(printingMinimumShrinkFactor), because.... PRINTING! So if @page asks for a
200px by 200px page, we get a 187.5px by 187.5px one. Pretty close, indeed,
given the circumstances.
> 
> I think I just threw up in my mouth a little bit. Where the hell did the 1.25
come from? I started a git log -S 10 minutes ago to find the origin, but all
it's found so far is old ChangeLog tweaks. *sigh*

git show 7ca6d7ac7f6ef0d87adc5edc61141c83b927821e

"Lay out the page into a width 25% wider than there's room for on the printed
page.
This will make pages that can fit into a thin area be scaled down a little when
printed,
which lets them fit on fewer pages. This closely matches what IE and Camino (at
least)
do; I used Google as my test page, and the Google logo is now precisely the same
size
when printed from Safari as when printed from IE. Pages that don't fit into a
thin
area are already causing the printed page to be scaled horizontally to fit, and
this
won't affect them."

[leviw_travelin_and_unemployed, 2015-12-08 22:26:45]

The goal was to lay out contents 25% wider than there was room for on the page.
The result 12 years later is we're using 0.9375 of the page.

I love this so much.

[commit-bot: I haz the power, 2015-12-08 23:15:49]

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

[commit-bot: I haz the power, 2015-12-08 23:15:52]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mstensho (USE GERRIT), 2015-12-08 23:18:50]

The CQ bit was checked by mstensho@opera.com

[mstensho (USE GERRIT), 2015-12-08 23:24:07]

On 2015/12/08 22:26:45, leviw wrote:
> The goal was to lay out contents 25% wider than there was room for on the
page.
> The result 12 years later is we're using 0.9375 of the page.
> 
> I love this so much.

Yeah, it's neat. But I think it still works as intended, unless you try to set
the page size with @page { size:<...> }. But hard to tell for sure. I don't
speak Printing fluently.

[commit-bot: I haz the power, 2015-12-08 23:25:44]

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

[commit-bot: I haz the power, 2015-12-09 00:22:54]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-09 00:23:59]

Description was changed from

==========
Add support for printing multicol containers, and enable it.

Introduce an abstract class FragmentationContext, which is either implemented
by LayoutMultiColumnFlowThread for multicol, or by the new
ViewFragmentationContext class, which is attached to a LayoutView when
printing. This way it will act as an enclosing fragmentation context for a
multicol container in the document. This is similar to how an outer multicol
container acts as an enclosing fragmentation context for an inner multicol
container.

The multicol flow thread implementation will now obtain and use a
FragmentationContext when attempting to locate its enclosing fragmentation
context, rather than only looking for another flowthread up there (and assume
that it's not nested if none was found). A big part of this CL is to teach the
multicol implementation about this, which means that there are quite a few
mechanical changes from enclosingFlowThread() (and LayoutMultiColumnFlowThread
method calls) to enclosingFragmentationContext() (and FragmentationContext
method calls).

Replaced an old printing test that tested that multicol didn't work, with one
that tests that multicol does work. :)

Also added another test that splits a multicol container over two pages.

BUG=99358
R=leviw@chromium.org
==========

to

==========
Add support for printing multicol containers, and enable it.

Introduce an abstract class FragmentationContext, which is either implemented
by LayoutMultiColumnFlowThread for multicol, or by the new
ViewFragmentationContext class, which is attached to a LayoutView when
printing. This way it will act as an enclosing fragmentation context for a
multicol container in the document. This is similar to how an outer multicol
container acts as an enclosing fragmentation context for an inner multicol
container.

The multicol flow thread implementation will now obtain and use a
FragmentationContext when attempting to locate its enclosing fragmentation
context, rather than only looking for another flowthread up there (and assume
that it's not nested if none was found). A big part of this CL is to teach the
multicol implementation about this, which means that there are quite a few
mechanical changes from enclosingFlowThread() (and LayoutMultiColumnFlowThread
method calls) to enclosingFragmentationContext() (and FragmentationContext
method calls).

Replaced an old printing test that tested that multicol didn't work, with one
that tests that multicol does work. :)

Also added another test that splits a multicol container over two pages.

BUG=99358
R=leviw@chromium.org

Committed: https://crrev.com/562a7409200d3bffd2e1270e227e36d87724a07c
Cr-Commit-Position: refs/heads/master@{#363838}
==========

[commit-bot: I haz the power, 2015-12-09 00:24:00]

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