[webkit-reviews] review granted: [Bug 131600] Support setting a background color on page overlays : [Attachment 229265] check color validity

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Tim Horton <thorton at apple.com>'s
request for review:
Bug 131600: Support setting a background color on page overlays
https://bugs.webkit.org/show_bug.cgi?id=131600

Attachment 229265: check color validity
https://bugs.webkit.org/attachment.cgi?id=229265&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=229265&action=review


> Source/WebKit2/WebProcess/WebPage/PageOverlay.cpp:85
> -	   int width = frameView->width();
> -	   int height = frameView->height();
> -
> -	   if (!ScrollbarTheme::theme()->usesOverlayScrollbars()) {
> -	       if (frameView->verticalScrollbar())
> -		   width -= frameView->verticalScrollbar()->width();
> -	       if (frameView->horizontalScrollbar())
> -		   height -= frameView->horizontalScrollbar()->height();
> -	   }
> -	   return IntRect(0, 0, width, height);
> +	   IntSize frameSize = frameView->size();
> +
> +	   if (ScrollbarTheme::theme()->usesOverlayScrollbars())
> +	       return IntRect(IntPoint(), frameSize);
> +
> +	   if (frameView->verticalScrollbar())
> +	       frameSize.setWidth(frameSize.width() -
frameView->verticalScrollbar()->width());
> +	   if (frameView->horizontalScrollbar())
> +	       frameSize.setHeight(frameSize.height() -
frameView->horizontalScrollbar()->height());
> +
> +	   return IntRect(IntPoint(), frameSize);

This doesn’t seem like much of an improvement.

> Source/WebKit2/WebProcess/WebPage/PageOverlay.cpp:116
> +    m_backgroundColor = backgroundColor;
> +
> +    if (m_webPage)
> +	  
m_webPage->pageOverlayController().didChangeOverlayBackgroundColor(this);

Seems we should not call didChangeOverlayBackgroundColor if the color didn’t
change.

Also, is an invalid color OK or not? If not, we should assert that it’s not
invalid here.

> Source/WebKit2/WebProcess/WebPage/PageOverlayController.cpp:63
> +static void updateOverlayGeometry(PageOverlay* overlay, GraphicsLayer*
graphicsLayer)

This function should take references, not pointers.

> Source/WebKit2/WebProcess/WebPage/PageOverlayController.cpp:317
> +void PageOverlayController::didChangeOverlayBackgroundColor(PageOverlay*
overlay)

This function should take a reference, not a pointer.

> Source/WebKit2/WebProcess/WebPage/PageOverlayController.cpp:320
> +    GraphicsLayer* graphicsLayer = m_overlayGraphicsLayers.get(overlay);

Since we know this is non-null, we should dereference immediately and store in
a reference rather than a pointer.

> Source/WebKit2/WebProcess/WebPage/PageOverlayController.cpp:325
> +    Color backgroundColor = overlay->backgroundColor();
> +    if (backgroundColor.isValid())
> +	   graphicsLayer->setBackgroundColor(backgroundColor);
> +    else
> +	   graphicsLayer->setBackgroundColor(Color::transparent);

Should have a function that turns invalid colors into transparent. I think that
if you don’t look at the validity state, an invalid color already acts like a
transparent color, so it’s really just clearing the redundant isValid flag. In
fact, the only difference between a Color and a straight RGB number is that a
Color has the extra valid bit. So it seems lame that the setBackgroundColor
function takes a Color if an invalid color is not allowed, since a color that
can’t be invalid would really be a different, smaller, type.