Bug 101332 – [CSS Regions] Add Region info for RootLineBoxes and pack the pagination data

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Andrei Bucur]

Pagination data is held on the RootInlineBoxes even though it's not a pagination layout. Currently, it's about the m_paginatedLineWidth and the m_paginationStrut. A new one is necessary, the current Region, that can be used to detect when lines move between regions during layout.
The pagination data should be packed and allocated only during a pagination layout.

[Andrei Bucur]

Created attachment 172784 [details]
Patch

[Andrei Bucur]

Comment on attachment 172784 [details]
Patch

Ooops, the patch doesn't cover all the cases (block shifted) to update the containing region. Don't know how I missed that...

[Andrei Bucur]

Created attachment 173122 [details]
Patch

[Early Warning System Bot]

Comment on attachment 173122 [details]
Patch

Attachment 173122 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14778123

[Early Warning System Bot]

Comment on attachment 173122 [details]
Patch

Attachment 173122 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14760968

[Build Bot]

Comment on attachment 173122 [details]
Patch

Attachment 173122 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14759980

[Build Bot]

Comment on attachment 173122 [details]
Patch

Attachment 173122 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14759988

[EFL EWS Bot]

Comment on attachment 173122 [details]
Patch

Attachment 173122 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14763796

[Peter Beverloo (cr-android ews)]

Comment on attachment 173122 [details]
Patch

Attachment 173122 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14762967

[WebKit Review Bot]

Comment on attachment 173122 [details]
Patch

Attachment 173122 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14779236

[Andrei Bucur]

Created attachment 173242 [details]
Patch

[Mihnea Ovidenie]

Comment on attachment 173242 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173242&action=review

> Source/WebCore/ChangeLog:12
> +        styling for layout properties.

I would mention here the font-size for instance, to make it more clear and to distinguish from region styling paint properties like color, which are supported right now.

> Source/WebCore/rendering/RootInlineBox.h:205
> +

I would modify this function to return a reference to the m_fragmentationData and use the returned reference to set struct members.

> Source/WebCore/rendering/RootInlineBox.h:222
> +    public:

Do you need to specify public here?

[Andrei Bucur]

Comment on attachment 173242 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173242&action=review

>> Source/WebCore/ChangeLog:12
>> +        styling for layout properties.
> 
> I would mention here the font-size for instance, to make it more clear and to distinguish from region styling paint properties like color, which are supported right now.

Good point, I'll update the patch.

>> Source/WebCore/rendering/RootInlineBox.h:205
>> +
> 
> I would modify this function to return a reference to the m_fragmentationData and use the returned reference to set struct members.

Will do.

>> Source/WebCore/rendering/RootInlineBox.h:222
>> +    public:
> 
> Do you need to specify public here?

Yes, the WTF_MAKE_FAST_ALLOCATED macro starts a private declarations context.

[Andrei Bucur]

Created attachment 173289 [details]
Patch

[Dave Hyatt]

Comment on attachment 173289 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 173289 [details]
Patch

Clearing flags on attachment: 173289

Committed r134482: <http://trac.webkit.org/changeset/134482>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 102140

[Andrei Bucur]

Created attachment 174417 [details]
Patch

[WebKit Review Bot]

Comment on attachment 174417 [details]
Patch

Attachment 174417 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14831788

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html

[Dave Hyatt]

Comment on attachment 174417 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174417&action=review

r-

> Source/WebCore/rendering/RenderBlock.cpp:7254
> +    // Just bail if we still don't have a region.
> +    if (!rootBox->hasContainingRegion() && !currentRegion)
> +        return false;
> +    // Just bail if the region didn't change.
> +    if (rootBox->hasContainingRegion() && rootBox->containingRegion() == currentRegion)
> +        return false;

Isn't all this just equivalent to:

if (rootBox->containingRegion() == currentRegion)
    return false;

?

I'm not really sure what all the extra code is buying you. If both regions are null, the == check will cause you to bail anyway, and you won't ever go from null to non-null or vice versa.

[Andrei Bucur]

Comment on attachment 174417 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174417&action=review

>> Source/WebCore/rendering/RenderBlock.cpp:7254
>> +        return false;
> 
> Isn't all this just equivalent to:
> 
> if (rootBox->containingRegion() == currentRegion)
>     return false;
> 
> ?
> 
> I'm not really sure what all the extra code is buying you. If both regions are null, the == check will cause you to bail anyway, and you won't ever go from null to non-null or vice versa.

The idea is to be able to reset the layout in a situation like this one:
1. A flow with 1000 regions is laid out.
2. All the regions are deleted
3. A layout is triggered - at this point all the RootInlineBoxes have a BAD containing region attached to them.
There are two options:
- The one I've chosen in this patch - write a sanitize() function that detects such invalid regions and sets their value to 0.
- Define a reference value for invalid Regions - create a static RenderRegion* = -1 or just a static RenderRegion and set the value of the invalid containing region to the address of the dummy Region. - I think this option is worse because there will be two special values that need to be handled - 0 and the reference value
4. Solve the invalid region situation with one of the options above.
5. If going with the first option, containingRegion == currentRegion is not enough to determine if a line is invalid. containingRegion can be a sanitized 0 value while currentRegion just means there are no more regions in the flow. So the lines may need to be invalidated because they were laid out in a region - the hasContainingRegion() flag needs to be used.

[Andrei Bucur]

Comment on attachment 174417 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174417&action=review

>>> Source/WebCore/rendering/RenderBlock.cpp:7254
>>> +        return false;
>> 
>> Isn't all this just equivalent to:
>> 
>> if (rootBox->containingRegion() == currentRegion)
>>     return false;
>> 
>> ?
>> 
>> I'm not really sure what all the extra code is buying you. If both regions are null, the == check will cause you to bail anyway, and you won't ever go from null to non-null or vice versa.
> 
> The idea is to be able to reset the layout in a situation like this one:
> 1. A flow with 1000 regions is laid out.
> 2. All the regions are deleted
> 3. A layout is triggered - at this point all the RootInlineBoxes have a BAD containing region attached to them.
> There are two options:
> - The one I've chosen in this patch - write a sanitize() function that detects such invalid regions and sets their value to 0.
> - Define a reference value for invalid Regions - create a static RenderRegion* = -1 or just a static RenderRegion and set the value of the invalid containing region to the address of the dummy Region. - I think this option is worse because there will be two special values that need to be handled - 0 and the reference value
> 4. Solve the invalid region situation with one of the options above.
> 5. If going with the first option, containingRegion == currentRegion is not enough to determine if a line is invalid. containingRegion can be a sanitized 0 value while currentRegion just means there are no more regions in the flow. So the lines may need to be invalidated because they were laid out in a region - the hasContainingRegion() flag needs to be used.

Unless you think that the layout behavior for a flow without any region is undefined. In this case it makes sense to just compare the two values and hasContainingRegion goes out as well.

[Dave Hyatt]

Comment on attachment 174417 [details]
Patch

r+

[WebKit Review Bot]

Comment on attachment 174417 [details]
Patch

Clearing flags on attachment: 174417

Committed r135750: <http://trac.webkit.org/changeset/135750>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.