Bug 81099 – [BlackBerry] Make sure WebPage and BackingStore don't crash without a Window

• Component: WebKit BlackBerry
• Status: RESOLVED FIXED

[Arvid Nilsson]

This is part of the refactoring to allow OpenGL based rendering of the WebPage, using any kind of OpenGL context (not necessarily tied to an EGL window surface)

[Arvid Nilsson]

Created attachment 131838 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 131838 [details]
Patch

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

> Source/WebKit/blackberry/Api/BackingStore.cpp:252
> +bool BackingStorePrivate::isOpenGLCompositing() const

isCompositingUsingOpenGL ?

> Source/WebKit/blackberry/Api/BackingStore.cpp:1160
> -    m_webPage->client()->window()->copyFromFrontToBack(previousContentsRegion);
> +    if (Window* window = m_webPage->client()->window())
> +        window->copyFromFrontToBack(previousContentsRegion);
>      windowBackBufferState()->addBlittedRegion(previousContentsRegion);

So this is the actual fix? It would be nicer to split patches up, like to do the isOpenGLCompositing() part before this one. At least think about that for the future

> Source/WebKit/blackberry/Api/BackingStore.cpp:2377
> -    m_webPage->client()->window()->post(dstRect);
> +    if (Window* window = m_webPage->client()->window())
> +        window->post(dstRect);

Maybe you should write in the changelog why this doesn't always exist and has to be checked

> Source/WebKit/blackberry/Api/BackingStore_p.h:110
> +    // BackingStore tiles. This can be due to the main window using

I would have written "backing store tiles", ie used more natural language. This makes sure that the comment is still valid after a rename

> Source/WebKit/blackberry/Api/BackingStore_p.h:328
>      static WebPage* currentBackingStoreOwner() { return BackingStorePrivate::s_currentBackingStoreOwner; }
>      bool isActive() const;
>  
> +    // Surface abstraction, maybe BlackBerry::Platform::Graphics::Buffer could
> +    // be made public instead.

Looking at the line length above it might make sense to make this one line

[Rob Buis]

Comment on attachment 131838 [details]
Patch

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

Looks good, maybe you can answer the prefix question before r+.

> Source/WebKit/blackberry/WebKitSupport/GLES2Context.cpp:78
> +        if (window->windowUsage() == Platform::Graphics::Window::GLES2Usage)

Is the full prefix needed, since you added the using above?

[Arvid Nilsson]

Created attachment 131846 [details]
Patch

[Arvid Nilsson]

Alright, here's a new one. In hindsight, isOpenGLCompositing rename should have been a different patch, I'll try to do more atomic stuff in the future.

[Rob Buis]

Comment on attachment 131846 [details]
Patch

This looks fine to me. Kenneth, can you maybe have a look if it fixes your concerns?

[Arvid Nilsson]

I'll have to try again tomorrow, the patch is not coming out right... I must be preparing it for upstream submission the wrong way.

[Arvid Nilsson]

Aha, I found the problem! We never upstreamed webkit/acba1b713b82850221f0aecb6772a0893d5b2618 and webkit/86b38492db763381fd71b9444133408c3c819b7d!

[Arvid Nilsson]

Hmm, I was just a few hours too early, Nima is upstreaming those changes in Bug #81118

[Rob Buis]

Hi Arvid,

(In reply to comment #9)
> Hmm, I was just a few hours too early, Nima is upstreaming those changes in Bug #81118

Exactly, unfortunately it is slow going in, #9 in the queue.
Cheers,

Rob.

[Arvid Nilsson]

Comment on attachment 131846 [details]
Patch

Alright, I'm adding back the r? flag, this patch should be unaffected by whether #81118 is in or not, only #81108 really depends on it.

[Rob Buis]

Comment on attachment 131846 [details]
Patch

Let's try, Kenneth already r+ed this, therefore also setting r+.

[Arvid Nilsson]

Thanks a million Rob, this is the most important patch in the chain, it fixes a crash that's gating an internal integration project =)

[Antonio Gomes]

Comment on attachment 131846 [details]
Patch

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

> Source/WebKit/blackberry/WebKitSupport/GLES2Context.cpp:78
> +        if (window->windowUsage() == Window::GLES2Usage)

I think I would have used m_backingStore->d->isOpenGLCompositing() here too.

[Arvid Nilsson]

(In reply to comment #14)
> (From update of attachment 131846 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131846&action=review
> 
> > Source/WebKit/blackberry/WebKitSupport/GLES2Context.cpp:78
> > +        if (window->windowUsage() == Window::GLES2Usage)
> 
> I think I would have used m_backingStore->d->isOpenGLCompositing() here too.

Good point, I think the problem was that the GLES2Context is no friend of the BackingStore... The BackingStore still contains a now obsolete "friend class WebCore::GLES2Context". A separate patch could correct all that =)

[WebKit Review Bot]

Comment on attachment 131846 [details]
Patch

Rejecting attachment 131846 [details] from commit-queue.

New failing tests:
svg/repaint/repainting-after-animation-element-removal.svg
Full output: http://queues.webkit.org/results/11955332

[WebKit Review Bot]

Comment on attachment 131846 [details]
Patch

Clearing flags on attachment: 131846

Committed r110806: <http://trac.webkit.org/changeset/110806>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.