Bug 119849

Summary: [CSS Shapes] Complete RasterShape::firstIncludedIntervalLogicalTop()
Product: WebKit Reporter: Hans Muller <giles_joplin>
Component: CSSAssignee: Hans Muller <giles_joplin>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, esprehn+autocc, glenn, kondapallykalyan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 116348    
Attachments:
Description Flags
Patch
none
Patch
achicu: review+
Patch none

Description Hans Muller 2013-08-15 11:15:58 PDT
Complete the implementation of RasterShape::firstIncludedIntervalLogicalTop(). Currently the first layout position isn't computed, it's assumed that layout can begin at the top of the shape.

A detailed description of the algorithm to be used here can be found in http://hansmuller-webkit.blogspot.com/2013/08/first-fit-location-for-image-shapes.html.
Comment 1 Hans Muller 2013-08-19 15:57:41 PDT
Created attachment 209132 [details]
Patch

Completed the implementation of RasterShape::firstIncludedIntervalLogicalTop(). The method now computes first logical top location where a line segment can be laid out within a RasterShape, i.e. a shape derived from an image valued URL resource. A detailed description of the algorithm can be found in http://hansmuller-webkit.blogspot.com/2013/08/first-fit-location-for-image-shapes.html.

The new tests exposed a bug in the existing getIncludedIntervals() method. A shape with a vertical gap that spans the entire line now causes the method to short circuit and return an empty interval list.
Comment 2 Hans Muller 2013-08-20 09:34:13 PDT
Created attachment 209202 [details]
Patch

Eliminated unnecessary Vector copies in RasterShape.cpp, where Region::rects() is called.
Comment 3 Alexandru Chiculita 2013-08-20 10:50:26 PDT
Comment on attachment 209202 [details]
Patch

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

r=me

> Source/WebCore/rendering/shapes/RasterShape.cpp:53
> +    for (int y = minY; y <= bounds().height() - minSize.height(); y++) {

nit: might be useful to explain why we combine absolute position minY with relative position from bounds().height(). Or just use maxY.

> Source/WebCore/rendering/shapes/RasterShape.cpp:101
> +        if (lineY > currentLineMaxY) // We've encountered a vertical gap in lineRects, there are no included intervals.

nit: put the comment inside the if statement. Use { }.
Comment 4 Hans Muller 2013-08-20 11:04:58 PDT
Created attachment 209209 [details]
Patch

Incorporated review feedback.
Comment 5 WebKit Commit Bot 2013-08-20 12:20:31 PDT
Comment on attachment 209209 [details]
Patch

Clearing flags on attachment: 209209

Committed r154349: <http://trac.webkit.org/changeset/154349>
Comment 6 WebKit Commit Bot 2013-08-20 12:20:33 PDT
All reviewed patches have been landed.  Closing bug.