Summary: | Implement vw/vh/vm (viewport sizes) from CSS 3 Values and Units | ||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Aryeh Gregor <ayg> | ||||||||||||||||||||||||||||||||||||||
Component: | CSS | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||
Severity: | Enhancement | CC: | 7raivis, abarth, arv, bugmail, bugs.webkit.org, bwebb, chrisjshull, commit-queue, dglazkov, eoconnor, eric, florens, hyatt, jackalmage, jdv, joethomas, jwalden+bwo, kling, koivisto, macpherson, menard, mrmazda, Ms2ger, ojan, paulirish, peter, phiw2, rniwa, simon.fraser, tabatkins, tkent, webkit.bugzilla, webkit.org, webkit.review.bot | ||||||||||||||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||||||||||||||||
URL: | http://www.w3.org/TR/css3-values/#vw | ||||||||||||||||||||||||||||||||||||||||
Bug Depends on: | 80316, 80897 | ||||||||||||||||||||||||||||||||||||||||
Bug Blocks: | 82773, 82811, 83425 | ||||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Aryeh Gregor
2009-07-10 14:47:24 PDT
The Mozilla bug is: https://bugzilla.mozilla.org/show_bug.cgi?id=503720 I've been working with the win8 dev preview and I'm waiting to see support for the new units in a real (non-ie) browser. If I had the foggiest idea how I would contribute support myself, but sadly I don't. Is anyone working on this problem? I am working on this. Joe you're my hero. Created attachment 121518 [details]
Initial Version
Created attachment 121519 [details] testCase Test Case for vw and vh unit which I found in https://bugzilla.mozilla.org/show_bug.cgi?id=503720 The editor's draft talks about "vmin" rather than "vm", which you omitted from the patch. Recalling previous discussion on the topic, at first an alternative using calc() and min() was being considered, but ISSUE-190 (http://www.w3.org/Style/CSS/Tracker/issues/190) has been resolved by renaming the unit to "vmin". Is the omission deliberate? http://dev.w3.org/csswg/css3-values/#viewport-relative-lengths (In reply to comment #8) > The editor's draft talks about "vmin" rather than "vm", which you omitted from the patch. Recalling previous discussion on the topic, at first an alternative using calc() and min() was being considered, but ISSUE-190 (http://www.w3.org/Style/CSS/Tracker/issues/190) has been resolved by renaming the unit to "vmin". Is the omission deliberate? > > http://dev.w3.org/csswg/css3-values/#viewport-relative-lengths I omitted vm deliberately based on http://www.w3.org/TR/css3-values/#viewport-relative-lengths. I can add the support during the next iteration after the patch review. Can someone please review the proposed patch for vw/vh implementation? Comment on attachment 121518 [details] Initial Version View in context: https://bugs.webkit.org/attachment.cgi?id=121518&action=review > Source/WebCore/ChangeLog:8 > + No new tests. (OOPS!) Need tests :( Also, your ChangeLog shoudl explain your change in greater detail. Created attachment 122128 [details]
Patch2
Added new test case. Also more details to ChangeLog.
Can someone please review this? Thanks. What happens when the window is resized? It seems like we need to re-calculate all lengths that have already been computed, which in effect means re-applying all CSS properties continuously during a window resize (ie. for every new width which the window size is changing). That seems like it would be really computationally expensive. Code style wise, it would be nice to not have to add another parameter to computeLengthDouble and friends. Perhaps this is an opportunity to clean up that code by providing some more useful parameter that can provide the values necessary for the computation. (In reply to comment #14) > What happens when the window is resized? It seems like we need to re-calculate all lengths that have already been computed, which in effect means re-applying all CSS properties continuously during a window resize (ie. for every new width which the window size is changing). That seems like it would be really computationally expensive. It's basically identical to using a lot of percentages, except it can potentially apply more widely. (In reply to comment #15) > (In reply to comment #14) > > What happens when the window is resized? It seems like we need to re-calculate all lengths that have already been computed, which in effect means re-applying all CSS properties continuously during a window resize (ie. for every new width which the window size is changing). That seems like it would be really computationally expensive. > > It's basically identical to using a lot of percentages, except it can potentially apply more widely. This is how it works in the current IE10 implementation. (In reply to comment #16) > (In reply to comment #15) > > (In reply to comment #14) > > > What happens when the window is resized? It seems like we need to re-calculate all lengths that have already been computed, which in effect means re-applying all CSS properties continuously during a window resize (ie. for every new width which the window size is changing). That seems like it would be really computationally expensive. > > > > It's basically identical to using a lot of percentages, except it can potentially apply more widely. > > This is how it works in the current IE10 implementation. Thanks for the inputs. I will make a new patch which works with window resize. Comment on attachment 122128 [details]
Patch2
vw vh are relative units and it needs to be updated with window resize which is missing in the current patch.
(In reply to comment #15) > (In reply to comment #14) > > What happens when the window is resized? It seems like we need to re-calculate all lengths that have already been computed, which in effect means re-applying all CSS properties continuously during a window resize (ie. for every new width which the window size is changing). That seems like it would be really computationally expensive. > > It's basically identical to using a lot of percentages, except it can potentially apply more widely. That isn't true if you look at the implementation in the patch. Normally percentage lengths are not converted to pixel lengths while applying the CSS values to RenderStyle as they are here. Instead there is a special kind of LengthType for percentages. I think you'd need to add another length type similar to percent for viewport-relative lengths to make this work efficiently.
> That isn't true if you look at the implementation in the patch. Normally percentage lengths are not converted to pixel lengths while applying the CSS values to RenderStyle as they are here. Instead there is a special kind of LengthType for percentages. I think you'd need to add another length type similar to percent for viewport-relative lengths to make this work efficiently.
Yes, current patch does not do that. In order to distinguish between relative viewport-width and relative viewport-height, I guess we need to add two different LengthTypes(since length does not have any info about CSSPrimitive type). Can we do it with one?
Also, what would be the best way to get viewport size in Length Structure to convert the relative length to pixel length ?
(In reply to comment #20) > Yes, current patch does not do that. In order to distinguish between relative viewport-width and relative viewport-height, I guess we need to add two different LengthTypes(since length does not have any info about CSSPrimitive type). Can we do it with one? One new LengthType should be sufficient. And a new sub-category of relative viewport Lengths can be added. > > Also, what would be the best way to get viewport size in Length Structure to convert the relative length to pixel length? A new function argument can be added to the getter functions in Length. But that does not look nice. Any other better approach? Created attachment 125981 [details]
Patch-with-windowResize-Support
This patch includes support for window-resizing(the unit value changes if the window gets resized.)
Comment on attachment 125981 [details] Patch-with-windowResize-Support Attachment 125981 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11459279 New failing tests: transforms/cssmatrix-3d-interface.xhtml svg/dynamic-updates/SVG-dynamic-css-transform.html transforms/svg-vs-css.xhtml svg/dom/css-transforms.xhtml transforms/cssmatrix-2d-interface.xhtml Comment on attachment 125981 [details] Patch-with-windowResize-Support Attachment 125981 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11460250 New failing tests: transforms/cssmatrix-3d-interface.xhtml svg/dynamic-updates/SVG-dynamic-css-transform.html transforms/svg-vs-css.xhtml svg/dom/css-transforms.xhtml transforms/cssmatrix-2d-interface.xhtml Created attachment 126010 [details]
Patch-updated
Fixed the failing tests
Comment on attachment 126010 [details] Patch-updated View in context: https://bugs.webkit.org/attachment.cgi?id=126010&action=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. > Source/WebCore/css/CSSStyleSelector.cpp:5692 > +Length RelativeviewportLength(CSSPrimitiveValue* primitiveValue, Document* document) This code almost certainly belongs in CSSPrimitiveValue::computeLength. > Source/WebCore/css/CSSStyleSelector.h:479 > +Length RelativeviewportLength(CSSPrimitiveValue*, Document*); captialization > 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. Comment on attachment 126010 [details] Patch-updated View in context: https://bugs.webkit.org/attachment.cgi?id=126010&action=review > Source/WebCore/css/CSSStyleSelector.cpp:3606 > + createTransformOperations(value, this, operations); I think after you patch "this" is the only value passed in. In that case this parameter should be removed. (In reply to comment #26) > (From update of attachment 126010 [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. > > 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. 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. (In reply to comment #27) > (From update of attachment 126010 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=126010&action=review > > > Source/WebCore/css/CSSStyleSelector.cpp:3606 > > + createTransformOperations(value, this, operations); > > I think after you patch "this" is the only value passed in. In that case this parameter should be removed. I didn't understand this. Could you please elaborate? (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. (In reply to comment #30) > (In reply to comment #28) > > (In reply to comment #26) > > > (From update of attachment 126010 [details] [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. > It's slightly difficult to make these functions as non-static member function of CSSStyleSelector. CSSStyleSelector::createTransformOperations is called from WebKitCSSMatrix::setMatrixValue and this class does not have access to CSSStyleSelector. It will be difficult to access this function if we make it non-static. It just passes null for style and rootElementStyle while calling CSSStyleSelector::createTransformOperations. convertToFloatLength & convertToIntLength are called from CSSStyleSelector::createTransformOperations. Since CSSStyleSelector argument is passed as NULL to createTransformOperations when called from WebKitCSSMatrix, it is not possible to call convertToFloatLength & convertToIntLength if they are member functions. Created attachment 126385 [details]
Patch-updated-I
Review comments incorporated
Comment on attachment 126385 [details] Patch-updated-I View in context: https://bugs.webkit.org/attachment.cgi?id=126385&action=review Someone more familiar with these standard should review (smfr, hyatt?). > Source/WebCore/css/CSSStyleSelector.cpp:2433 > -static Length convertToLength(CSSPrimitiveValue* primitiveValue, RenderStyle* style, RenderStyle* rootStyle, bool toFloat, double multiplier = 1, bool *ok = 0) > +static Length convertToLength(CSSPrimitiveValue* primitiveValue, CSSStyleSelector* styleSelector, bool toFloat, double multiplier = 1, bool *ok = 0) I don't like passing CSSStyleSelector*. Just try passing the minimal information you actually need for this (viewport?) Comment on attachment 126385 [details] Patch-updated-I View in context: https://bugs.webkit.org/attachment.cgi?id=126385&action=review > Source/WebCore/platform/Length.h:308 > + unsigned char m_viewportLengthType; > + Document *m_document; You can't add this stuff to Length. It is a memory-critical basic type. Not to mention Lengths may get shared between documents for various reasons and putting Document pointer there is nonsensical. Created attachment 127238 [details]
Patch-BasedOn-Antti's Review Comments
Patch based on Antti Koivisto's review comments
Included viewportSize as one of the parameter in the getter functions of Length structure (calcValue, calcMinValue, calcFloatValue) with a default value IntSize().
Currently, the viewportSize parameter is used only when the width and height is calculated in RenderBox. Length getters are used extensively in webCore. I think it is better to include the viewport parameter in other places on a need basis. Any thoughts on this?
Comment on attachment 127238 [details] Patch-BasedOn-Antti's Review Comments View in context: https://bugs.webkit.org/attachment.cgi?id=127238&action=review This patch is looking a lot better to me. My only concern right now is that there appear to be a lot of places where calcValue/calcMinValue should be able to handle viewport units, but the viewport size is not being passed in. You would probably pick this up if you were using more interesting test cases. I suggest running through "grep -r calcValue\( Source/WebCore/" and seeing all the other ways that Lengths are used. There are also probably existing test cases that could be modified simply by adding the appropriate units, and that could save you some work. > Source/WebCore/css/CSSStyleSelector.cpp:5768 > +Length viewportRelativeLength(CSSPrimitiveValue* primitiveValue) Now that this has been so effectively simplified, could this be made a method on CSSPrimitiveValue? I'm trying to kill the CSSPrimitiveValue::primitiveType() method at the moment (it should currently only be used in SVG code), and would also like to merge all CSSPrimitiveValue to Length conversions into a single point soon. > Source/WebCore/platform/Length.h:248 > + return type() == ViewportRelativeWidth || type() == ViewportRelativeHeight || type() == ViewportRelativeMin; This can simply be return type() >= ViewportRelativeWidth; > Source/WebCore/rendering/RenderBox.cpp:1753 > + logicalWidthResult = computeBorderBoxLogicalWidth(logicalWidth.calcValue(availableLogicalWidth, document()->viewportSize())); Is these the only place that viewport based units are used? I see 125 references to calcValue() in the codebase - why can't all of them potentially be viewport based? > Source/WebCore/rendering/RenderBoxModelObject.cpp:1078 > + LayoutUnit xPosition = fillLayer->xPosition().calcMinValue(positioningAreaSize.width() - geometry.tileSize().width(), IntSize(), true); Can fillLayer->xPosition() return a viewport relative size? If you need to assume this I think you should add an assertion. > Source/WebCore/rendering/RenderBoxModelObject.cpp:1084 > + LayoutUnit yPosition = fillLayer->yPosition().calcMinValue(positioningAreaSize.height() - geometry.tileSize().height(), IntSize(), true); Can fillLayer->yPosition() return a viewport relative size? If you need to assume this I think you should add an assertion. Looks good to me, please just address Luke's comments. (In reply to comment #37) > (From update of attachment 127238 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=127238&action=review > > This patch is looking a lot better to me. My only concern right now is that there appear to be a lot of places where calcValue/calcMinValue should be able to handle viewport units, but the viewport size is not being passed in. You would probably pick this up if you were using more interesting test cases. I suggest running through "grep -r calcValue\( Source/WebCore/" and seeing all the other ways that Lengths are used. There are also probably existing test cases that could be modified simply by adding the appropriate units, and that could save you some work. > Yes, viewport relative units can be used in most of the places where we call calcValue/CalcMinValue. I will make that change, but I am afraid that the patch is going to be big. > > Source/WebCore/css/CSSStyleSelector.cpp:5768 > > +Length viewportRelativeLength(CSSPrimitiveValue* primitiveValue) > > Now that this has been so effectively simplified, could this be made a method on CSSPrimitiveValue? I'm trying to kill the CSSPrimitiveValue::primitiveType() method at the moment (it should currently only be used in SVG code), and would also like to merge all CSSPrimitiveValue to Length conversions into a single point soon. > I will move this function to CSSPrimitiveValue > > Source/WebCore/platform/Length.h:248 > > + return type() == ViewportRelativeWidth || type() == ViewportRelativeHeight || type() == ViewportRelativeMin; > > This can simply be return type() >= ViewportRelativeWidth; > OK. > > Source/WebCore/rendering/RenderBox.cpp:1753 > > + logicalWidthResult = computeBorderBoxLogicalWidth(logicalWidth.calcValue(availableLogicalWidth, document()->viewportSize())); > > Is these the only place that viewport based units are used? I see 125 references to calcValue() in the codebase - why can't all of them potentially be viewport based? > I will make the change. > > Source/WebCore/rendering/RenderBoxModelObject.cpp:1078 > > + LayoutUnit xPosition = fillLayer->xPosition().calcMinValue(positioningAreaSize.width() - geometry.tileSize().width(), IntSize(), true); > > Can fillLayer->xPosition() return a viewport relative size? If you need to assume this I think you should add an assertion. > > > Source/WebCore/rendering/RenderBoxModelObject.cpp:1084 > > + LayoutUnit yPosition = fillLayer->yPosition().calcMinValue(positioningAreaSize.height() - geometry.tileSize().height(), IntSize(), true); > > Can fillLayer->yPosition() return a viewport relative size? If you need to assume this I think you should add an assertion. Attachment 127238 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource First, rewinding head to replay your work on top of it... Applying: dataTransfer.types (HTML5 drag & drop) should return DOMStringList Using index info to reconstruct a base tree... <stdin>:84: trailing whitespace. <stdin>:333: trailing whitespace. <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" <stdin>:334: trailing whitespace. width="300" height="300" onload="runRepaintTest()"> <stdin>:335: trailing whitespace. <script xlink:href="../../fast/repaint/resources/repaint.js" /> <stdin>:336: trailing whitespace. <script><![CDATA[ warning: squelched 16 whitespace errors warning: 21 lines add whitespace errors. Falling back to patching base and 3-way merge... Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging LayoutTests/platform/chromium/test_expectations.txt Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Failed to merge in the changes. Patch failed at 0001 dataTransfer.types (HTML5 drag & drop) should return DOMStringList When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. If any of these errors are false positives, please file a bug against check-webkit-style. Created attachment 127439 [details]
Patch-02-16
Patch which uses viewport relative units more widely
The test coverage seems weak. Code LGTM. I hope Document::viewportSize() is fast (it probably is), because it's going to be hotter than necessary - could avoid that by passing document and only calling it when evaluating a viewport based unit. Agree that more test coverage is needed. Sorry, I know that's the least fun part of adding a feature like this. Yes, ideal test coverage would be at least one usage of all three units in every property that can take lengths. Easy to auto-generate. OK. I will add more test cases. Tab, Could you please give me more details on how to auto-generate the test case? While adding more test cases, I found out that some of the CSS properties does not work well with newly introduced units. I will investigate more on it and update the patch soon. Created attachment 128091 [details]
Patch-with-More-testCases
Patch with more test cases added.
Created attachment 128343 [details]
patch-Updated-TestCases
Added more test cases.
Antti, Could you please review the patch? I added more test cases as per your review comments. (In reply to comment #46) > While adding more test cases, I found out that some of the CSS properties does not work well with newly introduced units. I will investigate more on it and update the patch soon. What was the problem? How did you address it? (In reply to comment #50) > (In reply to comment #46) > > While adding more test cases, I found out that some of the CSS properties does not work well with newly introduced units. I will investigate more on it and update the patch soon. > > What was the problem? How did you address it? Viewport Size was not passed to calcValue function in some of the cases (line-height, margin, etc). I added support for those missing cases. Hyatt, Antti Koivisto wanted you also to take a look at the rendering changes as per the IRC conversation. Patch looks good to him. Could you please review the changes? Comment on attachment 128343 [details]
patch-Updated-TestCases
I think you should add a viewportSize() member to RenderObject that calls document()->viewportSize(). The sheer # of times I see document()->viewportSize() makes me think it's worth it.
Created attachment 129988 [details]
Patch-incorporated-Hyatt's-Review-Comments
Incorporated Hyatt's Review comments
(In reply to comment #53) > (From update of attachment 128343 [details]) > I think you should add a viewportSize() member to RenderObject that calls document()->viewportSize(). The sheer # of times I see document()->viewportSize() makes me think it's worth it. Done. Uploaded new patch. r=me Comment on attachment 129988 [details] Patch-incorporated-Hyatt's-Review-Comments Clearing flags on attachment: 129988 Committed r109656: <http://trac.webkit.org/changeset/109656> All reviewed patches have been landed. Closing bug. (In reply to comment #43) > I hope Document::viewportSize() is fast (it probably is), because it's going to be hotter than necessary - could avoid that by passing document and only calling it when evaluating a viewport based unit. Was this comment ever addressed? I have to agree with Luke here, it looks like we're adding a pointless function call to the common case where passing a Document* would suffice. Passing Document as-is to Length::calcValue is not an option as that would be a layering violation. Maybe an abstract interface? Or just take care that getting viewport is fast on all platforms. This seems to have regressed Parser/html5-full-render by about 10% http://webkit-perf.appspot.com/graph.html#tests=[[3068,2001,32196]]&sel=1330759211690,1330784299690&displayrange=7&datatype=running This was rolled out in http://trac.webkit.org/changeset/109785 Created attachment 131175 [details] Patch-With-Abstract-Interface Added Abstract interface class for fetching the Viewport size. I ran Parser/html5-full-render on Mac Snow-Leopard with webkit revision r110376 with and without the new patch and below are the values Webkit rev 110376 - {16540.5ms, 16448ms, 16431ms, 16506.5ms, 16580ms} Webkit rev 110376 with Patch - {16463ms, 16517.5ms, 16593ms, 16543ms, 16506ms} The values looks more or less same in Mac SL but there is a variation of around 150-200 ms. Is it possible test the patch on performance bots for any possible regressions? Comment on attachment 131175 [details] Patch-With-Abstract-Interface Attachment 131175 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11935467 New failing tests: dom/svg/level3/xpath/Comment_Nodes.svg http/tests/misc/object-embedding-svg-delayed-size-negotiation.xhtml css2.1/20110323/background-intrinsic-004.htm css2.1/20110323/background-intrinsic-006.htm css2.1/20110323/background-intrinsic-005.htm compositing/images/direct-svg-image.html css3/images/cross-fade-overflow-position.html css2.1/20110323/replaced-intrinsic-002.htm dom/svg/level3/xpath/Attribute_Nodes_xmlns.svg http/tests/inspector/inspect-element.html http/tests/misc/SVGFont-delayed-load.html accessibility/aria-disabled.html css2.1/20110323/replaced-intrinsic-005.htm css2.1/20110323/replaced-intrinsic-004.htm css2.1/20110323/replaced-intrinsic-003.htm css2.1/20110323/background-intrinsic-002.htm dom/svg/level3/xpath/Conformance_ID.svg css2.1/20110323/background-intrinsic-007.htm dom/svg/level3/xpath/Conformance_hasFeature_3.svg dom/svg/level3/xpath/Conformance_hasFeature_empty.svg css2.1/20110323/background-intrinsic-009.htm compositing/masks/multiple-masks.html css2.1/20110323/background-intrinsic-001.htm css3/zoom-coords.xhtml css2.1/20110323/replaced-intrinsic-001.htm dom/svg/level3/xpath/Attribute_Nodes.svg css2.1/20110323/background-intrinsic-008.htm css3/images/cross-fade-invalidation.html dom/svg/level3/xpath/Conformance_Expressions.svg css2.1/20110323/background-intrinsic-003.htm Created attachment 131221 [details]
Archive of layout-test-results from ec2-cr-linux-03
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 131229 [details] Patch-With-Abstract-Interface-Updated Fixed the chromium test failures. Parser/html5-full-render gives almost the same values with and without the patch on Mac Snow-Leopard as mentioned in comment #63. Is there a way to test the patch on perf bots on other platforms? (In reply to comment #66) > Parser/html5-full-render gives almost the same values with and without the patch on Mac Snow-Leopard as mentioned in comment #63. Is there a way to test the patch on perf bots on other platforms? There's no EWS bot for performance tests. You need to land the patch and see what happens. Looking at this patch, I think what we really want to do is to move calc*Value functions out from Length (and platform). That will remove the need to have clumsy virtual interface just to avoid the layering violation. So instead of something like return offset + style()->top().calcValue(containingBlock->availableHeight(), view()); you would have return offset + calculateValue(style()->top(), containingBlock->availableHeight(), view()); where calculateValue() is defined in WebCore/css/LengthFunctions.h/.cpp (or similar). It would be good to do this refactoring first, separate from the viewport size feature. How does this sound? (In reply to comment #68) > Looking at this patch, I think what we really want to do is to move calc*Value functions out from Length (and platform). That will remove the need to have clumsy virtual interface just to avoid the layering violation. > > So instead of something like > > return offset + style()->top().calcValue(containingBlock->availableHeight(), view()); > > you would have > > return offset + calculateValue(style()->top(), containingBlock->availableHeight(), view()); > > where calculateValue() is defined in WebCore/css/LengthFunctions.h/.cpp (or similar). > > It would be good to do this refactoring first, separate from the viewport size feature. How does this sound? I think it is a good idea. Bug 80897 is created for this. Created attachment 133734 [details]
Patch-Updated
Ran PerformanceTests/Parser/html5-full-render.html(which got regressed previously) with and without the patch and there is no performance difference on MAC SL.
Comment on attachment 133734 [details] Patch-Updated Attachment 133734 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12130744 New failing tests: css3/viewport-relative-lengths/css3-viewport-relative-lengths-vw.html Created attachment 133743 [details]
Archive of layout-test-results from ec2-cr-linux-03
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 133734 [details] Patch-Updated Attachment 133734 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12131718 New failing tests: css3/viewport-relative-lengths/css3-viewport-relative-lengths-vw.html Created attachment 133750 [details]
Archive of layout-test-results from ec2-cr-linux-01
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
(In reply to comment #73) > (From update of attachment 133734 [details]) > Attachment 133734 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/12131718 > > New failing tests: > css3/viewport-relative-lengths/css3-viewport-relative-lengths-vw.html Looks like there is a 1 or 2 pixel offset in displaying the text "TEST PASSED" in chromium bot which fails the ref test. This test passes on Mac. (In reply to comment #75) > > > > New failing tests: > > css3/viewport-relative-lengths/css3-viewport-relative-lengths-vw.html > > Looks like there is a 1 or 2 pixel offset in displaying the text "TEST PASSED" in chromium bot which fails the ref test. This test passes on Mac. Scrollbar width was not considered while getting the viewport size during length calculation which caused the mismatch. I will update the patch. Created attachment 134068 [details]
Patch
Previously failed test case should fix by this patch.
Comment on attachment 134068 [details]
Patch
r=me
r=me Comment on attachment 134068 [details] Patch Clearing flags on attachment: 134068 Committed r112301: <http://trac.webkit.org/changeset/112301> All reviewed patches have been landed. Closing bug. font-size specified with vw, vh, or vmin not adjusting on browser resize -- see https://bugs.webkit.org/show_bug.cgi?id=106035 |