Bug 66142 – [CSSRegions] RenderRegion should not reference a parent RenderFlowThread

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Alexandru Chiculita]

Because RenderRegion can actually be part of other RenderFlowThreads we need to check that it is not actually trying to display a parent RenderFlowThread.

Example:

<div style="flow: 'flow1';">
    <div style="content: from-flow:'flow1'></div>
</div>

Also we need to check for circular dependencies like the following one:

<div style="flow: 'flow1';">
    <div style="content: from-flow:'flow2'></div>
</div>

<div style="flow: 'flow2';">
    <div style="content: from-flow:'flow1'></div>
</div>

[Alexandru Chiculita]

Created attachment 104044 [details]
Patch

[Alexandru Chiculita]

> Created an attachment (id=104044) [details]
> Patch

The patch creates a dependency tree and adds a region to a flow thread only if it adds no circular dependency to this tree. 

The patch also moves the layout of the RenderFlowThreads to a separate method called RenderView::layoutRenderFlowThreads, so that all threads are done at the end of the RenderView layout.

It will also sort the regions, so that all the dependent RenderRegions are laid out before they are needed.

[Dave Hyatt]

Comment on attachment 104044 [details]
Patch

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

> Source/WebCore/dom/NodeRenderingContext.cpp:373
> +    if (newRenderer->isRenderRegion())
> +        toRenderRegion(newRenderer)->attachRegion();

Seems like this would be better placed in renderer methods than over here. Maybe in RenderObjectChildList::append/insert/removeChildNode instead. Check out updateListMarkerNumbers call sites in RenderObjectChildList to see where you might attach and remove regions as they enter/leave the tree.

> Source/WebCore/rendering/RenderBlock.cpp:1535
> +    if (child->isRenderFlowThread())
> +        return true;

You've turned this into a virtual function call hit by every child you check, even non-positioned ones. This also only handles the insertion of positioned objects from blocks with block-level children. Changing to:

if (child->isPositioned && !child->isRenderFlowThread())

would limit the virtual function call only to positioned children instead of making it for all children.

However, you also didn't patch insertion from blocks with inline children that happens in RenderBlockLineLayout.cpp near the top of layoutInlineChildren.

I think an easier way to handle this might be to just patch insertPositionedObject to ignore RenderFlowThreads, and that way you only have to patch one place.

[Alexandru Chiculita]

Created attachment 104070 [details]
Patch V2

Thanks for the tips. Updated the patch.

[Dave Hyatt]

Comment on attachment 104070 [details]
Patch V2

You'll need to tweak this after my landing from r93144. You'll want to ditch my m_hasRenderFlowThreads boolean now that you added the list, and you can redefine my hasRenderFlowThreads function on RenderView in terms of the list being non-empty.

You'll also want to add isValid checks to the region walk I do in repaintIntoRegions.

Sorry for the conflicts. Should be easy to update though.

[Alexandru Chiculita]

Created attachment 104083 [details]
Patch V3

Rebased.

[Dave Hyatt]

Comment on attachment 104083 [details]
Patch V3

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

Fix the one misspelled method name. Sorry I didn't catch it before.

> Source/WebCore/ChangeLog:47
> +        (WebCore::RenderRegion::dettachRegion):

Typo. Should be detachRegion

> Source/WebCore/rendering/RenderObjectChildList.cpp:106
> +            toRenderRegion(oldChild)->dettachRegion();

detachRegion

> Source/WebCore/rendering/RenderRegion.cpp:111
> +void RenderRegion::dettachRegion()

detachRegion

> Source/WebCore/rendering/RenderRegion.h:53
> +    void dettachRegion();

detachRegion

[Alexandru Chiculita]

Created attachment 104096 [details]
Patch V4

Fixed it.

[Dave Hyatt]

Comment on attachment 104096 [details]
Patch V4

r=me

[WebKit Review Bot]

Comment on attachment 104096 [details]
Patch V4

Rejecting attachment 104096 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-02', '--port..." exit_code: 2

Last 500 characters of output:
omium-mac to mean Skia instead of CoreGraphics."
Failed to merge in the changes.
Patch failed at 0001 This patch prepares for chromium-mac to mean Skia instead of CoreGraphics.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 146.

Full output: http://queues.webkit.org/results/9399008

[Alexandru Chiculita]

Created attachment 104147 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 104147 [details]
Patch for landing

Rejecting attachment 104147 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Alexandru Chiculita]

Created attachment 104149 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 104149 [details]
Patch for landing

Rejecting attachment 104149 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Review Bot]

Comment on attachment 104149 [details]
Patch for landing

Rejecting attachment 104149 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-02', '--port..." exit_code: 2

Last 500 characters of output:
uto-merging Source/WebCore/platform/chromium/ScrollAnimatorChromiumMac.mm
Failed to merge in the changes.
Patch failed at 0001 2011-08-16  Andrey Kosyakov  <[e-mail]>

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 146.

Full output: http://queues.webkit.org/results/9401815

[Alexandru Chiculita]

*** Bug 66129 has been marked as a duplicate of this bug. ***

[WebKit Review Bot]

Comment on attachment 104149 [details]
Patch for landing

Clearing flags on attachment: 104149

Committed r93307: <http://trac.webkit.org/changeset/93307>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.