Bug 81099 - [BlackBerry] Make sure WebPage and BackingStore don't crash without a Window
Summary: [BlackBerry] Make sure WebPage and BackingStore don't crash without a Window
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Arvid Nilsson
URL:
Keywords:
Depends on:
Blocks: 81108
  Show dependency treegraph
 
Reported: 2012-03-14 06:18 PDT by Arvid Nilsson
Modified: 2012-03-16 01:22 PDT (History)
5 users (show)

See Also:


Attachments
Patch (23.63 KB, patch)
2012-03-14 07:22 PDT, Arvid Nilsson
no flags Details | Formatted Diff | Diff
Patch (23.60 KB, patch)
2012-03-14 07:55 PDT, Arvid Nilsson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Arvid Nilsson 2012-03-14 06:18:41 PDT
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)
Comment 1 Arvid Nilsson 2012-03-14 07:22:40 PDT
Created attachment 131838 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2012-03-14 07:28:33 PDT
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
Comment 3 Rob Buis 2012-03-14 07:35:41 PDT
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?
Comment 4 Arvid Nilsson 2012-03-14 07:55:08 PDT
Created attachment 131846 [details]
Patch
Comment 5 Arvid Nilsson 2012-03-14 07:56:30 PDT
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.
Comment 6 Rob Buis 2012-03-14 08:37:32 PDT
Comment on attachment 131846 [details]
Patch

This looks fine to me. Kenneth, can you maybe have a look if it fixes your concerns?
Comment 7 Arvid Nilsson 2012-03-14 10:10:44 PDT
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.
Comment 8 Arvid Nilsson 2012-03-14 14:33:18 PDT
Aha, I found the problem! We never upstreamed webkit/acba1b713b82850221f0aecb6772a0893d5b2618 and webkit/86b38492db763381fd71b9444133408c3c819b7d!
Comment 9 Arvid Nilsson 2012-03-14 14:35:32 PDT
Hmm, I was just a few hours too early, Nima is upstreaming those changes in Bug #81118
Comment 10 Rob Buis 2012-03-14 14:39:47 PDT
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.
Comment 11 Arvid Nilsson 2012-03-14 15:00:08 PDT
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.
Comment 12 Rob Buis 2012-03-14 15:03:11 PDT
Comment on attachment 131846 [details]
Patch

Let's try, Kenneth already r+ed this, therefore also setting r+.
Comment 13 Arvid Nilsson 2012-03-14 15:07:27 PDT
Thanks a million Rob, this is the most important patch in the chain, it fixes a crash that's gating an internal integration project =)
Comment 14 Antonio Gomes 2012-03-14 15:09:31 PDT
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.
Comment 15 Arvid Nilsson 2012-03-14 15:20:56 PDT
(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 =)
Comment 16 WebKit Review Bot 2012-03-14 16:53:57 PDT
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
Comment 17 WebKit Review Bot 2012-03-14 18:48:39 PDT
Comment on attachment 131846 [details]
Patch

Clearing flags on attachment: 131846

Committed r110806: <http://trac.webkit.org/changeset/110806>
Comment 18 WebKit Review Bot 2012-03-14 18:48:46 PDT
All reviewed patches have been landed.  Closing bug.