Bug 66142

Summary: [CSSRegions] RenderRegion should not reference a parent RenderFlowThread
Product: WebKit Reporter: Alexandru Chiculita <achicu>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: hyatt, mihnea, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 57312, 66129    
Attachments:
Description Flags
Patch
hyatt: review-, hyatt: commit-queue-
Patch V2
hyatt: review-, hyatt: commit-queue-
Patch V3
hyatt: review-, hyatt: commit-queue-
Patch V4
none
Patch for landing
none
Patch for landing none

Description Alexandru Chiculita 2011-08-12 09:39:19 PDT
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>
Comment 1 Alexandru Chiculita 2011-08-16 08:03:23 PDT
Created attachment 104044 [details]
Patch
Comment 2 Alexandru Chiculita 2011-08-16 08:20:27 PDT
> 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.
Comment 3 Dave Hyatt 2011-08-16 09:47:07 PDT
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.
Comment 4 Alexandru Chiculita 2011-08-16 11:27:54 PDT
Created attachment 104070 [details]
Patch V2

Thanks for the tips. Updated the patch.
Comment 5 Dave Hyatt 2011-08-16 12:44:29 PDT
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.
Comment 6 Alexandru Chiculita 2011-08-16 13:35:46 PDT
Created attachment 104083 [details]
Patch V3

Rebased.
Comment 7 Dave Hyatt 2011-08-16 14:41:16 PDT
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
Comment 8 Alexandru Chiculita 2011-08-16 14:59:18 PDT
Created attachment 104096 [details]
Patch V4

Fixed it.
Comment 9 Dave Hyatt 2011-08-16 15:04:04 PDT
Comment on attachment 104096 [details]
Patch V4

r=me
Comment 10 WebKit Review Bot 2011-08-16 16:37:31 PDT
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
Comment 11 Alexandru Chiculita 2011-08-16 22:02:01 PDT
Created attachment 104147 [details]
Patch for landing
Comment 12 WebKit Review Bot 2011-08-16 22:03:16 PDT
Comment on attachment 104147 [details]
Patch for landing

Rejecting attachment 104147 [details] from commit-queue.

achicu@adobe.com 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.
Comment 13 Alexandru Chiculita 2011-08-16 22:07:24 PDT
Created attachment 104149 [details]
Patch for landing
Comment 14 WebKit Review Bot 2011-08-16 22:07:58 PDT
Comment on attachment 104149 [details]
Patch for landing

Rejecting attachment 104149 [details] from commit-queue.

achicu@adobe.com 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.
Comment 15 WebKit Review Bot 2011-08-17 01:31:32 PDT
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  <caseq@chromium.org>

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
Comment 16 Alexandru Chiculita 2011-08-18 07:11:01 PDT
*** Bug 66129 has been marked as a duplicate of this bug. ***
Comment 17 WebKit Review Bot 2011-08-18 07:36:59 PDT
Comment on attachment 104149 [details]
Patch for landing

Clearing flags on attachment: 104149

Committed r93307: <http://trac.webkit.org/changeset/93307>
Comment 18 WebKit Review Bot 2011-08-18 07:37:05 PDT
All reviewed patches have been landed.  Closing bug.