Created attachment 128948 [details] Patch

Created attachment 128952 [details] Patch

Comment on attachment 128952 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128952&action=review > Source/WebCore/css/CSSCalculationValue.cpp:136 > + break; What's the case where we hit this? > Source/WebCore/platform/CalculationValue.cpp:78 > + // Skip m_index == 0 as zero key hits the assertion of HashMap. Make this ASSERT(m_index) instead. Then you don't need to check m_index in the loop below. > Source/WebCore/platform/CalculationValue.cpp:82 > + m_index++; Why do you need to increment here? Won't the loop above already get you the right index value? > Source/WebCore/platform/Length.cpp:225 > + float result = calcLength->evaluate(maxValue); > + if (isnan(result)) > + return 0; > + return result; This is repeated three times. Maybe make this a static helper function? nonNanResult or something? > Source/WebCore/platform/Length.h:159 > + // Note: May only be called for Fixed, Percent, Auto and Calculated lengths. > // Other types will ASSERT in order to catch invalid length calculations. This comment isn't terribly useful given that the code makes this pretty clear. > Source/WebCore/platform/Length.h:245 > + bool isPercent() const { return type() == Percent || type() == Calculated; } Can't Calculated be a non-percent?

This patch looks good to me. It's a bit tricky though, so I'd prefer someone with more experience with CSS review this. Antti, would you mind taking a look at this?

Comment on attachment 128952 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128952&action=review >> Source/WebCore/css/CSSCalculationValue.cpp:136 >> + break; > > What's the case where we hit this? Good catch - we should never hit that. >> Source/WebCore/platform/CalculationValue.cpp:78 >> + // Skip m_index == 0 as zero key hits the assertion of HashMap. > > Make this ASSERT(m_index) instead. Then you don't need to check m_index in the loop below. Fair enough - this just catches the extremely unlikely case of wraparound of the index. >> Source/WebCore/platform/CalculationValue.cpp:82 >> + m_index++; > > Why do you need to increment here? Won't the loop above already get you the right index value? m_index was supposed to be the next available index. So we find the next available index, use it, and step m_index along. The loop is really there for a sanity check. But now that I think about it, it's probably better not to step m_index along - then we better deal with the case of insertion/removal of the same element (with nothing else happening inbetween) - m_index will not continually increment. Unit tests would be nice right about now :) >> Source/WebCore/platform/Length.cpp:225 >> + return result; > > This is repeated three times. Maybe make this a static helper function? nonNanResult or something? Done. >> Source/WebCore/platform/Length.h:159 >> // Other types will ASSERT in order to catch invalid length calculations. > > This comment isn't terribly useful given that the code makes this pretty clear. OK - removed. >> Source/WebCore/platform/Length.h:245 >> + bool isPercent() const { return type() == Percent || type() == Calculated; } > > Can't Calculated be a non-percent? If it gets this far, it has to be a mixed percent/absolute expression. If it's a simple length, percent or number, it gets evaluated inside CSSPrimitiveValue.

Created attachment 129174 [details] Patch

Comment on attachment 129174 [details] Patch Thanks for the review Ojan! I've addressed your comments in this new patch.

Created attachment 129175 [details] Patch

Comment on attachment 129175 [details] Patch The last patch didn't remove that comment. It's gone now.

+hyatt, since this touches platform/Length.[h|cpp].

Comment on attachment 129175 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129175&action=review A few suggestions, just trying to move the patch along. Can this patch be made a little less massive by perhaps: 1) removing calculatedIs* stubs? 2) adding CalcExpression* incrementally? I wonder if Hyatt and Antti will be more inclined to review in smaller bits? > Source/WebCore/css/CSSStyleSelector.h:352 > + static Length convertToIntLength(CSSPrimitiveValue*, RenderStyle*, RenderStyle* rootStyle, double multiplier = 1, bool *ok = 0); > + static Length convertToFloatLength(CSSPrimitiveValue*, RenderStyle*, RenderStyle* rootStyle, double multiplier = 1, bool *ok = 0); These conversion functions (including the convertToLength itself) all look like the could be factored to live on the CSSPrimitiveValue. > Source/WebCore/platform/CalculationValue.cpp:66 > + // FIXME: result is NaN when there is a division by zero which File a bug and reference it here? > Source/WebCore/platform/CalculationValue.h:133 > +class HandleMap { The name is way too generic to be in WebCore namespace. Can this just be an implementation detail in Length.cpp? > Source/WebCore/platform/Length.cpp:179 > +// FIXME: It's impossible to know the sign of calc values without File a bug and reference it here? > Source/WebCore/platform/Length.cpp:196 > +bool Length::calculatedIsZero() const > +{ > + return false; > +} > + > +bool Length::calculatedIsPositive() const > +{ > + return true; > +} > + > +bool Length::calculatedIsNegative() const > +{ > + return false; > +} Why are these here?

