Bug 179386

Created attachment 329944 [details] Patch

Comment on attachment 329944 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329944&action=review > Source/WebKit/ChangeLog:10 > + testing. I'm probably missing some context, but can you explain this change? Do the new APIs implemented here make this feature "ready for wider testing" or can this change be done independently? Was it discussed elsewhere with other people or are you just proposing to do it now with that review request? (I'm not familiar at all with the rules regarding experimental flags)

Comment on attachment 329944 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329944&action=review >> Source/WebKit/ChangeLog:10 >> + testing. > > I'm probably missing some context, but can you explain this change? Do the new APIs implemented here make this feature "ready for wider testing" or can this change be done independently? Was it discussed elsewhere with other people or are you just proposing to do it now with that review request? > > (I'm not familiar at all with the rules regarding experimental flags) With the new APIs implemented here, the implementation of the spec is complete. This hasn't been discussed elsewhere, I'm just proposing it here.

Comment on attachment 329944 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329944&action=review >>> Source/WebKit/ChangeLog:10 >>> + testing. >> >> I'm probably missing some context, but can you explain this change? Do the new APIs implemented here make this feature "ready for wider testing" or can this change be done independently? Was it discussed elsewhere with other people or are you just proposing to do it now with that review request? >> >> (I'm not familiar at all with the rules regarding experimental flags) > > With the new APIs implemented here, the implementation of the spec is complete. This hasn't been discussed elsewhere, I'm just proposing it here. OK, then maybe the ChangeLog should indicate that this patch finishes the implementation of the spec and so the feature is ready for wider testing.

Created attachment 330486 [details] Patch

Comment on attachment 329944 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329944&action=review >>>> Source/WebKit/ChangeLog:10 >>>> + testing. >>> >>> I'm probably missing some context, but can you explain this change? Do the new APIs implemented here make this feature "ready for wider testing" or can this change be done independently? Was it discussed elsewhere with other people or are you just proposing to do it now with that review request? >>> >>> (I'm not familiar at all with the rules regarding experimental flags) >> >> With the new APIs implemented here, the implementation of the spec is complete. This hasn't been discussed elsewhere, I'm just proposing it here. > > OK, then maybe the ChangeLog should indicate that this patch finishes the implementation of the spec and so the feature is ready for wider testing. Done.

Comment on attachment 330486 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=330486&action=review > Source/WebCore/ChangeLog:45 > + (WebCore::getFrameViewAndLayoutIfNonNull): Deleted. I believe WebKit people like when some explanation of per-function/file change is provided in the ChangeLog. > Source/WebCore/page/FrameView.cpp:1679 > + } So IIUC, this is triggered by the call to updateLayoutIgnorePendingStylesheets, right? So we are sure that the member variables are up-to-date when we call offsetLeft(), scale() etc. > Source/WebCore/page/VisualViewport.cpp:71 > ASSERT(frame->pageZoomFactor()); It looks like the ASSERT(frame->pageZoomFactor()) is no longer really necessary now. I guess you should just move it to update(). > Source/WebCore/page/VisualViewport.cpp:76 > } I think the helper function is really simplified now and I wonder whether it's still worth factoring out the "m_frame != nullptr" check. How about just adding a member function void VisualViewport::updateFrameLayout() { ASSERT(m_frame); m_frame->document()->updateLayoutIgnorePendingStylesheets(Document::RunPostLayoutTasks::Synchronously) } ? Then you could use it in visualViewport::scale() too. > Source/WebCore/page/VisualViewport.cpp:83 > + return m_offsetLeft; With the above suggestion this would become if (!m_frame) return 0.0; updateFrameLayout(); return m_offsetLeft; > Source/WebCore/page/VisualViewport.cpp:132 > + m_frame->document()->updateLayoutIgnorePendingStylesheets(Document::RunPostLayoutTasks::Synchronously); This could be just be updateFrameLayout(). > Source/WebCore/page/VisualViewport.cpp:145 > + I think you don't need the ".0" https://webkit.org/code-style-guidelines/#floating-point-literals > Source/WebCore/page/VisualViewport.cpp:148 > + if (view) { if (auto* view = m_frame->view()) > Source/WebCore/page/VisualViewport.cpp:151 > + auto pageZoomFactor = m_frame->pageZoomFactor(); Maybe move the ASSERT(pageZoomFactor) here? > Source/WebCore/page/VisualViewport.h:73 > + double m_scale { 1.0 }; I think you don't need the ".0" https://webkit.org/code-style-guidelines/#floating-point-literals > LayoutTests/ChangeLog:8 > + Add tests for Visual Viewport API events. Maybe repeat here that they check resize events (after pinch zoom and window resize) and scroll events.

Created attachment 330722 [details] Patch

Comment on attachment 330486 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=330486&action=review Thanks for the review! >> Source/WebCore/ChangeLog:45 >> + (WebCore::getFrameViewAndLayoutIfNonNull): Deleted. > > I believe WebKit people like when some explanation of per-function/file change is provided in the ChangeLog. Done. >> Source/WebCore/page/FrameView.cpp:1679 >> + } > > So IIUC, this is triggered by the call to updateLayoutIgnorePendingStylesheets, right? So we are sure that the member variables are up-to-date when we call offsetLeft(), scale() etc. Right. >> Source/WebCore/page/VisualViewport.cpp:71 >> ASSERT(frame->pageZoomFactor()); > > It looks like the > > ASSERT(frame->pageZoomFactor()) > > is no longer really necessary now. I guess you should just move it to update(). Done. >> Source/WebCore/page/VisualViewport.cpp:76 >> } > > I think the helper function is really simplified now and I wonder whether it's still worth factoring out the "m_frame != nullptr" check. > > How about just adding a member function > > void VisualViewport::updateFrameLayout() > { > ASSERT(m_frame); > m_frame->document()->updateLayoutIgnorePendingStylesheets(Document::RunPostLayoutTasks::Synchronously) > } > > ? Then you could use it in visualViewport::scale() too. Done. >> Source/WebCore/page/VisualViewport.cpp:132 >> + m_frame->document()->updateLayoutIgnorePendingStylesheets(Document::RunPostLayoutTasks::Synchronously); > > This could be just be updateFrameLayout(). Done. >> Source/WebCore/page/VisualViewport.cpp:145 >> + > > I think you don't need the ".0" > https://webkit.org/code-style-guidelines/#floating-point-literals Thanks, done. >> Source/WebCore/page/VisualViewport.cpp:148 >> + if (view) { > > if (auto* view = m_frame->view()) Done. >> Source/WebCore/page/VisualViewport.cpp:151 >> + auto pageZoomFactor = m_frame->pageZoomFactor(); > > Maybe move the ASSERT(pageZoomFactor) here? Done. >> Source/WebCore/page/VisualViewport.h:73 >> + double m_scale { 1.0 }; > > I think you don't need the ".0" > https://webkit.org/code-style-guidelines/#floating-point-literals Done. >> LayoutTests/ChangeLog:8 >> + Add tests for Visual Viewport API events. > > Maybe repeat here that they check resize events (after pinch zoom and window resize) and scroll events. Done.

Comment on attachment 330722 [details] Patch LGTM, thanks!

Comment on attachment 330722 [details] Patch Clearing flags on attachment: 330722 Committed r226622: <https://trac.webkit.org/changeset/226622>

All reviewed patches have been landed. Closing bug.

<rdar://problem/36372751>

@Ali: This test fails on Apple El Capitan Debug WK1: https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK1%20(Tests)/r226622%20(5688)/results.html