Bug 66921 - [CSSRegions] RenderFlowThread::renderRegionForLine should use a faster search method
Summary: [CSSRegions] RenderFlowThread::renderRegionForLine should use a faster search...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andrei Bucur
URL:
Keywords: AdobeTracked, InRadar
Depends on: 113303
Blocks: 57312
  Show dependency tree / graph
 
Reported: 2011-08-24 23:17 PDT by Alexandru Chiculita
Modified: 2013-04-04 04:08 PDT (History)
9 users (show)

See Also:

Attachments
A sample from a client, that might prove to be a good test-case for this. Addmitedly, the algorithm is (very) naive, but it shouldn't take 5 minutes to finish, nonetheless. (19.80 KB, application/x-zip-compressed)
2012-08-16 09:36 PDT, Mihai Balan 		no flags Details
Patch (9.54 KB, patch)
2013-04-03 08:38 PDT, Andrei Bucur 		no flags Details | Formatted Diff | Diff
Patch (9.56 KB, patch)
2013-04-03 09:33 PDT, Andrei Bucur 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandru Chiculita 2011-08-24 23:17:47 PDT 
The RenderRegion rects are always in order. There should be a faster way to search the region at a specified position.
Comment 1 Radar WebKit Bug Importer 2011-09-30 16:08:38 PDT 
<rdar://problem/10218165>
Comment 2 Mihai Balan 2012-08-16 09:36:17 PDT 
Created attachment 158840 [details]
A sample from a client, that might prove to be a good test-case for this. Addmitedly, the algorithm is (very) naive, but it shouldn't take 5 minutes to finish, nonetheless.
Comment 3 Andrei Bucur 2013-04-03 08:38:58 PDT 
Created attachment 196358 [details]
Patch
Comment 4 Andrei Bucur 2013-04-03 08:40:52 PDT 
Comment on attachment 196358 [details]
Patch

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

> Source/WebCore/rendering/RenderFlowThread.cpp:968
> +    if (interval.data()->isRenderRegionSet())

Is it possible we'd want to return a region set that's not the last region? The old code seemed to have allowed it.
Comment 5 Dave Hyatt 2013-04-03 08:49:13 PDT 
Comment on attachment 196358 [details]
Patch

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

Please handle

expandToEncompassFlowThreadContentsIfNeeded()

which hacks the flow thread portion rect. You'll want to make sure that interval gets correctly updated.

> Source/WebCore/rendering/RenderFlowThread.cpp:827
> +        m_regionIntervalTree.add(RegionIntervalTree::createInterval(regionRect.y(), regionRect.maxY(), region));

I think because of writing modes it would read better not to use y() and maxY() here. You could just use logicalHeight and logicalHeight + regionLogicalHeight, and I think that would read slightly better.

>> Source/WebCore/rendering/RenderFlowThread.cpp:968
>> +    if (interval.data()->isRenderRegionSet())
> 
> Is it possible we'd want to return a region set that's not the last region? The old code seemed to have allowed it.

Yes, it will be possible to have multiple region sets. Your code is going to be applicable to region sets.
Comment 6 Andrei Bucur 2013-04-03 09:33:25 PDT 
Created attachment 196363 [details]
Patch
Comment 7 Dave Hyatt 2013-04-03 12:38:52 PDT 
Comment on attachment 196363 [details]
Patch

r=me
Comment 8 WebKit Review Bot 2013-04-04 04:08:44 PDT 
Comment on attachment 196363 [details]
Patch

Clearing flags on attachment: 196363

Committed r147620: <http://trac.webkit.org/changeset/147620>
Comment 9 WebKit Review Bot 2013-04-04 04:08:49 PDT 
All reviewed patches have been landed.  Closing bug.