Summary: | [CSSRegions] Collect flowed elements in different render element | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexandru Chiculita <achicu> | ||||||||||||||||||
Component: | CSS | Assignee: | 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 | ||||||||||||||||||||
Attachments: |
|
Description
Alexandru Chiculita
2011-07-14 00:34:33 PDT
Created attachment 101144 [details]
Patch
Created attachment 101146 [details]
Patch
Re-added fast/regions to the skipped list.
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? Created attachment 101457 [details]
Patch
Thanks for review! I think I fixed them all.
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. 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. Created attachment 101462 [details]
Patch
Fixed code style.
Created attachment 101467 [details]
Patch with forced RenderLayer
Using RenderLayer to avoid drawing the RenderFlowThread.
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. 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.
Created attachment 101570 [details]
Patch
Forgot to add the ChangeLogs in the diff file.
Comment on attachment 101570 [details]
Patch
Looks good. r=me.
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 Created attachment 101974 [details]
Rebased patch
I'm not a committer, so I rebased the patch and I'm adding it for commit queue.
Comment on attachment 101974 [details] Rebased patch Clearing flags on attachment: 101974 Committed r91760: <http://trac.webkit.org/changeset/91760> All reviewed patches have been landed. Closing bug. |