Bug 123898 – Refactor logical left/right offset for line methods

• Component: New Bugs
• Status: RESOLVED FIXED

[Bem Jones-Bey]

Refactor logical left/right offset for line methods

[Bem Jones-Bey]

Created attachment 216189 [details]
Patch

[Zoltan Horvath]

Comment on attachment 216189 [details]
Patch

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

> Source/WebCore/rendering/FloatingObjects.cpp:271
> +inline LayoutUnit ComputeFloatOffsetAdapter<FloatingObject::FloatLeft>::shapeOffset() const

For float left and float right the function is 90% same. I think it's worth an extra static function too remove duplication.

> Source/WebCore/rendering/FloatingObjects.cpp:300
> +LayoutUnit FloatingObjects::logicalLeftOffsetForPositioningFloat(LayoutUnit fixedOffset, LayoutUnit logicalTop, LayoutUnit *heightRemaining)

LayoutUnit* heightRemaining

Is it possible to make heightRemaining as a member of FloatingObject?

[Bear Travis]

Comment on attachment 216189 [details]
Patch

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

Refactoring looks good to me.

> Source/WebCore/rendering/FloatingObjects.h:36
> +// FIXME this should be removed once RenderBlockFlow::nextFloatLogicalBottomBelow doesn't need it anymore.

Is there a bug filed for this?

> Source/WebCore/rendering/RenderBlockFlow.cpp:2035
> +    return adjustLogicalRightOffsetForLine(offset, applyTextIndent);

Should this be adjustLogicalLeftOffsetForLine?

[Build Bot]

Comment on attachment 216189 [details]
Patch

Attachment 216189 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/22328025

New failing tests:
fast/line-grid/line-align-left-edges.html

[Build Bot]

Created attachment 216202 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Bem Jones-Bey]

(In reply to comment #2)
> (From update of attachment 216189 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=216189&action=review
> 
> > Source/WebCore/rendering/FloatingObjects.cpp:271
> > +inline LayoutUnit ComputeFloatOffsetAdapter<FloatingObject::FloatLeft>::shapeOffset() const
> 
> For float left and float right the function is 90% same. I think it's worth an extra static function too remove duplication.

Ok, I'll do that.

> 
> > Source/WebCore/rendering/FloatingObjects.cpp:300
> > +LayoutUnit FloatingObjects::logicalLeftOffsetForPositioningFloat(LayoutUnit fixedOffset, LayoutUnit logicalTop, LayoutUnit *heightRemaining)
> 
> LayoutUnit* heightRemaining
> 
> Is it possible to make heightRemaining as a member of FloatingObject?

I'm not sure. I'll look into that for a future patch. I do have plans to do something with heightRemaining, because it needs to stop being an out parameter.

[Bem Jones-Bey]

(In reply to comment #3)
> (From update of attachment 216189 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=216189&action=review
> 
> Refactoring looks good to me.
> 
> > Source/WebCore/rendering/FloatingObjects.h:36
> > +// FIXME this should be removed once RenderBlockFlow::nextFloatLogicalBottomBelow doesn't need it anymore.
> 
> Is there a bug filed for this?

No, I was planning to do this change in the near future, so if you'd prefer for me to file a bug now, I can, but I don't expect this to still be around a week from now. :-)

> 
> > Source/WebCore/rendering/RenderBlockFlow.cpp:2035
> > +    return adjustLogicalRightOffsetForLine(offset, applyTextIndent);
> 
> Should this be adjustLogicalLeftOffsetForLine?

Yes, thanks for catching that.

[Build Bot]

Comment on attachment 216189 [details]
Patch

Attachment 216189 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/22558008

New failing tests:
fast/line-grid/line-align-left-edges.html

[Build Bot]

Created attachment 216210 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 216189 [details]
Patch

Attachment 216189 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/22128027

New failing tests:
fast/line-grid/line-align-left-edges.html

[Build Bot]

Created attachment 216214 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Bem Jones-Bey]

Created attachment 216229 [details]
Updated Patch

[Build Bot]

Comment on attachment 216229 [details]
Updated Patch

Attachment 216229 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/22608023

[Dave Hyatt]

Comment on attachment 216229 [details]
Updated Patch

Drop the "OnLine" from the block methods, since that doesn't make sense (floats aren't on lines). Aside from that, a nice refactor!

r=me

[Bem Jones-Bey]

Created attachment 216310 [details]
Updated Patch

[WebKit Commit Bot]

Comment on attachment 216310 [details]
Updated Patch

Clearing flags on attachment: 216310

Committed r158855: <http://trac.webkit.org/changeset/158855>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jer Noble]

Committed a windows build fix in https://trac.webkit.org/r158856.