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)
Created attachment 131838 [details] Patch
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 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?
Created attachment 131846 [details] Patch
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 on attachment 131846 [details] Patch This looks fine to me. Kenneth, can you maybe have a look if it fixes your concerns?
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.
Aha, I found the problem! We never upstreamed webkit/acba1b713b82850221f0aecb6772a0893d5b2618 and webkit/86b38492db763381fd71b9444133408c3c819b7d!
Hmm, I was just a few hours too early, Nima is upstreaming those changes in Bug #81118
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 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 on attachment 131846 [details] Patch Let's try, Kenneth already r+ed this, therefore also setting r+.
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 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.
(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 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 on attachment 131846 [details] Patch Clearing flags on attachment: 131846 Committed r110806: <http://trac.webkit.org/changeset/110806>
All reviewed patches have been landed. Closing bug.