Bug 91644 – [BlackBerry] Allow nested suspend/resume screen & backingstore calls

• Component: WebKit BlackBerry
• Status: RESOLVED FIXED

[Jakob Petsovits]

From the commit message (patch below):

We expose suspendScreenAndBackingStoreUpdates() to the outside API, but also use it internally when reacting to a number of happenings, i.e. zooming, viewport resize, resetting view state on Committed state or when restoring it from previous pages, etc.

These two can clash. For instance, if we get a suspend call from outside that suspends us for app inactivity, or we are told to suspend because the main target surface is not available at the time, and while being suspended we try to rotate, finish loading a page, the we'll end up resuming operations even though we shouldn't.

This patch changes the suspend flag to be a counter instead, allowing nested suspend/resume calls and making suspend/resume more robust this way. It also changes several call sites to make sure suspend/resume calls are paired up correctly.

[Jakob Petsovits]

Created attachment 153060 [details]
Patch

[Adam Treat]

Comment on attachment 153060 [details]
Patch

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

See comments.

> Source/WebKit/blackberry/Api/BackingStore.cpp:286
> +        return;

Uhm, you are returning here... not increasing the counter.  What gives?

> Source/WebKit/blackberry/Api/BackingStore.cpp:313
> +            "Call mismatch: Backingstore hasn't been suspended, therefore won't resume!");

How about: "Call mismatch: Screen and backingstore haven't been suspended, therefore won't resume!"

> Source/WebKit/blackberry/Api/BackingStore_p.h:339
> +    int m_suspendBackingStoreUpdates;

unsigned int here?

[Jakob Petsovits]

Created attachment 153066 [details]
Better & smaller patch

This one incorporates Adam's suggestions (good catch for the bad return) and simplifies the patch by not changing the zoomAboutPoint() signature, instead putting resume calls after zoomAboutPoint() in the two cases where we would before use that method to resume implicitly. This makes the zoomChanged() notification come before the render/blit, but nothing says that zoomChanged() needs to come after the target surface has been updated so that should be ok.

[Jakob Petsovits]

Comment on attachment 153066 [details]
Better & smaller patch

Tested and still works, setting cq? flag.

[Antonio Gomes]

Comment on attachment 153066 [details]
Better & smaller patch

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

I need this patch! nice work.

