SSIA
Created attachment 115388 [details] Patch
Created attachment 115390 [details] Patch (bug fix)
Comment on attachment 115390 [details] Patch (bug fix) View in context: https://bugs.webkit.org/attachment.cgi?id=115390&action=review > Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1144 > + m_frame->coreFrame()->createView(webPage->size(), backgroundColor, false, IntSize(), isMainFrame && webPage->useFixedLayout()); maybe add a /* XXX */ comment to make explicit what this boolean is about? > Source/WebKit2/WebProcess/WebPage/WebPage.h:266 > + bool useFixedLayout() const { return m_useFixedLayout; }; would usesFixedLayout or shouldUseFixedLayout read better?
- if (!mainFrame() || !mainFrame()->document() || mainFrame()-> document()->viewportArguments() == m_viewportArguments) + if (!mainFrame() || !mainFrame()->document()) return; I was wondering whether there won't be any performance penalty of getting multiple (false) viewport updates (setDocument, body inserted), but they all come before any layout happens, so i guess it's safe. We should still revisit it at some point to check whether there's a less intrusive way of doing it. + bool useFixedLayout() const { return m_useFixedLayout; }; redundant semicolon at the end. looks good.
Comment on attachment 115390 [details] Patch (bug fix) View in context: https://bugs.webkit.org/attachment.cgi?id=115390&action=review r=me, but needs typo and comment fixes before landing :) > Source/WebKit2/ChangeLog:13 > + as that wouldn't work in cases, such as when the web process has > + crashes. "has crashes" -> "has crashed" or simply "crashes" without has :) >> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1144 >> + m_frame->coreFrame()->createView(webPage->size(), backgroundColor, false, IntSize(), isMainFrame && webPage->useFixedLayout()); > > maybe add a /* XXX */ comment to make explicit what this boolean is about? I agree with Antonio, it would be nice to fix that when landing. >> Source/WebKit2/WebProcess/WebPage/WebPage.h:266 >> + bool useFixedLayout() const { return m_useFixedLayout; }; > > would usesFixedLayout or shouldUseFixedLayout read better? useFixedLayout() is the same name that's also used for example in ScrollView.h. I guess for the moment it's better to be consistent here.
Comment on attachment 115390 [details] Patch (bug fix) Landed in r100594
(In reply to comment #6) > (From update of attachment 115390 [details]) > Landed in r100594 Broke GTK: ../../Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp: In member function ‘virtual void WebKit::WebFrameLoaderClient::transitionToCommittedForNewPage()’: ../../Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1148: error: ‘isMainFrame’ was not declared in this scope make[1]: *** [Source/WebKit2/WebProcess/WebCoreSupport/libwebkit2gtk_3_0_la-WebFrameLoaderClient.lo] Error 1
Build fix committed in r100595