[webkit-reviews] review granted: [Bug 22582] Need API to generate images from nodes on Windows : [Attachment 25648] Implement renderedImage for Windows

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted Steve Falkenburg
<sfalken at apple.com>'s request for review:
Bug 22582: Need API to generate images from nodes on Windows
https://bugs.webkit.org/show_bug.cgi?id=22582

Attachment 25648: Implement renderedImage for Windows
https://bugs.webkit.org/attachment.cgi?id=25648&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,18 @@
> +2008-12-01  Steve Falkenburg  <sfalken at apple.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   WARNING: NO TEST CASES ADDED OR CHANGED
> +
> +	   * page/Frame.h:
> +	   * page/win/FrameCGWin.cpp:
> +	   (WebCore::imageFromRect):
> +	   (WebCore::imageFromSelection):
> +	   (WebCore::Frame::nodeImage):
> +	   * page/win/FrameCairoWin.cpp:
> +	   (WebCore::imageFromNode):
> +	   * page/win/FrameWin.h:

You should reference this bug in your ChangeLog, and add some specific comments
about the functions you changed/added.

> +#if PLATFORM(WIN)
> +
> +public:
> +    HBITMAP nodeImage(Node*) const;

Someday it would be nice for nodeImage to return a WebCore::Image and for the
implementations to be shared. Maybe you could add a FIXME?

> +HBITMAP Frame::nodeImage(Node* node) const
> +{
> +    RenderObject* renderer = node->renderer();
> +    if (!renderer)
> +	   return 0;
> +
> +    IntRect topLevelRect;
> +    IntRect paintingRect = renderer->paintingRootRect(topLevelRect);
> +
> +    d->m_view->setNodeToDraw(node); // invoke special sub-tree drawing mode
> +    HBITMAP result = imageFromRect(this, paintingRect);
> +    d->m_view->setNodeToDraw(0);
> +
> +    return result;
>  }

I think we should call updateLayout() before calling imageFromRect, to match
the FrameMac implementation. Maybe imageFromRect should be the one to call
updateLayout?

> +++ WebCore/page/win/FrameWin.h	(working copy)
> @@ -34,6 +34,7 @@ typedef struct HBITMAP__* HBITMAP;
>  namespace WebCore {
>  
>      HBITMAP imageFromSelection(Frame* frame, bool forceWhiteText);
> +    HBITMAP imageFromNode(Frame*, Node*);

I don't think you need this declaration.

> +++ WebKit/win/ChangeLog	(working copy)
> @@ -1,3 +1,12 @@
> +2008-12-01  Steve Falkenburg  <sfalken at apple.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   * DOMCoreClasses.cpp:
> +	   (DOMElement::renderedImage):
> +	   * DOMCoreClasses.h:
> +	   * Interfaces/DOMPrivate.idl:

Same comments about the ChangeLog here as in WebCore.

> +HRESULT STDMETHODCALLTYPE DOMElement::renderedImage(HBITMAP* image)
> +{
> +    if (!image) {
> +	   ASSERT_NOT_REACHED();
> +	   return E_POINTER;
> +    }
> +    *image = 0;
> +
> +    ASSERT(m_element);
> +
> +    WebCore::Frame* frame = m_element->document()->frame();

We shouldn't be using the WebCore:: prefix in .cpp files.

r=me