Bug 131600 - Support setting a background color on page overlays
Summary: Support setting a background color on page overlays
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-04-13 16:37 PDT by Tim Horton
Modified: 2014-04-14 01:10 PDT (History)
6 users (show)

See Also:


Attachments
patch (11.53 KB, patch)
2014-04-13 16:41 PDT, Tim Horton
benjamin: review+
Details | Formatted Diff | Diff
check color validity (11.82 KB, patch)
2014-04-13 23:36 PDT, Tim Horton
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2014-04-13 16:37:11 PDT
So that clients who want to highlight a particular region of the page can do so with a page overlay without any backing store.
Comment 1 Tim Horton 2014-04-13 16:41:08 PDT
Created attachment 229249 [details]
patch
Comment 2 Benjamin Poulain 2014-04-13 20:25:39 PDT
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?
Comment 3 Tim Horton 2014-04-13 23:27:39 PDT
(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.
Comment 4 Tim Horton 2014-04-13 23:36:53 PDT
Created attachment 229265 [details]
check color validity
Comment 5 Darin Adler 2014-04-13 23:46:07 PDT
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.
Comment 6 Tim Horton 2014-04-14 00:03:32 PDT
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.
Comment 7 Tim Horton 2014-04-14 01:10:58 PDT
http://trac.webkit.org/changeset/167216