Bug 186822

Summary: [Fullscreen] Page sometimes ends up with an incorrect zoom level after entering fullscreen
Product: WebKit Reporter: Jer Noble <jer.noble>
Component: New BugsAssignee: Jer Noble <jer.noble>
Status: RESOLVED FIXED    
Severity: Normal CC: bugs-noreply, calvaris, cdumez, commit-queue, dbates, esprehn+autocc, ews-bot+gtk-1, ews-watchlist, kangil.han, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews206 for win-future
none
Patch none

Description Jer Noble 2018-06-19 14:52:40 PDT
[Fullscreen] Page sometimes ends up with an incorrect zoom level after entering fullscreen
Comment 1 Jer Noble 2018-06-19 15:03:35 PDT
Created attachment 343106 [details]
Patch
Comment 2 Simon Fraser (smfr) 2018-06-20 17:28:47 PDT
Comment on attachment 343106 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=343106&action=review

> Source/WebCore/dom/Document.h:395
> +    WEBCORE_EXPORT void setShouldOverrideViewportArguments(const std::optional<ViewportArguments>&);

A "set should override" method would take a bool, not an optional arguments. This should be setOverrideViewportArguments.

> Source/WebKit/UIProcess/WebPageProxy.h:563
> +    void setShouldOverrideViewportArguments(const std::optional<WebCore::ViewportArguments>&);

This name is also wrong.

> Source/WebKit/WebProcess/WebPage/WebPage.h:894
> +    void setShouldOverrideViewportArguments(const std::optional<WebCore::ViewportArguments>&);

Naming.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2485
> +void WebPage::setShouldOverrideViewportArguments(const std::optional<WebCore::ViewportArguments>& arguments)
> +{
> +    if (auto* document = m_page->mainFrame().document())
> +        document->setShouldOverrideViewportArguments(arguments);
> +}

This is scary. What happens if frames come and go while you're in fullscreen?
Comment 3 EWS Watchlist 2018-06-21 00:15:11 PDT
Comment on attachment 343106 [details]
Patch

Attachment 343106 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8274044

New failing tests:
http/tests/security/canvas-remote-read-remote-video-localhost.html
Comment 4 EWS Watchlist 2018-06-21 00:15:23 PDT
Created attachment 343219 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 5 Jer Noble 2018-06-21 10:37:57 PDT
(In reply to Simon Fraser (smfr) from comment #2)
> Comment on attachment 343106 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=343106&action=review
> 
> > Source/WebCore/dom/Document.h:395
> > +    WEBCORE_EXPORT void setShouldOverrideViewportArguments(const std::optional<ViewportArguments>&);
> 
> A "set should override" method would take a bool, not an optional arguments.
> This should be setOverrideViewportArguments.

Changed, here and elsewhere.

> > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2485
> > +void WebPage::setShouldOverrideViewportArguments(const std::optional<WebCore::ViewportArguments>& arguments)
> > +{
> > +    if (auto* document = m_page->mainFrame().document())
> > +        document->setShouldOverrideViewportArguments(arguments);
> > +}
> 
> This is scary. What happens if frames come and go while you're in fullscreen?

Frames can't come and go in fullscreen; any navigation causes fullscreen to exit.
Comment 6 Jer Noble 2018-06-21 10:40:23 PDT
Created attachment 343244 [details]
Patch
Comment 7 Simon Fraser (smfr) 2018-06-21 11:17:19 PDT
> > > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2485
> > > +void WebPage::setShouldOverrideViewportArguments(const std::optional<WebCore::ViewportArguments>& arguments)
> > > +{
> > > +    if (auto* document = m_page->mainFrame().document())
> > > +        document->setShouldOverrideViewportArguments(arguments);
> > > +}
> > 
> > This is scary. What happens if frames come and go while you're in fullscreen?
> 
> Frames can't come and go in fullscreen; any navigation causes fullscreen to
> exit.

Really? The document can add a frame outside of the fullscreen ancestor chain, right?
Comment 8 Jer Noble 2018-06-21 11:57:36 PDT
(In reply to Simon Fraser (smfr) from comment #7)
> > > > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2485
> > > > +void WebPage::setShouldOverrideViewportArguments(const std::optional<WebCore::ViewportArguments>& arguments)
> > > > +{
> > > > +    if (auto* document = m_page->mainFrame().document())
> > > > +        document->setShouldOverrideViewportArguments(arguments);
> > > > +}
> > > 
> > > This is scary. What happens if frames come and go while you're in fullscreen?
> > 
> > Frames can't come and go in fullscreen; any navigation causes fullscreen to
> > exit.
> 
> Really? The document can add a frame outside of the fullscreen ancestor
> chain, right?

Yes, but it's only the main frame that matters.  And any navigation at any level will cause fullscreen to exit.
Comment 9 WebKit Commit Bot 2018-06-21 14:23:19 PDT
Comment on attachment 343244 [details]
Patch

Clearing flags on attachment: 343244

Committed r233059: <https://trac.webkit.org/changeset/233059>
Comment 10 WebKit Commit Bot 2018-06-21 14:23:21 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2018-06-21 14:25:12 PDT
<rdar://problem/41343831>