Implement the attributes defined by the VisualViewport API: https://wicg.github.io/visual-viewport/#the-visualviewport-interface
Created attachment 326250 [details] Patch
Comment on attachment 326250 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=326250&action=review > Source/WebCore/page/VisualViewport.idl:28 > +EnabledBySetting=VisualViewport Should this be using a new setting, or is it ok to use this existing VisualViewport setting?
I think it should be a new Experimental Feature (VisualViewportAPI). You should update features.json so https://webkit.org/status/#?search=viewport says "Under development".
Comment on attachment 326250 [details] Patch Attachment 326250 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/5143035 New failing tests: imported/w3c/web-platform-tests/service-workers/service-worker/service-worker-csp-default.https.html
Created attachment 326302 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Created attachment 326342 [details] Patch
Comment on attachment 326342 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=326342&action=review Thanks for working on this Ali! This looks good to me. I have a few comments inline. > Source/WebCore/page/DOMWindow.cpp:811 > +{ Should we return null when !isCurrentlyDisplayedInFrame()? That's what other APIs do... If so, it should be mentioned in the spec and we should have a WPT test for that case. > Source/WebCore/page/DOMWindow.idl:137 > + [EnabledBySetting=VisualViewportAPI, Replaceable] readonly attribute VisualViewport visualViewport; // FIXME: Should be [SameObject]. In general, we open follow-up bugs and mention them in FIXME comments. > Source/WebCore/page/VisualViewport.cpp:135 > + It looks like you can introduce a helper function that returns a FrameView* or an optional<FrameView> and performs the updateLayoutIgnorePendingStylesheets if the result is non-null. Maybe it can even ASSERT that m_frame->pageZoomFactor() is non-null. Then you can use it in the functions offsetTop, offsetLeft, pageTop, pageLeft, width and height above, and avoid some code duplication. > Source/WebCore/page/VisualViewport.cpp:142 > + return 1; Can you please explain why we check the isMainFrame() condition? It seems page() is also available for non-main frame and the special case is not obvious to me from https://wicg.github.io/visual-viewport/#dom-visualviewport-scale. Maybe add a comment? And a test? > Source/WebCore/page/VisualViewport.cpp:147 > + Not sure we will re-use the above code in the future, but similarly you might consider introducing a helper function to get the Page* / optional<Page> and do the updateLayoutIgnorePendingStylesheets call. > LayoutTests/ChangeLog:21 > + * platform/gtk/TestExpectations: Please mention that you skip the GTK tests because of the lack of UIScriptController::zoomToScale. We should really have a bug entry for that and have it mentioned here and in TestExpectations... > LayoutTests/ChangeLog:25 > + * platform/ios/imported/w3c/web-platform-tests/viewport/viewport-unscaled-size-iframe-expected.txt: Added. I suppose it's on purpose that not as many tests pass on iOS as in macOS. Again, this need to be explained in the ChangeLog and we should have follow-up bug to handle that. > LayoutTests/ChangeLog:26 > + * platform/mac-wk1/TestExpectations: I guess you should repeat the explanation for Mac WK1 here too. > LayoutTests/imported/w3c/ChangeLog:8 > + Update expectations for viewport WPTs that now pass. Again, this does not seem to true for iOS as you added expected failures. (Hopefully, since they don't use UIScriptController::zoomToScale they will pass on GTK too!)
Created attachment 327105 [details] Patch Addressed review comments.
Comment on attachment 326342 [details] Patch Thanks for the detailed review! View in context: https://bugs.webkit.org/attachment.cgi?id=326342&action=review >> Source/WebCore/page/DOMWindow.cpp:811 >> +{ > > Should we return null when !isCurrentlyDisplayedInFrame()? That's what other APIs do... > > If so, it should be mentioned in the spec and we should have a WPT test for that case. Thanks for pointing this out! I opened a spec issue to clarify this (https://github.com/WICG/visual-viewport/issues/51) and it sounds like this is going to get added to the spec. I'll make the change here after the spec gets updated. >> Source/WebCore/page/DOMWindow.idl:137 >> + [EnabledBySetting=VisualViewportAPI, Replaceable] readonly attribute VisualViewport visualViewport; // FIXME: Should be [SameObject]. > > In general, we open follow-up bugs and mention them in FIXME comments. Added a link to the bug for implementing SameObject. >> Source/WebCore/page/VisualViewport.cpp:135 >> + > > It looks like you can introduce a helper function that returns a FrameView* or an optional<FrameView> and performs the updateLayoutIgnorePendingStylesheets if the result is non-null. Maybe it can even ASSERT that m_frame->pageZoomFactor() is non-null. Then you can use it in the functions offsetTop, offsetLeft, pageTop, pageLeft, width and height above, and avoid some code duplication. Done. >> Source/WebCore/page/VisualViewport.cpp:142 >> + return 1; > > Can you please explain why we check the isMainFrame() condition? It seems page() is also available for non-main frame and the special case is not obvious to me from https://wicg.github.io/visual-viewport/#dom-visualviewport-scale. Maybe add a comment? And a test? This is covered by fast/visual-viewport/viewport-dimensions-iframe.html as well as by a manual WPT (viewport-scale-iframe-manual.html). But I agree that this requirement isn't clear from the spec language, so I've opened a spec issue (https://github.com/WICG/visual-viewport/issues/52) to get the spec language clarified. >> Source/WebCore/page/VisualViewport.cpp:147 >> + > > Not sure we will re-use the above code in the future, but similarly you might consider introducing a helper function to get the Page* / optional<Page> and do the updateLayoutIgnorePendingStylesheets call. Left this as-is for now, I think makes sense to add a helper once we need to reuse this code. >> LayoutTests/ChangeLog:21 >> + * platform/gtk/TestExpectations: > > Please mention that you skip the GTK tests because of the lack of UIScriptController::zoomToScale. We should really have a bug entry for that and have it mentioned here and in TestExpectations... Added comment mentioning the existing bug. The bug number (webkit.org/b/168050) is already mentioned in TestExpectations :) >> LayoutTests/ChangeLog:25 >> + * platform/ios/imported/w3c/web-platform-tests/viewport/viewport-unscaled-size-iframe-expected.txt: Added. > > I suppose it's on purpose that not as many tests pass on iOS as in macOS. Again, this need to be explained in the ChangeLog and we should have follow-up bug to handle that. Added comments in the ChangeLog. The three iframe-related tests fail because iframes aren't scrollable on iOS -- filed a bug for these. The other test (viewport-unscaled-size) has a PASS expectation, but the numbers in the output are different just because of the different window size, so that doesn't need any follow-up. >> LayoutTests/ChangeLog:26 >> + * platform/mac-wk1/TestExpectations: > > I guess you should repeat the explanation for Mac WK1 here too. Done. >> LayoutTests/imported/w3c/ChangeLog:8 >> + Update expectations for viewport WPTs that now pass. > > Again, this does not seem to true for iOS as you added expected failures. (Hopefully, since they don't use UIScriptController::zoomToScale they will pass on GTK too!) Updated the comment, and added comments below about failures.
Comment on attachment 327105 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=327105&action=review Thanks for addressing the review comments. This basically LGTM, but I prefer to wait a bit the feedback from other more experienced reviewers as well as the changes in the spec wording. I'm happy to r+ the changes to features.json now though. I have some minor comments inline. > Source/WebCore/features.json:1171 > }, If you want this part right now so the status is displayed on https://webkit.org/status/#?search=viewport ; you can submit a separate patch and I'll r+ it. > Source/WebCore/page/DOMWindow.cpp:806 > +{ OK, I think you need to add the isCurrentlyDisplayedInFrame check per https://github.com/WICG/visual-viewport/issues/51#issuecomment-344990083 > Source/WebCore/page/VisualViewport.cpp:57 > + auto* view = frame? frame->view() : nullptr; nit: miss a space before the question mark > Source/WebCore/page/VisualViewport.cpp:69 > + return 0.0; nit: I would personally do if (auto* view = getFrameViewAndLayoutIfNonNull(m_frame)) return (...); return 0.0; to be slightly more concise. > Source/WebCore/page/VisualViewport.cpp:121 > + if (!m_frame || !m_frame->isMainFrame()) Let's wait the reply on https://github.com/WICG/visual-viewport/issues/52 and add a comment if necessary. > Tools/ChangeLog:11 > + (enableExperimentalFeatures): It is weird that this is only done for mac and only DumpRenderTree. I think there are other places to modify, for example InjectedBundle::beginTesting in Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp ; maybe you can compare with what was done for other similar experimental features. > LayoutTests/ChangeLog:24 > + Fails because iframes aren't scrollable on iOS (webkit.org/b/179794). OK, I heard about that bug ;-) > LayoutTests/fast/visual-viewport/viewport-dimensions-exclude-custom-scrollbars.html:29 > + internals.settings.setVisualViewportEnabled(true); Is it necessary to explicitly set this? IIUC you don't do that for WPT tests. Again maybe check what is done for other prefs...
Created attachment 327202 [details] Patch Addressed comments.
Comment on attachment 327105 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=327105&action=review >> Source/WebCore/features.json:1171 >> }, > > If you want this part right now so the status is displayed on https://webkit.org/status/#?search=viewport ; you can submit a separate patch and I'll r+ it. Thanks, done in bug 179818. >> Source/WebCore/page/DOMWindow.cpp:806 >> +{ > > OK, I think you need to add the isCurrentlyDisplayedInFrame check per https://github.com/WICG/visual-viewport/issues/51#issuecomment-344990083 Done. >> Source/WebCore/page/VisualViewport.cpp:57 >> + auto* view = frame? frame->view() : nullptr; > > nit: miss a space before the question mark Oops, fixed. >> Source/WebCore/page/VisualViewport.cpp:69 >> + return 0.0; > > nit: I would personally do > > if (auto* view = getFrameViewAndLayoutIfNonNull(m_frame)) > return (...); > return 0.0; > > to be slightly more concise. Done. >> Tools/ChangeLog:11 >> + (enableExperimentalFeatures): > > It is weird that this is only done for mac and only DumpRenderTree. I think there are other places to modify, for example InjectedBundle::beginTesting in Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp ; maybe you can compare with what was done for other similar experimental features. I compared to what's done for LinkPreload and WebAnimations. Neither of those is set in InjectedBundle::beginTesting. I think this isn't needed since for WebKitTestRunner, TestController::resetPreferencesToConsistentValues calls WKPreferencesEnableAllExperimentalFeatures which in turn calls WebPreferences::enableAllExperimentalFeatures (defined in the generated file DerivedSources/WebKit2/WebPreferencesExperimentalFeatures.cpp). But I'd missed updating DumpRenderTree.cpp for Windows; done now. >> LayoutTests/fast/visual-viewport/viewport-dimensions-exclude-custom-scrollbars.html:29 >> + internals.settings.setVisualViewportEnabled(true); > > Is it necessary to explicitly set this? IIUC you don't do that for WPT tests. Again maybe check what is done for other prefs... Note that this isn't setting the new experimental VisualViewportAPI pref; instead, it's setting the existing pref for enabling visual viewports. Visual viewports are already enabled by default on WK2 and on mac-WK1, so this only makes a difference on non-mac WK1 ports. So I guess the question is whether it's worth trying to run these tests on such ports (i.e., whether any such port ever intends to ship visual viewports, which is a prerequisite for the Visual Viewport API). If not, we can remove these lines. The existing tests in fast/visual-viewport do have these lines though. For WPTs, we can't add this even if we wanted to (since we can't add WebKit internals calls to cross-browser tests).
Created attachment 327254 [details] Patch Rebased
Comment on attachment 327254 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=327254&action=review OK, thanks for the clarification regarding tests / settings. It looks like the win port does not build anymore :-( > Source/WebCore/page/VisualViewport.cpp:69 > + return 0.0; Sorry, I forgot about that one: https://webkit.org/code-style-guidelines/#floating-point-literals
Created attachment 327425 [details] Patch
(In reply to Frédéric Wang (:fredw) from comment #14) > Comment on attachment 327254 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=327254&action=review > > OK, thanks for the clarification regarding tests / settings. > > It looks like the win port does not build anymore :-( I'd missed a few win-specific files that needed to be modified when adding a new setting. Hopefully fixed now. > > Source/WebCore/page/VisualViewport.cpp:69 > > + return 0.0; > > Sorry, I forgot about that one: > https://webkit.org/code-style-guidelines/#floating-point-literals Thanks, fixed. Also, added a comment to VisualViewport::scale based on the response to https://github.com/WICG/visual-viewport/issues/52
Created attachment 327426 [details] Patch
Created attachment 327434 [details] Patch
Comment on attachment 327434 [details] Patch OK, we got the clarification from spec authors and no complaints from other reviewers, so I think we can take this.
Comment on attachment 327434 [details] Patch Clearing flags on attachment: 327434 Committed r225093: <https://trac.webkit.org/changeset/225093>
All reviewed patches have been landed. Closing bug.
<rdar://problem/35664217>
Re-opened since this is blocked by bug 179938
@Ali: I reverted the patch because compilation failed on WinCairo Release Build (for some reason the EWS was green). Can you please check it? Maybe we should ask to the Sony people. https://build.webkit.org/builders/WinCairo%2064-Bit%20Release/builds/8578/steps/compile-webkit/logs/stdio
(In reply to Frédéric Wang (:fredw) from comment #24) > @Ali: I reverted the patch because compilation failed on WinCairo Release > Build (for some reason the EWS was green). Can you please check it? Maybe we > should ask to the Sony people. > > https://build.webkit.org/builders/WinCairo%2064-Bit%20Release/builds/8578/ > steps/compile-webkit/logs/stdio I did a clean build with Ali's patch and I did not see the failure. I have a feeling that one of the generated files was stale. I'm ok with landing this again and then if necessary forcing a clean build on the WinCairo bots. I would assume the Apple ones might need this as well.
Created attachment 327470 [details] Patch for re-landing Rebased
Comment on attachment 327470 [details] Patch for re-landing Rejecting attachment 327470 [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-02', 'validate-changelog', '--check-oops', '--non-interactive', 327470, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Frederic Wang found in /Volumes/Data/EWS/WebKit/LayoutTests/ChangeLog does not appear to be a valid reviewer according to contributors.json. /Volumes/Data/EWS/WebKit/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://webkit-queues.webkit.org/results/5333089
Created attachment 327471 [details] Patch for re-landing Added accents to reviewer's name :)
Comment on attachment 327471 [details] Patch for re-landing Clearing flags on attachment: 327471 Committed r225103: <https://trac.webkit.org/changeset/225103>
I did a clean build for our WinCairo buildbots and that seems to have done the trick. The build is green after this patch landed.
(In reply to Don Olmstead from comment #31) > I did a clean build for our WinCairo buildbots and that seems to have done > the trick. The build is green after this patch landed. Thanks for taking care of this!
(In reply to Frédéric Wang (:fredw) from comment #32) > (In reply to Don Olmstead from comment #31) > > I did a clean build for our WinCairo buildbots and that seems to have done > > the trick. The build is green after this patch landed. > > Thanks for taking care of this! No problem! The patch was always fine it looks like there were stale files being included. I opened https://bugs.webkit.org/show_bug.cgi?id=179960 to figure out what happened.