WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(23.60 KB, patch)
2012-03-14 07:55 PDT
,
Arvid Nilsson
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Arvid Nilsson
Comment 1
2012-03-14 07:22:40 PDT
Created
attachment 131838
[details]
Patch
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
Created
attachment 131846
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug