[webkit-reviews] review denied: [Bug 87846] vw/vh units used as font/line-height values don't scale with the viewport : [Attachment 231216] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Rik Cabanier <cabanier at adobe.com>'s
request for review:
Bug 87846: vw/vh units used as font/line-height values don't scale with the
viewport
https://bugs.webkit.org/show_bug.cgi?id=87846

Attachment 231216: Patch
https://bugs.webkit.org/attachment.cgi?id=231216&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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!