Support setting a background color on page overlays

• Component: WebKit2
• Status: RESOLVED FIXED

[Tim Horton]

So that clients who want to highlight a particular region of the page can do so with a page overlay without any backing store.

[Tim Horton]

Created attachment 229249 [details]
patch

[Benjamin Poulain]

Comment on attachment 229249 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=229249&action=review

> Source/WebKit2/WebProcess/WebPage/PageOverlayController.cpp:92
> +    layer->setBackgroundColor(overlay->backgroundColor());

What if the background color was not set on the overlay? Color would be default uninitialized.

> Source/WebKit2/WebProcess/WebPage/PageOverlayController.cpp:-144
> -    graphicsLayer->setNeedsDisplay();

Why do you remove this?

[Tim Horton]

(In reply to comment #2)
> (From update of attachment 229249 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=229249&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/PageOverlayController.cpp:92
> > +    layer->setBackgroundColor(overlay->backgroundColor());
> 
> What if the background color was not set on the overlay? Color would be default uninitialized.

Color is by default initialized to a !isValid color, which I thought GraphicsLayer handled correctly but it doesn't, so it's probably just using black with alpha=0. Will fix.

> > Source/WebKit2/WebProcess/WebPage/PageOverlayController.cpp:-144
> > -    graphicsLayer->setNeedsDisplay();
> 
> Why do you remove this?

setSize does it already if needed.

[Tim Horton]

Created attachment 229265 [details]
check color validity

[Darin Adler]

Comment on attachment 229265 [details]
check color validity

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.

[Tim Horton]

Comment on attachment 229265 [details]
check color validity

View in context: https://bugs.webkit.org/attachment.cgi?id=229265&action=review

>> Source/WebKit2/WebProcess/WebPage/PageOverlay.cpp:85
>> +        return IntRect(IntPoint(), frameSize);
> 
> This doesn’t seem like much of an improvement.

Fair enough.

>> Source/WebKit2/WebProcess/WebPage/PageOverlay.cpp:116
>> +        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.

True.

Invalid colors are fine, as discussed below, since they're just effectively transparent black when used. Maybe I should go back to the previous version of the patch that didn't treat them as special at all.

>> Source/WebKit2/WebProcess/WebPage/PageOverlayController.cpp:325
>> +        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.

Or... yeah. I could use the RGBA32 type everywhere, and just initialize it to transparent in PageOverlay. I didn't realize that type was in use outside of Color internals, but it looks like it is.

[Tim Horton]

http://trac.webkit.org/changeset/167216