Bug 87846 – vw/vh units used as font/line-height values don't scale with the viewport

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Sam]

While experimenting with vh/vw units in Chrome/Canary, I tried assigning line-height a vw unit side, which worked successfully, but the size didn't increase/decrease as the viewport did, unlike padding which increased/decreased as expected. Font size doesn't scale either. Padding and widths do. I believe the expected behavior is for anything sized using vh/vw units to scale with viewport, not just width/margin/padding.

[Sam]

(In reply to comment #0)
> While experimenting with vh/vw units in Chrome/Canary, I tried assigning line-height a vw unit side, which worked successfully, but the size didn't increase/decrease as the viewport did, unlike padding which increased/decreased as expected. Font size doesn't scale either. Padding and widths do. I believe the expected behavior is for anything sized using vh/vw units to scale with viewport, not just width/margin/padding.

Upon more experimenting, it seems as if only padding/margin scale as viewport does; height and width do not.

[François REMY]

I can confirm this bug. Reported at www-style by Giuseppe Bilotta<[e-mail]>;

[Giuseppe Bilotta]

Link to the discussion on www-style (with test cases): http://lists.w3.org/Archives/Public/www-style/2012Aug/0567.html

[Ojan Vafai]

Created attachment 159480 [details]
testcase

[Timothy Loh]

Created attachment 193747 [details]
Patch

[Timothy Loh]

The issue the above patch should fix is that we resolve the font-size and line-height values when we calculate styles, and we don't currently recalculate styles upon resize. The test case Ojan has attached is a slightly different issue arising from the code being designed with some no longer valid assumptions about font sizes and zoom. For fonts, we store a specified size (i.e. the intended pixel size), and a computed size (which takes zoom into account, as well as minimum and maximum font sizes). The calculation for the computed size simply takes the specified size so font sizes defined in viewport units still gets scaled.

[Timothy Loh]

Created attachment 194379 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 194379 [details]
Patch

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

> Source/WebCore/css/CSSGrammar.y.in:1761
> +      if (parser->m_styleSheet)
> +          parser->m_styleSheet->parserSetUsesViewportUnits(true);

It's not clear to me that this should be a side-effect of parsing.  This is a side-effect of parsing that we only sometimes want, right?

[Eric Seidel (no email)]

Comment on attachment 194379 [details]
Patch

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

>> Source/WebCore/css/CSSGrammar.y.in:1761
>> +          parser->m_styleSheet->parserSetUsesViewportUnits(true);
> 
> It's not clear to me that this should be a side-effect of parsing.  This is a side-effect of parsing that we only sometimes want, right?

For example, getComputedStyle presumably calls into the CSS parser but wouldn't want side-effects like this to occur.  I guess it wouldn't have an m_styleSheet set at the time?

[Timothy Loh]

Comment on attachment 194379 [details]
Patch

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

>>> Source/WebCore/css/CSSGrammar.y.in:1761
>>> +          parser->m_styleSheet->parserSetUsesViewportUnits(true);
>> 
>> It's not clear to me that this should be a side-effect of parsing.  This is a side-effect of parsing that we only sometimes want, right?
> 
> For example, getComputedStyle presumably calls into the CSS parser but wouldn't want side-effects like this to occur.  I guess it wouldn't have an m_styleSheet set at the time?

I think having this for any viewport units in a document means we'll be guaranteed to have viewport size information when we resolve CSSPrimitiveValues into  Length objects, which will mean we won't need viewport unit types for Length objects (sorry this isn't explicitly mentioned in the ChangeLog, I hadn't thought of it earlier). So while for this particular issue we'll only want it for font sizes and line heights, it seems like it'll make getting viewport units working with everything much simpler. Having viewport units go through the parser without actually using them doesn't seem like a case worth optimising right now.

[Bem Jones-Bey]

Created attachment 229147 [details]
Uber patch

Full change, to help explain the patch in Bug 131552

[Bem Jones-Bey]

Created attachment 230398 [details]
Patch

Updated patch for review

[Bem Jones-Bey]

Created attachment 230515 [details]
Patch

Fix build issues

[zalan]

Comment on attachment 230515 [details]
Patch

I have a feeling that ShapeInfo.cpp should not be in this patch.

[zalan]

Comment on attachment 230515 [details]
Patch

I have a feeling that ShapeInfo.cpp should not be in this patch.

[Build Bot]

Comment on attachment 230515 [details]
Patch

Attachment 230515 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6342612365606912

New failing tests:
http/tests/canvas/philip/tests/security.pattern.canvas.strokeStyle.html
accessibility/canvas-accessibilitynodeobject.html
accessibility/canvas-fallback-content.html
canvas/philip/tests/2d.drawImage.negativeSourceHeightAndWidth.html
http/tests/canvas/philip/tests/security.pattern.cross.html
compositing/sibling-positioning.html
http/tests/canvas/philip/tests/security.pattern.image.strokeStyle.html
accessibility/aria-used-on-image-maps.html
http/tests/canvas/philip/tests/security.reset.html
http/tests/canvas/philip/tests/security.pattern.canvas.timing.html
accessibility/canvas-fallback-content-2.html
compositing/generated-content.html
http/tests/canvas/philip/tests/security.drawImage.image.html
canvas/philip/tests/2d.drawImage.negativeSourceHeight2.html
canvas/philip/tests/2d.composite.operation.foobar.html
accessibility/internal-link-anchors2.html
accessibility/image-link-inline-cont.html
accessibility/aria-label.html
http/tests/canvas/philip/tests/security.pattern.image.fillStyle.html
http/tests/canvas/philip/tests/security.drawImage.canvas.html
accessibility/self-referencing-aria-labelledby.html
accessibility/element-haspopup.html
accessibility/aria-roles.html
http/tests/canvas/philip/tests/security.pattern.canvas.fillStyle.html
compositing/direct-image-compositing.html
accessibility/removed-continuation-element-causes-crash.html
http/tests/canvas/philip/tests/security.pattern.create.html
accessibility/image-map-title-causes-crash.html
accessibility/heading-title-includes-links.html
canvas/philip/tests/2d.drawImage.negativeSourceHeight.html

[Build Bot]

Created attachment 230521 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 230515 [details]
Patch

Attachment 230515 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4619516583608320

New failing tests:
accessibility/canvas-fallback-content-2.html
accessibility/canvas-accessibilitynodeobject.html
compositing/direct-image-compositing.html
css1/box_properties/acid_test.html
compositing/geometry/transfrom-origin-on-zero-size-layer.html
accessibility/aria-used-on-image-maps.html
compositing/repaint/become-overlay-composited-layer.html
compositing/sibling-positioning.html
css1/box_properties/border.html
accessibility/table-detection.html
compositing/generated-content.html
canvas/philip/tests/2d.drawImage.negativeSourceHeight2.html
canvas/philip/tests/2d.composite.operation.foobar.html
accessibility/internal-link-anchors2.html
accessibility/image-link-inline-cont.html
accessibility/aria-label.html
css1/basic/containment.html
accessibility/transformed-element.html
accessibility/self-referencing-aria-labelledby.html
accessibility/element-haspopup.html
accessibility/aria-roles.html
accessibility/canvas-fallback-content.html
accessibility/removed-continuation-element-causes-crash.html
css1/pseudo/anchor.html
css1/color_and_background/background_repeat.html
accessibility/image-map-title-causes-crash.html
canvas/philip/tests/2d.drawImage.negativeSourceHeightAndWidth.html
accessibility/heading-title-includes-links.html
canvas/philip/tests/2d.drawImage.negativeSourceHeight.html
compositing/geometry/root-layer-update.html

[Build Bot]

Created attachment 230527 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 230515 [details]
Patch

Attachment 230515 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5723419815444480

New failing tests:
accessibility/canvas-fallback-content-2.html
accessibility/canvas-accessibilitynodeobject.html
compositing/direct-image-compositing.html
css1/box_properties/acid_test.html
compositing/geometry/transfrom-origin-on-zero-size-layer.html
accessibility/aria-used-on-image-maps.html
compositing/repaint/become-overlay-composited-layer.html
compositing/sibling-positioning.html
css1/box_properties/border.html
accessibility/table-detection.html
compositing/generated-content.html
canvas/philip/tests/2d.drawImage.negativeSourceHeight2.html
canvas/philip/tests/2d.composite.operation.foobar.html
accessibility/internal-link-anchors2.html
accessibility/image-link-inline-cont.html
accessibility/aria-label.html
css1/basic/containment.html
accessibility/transformed-element.html
accessibility/self-referencing-aria-labelledby.html
accessibility/element-haspopup.html
accessibility/aria-roles.html
accessibility/canvas-fallback-content.html
accessibility/removed-continuation-element-causes-crash.html
css1/pseudo/anchor.html
css1/color_and_background/background_repeat.html
accessibility/image-map-title-causes-crash.html
canvas/philip/tests/2d.drawImage.negativeSourceHeightAndWidth.html
accessibility/heading-title-includes-links.html
canvas/philip/tests/2d.drawImage.negativeSourceHeight.html
compositing/geometry/root-layer-update.html

[Build Bot]

Created attachment 230532 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Bem Jones-Bey]

