[CoordinatedGraphics] Clear UpdateAtlases for each tests
Created attachment 310138 [details] Patch
I'm bit worried about to use DrawingArea inside of WKBundlePage. Do you have any suggestion where to put a code which uses drawingArea in WKBundles?
(In reply to Gwang Yoon Hwang from comment #2) > I'm bit worried about to use DrawingArea inside of WKBundlePage. Do you have > any suggestion where to put a code which uses drawingArea in WKBundles? Another possibility is to use Internals::resetToConsistentState(), which is also called in InjectedBundlePage::resetAfterTest() by WebCoreTestSupport::resetInternalsObject(). In that case you would need to add API to Page and ChromeClient instead of dealing with the C API.
Created attachment 310365 [details] Patch
(In reply to Carlos Garcia Campos from comment #3) > (In reply to Gwang Yoon Hwang from comment #2) > > I'm bit worried about to use DrawingArea inside of WKBundlePage. Do you have > > any suggestion where to put a code which uses drawingArea in WKBundles? > > Another possibility is to use Internals::resetToConsistentState(), which is > also called in InjectedBundlePage::resetAfterTest() by > WebCoreTestSupport::resetInternalsObject(). In that case you would need to > add API to Page and ChromeClient instead of dealing with the C API. I think it would be better to use WK API since we are going to dealing with Coordinated Graphics which is a part of WK2. Still, I couldn't make a decision where to put these APIs. :/
Comment on attachment 310365 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310365&action=review > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePagePrivate.h:127 > +#if USE_COORDINATED_GRAPHICS This is what's breaking EWS, should be USE(COORDINATED_GRAPHICS)
(In reply to Michael Catanzaro from comment #6) > Comment on attachment 310365 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=310365&action=review > > > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePagePrivate.h:127 > > +#if USE_COORDINATED_GRAPHICS > > This is what's breaking EWS, should be USE(COORDINATED_GRAPHICS) USE(COORDINATED_GRAPHICS) also breaks Apple's ews. https://webkit-queues.webkit.org/results/3744454 I don't know why but it is forbidden to use macro "USE"
Try including Platform.h
(In reply to Michael Catanzaro from comment #8) > Try including Platform.h No, don't include Platform.h in a C API header. You should declare the function unconditionally, and make the implementation platform-dependent, doing nothing in case of no coord graphics.
It's a private header.
Created attachment 310808 [details] Patch
(In reply to Carlos Garcia Campos from comment #9) > (In reply to Michael Catanzaro from comment #8) > > Try including Platform.h > > No, don't include Platform.h in a C API header. You should declare the > function unconditionally, and make the implementation platform-dependent, > doing nothing in case of no coord graphics. After thinking about to use C APIs for this use case, I decided to use Internals instead of C APIs as cgarcia suggested. It is prohibited to use platform specific ifdefs in C API, since they are supposed to be platform independent. (Except ios, I don't know why, again.) And AFAIK, we are going to abandon the platform independent C API idea at some point. I added the resetting apis to the Page, and ChromeClient.
Comment on attachment 310808 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310808&action=review I prefer this way, TBH. > Source/WebCore/testing/Internals.cpp:404 > + page.resetUpdateAtlasForTesting(); Can't you just call page.chrome().client().resetUpdateAtlasForTesting(); directly here? > Source/WebKit2/WebProcess/WebPage/AcceleratedDrawingArea.h:101 > +#if USE(COORDINATED_GRAPHICS) This should match the condition of DrawingArea.h > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CompositingCoordinator.cpp:456 > + if (m_releaseInactiveAtlasesTimer.isActive()) > + m_releaseInactiveAtlasesTimer.stop(); stop does nothing if not active, so no need to check. > Source/WebKit2/WebProcess/WebPage/DrawingArea.h:146 > + virtual void resetUpdateAtlasForTesting() = 0; Do we want this when to using coord graphics, but using texture mapper? I don't think so. If this configuration is no longer possible we should probably remove it, though.
Created attachment 310840 [details] Patch
Comment on attachment 310808 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310808&action=review Thanks for review. >> Source/WebCore/testing/Internals.cpp:404 >> + page.resetUpdateAtlasForTesting(); > > Can't you just call page.chrome().client().resetUpdateAtlasForTesting(); directly here? Yeah, that can remove couple of redundant interfaces in page. I modified it to call chromclient from internals directly. >> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CompositingCoordinator.cpp:456 >> + m_releaseInactiveAtlasesTimer.stop(); > > stop does nothing if not active, so no need to check. Fixed. >> Source/WebKit2/WebProcess/WebPage/DrawingArea.h:146 >> + virtual void resetUpdateAtlasForTesting() = 0; > > Do we want this when to using coord graphics, but using texture mapper? I don't think so. If this configuration is no longer possible we should probably remove it, though. I just moved this interface to the COORDINATED_GRAPHICS_THREADED and changed it to COORDINATED_GRAPHICS. UpdateAtlas is only used on WK2 with Coordinated Graphics, so it shouldn't be included in texmap only conf. And as we agreed, we are going to simplify those conditions in short term.
Comment on attachment 310840 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310840&action=review Thanks! > Source/WebKit2/WebProcess/WebPage/wpe/DrawingAreaWPE.cpp:198 > +#if USE(COORDINATED_GRAPHICS) > +void DrawingAreaWPE::resetUpdateAtlasForTesting() > +{ > + ASSERT(m_layerTreeHost); > + m_layerTreeHost->clearUpdateAtlases(); > +} > +#endif hmm, I wonder why WPE needs its own DrawingArea, I think it should be possible to use AcceleratedDrawinArea just ensuring that WPE always have AC mode forced, which I guess it's already ensured. Not related to this bug, though, just that I noticed it now.
(In reply to Carlos Garcia Campos from comment #16) > Comment on attachment 310840 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=310840&action=review > > Thanks! > > > Source/WebKit2/WebProcess/WebPage/wpe/DrawingAreaWPE.cpp:198 > > +#if USE(COORDINATED_GRAPHICS) > > +void DrawingAreaWPE::resetUpdateAtlasForTesting() > > +{ > > + ASSERT(m_layerTreeHost); > > + m_layerTreeHost->clearUpdateAtlases(); > > +} > > +#endif > > hmm, I wonder why WPE needs its own DrawingArea, I think it should be > possible to use AcceleratedDrawinArea just ensuring that WPE always have AC > mode forced, which I guess it's already ensured. Not related to this bug, > though, just that I noticed it now. I agree, AFAIU, DrawingAreaWPE forked to be simplified by removing non-accelerated compositing case. Since we have AcceleratedDrawingArea and Impl, it is simpler to use same AcceleratedDrawingArea in WPE case.
Comment on attachment 310840 [details] Patch Attachment 310840 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3793204 New failing tests: imported/w3c/web-platform-tests/media-source/mediasource-buffered.html
Created attachment 310845 [details] Archive of layout-test-results from ews116 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 310840 [details] Patch Clearing flags on attachment: 310840 Committed r217214: <http://trac.webkit.org/changeset/217214>
All reviewed patches have been landed. Closing bug.