[Webkit-unassigned] [Bug 27160] Implement vw/vh/vm (viewport sizes) from CSS 3 Values and Units

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=27160





--- Comment #30 from Luke Macpherson <macpherson at chromium.org>  2012-02-09 00:48:20 PST ---
(In reply to comment #28)
> (In reply to comment #26)
> > (From update of attachment 126010 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=126010&action=review
> > 
> Thanks for the review.
> 
> > > Source/WebCore/css/CSSStyleSelector.cpp:2379
> > > +static Length convertToLength(CSSPrimitiveValue* primitiveValue, CSSStyleSelector* styleSelector, bool toFloat, double multiplier = 1, bool *ok = 0)
> > 
> > This code should no longer be static, and should instead be a member function of CSSStyleSelector.
> > 
> > > Source/WebCore/css/CSSStyleSelector.cpp:2411
> > > +static Length convertToIntLength(CSSPrimitiveValue* primitiveValue, CSSStyleSelector* styleSelector, double multiplier = 1, bool *ok = 0)
> > 
> > This code should no longer be static, and should instead be a member function of CSSStyleSelector.
> > 
> > > Source/WebCore/css/CSSStyleSelector.cpp:2416
> > > +static Length convertToFloatLength(CSSPrimitiveValue* primitiveValue, CSSStyleSelector* styleSelector, double multiplier = 1, bool *ok = 0)
> > 
> > This code should no longer be static, and should instead be a member function of CSSStyleSelector.
> > 
> I will make above 3 functions as member functions. I changed the function signature of createTransformOperations static member function to include CSSStyleSelector. I think it is a good idea to remove the static from here too. Please let me know your opinion on this.

Yes, that sounds good to me.

> > > Source/WebCore/css/CSSStyleSelector.cpp:5692
> > > +Length RelativeviewportLength(CSSPrimitiveValue* primitiveValue, Document* document)
> > 
> > This code almost certainly belongs in CSSPrimitiveValue::computeLength.
> 
> I have not done this for 2 reasons. (1) CSSPrimitiveValue::ComputeLength is used mainly for "Fixed" length types for computing the actual length in pixels. It is not used for other Length types like "Percentage" or "Number". I think we should handle RelativeViewport length similar to "Percentage" since we are not calculating the pixel value in CSSPrimitiveValue class.  (2) RelativeViewport Length type needs a way to fetch the viewport size (currently I introduced an overloaded constructor and passing Document as an argument). So if I modify the arguments of CSSPrimitiveValue::computeLength templatized function, then I need to make changes in lots of places wherever this function is called. 

That is a fair argument. I personally think that percentage and number-based lengths should be in computeLength too, but perhaps that is something to look at later on.

> So I think it is better to keep it separate.
> > 
> > > Source/WebCore/css/CSSStyleSelector.h:479
> > > +Length RelativeviewportLength(CSSPrimitiveValue*, Document*);
> > 
> > captialization
> 
> I will make this change.
> > 
> > > Source/WebCore/platform/Length.h:248
> > > +    RelativeViewportLengthType relativeViewportLengthType() const { return static_cast<RelativeViewportLengthType>(m_viewportLengthType); }
> > 
> > I would like to s/RelativeViewport/ViewportRelative throughout this patch.
> 
> OK. I will change RelativeViewport to ViewportRelative everywhere.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.