Bug 172119 – [CoordinatedGraphics] Clear UpdateAtlases for each tests

• Component: New Bugs
• Status: RESOLVED FIXED

[Gwang Yoon Hwang]

[CoordinatedGraphics] Clear UpdateAtlases for each tests

[Gwang Yoon Hwang]

Created attachment 310138 [details]
Patch

[Gwang Yoon Hwang]

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]

(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]

Created attachment 310365 [details]
Patch

[Gwang Yoon Hwang]

(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 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]

(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]

Try including Platform.h

[Carlos Garcia Campos]

(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]

It's a private header.

[Gwang Yoon Hwang]

Created attachment 310808 [details]
Patch

[Gwang Yoon Hwang]

(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 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]

Created attachment 310840 [details]
Patch

[Gwang Yoon Hwang]

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 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]

(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 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]

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 on attachment 310840 [details]
Patch

Clearing flags on attachment: 310840

Committed r217214: <http://trac.webkit.org/changeset/217214>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.