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

Hans Muller
Reported 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.
Attachments
Patch (13.04 KB, patch)
2013-08-19 15:57 PDT, Hans Muller 		no flags
DetailsFormatted DiffDiff
Patch (14.48 KB, patch)
2013-08-20 09:34 PDT, Hans Muller 		achicu: review+
DetailsFormatted DiffDiff
Patch (14.51 KB, patch)
2013-08-20 11:04 PDT, Hans Muller 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Hans Muller
Comment 1 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.
Hans Muller
Comment 2 2013-08-20 09:34:13 PDT
Created attachment 209202 [details] Patch Eliminated unnecessary Vector copies in RasterShape.cpp, where Region::rects() is called.
Alexandru Chiculita
Comment 3 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 { }.
Hans Muller
Comment 4 2013-08-20 11:04:58 PDT
Created attachment 209209 [details] Patch Incorporated review feedback.
WebKit Commit Bot
Comment 5 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>
WebKit Commit Bot
Comment 6 2013-08-20 12:20:33 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.