RESOLVED FIXED 64516
[CSSRegions] Collect flowed elements in different render element
https://bugs.webkit.org/show_bug.cgi?id=64516
Summary [CSSRegions] Collect flowed elements in different render element
Alexandru Chiculita
Reported 2011-07-14 00:34:33 PDT
Elements that specify a different flow name should be collected inside an anonymous render block. It will later be used to layout the content inside multiple regions.
Attachments
Patch (31.04 KB, patch)
2011-07-18 04:07 PDT, Alexandru Chiculita
no flags
Patch (30.55 KB, patch)
2011-07-18 04:57 PDT, Alexandru Chiculita
hyatt: review-
hyatt: commit-queue-
Patch (31.66 KB, patch)
2011-07-20 06:28 PDT, Alexandru Chiculita
no flags
Patch (31.66 KB, patch)
2011-07-20 06:43 PDT, Alexandru Chiculita
no flags
Patch with forced RenderLayer (33.79 KB, patch)
2011-07-20 07:27 PDT, Alexandru Chiculita
hyatt: review-
hyatt: commit-queue-
Patch (33.75 KB, patch)
2011-07-21 02:40 PDT, Alexandru Chiculita
no flags
Patch (37.50 KB, patch)
2011-07-21 04:05 PDT, Alexandru Chiculita
hyatt: review+
webkit.review.bot: commit-queue-
Rebased patch (37.51 KB, patch)
2011-07-25 23:32 PDT, Alexandru Chiculita
no flags
Alexandru Chiculita
Comment 1 2011-07-18 04:07:23 PDT
Alexandru Chiculita
Comment 2 2011-07-18 04:57:10 PDT
Created attachment 101146 [details] Patch Re-added fast/regions to the skipped list.
Dave Hyatt
Comment 3 2011-07-19 13:32:19 PDT
Comment on attachment 101146 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=101146&action=review So my big concern with this patch is with the nextRenderer() and previousRenderer() functions. These seem like they could be very slow. At a bare minimum I want to make sure that nextRenderer() is fast, since that function is called for every element that is put into the render tree. This problem of picking out the correct order from the overall DOM tree feels similar to the issue we encounter with <style> elements in the body. In that case we dirty the style selector and then figure things out later. Rather than moving through all the nodes of the document in previousRenderer and nextRenderer, I think you should walk the current RenderFlowThread children instead. That way you can find the correct node much more quickly using compareDocumentPosition. Check out addStyleSheetCandidateNode in Document.cpp to see what I mean. > Source/WebCore/dom/Document.cpp:5067 > +RenderFlowThread* Document::renderFlowThreadWithName(const AtomicString& flowThread) I don't think it's appropriate to have code in the document for this. Just implement this on RenderView instead. The code will look much cleaner too. > Source/WebCore/dom/Document.h:162 > +#if ENABLE(CSS_REGIONS) > +class RenderFlowThread; > +#endif Move to RenderView. > Source/WebCore/dom/Document.h:1104 > +#if ENABLE(CSS_REGIONS) > + RenderFlowThread* renderFlowThreadWithName(const AtomicString& flowThread); > +#endif Move to RenderView. > Source/WebCore/dom/NodeRenderingContext.cpp:166 > + // Search the whole document for elements that are attached to the same flow thread. > + for (Node* node = m_node->traverseNextNode(); node; node = node->traverseNextNode()) This seems super slow. It seems like we should be able to do better. It also seems like this code should go right before the normal traversal loop rather than above the special case code to avoid o(n^2) issues when the parent isn't attached yet. > Source/WebCore/dom/NodeRenderingContext.cpp:213 > + // Search the whole document for elements that are attached to the same flow thread. > + for (Node* node = m_node->traversePreviousNode(); node; node = node->traversePreviousNode()) This seems like it could be super slow also. > Source/WebCore/dom/NodeRenderingContext.cpp:306 > + if (!m_style || m_style->flowThread().isEmpty()) > + return; > + m_flowThread = m_style->flowThread(); This shouldn't run for text nodes. Is there something I'm missing that would keep it from running for text nodes? > Source/WebCore/rendering/RenderFlowThread.cpp:53 > + RefPtr<RenderStyle> newStyle = RenderStyle::create(); > + newStyle->setDisplay(BLOCK); > + newStyle->setPosition(AbsolutePosition); > + newStyle->setLeft(Length(0, Fixed)); > + newStyle->setTop(Length(0, Fixed)); > + newStyle->setWidth(Length(100, Percent)); > + newStyle->setHeight(Length(100, Percent)); > + newStyle->setOverflowX(OHIDDEN); > + newStyle->setOverflowY(OHIDDEN); > + newStyle->font().update(0); I'd pull this out into a static createFlowThreadStyle function. > Source/WebCore/rendering/RenderFlowThread.cpp:65 > +void RenderFlowThread::paint(PaintInfo&, const LayoutPoint&) > +{ > +} > + > +bool RenderFlowThread::nodeAtPoint(const HitTestRequest&, HitTestResult&, const LayoutPoint& /*pointInContainer*/, const LayoutPoint& /*accumulatedOffset*/, HitTestAction) > +{ > + return false; > +} Just inline these in the header right where they're declared. > Source/WebCore/rendering/RenderFlowThread.h:49 > + virtual void paint(PaintInfo&, const LayoutPoint&); > + virtual bool nodeAtPoint(const HitTestRequest&, HitTestResult&, const LayoutPoint& pointInContainer, const LayoutPoint& accumulatedOffset, HitTestAction); I think it would be good to comment why these methods do nothing. It's an opportunity to explain what's special about RenderFlowThread (that it's just collecting the objects). > Source/WebCore/rendering/RenderFlowThread.h:57 > + ASSERT(!object || !strcmp(object->renderName(), "RenderFlowThread")); Why not just assert that the object isRenderFlowThread instead?
Alexandru Chiculita
Comment 4 2011-07-20 06:28:55 PDT
Created attachment 101457 [details] Patch Thanks for review! I think I fixed them all.
WebKit Review Bot
Comment 5 2011-07-20 06:31:47 PDT
Attachment 101457 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/rendering/RenderFlowThread.cpp:34: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Source/WebCore/rendering/RenderFlowThread.cpp:35: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Source/WebCore/rendering/RenderFlowThread.cpp:49: Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html). [readability/pass_ptr] [5] Source/WebCore/rendering/RenderFlowThread.h:70: The parameter name "node" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/rendering/RenderFlowThread.h:71: The parameter name "node" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 5 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.
Alexandru Chiculita
Comment 6 2011-07-20 06:37:03 PDT
Some tasks related to this patch: 1. Overriding the RenderFlowThread::paint and RenderFlowThread::nodeAtPoint do not seem enough. Child elements might render inside different RenderLayers, so the right approach would have been to force the RenderLayer to avoid rendering the RenderFlowThread and any descendant layer. The same for nodeAtPoint. I will post a new patch with that. 2. Elements inside the RenderFlowThread that require a RenderLayerBacking should only be created if the RenderFlowThread is attached to RenderRegions. The layers need to be attached to the layer of the regions instead of attaching them to the layer of the 'virtual' RenderFlowThread.
Alexandru Chiculita
Comment 7 2011-07-20 06:43:45 PDT
Created attachment 101462 [details] Patch Fixed code style.
Alexandru Chiculita
Comment 8 2011-07-20 07:27:41 PDT
Created attachment 101467 [details] Patch with forced RenderLayer Using RenderLayer to avoid drawing the RenderFlowThread.
Dave Hyatt
Comment 9 2011-07-20 11:11:32 PDT
Comment on attachment 101467 [details] Patch with forced RenderLayer View in context: https://bugs.webkit.org/attachment.cgi?id=101467&action=review Few more comments. > Source/WebCore/rendering/RenderFlowThread.cpp:84 > +RenderObject* RenderFlowThread::nextRendererForNode(Node* node) const > +{ > + for (RenderObject* child = firstChild(); child; child = child->nextSibling()) { > + ASSERT(child->node()); > + unsigned short position = node->compareDocumentPosition(child->node()); > + if (position & Node::DOCUMENT_POSITION_FOLLOWING) > + return child; > + } > + return 0; > +} > + > +RenderObject* RenderFlowThread::previousRendererForNode(Node* node) const > +{ > + for (RenderObject* child = lastChild(); child; child = child->previousSibling()) { > + ASSERT(child->node()); > + unsigned short position = node->compareDocumentPosition(child->node()); > + if (position & Node::DOCUMENT_POSITION_PRECEDING) > + return child; > + } > + return 0; > +} The one concern I can think of with these functions is that the RenderFlowThread may have been forced to create anonymous blocks to wrap content. For example let's say you put both a <div> and a <span> into the same flow thread. When you put the <span> into the thread, an anonymous block will be created to wrap the <span>. People can also write weird code like div { display: table-cell } and throw that into a RenderFlowThread, and a bunch of anonymous table elements would be created to wrap that object as well. I think you will probably have to maintain an actual list of the nodes themselves in the RenderFlowThread and walk that instead. Something similar to the ListHashSet used for candidate stylesheet nodes that I mentioned in the previous review. > Source/WebCore/rendering/RenderLayer.cpp:2551 > +#if ENABLE(CSS_REGIONS) > + // Do not let the RenderFlowThread to render directly to screen. It will only render > + // inside the RenderRegions. > + if (renderer()->isRenderFlowThread()) > + return; > +#endif I think a better way to handle this would be to simply avoid collecting the child layers into the lists. That way you only have to patch one place. I think you could just patch updateZOrderLists to skip flow thread layers. That seems a bit simpler to me than patching both painting and hit testing functions. > Source/WebCore/rendering/RenderLayer.h:-327 > - bool isStackingContext() const { return !hasAutoZIndex() || renderer()->isRenderView(); } I don't think you'll have to patch this function if you just hack updateZOrderLists to avoid collecting the layers instead.
Alexandru Chiculita
Comment 10 2011-07-21 02:40:12 PDT
Created attachment 101566 [details] Patch Thanks for the tips! I've integrated the fixes. updateZOrderLists also fixes the RenderLayers that have accelerated rendering. When RenderRegion patch will be committed more logic can be added to distribute the layers directly to the region's layer based on their position in the flow thread.
Alexandru Chiculita
Comment 11 2011-07-21 04:05:15 PDT
Created attachment 101570 [details] Patch Forgot to add the ChangeLogs in the diff file.
Dave Hyatt
Comment 12 2011-07-25 13:33:45 PDT
Comment on attachment 101570 [details] Patch Looks good. r=me.
WebKit Review Bot
Comment 13 2011-07-25 14:07:24 PDT
Comment on attachment 101570 [details] Patch Rejecting attachment 101570 [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: file Source/WebCore/rendering/RenderLayer.cpp patching file Source/WebCore/rendering/RenderObject.h Hunk #1 succeeded at 316 (offset 1 line). patching file Source/WebCore/rendering/RenderTreeAsText.cpp patching file Source/WebCore/rendering/RenderView.cpp patching file Source/WebCore/rendering/RenderView.h patching file Source/WebCore/rendering/style/RenderStyle.cpp Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'David Hyatt', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/9249086
Alexandru Chiculita
Comment 14 2011-07-25 23:32:04 PDT
Created attachment 101974 [details] Rebased patch I'm not a committer, so I rebased the patch and I'm adding it for commit queue.
WebKit Review Bot
Comment 15 2011-07-26 10:11:21 PDT
Comment on attachment 101974 [details] Rebased patch Clearing flags on attachment: 101974 Committed r91760: <http://trac.webkit.org/changeset/91760>
WebKit Review Bot
Comment 16 2011-07-26 10:11:27 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.