WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(12.23 KB, patch)
2017-05-17 02:49 PDT
,
Gwang Yoon Hwang
no flags
Details
Formatted Diff
Diff
Patch
(14.41 KB, patch)
2017-05-21 09:36 PDT
,
Gwang Yoon Hwang
no flags
Details
Formatted Diff
Diff
Patch
(13.29 KB, patch)
2017-05-22 01:46 PDT
,
Gwang Yoon Hwang
no flags
Details
Formatted Diff
Diff
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
Details
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Gwang Yoon Hwang
Comment 1
2017-05-15 08:07:19 PDT
Created
attachment 310138
[details]
Patch
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
Created
attachment 310365
[details]
Patch
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
Created
attachment 310808
[details]
Patch
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
Created
attachment 310840
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug