Bug 118383

Created attachment 206077 [details] Patch

Comment on attachment 206077 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206077&action=review > Source/WebCore/ChangeLog:29 > + PageViewportController > + This class is responsible to handling requests from WebCore (eg. content > + size changes, viewport attributes changes and scroll position requests). This is a huge change. I would start by dealing with the compositor itself rather than having a copy of PVC in WebCore.

Created attachment 242811 [details] Patch

Comment on attachment 242811 [details] Patch Why is this something you want to do? On both OS X and iOS, the trend is going towards doing UI side compositing.

(In reply to comment #4) > Comment on attachment 242811 [details] > Patch > > Why is this something you want to do? On both OS X and iOS, the trend is > going towards doing UI side compositing. There are 2 main reasons. 1. Mobile GPUs are quite unstable Unlike Apple's hardware, GPU drivers for embedded linux can be easily crashed unexpectedly. So we want to keep UI Process safely from OpenGL, we can recover Web Process from UI Process when we meet unexpected crashes from GPU drivers. 2. We need compositing thread even we decided to composite contents in the UI Process. For the responsive, we should avoid calling OpenGL calls in the main-thread of UI Process. We can provide additional thread in the UI Process side for compositing even we finally decide to composite in the UI Process using this implementation.

Comment on attachment 242811 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=242811&action=review Looks good, couple small issues. > Source/WebKit2/ChangeLog:4 > + [ThreadedCompositor] Implements an initial version of the Threaded Compositor. > + https://bugs.webkit.org/show_bug.cgi?id=118383 I updated the bug title to be more descriptive, so the patch should be updated as well. > Source/WebKit2/ChangeLog:50 > + (WebKit::CompositingRunLoop::CompositingRunLoop): > + (WebKit::CompositingRunLoop::callOnCompositingRunLoop): > + (WebKit::CompositingRunLoop::setUpdateTimer): > + (WebKit::CompositingRunLoop::stopUpdateTimer): > + (WebKit::CompositingRunLoop::runLoop): > + (WebKit::CompositingRunLoop::updateTimerFired): > + (WebKit::ThreadedCompositor::create): > + (WebKit::ThreadedCompositor::ThreadedCompositor): > + (WebKit::ThreadedCompositor::~ThreadedCompositor): > + (WebKit::ThreadedCompositor::setNeedsDisplay): > + (WebKit::ThreadedCompositor::setNativeSurfaceHandleForCompositing): You can omit the blank ones and just leave the one for the file itself. > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadSafeCoordinatedSurface.cpp:35 > +#if USE(TEXTURE_MAPPER) > +#include <WebCore/TextureMapperGL.h> > +#endif Doesn't COORDINATED_GRAPHICS rely on texture mapper anyway? Perhaps this could just be moved to the main block without the #ifdef? > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadSafeCoordinatedSurface.cpp:86 > +#if USE(TEXTURE_MAPPER) Same question here as above. > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:67 > + void setUpdateTimer(double interval = 0.0f) 0.0f should just be a 0 without since that converts properly. > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:256 > + m_scene->paintToCurrentGLContext(viewportTransform, 1.0, clipRect, Color::white, false, scrollPostion); 1.0 can just be 1 here. > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ViewportController.cpp:1 > +/* So this should probably move to the parent directory, but just have a different name. Perhaps SimpleViewportController or DesktopViewportController, since it isn't really related to threaded compositing, but the fact that this is targeting a simpler type of screen. > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ViewportController.cpp:2 > + * Copyright (C) 2014 Igalia S.L. Should this retain the copyright from the original ViewportController.h? > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ViewportController.cpp:77 > + m_allowsUserScaling = !!m_rawAttributes.userScalable; I don't believe the double !! is necessary. > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ViewportController.cpp:79 > + I don't think you need these parenthesis, right? > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ViewportController.cpp:132 > + FloatRect bounds; > + bounds.setWidth(std::max(0.f, m_contentsSize.width() - floorf(m_viewportSize.width() / scale))); > + bounds.setHeight(std::max(0.f, m_contentsSize.height() - floorf(m_viewportSize.height() / scale))); This can just be FloatRect bounds(std::max(0.f, m_contentsSize.width() - floorf(m_viewportSize.width() / scale)), ...) You don't need to use .f on the floating point number. > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ViewportController.cpp:136 > + FloatPoint position; > + position.setX(clampTo(framePosition.x(), bounds.x(), bounds.width())); > + position.setY(clampTo(framePosition.y(), bounds.y(), bounds.height())); This can just be FloatPoint position(clampTo(framePosition.x(), bounds.x(), bounds.width()), clampTo(framePosition.y(), bounds.y(), bounds.height()));

(In reply to comment #6) > > Source/WebKit2/ChangeLog:4 > > + [ThreadedCompositor] Implements an initial version of the Threaded Compositor. > > + https://bugs.webkit.org/show_bug.cgi?id=118383 > > I updated the bug title to be more descriptive, so the patch should be > updated as well. Good suggestion. I'll modify the patch for that. > > Source/WebKit2/ChangeLog:50 > > + (WebKit::CompositingRunLoop::CompositingRunLoop): > > + (WebKit::CompositingRunLoop::callOnCompositingRunLoop): > > + (WebKit::CompositingRunLoop::setUpdateTimer): > > + (WebKit::CompositingRunLoop::stopUpdateTimer): > > + (WebKit::CompositingRunLoop::runLoop): > > + (WebKit::CompositingRunLoop::updateTimerFired): > > + (WebKit::ThreadedCompositor::create): > > + (WebKit::ThreadedCompositor::ThreadedCompositor): > > + (WebKit::ThreadedCompositor::~ThreadedCompositor): > > + (WebKit::ThreadedCompositor::setNeedsDisplay): > > + (WebKit::ThreadedCompositor::setNativeSurfaceHandleForCompositing): > > You can omit the blank ones and just leave the one for the file itself. Ditto. > > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadSafeCoordinatedSurface.cpp:35 > > +#if USE(TEXTURE_MAPPER) > > +#include <WebCore/TextureMapperGL.h> > > +#endif > > Doesn't COORDINATED_GRAPHICS rely on texture mapper anyway? Perhaps this > could just be moved to the main block without the #ifdef? I think it is good point also. I'll remove these from my patch set, but also we need to clean up COORDINATED_GRAPHICS, TEXTURE_MAPPER, and TILED_BACKING_STORE stuff globally. Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:67 > > + void setUpdateTimer(double interval = 0.0f) > > 0.0f should just be a 0 without since that converts properly. Good, fixed. > > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:256 > > + m_scene->paintToCurrentGLContext(viewportTransform, 1.0, clipRect, Color::white, false, scrollPostion); > > 1.0 can just be 1 here. Ditto. > > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ViewportController.cpp:1 > > +/* > > So this should probably move to the parent directory, but just have a > different name. Perhaps SimpleViewportController or > DesktopViewportController, since it isn't really related to threaded > compositing, but the fact that this is targeting a simpler type of screen. I agree about it. I prefer SimpleViewportController because it can handle fixed layout also. > > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ViewportController.cpp:2 > > + * Copyright (C) 2014 Igalia S.L. > > Should this retain the copyright from the original ViewportController.h? I omitted original PageViewportController's copyright. I'll update license for SimpleViewportController. > > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ViewportController.cpp:77 > > + m_allowsUserScaling = !!m_rawAttributes.userScalable; > > I don't believe the double !! is necessary. > Ah. old legacy code. I'll remove "!!" . > > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ViewportController.cpp:79 > > + > > I don't think you need these parenthesis, right? > Yes, I don't need it. > > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ViewportController.cpp:132 > > + FloatRect bounds; > > + bounds.setWidth(std::max(0.f, m_contentsSize.width() - floorf(m_viewportSize.width() / scale))); > > + bounds.setHeight(std::max(0.f, m_contentsSize.height() - floorf(m_viewportSize.height() / scale))); > > This can just be FloatRect bounds(std::max(0.f, m_contentsSize.width() - > floorf(m_viewportSize.width() / scale)), ...) > > You don't need to use .f on the floating point number. > If we do not use .f in this place, deduction failure will be happen at std::max. It will try std::max(int, float) > > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ViewportController.cpp:136 > > + FloatPoint position; > > + position.setX(clampTo(framePosition.x(), bounds.x(), bounds.width())); > > + position.setY(clampTo(framePosition.y(), bounds.y(), bounds.height())); > > This can just be FloatPoint position(clampTo(framePosition.x(), bounds.x(), > bounds.width()), clampTo(framePosition.y(), bounds.y(), bounds.height())); I think merging these in to the single statement reduces readability. I'll make more simplified statement for these.

(In reply to comment #7) > > > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ViewportController.cpp:136 > > > + FloatPoint position; > > > + position.setX(clampTo(framePosition.x(), bounds.x(), bounds.width())); > > > + position.setY(clampTo(framePosition.y(), bounds.y(), bounds.height())); > > > > This can just be FloatPoint position(clampTo(framePosition.x(), bounds.x(), > > bounds.width()), clampTo(framePosition.y(), bounds.y(), bounds.height())); > > I think merging these in to the single statement reduces readability. I'll > make more simplified statement for these. One option for improved readability is to split it over multiple lines like this: FloatPoint position( clampTo(framePosition.x(), bounds.x(), bounds.width()), clampTo(framePosition.y(), bounds.y(), bounds.height())); That way you avoid the slight overhead of method calls.

Created attachment 242915 [details] Patch

(In reply to comment #7) > > > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ViewportController.cpp:132 > > > + FloatRect bounds; > > > + bounds.setWidth(std::max(0.f, m_contentsSize.width() - floorf(m_viewportSize.width() / scale))); > > > + bounds.setHeight(std::max(0.f, m_contentsSize.height() - floorf(m_viewportSize.height() / scale))); > > > > This can just be FloatRect bounds(std::max(0.f, m_contentsSize.width() - > > floorf(m_viewportSize.width() / scale)), ...) > > > > You don't need to use .f on the floating point number. > > > > If we do not use .f in this place, deduction failure will be happen at > std::max. > It will try std::max(int, float) > Not really, std::max is templated so you could do something like std::max<float>().

(In reply to comment #10) > (In reply to comment #7) > > > > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ViewportController.cpp:132 > > > > + FloatRect bounds; > > > > + bounds.setWidth(std::max(0.f, m_contentsSize.width() - floorf(m_viewportSize.width() / scale))); > > > > + bounds.setHeight(std::max(0.f, m_contentsSize.height() - floorf(m_viewportSize.height() / scale))); > > > > > > This can just be FloatRect bounds(std::max(0.f, m_contentsSize.width() - > > > floorf(m_viewportSize.width() / scale)), ...) > > > > > > You don't need to use .f on the floating point number. > > > > > > > If we do not use .f in this place, deduction failure will be happen at > > std::max. > > It will try std::max(int, float) > > > > Not really, std::max is templated so you could do something like > std::max<float>(). That's why I said the deduction failure. std::max is templated somewhat like below: template< class T > const T& max( const T& a, const T& b ); So the compiler cannot decide whether the T is int or float in this case.

Comment on attachment 242915 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=242915&action=review > Source/WebKit2/Shared/CoordinatedGraphics/SimpleViewportController.cpp:125 > + return FloatSize(m_viewportSize.width() / m_pageScaleFactor, m_viewportSize.height() / m_pageScaleFactor); Could you do FloatSize(m_viewportSize).scale(1.0 / m_pageScaleFactor); here?

Comment on attachment 242915 [details] Patch Clearing flags on attachment: 242915 Committed r177276: <http://trac.webkit.org/changeset/177276>

All reviewed patches have been landed. Closing bug.