Summary: | Make use-fixed-layout work reliable | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kenneth Rohde Christiansen <kenneth> | ||||||
Component: | WebKit2 | Assignee: | Kenneth Rohde Christiansen <kenneth> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | hausmann, pnormand, zalan, zeno | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Kenneth Rohde Christiansen
2011-11-16 08:44:00 PST
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 |