[webkit-reviews] review denied: [Bug 31856] Make WebKit build on Windows with ACCELERATED_COMPOSITING turned on. : [Attachment 44199] Same replacement patch with a better changelog message

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Dec 3 07:27:55 PST 2009


Adam Roben (aroben) <aroben at apple.com> has denied Chris Marrin
<cmarrin at apple.com>'s request for review:
Bug 31856: Make WebKit build on Windows with ACCELERATED_COMPOSITING turned on.
https://bugs.webkit.org/show_bug.cgi?id=31856

Attachment 44199: Same replacement patch with a better changelog message
https://bugs.webkit.org/attachment.cgi?id=44199&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 51618)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,47 @@
> +2009-12-02  Chris Marrin  <cmarrin at apple.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Delay load DLLs for accelerated compositing
> +	   https://bugs.webkit.org/show_bug.cgi?id=31856
> +	   
> +	   If the DLLs (d3d9 and QuartzCore). are not present it
> +	   turns off accelerated compositing and avoids calling 
> +	   any of the functions in the DLLs.
> +
> +	   * platform/graphics/win/WKCACFLayer.cpp:
> +	   (WebCore::WKCACFLayer::acceleratedCompositingAvailable):
> +	   (WebCore::displayInContext):
> +	   (WebCore::WKCACFLayer::WKCACFLayer):
> +	   (WebCore::WKCACFLayer::~WKCACFLayer):
> +	   (WebCore::WKCACFLayer::display):
> +	   (WebCore::WKCACFLayer::becomeRootLayerForContext):
> +	   (WebCore::WKCACFLayer::setNeedsCommit):
> +	   (WebCore::WKCACFLayer::addSublayer):
> +	   (WebCore::WKCACFLayer::insertSublayer):
> +	   (WebCore::WKCACFLayer::insertSublayerAboveLayer):
> +	   (WebCore::WKCACFLayer::insertSublayerBelowLayer):
> +	   (WebCore::WKCACFLayer::replaceSublayer):
> +	   (WebCore::WKCACFLayer::removeFromSuperlayer):
> +	   (WebCore::WKCACFLayer::removeSublayer):
> +	   (WebCore::WKCACFLayer::indexOfSublayer):
> +	   (WebCore::WKCACFLayer::ancestorOrSelfWithSuperlayer):
> +	   (WebCore::WKCACFLayer::setBounds):
> +	   (WebCore::WKCACFLayer::setFrame):
> +	   (WebCore::WKCACFLayer::rootLayer):
> +	   (WebCore::WKCACFLayer::removeAllSublayers):
> +	   (WebCore::WKCACFLayer::setSublayers):
> +	   (WebCore::WKCACFLayer::moveSublayers):
> +	   (WebCore::WKCACFLayer::superlayer):
> +	   (WebCore::WKCACFLayer::setNeedsDisplay):
> +	   * platform/graphics/win/WKCACFLayer.h:
> +	   (WebCore::WKCACFLayer::create):
> +	   (WebCore::WKCACFLayer::cfValue):
> +	   * platform/graphics/win/WKCACFLayerRenderer.cpp:
> +	   (WebCore::WKCACFLayerRenderer::create):
> +	   * platform/graphics/win/WKCACFLayerRenderer.h:
> +	   * rendering/RenderLayerBacking.cpp:

I think many of these functions are no longer affected by this patch. Maybe you
should regenerate the ChangeLog.

> +++ WebCore/platform/graphics/win/WKCACFLayer.cpp	(working copy)
> @@ -35,10 +35,37 @@
>  #include <QuartzCore/CACFContext.h>
>  #include <QuartzCore/CARender.h>
>  
> +#pragma comment(lib, "d3d9")
> +#pragma comment(lib, "d3dx9")
> +#pragma comment(lib, "QuartzCore")

It would be slightly more correct to only link against QuartzCore in this file,
since nothing in this file explicitly requires D3D. Then you would like against
D3D in WKCACFLayerRenderer.cpp. (Maybe someday in the Star Trek Future there
will be a CACF implementation that doesn't use D3D, and this source file had
better be ready for it!)

> +bool WKCACFLayer::acceleratedCompositingAvailable()
> +{
> +    static bool available;
> +    static bool tested;
> +
> +    if (tested)
> +	   return available;
> +
> +    tested = true;
> +    HMODULE library = LoadLibrary(TEXT("d3d9.dll"));
> +    if (!library)
> +	   return false;
> +
> +    FreeLibrary(library);
> +    library = LoadLibrary(TEXT("QuartzCore.dll"));
> +    if (!library)
> +	   return false;
> +
> +    FreeLibrary(library);
> +    available = true;
> +    return available;
> +}

I think this might be more appropriate in WKCACFLayerRenderer.

> +WKCACFLayerRenderer* WKCACFLayerRenderer::create()
> +{
> +    if (!WKCACFLayer::acceleratedCompositingAvailable())
> +	   return 0;
> +    return new WKCACFLayerRenderer;
> +}

This should return a PassOwnPtr<WKCACFLayerRenderer>.

> +	   This patch also changes the WKCACFLayerRenderer to be a pointer.
> +	   This allows me to have a create() method which will not create it
when
> +	   accelerated compositing is disabled because of missing DLLs. It 
> +	   avoids having to do so many checks. I also made WebViewWndProc 
> +	   a member function to allow several methods to be made protected,
which
> +	   allows me to avoid doing availability checks there as well.

I think you could get the same effect with many fewer changes by just making
WebViewWndProc a static member function and not adding the wndProc non-static
member function. Or you could keep WebViewWndProc a non-member and declare it
as a friend. I think it's good to keep this patch as small and focused as
possible. And I really like that you've minimized the number of
acceleratedCompositingAvailable() checks!

> +    , m_layerRenderer(0)

You won't need this initializer if you make m_layerRenderer an OwnPtr.

>  WebView::~WebView()
>  {
> +#if USE(ACCELERATED_COMPOSITING)
> +    setAcceleratedCompositing(false);
> +#endif

I don't think you'll need to do this if you make m_layerRenderer and OwnPtr.

> -	       if (SUCCEEDED(webView->uiDelegate(&uiDelegate)) && uiDelegate &&

> +	       if (SUCCEEDED(WebView::uiDelegate(&uiDelegate)) && uiDelegate &&


We normally write this as "this->uiDelegate(&uiDelegate)", but hopefully you'll
take my advice to revert these changes.

> +    if (accelerated) {
> +	   if (m_layerRenderer)
> +	       delete m_layerRenderer;

It's OK to try to delete 0. But if you make m_layerRenderer an OwnPtr you won't
have to do this at all.

> -    void setRootLayerNeedsDisplay() { m_layerRenderer.setNeedsDisplay(); }
> +    void setRootLayerNeedsDisplay() { if (m_layerRenderer)
m_layerRenderer->setNeedsDisplay(); }

Is it an error for this function to be called if m_layerRenderer is 0?

> -    WebCore::WKCACFLayerRenderer m_layerRenderer;
> +    WebCore::WKCACFLayerRenderer* m_layerRenderer;

You should make this an OwnPtr (if that wasn't clear by now :-)).

r- so we can reduce the number of changes to WebView.cpp (which I think will
also get the style-queue to stop complaining).


More information about the webkit-reviews mailing list