Bug 117199 - [CSS Regions] Improve getRegionsByContent by testing in flow thread coordinates instead of view coordinates
Summary: [CSS Regions] Improve getRegionsByContent by testing in flow thread coordinat...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: AdobeTracked
Depends on:
Blocks: 128165
  Show dependency treegraph
 
Reported: 2013-06-04 08:14 PDT by Mihnea Ovidenie
Modified: 2014-02-03 23:48 PST (History)
4 users (show)

See Also:


Attachments
Patch (7.13 KB, patch)
2013-06-04 08:24 PDT, Mihnea Ovidenie
achicu: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mihnea Ovidenie 2013-06-04 08:14:23 PDT
If the object for which we want to check whether it is inside a region is not a box, we can do the testing in flow thread coordinates instead of converting everything to render view coordinates. We can thus avoid calls to absoluteBoundingBoxRect for both the object and the region.
Comment 1 Mihnea Ovidenie 2013-06-04 08:24:33 PDT
Created attachment 203702 [details]
Patch
Comment 2 Alexandru Chiculita 2013-06-06 09:51:12 PDT
Comment on attachment 203702 [details]
Patch

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

> Source/WebCore/rendering/RenderFlowThread.cpp:818
> +    LayoutRect boundingBox;

Why not just use localToContainerQuad instead (now that it can also return coordinates in the flowthread space)?

> Source/WebCore/rendering/RenderFlowThread.cpp:824
> +        ASSERT(false);

nit: this could be ASSERT_NOT_REACHED.

Why only renderInlines and text? Looks like the caller could send any node in here.

> Source/WebKit2/Shared/WebPreferencesStore.h:98
> +    macro(RegionBasedColumnsEnabled, regionBasedColumnsEnabled, Bool, bool, true) \

I don't see any comments in the changelog about this change.
Comment 3 Mihnea Ovidenie 2013-06-06 10:49:54 PDT
(In reply to comment #2)
> (From update of attachment 203702 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=203702&action=review
> 
> > Source/WebCore/rendering/RenderFlowThread.cpp:818
> > +    LayoutRect boundingBox;
> 
> Why not just use localToContainerQuad instead (now that it can also return coordinates in the flowthread space)?
> 

I can try that too.

> > Source/WebCore/rendering/RenderFlowThread.cpp:824
> > +        ASSERT(false);
> 
> nit: this could be ASSERT_NOT_REACHED.
> 
> Why only renderInlines and text? Looks like the caller could send any node in here.
> 

Because for boxes i have checked above and relied on getRegionRangeForBox.

> > Source/WebKit2/Shared/WebPreferencesStore.h:98
> > +    macro(RegionBasedColumnsEnabled, regionBasedColumnsEnabled, Bool, bool, true) \
> 
> I don't see any comments in the changelog about this change.

This slipped in from a research, will remove, thx for catching.