RESOLVED FIXED Bug 81099
[BlackBerry] Make sure WebPage and BackingStore don't crash without a Window
https://bugs.webkit.org/show_bug.cgi?id=81099
Summary [BlackBerry] Make sure WebPage and BackingStore don't crash without a Window
Arvid Nilsson
Reported 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)
Attachments
Patch (23.63 KB, patch)
2012-03-14 07:22 PDT, Arvid Nilsson
no flags
Patch (23.60 KB, patch)
2012-03-14 07:55 PDT, Arvid Nilsson
no flags
Arvid Nilsson
Comment 1 2012-03-14 07:22:40 PDT
Kenneth Rohde Christiansen
Comment 2 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
Rob Buis
Comment 3 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?
Arvid Nilsson
Comment 4 2012-03-14 07:55:08 PDT
Arvid Nilsson
Comment 5 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.
Rob Buis
Comment 6 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?
Arvid Nilsson
Comment 7 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.
Arvid Nilsson
Comment 8 2012-03-14 14:33:18 PDT
Aha, I found the problem! We never upstreamed webkit/acba1b713b82850221f0aecb6772a0893d5b2618 and webkit/86b38492db763381fd71b9444133408c3c819b7d!
Arvid Nilsson
Comment 9 2012-03-14 14:35:32 PDT
Hmm, I was just a few hours too early, Nima is upstreaming those changes in Bug #81118
Rob Buis
Comment 10 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.
Arvid Nilsson
Comment 11 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.
Rob Buis
Comment 12 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+.
Arvid Nilsson
Comment 13 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 =)
Antonio Gomes
Comment 14 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.
Arvid Nilsson
Comment 15 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 =)
WebKit Review Bot
Comment 16 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
WebKit Review Bot
Comment 17 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>
WebKit Review Bot
Comment 18 2012-03-14 18:48:46 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.