Thanks for the review Dimitri. > A few suggestions, just trying to move the patch along. > > Can this patch be made a little less massive by perhaps: > 1) removing calculatedIs* stubs? > 2) adding CalcExpression* incrementally? This patch is already pretty pared back - everything is kinda 'bare minimum to function'. - I've removed those stubs and replaced them directly with the current return values. - Adding CalcExpression* incrementally could only be done non-functionally - ie I could put those bits in in separate patches, but nothing would 'work'. - I've landed the rewritten margin.html test separately. > > Source/WebCore/css/CSSStyleSelector.h:352 > > + static Length convertToIntLength(CSSPrimitiveValue*, RenderStyle*, RenderStyle* rootStyle, double multiplier = 1, bool *ok = 0); > > + static Length convertToFloatLength(CSSPrimitiveValue*, RenderStyle*, RenderStyle* rootStyle, double multiplier = 1, bool *ok = 0); > > These conversion functions (including the convertToLength itself) all look like the could be factored to live on the CSSPrimitiveValue. Fair point. I'm inclined to do this as a followup though, since those functions handle NULL CSSPrimitiveValue pointers, meaning the pointer would potentially have to be tested before calling a function on it. Or the NULL check is unnecessary/obsolete. > > Source/WebCore/platform/CalculationValue.cpp:66 > > + // FIXME: result is NaN when there is a division by zero which > > File a bug and reference it here? Done. > > Source/WebCore/platform/CalculationValue.h:133 > > +class HandleMap { > > The name is way too generic to be in WebCore namespace. Can this just be an implementation detail in Length.cpp? Once upon a time it was in Length.cpp, but I pulled it out in response to some IRC comments from Hyatt. I've put it back - it does reduce the size of this patch marginally. > > Source/WebCore/platform/Length.cpp:179 > > +// FIXME: It's impossible to know the sign of calc values without > > File a bug and reference it here? Done. > > Source/WebCore/platform/Length.cpp:196 > > +bool Length::calculatedIsZero() const > > +{ > > + return false; > > +} > > + > > +bool Length::calculatedIsPositive() const > > +{ > > + return true; > > +} > > + > > +bool Length::calculatedIsNegative() const > > +{ > > + return false; > > +} > > Why are these here? Just as explicit functions saying what they do. I've removed these and folded their return values directly into the callsites instead.

Created attachment 130371 [details] Patch

Comment on attachment 130371 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130371&action=review > Source/WebCore/platform/Length.cpp:192 > + HashMap<int, RefPtr<CalculationValue> >* m_map; It doesn't look like you're ever destroying this map? I understand the class is only allocated as static, but still that seems like something we should avoid. Does it have to be a pointer? Can it just be a member?

Comment on attachment 130371 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130371&action=review Some drive-by comments: > Source/WebCore/css/CSSCalculationValue.cpp:135 > + case CalcPercentNumber: > + case CalcOther: > + ASSERT_NOT_REACHED(); This seems like a good place for a comment. > Source/WebCore/platform/Length.cpp:157 > +Length::~Length() > +{ > + if (isCalculated()) > + decrementCalculatedRef(); > +} This destructor should be inline, only decrementCalculatedRef() belongs in Length.cpp. > Source/WebCore/platform/Length.cpp:162 > + : m_index(1) Wrong indentation here. > Source/WebCore/platform/Length.cpp:199 > +static CalculationValueHandleMap* calcHandles() > +{ > + DEFINE_STATIC_LOCAL(CalculationValueHandleMap, handleMap, ()); > + return &handleMap; > +} Nit: static CalculationValueHandleMap& calcHandles() would be a slightly nicer signature.

