[Webkit-unassigned] [Bug 92753] [Qt] Snowshoe desktop crashes when opening a new tab

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=92753





--- Comment #11 from Balazs Kelemen <kbalazs at webkit.org>  2012-08-08 00:48:37 PST ---
(In reply to comment #9)
> (From update of attachment 156946 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156946&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:-180
> > -    // This is necessary for running layout tests. Since in this case we are not waiting for a UIProcess to reply nicely.
> > -    // Instead we are just triggering forceRepaint. But we still want to have the scripted animation callbacks being executed.
> > -    syncDisplayState();
> > -
> 
> You'll need to keep this if you agree with the comment below.

But from now we will use forceSyncAsync for testing, and it does a syncDisplayState before sending back the reply (in performScheduledLayerFlush after the timer fires), so I think we can remove it from forceRepaint.

> 
> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:183
> > -    flushPendingLayerChanges();
> > +
> > +    performScheduledLayerFlush();
> 
> Please move the m_waitingForUIProcess check at the top of flushPendingLayerChange instead to make sure that we keep the behavior relatively to m_isSuspended and didPerformScheduledLayerFlush().
> It will also make sure that we check this variable in the same method that we reset it.
> 

Ok.

> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:192
> > +bool LayerTreeCoordinator::forceRepaintAsync(uint64_t callbackID)
> > +{
> > +    // We expect the UI process to not require a new repaint until the previous one has finished.
> > +    ASSERT(!m_forceRepaintAsyncCallbackID);
> > +    m_forceRepaintAsyncCallbackID = callbackID;
> > +    return true;
> >  }
> 
> You're missing a scheduleLayerFlush() in here somewhere, no?

Yes.

> Also, I would prefer safely doing this before assigning m_forceRepaintAsyncCallbackID and comment that this is just a clumsy safety measure instead of asserting:
> 
> if (m_forceRepaintAsyncCallbackID)
>     m_webPage->send(Messages::WebPageProxy::VoidCallback(m_forceRepaintAsyncCallbackID));

Sending back the message without actually performing the repaint? I don't think that makes sense.

> 
> > Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp:206
> > +    if (!m_layerTreeHost) {
> 
> Place this at the beginning, you don't want to invalidate and layout if you're not going to do anything with it here.

Yep.

> 
> > Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp:207
> > +        // FIXME: this could be implemented for the non composited path as well.
> 
> It's a FIXME that might never be fixed and we have no need for it now, I think it would be better to avoid it.

Ok.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.