> Source/WebKit/blackberry/Api/BackingStore.cpp:310
> +    if (m_suspendScreenUpdates <= 0 || m_suspendBackingStoreUpdates <= 0) {

i would assert negative values instead. they should not happen

> Source/WebKit/blackberry/Api/BackingStore_p.h:339
> +    unsigned int m_suspendScreenUpdates;
> +    unsigned int m_suspendBackingStoreUpdates;

nit: unsigned is enough

[Jakob Petsovits]

Created attachment 153265 [details]
Patch

[Jakob Petsovits]

Comment on attachment 153265 [details]
Patch

This one modifies the assertion/early-return things to use boolean conditions rather than greater-than/less-than operators. After all, with the values now being unsigned, we can never be less than 0. Incorporates the nitpick of leaving out "int" from "unsigned int". Antonio, we do assert three lines above the line that you mentioned. I thought it's still a good idea to leave in the warning, because it doesn't hurt and makes it easier if we get a bug report from somebody who's been running in release mode.

[Antonio Gomes]

Comment on attachment 153265 [details]
Patch

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

> Source/WebKit/blackberry/Api/BackingStore.cpp:319
> +    // Out of all nested resume calls, resume with the maximum-impact operation.
> +    if (op == BackingStore::RenderAndBlit
> +        || (m_resumeOperation == BackingStore::None && op == BackingStore::Blit))
> +        m_resumeOperation = op;

that would read better if we could have the values in the enum in precedence order (i.e. None, blit, RenderAndBlit), and compare them with ">" (even if a static_cast to int was needed).

> Source/WebKit/blackberry/Api/BackingStore.cpp:329
> +    --m_suspendBackingStoreUpdates;

can not we decrease the --m_suspendScreenUpdates right below this like here as well, since it is going to happen in both code paths below anyways?

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:1071
> -        m_webPagePrivate->m_backingStore->d->renderVisibleContents();
> +    bool didZoom = m_webPagePrivate->zoomAboutPoint(scale, m_frame->view()->scrollPosition(), true /* enforceScaleClamping */, true /*forceRendering*/, true /*isRestoringZoomLevel*/);
> +    // If we're already at that scale, then we should still force rendering
> +    // since our scroll position changed.
> +    m_webPagePrivate->m_backingStore->d->resumeScreenAndBackingStoreUpdates(BackingStore::RenderAndBlit);

is this semantically the same now? Could you explain this piece of the change?

[Jakob Petsovits]

(In reply to comment #8)
> (From update of attachment 153265 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153265&action=review
> 
> > Source/WebKit/blackberry/Api/BackingStore.cpp:319
> > +    // Out of all nested resume calls, resume with the maximum-impact operation.
> > +    if (op == BackingStore::RenderAndBlit
> > +        || (m_resumeOperation == BackingStore::None && op == BackingStore::Blit))
> > +        m_resumeOperation = op;
> 
> that would read better if we could have the values in the enum in precedence order (i.e. None, blit, RenderAndBlit), and compare them with ">" (even if a static_cast to int was needed).

I think it would look cleaner, but I'm not sure if it also conveys the meaning as nicely. You'd have to look into the header file to make the connection. I'm on the fence about this, if you've got a strong opinion then I'll change it but I'm not sure it makes too much of a positive impact.

> > Source/WebKit/blackberry/Api/BackingStore.cpp:329
> > +    --m_suspendBackingStoreUpdates;
> 
> can not we decrease the --m_suspendScreenUpdates right below this like here as well, since it is going to happen in both code paths below anyways?

Err, no. The values for both are always the same, only the timing of setting them is different because of synchronization with the UI thread (without using mutexes to block). Adam can tell you more about this as he wrote that code; however, the reason why we have to values in the first place is because we can't assign them in the same place or we'd have to do something else to account for proper synchronization. I actually have a question for Adam about this, but anyways if we change this then it should be part of a different bug/patch.

> > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:1071
> > -        m_webPagePrivate->m_backingStore->d->renderVisibleContents();
> > +    bool didZoom = m_webPagePrivate->zoomAboutPoint(scale, m_frame->view()->scrollPosition(), true /* enforceScaleClamping */, true /*forceRendering*/, true /*isRestoringZoomLevel*/);
> > +    // If we're already at that scale, then we should still force rendering
> > +    // since our scroll position changed.
> > +    m_webPagePrivate->m_backingStore->d->resumeScreenAndBackingStoreUpdates(BackingStore::RenderAndBlit);
> 
> is this semantically the same now? Could you explain this piece of the change?

Yes, it's semantically the same. What's happening here is that this method calls suspendScreenAndBackingStoreUpdates() further up, and then relied on zoomAboutPoint() to resume it. This version of the patch (as opposed to the first attempt, see obsolete patches) removes any asymmetry from zoomAboutPoint(), it now always calls one suspend and one resume, or none of both.

In the context of restoreViewState(), that means the suspend and resume calls in zoomAboutPoint() are going to be nested within the suspend and resume calls in restoreViewState(). I'm adding the resume call to restoreViewState() to pair it up properly, replacing the "Will restore updates to backingstore guaranteed!" comment with an actual unconditional resume call. The only semantic difference that this brings is that the zoomChanged() notification for the WebPageClient now occurs before we resume rendering/blitting operations, but zoomChanged() is not meant to tell about any blits (it only notifies about the state change itself) so that should be ok.

The call in zoomAboutPointTimerFired() has the same change. All other calls did not previously suspend the backingstore, so we don't have to resume afterwards. Anyways, making this resume call explicit on the caller side avoids adding a "resumeScreenAndBackingStoreUpdates" bool argument to zoomAboutPoint() and I think adds to readability too.

[Jakob Petsovits]

Created attachment 153291 [details]
Patch as r+ed above, with both Adam and Antonio listed as reviewers

[Jakob Petsovits]

Created attachment 153292 [details]
Patch as r+ed above, with both Adam and Antonio *actually* listed as reviewers

[WebKit Review Bot]

Comment on attachment 153292 [details]
Patch as r+ed above, with both Adam and Antonio *actually* listed as reviewers

Clearing flags on attachment: 153292

Committed r123129: <http://trac.webkit.org/changeset/123129>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.