[webkit-reviews] review denied: [Bug 59183] Overlay scroller hard to see on pages with dark background : [Attachment 90683] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Jon Lee <jonlee at apple.com>'s request for
review:
Bug 59183: Overlay scroller hard to see on pages with dark background
https://bugs.webkit.org/show_bug.cgi?id=59183

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

------- Additional Comments from Sam Weinig <sam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=90683&action=review

> Source/WebCore/platform/mac/ScrollbarThemeMac.mm:214
> +    if (!scrollbar->parent()->isFrameView())
> +	   return ScrollbarOverlayStyleDefault;
> +    
> +    FrameView* frameView = static_cast<FrameView*>(scrollbar->parent());
> +    Document* document = frameView->frame()->document();
> +    Element* rootElementToUse = document->body();
> +    if (!rootElementToUse)
> +	   rootElementToUse = document->documentElement();
> +    if (!rootElementToUse)
> +	   return ScrollbarOverlayStyleDefault;
> +    
> +    RenderObject* renderer = rootElementToUse->renderer();
> +    if (!renderer)
> +	   return ScrollbarOverlayStyleDefault;
> +    Color color =
renderer->style()->visitedDependentColor(CSSPropertyBackgroundColor);
> +    if (!color.isValid())
> +	   return ScrollbarOverlayStyleDefault;
> +    
> +    // reduce the background color from RGB to a lightness value
> +    // and determine which scrollbar style to use based on a magic
> +    // lightness heuristic
> +    double h, s, l;
> +    color.getHSL(h, s, l);
> +    if (l < .5)
> +	   return ScrollbarOverlayStyleLight;

This is a layering violation, since classes in WebCore/platform are not
supposed to know about rendering, the DOM, or the frame tree.  Instead, I think
you should add a virtual method to ScrollableArea which FrameView can implement
to return the background color.  This will also make this possible to implement
for overflow areas in the future.