[webkit-reviews] review granted: [Bug 131600] Support setting a background color on page overlays : [Attachment 229265] check color validity
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sun Apr 13 23:45:46 PDT 2014
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.
More information about the webkit-reviews
mailing list