[webkit-reviews] review granted: [Bug 23623] Windowed Flash instances don't respond to WM_PRINTCLIENT : [Attachment 27151] patch v1 + ChangeLog

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Adam Roben (aroben)
<aroben at apple.com>'s request for review:
Bug 23623: Windowed Flash instances don't respond to WM_PRINTCLIENT
https://bugs.webkit.org/show_bug.cgi?id=23623

Attachment 27151: patch v1 + ChangeLog
https://bugs.webkit.org/attachment.cgi?id=27151&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    sysCallID = *reinterpret_cast<unsigned*>(pProc + 1);

This is an unaligned read. Is this the best way to do this? In other cases in
the past I've done this sort of thing by fetching the bytes individually and
shifting them into place. On some platforms that was required.

> +    *reinterpret_cast<unsigned*>(pProc + 1) =
reinterpret_cast<unsigned>(pNewProc) - reinterpret_cast<unsigned>(pProc + 5);

Why do we need to cast pNewProc to an integer? The difference between two
pointers is an integer, so normally I'd expect the variables to be cast to a
pointer type. Or to intptr_t.

> +void PluginView::setUpOffscreenPaintingHooks()

Can this be a non-member function instead? I'd prefer to see it not mentioned
in the header file at all.

> +void PluginView::paintWindowedPluginIntoContext(GraphicsContext* context,
const IntRect& rect) const

I don't think the words "IntoContext" add much in the name of this function.
The fact that the function takes a context seems to be enough.

> +	   Fix Bug 23623: Windowed Flash instances don't respond to
> +	   WM_PRINTCLIENT

I think the name of the bug is wrong. This is describing what's wrong with the
plug-in, not what effects this has on WebKit clients nor what we're changing
about WebKit. The title of the bug will still be true after it's fixed.

> -void WebView::updateBackingStore(FrameView* frameView, HDC dc, bool
backingStoreCompletelyDirty)
> +void WebView::updateBackingStore(FrameView* frameView, HDC dc, bool
backingStoreCompletelyDirty, bool shouldIncludeChildWindows)

Booleans stink for things like this! I much prefer enums or avoiding the
boolean entirely.

r=me