Bug 72511 – Make use-fixed-layout work reliable

• Component: WebKit2
• Status: RESOLVED FIXED

[Kenneth Rohde Christiansen]

SSIA

[Kenneth Rohde Christiansen]

Created attachment 115388 [details]
Patch

[Kenneth Rohde Christiansen]

Created attachment 115390 [details]
Patch (bug fix)

[Antonio Gomes]

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?

[alan]

-    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.

[Simon Hausmann]

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.

[Kenneth Rohde Christiansen]

Comment on attachment 115390 [details]
Patch (bug fix)

Landed in r100594

[Philippe Normand]

(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

[Kenneth Rohde Christiansen]

Build fix committed in r100595