Bug 87846 - vw/vh units used as font/line-height values don't scale with the viewport
Summary: vw/vh units used as font/line-height values don't scale with the viewport
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Bem Jones-Bey
URL:
Keywords:
Depends on: 131552
Blocks: 124052
  Show dependency treegraph
 
Reported: 2012-05-30 05:32 PDT by Sam
Modified: 2014-05-28 11:33 PDT (History)
50 users (show)

See Also:


Attachments
testcase (541 bytes, text/html)
2012-08-20 11:41 PDT, Ojan Vafai
no flags Details
Patch (14.09 KB, patch)
2013-03-19 00:06 PDT, Timothy Loh
no flags Details | Formatted Diff | Diff
Patch (14.80 KB, patch)
2013-03-21 16:35 PDT, Timothy Loh
no flags Details | Formatted Diff | Diff
Uber patch (282.80 KB, patch)
2014-04-11 12:12 PDT, Bem Jones-Bey
no flags Details | Formatted Diff | Diff
Patch (177.11 KB, patch)
2014-04-29 12:21 PDT, Bem Jones-Bey
no flags Details | Formatted Diff | Diff
Patch (178.24 KB, patch)
2014-04-30 13:26 PDT, Bem Jones-Bey
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (141.06 KB, application/zip)
2014-04-30 14:27 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (203.24 KB, application/zip)
2014-04-30 15:03 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (203.21 KB, application/zip)
2014-04-30 16:04 PDT, Build Bot
no flags Details
Patch (193.02 KB, patch)
2014-05-09 16:32 PDT, Bem Jones-Bey
no flags Details | Formatted Diff | Diff
Patch (187.05 KB, patch)
2014-05-09 22:51 PDT, Rik Cabanier
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion (2.70 MB, application/zip)
2014-05-10 00:50 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (2.69 MB, application/zip)
2014-05-10 01:43 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 (2.66 MB, application/zip)
2014-05-10 02:09 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (2.66 MB, application/zip)
2014-05-10 02:58 PDT, Build Bot
no flags Details
Patch (187.50 KB, patch)
2014-05-11 03:31 PDT, Rik Cabanier
no flags Details | Formatted Diff | Diff
Patch (216.60 KB, patch)
2014-05-19 14:29 PDT, Bem Jones-Bey
no flags Details | Formatted Diff | Diff
Patch (219.72 KB, patch)
2014-05-27 15:37 PDT, Bem Jones-Bey
no flags Details | Formatted Diff | Diff
Patch (219.56 KB, patch)
2014-05-27 17:55 PDT, Bem Jones-Bey
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam 2012-05-30 05:32:36 PDT
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.
Comment 1 Sam 2012-05-30 06:28:50 PDT
(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.
Comment 2 François REMY 2012-08-20 09:26:50 PDT
I can confirm this bug. Reported at www-style by Giuseppe Bilotta<giuseppe.bilotta@gmail.com>;
Comment 3 Giuseppe Bilotta 2012-08-20 10:07:36 PDT
Link to the discussion on www-style (with test cases): http://lists.w3.org/Archives/Public/www-style/2012Aug/0567.html
Comment 4 Ojan Vafai 2012-08-20 11:41:19 PDT
Created attachment 159480 [details]
testcase
Comment 5 Timothy Loh 2013-03-19 00:06:27 PDT
Created attachment 193747 [details]
Patch
Comment 6 Timothy Loh 2013-03-19 00:19:42 PDT
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.
Comment 7 Timothy Loh 2013-03-21 16:35:10 PDT
Created attachment 194379 [details]
Patch
Comment 8 Eric Seidel (no email) 2013-03-27 17:51:13 PDT
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?
Comment 9 Eric Seidel (no email) 2013-03-27 17:52:47 PDT
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?
Comment 10 Timothy Loh 2013-03-27 18:05:47 PDT
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.
Comment 11 Bem Jones-Bey 2014-04-11 12:12:26 PDT
Created attachment 229147 [details]
Uber patch

Full change, to help explain the patch in Bug 131552
Comment 12 Bem Jones-Bey 2014-04-29 12:21:29 PDT
Created attachment 230398 [details]
Patch

Updated patch for review
Comment 13 Bem Jones-Bey 2014-04-30 13:26:58 PDT
Created attachment 230515 [details]
Patch

Fix build issues
Comment 14 zalan 2014-04-30 14:26:54 PDT
Comment on attachment 230515 [details]
Patch

I have a feeling that ShapeInfo.cpp should not be in this patch.
Comment 15 zalan 2014-04-30 14:27:12 PDT
Comment on attachment 230515 [details]
Patch

I have a feeling that ShapeInfo.cpp should not be in this patch.
Comment 16 Build Bot 2014-04-30 14:27:14 PDT
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
Comment 17 Build Bot 2014-04-30 14:27:33 PDT
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
Comment 18 Build Bot 2014-04-30 15:03:27 PDT
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
Comment 19 Build Bot 2014-04-30 15:03:41 PDT
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
Comment 20 Build Bot 2014-04-30 16:04:37 PDT
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
Comment 21 Build Bot 2014-04-30 16:04:51 PDT
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
Comment 22 Bem Jones-Bey 2014-04-30 16:06:42 PDT
(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...)
Comment 23 Bem Jones-Bey 2014-05-09 16:32:28 PDT
Created attachment 231194 [details]
Patch

The tests are still broken in release builds. This is merely a checkpoint.
Comment 24 Rik Cabanier 2014-05-09 22:51:26 PDT
Created attachment 231216 [details]
Patch
Comment 25 Build Bot 2014-05-10 00:50:13 PDT
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
Comment 26 Build Bot 2014-05-10 00:50:34 PDT
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
Comment 27 Build Bot 2014-05-10 01:43:06 PDT
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
Comment 28 Build Bot 2014-05-10 01:43:17 PDT
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
Comment 29 Build Bot 2014-05-10 02:09:15 PDT
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
Comment 30 Build Bot 2014-05-10 02:09:28 PDT
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
Comment 31 Build Bot 2014-05-10 02:58:20 PDT
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
Comment 32 Build Bot 2014-05-10 02:58:32 PDT
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
Comment 33 Darin Adler 2014-05-10 11:31:28 PDT
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!
Comment 34 Rik Cabanier 2014-05-11 03:31:46 PDT
Created attachment 231259 [details]
Patch
Comment 35 Bem Jones-Bey 2014-05-19 14:29:34 PDT
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.
Comment 36 Darin Adler 2014-05-24 19:14:18 PDT
(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.
Comment 37 Darin Adler 2014-05-24 19:25:03 PDT
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.
Comment 38 Bem Jones-Bey 2014-05-27 15:32:03 PDT
(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.
Comment 39 Bem Jones-Bey 2014-05-27 15:37:27 PDT
Created attachment 232152 [details]
Patch

Address review feedback.
Comment 40 Darin Adler 2014-05-27 16:41:48 PDT
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.
Comment 41 Bem Jones-Bey 2014-05-27 17:54:38 PDT
(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.
Comment 42 Bem Jones-Bey 2014-05-27 17:55:36 PDT
Created attachment 232160 [details]
Patch

Patch for landing
Comment 43 WebKit Commit Bot 2014-05-27 18:33:57 PDT
Comment on attachment 232160 [details]
Patch

Clearing flags on attachment: 232160

Committed r169407: <http://trac.webkit.org/changeset/169407>
Comment 44 WebKit Commit Bot 2014-05-27 18:34:13 PDT
All reviewed patches have been landed.  Closing bug.
Comment 45 Simon Fraser (smfr) 2014-05-27 22:43:32 PDT
A test is failing after this commit:
http://build.webkit.org/results/Apple%20Mavericks%20Release%20WK2%20(Tests)/r169407%20(6003)/results.html
Comment 46 Bem Jones-Bey 2014-05-28 08:18:42 PDT
(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.
Comment 47 Simon Fraser (smfr) 2014-05-28 08:45:52 PDT
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.
Comment 48 Bem Jones-Bey 2014-05-28 11:33:54 PDT
(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.