Bug 96129 - Make RenderBox::computeLogicalWidthInRegion const
Summary: Make RenderBox::computeLogicalWidthInRegion const
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: Tony Chang
URL:
Keywords:
Depends on:
Blocks: 94982
  Show dependency treegraph
 
Reported: 2012-09-07 10:13 PDT by Tony Chang
Modified: 2012-09-07 13:50 PDT (History)
4 users (show)

See Also:


Attachments
Patch (16.78 KB, patch)
2012-09-07 10:14 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch for landing (16.76 KB, patch)
2012-09-07 10:50 PDT, Tony Chang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Chang 2012-09-07 10:13:29 PDT
This allows us to get rid of the hack in RenderBox::renderBoxRegionInfo.
Comment 1 Tony Chang 2012-09-07 10:14:47 PDT
Created attachment 162799 [details]
Patch
Comment 2 Tony Chang 2012-09-07 10:18:47 PDT
Comment on attachment 162799 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:1706
> +        bool hasInvertedDirection = cb->style()->isLeftToRightDirection() != style()->isLeftToRightDirection();
> +        computeInlineDirectionMargins(cb, containerLogicalWidthForAutoMargins, computedValues.m_extent,
> +            hasInvertedDirection ? computedValues.m_margins.m_end : computedValues.m_margins.m_start,
> +            hasInvertedDirection ? computedValues.m_margins.m_start : computedValues.m_margins.m_end);

This variable was named incorrectly. I made the name correct by flipping the expression.

> Source/WebCore/rendering/RenderBox.cpp:1715
> +        bool hasInvertedDirection = cb->style()->isLeftToRightDirection() != style()->isLeftToRightDirection();
> +        if (hasInvertedDirection)
> +            computedValues.m_margins.m_start = containerLogicalWidth - computedValues.m_extent - cb->marginStartForChild(this);
> +        else
> +            computedValues.m_margins.m_end = containerLogicalWidth - computedValues.m_extent - cb->marginStartForChild(this);

I could make a helper function for this on MarginValues if you think it helps.
Comment 3 Ojan Vafai 2012-09-07 10:29:04 PDT
Comment on attachment 162799 [details]
Patch

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

>> Source/WebCore/rendering/RenderBox.cpp:1715
>> +            computedValues.m_margins.m_end = containerLogicalWidth - computedValues.m_extent - cb->marginStartForChild(this);
> 
> I could make a helper function for this on MarginValues if you think it helps.

Can you just create a local variable before the if/else?
LayoutUnit newMargin = containerLogicalWidth - computedValues.m_extent - cb->marginStartForChild(this);
Comment 4 Tony Chang 2012-09-07 10:50:51 PDT
Created attachment 162815 [details]
Patch for landing
Comment 5 Tony Chang 2012-09-07 10:51:13 PDT
Comment on attachment 162815 [details]
Patch for landing

Want to let the ews bots chew on this first.
Comment 6 WebKit Review Bot 2012-09-07 13:50:18 PDT
Comment on attachment 162815 [details]
Patch for landing

Clearing flags on attachment: 162815

Committed r127914: <http://trac.webkit.org/changeset/127914>
Comment 7 WebKit Review Bot 2012-09-07 13:50:21 PDT
All reviewed patches have been landed.  Closing bug.