Bug 101332

Summary: [CSS Regions] Add Region info for RootLineBoxes and pack the pagination data
Product: WebKit Reporter: Andrei Bucur <abucur>
Component: WebCore Misc.Assignee: Andrei Bucur <abucur>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, eric, gustavo, hyatt, jchaffraix, mihnea, peter+ews, philn, WebkitBugTracker, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2 Keywords: AdobeTracked
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 102140    
Bug Blocks: 95559    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Andrei Bucur 2012-11-06 04:09:09 PST
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.
Comment 1 Andrei Bucur 2012-11-07 06:32:33 PST
Created attachment 172784 [details]
Patch
Comment 2 Andrei Bucur 2012-11-07 15:44:16 PST
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...
Comment 3 Andrei Bucur 2012-11-08 14:45:51 PST
Created attachment 173122 [details]
Patch
Comment 4 Early Warning System Bot 2012-11-08 14:52:06 PST
Comment on attachment 173122 [details]
Patch

Attachment 173122 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14778123
Comment 5 Early Warning System Bot 2012-11-08 14:52:32 PST
Comment on attachment 173122 [details]
Patch

Attachment 173122 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14760968
Comment 6 Build Bot 2012-11-08 15:19:06 PST
Comment on attachment 173122 [details]
Patch

Attachment 173122 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14759980
Comment 7 Build Bot 2012-11-08 15:52:08 PST
Comment on attachment 173122 [details]
Patch

Attachment 173122 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14759988
Comment 8 EFL EWS Bot 2012-11-08 15:55:38 PST
Comment on attachment 173122 [details]
Patch

Attachment 173122 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14763796
Comment 9 Peter Beverloo (cr-android ews) 2012-11-08 20:46:26 PST
Comment on attachment 173122 [details]
Patch

Attachment 173122 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14762967
Comment 10 WebKit Review Bot 2012-11-08 22:36:29 PST
Comment on attachment 173122 [details]
Patch

Attachment 173122 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14779236
Comment 11 Andrei Bucur 2012-11-09 01:57:42 PST
Created attachment 173242 [details]
Patch
Comment 12 Mihnea Ovidenie 2012-11-09 03:11:34 PST
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?
Comment 13 Andrei Bucur 2012-11-09 03:25:53 PST
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.
Comment 14 Andrei Bucur 2012-11-09 06:10:29 PST
Created attachment 173289 [details]
Patch
Comment 15 Dave Hyatt 2012-11-13 13:55:48 PST
Comment on attachment 173289 [details]
Patch

r=me
Comment 16 WebKit Review Bot 2012-11-13 14:14:39 PST
Comment on attachment 173289 [details]
Patch

Clearing flags on attachment: 173289

Committed r134482: <http://trac.webkit.org/changeset/134482>
Comment 17 WebKit Review Bot 2012-11-13 14:14:44 PST
All reviewed patches have been landed.  Closing bug.
Comment 18 WebKit Review Bot 2012-11-13 15:05:10 PST
Re-opened since this is blocked by bug 102140
Comment 19 Andrei Bucur 2012-11-15 05:49:59 PST
Created attachment 174417 [details]
Patch
Comment 20 WebKit Review Bot 2012-11-15 07:06:00 PST
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
Comment 21 Dave Hyatt 2012-11-15 13:09:21 PST
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.
Comment 22 Andrei Bucur 2012-11-15 13:42:47 PST
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.
Comment 23 Andrei Bucur 2012-11-15 13:49:46 PST
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.
Comment 24 Dave Hyatt 2012-11-26 11:52:35 PST
Comment on attachment 174417 [details]
Patch

r+
Comment 25 WebKit Review Bot 2012-11-26 12:01:37 PST
Comment on attachment 174417 [details]
Patch

Clearing flags on attachment: 174417

Committed r135750: <http://trac.webkit.org/changeset/135750>
Comment 26 WebKit Review Bot 2012-11-26 12:01:44 PST
All reviewed patches have been landed.  Closing bug.