Need API to control page underlay color

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Conrad Shultz]

It would be useful for clients to have API to customize the background/underlay color displayed beneath the page, e.g. during rubber-banding.

[Conrad Shultz]

Created attachment 190402 [details]
Patch

[Tim Horton]

Comment on attachment 190402 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1871
> +void WebPage::setUnderlayColor(const WebCore::Color& color)

This file using namespace WebCore;s. Maybe some of the others too, but definitely this one.

[Conrad Shultz]

Comment on attachment 190402 [details]
Patch

Will submit a new version based on feedback.

[Conrad Shultz]

Created attachment 190662 [details]
Patch

[Early Warning System Bot]

Comment on attachment 190662 [details]
Patch

Attachment 190662 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16791196

[WebKit Review Bot]

Comment on attachment 190662 [details]
Patch

Attachment 190662 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16771916

[Conrad Shultz]

Created attachment 190668 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 190668 [details]
Patch

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

> Source/WebCore/page/ChromeClient.h:190
> +    virtual Color underlayColor() const = 0;

I think this should not be pure virtual, and can return Color() so you don't have to touch all the other clients.

> Source/WebCore/platform/mac/ScrollbarThemeMac.mm:620
> +    graphicsLayer->platformLayer().backgroundColor = backgroundColor.isValid() ? nsColor(backgroundColor).CGColor : cachedLinenBackgroundColor;

I think this should use cachedCGColor(backgroundColor, ColorSpaceDeviceRGB).

[Conrad Shultz]

(In reply to comment #8)
> (From update of attachment 190668 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190668&action=review
> 
> > Source/WebCore/page/ChromeClient.h:190
> > +    virtual Color underlayColor() const = 0;
> 
> I think this should not be pure virtual, and can return Color() so you don't have to touch all the other clients.

Changed.

> 
> > Source/WebCore/platform/mac/ScrollbarThemeMac.mm:620
> > +    graphicsLayer->platformLayer().backgroundColor = backgroundColor.isValid() ? nsColor(backgroundColor).CGColor : cachedLinenBackgroundColor;
> 
> I think this should use cachedCGColor(backgroundColor, ColorSpaceDeviceRGB).

Good idea, changed.

Will post a revised patch shortly.

[Conrad Shultz]

Created attachment 190766 [details]
Patch

[WebKit Review Bot]

Comment on attachment 190766 [details]
Patch

Clearing flags on attachment: 190766

Committed r144397: <http://trac.webkit.org/changeset/144397>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 190766 [details]
Patch

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

> Source/WebKit2/UIProcess/WebPageProxy.cpp:865
> +    if (isValid())
> +        m_process->send(Messages::WebPage::SetUnderlayColor(color), m_pageID);

I don’t understand why the isValid() check here is a good idea. If we set the underlay color to an invalid color, then m_underlayColor gets set, but the other process is never told and is left with the old underlay color? That’s strange behavior. Seems we should send a SetUnderlayColor message to clear the underlay color if the new color is invalid.

[Tim Horton]

(In reply to comment #13)
> (From update of attachment 190766 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190766&action=review
> 
> > Source/WebKit2/UIProcess/WebPageProxy.cpp:865
> > +    if (isValid())
> > +        m_process->send(Messages::WebPage::SetUnderlayColor(color), m_pageID);
> 
> I don’t understand why the isValid() check here is a good idea. If we set the underlay color to an invalid color, then m_underlayColor gets set, but the other process is never told and is left with the old underlay color? That’s strange behavior. Seems we should send a SetUnderlayColor message to clear the underlay color if the new color is invalid.

isValid checks if the WebProcess is valid, not the color (right?). If we try to send a message to !isValid() WebProcess, we crash. We've really gotta sort this out, because there are a bunch of WebPageProxy methods where it's easy to cause a crash by trying to do something from the UIProcess at the wrong time (especially post-WebProcess-crash).