On slow CPU hardware webprocess is not able to render enough tiles during fast scrolling and UI shows white background behind tiles. If page has dark background this flicker is very noticeable and annoying.
Created attachment 173748 [details] Match view background to page background.
Comment on attachment 173748 [details] Match view background to page background. You should clear the color inside ewk_view before painting, not add an API to LayerTreeRenderer.
(In reply to comment #2) > (From update of attachment 173748 [details]) > You should clear the color inside ewk_view before painting, not add an API to LayerTreeRenderer. Won't it will be useful to other ports? I 1st made it like you say clearing viewport before painting in EwkViewImpl, but it looked ugly with all GL ifdefs/includes and 2 branches for GL and non-GL cases.
(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 173748 [details] [details]) > > You should clear the color inside ewk_view before painting, not add an API to LayerTreeRenderer. > > Won't it will be useful to other ports? > I 1st made it like you say clearing viewport before painting in EwkViewImpl, but it looked ugly with all GL ifdefs/includes and 2 branches for GL and non-GL cases. It would be useful for other ports, but not in this way. We should pass the actual background color to LayerTreeCoordinatorProxy, not go out to the API and back in...
(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > (From update of attachment 173748 [details] [details] [details]) > > > You should clear the color inside ewk_view before painting, not add an API to LayerTreeRenderer. > > > > Won't it will be useful to other ports? > > I 1st made it like you say clearing viewport before painting in EwkViewImpl, but it looked ugly with all GL ifdefs/includes and 2 branches for GL and non-GL cases. > > It would be useful for other ports, but not in this way. We should pass the actual background color to LayerTreeCoordinatorProxy, not go out to the API and back in... Ok
Created attachment 173814 [details] Send color info directly to LayerTreeRenderer .
Comment on attachment 173814 [details] Send color info directly to LayerTreeRenderer . View in context: https://bugs.webkit.org/attachment.cgi?id=173814&action=review This will not work with semi-transparent backgrounds or with non rectangular clip, as it uses glClear and friends which completely clear the pixels in a given rectangle. I suggest that instead of this we expose backgroundColor from DrawingAreaProxy, and let the embedded (ewk_view?) decide what and how to clear. > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:96 > + , m_backgroundColor(255, 255, 255, 255) Color::white() > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:768 > + m_webPage->send(Messages::LayerTreeCoordinatorProxy::SetBackgroundColor(color)); So we do this on every layout? seems wasteful. Maybe we should make sure that the color has actually changed? > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2770 > + WebCore::Color color = WebCore::Color(float(red), float(green), float(blue), float(alpha)); Color color(red, green, blue, alpha);
(In reply to comment #7) > (From update of attachment 173814 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=173814&action=review > > This will not work with semi-transparent backgrounds or with non rectangular clip, as it uses glClear and friends which completely clear the pixels in a given rectangle. > I suggest that instead of this we expose backgroundColor from DrawingAreaProxy, and let the embedded (ewk_view?) decide what and how to clear. So, every port will have its own code for this? > > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:96 > > + , m_backgroundColor(255, 255, 255, 255) > > Color::white() Ok > > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:768 > > + m_webPage->send(Messages::LayerTreeCoordinatorProxy::SetBackgroundColor(color)); > > So we do this on every layout? seems wasteful. Maybe we should make sure that the color has actually changed? Ok > > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2770 > > + WebCore::Color color = WebCore::Color(float(red), float(green), float(blue), float(alpha)); > > Color color(red, green, blue, alpha); Doesn't work. red, green, blue and alpha are doubles. There is 2 constructors for Color - with 4 ints and 4 floats. Compiler cannot decide which constructor to use.
(In reply to comment #8) > (In reply to comment #7) > > (From update of attachment 173814 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=173814&action=review > > > > This will not work with semi-transparent backgrounds or with non rectangular clip, as it uses glClear and friends which completely clear the pixels in a given rectangle. > > I suggest that instead of this we expose backgroundColor from DrawingAreaProxy, and let the embedded (ewk_view?) decide what and how to clear. > > So, every port will have its own code for this? If you're using glClear on the entire clipRect, yes. I don't mind having shared code here, if it does the right thing - draws a solid color rect only on the contents rect of the page, and only if the page is not transparent.
(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > > (From update of attachment 173814 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=173814&action=review > > > > > > This will not work with semi-transparent backgrounds or with non rectangular clip, as it uses glClear and friends which completely clear the pixels in a given rectangle. > > > I suggest that instead of this we expose backgroundColor from DrawingAreaProxy, and let the embedded (ewk_view?) decide what and how to clear. > > > > So, every port will have its own code for this? > If you're using glClear on the entire clipRect, yes. > I don't mind having shared code here, if it does the right thing - draws a solid color rect only on the contents rect of the page, and only if the page is not transparent. No no no! It shouldn't be used if page transparency is desired. That's why it has a switch and disabled by default. IMHO, checking for page transparency would be unnecessary overhead. Probably need to add note about this to API description and to the changelog. I'll fix rect drawing without glClear.
> No no no! That's very negative :) > Probably need to add note about this to API description and to the changelog. > I'll fix rect drawing without glClear. Well, in this case it would also work with page transparency, wouldn't it? Which makes your first statement incorrect.
(In reply to comment #11) > > No no no! > That's very negative :) > > > Probably need to add note about this to API description and to the changelog. > > I'll fix rect drawing without glClear. > Well, in this case it would also work with page transparency, wouldn't it? Which makes your first statement incorrect. Do you mean, that if page is transparent then background color will be transparent also? The changelog on getDocumentBackgroundColor says: Call down into the document to get either the body's, or if there is no body, the root element's, background color. So, let's say, the root element has non-transparent background, but has opacity set on it. Should I mix in opacity?
(In reply to comment #12) > (In reply to comment #11) > > > No no no! > > That's very negative :) > > > > > Probably need to add note about this to API description and to the changelog. > > > I'll fix rect drawing without glClear. > > Well, in this case it would also work with page transparency, wouldn't it? Which makes your first statement incorrect. > > Do you mean, that if page is transparent then background color will be transparent also? > The changelog on getDocumentBackgroundColor says: > Call down into the document to get either the body's, or if there is no body, the root element's, background color. > > So, let's say, the root element has non-transparent background, but has opacity set on it. Should I mix in opacity? Yes, we should mix in the opacity that's passed with paintToCurrentGLContext.
Created attachment 173926 [details] Updated patch.
Comment on attachment 173926 [details] Updated patch. View in context: https://bugs.webkit.org/attachment.cgi?id=173926&action=review > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:432 > + FloatRect targetRect(0, 0, data().viewport[2], data().viewport[3]); > + drawQuad(targetRect, TransformationMatrix(), program.get(), GraphicsContext3D::TRIANGLE_FAN, false); Clearing the whole viewport doesn't make sense. We should expose drawSolidColor(const FloatRect&, const TransformationMatrix&, const Color&), and pass in the viewport rect from LayerTreeRenderer. > Source/WebKit2/ChangeLog:38 > + (WebKit::LayerTreeRenderer::enableViewBackgroundMatchToPage): Let's change this to setDrawsBackground
Created attachment 173936 [details] Updated patch.
Comment on attachment 173936 [details] Updated patch. View in context: https://bugs.webkit.org/attachment.cgi?id=173936&action=review > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2776 > + if (m_drawingArea && m_drawingArea->layerTreeHost()) { > + double red, green, blue, alpha; > + m_mainFrame->getDocumentBackgroundColor(&red, &green, &blue, &alpha); > + RGBA32 rgba = makeRGBA32FromFloats(red, green, blue, alpha); > + if (m_backgroundColor.rgb() != rgba) { > + m_backgroundColor.setRGB(rgba); > + m_drawingArea->layerTreeHost()->setBackgroundColor(m_backgroundColor); > + } > + } This should be guarded with ACCELERATED_COMPOSITING, or alternatively it should move to a DrawingArea[Proxy] message set rather than go via coordinated graphics. I'm ok with this in general but I'm not comfortable r+ing it because I'm not sure of the implication of calling getDocumentBackgroundColor after every layout. I'm OK with the changes to TextureMapper, if you want to post them in a separate patch.
(In reply to comment #17) > (From update of attachment 173936 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=173936&action=review > > > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2776 > > + if (m_drawingArea && m_drawingArea->layerTreeHost()) { > > + double red, green, blue, alpha; > > + m_mainFrame->getDocumentBackgroundColor(&red, &green, &blue, &alpha); > > + RGBA32 rgba = makeRGBA32FromFloats(red, green, blue, alpha); > > + if (m_backgroundColor.rgb() != rgba) { > > + m_backgroundColor.setRGB(rgba); > > + m_drawingArea->layerTreeHost()->setBackgroundColor(m_backgroundColor); > > + } > > + } > > This should be guarded with ACCELERATED_COMPOSITING, or alternatively it should move to a DrawingArea[Proxy] message set rather than go via coordinated graphics. > I'm ok with this in general but I'm not comfortable r+ing it because I'm not sure of the implication of calling getDocumentBackgroundColor after every layout. It composed of 2 color values requested from style of body and html elements and one color (baseBackgroundColor) stored in a FrameView. Getting those values from style does invoke any search or tree traverse, but just get color values stored in style itself. Those style functions are called a lot from different places. Also, it seems been called already on every layout for overlay scrollbars style calculations (FrameView::recalculateScrollbarOverlayStyle()) and from RenderLayerCompositor. IMHO, it would be good to make caching of this value at least to avoid re-calculation if there was no layout (in a separate patch?). But it has to be recalculated after every layout. > I'm OK with the changes to TextureMapper, if you want to post them in a separate patch. Ok, I'll put it to the separate patch.
Created attachment 174073 [details] Add drawSolidColor method to Texture Mapper
Created attachment 174074 [details] View background color patch. Depends on "Draw solid color" patch.
Comment on attachment 174074 [details] View background color patch. Attachment 174074 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14833165
Comment on attachment 174074 [details] View background color patch. Attachment 174074 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14837166
Comment on attachment 174074 [details] View background color patch. Attachment 174074 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14828359 New failing tests: inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html
Comment on attachment 174074 [details] View background color patch. View in context: https://bugs.webkit.org/attachment.cgi?id=174074&action=review > Source/WebKit2/ChangeLog:11 > + with new API function ewk_view_draws_page_brackground_set . Please check spelling! brackground? :) Info on why it makes sense to have it disabled by default. Could we do something smarter even by showing a low scale version of the content instead (or between real content and background color) ? say scale 0.25, that should account for max 3 tiles for that. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:925 > +void ewk_view_draws_page_brackground_set(Evas_Object *ewkView, Eina_Bool enabled) oh no! even here you spell it wrong!
Comment on attachment 174073 [details] Add drawSolidColor method to Texture Mapper View in context: https://bugs.webkit.org/attachment.cgi?id=174073&action=review > Source/WebCore/platform/graphics/texmap/TextureMapperImageBuffer.cpp:130 > + UNUSED_PARAM(matrix); maybe context->setCTM?
Comment on attachment 174074 [details] View background color patch. View in context: https://bugs.webkit.org/attachment.cgi?id=174074&action=review >> Source/WebKit2/ChangeLog:11 >> + with new API function ewk_view_draws_page_brackground_set . > > Please check spelling! brackground? :) > > Info on why it makes sense to have it disabled by default. > > Could we do something smarter even by showing a low scale version of the content instead (or between real content and background color) ? say scale 0.25, that should account for max 3 tiles for that. That would require much bigger overhead for creating and maintaining this lowres copy. Opera does something like this, but without rendering text. I don't think it looks much better. Also, webkit rendering is much faster and usually it is just small region near the border of view that flickers. In opera on the same device I was able to scroll out the whole screen and see lowres page. IMHO, doesn't look better and doesn't worth it.
Created attachment 174162 [details] Add context transformation for image buffer texture mapper.
Created attachment 174164 [details] Typo fixed. Changelog fixed. Depends on patch 174162
Comment on attachment 174164 [details] Typo fixed. Changelog fixed. Depends on patch 174162 Attachment 174164 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14828488
Comment on attachment 174164 [details] Typo fixed. Changelog fixed. Depends on patch 174162 Attachment 174164 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14823926
Comment on attachment 174162 [details] Add context transformation for image buffer texture mapper. Clearing flags on attachment: 174162 Committed r134671: <http://trac.webkit.org/changeset/134671>
Comment on attachment 174164 [details] Typo fixed. Changelog fixed. Depends on patch 174162 Patch 174162 has landed, so it should compile fine now.
Comment on attachment 174164 [details] Typo fixed. Changelog fixed. Depends on patch 174162 this doesnt seem o work on qt-wk2 nor efl
Created attachment 174456 [details] Resubmit patch to restart EWS
Comment on attachment 174456 [details] Resubmit patch to restart EWS Don't land before we know it passes EWS
Comment on attachment 174456 [details] Resubmit patch to restart EWS Clearing flags on attachment: 174456 Committed r134953: <http://trac.webkit.org/changeset/134953>
All reviewed patches have been landed. Closing bug.