Bug 95179 – [BlackBerry] One shot drawing synchronization broken

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit BlackBerry

[Andrew Lo]

http://trac.webkit.org/changeset/126598 broke one shot drawing synchronization, which is a render + commit that happens without a blit in between.
This can cause something that moves from the backing store to an AC layer to be drawn twice or not drawn for a frame, resulting in flashing.

Reproducible on m.bgr.com, when you click the arrows to expand a story, the top bar flashes.

[Andrew Lo]

Created attachment 160938 [details]
Patch

[Andrew Lo]

Created attachment 160957 [details]
Patch

[Antonio Gomes]

Comment on attachment 160957 [details]
Patch

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

> Source/WebKit/blackberry/Api/BackingStore.cpp:2625
> +    if (!shouldDirectRenderingToWindow()) {
> +        if (!m_webPage->d->needsOneShotDrawingSynchronization())
> +            blitVisibleContents();
> +    } else
> +        invalidateWindow();

I was not expecting we do anything other than "notifying" from a notify-like method.

So this method should be renamed to ::doSomethingAndNotifyBlah

[Arvid Nilsson]

Some background:

One shot drawing sync (OSDS) happens when content transitions from being drawn to the BS, to being drawn to an AC layer instead, or vice versa.

The optimal order of performing these operations are:

render to BS
commit (render to AC layers, start animations)
blit

This is because commit internally does more rendering, starts animations, but you won't actually see the animations until the blit. This order guarantees minimum latency between starting animations and actually rendering the effects of the animations.

To make sure animations appear, we recently made a change so the commit operation contains a blit:

render to BS
commit (render to AC layers, start animations, blit)

The commit operation was placed in renderContents, before the rendering. And a BackingStorePrivate::renderJob() run could consist of several renderContents calls.

So you'd get this:

commit (render to AC layers, start animations, blit)
render to BS
render some more to BS
blit

This guaranteed that one shot drawing sync would flicker, because it blits immediately after a commit (when we've removed the layer from the scene graph) but before we've rendered the layer's contents to the BS. So the layer would be drawn exactly zero times if the OSDS was due to a layer being removed.

Similarly, it would draw a layer twice if the OSDS was due to a layer being added.

[Arvid Nilsson]

Comment on attachment 160957 [details]
Patch

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

>> Source/WebKit/blackberry/Api/BackingStore.cpp:2625
>> +        invalidateWindow();
> 
> I was not expecting we do anything other than "notifying" from a notify-like method.
> 
> So this method should be renamed to ::doSomethingAndNotifyBlah

So the idea is that the RenderQueue is notifying it's listener, the BackingStore, and that it's a callback, so the BackingStore should take appropriate action. Then, the BackingStore's appropriate action happens to be to blit and to in turn notify it's listener, which is the WebPageClient.

The naming and content of the method's implementation would make more sense if we introduced an interface "RenderQueueClient" and had teh BackingStore implement that interface, but that would just introduce a virtual method for no good reason.

[Antonio Gomes]

(In reply to comment #5)
> (From update of attachment 160957 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160957&action=review
> 
> >> Source/WebKit/blackberry/Api/BackingStore.cpp:2625
> >> +        invalidateWindow();
> > 
> > I was not expecting we do anything other than "notifying" from a notify-like method.
> > 
> > So this method should be renamed to ::doSomethingAndNotifyBlah
> 
> So the idea is that the RenderQueue is notifying it's listener, the BackingStore, and that it's a callback, so the BackingStore should take appropriate action. Then, the BackingStore's appropriate action happens to be to blit and to in turn notify it's listener, which is the WebPageClient.
> 
> The naming and content of the method's implementation would make more sense if we introduced an interface "RenderQueueClient" and had teh BackingStore implement that interface, but that would just introduce a virtual method for no good reason.

so it should be called didBleh?

[Arvid Nilsson]

Comment on attachment 160957 [details]
Patch

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

>>> Source/WebKit/blackberry/Api/BackingStore.cpp:2625
>>> +        invalidateWindow();
>> 
>> I was not expecting we do anything other than "notifying" from a notify-like method.
>> 
>> So this method should be renamed to ::doSomethingAndNotifyBlah
> 
> So the idea is that the RenderQueue is notifying it's listener, the BackingStore, and that it's a callback, so the BackingStore should take appropriate action. Then, the BackingStore's appropriate action happens to be to blit and to in turn notify it's listener, which is the WebPageClient.
> 
> The naming and content of the method's implementation would make more sense if we introduced an interface "RenderQueueClient" and had teh BackingStore implement that interface, but that would just introduce a virtual method for no good reason.

Antonio has a point that "notifyBlah" is not the regular naming convention for WebKit codestyle when naming callbacks. Callbacks are typically named "will/didBlah" and delegate methods are named "bool shouldBlah".
So, we established that conceptually, BackingStore is implementing "RenderQueueClient" interface, which has "virtual void RenderQueueClient::notifyContentRendered(const IntRect&)", but we devirtualized the whole thing and got rid of the interface before it even existed
But if RenderQueueClient really existed, the method should follow WebKit callback interface naming conventions and actually be called "didRenderContent(IntRect)" or "renderQueueDidRenderContent(RenderQueue*, IntRect)". Which one would you prefer?

[Arvid Nilsson]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 160957 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=160957&action=review
> > 
> > >> Source/WebKit/blackberry/Api/BackingStore.cpp:2625
> > >> +        invalidateWindow();
> > > 
> > > I was not expecting we do anything other than "notifying" from a notify-like method.
> > > 
> > > So this method should be renamed to ::doSomethingAndNotifyBlah
> > 
> > So the idea is that the RenderQueue is notifying it's listener, the BackingStore, and that it's a callback, so the BackingStore should take appropriate action. Then, the BackingStore's appropriate action happens to be to blit and to in turn notify it's listener, which is the WebPageClient.
> > 
> > The naming and content of the method's implementation would make more sense if we introduced an interface "RenderQueueClient" and had teh BackingStore implement that interface, but that would just introduce a virtual method for no good reason.
> 
> so it should be called didBleh?

Exactly, hehee

[Andrew Lo]

Created attachment 160978 [details]
Patch

[WebKit Review Bot]

Comment on attachment 160978 [details]
Patch

Clearing flags on attachment: 160978

Committed r126878: <http://trac.webkit.org/changeset/126878>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.