Bug 72511 - Make use-fixed-layout work reliable
Summary: Make use-fixed-layout work reliable
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kenneth Rohde Christiansen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-16 08:44 PST by Kenneth Rohde Christiansen
Modified: 2011-11-17 03:58 PST (History)
4 users (show)

See Also:


Attachments
Patch (8.97 KB, patch)
2011-11-16 08:50 PST, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Patch (bug fix) (deleted)
2011-11-16 08:57 PST, Kenneth Rohde Christiansen
hausmann: review+
hausmann: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Rohde Christiansen 2011-11-16 08:44:00 PST
SSIA
Comment 1 Kenneth Rohde Christiansen 2011-11-16 08:50:45 PST
Created attachment 115388 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2011-11-16 08:57:08 PST
Created attachment 115390 [details]
Patch (bug fix)
Comment 3 Antonio Gomes 2011-11-16 10:49:19 PST
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?
Comment 4 zalan 2011-11-16 11:05:58 PST
-    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 5 Simon Hausmann 2011-11-17 02:50:14 PST
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 6 Kenneth Rohde Christiansen 2011-11-17 03:27:00 PST
Comment on attachment 115390 [details]
Patch (bug fix)

Landed in r100594
Comment 7 Philippe Normand 2011-11-17 03:49:56 PST
(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
Comment 8 Kenneth Rohde Christiansen 2011-11-17 03:58:00 PST
Build fix committed in r100595