Summary: | [Fullscreen] Page sometimes ends up with an incorrect zoom level after entering fullscreen | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jer Noble <jer.noble> | ||||||||
Component: | New Bugs | Assignee: | 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
Jer Noble
2018-06-19 14:52:40 PDT
Created attachment 343106 [details]
Patch
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 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 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
(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. Created attachment 343244 [details]
Patch
> > > 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?
(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 on attachment 343244 [details] Patch Clearing flags on attachment: 343244 Committed r233059: <https://trac.webkit.org/changeset/233059> All reviewed patches have been landed. Closing bug. |