[webkit-reviews] review granted: [Bug 81099] [BlackBerry] Make sure WebPage and BackingStore don't crash without a Window : [Attachment 131838] Patch

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has granted Arvid Nilsson
<anilsson at rim.com>'s request for review:
Bug 81099: [BlackBerry] Make sure WebPage and BackingStore don't crash without
a Window
https://bugs.webkit.org/show_bug.cgi?id=81099

Attachment 131838: Patch
https://bugs.webkit.org/attachment.cgi?id=131838&action=review

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
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