(In reply to comment #15)
> (From update of attachment 230515 [details])
> I have a feeling that ShapeInfo.cpp should not be in this patch.

That feeling is correct. Will remove and investigate the test failures. (Didn't see those locally for some reason...)

[Bem Jones-Bey]

Created attachment 231194 [details]
Patch

The tests are still broken in release builds. This is merely a checkpoint.

[Rik Cabanier]

Created attachment 231216 [details]
Patch

[Build Bot]

Comment on attachment 231216 [details]
Patch

Attachment 231216 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4796190197547008

New failing tests:
fast/block/basic/011.html
fast/forms/content-with-margins-inside-button.html
fast/repaint/box-shadow-h.html
fast/text/international/inline-plaintext-with-generated-content.html
fast/forms/floating-textfield-relayout.html
css1/box_properties/acid_test.html
fast/regions/counters/extract-ordered-lists-in-regions-explicit-counters-004.html
css2.1/t09-c5526c-display-00-e.html
fast/text/international/inline-plaintext-relayout-with-leading-neutrals.html
fast/regions/counters/extract-ordered-lists-in-regions-explicit-counters-005.html
fast/css/word-space-extra.html
fast/text/international/unicode-bidi-plaintext-in-textarea.html
fast/css/html-attr-case-sensitivity.html
css2.1/t0905-c414-flt-wrap-00-e.html
fast/text/bidi-override-isolate.html
fast/forms/basic-textareas-quirks.html
fast/block/margin-collapse/block-inside-inline/004.html
fast/text/trailing-white-space.html
fast/repaint/box-shadow-v.html
fast/regions/hit-test-region.html
fast/overflow/overflow-height-float-not-removed-crash3.html
fast/regions/counters/extract-ordered-lists-in-regions-001.html
fast/text/international/unicode-bidi-plaintext.html
fast/text/trailing-white-space-2.html
ietestcenter/css3/flexbox/flexbox-direction-001.htm
fast/text/international/inline-plaintext-is-isolated.html
fast/block/float/float-avoidance.html

[Build Bot]

Created attachment 231220 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 231216 [details]
Patch

Attachment 231216 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6399341501612032

New failing tests:
fast/block/basic/011.html
fast/forms/content-with-margins-inside-button.html
fast/repaint/box-shadow-h.html
fast/text/international/inline-plaintext-with-generated-content.html
fast/forms/floating-textfield-relayout.html
css1/box_properties/acid_test.html
fast/regions/counters/extract-ordered-lists-in-regions-explicit-counters-004.html
css2.1/t09-c5526c-display-00-e.html
fast/text/international/inline-plaintext-relayout-with-leading-neutrals.html
fast/regions/counters/extract-ordered-lists-in-regions-explicit-counters-005.html
fast/css/word-space-extra.html
fast/text/international/unicode-bidi-plaintext-in-textarea.html
fast/css/html-attr-case-sensitivity.html
css2.1/t0905-c414-flt-wrap-00-e.html
fast/text/bidi-override-isolate.html
fast/forms/basic-textareas-quirks.html
fast/block/margin-collapse/block-inside-inline/004.html
fast/text/trailing-white-space.html
fast/repaint/box-shadow-v.html
fast/regions/hit-test-region.html
fast/overflow/overflow-height-float-not-removed-crash3.html
fast/regions/counters/extract-ordered-lists-in-regions-001.html
fast/text/international/unicode-bidi-plaintext.html
fast/text/trailing-white-space-2.html
ietestcenter/css3/flexbox/flexbox-direction-001.htm
fast/text/international/inline-plaintext-is-isolated.html
fast/block/float/float-avoidance.html

[Build Bot]

Created attachment 231221 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 231216 [details]
Patch

Attachment 231216 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6386443177951232

New failing tests:
fast/block/basic/011.html
fast/forms/content-with-margins-inside-button.html
fast/repaint/box-shadow-h.html
fast/text/international/inline-plaintext-with-generated-content.html
fast/forms/floating-textfield-relayout.html
css1/box_properties/acid_test.html
fast/regions/counters/extract-ordered-lists-in-regions-explicit-counters-004.html
css2.1/t09-c5526c-display-00-e.html
fast/text/international/inline-plaintext-relayout-with-leading-neutrals.html
fast/regions/counters/extract-ordered-lists-in-regions-explicit-counters-005.html
fast/css/word-space-extra.html
fast/text/international/unicode-bidi-plaintext-in-textarea.html
fast/css/html-attr-case-sensitivity.html
css2.1/t0905-c414-flt-wrap-00-e.html
fast/text/bidi-override-isolate.html
fast/forms/basic-textareas-quirks.html
fast/block/margin-collapse/block-inside-inline/004.html
fast/text/trailing-white-space.html
fast/repaint/box-shadow-v.html
fast/regions/hit-test-region.html
fast/regions/counters/extract-ordered-lists-in-regions-001.html
fast/text/international/unicode-bidi-plaintext.html
fast/text/trailing-white-space-2.html
ietestcenter/css3/flexbox/flexbox-direction-001.htm
fast/text/international/inline-plaintext-is-isolated.html
fast/block/float/float-avoidance.html

[Build Bot]

Created attachment 231222 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 231216 [details]
Patch

Attachment 231216 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5040390495272960

New failing tests:
fast/block/basic/011.html
fast/forms/content-with-margins-inside-button.html
fast/repaint/box-shadow-h.html
fast/text/international/inline-plaintext-with-generated-content.html
fast/forms/floating-textfield-relayout.html
css1/box_properties/acid_test.html
fast/regions/counters/extract-ordered-lists-in-regions-explicit-counters-004.html
css2.1/t09-c5526c-display-00-e.html
fast/text/international/inline-plaintext-relayout-with-leading-neutrals.html
fast/regions/counters/extract-ordered-lists-in-regions-explicit-counters-005.html
fast/css/word-space-extra.html
fast/text/international/unicode-bidi-plaintext-in-textarea.html
fast/css/html-attr-case-sensitivity.html
css2.1/t0905-c414-flt-wrap-00-e.html
fast/text/bidi-override-isolate.html
fast/forms/basic-textareas-quirks.html
fast/block/margin-collapse/block-inside-inline/004.html
fast/text/trailing-white-space.html
fast/repaint/box-shadow-v.html
fast/regions/hit-test-region.html
fast/regions/counters/extract-ordered-lists-in-regions-001.html
fast/text/international/unicode-bidi-plaintext.html
fast/text/trailing-white-space-2.html
ietestcenter/css3/flexbox/flexbox-direction-001.htm
fast/text/international/inline-plaintext-is-isolated.html
fast/block/float/float-avoidance.html

[Build Bot]

Created attachment 231224 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Darin Adler]

Comment on attachment 231216 [details]
Patch

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

This looks like a good direction, but needs some additional scrutiny and test cases.

> Source/WebCore/css/CSSToLengthConversionData.h:37
> +#include "IntSize.h"
> +
>  #include <wtf/Assertions.h>
>  #include <wtf/Noncopyable.h>

No blank line here in WebKit coding style.

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:867
> +                // If we have viewport units the conversion will mark the parent style as having viewport units.
> +                bool parentStyleHasViewportUnits = parentStyle->hasViewportUnits();
> +                parentStyle->setHasViewportUnits(false);

This save/restore thing is an ugly way to handle it. Might be nice to find a cleaner way to deal with this.

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:1554
> -        return value.get().computeLength<Length>(CSSToLengthConversionData());
> +        return value.get().computeLength<Length>(CSSToLengthConversionData(nullptr, nullptr, nullptr));

Is this really an improvement? We should consider having default values for these arguments or a default constructor.

> Source/WebCore/css/MediaQueryEvaluator.cpp:730
> -        CSSToLengthConversionData conversionData(m_style.get(), m_frame->document()->documentElement()->renderStyle());
> +        auto rootRenderStyle = m_frame->document()->documentElement()->renderStyle();
> +        auto renderView = m_frame->document()->renderView();
> +        CSSToLengthConversionData conversionData(m_style.get(), rootRenderStyle, renderView);

Not sure these local variables help all that much. Might have been nicer just to break this up into multiple lines, indenting by four.

> Source/WebCore/css/StyleResolver.cpp:238
> +    m_cssToLengthConversionData = CSSToLengthConversionData(nullptr, nullptr, nullptr);

This nullptr, nullptr, nullptr thing is completely unclear. A default argument would be better, something more explicit probably even better.

> Source/WebCore/css/StyleResolver.h:336
> +            : m_element(0)

nullptr

> Source/WebCore/css/StyleResolver.h:337
> +            , m_styledElement(0)

nullptr

> Source/WebCore/css/StyleResolver.h:338
> +            , m_parentStyle(0)

nullptr

> Source/WebCore/css/StyleResolver.h:339
> +            , m_rootElementStyle(0)

nullptr

> Source/WebCore/css/StyleResolver.h:340
> +            , m_regionForStyling(0)

nullptr

> Source/WebCore/css/StyleResolver.h:350
> +            , m_cssToLengthConversionData(nullptr, nullptr, nullptr)

IF we had a default constructor we could (and should) just leave this out entirely.

> Source/WebCore/css/StyleResolver.h:351
> +            { }

Should not be indented. Also would be nicer on two successive lines.

> Source/WebCore/css/StyleResolver.h:388
> +        bool fontSizeHasViewportUnits() { return m_fontSizeHasViewportUnits; }

const

> Source/WebCore/dom/Document.h:1279
> +    void setHasViewportUnits() { m_hasViewportUnits = true; }
> +    bool hasViewportUnits() const { return m_hasViewportUnits; }

I don’t think the name of this is good. Does it make sense to say a document “has viewport units”. There must be some more precise language that is not too wordy.

> Source/WebCore/dom/TreeScope.cpp:408
> +    for (Element* element = ElementTraversal::firstWithin(&rootNode()); element; element = ElementTraversal::nextIncludingPseudo(element)) {

Does not seem right to have a TreeScope public function for this; this is too specific an operation for that. I suggest considering leaving this in-line at the call site, or making it a private function in Document. A protected function in TreeScope might be OK.

For new code we should use iterators rather than traversal functions. But maybe there is none for “including pseudo”.

I was thinking we would want to traverse the render tree, not the DOM tree, but I’m not sure. Besides pseudo-elements, what other benefit would there be to traversing the render tree instead? One benefit is that we’d efficiently skip non-rendered elements, which seems well worth it.

Since most documents don’t have many elements in this category, I think we probably want a different approach, perhaps maintaining a set of all the renderers with styles that have viewport units or all the nodes in that category.

> Source/WebCore/dom/TreeScope.cpp:409
> +        RenderStyle* style = element->renderStyle();

This is a bad idea. For elements without renderers it’s going to call nonRendererStyle, which is costly and also gives us styles that are not relevant for setNeedsStyleRecalc.

> Source/WebCore/page/FrameView.cpp:1140
> +        document.notifyResizeForViewportUnits();

Why are we calling this unconditionally here? There’s no guarantee that a resize has occurred. Looks like we’d do a lot of extra work for many cases; need to check if the size is different or not before telling everything it needs style recalculation! And walking the whole document an extra time just for that!

[Rik Cabanier]

Created attachment 231259 [details]
Patch

[Bem Jones-Bey]

Created attachment 231724 [details]
Patch

Addressed all the review feedback except this patch is still walking the DOM tree. I don't see an iterator for this walk of the DOM tree, and there isn't even a traversal function (much less an iterator) to traverse the entire render tree. I also looked into changing it to save the renderers or elements that have viewport units so that we would only have to iterate over those, but I haven't been able to come up with an approach that would work there. I would be happy to add an iterator so that this can iterate over the render tree, but I would like to do that in a future patch.

[Darin Adler]

(In reply to comment #35)
> there isn't even a traversal function (much less an iterator) to traverse the entire render tree

RenderObject::nextInPreOrder is a traversal function to traverse the entire render tree.

[Darin Adler]

Comment on attachment 231724 [details]
Patch

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

This patch seems quite good. I reluctantly need to review- because of the for loop mistake that is copying every MatchedPropertiesCacheItem.

> Source/WebCore/css/CSSToLengthConversionData.cpp:69
> +    if (m_renderView) {
> +        IntSize viewportSize = m_renderView->viewportSize();
> +        return std::min(viewportSize.width(), viewportSize.height()) / 100.0;
> +    }
> +
> +    return 0.0;

WebKit coding style is to use early return:

    if (!m_renderView)
        return 0;
    ...

We don’t like to nest the normal case inside an if statement.

> Source/WebCore/css/StyleResolver.cpp:1582
> +    for (auto cacheKeyValue : m_matchedPropertiesCache) {

This is going to copy each item out of the cache, and we definitely don’t want to do that! That’s copying a vector and churning the reference count on two objects among other things. To avoid that, simply use auto& instead of auto here.

> Source/WebCore/dom/Document.cpp:3190
> +    // FIXME Ideally, we should save the list of elements that have viewport units and only iterate over those.

WebKit comment style puts a colon after the FIXME.

> Source/WebCore/dom/Document.cpp:3192
> +        RenderObject* renderObject = element->renderer();

I would suggest calling this local variable renderer rather than renderObject. Also, the correct type for Element::renderer is RenderElement*, not RenderObject*; using RenderObject unnecessarily lowers the type and calls the slower RenderObject::style rather than the faster RenderElement::style. I suggest using auto or auto* so we automatically get the type right, or RenderElement* would also be OK.

> Source/WebCore/dom/Document.h:1280
> +    void notifyResizeForViewportUnits();

While there are other functions with names like this, I really don’t like them. What does “notify resize for viewport units” mean? We could probably make a phrase for this that makes sense in English and is less jargony.

[Bem Jones-Bey]

(In reply to comment #36)
> (In reply to comment #35)
> > there isn't even a traversal function (much less an iterator) to traverse the entire render tree
> 
> RenderObject::nextInPreOrder is a traversal function to traverse the entire render tree.

Doh. I was just looking at the RenderTraversal and RenderIterators. For now, I'd like to stick with iterating over the DOM tree, since it's not clear to me that the render tree would be faster in all cases. If one iterates over the render tree, then one does skip all elements without renderers, but when iterating over the DOM tree, you skip all of the anonymous renderers. I'm not sure which is more prevalent on real pages.

(In reply to comment #37)
> (From update of attachment 231724 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=231724&action=review
> 
> This patch seems quite good. I reluctantly need to review- because of the for loop mistake that is copying every MatchedPropertiesCacheItem.
> 
> > Source/WebCore/css/CSSToLengthConversionData.cpp:69
> > +    if (m_renderView) {
> > +        IntSize viewportSize = m_renderView->viewportSize();
> > +        return std::min(viewportSize.width(), viewportSize.height()) / 100.0;
> > +    }
> > +
> > +    return 0.0;
> 
> WebKit coding style is to use early return:
> 
>     if (!m_renderView)
>         return 0;
>     ...
> 
> We don’t like to nest the normal case inside an if statement.

Oops. That was a oversight on my part. Fixed.

> 
> > Source/WebCore/css/StyleResolver.cpp:1582
> > +    for (auto cacheKeyValue : m_matchedPropertiesCache) {
> 
> This is going to copy each item out of the cache, and we definitely don’t want to do that! That’s copying a vector and churning the reference count on two objects among other things. To avoid that, simply use auto& instead of auto here.

Fixed.

> 
> > Source/WebCore/dom/Document.cpp:3190
> > +    // FIXME Ideally, we should save the list of elements that have viewport units and only iterate over those.
> 
> WebKit comment style puts a colon after the FIXME.

Fixed.

> 
> > Source/WebCore/dom/Document.cpp:3192
> > +        RenderObject* renderObject = element->renderer();
> 
> I would suggest calling this local variable renderer rather than renderObject. Also, the correct type for Element::renderer is RenderElement*, not RenderObject*; using RenderObject unnecessarily lowers the type and calls the slower RenderObject::style rather than the faster RenderElement::style. I suggest using auto or auto* so we automatically get the type right, or RenderElement* would also be OK.

Ah, I had renamed it because lldb was getting confused between the local variable renderer and the renderer member function. I've changed the name back to renderer, and made it auto*.

> 
> > Source/WebCore/dom/Document.h:1280
> > +    void notifyResizeForViewportUnits();
> 
> While there are other functions with names like this, I really don’t like them. What does “notify resize for viewport units” mean? We could probably make a phrase for this that makes sense in English and is less jargony.

I agree. I've renamed both this one and the function with the same name on StyleResolver to be something that makes more sense.

[Bem Jones-Bey]

Created attachment 232152 [details]
Patch

Address review feedback.

[Darin Adler]

Comment on attachment 232152 [details]
Patch

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

> Source/WebCore/css/CSSGradientValue.cpp:1120
> +    addStops(*gradient,  conversionData, maxExtent);

Extra space here after the comma.

> Source/WebCore/css/CSSToLengthConversionData.h:34
> +#include "IntSize.h"

Why are we adding this include? I don’t see anything new in the header that seems to be using it.

[Bem Jones-Bey]

(In reply to comment #40)
> (From update of attachment 232152 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=232152&action=review
> 
> > Source/WebCore/css/CSSGradientValue.cpp:1120
> > +    addStops(*gradient,  conversionData, maxExtent);
> 
> Extra space here after the comma.

Removed.

> 
> > Source/WebCore/css/CSSToLengthConversionData.h:34
> > +#include "IntSize.h"
> 
> Why are we adding this include? I don’t see anything new in the header that seems to be using it.

Forgot to remove it after an earlier refactoring. Removed.

[Bem Jones-Bey]

Created attachment 232160 [details]
Patch

Patch for landing

[WebKit Commit Bot]

Comment on attachment 232160 [details]
Patch

Clearing flags on attachment: 232160

Committed r169407: <http://trac.webkit.org/changeset/169407>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

A test is failing after this commit:
http://build.webkit.org/results/Apple%20Mavericks%20Release%20WK2%20(Tests)/r169407%20(6003)/results.html

[Bem Jones-Bey]

(In reply to comment #45)
> A test is failing after this commit:
> http://build.webkit.org/results/Apple%20Mavericks%20Release%20WK2%20(Tests)/r169407%20(6003)/results.html

Hrm. That looks like a flake. It isn't failing anymore on the bots, and it doesn't fail for me locally on Mavericks. I'm open to suggestions about how I could make the test more robust.

[Simon Fraser (smfr)]

It flaked again later:
http://build.webkit.org/results/Apple%20Mavericks%20Release%20WK2%20(Tests)/r169416%20(6007)/results.html

Seems like the test needs to be made more robust.

[Bem Jones-Bey]

(In reply to comment #47)
> It flaked again later:
> http://build.webkit.org/results/Apple%20Mavericks%20Release%20WK2%20(Tests)/r169416%20(6007)/results.html
> 
> Seems like the test needs to be made more robust.

I've put a fix for the tests in Bug 133351.