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
Andrei Bucur
2012-11-06 04:09:09 PST
Created attachment 172784 [details]
Patch
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...
Created attachment 173122 [details]
Patch
Comment on attachment 173122 [details] Patch Attachment 173122 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14778123 Comment on attachment 173122 [details] Patch Attachment 173122 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14760968 Comment on attachment 173122 [details] Patch Attachment 173122 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14759980 Comment on attachment 173122 [details] Patch Attachment 173122 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14759988 Comment on attachment 173122 [details] Patch Attachment 173122 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14763796 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 on attachment 173122 [details] Patch Attachment 173122 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14779236 Created attachment 173242 [details]
Patch
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 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. Created attachment 173289 [details]
Patch
Comment on attachment 173289 [details]
Patch
r=me
Comment on attachment 173289 [details] Patch Clearing flags on attachment: 173289 Committed r134482: <http://trac.webkit.org/changeset/134482> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by bug 102140 Created attachment 174417 [details]
Patch
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 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 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 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 on attachment 174417 [details]
Patch
r+
Comment on attachment 174417 [details] Patch Clearing flags on attachment: 174417 Committed r135750: <http://trac.webkit.org/changeset/135750> All reviewed patches have been landed. Closing bug. |