RESOLVED FIXED 172119
[CoordinatedGraphics] Clear UpdateAtlases for each tests
https://bugs.webkit.org/show_bug.cgi?id=172119
Summary [CoordinatedGraphics] Clear UpdateAtlases for each tests
Gwang Yoon Hwang
Reported 2017-05-15 08:06:23 PDT
[CoordinatedGraphics] Clear UpdateAtlases for each tests
Attachments
Patch (12.29 KB, patch)
2017-05-15 08:07 PDT, Gwang Yoon Hwang
no flags
Patch (12.23 KB, patch)
2017-05-17 02:49 PDT, Gwang Yoon Hwang
no flags
Patch (14.41 KB, patch)
2017-05-21 09:36 PDT, Gwang Yoon Hwang
no flags
Patch (13.29 KB, patch)
2017-05-22 01:46 PDT, Gwang Yoon Hwang
no flags
Archive of layout-test-results from ews116 for mac-elcapitan (1.65 MB, application/zip)
2017-05-22 03:04 PDT, Build Bot
no flags
Gwang Yoon Hwang
Comment 1 2017-05-15 08:07:19 PDT
Gwang Yoon Hwang
Comment 2 2017-05-15 08:09:39 PDT
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?
Carlos Garcia Campos
Comment 3 2017-05-16 00:36:43 PDT
(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.
Gwang Yoon Hwang
Comment 4 2017-05-17 02:49:33 PDT
Gwang Yoon Hwang
Comment 5 2017-05-17 02:53:42 PDT
(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. :/
Michael Catanzaro
Comment 6 2017-05-17 05:47:25 PDT
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)
Gwang Yoon Hwang
Comment 7 2017-05-17 07:23:41 PDT
(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"
Michael Catanzaro
Comment 8 2017-05-17 07:46:50 PDT
Try including Platform.h
Carlos Garcia Campos
Comment 9 2017-05-17 23:46:31 PDT
(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.
Michael Catanzaro
Comment 10 2017-05-18 08:49:45 PDT
It's a private header.
Gwang Yoon Hwang
Comment 11 2017-05-21 09:36:03 PDT
Gwang Yoon Hwang
Comment 12 2017-05-21 09:40:46 PDT
(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.
Carlos Garcia Campos
Comment 13 2017-05-21 23:52:49 PDT
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.
Gwang Yoon Hwang
Comment 14 2017-05-22 01:46:35 PDT
Gwang Yoon Hwang
Comment 15 2017-05-22 01:52:11 PDT
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.
Carlos Garcia Campos
Comment 16 2017-05-22 02:02:55 PDT
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.
Gwang Yoon Hwang
Comment 17 2017-05-22 02:53:56 PDT
(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.
Build Bot
Comment 18 2017-05-22 03:04:03 PDT
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
Build Bot
Comment 19 2017-05-22 03:04:05 PDT
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
WebKit Commit Bot
Comment 20 2017-05-22 03:54:06 PDT
Comment on attachment 310840 [details] Patch Clearing flags on attachment: 310840 Committed r217214: <http://trac.webkit.org/changeset/217214>
WebKit Commit Bot
Comment 21 2017-05-22 03:54:08 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.