[webkit-reviews] review granted: [Bug 90641] [Qt] QRawWebView should notify when rendering is done, so that pixel results can be grabbed at the appropriate moment. : [Attachment 151516] Patch

[bugzilla-daemon at webkit.org]

Jocelyn Turcotte <jocelyn.turcotte at nokia.com> has granted Noam Rosenthal
<noam.rosenthal at nokia.com>'s request for review:
Bug 90641: [Qt] QRawWebView should notify when rendering is done, so that pixel
results can be grabbed at the appropriate moment.
https://bugs.webkit.org/show_bug.cgi?id=90641

Attachment 151516: Patch
https://bugs.webkit.org/attachment.cgi?id=151516&action=review

------- Additional Comments from Jocelyn Turcotte <jocelyn.turcotte at nokia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=151516&action=review


r=me with one readability issue.

>
Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.cpp
:255
> +    if (!didSync)
> +	   return false;
> +
> +    if (!m_shouldSyncFrame)
> +	   return true;

This part hardly makes sense if you just look at the variable names.
I think it would read better as:
if (!m_shouldSyncFrame)
    return didSync;

And at the end, return didSync; instead of true (or remove the "didSync =
didSync || m_shouldSyncFrame;" line and keep the return true).

Else it's impossible to see that m_shouldSyncFrame is related to the
DidRenderFrame message.
You could also rename m_shouldSyncFrame to m_shouldSendDidRenderFrame or
something like this.