Bug 101332

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.