Bug 23623 – Windowed Flash instances aren't captured when a WebView receives a WM_PRINTCLIENT message

• Component: Plug-ins
• Status: RESOLVED FIXED

[Adam Roben (:aroben)]

To reproduce:

1. Go to <http://homestarrunner.com/>
2. Drag a tab out of the Safari window

The Flash instance doesn't appear in the dragged-out tab mini-window. This is because Flash isn't responding to WM_PRINTCLIENT.

[Adam Roben (:aroben)]

<rdar://problem/6513921>

[Adam Roben (:aroben)]

Created attachment 27151 [details]
patch v1 + ChangeLog

[Darin Adler]

Comment on attachment 27151 [details]
patch v1 + ChangeLog

> +    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

[Adam Roben (:aroben)]

(In reply to comment #3)
> (From update of attachment 27151 [details] [review])
> > +    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.

I don't know if it's the best way.

> > +    *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.

The code is this way currently because that's how it was on fengyuan.com (though the original code used C-style casts). I'll try changing to casting 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.

It can be a non-member function if I pass in pointers to the HookedBeginPaint/HookedEndPaint functions (which, come to think of it, should start with lowercase letters). I'll make that change.

> > +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.

The reason I added "IntoContext" is because we already have a paint() function that takes a GraphicsContext that is called for both windowed and windowless plugins, but that normally doesn't do anything for windowed plugins. So I think "IntoContext" adds a little clarity, but maybe not very much.

> > +        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.

OK, I'll retitle it to "Windowed Flash instances aren't captured when a WebView receives a WM_PRINTCLIENT message"

> > -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.

I'm not sure how to avoid the boolean in this case without doing extra work every time we paint the area beneath a windowed plugin. I can switch to using an enum.

Thanks for the review!

[Adam Roben (:aroben)]

Landed in r40366