[webkit-dev] some feedback on the calcAbsoluteHorizontalValues patch

[Sam Weinig]

There is already a patch that has been reviewed for this issue
(although not committed yet) attached to bug
http://bugzilla.opendarwin.org/show_bug.cgi?id=9100.

On 5/31/06, Darin Adler <darin at apple.com> wrote:
>
>
> Begin forwarded message:
>
> From: Allan Sandfeld Jensen <kde at carewolf.com>
> Date: May 31, 2006 7:56:38 AM PDT
> To: ap at opensource.apple.com
> Cc: khtml-devel at kde.org
> Subject: Re: [webkit-changes] [14351] trunk/WebCore
> Reply-To: khtml-devel at kde.org
>
>
> On Saturday 13 May 2006 16:58, ap at opensource.apple.com wrote:
>
>
> 2006-05-13  Sam Weinig  <sam.weinig at gmail.com>
>
>         Reviewed by Hyatt, landed by ap.
>
> WebCore:
>         Patch for
> http://bugzilla.opendarwin.org/show_bug.cgi?id=7604
>         calcAbsoluteHorizontalValues() is being getting passed arguments
>         in the wrong order in calcAbsoluteHorizontal()
>
>         Cleans up the RenderBox code for absolutely positioned elements
>         and adds new functions for replaced absolutely positioned
>         elements. Now uses Length so that magic number -666666 for
>         auto lengths is no longer used.
>
>         * rendering/RenderBox.cpp:
>
> (WebCore::RenderBox::calcAbsoluteHorizontal):
>
> (WebCore::RenderBox::calcAbsoluteHorizontalValues):
>
> (WebCore::RenderBox::calcAbsoluteVertical):
>
> (WebCore::RenderBox::calcAbsoluteVerticalValues):
>
> (WebCore::RenderBox::calcAbsoluteHorizontalReplaced):
> Handle
> replaced case separately.
>
> (WebCore::RenderBox::calcAbsoluteVerticalReplaced):
> ditto.
>         * rendering/RenderBox.h:
>
> I just merged this patch to KHTML, and noticed a regression:
>
> In the overconstrained case in calcAbsoluteVerticalReplaced() (step 6) you
> discard information about top, margin-top and margin-bottom.
>
> It should look like this:
>
> /*-----------------------------------------------------------------------*\
>      * 6. If at this point the values are over-constrained, ignore the value
>      *    for 'bottom' and solve for that value.
>
> \*-----------------------------------------------------------------------*/
>     else {
>         m_marginTop = marginTop.width(containerHeight);
>         m_marginBottom = marginBottom.width(containerHeight);
>         topValue = top.width(containerHeight);
>
>         // Solve for 'bottom'
>         // NOTE: It is not necessary to solve for 'bottom' because we don't
> ever
>         // use the value.
>     }
>
> There is of course the same problem in calcAbsoluteVerticalReplaced() where
> it
> should look like:
>
> /*-----------------------------------------------------------------------*\
>      * 6. If at this point the values are over-constrained, ignore the value
>      *    for either 'left' (in case the 'direction' property of the
>      *    containing block is 'rtl') or 'right' (in case 'direction' is
>      *    'ltr') and solve for that value.
>
> \*-----------------------------------------------------------------------*/
>     else  {
>         m_marginLeft = marginLeft.width(containerWidth);
>         m_marginRight = marginRight.width(containerWidth);
>         if (containerDirection  == LTR) {
>             leftValue = left.width(containerWidth);
>             rightValue = availableSpace - (leftValue + m_marginLeft +
> m_marginRight);
>         }
>         else {
>             rightValue = right.width(containerWidth);
>             leftValue = availableSpace - (rightValue + m_marginLeft +
> m_marginRight);
>         }
>     }
>
> Regards
> `Allan
>
> _______________________________________________
> webkit-dev mailing list
> webkit-dev at opendarwin.org
> http://www.opendarwin.org/mailman/listinfo/webkit-dev
>
>
>