Address fullscreen api CSS env feedback
<rdar://problem/41165374>
Created attachment 342830 [details] Patch
Comment on attachment 342830 [details] Patch Attachment 342830 [details] did not pass gtk-wk2-ews (gtk-wk2): Output: http://webkit-queues.webkit.org/results/8234086 New failing tests: fullscreen/fullscreen-env.html
Created attachment 342956 [details] Archive of layout-test-results from ltilve-gtk-wk2-ews for gtk-wk2 The attached test failures were seen while running run-webkit-tests on the gtk-wk2-ews. Bot: ltilve-gtk-wk2-ews Port: gtk-wk2 Platform: Linux-4.16.0-0.bpo.1-amd64-x86_64-with-debian-9.4
(In reply to Igalia-pontevedra EWS from comment #3) > Comment on attachment 342830 [details] > Patch > > Attachment 342830 [details] did not pass gtk-wk2-ews (gtk-wk2): > Output: http://webkit-queues.webkit.org/results/8234086 > > New failing tests: > fullscreen/fullscreen-env.html I think this is our first EWS test failure that's not a false positive! The diff is: --- /home/ews/ltilve-gtk-wk2-ews/WebKit/WebKitBuild/Release/layout-test-results/fullscreen/fullscreen-env-expected.txt +++ /home/ews/ltilve-gtk-wk2-ews/WebKit/WebKitBuild/Release/layout-test-results/fullscreen/fullscreen-env-actual.txt @@ -28,8 +28,8 @@ EXPECTED (window.getComputedStyle(target).transitionDuration == '0s') OK EVENT(webkitfullscreenchange) RUN(internals.setFullscreenControlsHidden(true)) -EXPECTED (document.querySelector("#target:-webkit-full-screen-controls-hidden") == '[object HTMLDivElement]') OK +SyntaxError: The string did not match the expected pattern. RUN(internals.setFullscreenControlsHidden(false)) -EXPECTED (document.querySelector("#target:-webkit-full-screen-controls-hidden") == 'null') OK +SyntaxError: The string did not match the expected pattern. END OF TEST
(In reply to Michael Catanzaro from comment #5) > (In reply to Igalia-pontevedra EWS from comment #3) > > Comment on attachment 342830 [details] > > Patch > > > > Attachment 342830 [details] did not pass gtk-wk2-ews (gtk-wk2): > > Output: http://webkit-queues.webkit.org/results/8234086 > > > > New failing tests: > > fullscreen/fullscreen-env.html > > I think this is our first EWS test failure that's not a false positive! > > The diff is: > > --- > /home/ews/ltilve-gtk-wk2-ews/WebKit/WebKitBuild/Release/layout-test-results/ > fullscreen/fullscreen-env-expected.txt > +++ > /home/ews/ltilve-gtk-wk2-ews/WebKit/WebKitBuild/Release/layout-test-results/ > fullscreen/fullscreen-env-actual.txt > @@ -28,8 +28,8 @@ > EXPECTED (window.getComputedStyle(target).transitionDuration == '0s') OK > EVENT(webkitfullscreenchange) > RUN(internals.setFullscreenControlsHidden(true)) > -EXPECTED > (document.querySelector("#target:-webkit-full-screen-controls-hidden") == > '[object HTMLDivElement]') OK > +SyntaxError: The string did not match the expected pattern. > RUN(internals.setFullscreenControlsHidden(false)) > -EXPECTED > (document.querySelector("#target:-webkit-full-screen-controls-hidden") == > 'null') OK > +SyntaxError: The string did not match the expected pattern. > END OF TEST Does this bot have the FULLSCREEN_API_ENABLED flag set?
(In reply to Jer Noble from comment #6) > (In reply to Michael Catanzaro from comment #5) > > (In reply to Igalia-pontevedra EWS from comment #3) > > > Comment on attachment 342830 [details] > > > Patch > > > > > > Attachment 342830 [details] did not pass gtk-wk2-ews (gtk-wk2): > > > Output: http://webkit-queues.webkit.org/results/8234086 > > > > > > New failing tests: > > > fullscreen/fullscreen-env.html > > > > I think this is our first EWS test failure that's not a false positive! > > > > The diff is: > > > > --- > > /home/ews/ltilve-gtk-wk2-ews/WebKit/WebKitBuild/Release/layout-test-results/ > > fullscreen/fullscreen-env-expected.txt > > +++ > > /home/ews/ltilve-gtk-wk2-ews/WebKit/WebKitBuild/Release/layout-test-results/ > > fullscreen/fullscreen-env-actual.txt > > @@ -28,8 +28,8 @@ > > EXPECTED (window.getComputedStyle(target).transitionDuration == '0s') OK > > EVENT(webkitfullscreenchange) > > RUN(internals.setFullscreenControlsHidden(true)) > > -EXPECTED > > (document.querySelector("#target:-webkit-full-screen-controls-hidden") == > > '[object HTMLDivElement]') OK > > +SyntaxError: The string did not match the expected pattern. > > RUN(internals.setFullscreenControlsHidden(false)) > > -EXPECTED > > (document.querySelector("#target:-webkit-full-screen-controls-hidden") == > > 'null') OK > > +SyntaxError: The string did not match the expected pattern. > > END OF TEST > > Does this bot have the FULLSCREEN_API_ENABLED flag set? Yes. It builds with ENABLE_FULLSCREEN_API enabled.
Created attachment 343102 [details] Patch
(In reply to Jer Noble from comment #8) > Created attachment 343102 [details] > Patch I haven't yet figured out the gtk+ test error, but this patch should take care of the other bots' build errors.
Created attachment 343113 [details] Patch
Comment on attachment 343113 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=343113&action=review > Source/WebCore/dom/ConstantPropertyMap.h:63 > void setFullscreenAutoHideDelay(double); > + void setFullscreenAutoHideDuration(double); These should take Seconds > Source/WebCore/page/Page.cpp:2408 > + for (Frame* frame = &mainFrame(); frame; frame = frame->tree().traverseNext()) { > + if (!frame->document()) > + continue; > + frame->document()->constantProperties().setFullscreenAutoHideDuration(duration); > + } This is weird; why don't we have a singleton repository of constant properties? (not for this patch tho). > Source/WebCore/page/Page.h:356 > WEBCORE_EXPORT void setFullscreenAutoHideDelay(double); Seconds > Source/WebCore/page/Page.h:358 > + WEBCORE_EXPORT void setFullscreenAutoHideDuration(double); > + WEBCORE_EXPORT void setFullscreenControlsHidden(bool); Seconds > Source/WebCore/testing/Internals.cpp:503 > + page.setFullscreenAutoHideDuration(0); This would be 0_s if you use Seconds. > Source/WebCore/testing/Internals.cpp:504 > + page.setFullscreenInsets(FloatBoxExtent()); Can this be page.setFullscreenInsets({ }); > Source/WebKit/UIProcess/WebFullScreenManagerProxy.cpp:141 > +void WebFullScreenManagerProxy::setFullscreenAutoHideDuration(double duration) > +{ > + m_page->process().send(Messages::WebFullScreenManager::SetFullscreenAutoHideDuration(duration), m_page->pageID()); > +} > + > +void WebFullScreenManagerProxy::setFullscreenControlsHidden(bool hidden) > +{ > + m_page->process().send(Messages::WebFullScreenManager::SetFullscreenControlsHidden(hidden), m_page->pageID()); > +} Do we really need to send a separate IPC message for each of these? Can we pass them en masse somewhere? > Source/WebKit/UIProcess/WebFullScreenManagerProxy.h:77 > void setFullscreenAutoHideDelay(double); > + void setFullscreenAutoHideDuration(double); Seconds > Source/WebKit/WebProcess/FullScreen/WebFullScreenManager.cpp:184 > + setFullscreenInsets(FloatBoxExtent()); { } > Source/WebKit/WebProcess/FullScreen/WebFullScreenManager.h:86 > void setFullscreenAutoHideDelay(double); > + void setFullscreenAutoHideDuration(double); > + void setFullscreenControlsHidden(bool); Seconds
Comment on attachment 343113 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=343113&action=review >> Source/WebCore/dom/ConstantPropertyMap.h:63 >> + void setFullscreenAutoHideDuration(double); > > These should take Seconds Changed. >> Source/WebCore/page/Page.h:356 >> WEBCORE_EXPORT void setFullscreenAutoHideDelay(double); > > Seconds Changed. >> Source/WebCore/page/Page.h:358 >> + WEBCORE_EXPORT void setFullscreenControlsHidden(bool); > > Seconds Changed. >> Source/WebCore/testing/Internals.cpp:503 >> + page.setFullscreenAutoHideDuration(0); > > This would be 0_s if you use Seconds. Changed. >> Source/WebCore/testing/Internals.cpp:504 >> + page.setFullscreenInsets(FloatBoxExtent()); > > Can this be page.setFullscreenInsets({ }); Appears so. >> Source/WebKit/UIProcess/WebFullScreenManagerProxy.cpp:141 >> +} > > Do we really need to send a separate IPC message for each of these? Can we pass them en masse somewhere? Renamed to "SetFullscreenAutoHideTiming(delay, duration)". >> Source/WebKit/UIProcess/WebFullScreenManagerProxy.h:77 >> + void setFullscreenAutoHideDuration(double); > > Seconds Changed. >> Source/WebKit/WebProcess/FullScreen/WebFullScreenManager.cpp:184 >> + setFullscreenInsets(FloatBoxExtent()); > > { } Changed. >> Source/WebKit/WebProcess/FullScreen/WebFullScreenManager.h:86 >> + void setFullscreenControlsHidden(bool); > > Seconds Changed.
Created attachment 343243 [details] Patch for landing
Created attachment 343254 [details] Patch for landing
Comment on attachment 343254 [details] Patch for landing Attachment 343254 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/8279977 New failing tests: fullscreen/fullscreen-inset-top.html
Created attachment 343261 [details] Archive of layout-test-results from ews102 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 343254 [details] Patch for landing Attachment 343254 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/8279957 New failing tests: fullscreen/fullscreen-inset-top.html
Created attachment 343263 [details] Archive of layout-test-results from ews107 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Created attachment 343264 [details] Patch for landing
Comment on attachment 343264 [details] Patch for landing Rejecting attachment 343264 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 343264, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in LayoutTests/ChangeLog contains OOPS!. Full output: https://webkit-queues.webkit.org/results/8280566
Created attachment 343270 [details] Patch for landing
Comment on attachment 343270 [details] Patch for landing Rejecting attachment 343270 [details] from commit-queue. New failing tests: performance-api/performance-observer-no-document-leak.html Full output: https://webkit-queues.webkit.org/results/8281742
Created attachment 343281 [details] Archive of layout-test-results from webkit-cq-03 for mac-sierra The attached test failures were seen while running run-webkit-tests on the commit-queue. Bot: webkit-cq-03 Port: mac-sierra Platform: Mac OS X 10.12.6
Committed r233066: <https://trac.webkit.org/changeset/233066>