Bug 89033 – [Chromium] Sub-pixel text rendering is incorrectly used for WebView with transparent background.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[David Reveman]

This is only a problem when software rendering is used. The compositor already handles this properly.

[David Reveman]

Created attachment 147402 [details]
Patch

Possible fix. Not sure if this is how we want solve this.

[James Robinson]

https://bugs.webkit.org/show_bug.cgi?id=89031 happens to solve this.

[James Robinson]

Comment on attachment 147402 [details]
Patch

I think you'd be better off calling setDrawingToImageBuffer() in NonCompositedContentHost, or just waiting for https://bugs.webkit.org/show_bug.cgi?id=89031

[David Reveman]

(In reply to comment #2)
> https://bugs.webkit.org/show_bug.cgi?id=89031 happens to solve this.

(In reply to comment #3)
> (From update of attachment 147402 [details])
> I think you'd be better off calling setDrawingToImageBuffer() in NonCompositedContentHost, or just waiting for https://bugs.webkit.org/show_bug.cgi?id=89031

I'm confused. This patch is to avoid sub-pixel rendering in software mode. Not when the compositor is used which is already handled correctly. So changes to NonCompositedContentHost isn't going to help here afaik.

Tried the patch attached to https://bugs.webkit.org/show_bug.cgi?id=89031 and it doesn't fix the problem.

[James Robinson]

Comment on attachment 147402 [details]
Patch

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

> Source/WebKit/chromium/src/WebPagePopupImpl.cpp:240
> +    PageWidgetDelegate::paint(m_page.get(), 0, canvas, rect, !m_webView->isTransparent());

this isn't obvious to me - if a web view is transparent, does that really mean that popups associated with that view are also transparent?  how is the background color of a popup determined?

[James Robinson]

Ah sorry, I didn't realize this was for software mode.

[David Reveman]

(In reply to comment #5)
> (From update of attachment 147402 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147402&action=review
> 
> > Source/WebKit/chromium/src/WebPagePopupImpl.cpp:240
> > +    PageWidgetDelegate::paint(m_page.get(), 0, canvas, rect, !m_webView->isTransparent());
> 
> this isn't obvious to me - if a web view is transparent, does that really mean that popups associated with that view are also transparent?  how is the background color of a popup determined?

yea, that's wrong. popups are always opaque afaik. WebPagePopupImpl::initPage() calls frame->view()->setTransparent(false).

btw, not sure how to test this other than the manual testing I'm doing on chromeos right now. any ideas?

[David Reveman]

Created attachment 147626 [details]
Patch

Popups are always opaque

[James Robinson]

(In reply to comment #7)
> (In reply to comment #5)
> > (From update of attachment 147402 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=147402&action=review
> > 
> > > Source/WebKit/chromium/src/WebPagePopupImpl.cpp:240
> > > +    PageWidgetDelegate::paint(m_page.get(), 0, canvas, rect, !m_webView->isTransparent());
> > 
> > this isn't obvious to me - if a web view is transparent, does that really mean that popups associated with that view are also transparent?  how is the background color of a popup determined?
> 
> yea, that's wrong. popups are always opaque afaik. WebPagePopupImpl::initPage() calls frame->view()->setTransparent(false).
> 
> btw, not sure how to test this other than the manual testing I'm doing on chromeos right now. any ideas?

So sadly we force grayscale all the time with layout tests, so there's no way that I know of in WebKit to check this automatically :(

[James Robinson]

Comment on attachment 147626 [details]
Patch

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

R=me but consider 2-state enum instead of bool for the paint parameter

> Source/WebKit/chromium/src/WebPagePopupImpl.cpp:240
> +    PageWidgetDelegate::paint(m_page.get(), 0, canvas, rect, true);

this is a good sign that we want a two-state enum here instead of a bool - it's really hard to guess what "true" means in this context.

[David Reveman]

Created attachment 147650 [details]
Patch

Add two-state enum

[WebKit Review Bot]

Comment on attachment 147650 [details]
Patch

Clearing flags on attachment: 147650

Committed r120381: <http://trac.webkit.org/changeset/120381>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.