Bug 97773 – [Qt] Wait for the UI process before re-enabling rendering during page load

• Component: New Bugs
• Status: RESOLVED FIXED

[Jocelyn Turcotte]

[Qt] Wait for the UI process before re-enabling rendering during page load

[Jocelyn Turcotte]

Created attachment 165979 [details]
Patch

[Kenneth Rohde Christiansen]

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.

[Jocelyn Turcotte]

(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?

[Kenneth Rohde Christiansen]

transitionToCommittedForNewPage is used elsewhere

[Jocelyn Turcotte]

(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

[Kenneth Rohde Christiansen]

(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?

[Jocelyn Turcotte]

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

[Kenneth Rohde Christiansen]

viewportReadyForPageTransition ?

[Jocelyn Turcotte]

(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

[Kenneth Rohde Christiansen]

I thikn that is ok

[Jocelyn Turcotte]

Created attachment 166210 [details]
Patch

s/initial/pageTransition/

[Jocelyn Turcotte]

Committed r130029: <http://trac.webkit.org/changeset/130029>

[Csaba Osztrogonác]

(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?