Bug 97773 - [Qt] Wait for the UI process before re-enabling rendering during page load
Summary: [Qt] Wait for the UI process before re-enabling rendering during page load
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jocelyn Turcotte
URL:
Keywords:
Depends on: 98045
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-27 05:23 PDT by Jocelyn Turcotte
Modified: 2012-10-01 10:00 PDT (History)
9 users (show)

See Also:


Attachments
Patch (17.68 KB, patch)
2012-09-27 05:31 PDT, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff
Patch (17.87 KB, patch)
2012-09-28 04:52 PDT, Jocelyn Turcotte
kenneth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jocelyn Turcotte 2012-09-27 05:23:11 PDT
[Qt] Wait for the UI process before re-enabling rendering during page load
Comment 1 Jocelyn Turcotte 2012-09-27 05:31:25 PDT
Created attachment 165979 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2012-09-27 07:53:48 PDT
Comment on attachment 165979 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165979&action=review

> Source/WebKit2/UIProcess/qt/WebPageProxyQt.cpp:127
> +void WebPageProxy::initialViewportReady()

viewportReadyForNewPage ? I think that is more clear.
Comment 3 Jocelyn Turcotte 2012-09-27 08:48:39 PDT
(In reply to comment #2)
> (From update of attachment 165979 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165979&action=review
> 
> > Source/WebKit2/UIProcess/qt/WebPageProxyQt.cpp:127
> > +void WebPageProxy::initialViewportReady()
> 
> viewportReadyForNewPage ? I think that is more clear.

I tried to find a good name for that one for a while but it's not easy.
What this signal means is essentially that the web process sent all page load related viewport messages and wants the ui process to confirm that it handled them and sent back the resulting visible rect. The name should also reflect that the web process will be locked until the commitInitialViewport message is sent back.

I'm not sure about NewPage, it sounds like it applies when a new WebPage is created.
Any other idea?
Comment 4 Kenneth Rohde Christiansen 2012-09-27 08:53:46 PDT
transitionToCommittedForNewPage is used elsewhere
Comment 5 Jocelyn Turcotte 2012-09-27 09:44:29 PDT
(In reply to comment #4)
> transitionToCommittedForNewPage is used elsewhere


"ForNewPage" is used as a contrast for "FromCachedFrame" and makes sense in the context of the frame loader, also because it starts with "transition". But I think that it's ambiguous in WebPage.

What about this:
viewportReadyForLoadedPage
commitViewportForLoadedPage
Comment 6 Kenneth Rohde Christiansen 2012-09-27 10:11:54 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > transitionToCommittedForNewPage is used elsewhere
> 
> 
> "ForNewPage" is used as a contrast for "FromCachedFrame" and makes sense in the context of the frame loader, also because it starts with "transition". But I think that it's ambiguous in WebPage.
> 
> What about this:
> viewportReadyForLoadedPage
> commitViewportForLoadedPage

does it have to be fully loaded? 

Maybe just newViewportReady? I guess it will be called if someone changes the viewport meta tag programmatically?
Comment 7 Jocelyn Turcotte 2012-09-27 10:18:05 PDT
> does it have to be fully loaded? 
> 
> Maybe just newViewportReady? I guess it will be called if someone changes the viewport meta tag programmatically?

initialViewportReady is sent by didCompletePageTransition which is called either by dispatchDidLayout or frameLoadCompleted, whichever comes first.

So it is sent instead of unfreezing the rendering directly after the first layout.
Comment 8 Kenneth Rohde Christiansen 2012-09-27 10:23:27 PDT
viewportReadyForPageTransition ?
Comment 9 Jocelyn Turcotte 2012-09-28 04:20:54 PDT
(In reply to comment #8)
> viewportReadyForPageTransition ?
commitViewportForPageTransition sounds like it's happening before the transition while it should be at the end.

I'd put it that way, what do you think:
pageTransitionViewportReady
commitPageTransitionViewport

/me crosses fingers
Comment 10 Kenneth Rohde Christiansen 2012-09-28 04:29:23 PDT
I thikn that is ok
Comment 11 Jocelyn Turcotte 2012-09-28 04:52:58 PDT
Created attachment 166210 [details]
Patch

s/initial/pageTransition/
Comment 12 Jocelyn Turcotte 2012-10-01 04:37:21 PDT
Committed r130029: <http://trac.webkit.org/changeset/130029>
Comment 13 Csaba Osztrogonác 2012-10-01 10:00:58 PDT
(In reply to comment #12)
> Committed r130029: <http://trac.webkit.org/changeset/130029>

It caused a regression - https://bugs.webkit.org/show_bug.cgi?id=98045
Could you check it, please?