Bug 186684 – Address fullscreen api CSS env feedback

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

Address fullscreen api CSS env feedback

[Radar WebKit Bug Importer]

<rdar://problem/41165374>

[Jer Noble]

Created attachment 342830 [details]
Patch

[Igalia-pontevedra EWS]

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

[Igalia-pontevedra EWS]

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

[Michael Catanzaro]

(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

[Jer Noble]

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

[Carlos Alberto Lopez Perez]

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

[Jer Noble]

Created attachment 343102 [details]
Patch

[Jer Noble]

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

[Jer Noble]

Created attachment 343113 [details]
Patch

[Simon Fraser (smfr)]

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

[Jer Noble]

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.

[Jer Noble]

Created attachment 343243 [details]
Patch for landing

[Jer Noble]

Created attachment 343254 [details]
Patch for landing

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Jer Noble]

Created attachment 343264 [details]
Patch for landing

[WebKit Commit Bot]

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

[Jer Noble]

Created attachment 343270 [details]
Patch for landing

[WebKit Commit Bot]

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

[WebKit Commit Bot]

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

[Jer Noble]

Committed r233066: <https://trac.webkit.org/changeset/233066>