Propagate writing-mode from the first region to the flow thread.

Propagate writing-mode from the first region to the flow thread.

Since the flow thread is a direct child of RenderView, it doesn't inherit
proper writing-mode automatically. It should be mentioned that if the thread's
contents' writing-mode differs from that of the first region, things are
typically going to look useless (although perhaps that's how it should be),
but as long as writing-mode is only specified on a common parent of the
thread's contents and the regions, things look fine, and also, we're now
following what the spec has to say on the matter:

http://www.w3.org/TR/2013/WD-css3-regions-20130528/#the-flow-into-property

    "The first region defines the principal writing mode for the entire flow.
     The writing mode on subsequent regions is ignored."

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

[mstensho (USE GERRIT), 2013-07-24 20:54:24]

@pfeldman: would you be the right person to review this, or should I choose
someone else?

[pfeldman, 2013-07-29 21:54:09]

Sorry for the delay. No, I am not the right person. I cover inspector-related
work.

[mihnea, 2013-07-30 15:50:44]

(this is an informal review, feel free to ignore it)

1. Have you tested what happens when you dynamically change the writing mode of
the first region?

2. I looked over the tests and i was not able to see:
* a test in which the first region with a different writing mode than the second
region is dynamically removed so that the flow thread writing mode gets updated
to the second region writing mode
* a test in which the content node (the one with flow-into) has a different
writing mode than the first region

I also had a comment about a check below in
RenderNamedFlowThread::updateWritingMode.

https://codereview.chromium.org/18374008/diff/1/Source/core/rendering/RenderN...
File Source/core/rendering/RenderNamedFlowThread.cpp (right):

https://codereview.chromium.org/18374008/diff/1/Source/core/rendering/RenderN...
Source/core/rendering/RenderNamedFlowThread.cpp:95: if (m_regionList.isEmpty())
I believe you want to add an assert here instead. In which situation do you call
this function with an empty region list?

[mihnea, 2013-07-30 15:51:58]

(this is an informal review, feel free to ignore it)

1. Have you tested what happens when you dynamically change the writing mode of
the first region?

2. I looked over the tests and i was not able to see:
* a test in which the first region with a different writing mode than the second
region is dynamically removed so that the flow thread writing mode gets updated
to the second region writing mode
* a test in which the content node (the one with flow-into) has a different
writing mode than the first region

I also had a comment about a check below in
RenderNamedFlowThread::updateWritingMode.

[mstensho (USE GERRIT), 2013-07-30 19:36:20]

On 2013/07/30 15:51:58, mihnea wrote:
> (this is an informal review, feel free to ignore it)
> 
> 1. Have you tested what happens when you dynamically change the writing mode
of
> the first region?

Ehrm... no, but I have now, and it doesn't work. :) Will fix (and add test).

> 2. I looked over the tests and i was not able to see:
> * a test in which the first region with a different writing mode than the
second

I'm not exactly sure how that's supposed to work, and I consider it to be
outside the scope of this patch.

> region is dynamically removed so that the flow thread writing mode gets
updated
> to the second region writing mode

Like I wrote above: I'm not sure how we should behave when mixing writing modes,
but yes, I can add a test to verify that we at least behave consistently,
regardless of styles/content being changed dynamically or not.

> * a test in which the content node (the one with flow-into) has a different
> writing mode than the first region

Like above. I hope we don't have to involve mixing of writing modes in this
patch.

Thanks for the informal review!

[mstensho (USE GERRIT), 2013-07-31 13:09:37]

Changing reviewer to apavlov. Hope that's better.

https://codereview.chromium.org/18374008/diff/1/Source/core/rendering/RenderN...
File Source/core/rendering/RenderNamedFlowThread.cpp (right):

https://codereview.chromium.org/18374008/diff/1/Source/core/rendering/RenderN...
Source/core/rendering/RenderNamedFlowThread.cpp:95: if (m_regionList.isEmpty())
On 2013/07/30 15:50:44, mihnea wrote:
> I believe you want to add an assert here instead. In which situation do you
call
> this function with an empty region list?

If you remove the last region, this list may be empty. But yes, the way the code
is right now, this method isn't even called then. I'm not sure. It's probably a
matter of taste. My idea was that this method should cope also when there are no
regions.

[mihnea, 2013-08-01 06:10:54]

On 2013/07/31 13:09:37, Morten Stenshorne wrote:
> Changing reviewer to apavlov. Hope that's better.
> 
>
https://codereview.chromium.org/18374008/diff/1/Source/core/rendering/RenderN...
> File Source/core/rendering/RenderNamedFlowThread.cpp (right):
> 
>
https://codereview.chromium.org/18374008/diff/1/Source/core/rendering/RenderN...
> Source/core/rendering/RenderNamedFlowThread.cpp:95: if
(m_regionList.isEmpty())
> On 2013/07/30 15:50:44, mihnea wrote:
> > I believe you want to add an assert here instead. In which situation do you
> call
> > this function with an empty region list?
> 
> If you remove the last region, this list may be empty. But yes, the way the
code
> is right now, this method isn't even called then. I'm not sure. It's probably
a
> matter of taste. My idea was that this method should cope also when there are
no
> regions.

For regions, the best reviewers are jchaffraix, eseidel, esprehn and ojan.

[apavlov, 2013-08-01 08:22:33]

On 2013/08/01 06:10:54, mihnea wrote:
> On 2013/07/31 13:09:37, Morten Stenshorne wrote:
> > Changing reviewer to apavlov. Hope that's better.
> > 
> >
>
https://codereview.chromium.org/18374008/diff/1/Source/core/rendering/RenderN...
> > File Source/core/rendering/RenderNamedFlowThread.cpp (right):
> > 
> >
>
https://codereview.chromium.org/18374008/diff/1/Source/core/rendering/RenderN...
> > Source/core/rendering/RenderNamedFlowThread.cpp:95: if
> (m_regionList.isEmpty())
> > On 2013/07/30 15:50:44, mihnea wrote:
> > > I believe you want to add an assert here instead. In which situation do
you
> > call
> > > this function with an empty region list?
> > 
> > If you remove the last region, this list may be empty. But yes, the way the
> code
> > is right now, this method isn't even called then. I'm not sure. It's
probably
> a
> > matter of taste. My idea was that this method should cope also when there
are
> no
> > regions.
> 
> For regions, the best reviewers are jchaffraix, eseidel, esprehn and ojan.

That is correct. Pavel and I are on the DevTools team and can effectively review
only the regions instrumentation and processing in the DevTools agents.

[esprehn, 2013-08-07 05:23:41]

I'll review this tomorrow, sorry for the delay.

[esprehn, 2013-08-08 03:10:38]

LGTM, please fix the tests to use getElementById and the argument name before
landing.

https://codereview.chromium.org/18374008/diff/10001/LayoutTests/fast/regions/...
File LayoutTests/fast/regions/changing-writing-mode-5.html (right):

https://codereview.chromium.org/18374008/diff/10001/LayoutTests/fast/regions/...
LayoutTests/fast/regions/changing-writing-mode-5.html:43:
firstregion.style.display = 'block';
do getElementById please.

https://codereview.chromium.org/18374008/diff/10001/LayoutTests/fast/regions/...
File LayoutTests/fast/regions/changing-writing-mode.html (right):

https://codereview.chromium.org/18374008/diff/10001/LayoutTests/fast/regions/...
LayoutTests/fast/regions/changing-writing-mode.html:18: container.offsetLeft; //
Make sure we have laid out.
Please don't access vars like this:
var container = document.getElementById('container');

https://codereview.chromium.org/18374008/diff/10001/Source/core/rendering/Ren...
File Source/core/rendering/RenderFlowThread.h (right):

https://codereview.chromium.org/18374008/diff/10001/Source/core/rendering/Ren...
Source/core/rendering/RenderFlowThread.h:90: virtual void
regionChangedWritingMode(RenderRegion* region) { }
leave off "region", it adds no value.

[mstensho (USE GERRIT), 2013-08-08 06:51:50]

https://codereview.chromium.org/18374008/diff/10001/LayoutTests/fast/regions/...
File LayoutTests/fast/regions/changing-writing-mode-5.html (right):

https://codereview.chromium.org/18374008/diff/10001/LayoutTests/fast/regions/...
LayoutTests/fast/regions/changing-writing-mode-5.html:43:
firstregion.style.display = 'block';
On 2013/08/08 03:10:39, esprehn wrote:
> do getElementById please.

Done.

https://codereview.chromium.org/18374008/diff/10001/LayoutTests/fast/regions/...
File LayoutTests/fast/regions/changing-writing-mode.html (right):

https://codereview.chromium.org/18374008/diff/10001/LayoutTests/fast/regions/...
LayoutTests/fast/regions/changing-writing-mode.html:18: container.offsetLeft; //
Make sure we have laid out.
On 2013/08/08 03:10:39, esprehn wrote:
> Please don't access vars like this:
> var container = document.getElementById('container');

I did "document.getElementById('container').offsetLeft" etc. instead. Hope
that's fine.

BTW, I'd like to know why it's a problem to address elements directly, like I
did. Engine compatibility? I seem to remember that it doesn't work everywhere.
Gecko?

https://codereview.chromium.org/18374008/diff/10001/Source/core/rendering/Ren...
File Source/core/rendering/RenderFlowThread.h (right):

https://codereview.chromium.org/18374008/diff/10001/Source/core/rendering/Ren...
Source/core/rendering/RenderFlowThread.h:90: virtual void
regionChangedWritingMode(RenderRegion* region) { }
On 2013/08/08 03:10:39, esprehn wrote:
> leave off "region", it adds no value.

Done.

[commit-bot: I haz the power, 2013-08-08 08:46:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mstensho@opera.com/18374008/26001

[commit-bot: I haz the power, 2013-08-08 15:59:40]

Change committed as 155779

[mihnea, 2013-08-13 13:44:12]

On 2013/08/08 06:51:50, Morten Stenshorne wrote:
>
https://codereview.chromium.org/18374008/diff/10001/LayoutTests/fast/regions/...
> File LayoutTests/fast/regions/changing-writing-mode-5.html (right):
> 
>
https://codereview.chromium.org/18374008/diff/10001/LayoutTests/fast/regions/...
> LayoutTests/fast/regions/changing-writing-mode-5.html:43:
> firstregion.style.display = 'block';
> On 2013/08/08 03:10:39, esprehn wrote:
> > do getElementById please.
> 
> Done.
> 
>
https://codereview.chromium.org/18374008/diff/10001/LayoutTests/fast/regions/...
> File LayoutTests/fast/regions/changing-writing-mode.html (right):
> 
>
https://codereview.chromium.org/18374008/diff/10001/LayoutTests/fast/regions/...
> LayoutTests/fast/regions/changing-writing-mode.html:18: container.offsetLeft;
//
> Make sure we have laid out.
> On 2013/08/08 03:10:39, esprehn wrote:
> > Please don't access vars like this:
> > var container = document.getElementById('container');
> 
> I did "document.getElementById('container').offsetLeft" etc. instead. Hope
> that's fine.
> 
> BTW, I'd like to know why it's a problem to address elements directly, like I
> did. Engine compatibility? I seem to remember that it doesn't work everywhere.
> Gecko?
> 
>
https://codereview.chromium.org/18374008/diff/10001/Source/core/rendering/Ren...
> File Source/core/rendering/RenderFlowThread.h (right):
> 
>
https://codereview.chromium.org/18374008/diff/10001/Source/core/rendering/Ren...
> Source/core/rendering/RenderFlowThread.h:90: virtual void
> regionChangedWritingMode(RenderRegion* region) { }
> On 2013/08/08 03:10:39, esprehn wrote:
> > leave off "region", it adds no value.
> 
> Done.

Hi,
Would you consider adding this functionality back to WebKit?
Cheers,
M.

[mstensho (USE GERRIT), 2013-08-13 15:33:01]

> Hi,
> Would you consider adding this functionality back to WebKit?
> Cheers,

Sure! Can you suggest a reviewer?

[mihnea, 2013-08-14 12:06:29]

On 2013/08/13 15:33:01, Morten Stenshorne wrote:
> > Hi,
> > Would you consider adding this functionality back to WebKit?
> > Cheers,
> 
> Sure! Can you suggest a reviewer?

David Hyatt

[mstensho (USE GERRIT), 2013-08-16 13:11:50]

On 2013/08/14 12:06:29, mihnea wrote:
> On 2013/08/13 15:33:01, Morten Stenshorne wrote:
> > > Hi,
> > > Would you consider adding this functionality back to WebKit?
> > > Cheers,
> > 
> > Sure! Can you suggest a reviewer?
> 
> David Hyatt

https://bugs.webkit.org/show_bug.cgi?id=119795

[mstensho (USE GERRIT), 2013-08-19 18:51:37]

On 2013/08/16 13:11:50, Morten Stenshorne wrote:
> On 2013/08/14 12:06:29, mihnea wrote:
> > On 2013/08/13 15:33:01, Morten Stenshorne wrote:
> > > > Hi,
> > > > Would you consider adding this functionality back to WebKit?
> > > > Cheers,
> > > 
> > > Sure! Can you suggest a reviewer?
> > 
> > David Hyatt
> 
> https://bugs.webkit.org/show_bug.cgi?id=119795

That patch has landed, but additional issues were raised and handled during code
review. I've ported them back to Blink:
https://codereview.chromium.org/23064019/