Comment on attachment 130371 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130371&action=review Thanks for the reviews guys! Comments inline .... >> Source/WebCore/css/CSSCalculationValue.cpp:135 >> + ASSERT_NOT_REACHED(); > > This seems like a good place for a comment. Done. >> Source/WebCore/platform/Length.cpp:157 >> +} > > This destructor should be inline, only decrementCalculatedRef() belongs in Length.cpp. OK - done. >> Source/WebCore/platform/Length.cpp:162 >> + : m_index(1) > > Wrong indentation here. Fixed. >> Source/WebCore/platform/Length.cpp:192 >> + HashMap<int, RefPtr<CalculationValue> >* m_map; > > It doesn't look like you're ever destroying this map? I understand the class is only allocated as static, but still that seems like something we should avoid. Does it have to be a pointer? Can it just be a member? Good point. I've made it a member. >> Source/WebCore/platform/Length.cpp:199 >> +} > > Nit: static CalculationValueHandleMap& calcHandles() would be a slightly nicer signature. I'm oldskool when it comes to pointers. But changed to a reference.

Created attachment 130498 [details] Patch

Created attachment 130529 [details] Patch

Created attachment 130541 [details] Patch

Comment on attachment 130529 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130529&action=review > Source/WebCore/ChangeLog:13 > + Length (in platform) cannot refer to CSSCalculatedValue (in css) due to layering The terminology is a bit confusing, maybe I'm missing something. Along the patch you use CSSCalculationValue instead of of CalculatedValue, so which one will you use? > Source/WebCore/platform/Length.cpp:164 > + m_index++; I believe at some point we could overflow this. Can't you reuse ids instead to avoid this?

Comment on attachment 130529 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130529&action=review >> Source/WebCore/ChangeLog:13 >> + Length (in platform) cannot refer to CSSCalculatedValue (in css) due to layering > > The terminology is a bit confusing, maybe I'm missing something. Along the patch you use CSSCalculationValue instead of of CalculatedValue, so which one will you use? My mistake, I meant [CSS]CalculationValue, not [CSS]CalculatedValue. >> Source/WebCore/platform/Length.cpp:164 >> + m_index++; > > I believe at some point we could overflow this. Can't you reuse ids instead to avoid this? It will overflow, after 2 billion or so indices, hence the ASSERT. Reusing ids would be more complicated than what's here. I'd prefer to keep this patch as simple as possible until this is proven to be a problem. I've filed a bug to track this.

Created attachment 130543 [details] Patch

Comment on attachment 130543 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130543&action=review r=me based on prior partial approval from others, and the changes looking good to me. It's great that you've sprinkled FIXMEs. Makes it much easier to understand the patch. > Source/WebCore/css/CSSCalculationValue.cpp:129 > + { > + Length length = CSSStyleSelector::convertToFloatLength(m_value.get(), style, rootStyle, zoom); > + return adoptPtr(new CalcExpressionLength(length)); > + } I'd write this as a single line. > Source/WebCore/platform/Length.h:246 > bool isZero() const > { > ASSERT(!isUndefined()); > - return m_isFloat ? !m_floatValue : !m_intValue; > + return isCalculated() ? false : m_isFloat ? !m_floatValue : !m_intValue; > + } > + bool isPositive() const > + { > + return isUndefined() ? false : isCalculated() ? true : getFloatValue() > 0; > + } > + bool isNegative() const > + { > + return isUndefined() ? false : isCalculated() ? false : getFloatValue() < 0; > } I feel like we can do this without abusing ternary operators. > Source/WebCore/platform/Length.h:315 > + void incrementCalculatedRef() const; > + void decrementCalculatedRef() const; I don't understand why these are const.

Comment on attachment 130543 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130543&action=review Yay - thanks for the review! >> Source/WebCore/css/CSSCalculationValue.cpp:129 >> + } > > I'd write this as a single line. Done. >> Source/WebCore/platform/Length.h:246 >> } > > I feel like we can do this without abusing ternary operators. OK - ternarys involving isCalculated removed. >> Source/WebCore/platform/Length.h:315 >> + void decrementCalculatedRef() const; > > I don't understand why these are const. They will only affect the CalculationValueHandleMap, not the Length object itself. So Length can be const, as it is only holding a key into the map.

Created attachment 130712 [details] Patch for landing

Comment on attachment 130712 [details] Patch for landing Clearing flags on attachment: 130712 Committed r110126: <http://trac.webkit.org/changeset/110126>

All reviewed patches have been landed. Closing bug.

Reopening as the patch was rolled out due to causing a compile error due to exit time destructors: https://bugs.webkit.org/show_bug.cgi?id=80558. Compile error fix has landed in http://trac.webkit.org/changeset/110144, so I'll try landing this patch again.

Comment on attachment 130712 [details] Patch for landing Clearing flags on attachment: 130712 Committed r110148: <http://trac.webkit.org/changeset/110148>

All reviewed patches have been landed. Closing bug.